Re: [PATCHv4][PING][BZ #17833] _dl_close_worker() does not release inconsistent objects.

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, May 27, 2015 at 6:36 AM, Yury Gribov <y.gribov@samsung.com> wrote:
> On 05/27/2015 04:23 PM, H.J. Lu wrote:
>>
>> On Wed, May 27, 2015 at 6:12 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>>>
>>> On 05/27/2015 04:04 PM, H.J. Lu wrote:
>>>>
>>>>
>>>> On Wed, May 27, 2015 at 5:17 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On 05/13/2015 02:40 PM, Pavel Kopyl wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 04/24/2015 02:47 PM, H.J. Lu wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Apr 24, 2015 at 3:33 AM, Pavel Kopyl <p.kopyl@samsung.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 03/03/2015 12:13 PM, Pavel Kopyl wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 03/01/2015 10:17 PM, Mike Frysinger wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 27 Feb 2015 16:32, Pavel Kopyl wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/elf/tst-unique5lib.cc
>>>>>>>>>>> @@ -0,0 +1,13 @@
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> i know existing tests are bad examples, but lets try and start
>>>>>>>>>> fixing
>>>>>>>>>> that.
>>>>>>>>>> namely, there should be a header here giving a quick overview of
>>>>>>>>>> what
>>>>>>>>>> it
>>>>>>>>>> is
>>>>>>>>>> exactly you're testing for, and a BZ reference.
>>>>>>>>>>
>>>>>>>>>>> +extern int not_exist ();
>>>>>>>>>>> +
>>>>>>>>>>> +inline int make_unique ()
>>>>>>>>>>> +{
>>>>>>>>>>> +  static int unique;
>>>>>>>>>>> +  return ++unique;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +int foo ()
>>>>>>>>>>> +{
>>>>>>>>>>> +  return make_unique () + not_exist ();
>>>>>>>>>>> +}
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> i don't know if this is just copy & pasting, but prototypes that
>>>>>>>>>> do
>>>>>>>>>> not
>>>>>>>>>> intend
>>>>>>>>>> to take args should always be (void).
>>>>>>>>>> -mike
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks for review, I fixed that in patch v3.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Ping.
>>>>>>>>
>>>>>>> Some comments:
>>>>>>>
>>>>>>> 1. The bug report is against STB_GNU_UNIQUE.  But I don't see
>>>>>>> STB_GNU_UNIQUE in
>>>>>>> testcase.  I can't tell if the original STB_GNU_UNIQUE bug is fixed.
>>>>>>> 2. Your testcase ignores dlopen error.  Why should it work at all?
>>>>>>> 3. Your testcase doesn't use test-skeleton.c.
>>>>>>>
>>>>>>
>>>>>> Thanks for review!
>>>>>>
>>>>>> 1. I added some comments in testcase to be more clear.
>>>>>>
>>>>>> 2. Actually the call below is expected to return NULL because of
>>>>>> undefined
>>>>>> symbol in the library.
>>>>>>
>>>>>> dlopen ("$ORIGIN/tst-unique5lib.so", RTLD_NOW);
>>>>>>
>>>>>> It's a goal of this testcase: I need some library with STB_GNU_UNIQUE
>>>>>> symbols that returns error when trying to load it.
>>>>>> In new patch version I especially point out to this fact by checking
>>>>>> return value.
>>>>>>
>>>>>> 3. It seems for me that template of the test-skeleton.c is not
>>>>>> appropriate
>>>>>> because this testcase has two related binary - library and executable.
>>>>>>
>>>>>>
>>>>>> -Pavel
>>>>>
>>>>>
>>>>>
>>>>> Ping.
>>>>>
>>>>
>>>> Since you changed DF_1_NODELETE, how did you test it?  Does your test
>>>> fail without the DF_1_NODELETE change?
>>>
>>>
>>>
>>> AFAIU DF_1_NODELETE change is necessary. Otherwise unique symbols force
>>> library to remain loaded in half-initialized state (with missing
>>> dependencies). This usecase is demonstrated by the testcase in patch.
>>>
>>
>> But unique symbols != DF_1_NODELETE:
>
>
> DF_1_NODELETE is set in do_lookup_unique at runtime (Paul, am I right?):
>
>       enter_unique_sym (entries, size,
>                         new_hash, strtab + sym->st_name, sym, map);
>
>       if (map->l_type == lt_loaded)
>         /* Make sure we don't unload this object by
>            setting the appropriate flag.  */
>         ((struct link_map *) map)->l_flags_1 |= DF_1_NODELETE;
>
> -Y

Should we set DF_1_NODELETE when dlopen fails?  What happens
when dlopen a  DF_1_NODELETE fails?  Do we keep it in memory
even when dopen fails?

-- 
H.J.