Re: [PATCH 2/3] gdb, gdbserver, gdbsupport: include config.h files with -include

[Simon Marchi <simark@simark.ca>]

On 3/20/24 16:32, Pedro Alves wrote:
> On 2024-03-18 20:01, Simon Marchi wrote:
>> The motivation for this change is for analysis tools and IDEs to be
>> better at analyzing header files on their own.
>>
>> There are some definitions and includes we want to occur at the very
>> beginning of all translation units.  The way we currently do that is by
>> requiring all source files (.c and .cc files) to include one of defs.h
>> (for gdb), server.h (for gdbserver) of common-defs.h (for gdbsupport and
>> shared source files).  These special header files define and include
>> everything that needs to be included at the very beginning.  Other
>> header files are written in a way that assume that these special
>> "prologue" header files have already been included.
>>
>> My problem with that is that my editor (clangd-based) provides a very
>> bad experience when editing source files.  
> 
> I think you meant "when editing header files."

Oops, yes.  Fixed.

>> Since clangd doesn't know
>> that one of defs.h/server.h/common-defs.h was included already, a lot of
>> things are flagged as errors.  For instance, CORE_ADDR is not known.
>> It's possible to edit the files in this state, but a lot of the power of
>> the editor is unavailable.
>>
>> My proposal to help with this is to include those things we always want
>> to be there using the compilers' `-include` option.  Tom Tromey said
>> that the current approach might exist because not all compilers used to
>> have an option like this.  But I believe that it's safe to assume they
>> do today.
>>
>> With this change, clangd picks up the -include option from the compile
>> command, and is able to analyze the header file correctly, as it sees
>> all that stuff included or defined but that -include option.  That works
>> because when editing a header file, clangd tries to get the compilation
>> flags from a source file that includes said header file.
>>
>> This change is a bit, because it addresses one of my frustrations when
> 
> This change is a bit ______? (fill in missing word).

Oops again.  I meant "This change is a bit self-serving".

>> editing header files, but it might help others too.  I'd be curious to
>> know if others encounter the same kinds of problems when editing header
>> files.  Also, even if the change is not necessary by any means, I think
>> the solution of using -include for stuff we always want to be there is
>> more elegant than the current solution.
>>
>> Even with this -include flag, many header files currently don't include
>> what they use, but rather depend on files included before them.  This
>> will still cause errors when editing them, but it should be easily
>> fixable by adding the appropriate include.  There's no rush to do so, as
>> long as the code still copiles, it's just a convenience thing.
> 
> copiles -> compiles

Fixed.

> Note there is a make rule to check whether gdb headers are standalone.  "make check-headers".
> Unfortunately, nobody ever runs that ( me included, after adding it a decade ago :-P ).
> Ideally, we'd fix all it flags and run it (or something like it) once in a while in a CI.
> (And same for gdbserver/gdbsupport, of course.)

Ah! I probably saw that in the past but forgot about it.  It might need
to be changed too, depending on what follows.

>> This patch does the small step of moving the inclusion of the various
>> config.h files to that new method.  The changes are:
>>
>>  - Add three files meant to be included with -include: gdb/gdb.inc.h,
>>    gdbserver/gdbserver.inc.h and gdbsupport/gdbsupport.inc.h.
>>  - Move the inclusion of the various config.h files there
>>  - Modify the compilation flags of all three subdirectories to add the
>>    appropriate -include option.
> 
> I'm surprised by the actual patch.  Why isn't this including defs.h/common-defs.h?  There are
> surely things defined in those files that need to be defined in headers too.  Why create
> this divergence?  I'd think that we would include defs.h/common-defs.h in headers too, and
> then work on moving things out of defs.h/common-defs.h over time.

I am not sure I understand what you mean.  If a given header file uses
something defined in defs.h, then it should include defs.h.  Otherwise
it doesn't need to.  Maybe if you give a concrete example I will get
your point better.

Simon