fix #19443 - build failures with -DDEBUG
Commit Message
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.
Martin
Comments
On 01/12/2016 12:37 PM, Martin Sebor wrote:
> 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.
Sounds good.
> - ? &((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),
In the cast, please use (void *) instead of (char *), and put a space
after the ")". Otherwise, looks good to me; thanks.
On 01/12/2016 04:35 PM, Paul Eggert wrote:
> On 01/12/2016 12:37 PM, Martin Sebor wrote:
>> 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.
>
> Sounds good.
Do you plan to make similar changes to gnulib?
Cheers,
Carlos.
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.
On 01/12/2016 04:36 PM, Carlos O'Donell wrote:
> Do you plan to make similar changes to gnulib?
Yes, I'll do that once the glibc change is made. Or I can do it now if
you prefer. The two copies of mktime.c should be byte-for-byte identical.
On 01/12/2016 08:47 PM, Paul Eggert wrote:
> On 01/12/2016 04:36 PM, Carlos O'Donell wrote:
>> Do you plan to make similar changes to gnulib?
>
> Yes, I'll do that once the glibc change is made. Or I can do it now
> if you prefer. The two copies of mktime.c should be byte-for-byte
> identical.
OK, awesome. As you prefer.
Cheers,
Carlos.
>> 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?
The latest rshift.c looks quite different:
https://gmplib.org/repo/gmp/file/da670a8513db/mpn/generic/rshift.c
I haven't looked at the rest but if (according to the Wiki)
the last time GMP was integrated into Glibc was 1996 it seems
that replacing the DEBUG block with an assert might be the thing
to do for this bug and syncing the two up as a separate project.
(I'm not sure I have the cycles to take on the latter.)
> 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?
Gnulib still uses DEBUG:
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/mktime.c
> Usually rather than work around such an issue we propose a global change
> to gnulib and glibc that harmonizes such uses.
I'll wait for Paul to let us know. Another option might be
to undefine DEBUG in the Makefile just for this file to avoid
changing it.
Martin
Carlos O'Donell wrote:
> On 01/12/2016 08:47 PM, Paul Eggert wrote:
>> >On 01/12/2016 04:36 PM, Carlos O'Donell wrote:
>>> >>Do you plan to make similar changes to gnulib?
>> >
>> >Yes, I'll do that once the glibc change is made. Or I can do it now
>> >if you prefer. The two copies of mktime.c should be byte-for-byte
>> >identical.
> OK, awesome. As you prefer.
OK, committed in gnulib here:
http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=0433724bb2a1eb7db42181746b79e15ad7a79930
On 01/12/2016 11:29 PM, Martin Sebor wrote:
>>> 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?
>
> The latest rshift.c looks quite different:
> https://gmplib.org/repo/gmp/file/da670a8513db/mpn/generic/rshift.c
>
> I haven't looked at the rest but if (according to the Wiki)
> the last time GMP was integrated into Glibc was 1996 it seems
> that replacing the DEBUG block with an assert might be the thing
> to do for this bug and syncing the two up as a separate project.
> (I'm not sure I have the cycles to take on the latter.)
Agreed, replace it with an assert.
OK with that change.
I'm definitely not asking you to update from upstream GMP (which
is likely impossible due to licensing).
>> 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?
>
> Gnulib still uses DEBUG:
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/mktime.c
>
>> Usually rather than work around such an issue we propose a global change
>> to gnulib and glibc that harmonizes such uses.
>
> I'll wait for Paul to let us know. Another option might be
> to undefine DEBUG in the Makefile just for this file to avoid
> changing it.
Paul just checked it in to gnulib. So you're good to go.
Cheers,
Carlos.
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.
@@ -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);
}
printf(" ");
}
-static void
+static void __attribute__ ((unused))
_ufc_set_bits (ufc_long v, ufc_long *b)
{
ufc_long i;
@@ -219,6 +219,9 @@ libc_freeres_fn (free_mem)
#ifdef DEBUG
+
+#include <stdio.h>
+
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))
print_all (void)
{
__twalk (loaded, do_print);
@@ -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),
tmpbuf, sizeof (tmpbuf))));
if (__glibc_unlikely (v_circuit)) {
@@ -42,7 +42,7 @@ mpn_rshift (register mp_ptr wp,
#ifdef DEBUG
if (usize == 0 || cnt == 0)
- abort ();
+ __builtin_abort ();
#endif
sh_1 = cnt;
@@ -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:
*/