[v2,0/6] Move gdbsupport to top level

From: Pedro Alves <palves@redhat.com>

  On 1/17/20 11:54 AM, Simon Marchi wrote:
> On 2020-01-16 9:29 a.m., Pedro Alves wrote:
>> On 1/16/20 4:23 AM, Simon Marchi wrote:
>>> On 2020-01-15 4:35 p.m., Pedro Alves wrote:
>>>> Here's an improved version, which fixes gdbserver's standalone
>>>> build, simplifies gdbsupport's config.h (there's no need for
>>>> #ifdef GDBSERVER stuff since gdbserver doesn't use gdbsupport 
>>>> as a library yet), and adds copyright/intro comments.
>>>
>>> I have to admit I'm a bit lost in the spaghetti of config.h inclusion.  I don't
>>> understand, for example, why GDB needs to include gdbsupport's config.  All the
>>> features that GDB needs are checked by its own configure script, aren't they?
>>
>> Good question.  I guess the idea is that instead of having all of gdb, gdbserver,
>> and gdbsupport share tests via gdbsupport/common.m4, having run them more
>> than once, we can run the tests only once by gdbsupport's configure, and then
>> include the support-config.h file.
> 
> Ok, but that's not what is done today, that's a possibility for the future, right?
> common.m4 defines a macro used by all three configure scripts, so each configure
> script ends up doing its own checks.

It's my guess, but to be sure, we'd need to hear from Tom.

> 
>>
>> I've given this another thought, and went through the process of thinking
>> that this more complicated patch isn't really necessary, and then concluded
>> that having it makes things more normalized between all the dirs.  But
>> we can simplify it -- don't need to generate the "trampoline" config.h.
>>
>> My initial thought that led to the more complicated patch was to make
>> sure that that "#include <config.h>" in gnulib's libc-config.h picks up
>> gnulib config.h.  But it's really harmless if that picks some other config.h
>> instead as long as we're sure that the gnulib config.h is also included
>> somehow.  Which we are sure of, from the fact that common-defs.h
>> is always included as first thing in every .c file.
>>
>> So while we're building gdbsupport, we don't have any config.h in the
>> include path other than the BFD one, and we hit the problem.  We could
>> add an empty gdbsupport/config/config.h file in the
>> source and add -Igdbsupport/config/ to gdbsupport's Makefile, or,
>> add gnulib's build dir to the include path, like in my original patch.
>>
>> So the original solution of adding the gnulib dir to the include path
>> isn't that bad, though I call it a hack.
> 
> I would have to see it, but it sounds like the simplest solution.

It was the first patch I posted, here:
 https://sourceware.org/ml/gdb-patches/2020-01/msg00419.html

> 
>> The gdbsupport/config/config.h approach is pedantically less of a hack
>> and more in line with the "solution" that we happen to have in gdb
>> and gdbserver's dirs currently, by chance.
>>
>> OK, so I tried that.  And then, I realized, well, if we have that
>> new config.h file, then we could as well move the gnulib config.h inclusion
>> to that file, like I was doing in the complicated patch.  Essentially
>> it's the same as the complicated patch, except the "trampoline" config.h
>> isn't generated in the build dir, but instead is put in the source tree.
>> We'd do the same to gdb and gdbserver, for consistency.
>>
>> (I actually named the new dirs "config.h", as there's already a config
>> dir under gdb.  It's more to the point, anyway.)
> 
> I think it's a bit confusing to have directories named "config.h", but not
> really a big deal.

I think (that if we do this path), it's a good name in the sense that
you find it when you go look for a config.h file in (e.g.) the gdb dir.
Naming it something else would hide it more.

OTOH, I figured that we don't really need a separate directory.
Putting it in the source dir (gdb/, gdbsuppor/, gdbserver/) works
just as well, since each dir's Makefile.in pulls in its own source
dir before other top level directories.  That results in a smaller
patch.

But, gosh makes me realize there's an even simpler solution...
Just rename support-config.h to config.h...  Do we need to call
it something else for some reason?  I couldn't find one.

> 
>>> The gdb-config.h above should be gdbserver-config.h.
>> Here's v2.  WDYT?  Which of all the approaches discussed would
>> you prefer?
> 
> Probably the solution that involves the least files and indirections.  But
> the one proposed in the latest patch is fine with me too.  It's set and
> forget, once it builds again we won't think about it, at least until the
> next time it breaks.

Note that we already kind of have the config.h indirections via common-defs.h,
where we include the multiple config.h files.  This just moves those
indirections to the file actually named config.h.

> The buildbot is struggling with this (sending many breakage emails), so I
> think you should choose one and push it.  Then we can cancel all the builds
> on the master branch until that commit.

Anyway, here's the simpler patch...    Let's call it approach #3.

Approach #1 - add -Ignulib
Approach #2 - add wrapper config.h files in the source dirs
Approach #3 - rename support-config.h -> config.h

From a9e200d48f028682372bf8ddb02a5b7d52c6f053 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 17 Jan 2020 13:09:39 +0000
Subject: [PATCH] Fix gdbsupport build

I'm seeing this on F27 (a clean build from scratch):

~~~~~~~~~~~~~~~~~~~~~~~~~~~
 make[3]: Entering directory '/home/pedro/brno/pedro/gdb/binutils-gdb/build/gdbsupport'
   CC       gdb_tilde_expand.o
 In file included from /home/pedro/gdb/binutils-gdb/src/gdbsupport/../gnulib/import/libc-config.h:33:0,
                  from ../gnulib/import/glob.h:544,
                  from /home/pedro/gdb/binutils-gdb/src/gdbsupport/gdb_tilde_expand.c:22:
 ../bfd/config.h:7:4: error: #error config.h must be #included before system headers
  #  error config.h must be #included before system headers
     ^~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~

libc-config.h, where it includes config.h, says:

~~~~~~~~~~~~~~~~~~~~~~~~~~~
 /* This is intended to be a good-enough substitute for glibc system
    macros like those defined in <sys/cdefs.h>, so that Gnulib code
    shared with glibc can do this as the first #include:

      #ifndef _LIBC
      # include <libc-config.h>
      #endif

    When compiled as part of glibc this is a no-op; when compiled as
    part of Gnulib this includes Gnulib's <config.h> and defines macros
    that glibc library code would normally assume.  */

 #include <config.h>
~~~~~~~~~~~~~~~~~~~~~~~~~~~

The issue is that that '#include <config.h>' picks up bfd's config.h
instead of gnulib's.

This problem doesn't trigger in the gdb dir because there we generate
config.h under that exact name so gnulib's libc-config.h ends up
picking gdb's config.h instead of gnulib.c and that ends up harmless.

In gdbsupport, the config.h file is really named support-config.h, so
that '#include <config.h>' in libc-config.h doesn't pick it like it
would if it had the conventional config.h name.

This patch fixes it by simply renaming gdbserver's support-config.h to
config.h

gdbsupport/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* configure.ac: Generate config.h instead of support-config.h.
	* common-defs.h: Include <gdbsupport/config.h> instead of
	<gdbsupport/support-config.h>.
	* Makefile.in: Regenerate.
	* configure: Regenerate.
---
 gdbsupport/Makefile.in   | 12 +++----
 gdbsupport/common-defs.h |  2 +-
 gdbsupport/configure     | 92 ++++++++++++++++++++++++------------------------
 gdbsupport/configure.ac  |  2 +-
 4 files changed, 54 insertions(+), 54 deletions(-)

base-commit: bf8e4b6c8144a687d5edc24eda1bf0a3687ce71e

[v2,0/6] Move gdbsupport to top level

Commit Message

Comments

Patch