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

[Ralf Wildenhues]

Hi Charles,

* Charles Wilson wrote on Fri, Feb 19, 2010 at 03:48:40AM CET:
>       Enable runtime cwrapper debugging; add tests
>       * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src):
>       Update comments. Initialize program_name. Eliminate _LENGTH
>       variables for string constants. In debug mode, print a
>       banner with known content before any other output. Remove
>       LTWRAPPER_DEBUGPRINTF macro. Add constants and variables
>       to support new --lt-debug option.
>       (func_emit_cwrapperexe_src:ltwrapper_debugprintf): Renamed to...
>       (func_emit_cwrapperexe_src:lt_debugprintf): this. Only print
>       messages if lt_debug != 0. Ensure appearance of messages
>       conforms to GCS.
>       (func_emit_cwrapperexe_src:lt_fatal): Ditto.
>       (func_emit_cwrapperexe_src:lt_error_core): Ditto.
>       (func_emit_cwrapperexe_src): Update all callers to lt_fatal.
>       Update all users of LTWRAPPER_DEBUGPRINTF (()) to call
>       lt_debugprintf () directly.
>       (func_emit_cwrapperexe_src:main): Consolidate option parsing.
>       Ensure first use of lt_debugprintf occurs after option parsing.
>       Add stanza to parse for --lt-debug and set lt_debug variable.
>       Use strcmp rather than strncmp, where safe.
>       * tests/cwrapper.at: Add new tests for --lt-debug and
>       -DLT_DEBUGWRAPPER.

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

Thanks!
Ralf

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

> @@ -2855,22 +2851,13 @@ int setenv (const char *, const char *, int);
>    if (stale) { free ((void *) stale); stale = 0; } \
>  } while (0)
>  
> -#undef LTWRAPPER_DEBUGPRINTF
> -#if defined LT_DEBUGWRAPPER
> -# define LTWRAPPER_DEBUGPRINTF(args) ltwrapper_debugprintf args
> -static void
> -ltwrapper_debugprintf (const char *fmt, ...)
> -{
> -    va_list args;
> -    va_start (args, fmt);
> -    (void) vfprintf (stderr, fmt, args);
> -    va_end (args);
> -}
> +#if defined(LT_DEBUGWRAPPER)
> +static int lt_debug = 1;
>  #else
> -# define LTWRAPPER_DEBUGPRINTF(args)
> +static int lt_debug = 0;
>  #endif
>  
> -const char *program_name = NULL;
> +const char *program_name = "wrapper"; /* in case xstrdup fails */

"libtool wrapper" might be clearer.  You decide.

> @@ -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.

> +      if (strcmp (argv[i], ltwrapper_option_prefix) == 0)
> +        {
> +          /* 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.

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?)

> +          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?

> +  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.

> @@ -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.

> +                   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.

> @@ -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.

> @@ -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.

>                 tmp_len = strlen (tmp);
>                 concat_name =
>                   XMALLOC (char, tmp_len + 1 + strlen (wrapper) + 1);
> @@ -3279,7 +3284,7 @@ find_executable (const char *wrapper)
>      }
>    /* Relative path | not found in path: prepend cwd */
>    if (getcwd (tmp, LT_PATHMAX) == NULL)
> -    lt_fatal ("getcwd failed");
> +    lt_fatal (__FILE__, __LINE__, "getcwd failed");
>    tmp_len = strlen (tmp);
>    concat_name = XMALLOC (char, tmp_len + 1 + strlen (wrapper) + 1);
>    memcpy (concat_name, tmp, tmp_len);


> --- 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.

> +# 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.

> +         [], [ignore], [ignore])
> +AT_CHECK([test -f liba.la])
> +
> +AT_CHECK([$CC $CPPFLAGS $CFLAGS -c usea.c],
> +         [], [ignore], [ignore])
> +AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o usea$EXEEXT 
> usea.$OBJEXT liba.la],
> +         [], [ignore], [ignore])
> +LT_AT_EXEC_CHECK([./usea], [0], [ignore], [stderr], [--lt-debug])
> +LT_AT_UNIFY_NL([stderr])
> +AT_CHECK([grep 'libtool wrapper' stderr], [0], [ignore], [ignore])
> +
> +
> +# Test COMPILE-TIME activation of wrapper debugging

> +# 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.  Nifty trick
to make the test pass on non-w32 BTW.  :-)

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

> +  LIBTOOL=./libtool

>  AT_CLEANUP