lt_dlerror changes
Hi, Well, this is what I ended up with, it does not change the currently documented saving of error messages until lt_dlerror() is called, it copies the error message to ensure that we don't return garbage when lt_dlerror is called. I think I also got all the places where we were setting file not found. Still, I am not overjoyed with this. Peter From 40202fade8f55891b5f4acfee54eba02636b91e8 Mon Sep 17 00:00:00 2001 From: Peter O'Gorman pe...@pogma.com Date: Thu, 17 Jun 2010 12:42:28 -0500 Subject: [PATCH] Improve libltdl error messages. * libltdl/lt_error.c: Add new functions to copy error messages and keep them until lt_dlerror() is called. * libltdl/libltdl/lt__private.h: Prototypes for new functions, new macros. * libltdl/ltdl.c: Use new macros and functions. * libltdl/loaders/preopen.c: Likewise. * tests/lt_dlerror.at: New test. * Makefile.am: Add new test. * tests/lt_dlopen.at: Make it pass. --- ChangeLog | 13 Makefile.am |1 + libltdl/libltdl/lt__private.h | 14 +++- libltdl/loaders/preopen.c | 10 ++- libltdl/lt_error.c| 44 +- libltdl/ltdl.c| 141 ++--- tests/lt_dlerror.at | 88 + tests/lt_dlopen.at|7 +-- 8 files changed, 253 insertions(+), 65 deletions(-) create mode 100644 tests/lt_dlerror.at diff --git a/ChangeLog b/ChangeLog index a5676ef..c6f0179 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,16 @@ +2010-06-17 Peter O'Gorman pe...@pogma.com + + Improve libltdl error messages. + * libltdl/lt_error.c: Add new functions to copy error messages + and keep them until lt_dlerror() is called. + * libltdl/libltdl/lt__private.h: Prototypes for new functions, + new macros. + * libltdl/ltdl.c: Use new macros and functions. + * libltdl/loaders/preopen.c: Likewise. + * tests/lt_dlerror.at: New test. + * Makefile.am: Add new test. + * tests/lt_dlopen.at: Make it pass. + 2010-06-16 Ralf Wildenhues ralf.wildenh...@gmx.de Optimize func_ltwrapper_scriptname to assume a cwrapper. diff --git a/Makefile.am b/Makefile.am index d0688ee..40fce35 100644 --- a/Makefile.am +++ b/Makefile.am @@ -488,6 +488,7 @@ TESTSUITE_AT = tests/testsuite.at \ tests/lt_dlopen.at \ tests/lt_dlopen_a.at \ tests/lt_dlopenext.at \ + tests/lt_dlerror.at \ tests/ltdl-libdir.at \ tests/ltdl-api.at \ tests/dlloader-api.at \ diff --git a/libltdl/libltdl/lt__private.h b/libltdl/libltdl/lt__private.h index f4c4a3d..dc043cc 100644 --- a/libltdl/libltdl/lt__private.h +++ b/libltdl/libltdl/lt__private.h @@ -137,13 +137,19 @@ struct lt__advise { #define LT__STRERROR(name) lt__error_string(LT_CONC(LT_ERROR_,name)) #define LT__GETERROR(lvalue) (lvalue) = lt__get_last_error() -#define LT__SETERRORSTR(errormsg) lt__set_last_error(errormsg) -#define LT__SETERROR(errorcode) LT__SETERRORSTR(LT__STRERROR(errorcode)) +#define LT__SETERRORSTR(errormsg) lt__set_last_error(errormsg, 0) +#define LT__SETERROR(errorcode) if (0 == lt__get_last_error()) \ + LT__SETERRORSTR(LT__STRERROR(errorcode)) +#define LT__ENTER_PUBLIC_FUNC(x) lt__enter_err() +#define LT__PUBLIC_FUNC_RETURN(x) lt__keep_error(); return x; +#define LT__FORCEERROR(errorstr) LT__SETERRORSTR(errorstr) LT_SCOPE const char *lt__error_string (int errorcode); LT_SCOPE const char *lt__get_last_error (void); -LT_SCOPE const char *lt__set_last_error (const char *errormsg); - +LT_SCOPE const char *lt__set_last_error (const char *errormsg, int mustfree); +LT_SCOPE voidlt__keep_error (void); +LT_SCOPE voidlt__enter_err (void); +LT_SCOPE const char *lt__dlerror(void); LT_END_C_DECLS #endif /*!defined(LT__PRIVATE_H)*/ diff --git a/libltdl/loaders/preopen.c b/libltdl/loaders/preopen.c index 7149287..8b14121 100644 --- a/libltdl/loaders/preopen.c +++ b/libltdl/loaders/preopen.c @@ -186,7 +186,6 @@ vm_open (lt_user_data LT__UNUSED loader_data, const char *filename, } LT__SETERROR (FILE_NOT_FOUND); - done: return module; } @@ -292,8 +291,9 @@ add_symlist (const lt_dlsymlist *symlist) int lt_dlpreload_default (const lt_dlsymlist *preloaded) { + LT__ENTER_PUBLIC_FUNC (); default_preloaded_symbols = preloaded; - return 0; + LT__PUBLIC_FUNC_RETURN (0); } @@ -303,6 +303,7 @@ int lt_dlpreload (const lt_dlsymlist *preloaded) { int errors = 0; + LT__ENTER_PUBLIC_FUNC (); if (preloaded) { @@ -318,7 +319,7 @@ lt_dlpreload (const lt_dlsymlist *preloaded) } } - return errors; + LT__PUBLIC_FUNC_RETURN (errors); } @@ -331,6 +332,7 @@ lt_dlpreload_open (const char *originator, lt_dlpreload_callback_func *func) symlist_chain *list; int errors = 0; int found = 0; + LT__ENTER_PUBLIC_FUNC (); /* For each symlist in the chain... */ for (list = preloaded_symlists; list; list = list-next) @@ -371,5 +373,5 @@
Re: lt_dlerror changes
On 6/17/2010 4:54 PM, Peter O'Gorman wrote: Well, this is what I ended up with, it does not change the currently documented saving of error messages until lt_dlerror() is called, it copies the error message to ensure that we don't return garbage when lt_dlerror is called. I think I also got all the places where we were setting file not found. Still, I am not overjoyed with this. Fails on cygwin. However, adding -no-undefined fixes that. AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o good-plugin.la -rpath $libdir ]dnl --here- [-module -avoid-version good-plugin.lo], [], [ignore], [ignore]) Unfortunately, this doesn't magically assist solving my problem with 71. dlloader-api.at:23: FAILED (dlloader-api.at:422) but that's not a reason to object to the patch. -- Chuck
Re: lt_dlerror changes
On 06/17/2010 08:36 PM, Charles Wilson wrote: On 6/17/2010 4:54 PM, Peter O'Gorman wrote: Well, this is what I ended up with, it does not change the currently documented saving of error messages until lt_dlerror() is called, it copies the error message to ensure that we don't return garbage when lt_dlerror is called. I think I also got all the places where we were setting file not found. Still, I am not overjoyed with this. Fails on cygwin. However, adding -no-undefined fixes that. AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o good-plugin.la -rpath $libdir ]dnl --here- [-module -avoid-version good-plugin.lo], [], [ignore], [ignore]) Unfortunately, this doesn't magically assist solving my problem with 71. dlloader-api.at:23: FAILED (dlloader-api.at:422) but that's not a reason to object to the patch. Well that sucks :( I suggest changing the LT__SETERROR macro in libltdl/libltdl/lt__private.h to print __FILE__ and __LINE__ etc. if errorcode == FILE_NOT_FOUND, then running test 71 again. It doesn't look like it's happening in loadlibrary.c. Thanks for checking this out! Peter
Reviving: [PATCH] [cygwin|mingw] Create UAC manifest files.
Original thread(s) http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00058.html http://lists.gnu.org/archive/html/libtool-patches/2009-07/msg2.html Revived thread(s): http://lists.gnu.org/archive/html/libtool-patches/2010-02/msg00019.html http://lists.gnu.org/archive/html/libtool-patches/2010-03/msg00011.html I think where this thread left off, Ralf had posted some (incomplete?) texinfo documentation attempting to describe the whole UAC problem. I think Ralf planned to extend that texinfo to serve as a sort of design document for how we OUGHT to treat the issue, and then move on to rewriting a patch to implement the desired behavior. So...the subthread starting with the fourth link above is probably the place to start. In the third subthread, I *think* Ralf was leaning toward the following behavior/assumptions. I don't necessarily agree with these, but: a) We should care only about the manifest file associated with the cwrapper. It is up to the package maintainer to take care to provide an appropriate manifest for the ACTUAL exe. b) If they DO provide a manifest, the package maintainer is responsible for putting the appropriate rules in the Makefile to ensure that it gets copied to $builddir if builddir != srcdir. (However, the copy over to builddir only matters if you're NOT compiling the manifest into a binary resource and linking it directly into the app). b1) Since, if we need a wrapper, it will go in $builddir, then the cwrapper will use that one. b2) libtool should probably copy the manifest file into .lib/ so that the real exe will get a copy too. This may not work correctly if the real exe triggers the lt- prefixing rule (because then the name encoded in the body of the manifest won't match the actual name of the .exe). b3) For these reasons, it's probably best if the package maintainer also provides rules for embedding the manifest in binary form into the real exe. How these rules will deal with the whole lt-*.exe naming problem (recompiling the manifest object to specify the true exe name, and then relinking, on libtool --mode=install?) is an open question. b4) The maintainer is also responsible for creating a rule to clean up the (copy of) the manifest file in $builddir, if it is a copy of a custom file in $srcdir. c) If these do NOT provide a manifest, then libtool will autogenerate one for the use of the cwrapper only, never for the actual exe. Of course, this means that the actual exe, if it has a naughty name, will fail to execute -- whether installed or uninstalled. But, see a), above. c1) We should automatically clean up the manifest in $builddir/ if and only if that manifest was created by libtool. This probably requires a change to automake. Now, the above is a lot of easy-to-get-wrong custom automake stuff (and some libtool clients don't use automake, but instead have handcrafted Makefile.in's -- but I guess we don't care much about them). And, because it would require a lot of custom, windows-only rules, many upstream projects would be hesitant to accept such patches. We could make this easier by adding, at least, .rc-.o (hmm: msvc, .rc-.res) rules to Automake. If we had that, then perhaps we could link the resource object (whether .o or .res) into the wrapper as well. But this pre-supposes that all such apps with naughty names MUST be patched to add: a .rc file specifying the manifest a manifest file -- perhaps populated as an AC_OUTPUT .in-file. but probably the Automake changes would include the necessary win32-platform AM_CONDITIONAL and emit appropriate rules to conditionally add the .rc file to prog_SOURCES. [[err, what if the .rc file is ALSO an AC_OUTPUT-generated file? How will Automake know to use BUILT_SOURCES? ]] But at least that's less invasive (in terms of the patch upstream projects would have to accept to their Makefile.am's) than all-custom. Ok, over to you, Ralf! I said, summarizing Ralf: http://lists.gnu.org/archive/html/libtool-patches/2010-02/msg00035.html It really sounds to me as if you would prefer that developers of packages that create executables with bad names: 1) MUST provide a .manifest file 2) MUST manually add sufficient rules to copy that .manifest to ${builddir}/ and manually add correct clean: rules to get rid of that copy 3) MUST manually add sufficient rules to install said manifest file 4) libtool should only be concerned with getting a manifest into /.libs. And really, if the project is responsible for creating one, and getting it into ${builddir}, then libtool doesn't need to autogenerate one, it can simply copy it into .libs if necessary 5) therefore libtool should never remove a .manifest file except for the one in .libs but as a consequence of 2), there's no need for automake clean rules (or libtool mode=clean rules) that address
Re: lt_dlerror changes
On 6/17/2010 10:24 PM, Peter O'Gorman wrote: Unfortunately, this doesn't magically assist solving my problem with 71. dlloader-api.at:23: FAILED (dlloader-api.at:422) but that's not a reason to object to the patch. Well that sucks :( I suggest changing the LT__SETERROR macro in libltdl/libltdl/lt__private.h to print __FILE__ and __LINE__ etc. if errorcode == FILE_NOT_FOUND, then running test 71 again. It doesn't look like it's happening in loadlibrary.c. ../libtool/libltdl/loaders/preopen.c (188) ../libtool/libltdl/loaders/preopen.c (188) ../libtool/libltdl/loaders/preopen.c (188) Hmm. Adding more debug output to preopen.c:vm_open ... Searching for preloaded symbol table for dlopen.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a oops, bailing at line 183 Searching for preloaded symbol table for loadlibrary.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a oops, bailing at line 183 Searching for preloaded symbol table for first.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a Found preloaded symbol table for loadlibrary_LTX_get_vtable Found preloaded symbol table for @PROGRAM@ ../libtool/libltdl/loaders/preopen.c (191) Searching for preloaded symbol table for module.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a Found preloaded symbol table for loadlibrary_LTX_get_vtable Found preloaded symbol table for @PROGRAM@ ../libtool/libltdl/loaders/preopen.c (191) Searching for preloaded symbol table for last.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a Found preloaded symbol table for loadlibrary_LTX_get_vtable Found preloaded symbol table for @PROGRAM@ ../libtool/libltdl/loaders/preopen.c (191) Searching for preloaded symbol table for /usr/bin/last Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a Found preloaded symbol table for loadlibrary_LTX_get_vtable Found preloaded symbol table for @PROGRAM@ Err...why is it looking for a module named /usr/bin/last? Is it somehow using a $PATH search, and matching last to /usr/bin/last.exe? cygwin's dlopen does do some funky things with $PATH, because that's how windows looks for DLLs -- but we're in preopen, here... Peter, can you try the attached patch on top of your mods, on some platform OTHER than cygwin, and tell me what output you get? -- Chuck diff --git a/libltdl/loaders/preopen.c b/libltdl/loaders/preopen.c index 7149287..8249c7d 100644 --- a/libltdl/loaders/preopen.c +++ b/libltdl/loaders/preopen.c @@ -163,11 +163,13 @@ vm_open (lt_user_data LT__UNUSED loader_data, const char *filename, filename = @PROGRAM@; } + fprintf(stderr, Searching for preloaded symbol table for %s\n, filename); for (lists = preloaded_symlists; lists; lists = lists-next) { const lt_dlsymlist *symbol; for (symbol= lists-symlist; symbol-name; ++symbol) { + fprintf (stderr, Found preloaded symbol table for %s\n, symbol-name); if (!symbol-address streq (symbol-name, filename)) { /* If the next symbol's name and address is 0, it means @@ -178,6 +180,7 @@ vm_open (lt_user_data LT__UNUSED loader_data, const char *filename, const lt_dlsymlist *next_symbol = symbol +1; if (next_symbol-address next_symbol-name) { + fprintf(stderr, oops, bailing at line %d\n, __LINE__); module = (lt_module) lists-symlist; goto done; } --- lt__private.h-bak 2010-06-17 22:33:21.82020 -0400 +++ lt__private.h 2010-06-17 22:53:39.98860 -0400 @@ -138,8 +138,13 @@ #define LT__GETERROR(lvalue) (lvalue) = lt__get_last_error() #define LT__SETERRORSTR(errormsg) lt__set_last_error(errormsg, 0) -#define LT__SETERROR(errorcode) if (0 == lt__get_last_error()) \ - LT__SETERRORSTR(LT__STRERROR(errorcode)) +#define LT__SETERROR(errorcode) do { if (0 == lt__get_last_error()) \ + if (LT_CONC(LT_ERROR_,FILE_NOT_FOUND) == LT_CONC(LT_ERROR_,errorcode)) { \ + fprintf (stderr, %s (%d)\n, __FILE__, __LINE__); \ + LT__SETERRORSTR(LT__STRERROR(errorcode)); \ + } else { \ + LT__SETERRORSTR(LT__STRERROR(errorcode)); \ + }} while (0) #define LT__ENTER_PUBLIC_FUNC(x) lt__enter_err() #define LT__PUBLIC_FUNC_RETURN(x) lt__keep_error(); return x; #define LT__FORCEERROR(errorstr) LT__SETERRORSTR(errorstr)
Re: lt_dlerror changes
On 06/17/2010 10:21 PM, Charles Wilson wrote: On 6/17/2010 10:24 PM, Peter O'Gorman wrote: Unfortunately, this doesn't magically assist solving my problem with 71. dlloader-api.at:23: FAILED (dlloader-api.at:422) but that's not a reason to object to the patch. Well that sucks :( I suggest changing the LT__SETERROR macro in libltdl/libltdl/lt__private.h to print __FILE__ and __LINE__ etc. if errorcode == FILE_NOT_FOUND, then running test 71 again. It doesn't look like it's happening in loadlibrary.c. ../libtool/libltdl/loaders/preopen.c (188) ../libtool/libltdl/loaders/preopen.c (188) ../libtool/libltdl/loaders/preopen.c (188) Hmm. Adding more debug output to preopen.c:vm_open ... Searching for preloaded symbol table for dlopen.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a oops, bailing at line 183 Searching for preloaded symbol table for loadlibrary.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a oops, bailing at line 183 Searching for preloaded symbol table for first.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a Found preloaded symbol table for loadlibrary_LTX_get_vtable Found preloaded symbol table for @PROGRAM@ ../libtool/libltdl/loaders/preopen.c (191) Searching for preloaded symbol table for module.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a Found preloaded symbol table for loadlibrary_LTX_get_vtable Found preloaded symbol table for @PROGRAM@ ../libtool/libltdl/loaders/preopen.c (191) Searching for preloaded symbol table for last.a Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a Found preloaded symbol table for loadlibrary_LTX_get_vtable Found preloaded symbol table for @PROGRAM@ ../libtool/libltdl/loaders/preopen.c (191) Searching for preloaded symbol table for /usr/bin/last Found preloaded symbol table for libltdlc Found preloaded symbol table for dlopen.a Found preloaded symbol table for dlopen_LTX_get_vtable Found preloaded symbol table for loadlibrary.a Found preloaded symbol table for loadlibrary_LTX_get_vtable Found preloaded symbol table for @PROGRAM@ Err...why is it looking for a module named /usr/bin/last? Is it somehow using a $PATH search, and matching last to /usr/bin/last.exe? cygwin's dlopen does do some funky things with $PATH, because that's how windows looks for DLLs -- but we're in preopen, here... Peter, can you try the attached patch on top of your mods, on some platform OTHER than cygwin, and tell me what output you get? Got this on fedora 13. Peter # -*- compilation -*- 72. dlloader-api.at:23: testing ... ++ cat ++ cat ++ cat ++ case $host_os in ++ : -I/home/pogma/gitco/cw/../libtool/libltdl ++ : /home/pogma/gitco/cw/tests/../libltdl/libltdlc.la ++ set +x ../../libtool/tests/dlloader-api.at:400: case $LIBLTDL in #( */_inst/lib/*) test -f $LIBLTDL || (exit 77) ;; esac Not enabling shell tracing (command contains an embedded newline) stdout: ++ CPPFLAGS='-I/home/pogma/gitco/cw/../libtool/libltdl ' ++ set +x ../../libtool/tests/dlloader-api.at:406: $LIBTOOL --mode=compile $CC $CPPFLAGS $CFLAGS -c module.c ++ /home/pogma/gitco/cw/libtool --mode=compile gcc -I/home/pogma/gitco/cw/../libtool/libltdl -g -O2 -c module.c stderr: stdout: libtool: compile: gcc -I/home/pogma/gitco/cw/../libtool/libltdl -g -O2 -c module.c -fPIC -DPIC -o .libs/module.o libtool: compile: gcc -I/home/pogma/gitco/cw/../libtool/libltdl -g -O2 -c module.c -o module.o /dev/null 21 ++ set +x ../../libtool/tests/dlloader-api.at:408: $LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o module.la -rpath /nowhere -module -avoid-version -no-undefinedmodule.lo ++ /home/pogma/gitco/cw/libtool --mode=link gcc -g -O2 -o module.la -rpath /nowhere -module -avoid-version -no-undefined module.lo stderr: stdout: libtool: link: gcc -shared .libs/module.o -Wl,-soname -Wl,module.so -o .libs/module.so libtool: link: ar cru .libs/module.a module.o libtool: link: ranlib .libs/module.a libtool: link: ( cd .libs rm -f module.la ln -s ../module.la module.la ) ++ . ./module.la +++ dlname=module.so +++ library_names='module.so module.so module.so' +++ old_library=module.a +++ inherited_linker_flags= +++ dependency_libs= +++ weak_library_names= +++ current=0 +++ age=0 +++ revision=0 +++ installed=no +++ shouldnotlink=yes +++ dlopen= +++ dlpreopen= +++ libdir=/nowhere ++ set +x ../../libtool/tests/dlloader-api.at:415: test -n $dlname || (exit 77) ++ test -n module.so ++
Re: lt_dlerror changes
On 06/17/2010 10:35 PM, Peter O'Gorman wrote: On 06/17/2010 10:21 PM, Charles Wilson wrote: On 6/17/2010 10:24 PM, Peter O'Gorman wrote: Unfortunately, this doesn't magically assist solving my problem with 71. dlloader-api.at:23: FAILED (dlloader-api.at:422) but that's not a reason to object to the patch. Well that sucks :( I suggest changing the LT__SETERROR macro in libltdl/libltdl/lt__private.h to print __FILE__ and __LINE__ etc. if errorcode == FILE_NOT_FOUND, then running test 71 again. It doesn't look like it's happening in loadlibrary.c. ../libtool/libltdl/loaders/preopen.c (188) ../libtool/libltdl/loaders/preopen.c (188) ../libtool/libltdl/loaders/preopen.c (188) Hmm. Adding more debug output to preopen.c:vm_open ... Searching for preloaded symbol table for dlopen.a Err...why is it looking for a module named /usr/bin/last? Is it somehow using a $PATH search, and matching last to /usr/bin/last.exe? cygwin's dlopen does do some funky things with $PATH, because that's how windows looks for DLLs -- but we're in preopen, here... Ok, it's adding a loader (at the end of the list of loaders), then trying to lt_dlopen(last), that's going to try all other loaders before getting to the added loader, and in your case the loadlibrary loader finds /usr/bin/last (because /usr/bin is properly in the search path). My changes to lt_dlerror should have given you the error from loadlibrary.c, but didn't, I will check why, it looks like the patch needs more work though. Peter