diff mbox

Oh dear. I regret to inform you that commit 0e65dfbaf3a0299e4837216a103c28625d4b4f1d might be unfortunate

Message ID yddr28ha7e4.fsf@CeBiTec.Uni-Bielefeld.DE
State New, archived
Headers

Commit Message

Rainer Orth May 29, 2019, 10:21 a.m. UTC 
  Simon Marchi <simark@simark.ca> writes:

> On 2019-05-28 8:02 p.m., gdb-buildbot@sergiodj.net wrote:
>> My lords, ladies, gentlemen, members of the public.
>> 
>> It is a matter of great regret and sadness to inform you that commit:
>> 
>> 	libctf: build system
>> 	0e65dfbaf3a0299e4837216a103c28625d4b4f1d
>> 
>> might have made GDB unwell.  Since I am just your Butler BuildBot,
>> I kindly ask that a human superior officer double-check this.
>> 
>> Please note that if you are reading this message on gdb-patches, there might
>> be other builders broken.
>> 
>> You can find more details about the unfortunate breakage in the next messages.
>> 
>> Cheers,
>> 
>> Your GDB BuildBot.
>> 
>
> In case you have trouble spotting the error, I believe it is
>
> ../../binutils-gdb/libctf/ctf-archive.c:24:10: fatal error: endian.h: No such file or directory
>  #include <endian.h>
>           ^~~~~~~~~~
>
> which you can find if you follow one of the "Full build" URLs in the other messages.

Unfortunately, it's way worse than that:

* <endian.h> isn't even needed at all: gdb master still compiles with
  that include removed.

* Once this is fixed, the Solaris build still fails with several
  instances of

/vol/src/gnu/gdb/hg/master/dist/libctf/ctf-hash.c: In function 'ctf_hashtab_insert':
/vol/src/gnu/gdb/hg/master/dist/libctf/ctf-hash.c:141:7: error: 'errno' undeclared (first use in this function)
  141 |       errno = -ENOMEM;
      |       ^~~~~

  It turns out that ctf-impl.h (for reasons that completely escape me)
  includes <sys/errno.h> instead of <errno.h>.  Once that is fixed, the
  amd64-pc-solaris2.11 libctf build at least finishes.

* There are still quite a number of warnings that I didn't bother look
  into (all seen with gdb 9.1.0 on Solaris 11.5/x86):

/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_arc_write':
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:110:24: warning: implicit declaration of function 'htole64' [-Wimplicit-function-declaration]
  110 |   archdr->ctfa_magic = htole64 (CTFA_MAGIC);
      |                        ^~~~~~~
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:132:31: warning: implicit declaration of function 'le64toh' [-Wimplicit-function-declaration]
  132 |   for (i = 0, namesz = 0; i < le64toh (archdr->ctfa_nfiles); i++)
      |                               ^~~~~~~
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:132:29: warning: comparison of integer expressions of different signedness: 'size_t' {aka 'long unsigned int'} and 'int' [-Wsign-compare]
  132 |   for (i = 0, namesz = 0; i < le64toh (archdr->ctfa_nfiles); i++)
      |                             ^
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:145:10: warning: comparison of integer expressions of different signedness: 'size_t' {aka 'long unsigned int'} and 'int' [-Wsign-compare]
  145 |        i < le64toh (archdr->ctfa_nfiles); i++)
      |          ^
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:172:3: warning: implicit declaration of function 'qsort_r'; did you mean 'qsort'? [-Wimplicit-function-declaration]
  172 |   qsort_r ((ctf_archive_modent_t *) ((char *) archdr
      |   ^~~~~~~
      |   qsort
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_arc_bufopen':
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:320:33: warning: comparison is always true due to limited range of data type [-Wtype-limits]
  320 |   if (le64toh (arc->ctfa_magic) != CTFA_MAGIC)
      |                                 ^~
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_arc_open_internal':
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:357:33: warning: comparison is always true due to limited range of data type [-Wtype-limits]
  357 |   if (le64toh (arc->ctfa_magic) != CTFA_MAGIC)
      |                                 ^~
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_arc_open_by_offset':
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:514:3: warning: implicit declaration of function 'bzero' [-Wimplicit-function-declaration]
  514 |   bzero (&ctfsect, sizeof (ctf_sect_t));
      |   ^~~~~
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:514:3: warning: incompatible implicit declaration of built-in function 'bzero'
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_archive_raw_iter_internal':
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:546:17: warning: comparison of integer expressions of different signedness: 'size_t' {aka 'long unsigned int'} and 'int' [-Wsign-compare]
  546 |   for (i = 0; i < le64toh (arc->ctfa_nfiles); i++)
      |                 ^
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c: In function 'ctf_update':
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c:346:3: warning: implicit declaration of function 'qsort_r'; did you mean 'qsort'? [-Wimplicit-function-declaration]
  346 |   qsort_r (dvarents, nvars, sizeof (ctf_varent_t), ctf_sort_var, s0);
      |   ^~~~~~~
      |   qsort
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_archive_iter_internal':
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:594:17: warning: comparison of integer expressions of different signedness: 'size_t' {aka 'long unsigned int'} and 'int' [-Wsign-compare]
  594 |   for (i = 0; i < le64toh (arc->ctfa_nfiles); i++)
      |                 ^
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c: In function 'ctf_add_member_offset':
configure: loading cache ./config.cache
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c:1351:10: warning: implicit declaration of function 'roundup' [-Wimplicit-function-declaration]
 1351 |    off = roundup (off, NBBY) / NBBY;
      |          ^~~~~~~
/vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c:1352:24: warning: implicit declaration of function 'MAX' [-Wimplicit-function-declaration]
 1352 |    off = roundup (off, MAX (malign, 1));
      |                        ^~~

  E.g. qsort_r is a glibc addition not present on Solaris (and NetBSD,
  it seems), bzero would need <strings.h> if really necessary rather
  than using memset.

I'll never understand why some Oracle guys find it so hard to locate
some Solaris box in the company and try that stuff there.  After all,
CTF and libctf originated on Solaris and adding it to binutils-gdb
shouldn't break the Solaris build.

	Rainer

Comments

Nix May 29, 2019, 12:46 p.m. UTC | #1 
On 29 May 2019, Rainer Orth outgrape:

>> In case you have trouble spotting the error, I believe it is
>>
>> ../../binutils-gdb/libctf/ctf-archive.c:24:10: fatal error: endian.h: No such file or directory
>>  #include <endian.h>
>>           ^~~~~~~~~~
>>
>> which you can find if you follow one of the "Full build" URLs in the other messages.
>
> Unfortunately, it's way worse than that:
>
> * <endian.h> isn't even needed at all: gdb master still compiles with
>   that include removed.

Uh... the ctf-archive.c warnings below are 90% down to the removal of
this header. So I'd say it doesn't compile *well*. :)

Does Solaris seriously not have this header? It's in Illumos, but I
guess it's a later addition...

I'll have to add local implementations.

> * Once this is fixed, the Solaris build still fails with several
>   instances of
>
> /vol/src/gnu/gdb/hg/master/dist/libctf/ctf-hash.c: In function 'ctf_hashtab_insert':
> /vol/src/gnu/gdb/hg/master/dist/libctf/ctf-hash.c:141:7: error: 'errno' undeclared (first use in this function)
>   141 |       errno = -ENOMEM;
>       |       ^~~~~
>
>   It turns out that ctf-impl.h (for reasons that completely escape me)
>   includes <sys/errno.h> instead of <errno.h>.  Once that is fixed, the
>   amd64-pc-solaris2.11 libctf build at least finishes.

Historical madness. I really thought I removed this in an earlier review
round, but it must have crept back in :(

Will re-fix.

> * There are still quite a number of warnings that I didn't bother look
>   into (all seen with gdb 9.1.0 on Solaris 11.5/x86):
>
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_arc_write':
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:110:24: warning: implicit declaration of function 'htole64' [-Wimplicit-function-declaration]
>   110 |   archdr->ctfa_magic = htole64 (CTFA_MAGIC);
>       |                        ^~~~~~~
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:132:31: warning: implicit declaration of function 'le64toh' [-Wimplicit-function-declaration]
>   132 |   for (i = 0, namesz = 0; i < le64toh (archdr->ctfa_nfiles); i++)

Will adjust, as noted above.

> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:172:3: warning: implicit declaration of function 'qsort_r'; did you mean 'qsort'? [-Wimplicit-function-declaration]
>   172 |   qsort_r ((ctf_archive_modent_t *) ((char *) archdr

OK, I'll have to reimplement it (using qsort() would massively uglify
the code).

> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_arc_open_by_offset':
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:514:3: warning: implicit declaration of function 'bzero' [-Wimplicit-function-declaration]
>   514 |   bzero (&ctfsect, sizeof (ctf_sect_t));
>       |   ^~~~~

I stripped almost all of these out, but it looks like one single one
survived. It is gone now.

> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c: In function 'ctf_add_member_offset':
> configure: loading cache ./config.cache
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c:1351:10: warning: implicit declaration of function 'roundup' [-Wimplicit-function-declaration]
>  1351 |    off = roundup (off, NBBY) / NBBY;
>       |          ^~~~~~~
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c:1352:24: warning: implicit declaration of function 'MAX' [-Wimplicit-function-declaration]
>  1352 |    off = roundup (off, MAX (malign, 1));
>       |                        ^~~

Augh, roundup() is a glibc-specific extension? (But this is as old as
the import from Solaris, so it was clearly a Solaris extension once,
too.)
 
I'll fix that, too. :/

Working on a fix for all of these now.
Andreas Schwab May 29, 2019, 1:14 p.m. UTC | #2 
On Mai 29 2019, Nix <nix@esperi.org.uk> wrote:

> Augh, roundup() is a glibc-specific extension?

roundup is an old *BSD* extension (all of <sys/param.h> is).

Andreas.
Rainer Orth May 29, 2019, 1:38 p.m. UTC | #3 
Nix <nix@esperi.org.uk> writes:

> On 29 May 2019, Rainer Orth outgrape:
>
>>> In case you have trouble spotting the error, I believe it is
>>>
>>> ../../binutils-gdb/libctf/ctf-archive.c:24:10: fatal error: endian.h: No such file or directory
>>>  #include <endian.h>
>>>           ^~~~~~~~~~
>>>
>>> which you can find if you follow one of the "Full build" URLs in the other messages.
>>
>> Unfortunately, it's way worse than that:
>>
>> * <endian.h> isn't even needed at all: gdb master still compiles with
>>   that include removed.
>
> Uh... the ctf-archive.c warnings below are 90% down to the removal of
> this header. So I'd say it doesn't compile *well*. :)
>
> Does Solaris seriously not have this header? It's in Illumos, but I
> guess it's a later addition...

As even the Linux htole64(3) man page notes, this stuff is highly
unportable:

CONFORMING TO
       These  functions are nonstandard.  Similar functions are present on the
       BSDs, where the required  header  file  is  <sys/endian.h>  instead  of
       <endian.h>.  Unfortunately, NetBSD, FreeBSD, and glibc haven't followed
       the original OpenBSD naming convention for these functions, whereby the
       nn  component always appears at the end of the function name (thus, for
       example, in NetBSD, FreeBSD, and  glibc,  the  equivalent  of  OpenBSDs
       "betoh32" is "be32toh").

>> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:172:3: warning:
>> implicit declaration of function 'qsort_r'; did you mean 'qsort'?
>> [-Wimplicit-function-declaration]
>>   172 |   qsort_r ((ctf_archive_modent_t *) ((char *) archdr
>
> OK, I'll have to reimplement it (using qsort() would massively uglify
> the code).

The gnulib docs for qsort_r gives

Portability problems fixed by Gnulib:
@itemize
@item
This function has an incompatible API on some platforms:
FreeBSD 10.
@item
This function is missing on some platforms:
glibc 2.7, NetBSD 5.0, OpenBSD 3.8, Minix 3.1.8, AIX 7.1, HP-UX 11.31, IRIX 6.5,
 OSF/1 5.1, Solaris 11.4, Cygwin, mingw, MSVC 14, Interix 3.5, BeOS, Android 9.0

>> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function
>> 'ctf_arc_open_by_offset':
>> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:514:3: warning:
>> implicit declaration of function 'bzero'
>> [-Wimplicit-function-declaration]
>>   514 |   bzero (&ctfsect, sizeof (ctf_sect_t));
>>       |   ^~~~~
>
> I stripped almost all of these out, but it looks like one single one
> survived. It is gone now.

Thanks.

>> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c: In function
>> 'ctf_add_member_offset':
>> configure: loading cache ./config.cache
>> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c:1351:10: warning:
>> implicit declaration of function 'roundup'
>> [-Wimplicit-function-declaration]
>>  1351 |    off = roundup (off, NBBY) / NBBY;
>>       |          ^~~~~~~
>> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c:1352:24: warning:
>> implicit declaration of function 'MAX' [-Wimplicit-function-declaration]
>>  1352 |    off = roundup (off, MAX (malign, 1));
>>       |                        ^~~
>
> Augh, roundup() is a glibc-specific extension? (But this is as old as
> the import from Solaris, so it was clearly a Solaris extension once,
> too.)

On Solaris, it lives in <sys/sysmacros.h>, but even in /usr/include it's
almost exclusively used in lowlevel system headers (<nfs/*.h>, <vm/*.h>,
<sys/*.h>).

	Rainer
Nick Alcock May 29, 2019, 2:18 p.m. UTC | #4 
On 29 May 2019, Rainer Orth uttered the following:

> Nix <nix@esperi.org.uk> writes:
>> Does Solaris seriously not have this header? It's in Illumos, but I
>> guess it's a later addition...
>
> As even the Linux htole64(3) man page notes, this stuff is highly
> unportable:

I've implemented my own in terms of swap.h. It was only about five
lines. :)

>> OK, I'll have to reimplement it (using qsort() would massively uglify
>> the code).
>
> The gnulib docs for qsort_r gives

... and I've taken an implementation from gnulib. This seems to be an
acceptable source for this stuff. (I'd change it to use globals instead,
but it would be going in the opposite direction to the one I'd like to
go in, of increasing threadsafety, since the linker plugin might end up
multithreading if it turns out that parallelization helps.)

>>> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-create.c:1352:24: warning:
>>> implicit declaration of function 'MAX' [-Wimplicit-function-declaration]
>>>  1352 |    off = roundup (off, MAX (malign, 1));
>>>       |                        ^~~
>>
>> Augh, roundup() is a glibc-specific extension? (But this is as old as
>> the import from Solaris, so it was clearly a Solaris extension once,
>> too.)
>
> On Solaris, it lives in <sys/sysmacros.h>, but even in /usr/include it's
> almost exclusively used in lowlevel system headers (<nfs/*.h>, <vm/*.h>,
> <sys/*.h>).

Honestly it's a one-liner. Easy enough to reimplement. :)

(And part of the portability push for this many years ago did indeed
strip out a reference to <sys/sysmacros.h>. All is explained.)

Testing a fix for all of these annoying problems now. Sorry to spoil your
day with broken builds :(
Nix May 29, 2019, 4:04 p.m. UTC | #5 
On 29 May 2019, Rainer Orth told this:
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_arc_bufopen':
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:320:33: warning: comparison is always true due to limited range of data type [-Wtype-limits]
>   320 |   if (le64toh (arc->ctfa_magic) != CTFA_MAGIC)
>       |                                 ^~
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c: In function 'ctf_arc_open_internal':
> /vol/src/gnu/gdb/hg/master/local/libctf/ctf-archive.c:357:33: warning: comparison is always true due to limited range of data type [-Wtype-limits]
>   357 |   if (le64toh (arc->ctfa_magic) != CTFA_MAGIC)

*Thank* you for this. This revealed significant signedness problems on
32-bit platforms with long <= 32 bits in length, all of which I have now
crushed, to the net benefit of the codebase. :)

Running tests and writing changelogs now.
diff mbox

Patch

diff --git a/libctf/ctf-archive.c b/libctf/ctf-archive.c
--- a/libctf/ctf-archive.c
+++ b/libctf/ctf-archive.c
@@ -21,7 +21,6 @@ 
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <elf.h>
-#include <endian.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
diff --git a/libctf/ctf-impl.h b/libctf/ctf-impl.h
--- a/libctf/ctf-impl.h
+++ b/libctf/ctf-impl.h
@@ -21,7 +21,7 @@ 
 #define	_CTF_IMPL_H
 
 #include "config.h"
-#include <sys/errno.h>
+#include <errno.h>
 #include <ctf-api.h>
 #include <sys/types.h>
 #include <stdlib.h>