Re: lt_dlopenadvise() inconsistencies in libtool-2.2.2

[Gary V . Vaughan]

Hi Gary,

On 10 Apr 2008, at 18:00, Gary Kumfert wrote:
> I am reporting either a documentation bug, an implementation bug,
> or both in 2.2.2.  I'm not sure what the intent was, but it took
> me (and my good friend, Tom) a bit of time to hunt
> down why my libraries weren't being loaded globally like they
> used to when I upgraded libtool.

Thanks for the excellent in depth report.  Sorry it took me a while to
get to it while we migrated libtool from CVS into git.

> The code says:
>         lt_dlopenadvise() takes a lt_dladvise as its
>         second argument (not a lt_dladvise* as with
>         all the other lt_dladvise_* functions!)

And this is also what the testsuite checks is working in tests/ 
lt_dladvise.at.

> The documentation says:
> 1.  Actually Section 11.1 doesn't have an entry for lt_dlopenadvise()
>         which is a bug in itself.

Worse, I corrected this by extrapolating some documentation for  
lt_dlopenadvise from the bogus example in the documentation.  In this  
case the documentation was wrong to start with, and is now even more  
wrong.  I'm preparing a patch at the moment.

> 2. AND the code examples where lt_dlopenadvise() appears
>         (e.g. lt_dladvise_ext() in Section 11.1) indicate a lt_dladvise*
>         which doesn't gibe with the code.
>          lt_dlhandle
>          my_dlopenext (const char *filename)
>          {
>            lt_dlhandle handle = 0;
>            lt_dladvise advise;
>
>            if (!lt_dladvise_init (&advise) && !lt_dladvise_ext  
> (&advise))
>              handle = lt_dlopenadvise (filename, &advise); // 
> <<<<---- wrong!!!
>
>            lt_dladvise_destroy (&advise);
>
>            return handle;
>          }

Also wrong in the documentation, and now corrected in the forthcoming  
patch.

> See the problem?
>
> Questions:
> 1. What was the intended API?  lt_dladvise or lt_dladvise* ?

lt_dlopenadvise does not change the contents of lt_dladvise, and thus  
accepts the
type directly rather than a pointer to it.   The other API calls  
potentially give
back new memory (or even delete the memory at the address passed) and  
thus require
a pointer to an lt_dladvise.

> 2. Why bother with all the casting in the implementation?
>   The type is known statically and the compiler *could*
>   have helped in finding this problem if we let it.

Mostly for consistency with how lt_dlhandles are manipulated.  The  
idea is that
outside libltdl implementation code only a "void *" type is available,  
and callers
cannot play with the internals of the struct.  However, when the  
implementation
wants to access the internals it must cast the void * address back to  
a struct
pointer.

I agree that this was arguably not the best decision, and maybe once  
we've
branched the tree for 2.2 maintenance we should consider putting the  
struct
in a public header along with a warning that it should not be accessed  
to give
the compiler the best chance to catch these kinds of errors in future.

> 3. What is the resolution?  I'd like to know what to plan for.


Implementation beats documentation.  Patch forthcoming.

Cheers,
        Gary
--
  ())_.              Email me: address@hidden
  ( '/           Read my blog: http://blog.azazil.net
  / )=         ...and my book: http://sources.redhat.com/autobook
`(_~)_

PGP.sig
Description: This is a digitally signed message part