Re: [PATCH v2] Also use l_tls_dtor_count to decide on object unload (BZ #18657)

[Roland McGrath <roland at hack dot frob dot com>]

> +      size_t tls_dtor_count = atomic_load_relaxed (&l->l_tls_dtor_count);
> +
>        /* Check whether this object is still used.  */
>        if (l->l_type == lt_loaded
>  	  && l->l_direct_opencount == 0
>  	  && (l->l_flags_1 & DF_1_NODELETE) == 0
> +	  && tls_dtor_count == 0
>  	  && !used[done_index])
>  	continue;

IMHO the separate variable here is actually less readable than
just putting atomic_load_relaxed (...) inside the if.

> +static void *
> +use_handle (void *h)
> +{
> +  void (*foo) (void) = (void (*) (void)) dlsym(h, "do_foo");

Space before paren after dlsym.

> +  if (!foo)

No implicit Boolean coercion.

> +  /* Load our module.  We access (and hence construct the object) in the
> +     thread.  */

I'm not sure I understand this comment.  Access what?  Which object?  What
does construct mean here?  The best reading I can guess is, "We call into
the module in the thread created below, so it accesses its TLS and
registers its destructor in that thread."  Is that what it means?

> +  if (thr_ret != NULL)
> +    return 1;

What's the point of this test?  It should be an assert, if anything.
There is no aspect of what's being tested here that affects the return
value of the thread function, is there?

> +  /* This sequence should not unload the DSO.  */
> +  void *h2 = dlopen ("$ORIGIN/tst-tls-atexit-lib.so",
> +		     RTLD_LAZY | RTLD_NODELETE);

Perhaps use a macro for the DSO name string, so it's more obvious that it's
exactly the same thing in both calls.

> +  /* Close the handle we created in the thread.  */
> +  dlclose (handle);
> +  dlclose (h2);

I don't understand this comment.  I guess "create the handle" means "call
dlopen", and both calls are in this function, on the main thread.

> +  /* Run through our maps and ensure that the DSO is unloaded.  */
> +  FILE *f = fopen ("/proc/self/maps", "r");

This is unnecessarily Linux-specific.  I don't think you need to do
external inspection to test this.  You can just do something like save a
pointer from dlsym, and then try to use it and if the module was unloaded
that will crash.

It's also repeating code from tst-tls-atexit.c (which I don't recall
previously noticing was doing this /proc/self/maps funny business that I
don't think is really what we want).  Perhaps these two tests can share
that part of the code?


Thanks,
Roland