Re: 329-gary-allow-RTLD_GLOBAL

[Gary V. Vaughan]

On 6 May 2007, at 21:57, Ralf Wildenhues wrote:

Hi Gary,

Hallo Ralf,

Thanks for the review :-)  Patch now committed to CVS.

* Gary V. Vaughan wrote on Tue, May 01, 2007 at 04:33:37PM CEST:

Sorry for the delay.

that your mailer does not wrap long lines;

Don't worry, and likewise. Thanks for the patch. Please ensure next time

to work in that environment.

Unfortunately, my laptop is in repair, and I don't want to spend the time setting up postfix on my desktop, then configuring my patch posting scripts

I should apologize in advance that my mailer may do the same now.

No problem.  I'm afraid that my desktop setup will continue to wrap long
lines too, although I should have my laptop back in a day or two.

I've added a test, which tries to be sympathetic to architectures that

have

no loaders that listen to advise, but will probably take a few

iterations

to shake out spurious failures on some hosts.

Sure.  Let's get the remaining stuff out once this is in.
Which hosts have you done testing on already?

Just Ubuntu and OS X.  Though, after my next couple of patches I think
we'll be in good shape for an alpha release, which I'll test extensively
on all the architectures I have access to.

Okay to commit?

Yes.  Below is a bunch of nits.  Please address as many as you can and
indicate which ones you left out, and apply.  Thanks.

Will do.

        (lt_dladvise_init, lt_dladvise_destroy): New functions to
        initialize and destroy an advise type hint.
        (lt_dladvise_ext, lt_dladvise_resident, lt_dladvise_local)
        (lt_dladvise_global): Set hints on an advise type.

hurt the text and the code.  No idea really.

Upon reading, I wondered whether s/advise type/advice/ would benefit or

I went back and forth on this myself.  Arguably we could use verbs for
functions (advise) and nouns for types (advice), but in the end I think
the consistency of advise saves having to remember what word to use at
each point in the API.  I don't have any particularly strong feelings
about it though incase someone would like to patch for `advice' in some
or all of the API after this commit.

FYA(musement) I took inspiration for the name from common lisp's advise

run time.

function, which wraps some other function with pre and post processing at

+ - New lt_dlopenadvise takes a new lt_dladvise type argument, which

+    lets the caller request local or global symbol visibility from
the

  +    module loader with lt_dladvise_local and lt_dladvise_global +

respectively.  If neither is given, or if lt_dlopen (or

lt_dlopenext)

used.
  +  - The new lt_dladvise_init based APIs also allow caller requests

+ are called, then the system default module symbol visibility is

for

+ a filename extension search with lt_dladvise_ext, and for marking

a

  +    module unloadable with lt_dladvise_resident.

that, even if you don't give any explanation for their semantics.
Autoconf does this, too.

Should all new interfaces (all functions) be listed in NEWS? I'd prefer

I included lt_dladvise_destroy.  Now fixed.

Yes, and I thought I had done that. Turns out I didn't update NEWS after

  [EMAIL PROTECTED] int lt_dladvise_init (lt_dladvise [EMAIL PROTECTED])
  +The @var{advise} parameter can be used to pass hints to the module

+loader when using @code{lt_dlopenadvise} to perform the loading. +The
@var{advise} paramater needs to be initialised by this function

s/paramater/parameter/

Thanks.  Done.

  +before it can be used.  Any memory used by @var{advise} is

automatically

  +recycled when it is passed to @code{lt_dlopenadvise}.

Is that a good idea?  What if I want to advise hundreds of libraries?
Also, I think this bit of documentation does not match the

not free the storage that `advise' uses.  So I guess rather point to
lt_dladvise_destroy here, or, as it's described as very next, just do
without a pointer.

implementation, nor the my_dlopenext example below: lt_dlopenadvice does

but, as you noticed, realised that it wasn't a good idea.  I forgot to
update the docs after changing the implementation.  Now fixed.

ACK. I started out with it implemented as described in the documentation,

  +The following example is equivalent to calling
  [EMAIL PROTECTED] (filename)}:
  +
  [EMAIL PROTECTED]
  +lt_dlhandle
  +my_dlopenext (const char *filename)
  [EMAIL PROTECTED]
  +  lt_dlhandle handle = 0;
  +  lt_dladvise advise;
  +
  +  if (!lt_dladvise_init (&advise) && !lt_dladvise_ext (&advise)) +

handle = lt_dlopenadvise (filename, &advise);

  +
  +  lt_dladvise_destroy (&advise);
  +
  +  return handle;
  [EMAIL PROTECTED]
  [EMAIL PROTECTED] example

It would be cool if this example matched exactly a bit of testsuite code. As of now, there are small differences to hint_ext in the test you added.

error checking and non-expandable code from the test, or else change the
way the tests are written to make them identical.  There's no reason we
can't extract the example code into a separate test later though.

The current test is a minimal effort bad example of coding that is designed to stress test the api, where the documentation is trying to show a minimal example with relatively clean code. I'd either have to expose my lack of

+On failure, @code{lt_dladvise_ext} returns non-zero and sets an error

+message that can be retrieved with @code{lt_dlerror}.

Also, it would be cool if this functionality (failure to open) were
exposed in the testsuite; likewise for the other new functions.

The only way it can fail at the moment is with an out of memory error in

be a challenge.  More tests are always welcome though.

lt_dladvise_init, in which case getting the test to run to completion will

  [EMAIL PROTECTED] int lt_dladvise_local (lt_dladvise [EMAIL PROTECTED]) +Set

the @code{symlocal} hint on @var{advise}.  Passing an
@var{advise}

+parameter to @code{lt_dlopenadvise} with this hint set causes it to

try

  +to keep the loaded module's symbols hidden so that they are not

+visible to subsequently loaded modules.

  +
  +On failure, @code{lt_dladvise_local} returns non-zero and sets an

error

  +message that can be retrieved with @code{lt_dlerror}.

host /can/ localize/hide symbols.  WDYT?

Should the hint about lt_dlgetinfo be repeated here? Otherwise, the user may think that a non-failure of lt_dladvise_local may indicate that the

Good call.  Done.

  @@ -3853,17 +3944,16 @@ Some of the internal information about e

maintained by libltdl is available to the user, in the form of this

structure:

  [EMAIL PROTECTED] {Type} {struct} lt_dlinfo @{ @w{char [EMAIL PROTECTED];}
@w{char [EMAIL PROTECTED];} @w{int @var{ref_count};} @w{lt_module
@var{module};[EMAIL PROTECTED]
  [EMAIL PROTECTED] {Type} {struct} lt_dlinfo @{ @w{char [EMAIL PROTECTED];}
@w{char [EMAIL PROTECTED];} @w{int @var{ref_count};} @w{int

@var{is_resident};}

@w{int @var{is_symglobal};} @w{int @var{is_symlocal};[EMAIL PROTECTED]

version bump.

Hmm. If lt_dlinfo is part of the public interface, and looking at 1.5.22 it seems is it, then this is an incompatible change that requires a major

Yes indeed.  Actually, the module field had already been moved out of
lt_dlinfo before I changed the docs, so we're past due the major version
bump.

We could discourage use of lt_dlinfo without going through pointer access

and malloc, in order to avoid this next time.

module field originally :-/

I don't follow. What would be malloced? We already state that this is a read-only structure. Although I have no clue why we exposed the internal

   /* lt_dlloader.c -- dynamic library loader interface
  -   Copyright (C) 2004 Free Software Foundation, Inc.
  +   Copyright (C) 2004, 2007 Free Software Foundation, Inc.

Any reason for this change?

Forgot to revert it after I undid a change to that file.  Fixed.

  +/* Try all dlloaders for FILENAME.  If the library is not

successfully

+ loaded, return non-zero. Otherwise, the dlhondle is stored at the

s/dlhondle/dlhandle/

The joys of dvorak keymapping vs a small font :-)  Good catch.  Thanks.

  +static lt__advise *
  +advise_dup (lt__advise *advise)
  +{

+ lt__advise *dup = (lt__advise *) lt__zalloc (sizeof (lt__advise));

+  return memcpy (dup, advise, sizeof (lt__advise));

  +}
  +
  +/* Libtool-1.5x interface for loading a new module named FILENAME.

*/

s/1\.5x/1.5.x/

Thanks.

     char *     name;           /* module name */
     int                ref_count;      /* number of times lt_dlopened minus
                                   number of times lt_dlclosed. */
  +  int                is_resident:1;  /* module can't be unloaded. */
  +  int                is_symglobal:1; /* module symbols can satisfy
  +                                subsequently loaded modules.  */
  +  int                is_symlocal:1;  /* module symbols are only available
  +                                locally. */

prerequisite that should be no issue.

Are we certain that bitfields are portable enough? I suppose with C89 as

during alpha testing.

I don't know, but I didn't get any feedback when I posed that question with my original patch proposal. I think they ought to be, and we'll find out soon enough

  +void
  +hint_ext (void)
  +{
  +  lt_dlhandle handle;
  +  lt_dladvise advise;
  +
  +  if (lt_dladvise_init (&advise) || lt_dladvise_ext (&advise)) +

complain ("error setting advise ext");

  +
  +  handle = moduleopen ("moddepend", advise);
  +
  +  if (handle)
  +    printf ("depend: %d\n", moduletest (handle, "g", "j"));
  +
  +  lt_dladvise_destroy (&advise);
  +}

caught at module closing time.

handle is never closed here. This is important: some failures are only

in main I've written:

  if (lt_dlexit () != 0)
    complain ("error during exit");

Is there something this won't catch?

  +$CC $CPPFLAGS $CFLAGS -c main.c
  +for file in modresident modlocal modglobal moddepend; do
  +  $LIBTOOL --mode=compile $CC $CPPFLAGS $CFLAGS -c $file.c
  +  AT_CHECK([$LIBTOOL --mode=link $CC -module $CFLAGS $LDFLAGS -o
$file.la \
  +            $file.lo -rpath /foo -avoid-version], [], [ignore],

What about -no-undefined, for w32?

so that we can test that it correctly calls the matching symbols from

know how to fix it.  I guess we'll need to skip this test on Windows and
AIX due to linker brain damage?

Cheers,
        Gary
--
  ())_.              Email me: [EMAIL PROTECTED]
  ( '/           Read my blog: http://blog.azazil.net
  / )=         ...and my book: http://sources.redhat.com/autobook
`(_~)_ Join my AGLOCO Network: http://www.agloco.com/r/BBBS7912

That would defeat building moddepend with unresolved symbols at link time, modglobal after load time symbol resolution :-( Known issue, but I don't

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