Re: [PATCH] fix #19443 - build failures with -DDEBUG

["Carlos O'Donell" <carlos at redhat dot com>]

On 01/12/2016 03:37 PM, Martin Sebor wrote:
> A number of glibc source files contain debugging code guarded
> by the "#if DEBUG" preprocessor conditional.  The DEBUG macro
> isn't documented but as Carlos notes in his comment on the bug,
> it can be useful to enable various debugging aspects of glibc,
> for example in the resolver library.
> 
> The attached patch makes it possible to build glibc with the macro
> defined.  In one instance (time/mktime.c) the macro is being used
> in a way that's incompatible with the other uses. In that case I
> renamed to avoid this problem.
> 
> In case that raises questions, in resolv/res_send.c, I chose
> to replace the undeclared abort with __builtin_abort to avoid
> (mostly) unnecessarily including <stdlib.h>.  I checked for
> uses of other GCC builtins and found a bunch, though not any
> of __builtin_abort.  Either approach works for me.
> 
> Tested by building and using the library on powerpc64le and
> x86_64 with the macro defined.

Generally looks good to me.

The GMP file should be reviewed upstream.

Similarly for the gnulib file.

> Martin
> 
> glibc-19443.patch
> 
> 
> 2016-01-12  Martin Sebor  <msebor@redhat.com>
> 
> 	[BZ #19443]
> 	* crypt/crypt_util.c [DEBUG] (_ufc_prbits): Correct format string.
> 	[DEBUG] (_ufc_set_bits): Declare used.
> 	* iconv/gconv_dl.c [DEBUG]: Add a missing include directive.
> 	[DEBUG] (print_all): Declare used.
> 	* resolv/res_send.c [DEBUG] (__libc_res_nsend): Explicitly convert
> 	operands of the ternary ?: expression to target type.
> 	* stdlib/rshift.c [DEBUG] (mpn_rshift): Call __builtin_abort()  
> 	instead of the undeclared abort.
> 	* time/mktime.c [DEBUG] (DEBUG): Rename to DEBUG_MKTIME.
> 
> diff --git a/crypt/crypt_util.c b/crypt/crypt_util.c
> index 4b6490e..1f42f59 100644
> --- a/crypt/crypt_util.c
> +++ b/crypt/crypt_util.c
> @@ -271,12 +271,12 @@ _ufc_prbits (ufc_long *a, int n)
>        t=8*i+j;
>        tmp|=(a[t/24] & BITMASK[t % 24])?bytemask[j]:0;
>      }
> -    (void)printf("%02x ",tmp);
> +    (void)printf("%02lx ", tmp);

OK.

>    }
>    printf(" ");
>  }
>  
> -static void
> +static void __attribute__ ((unused))

OK.

>  _ufc_set_bits (ufc_long v, ufc_long *b)
>  {
>    ufc_long i;
> diff --git a/iconv/gconv_dl.c b/iconv/gconv_dl.c
> index 86559d3..4e9dfea 100644
> --- a/iconv/gconv_dl.c
> +++ b/iconv/gconv_dl.c
> @@ -219,6 +219,9 @@ libc_freeres_fn (free_mem)
>  
>  
>  #ifdef DEBUG
> +
> +#include <stdio.h>
> +

OK.

>  static void
>  do_print (const void *nodep, VISIT value, int level)
>  {
> @@ -231,7 +234,7 @@ do_print (const void *nodep, VISIT value, int level)
>  	  obj->name, obj->counter);
>  }
>  
> -static void
> +static void __attribute__ ((used))

OK.

>  print_all (void)
>  {
>    __twalk (loaded, do_print);
> diff --git a/resolv/res_send.c b/resolv/res_send.c
> index 3550740..776dcb5 100644
> --- a/resolv/res_send.c
> +++ b/resolv/res_send.c
> @@ -499,8 +499,8 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen,
>  		       (stdout, ";; Querying server (# %d) address = %s\n",
>  			ns + 1, inet_ntop(nsap->sa_family,
>  					  (nsap->sa_family == AF_INET6
> -					   ? &((struct sockaddr_in6 *) nsap)->sin6_addr
> -					   : &((struct sockaddr_in *) nsap)->sin_addr),
> +					   ? (char *)&((struct sockaddr_in6 *) nsap)->sin6_addr
> +					   : (char *)&((struct sockaddr_in *) nsap)->sin_addr),

Paul commented on this.

>  					  tmpbuf, sizeof (tmpbuf))));
>  
>  		if (__glibc_unlikely (v_circuit))       {
> diff --git a/stdlib/rshift.c b/stdlib/rshift.c
> index 4cf345c..71f8736 100644
> --- a/stdlib/rshift.c
> +++ b/stdlib/rshift.c
> @@ -42,7 +42,7 @@ mpn_rshift (register mp_ptr wp,
>  
>  #ifdef DEBUG
>    if (usize == 0 || cnt == 0)
> -    abort ();
> +    __builtin_abort ();
>  #endif
>  
>    sh_1 = cnt;

This file is shared with GMP.

See: https://sourceware.org/glibc/wiki/SharedSourceFiles

Can you peek at upstream to see if they included the apporpriate
header to make abort(); work and try to harmonize with whatever
changes GMP made?

Keep in mind GMP v6 and above are LGPLv3 or GPLv2, and I don't
know if that blocks us from another import of GMP into glibc.


> diff --git a/time/mktime.c b/time/mktime.c
> index 7d06314..bc7ed56 100644
> --- a/time/mktime.c
> +++ b/time/mktime.c

This file belongs to gnulib and was last merged 2014-06-27.

Could you please look at upstream gnulib and see what they are doing?

Usually rather than work around such an issue we propose a global change
to gnulib and glibc that harmonizes such uses.

> @@ -19,7 +19,7 @@
>  
>  /* Define this to have a standalone program to test this implementation of
>     mktime.  */
> -/* #define DEBUG 1 */
> +/* #define DEBUG_MKTIME 1 */
>  
>  #ifndef _LIBC
>  # include <config.h>
> @@ -38,13 +38,13 @@
>  
>  #include <string.h>		/* For the real memcpy prototype.  */
>  
> -#if defined DEBUG && DEBUG
> +#if defined DEBUG_MKTIME && DEBUG_MKTIME
>  # include <stdio.h>
>  # include <stdlib.h>
>  /* Make it work even if the system's libc has its own mktime routine.  */
>  # undef mktime
>  # define mktime my_mktime
> -#endif /* DEBUG */
> +#endif /* DEBUG_MKTIME */
>  
>  /* Some of the code in this file assumes that signed integer overflow
>     silently wraps around.  This assumption can't easily be programmed
> @@ -600,7 +600,7 @@ libc_hidden_def (mktime)
>  libc_hidden_weak (timelocal)
>  #endif
>  
> -#if defined DEBUG && DEBUG
> +#if defined DEBUG_MKTIME && DEBUG_MKTIME
>  
>  static int
>  not_equal_tm (const struct tm *a, const struct tm *b)
> @@ -732,10 +732,10 @@ main (int argc, char **argv)
>    return status;
>  }
>  
> -#endif /* DEBUG */
> +#endif /* DEBUG_MKTIME */
>  
>  /*
>  Local Variables:
> -compile-command: "gcc -DDEBUG -I. -Wall -W -O2 -g mktime.c -o mktime"
> +compile-command: "gcc -DDEBUG_MKTIME -I. -Wall -W -O2 -g mktime.c -o mktime"
>  End:
>  */

Cheers,
Carlos.