Re: [PATCH] Enable runtime cwrapper debugging; add tests

[Charles Wilson]

Ralf Wildenhues wrote:
> This patch is ok to commit with nits below addressed as you see fit;
> no need for further review.

Thanks for the review.

> 
>> --- a/libltdl/config/ltmain.m4sh
>> +++ b/libltdl/config/ltmain.m4sh

>>  
>> -const char *program_name = NULL;
>> +const char *program_name = "wrapper"; /* in case xstrdup fails */
> 
> "libtool wrapper" might be clearer.  You decide.

Fixed.

> 
>> @@ -2880,7 +2867,8 @@ char *chase_symlinks (const char *pathspec);
>>  int make_executable (const char *path);
>>  int check_executable (const char *path);
>>  char *strendzap (char *str, const char *pat);
>> -void lt_fatal (const char *message, ...);
>> +void lt_debugprintf (const char *file, int line, const char *fmt, ...);
>> +void lt_fatal (const char *file, int line, const char *message, ...);
> 
> It'd be nicer to wrap these two in a macro that expands __FILE__ and
> __LINE__ automatically, but varargs macros are not yet portable, and
> multiple defines are somewhat ugly too,
> 
>   #define lt_fatal0 (message) lt_fatal_in (__FILE__, __LINE__, message)
>   #define lt_fatal1 (message, arg1) \
>           lt_fatal_in (__FILE__, __LINE__, message, arg1)
>   #define lt_fatal2 (message, arg1, arg2) \
>           lt_fatal_in (__FILE__, __LINE__, message, arg1, arg2)
>   ...
> 
> so maybe the current code is a good compromise.

Yeah, the original LTWRAPPER_DEBUGPRINTF was also a workaround for the
lack of vararg macros, but it forced you to use (()) which I hated.  I
really didn't want to create N different macros for *both*
lt_debugprintf and lt_fatal (especially lt_debugprintf, which
coincidentally also needs N=2 support macros of this type).  Of course,
N=2 /now/ -- and we could just go with "use these macros, and if you
every need N=3... add the macros then.

But it's so UGLY.

So, yeah, the current code is an explicit compromise to work around the
lack of vararg macro support.  I think explicitly invoking __FILE__ and
__LINE__ is marginally less ugly than lt_foo_N() macros...

>> +      if (strcmp (argv[i], ltwrapper_option_prefix) == 0)
>> +        {

[THIS]:

>> +          /* however, if there is an option in the LTWRAPPER_OPTION_PREFIX
>> +             namespace, but it is not one of the ones we know about and
>> +             have already dealt with, above (inluding dump-script), then
>> +             report an error. Otherwise, targets might begin to believe
>> +             they are allowed to use options in the LTWRAPPER_OPTION_PREFIX
>> +             namespace. The first time any user complains about this, we'll
>> +             need to make LTWRAPPER_OPTION_PREFIX a configure-time option
>> +             or a configure.ac-settable value.
>> +           */
> 
> The problem being that we cannot please this user without providing an
> upgrade, or letting her change libtool code.  Maybe warn instead of fail
> hard?  I'm really torn here; we suddenly grab in-band namespace without
> an easy way to avoid it through some option.

Well, this code has been in the cygwin libtool, in one form or another,
for almost two years.  It's been used to build all of KDE and about 1400
total open source packages (cygwinports project) as well about 2GB
(compressed) source packages in the main cygwin distribution.

If none of THOSE used --lt-* I think we're pretty safe in 'grabbing' it.

> This issue should not be addressed in this patch, it is not new here,
> and we first should get consensus on the right way to address it.
> (Have we done that elsewhere already?)

I believe the consensus was, yeah...the RTTD is to add an ugly configure
option to ltoptions like --libtool-wrapper-option-prefix which defaults
to "--lt-" (*), and use that value when emitting the cwrapper source and
shwrapper.

http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00066.html
But Eric said
http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00067.html
"I don't want to spend any effort coding around it unless someone (and
it won't be me) demonstrates a real need for the extra flexability."

FWIW, that big comment block ([THIS], above) is really just a
placeholder/reminder that we can/should implement some sort of 'easy way
to avoid it' mechanism...

(*) Specifying the value this way requires the '=' form on the configure
line;  --libtool-wrapper-option-prefix=--lt-, but if you 'assume' the
double-dash, then you might be able to get away with
--libtool-wrapper-option-prefix lt-
and no '='

>> +          lt_fatal (__FILE__, __LINE__,
>> +                "Unrecognized %s option: '%s'",
>> +                    ltwrapper_option_prefix, argv[i]);
> 
>> +        cat <<EOF
>> +  /* first use of lt_debugprintf AFTER parsing options */
> 
> Not sure what this comment aims at.  Is this a statement of a nontrivial
> fact?

The important thing is that the very first output, when in debug mode
and no errors have occured (eg. bad arguments, etc), be the "banner" so
that cwrapper.at is happy.  I'll reword the comment.

/* The GNU banner must be the first non-error debug message */


>> +  lt_debugprintf (__FILE__, __LINE__, "libtool wrapper (GNU 
>> $PACKAGE$TIMESTAMP) $VERSION\n");
>> +EOF
>> +        cat <<"EOF"
>> +  lt_debugprintf (__FILE__, __LINE__, "(main) argv[0]: %s\n", argv[0]);
>> +  lt_debugprintf (__FILE__, __LINE__, "(main) program_name: %s\n", 
>> program_name);
> 
>>    actual_cwrapper_path = chase_symlinks (tmp_pathspec);
>> -  LTWRAPPER_DEBUGPRINTF (("(main) found exe (after symlink chase) at : 
>> %s\n",
>> -                      actual_cwrapper_path));
>> +  lt_debugprintf (__FILE__, __LINE__,
>> +                  "(main) found exe (after symlink chase) at: %s\n",
>> +              actual_cwrapper_path);
>>    XFREE (tmp_pathspec);
>>  
>>    actual_cwrapper_name = xstrdup( base_name (actual_cwrapper_path));
> 
> This last line existed before the patch, but has spacing wrong,
> according to GCS.  Fix it or not, however you like.

Fixed.

>> @@ -3099,7 +3102,9 @@ EOF
>>    if (rval == -1)
>>      {
>>        /* failed to start process */
>> -      LTWRAPPER_DEBUGPRINTF (("(main) failed to launch target \"%s\": errno 
>> = %d\n", lt_argv_zero, errno));
>> +      lt_debugprintf (__FILE__, __LINE__,
>> +                  "(main) failed to launch target \"%s\": errno = %d\n",
> 
> s/(main) //  ?  In all debug messages?  Hmm, maybe not.

Well, is __func__ portable enough (at least as far as mingw, cygwin,
(msvc?) compilers are concerned)?  I'd've preferred
  foo("(%s)...", __func__', ...)
but was afraid to use it, which is why I hardcoded the function names
into (almost) all the debug messages, WAY back when. OTOH, I don't think
it makes sense for debug messages from functions other than main()
include their function name, but that ones in main() itself don't, so I
don't think s/(main // is the way to go.

If desired, I'll put together a separate single-purpose patch to change
"(func) ..." to '"(%s) ...", __func__' throughout, so that it can be
committed (and reverted if we discover it causes problems) as a single
commit.

>> +                  lt_argv_zero, errno);
> 
> strerror (errno) if it's non-NULL, instead of errno = %d?
> 
>>        return 127;
>>      }
>>    return rval;
>> @@ -3121,7 +3126,7 @@ xmalloc (size_t num)
>>  {
>>    void *p = (void *) malloc (num);
>>    if (!p)
>> -    lt_fatal ("Memory exhausted");
>> +    lt_fatal (__FILE__, __LINE__, "Memory exhausted");
> 
> No first-word capitalization of errors, in all error messages.

Fixed.

>> @@ -3155,8 +3160,8 @@ check_executable (const char *path)
>>  {
>>    struct stat st;
>>  
>> -  LTWRAPPER_DEBUGPRINTF (("(check_executable)  : %s\n",
>> -                      path ? (*path ? path : "EMPTY!") : "NULL!"));
>> +  lt_debugprintf (__FILE__, __LINE__, "(check_executable): %s\n",
>> +              path ? (*path ? path : "EMPTY!") : "NULL!");
> 
> s/EMPTY!/(empty)/;  s/NULL!/(null)/   here and below?  No need to scream
> at the user, and (null) is what glibc uses.  Similar for FATAL and
> <NULL> below.  Actually, it might even be more compact to just use
> 
>   static const char *nonnull (const char *s)
>   {
>     return s ? s : "(null)";
>   }
> 
>   static const char *nonempty (const char *s)
>   {
>     return (s && !*s) ? "(empty)" : nonnull (s);
>   }
> 
> throughout.

I like it. Fixed.


>> @@ -3254,7 +3259,7 @@ find_executable (const char *wrapper)
>>              {
>>                /* empty path: current directory */
>>                if (getcwd (tmp, LT_PATHMAX) == NULL)
>> -                lt_fatal ("getcwd failed");
>> +                lt_fatal (__FILE__, __LINE__, "getcwd failed");
> 
> getcwd sets errno, so strerror (errno) might be interesting here
> and below.

Is strerror/errno portable enough to use? I know it's supported by
cygwin and mingw, but msvc?

http://msdn.microsoft.com/en-us/library/zc53h9bh(VS.80).aspx
says it is declared in string.h and supported all the way back to Win95,
so I guess so.

...and never mind. We're already using it elsewhere. So, fixed.

>> --- a/tests/cwrapper.at
>> +++ b/tests/cwrapper.at
>> @@ -79,5 +79,57 @@ for restrictive_flags in '-Wall -Werror' '-std=c89 -Wall 
>> -Werror' '-std=c99 -Wal
>>    LT_AT_EXEC_CHECK([./usea], [0], [ignore], [ignore], [])
>>  done
>>  
>> +
>> +# Test RUN-TIME activation of wrapper debugging
> 
> s/RUN-TIME/run-time/.  Missing period.  Similar below for COMPILE-TIME.

Fixed.

>> +# This is not part of the loop above, because we
>> +# need to check, not ignore, the output.
>> +CFLAGS="$orig_CFLAGS"
>> +LIBTOOL=$orig_LIBTOOL
>> +
>> +AT_CHECK([$LIBTOOL --mode=compile $CC $CPPFLAGS $CFLAGS -c liba.c],
>> +         [], [ignore], [ignore])
>> +AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -version-info=0.0.0 
>> -no-undefined -o liba.la -rpath /foo liba.lo],
> 
> -version-info needs ':' as separator, not '.'.  You can also just omit
> it, though.

D'oh. Fixed.

>> +# We structure this test as a loop, so that we can 'break' out of it
>> +# if necessary -- even though the loop by design executes only once.
>> +for debugwrapper_flags in '-DLT_DEBUGWRAPPER'; do
>> +  CFLAGS="$orig_CFLAGS $debugwrapper_flags"
>> +  sed "s/LTCFLAGS=.*/&' $debugwrapper_flags'/" < "$orig_LIBTOOL" |
>> +  sed "s/^lt_option_debug=/lt_option_debug=1/" > ./libtool
> 
> Can be just one sed command, but hey, however you like.

Fixed.

> It took me a few seconds to realize that you don't need chmod here
> because you overwrite an existing script here ...

Yeah, but it is confusing. I can add a redundant chmod +x here if you
think it necessary, but that's a one-liner for later on.

So, open issues, to be addressed if necessary in additional patches:
1) "(func) ..." ---> '"(%s) ...", __func__' ?
2) chmod in cwrapper.at?

As pushed.


--
Chuck

0022-Enable-runtime-cwrapper-debugging-add-tests-rev4.patch
Description: Text Data