[PATCH] Include string.h in elf-bfd.h

[PATCH] Include string.h in elf-bfd.h

[Saagar Jha]

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]

elf-bfd.h uses functions from <string.h> but doesn’t include it, which causes problems for a configure test that uses it when compiled with a compiler that is picky about missing prototypes (such as macOS’s as of recently). This patch adds the missing include.


[-- Attachment #2: Include-string.h-in-elf-bfd.h.patch --]
[-- Type: application/octet-stream, Size: 1119 bytes --]

From cdeac45356a0020c6468d7ff38e966ccbd6eb636 Mon Sep 17 00:00:00 2001
From: Saagar Jha <saagar@saagarjha.com>
Date: Thu, 13 Aug 2020 22:09:52 -0700
Subject: [PATCH] Include string.h in elf-bfd.h

Some of the configure tests include just this header, and since it uses
functions from string they fail to compile and configure thinks that
binutils is missing ELF support.

bfd/ChangeLog:

	* bfd/elf-bfd.h: Include <string.h>.
---
 bfd/ChangeLog | 4 ++++
 bfd/elf-bfd.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 75bcf55316..26817c7e7a 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,7 @@
+2020-09-05  Saagar Jha  <saagar@saagarjha.com>
+
+	* bfd/elf-bfd.h: Include <string.h>.
+
 2020-09-04  Alan Modra  <amodra@gmail.com>
 
 	PR 26574
diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index eebdf9ac73..32ab3733be 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -26,6 +26,7 @@
 #include "elf/external.h"
 #include "elf/internal.h"
 #include "bfdlink.h"
+#include <string.h>
 
 #ifdef __cplusplus
 extern "C" {
-- 
2.28.0

Re: [PATCH] Include string.h in elf-bfd.h

[Nick Clifton]

Hi Saagar,

> elf-bfd.h uses functions from <string.h> but doesn’t include it, which causes problems for a configure test that uses it when compiled with a compiler that is picky about missing prototypes (such as macOS’s as of recently). This patch adds the missing include.

Which configure test(s) are causing this problem ?

The inclusion of <string.h> is actually handled by "sysdep.h", which
handles the correct selection of <string.h> vs <strings.h>.  No normal code
should be including elf-bfd.h without previously including that header.

Configure tests are special, since they are hand crafted, but to me this
sounds like a bug with the configure test, not the elf-bfd.h header.

Cheers
  Nick

Re: [PATCH] Include string.h in elf-bfd.h

[Saagar Jha]

Hi Nick,

This is the GDB configure test for ELF support: https://sourceware.org/git?p=binutils-gdb.git;a=blob;f=gdb/configure;h=e7811e807a6b42db1b28f46bdf43afb321f0ae05;hb=HEAD#l16820. It doesn’t include “sysdep.h” or <string(s).h> before “elf-bfd.h”, so it causes that check to fail and GDB to build without ELF support. I changed the header itself just in case something else was also including it without including other appropriate headers, but if it’s just this instance we can just include the right header in the test. Would that be “sysdep.h”?

Regards,
Saagar

> On Sep 7, 2020, at 03:04, Nick Clifton <nickc@redhat.com> wrote:
> 
> Hi Saagar,
> 
>> elf-bfd.h uses functions from <string.h> but doesn’t include it, which causes problems for a configure test that uses it when compiled with a compiler that is picky about missing prototypes (such as macOS’s as of recently). This patch adds the missing include.
> 
> Which configure test(s) are causing this problem ?
> 
> The inclusion of <string.h> is actually handled by "sysdep.h", which
> handles the correct selection of <string.h> vs <strings.h>.  No normal code
> should be including elf-bfd.h without previously including that header.
> 
> Configure tests are special, since they are hand crafted, but to me this
> sounds like a bug with the configure test, not the elf-bfd.h header.
> 
> Cheers
>  Nick
> 
> 

Re: [PATCH] Include string.h in elf-bfd.h

[Nick Clifton]

Hi Saagar,

> but if it’s just this instance we can just include the right header in the test. Would that be “sysdep.h”?

Yes.  I am assuming that at the point where this test is run, configure will have
completed in the bfd directory and that the bfd/config.h header will have been
created.  The bfd/sysdep.h header needs that header in order to correctly choose
between <string.h> and <strings.h>.

Cheers
  Nick

Re: [PATCH] Include string.h in elf-bfd.h

[Saagar Jha]

Perhaps I’m doing it wrong, but I stuck #include “sysdep.h" at the top of that configure test and it didn’t seem to like it:

configure:16833: gcc -o conftest -I./../include -I../bfd -I./../bfd -g -O2  -L../bfd -L../libiberty -L../zlib    conftest.c -lbfd -liberty -lz ../intl/libintl.a -liconv -lncurses -lm  >&5
In file included from conftest.c:171:
../bfd/sysdep.h:26:2: error: sysdep.h must be included in lieu of config.h
#error sysdep.h must be included in lieu of config.h
 ^
In file included from conftest.c:171:
In file included from ../bfd/sysdep.h:29:
../bfd/config.h:310:9: warning: 'PACKAGE' macro redefined [-Wmacro-redefined]
#define PACKAGE "bfd"
        ^
conftest.c:26:9: note: previous definition is here
#define PACKAGE "gdb"
        ^
In file included from conftest.c:171:
In file included from ../bfd/sysdep.h:29:
../bfd/config.h:316:9: warning: 'PACKAGE_NAME' macro redefined [-Wmacro-redefined]
#define PACKAGE_NAME "bfd"
        ^
conftest.c:2:9: note: previous definition is here
#define PACKAGE_NAME ""
        ^
In file included from conftest.c:171:
In file included from ../bfd/sysdep.h:29:
../bfd/config.h:319:9: warning: 'PACKAGE_STRING' macro redefined [-Wmacro-redefined]
#define PACKAGE_STRING "bfd 2.35.50"
        ^
conftest.c:5:9: note: previous definition is here
#define PACKAGE_STRING ""
        ^
In file included from conftest.c:171:
In file included from ../bfd/sysdep.h:29:
../bfd/config.h:322:9: warning: 'PACKAGE_TARNAME' macro redefined [-Wmacro-redefined]
#define PACKAGE_TARNAME "bfd"
        ^
conftest.c:3:9: note: previous definition is here
#define PACKAGE_TARNAME ""
        ^
In file included from conftest.c:171:
In file included from ../bfd/sysdep.h:29:
../bfd/config.h:328:9: warning: 'PACKAGE_VERSION' macro redefined [-Wmacro-redefined]
#define PACKAGE_VERSION "2.35.50"
        ^
conftest.c:4:9: note: previous definition is here
#define PACKAGE_VERSION ""
        ^
In file included from conftest.c:171:
../bfd/sysdep.h:185:11: fatal error: 'libintl.h' file not found
# include <libintl.h>
          ^~~~~~~~~~~
5 warnings and 2 errors generated.
configure:16833: $? = 1
configure: failed program was:
| /* confdefs.h */
| #define PACKAGE_NAME ""
| #define PACKAGE_TARNAME ""
| #define PACKAGE_VERSION ""
| #define PACKAGE_STRING ""
| #define PACKAGE_BUGREPORT ""
| #define PACKAGE_URL ""
| #define STDC_HEADERS 1
| #define HAVE_SYS_TYPES_H 1
| #define HAVE_SYS_STAT_H 1
| #define HAVE_STDLIB_H 1
| #define HAVE_STRING_H 1
| #define HAVE_MEMORY_H 1
| #define HAVE_STRINGS_H 1
| #define HAVE_INTTYPES_H 1
| #define HAVE_STDINT_H 1
| #define HAVE_UNISTD_H 1
| #define __EXTENSIONS__ 1
| #define _ALL_SOURCE 1
| #define _GNU_SOURCE 1
| #define _POSIX_PTHREAD_SEMANTICS 1
| #define _TANDEM_SOURCE 1
| #define HAVE_DLFCN_H 1
| #define HAVE_CXX11 1
| #define ENABLE_NLS 1
| #define PACKAGE "gdb"
| #define DEBUGDIR "/tmp/gdb/lib/debug"
| #define DEBUGDIR_RELOCATABLE 1
| #define BINDIR "/tmp/gdb/bin"
| #define GDB_DATADIR "/tmp/gdb/share/gdb"
| #define GDB_DATADIR_RELOCATABLE 1
| #define AUTO_LOAD_DIR ":${prefix}/share/auto-load"
| #define AUTO_LOAD_SAFE_PATH ":${prefix}/share/auto-load"
| #define DEFAULT_BFD_ARCH bfd_aarch64_arch
| #define DEFAULT_BFD_VEC aarch64_mach_o_vec
| #define HAVE_MONSTARTUP 1
| #define PKGVERSION "(GDB) "
| #define REPORT_BUGS_TO "<https://www.gnu.org/software/gdb/bugs/>"
| #define HAVE_LIBM 1
| #define HAVE_ICONV 1
| #define ICONV_CONST 
| #define SIZEOF_UNSIGNED_LONG_LONG 8
| #define SIZEOF_UNSIGNED_LONG 8
| #define SIZEOF_UNSIGNED___INT128 16
| #define JIT_READER_DIR "/tmp/gdb/lib/gdb"
| #define JIT_READER_DIR_RELOCATABLE 1
| #define HAVE_LIBEXPAT 1
| #define HAVE_XML_STOPPARSER 1
| #define WITH_PYTHON_PATH "/System/Library/Frameworks/Python.framework/Versions/2.7"
| #define PYTHON_PATH_RELOCATABLE 0
| #define HAVE_PYTHON 1
| #define WITH_PYTHON_LIBDIR "/System/Library/Frameworks/Python.framework/Versions/2.7/lib"
| #define PYTHON_LIBDIR_RELOCATABLE 0
| #define STDC_HEADERS 1
| #define HAVE_NLIST_H 1
| #define HAVE_SYS_FILE_H 1
| #define HAVE_SYS_FILIO_H 1
| #define HAVE_SYS_IOCTL_H 1
| #define HAVE_SYS_PARAM_H 1
| #define HAVE_SYS_RESOURCE_H 1
| #define HAVE_SYS_PTRACE_H 1
| #define HAVE_TERMIOS_H 1
| #define HAVE_SYS_USER_H 1
| #define HAVE_CURSES_H 1
| #define HAVE_NCURSES_H 1
| #define HAVE_TERM_H 1
| #define HAVE_SYS_SOCKET_H 1
| #define HAVE_LONG_LONG 1
| #define SIZEOF_LONG_LONG 8
| #define HAVE_DECL_BASENAME 0
| #define HAVE_DECL_FFS 1
| #define HAVE_DECL_ASPRINTF 1
| #define HAVE_DECL_VASPRINTF 1
| #define HAVE_DECL_SNPRINTF 1
| #define HAVE_DECL_VSNPRINTF 1
| #define HAVE_DECL_STRTOL 1
| #define HAVE_DECL_STRTOUL 1
| #define HAVE_DECL_STRTOLL 1
| #define HAVE_DECL_STRTOULL 1
| #define HAVE_DECL_STRVERSCMP 0
| #define HAVE_DECL_SNPRINTF 1
| #define HAVE_LC_MESSAGES 1
| #define HAVE_SOCKLEN_T 1
| #define HAVE_GETUID 1
| #define HAVE_GETGID 1
| #define HAVE_PIPE 1
| #define HAVE_PREAD 1
| #define HAVE_PWRITE 1
| #define HAVE_RESIZE_TERM 1
| #define HAVE_GETPGID 1
| #define HAVE_SETSID 1
| #define HAVE_SIGACTION 1
| #define HAVE_SIGSETMASK 1
| #define HAVE_SOCKETPAIR 1
| #define HAVE_WBORDER 1
| #define HAVE_WRESIZE 1
| #define HAVE_SETLOCALE 1
| #define HAVE_BTOWC 1
| #define HAVE_SETRLIMIT 1
| #define HAVE_GETRLIMIT 1
| #define HAVE_POSIX_MADVISE 1
| #define HAVE_WAITPID 1
| #define HAVE_USE_DEFAULT_COLORS 1
| #define HAVE_LANGINFO_CODESET 1
| #define HAVE_STDLIB_H 1
| #define HAVE_UNISTD_H 1
| #define HAVE_SYS_PARAM_H 1
| #define STDC_HEADERS 1
| #define HAVE_ALLOCA_H 1
| #define HAVE_ALLOCA 1
| #define HAVE_LANGINFO_CODESET 1
| #define HAVE_LOCALE_H 1
| #define HAVE_MEMORY_H 1
| #define HAVE_SIGNAL_H 1
| #define HAVE_SYS_RESOURCE_H 1
| #define HAVE_SYS_SOCKET_H 1
| #define HAVE_SYS_UN_H 1
| #define HAVE_SYS_WAIT_H 1
| #define HAVE_TERMIOS_H 1
| #define HAVE_DLFCN_H 1
| #define HAVE_POLL_H 1
| #define HAVE_SYS_POLL_H 1
| #define HAVE_SYS_SELECT_H 1
| #define HAVE_GETPAGESIZE 1
| #define HAVE_MMAP 1
| #define HAVE_FORK 1
| #define HAVE_VFORK 1
| #define HAVE_WORKING_VFORK 1
| #define HAVE_WORKING_FORK 1
| #define HAVE_GETRLIMIT 1
| #define HAVE_PIPE 1
| #define HAVE_POLL 1
| #define HAVE_SOCKETPAIR 1
| #define HAVE_SIGACTION 1
| #define HAVE_SBRK 1
| #define HAVE_SIGALTSTACK 1
| #define HAVE_SIGPROCMASK 1
| #define HAVE_SETPGID 1
| #define HAVE_SETPGRP 1
| #define HAVE_GETRUSAGE 1
| #define HAVE_DECL_ADDR_NO_RANDOMIZE 0
| #define HAVE_DECL_STRSTR 1
| #define HAVE_STRUCT_STAT_ST_BLOCKS 1
| #define HAVE_STRUCT_STAT_ST_BLKSIZE 1
| #define HAVE_PTHREAD_PRIO_INHERIT 1
| #define HAVE_PTHREAD_SIGMASK 1
| #define HAVE_PTHREAD_SETNAME_NP 1
| #define CXX_STD_THREAD 1
| #define HAVE_SIGSETJMP 1
| #define _STRUCTURED_PROC 1
| #define HAVE_SYS_PTRACE_H 1
| #define PTRACE_TYPE_RET int
| #define PTRACE_TYPE_ARG1 int
| #define PTRACE_TYPE_ARG3 caddr_t
| #define PTRACE_TYPE_ARG4 int
| #define SETPGRP_VOID 1
| #define USE_INCLUDED_REGEX 1
| #define PRINTF_HAS_LONG_LONG 1
| #define HAVE_LONG_DOUBLE 1
| #define PRINTF_HAS_LONG_DOUBLE 1
| #define SCANF_HAS_LONG_DOUBLE 1
| #define GDBINIT ".gdbinit"
| #define TARGET_SYSTEM_ROOT ""
| #define TARGET_SYSTEM_ROOT_RELOCATABLE 0
| #define SYSTEM_GDBINIT ""
| #define SYSTEM_GDBINIT_RELOCATABLE 0
| #define SYSTEM_GDBINIT_DIR ""
| #define SYSTEM_GDBINIT_DIR_RELOCATABLE 0
| /* end confdefs.h.  */
| #include <stdlib.h>
|   #include "sysdep.h"
|   #include "bfd.h"
|   #include "elf-bfd.h"
| 
| int
| main ()
| {
| return bfd_get_elf_phdr_upper_bound (NULL);
|   ;
|   return 0;
| }
configure:16841: result: no

Is there something else I should be doing?

> On Sep 9, 2020, at 05:07, Nick Clifton <nickc@redhat.com> wrote:
> 
> Hi Saagar,
> 
>> but if it’s just this instance we can just include the right header in the test. Would that be “sysdep.h”?
> 
> Yes.  I am assuming that at the point where this test is run, configure will have
> completed in the bfd directory and that the bfd/config.h header will have been
> created.  The bfd/sysdep.h header needs that header in order to correctly choose
> between <string.h> and <strings.h>.
> 
> Cheers
>  Nick
> 
> 

Re: [PATCH] Include string.h in elf-bfd.h

[Nick Clifton]

Hi Saagar,

> Perhaps I’m doing it wrong, but I stuck #include “sysdep.h" at the top of that configure test and it didn’t seem to like it:

> In file included from conftest.c:171:
> ../bfd/sysdep.h:26:2: error: sysdep.h must be included in lieu of config.h
> #error sysdep.h must be included in lieu of config.h

Ah yes.  For normal code "sysdep,h" is included and this will automatically
include "config.h" for you.  But with configure tests a lot of the defines
that are provided by config.h are already set up.  Like this:

> ../bfd/config.h:310:9: warning: 'PACKAGE' macro redefined [-Wmacro-redefined]
> conftest.c:26:9: note: previous definition is here
> #define PACKAGE "gdb"

So in order to include sysdep.h you must either stop configure from providing
all of these defines (probably not a good idea) or else undefine them before
including sysdep.h (also a bad idea) or...


> configure:16833: $? = 1
> configure: failed program was:
[...]
> | #define HAVE_STRING_H 1
> | #define HAVE_STRINGS_H 1

Give up on including sysdep.h, and instead copy the logic from that file that decides
which string header to include.  IE:

  #ifdef STRING_WITH_STRINGS
  #include <string.h>
  #include <strings.h>
  #else
  #ifdef HAVE_STRING_H
  #include <string.h>
  #else
  #ifdef HAVE_STRINGS_H
  #include <strings.h>
  #else
  extern char *strchr ();
  extern char *strrchr ();
  #endif
  #endif
  #endif

If you do this I would strongly urge you to add a comment as well, explaining that
the code has been copied from bfd/sysdep.h and that that header is not included
directly because it conflicts with the way that configure sets up its defines.

Cheers
  Nick

Re: [PATCH] Include string.h in elf-bfd.h

[Saagar Jha]

[-- Attachment #1: Type: text/plain, Size: 209 bytes --]

Hi Nick,

Sorry for the delay, I forgot to send the patch to the list ;) Here is an updated one that is largely based on your suggestions. Please let me know if it needs anything else.

Thanks,
Saagar


[-- Attachment #2: Include-string-headers-in-a-configure-test-for-GDB.patch --]
[-- Type: application/octet-stream, Size: 1905 bytes --]

From 0b6c348aff36defee70306eee64b52c9cba1138f Mon Sep 17 00:00:00 2001
From: Saagar Jha <saagar@saagarjha.com>
Date: Tue, 22 Sep 2020 23:52:18 -0700
Subject: [PATCH] Include string headers in a configure test for GDB

Some configure tests include elf-bfd.h by itself and fail because they
can't find prototypes for the string functions in it (which makes GDB
think BFD has no support for ELF files). Normally we'd include sysdep.h,
but for the case of this configure test it's easier to copy the relevant
parts out rather than deal with the redefinitions that would arise were
we to include it directly.

gdb/Changelog:

	* configure: Include string headers in a test that needed it.
---
 gdb/ChangeLog |  4 ++++
 gdb/configure | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ce9cef5f516..23a4e13b38f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2020-09-22  Saagar Jha  <saagar@saagarjha.com>
+
+	* configure: Include string headers in a test that needed it.
+
 2020-09-21  Tom Tromey  <tromey@adacore.com>
 
 	* sparc-tdep.c (sparc32_skip_prologue): Use
diff --git a/gdb/configure b/gdb/configure
index a8942ecbd5d..3345320a0fe 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -16771,6 +16771,24 @@ else
   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 /* end confdefs.h.  */
 #include <stdlib.h>
+// This configure test needs string functions, but bfd/sysdep.h isn't included
+// here so we need to copy/paste the relevant part into this test.
+#ifdef STRING_WITH_STRINGS
+#include <string.h>
+#include <strings.h>
+#else
+#ifdef HAVE_STRING_H
+#include <string.h>
+#else
+#ifdef HAVE_STRINGS_H
+#include <strings.h>
+#else
+extern char *strchr ();
+extern char *strrchr ();
+#endif
+#endif
+#endif
+
   #include "bfd.h"
   #include "elf-bfd.h"
 
-- 
2.28.0


[-- Attachment #3: Type: text/plain, Size: 1882 bytes --]



> On Sep 14, 2020, at 02:55, Nick Clifton <nickc@redhat.com> wrote:
> 
> Hi Saagar,
> 
>> Perhaps I’m doing it wrong, but I stuck #include “sysdep.h" at the top of that configure test and it didn’t seem to like it:
> 
>> In file included from conftest.c:171:
>> ../bfd/sysdep.h:26:2: error: sysdep.h must be included in lieu of config.h
>> #error sysdep.h must be included in lieu of config.h
> 
> Ah yes.  For normal code "sysdep,h" is included and this will automatically
> include "config.h" for you.  But with configure tests a lot of the defines
> that are provided by config.h are already set up.  Like this:
> 
>> ../bfd/config.h:310:9: warning: 'PACKAGE' macro redefined [-Wmacro-redefined]
>> conftest.c:26:9: note: previous definition is here
>> #define PACKAGE "gdb"
> 
> So in order to include sysdep.h you must either stop configure from providing
> all of these defines (probably not a good idea) or else undefine them before
> including sysdep.h (also a bad idea) or...
> 
> 
>> configure:16833: $? = 1
>> configure: failed program was:
> [...]
>> | #define HAVE_STRING_H 1
>> | #define HAVE_STRINGS_H 1
> 
> Give up on including sysdep.h, and instead copy the logic from that file that decides
> which string header to include.  IE:
> 
>  #ifdef STRING_WITH_STRINGS
>  #include <string.h>
>  #include <strings.h>
>  #else
>  #ifdef HAVE_STRING_H
>  #include <string.h>
>  #else
>  #ifdef HAVE_STRINGS_H
>  #include <strings.h>
>  #else
>  extern char *strchr ();
>  extern char *strrchr ();
>  #endif
>  #endif
>  #endif
> 
> If you do this I would strongly urge you to add a comment as well, explaining that
> the code has been copied from bfd/sysdep.h and that that header is not included
> directly because it conflicts with the way that configure sets up its defines.
> 
> Cheers
>  Nick
> 
> 

Re: [PATCH] Include string.h in elf-bfd.h

[Nick Clifton]

Hi Saagar,

> Sorry for the delay, I forgot to send the patch to the list ;) Here is an updated one that is largely based on your suggestions. Please let me know if it needs anything else.
 
Nope - it looks great to me.  I cannot approve it however, since it is a GDB patch, but I doubt if anyone will raise any objections once you submit it there.

Cheers
  Nick