Re: [PATCH v2 0/6] Move gdbsupport to top level

[Simon Marchi <simark@simark.ca>]

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.

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

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

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

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.

Simon