[PATCH] Include string headers in a configure test for GDB

[PATCH] Include string headers in a configure test for GDB

[Saagar Jha]

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

(This patch originally started out on the binutils list, as I made changes there first to resolve this issue, but ended moving the fix to GDB: https://sourceware.org/pipermail/binutils/2020-September/113206.html)

This patch fixes an issue with a configure test for GD (for ELF support in BFD) that includes elf-bfd.h–which uses certain string functions–but does not include the relevant headers to make this work.


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

Re: [PATCH] Include string headers in a configure test for GDB

[Simon Marchi]

On 2020-09-23 11:54 p.m., Saagar Jha wrote:
> (This patch originally started out on the binutils list, as I made changes there first to resolve this issue, but ended moving the fix to GDB: https://sourceware.org/pipermail/binutils/2020-September/113206.html)
>
> This patch fixes an issue with a configure test for GD (for ELF support in BFD) that includes elf-bfd.h–which uses certain string functions–but does not include the relevant headers to make this work.
>

Hi,

For the future, can you try sending the patches using git-send-email?
That makes them much easier to respond to.

In the commit message, can you paste the error that the compiler gives
you, that makes the test fail?  This will give an idea of what functions
the compiler complains about.

About the patch itself: configure is a generated file, you can't modify
it directly.  I think that what you are trying to change comes from this
part of configure.ac:

    1763 # Add ELF support to GDB, but only if BFD includes ELF support.
    1764 GDB_AC_CHECK_BFD([for ELF support in BFD], gdb_cv_var_elf,
    1765                  [bfd_get_elf_phdr_upper_bound (NULL)], elf-bfd.h)


GDB_AC_CHECK_BFD is itself defined in gdb/acinclude.m4.  I think it
would be fine to add the required includes in GDB_AC_CHECK_BFD directly.

Note that GDB is built in C++, where I am pretty sure we are guaranteed
to have the functions you are looking for in "string.h".  So I think it
would be safe to just include "string.h" and not care about the others.
And this configure test should probably be written as C++, but that's
another story.

Simon

Re: [PATCH] Include string headers in a configure test for GDB

[Pedro Alves]

On 9/24/20 4:15 PM, Simon Marchi wrote:

> Note that GDB is built in C++, where I am pretty sure we are guaranteed
> to have the functions you are looking for in "string.h".  So I think it
> would be safe to just include "string.h" and not care about the others.
> And this configure test should probably be written as C++, but that's
> another story.

Also GDB uses gnulib, so string.h is definitely available when GDB sources
are built, either from the system, or as a gnulib replacement.  However,
we don't put the gnulib headers in the include path when running
configure tests, I think.  Maybe we should.

Re: [PATCH] Include string headers in a configure test for GDB

[Saagar Jha]

> On Sep 24, 2020, at 08:15, Simon Marchi <simark@simark.ca> wrote:

> For the future, can you try sending the patches using git-send-email?

Yes, of course. I actually chose to not use git-send-email because I thought the contribution checklist asked me to add patches as attachments, but now that I look at it again it sais to do either. Sorry about that.

> In the commit message, can you paste the error that the compiler gives
> you, that makes the test fail?

Sure. For reference, it’s

In file included from conftest.c:172:
../bfd/elf-bfd.h:3086:10: error: implicitly declaring library function 'strncmp' with type 'int (const char *, const char *, unsigned long)' [-Werror
,-Wimplicit-function-declaration]
  return strncmp (name, ".ctf", 4) == 0 && (name[4] == 0 || name[4] == '.');
         ^
../bfd/elf-bfd.h:3086:10: note: include the header <string.h> or explicitly provide a declaration for 'strncmp'

> On Sep 24, 2020, at 08:38, Pedro Alves <pedro@palves.net> wrote:
> 
> On 9/24/20 4:15 PM, Simon Marchi wrote:
> 
>> Note that GDB is built in C++, where I am pretty sure we are guaranteed
>> to have the functions you are looking for in "string.h".  So I think it
>> would be safe to just include "string.h" and not care about the others.
>> And this configure test should probably be written as C++, but that's
>> another story.
> 
> Also GDB uses gnulib, so string.h is definitely available when GDB sources
> are built, either from the system, or as a gnulib replacement.  However,
> we don't put the gnulib headers in the include path when running
> configure tests, I think.  Maybe we should.

What exactly would go into a C++ configure test? There’s not much code there so I’m not quite sure what you mean. For the suggestion to add gnulib–should I just set CXXFLAGS to include gnulib around this one test? Or were you suggesting that we should do this at the very top of the script?

Re: [PATCH] Include string headers in a configure test for GDB

[Simon Marchi]

Subject:   Re: [PATCH] Include string headers in a configure test for
GDB To:        Saagar Jha <saagar@saagarjha.com>, Pedro Alves
<pedro@palves.net> Cc:        gdb-patches@sourceware.org Bcc:
-=-=-=-=-=-=-=-=-=# Don't remove this line #=-=-=-=-=-=-=-=-=- On
2020-09-25 3:40 a.m., Saagar Jha wrote:
>> On Sep 24, 2020, at 08:15, Simon Marchi <simark@simark.ca> wrote:
>
>> For the future, can you try sending the patches using git-send-email?
>
> Yes, of course. I actually chose to not use git-send-email because I
> thought the contribution checklist asked me to add patches as
> attachments, but now that I look at it again it sais to do either.
> Sorry about that.

Ah, sorry about the confusion.  The checklist still says that we accept
patches as attachment, I think it encourages people do to that.  I
updated the checklist to say that patches must be sent using
git-send-email.

>
>> In the commit message, can you paste the error that the compiler
>> gives you, that makes the test fail?
>
> Sure. For reference, it’s
>
> In file included from conftest.c:172: ../bfd/elf-bfd.h:3086:10: error:
> implicitly declaring library function 'strncmp' with type 'int (const
> char *, const char *, unsigned long)' [-Werror
> ,-Wimplicit-function-declaration] return strncmp (name, ".ctf", 4) ==
> 0 && (name[4] == 0 || name[4] == '.'); ^ ../bfd/elf-bfd.h:3086:10:
> note: include the header <string.h> or explicitly provide a
> declaration for 'strncmp'

Thanks.

>> On Sep 24, 2020, at 08:38, Pedro Alves <pedro@palves.net> wrote:
>>
>> On 9/24/20 4:15 PM, Simon Marchi wrote:
>>
>>> Note that GDB is built in C++, where I am pretty sure we are
>>> guaranteed to have the functions you are looking for in "string.h".
>>> So I think it would be safe to just include "string.h" and not care
>>> about the others.  And this configure test should probably be
>>> written as C++, but that's another story.
>>
>> Also GDB uses gnulib, so string.h is definitely available when GDB
>> sources are built, either from the system, or as a gnulib
>> replacement.  However, we don't put the gnulib headers in the include
>> path when running configure tests, I think.  Maybe we should.
>
> What exactly would go into a C++ configure test? There’s not much code
> there so I’m not quite sure what you mean. For the suggestion to add
> gnulib–should I just set CXXFLAGS to include gnulib around this one
> test? Or were you suggesting that we should do this at the very top of
> the script?

When I meant it should be "written as C++", I meant that it should be
"compiled as C++".  The code of the test itself probably doesn't need to
be changed (or very little), rather we need to ask autoconf to use $CXX
instead of $CC to compile the test case.  But at the end of the day it
doesn't change much, as the point of the test is only to check if BFD
was compiled with ELF support, not to check if BFD's ELF support if
usable from C++.

For gnulib, yes: I think it would mean adding the relevant -I in the
C(XX)FLAGS while the test runs.  I don't know what would be the best way
to do this though.

In any case, I don't think GDB targets any "exotic" architecture where
strncmp wouldn't be provided bu string.h.  So I think we can keep it
simple and just include string.h.

Simon