[v2,3/4] gdb, gdbserver, gdbsupport: include early header files with `-include`

  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 header files.  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 self-serving, because it addresses one of my
frustrations when 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 compiles, it's just a convenience thing.

The changes are:

 - Add the appropriate `-include` option to the various Makefiles.

 - There is one problem for gdbserver's Makefile: we do not want to
   include server.h when building `gdbreplay.o`.  So we can't simply put
   the `-include` in `INTERNAL_CFLAGS`.  I added the `-include` option
   to the `COMPILE` and `IPAGENT_COMPILE` variables, and added a special
   rule to compile `gdbreplay.o`.

 - Remove the `-include` option from the `check-headers` rule, since it
   is already included in `INTERNAL_CFLAGS`.

Change-Id: If3e345d00a9fc42336322f1d8286687d22134340
---
 gdb/Makefile.in        |  3 ++-
 gdbserver/Makefile.in  | 11 +++++++++--
 gdbsupport/Makefile.am |  1 +
 gdbsupport/Makefile.in |  1 +
 4 files changed, 13 insertions(+), 3 deletions(-)
  

On 2024-03-23 02:14, Simon Marchi wrote:

> 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

but that -> by that


>  - There is one problem for gdbserver's Makefile: we do not want to
>    include server.h when building `gdbreplay.o`.  So we can't simply put
>    the `-include` in `INTERNAL_CFLAGS`.  I added the `-include` option
>    to the `COMPILE` and `IPAGENT_COMPILE` variables, and added a special
>    rule to compile `gdbreplay.o`.
> 

...


> diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
> index 45073abca493..4d29491a8721 100644
> --- a/gdbserver/Makefile.in
> +++ b/gdbserver/Makefile.in
> @@ -69,9 +69,11 @@ COMPILE.pre = $(CXX) $(CXX_DIALECT)
>  COMPILE.post = -c -o $@
>  POSTCOMPILE = @true
>  
> +INCLUDE_SERVER_H = -include $(srcdir)/server.h
> +

...

> +# Rule for gdbreplay.o.  This is the same as COMPILE, without INCLUDE_SERVER_H.
> +
> +gdbreplay.o: gdbreplay.cc
> +	$(ECHO_CXX) $(COMPILE.pre) $(INTERNAL_CFLAGS) $(CXXFLAGS) $(COMPILE.post) $<

It would seem better to make gdbreplay include common-defs.h with -include, to me.  Any
reason not to do that?
  

On 3/26/24 7:57 AM, Pedro Alves wrote:
> On 2024-03-23 02:14, Simon Marchi wrote:
> 
>> 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
> 
> but that -> by that

Fixed.

>>  - There is one problem for gdbserver's Makefile: we do not want to
>>    include server.h when building `gdbreplay.o`.  So we can't simply put
>>    the `-include` in `INTERNAL_CFLAGS`.  I added the `-include` option
>>    to the `COMPILE` and `IPAGENT_COMPILE` variables, and added a special
>>    rule to compile `gdbreplay.o`.
>>
> 
> ...
> 
> 
>> diff --git a/gdbserver/Makefile.in b/gdbserver/Makefile.in
>> index 45073abca493..4d29491a8721 100644
>> --- a/gdbserver/Makefile.in
>> +++ b/gdbserver/Makefile.in
>> @@ -69,9 +69,11 @@ COMPILE.pre = $(CXX) $(CXX_DIALECT)
>>  COMPILE.post = -c -o $@
>>  POSTCOMPILE = @true
>>  
>> +INCLUDE_SERVER_H = -include $(srcdir)/server.h
>> +
> 
> ...
> 
>> +# Rule for gdbreplay.o.  This is the same as COMPILE, without INCLUDE_SERVER_H.
>> +
>> +gdbreplay.o: gdbreplay.cc
>> +	$(ECHO_CXX) $(COMPILE.pre) $(INTERNAL_CFLAGS) $(CXXFLAGS) $(COMPILE.post) $<
> 
> It would seem better to make gdbreplay include common-defs.h with -include, to me.  Any
> reason not to do that?

No, I just didn't give much attention to gdbreplay because... I never
used it.

Really the simplest would be if gdbreplay could use the same -include as
the rest of gdbserver.  The immediate problem I got when using `-include
server.h` for gdbreplay.cc is:

      CXX    gdbreplay.o
    /home/smarchi/src/binutils-gdb/gdbserver/gdbreplay.cc:80:1: error: ‘void remote_close()’ was declared ‘extern’ and later ‘static’ [-fpermissive]
       80 | remote_close (void)
          | ^~~~~~~~~~~~
    In file included from /home/smarchi/src/binutils-gdb/gdbserver/server.h:94,
                     from <command-line>:
    /home/smarchi/src/binutils-gdb/gdbserver/remote-utils.h:36:6: note: previous declaration of ‘void remote_close()’
       36 | void remote_close (void);
          |      ^~~~~~~~~~~~

That is, if gdbreplay includes server.h, it sees some declaration of
remote_close() which conflicts with its own declaration of
remote_close().

One quick solution to that could be renaming gdbreplay's remote_close
function.  Or, put all of gdbreplay.cc (except main()) in an anonymous
namespace, which would be the appropriate C++ thing to do anyway.  Now,
if gdbreplay.cc uses `-include server.h`, it will "see" more things than
it needs to, but if it builds, it should work?  I guess?  And anyway a
subsequent goal will be to strip server.h to the bare minimum.
Ultimately, it should only contain just a few things that should be
suitable to both gdbserver and gdbreplay (include of
gdbsupport/common-defs.h, include of gdbserver/config.h and a few other
basic things).  Does that sound good to you?

Simon
  

On 2024-03-26 14:39, Simon Marchi wrote:
> On 3/26/24 7:57 AM, Pedro Alves wrote:
>> On 2024-03-23 02:14, Simon Marchi wrote:

>>
>>> +# Rule for gdbreplay.o.  This is the same as COMPILE, without INCLUDE_SERVER_H.
>>> +
>>> +gdbreplay.o: gdbreplay.cc
>>> +	$(ECHO_CXX) $(COMPILE.pre) $(INTERNAL_CFLAGS) $(CXXFLAGS) $(COMPILE.post) $<
>>
>> It would seem better to make gdbreplay include common-defs.h with -include, to me.  Any
>> reason not to do that?
> 
> No, I just didn't give much attention to gdbreplay because... I never
> used it.
> 
> Really the simplest would be if gdbreplay could use the same -include as
> the rest of gdbserver.  The immediate problem I got when using `-include
> server.h` for gdbreplay.cc is:
> 
>       CXX    gdbreplay.o
>     /home/smarchi/src/binutils-gdb/gdbserver/gdbreplay.cc:80:1: error: ‘void remote_close()’ was declared ‘extern’ and later ‘static’ [-fpermissive]
>        80 | remote_close (void)
>           | ^~~~~~~~~~~~
>     In file included from /home/smarchi/src/binutils-gdb/gdbserver/server.h:94,
>                      from <command-line>:
>     /home/smarchi/src/binutils-gdb/gdbserver/remote-utils.h:36:6: note: previous declaration of ‘void remote_close()’
>        36 | void remote_close (void);
>           |      ^~~~~~~~~~~~
> 
> That is, if gdbreplay includes server.h, it sees some declaration of
> remote_close() which conflicts with its own declaration of
> remote_close().
> 
> One quick solution to that could be renaming gdbreplay's remote_close
> function.  Or, put all of gdbreplay.cc (except main()) in an anonymous
> namespace, which would be the appropriate C++ thing to do anyway.  Now,
> if gdbreplay.cc uses `-include server.h`, it will "see" more things than
> it needs to, but if it builds, it should work?  I guess?  And anyway a
> subsequent goal will be to strip server.h to the bare minimum.
> Ultimately, it should only contain just a few things that should be
> suitable to both gdbserver and gdbreplay (include of
> gdbsupport/common-defs.h, include of gdbserver/config.h and a few other
> basic things).  Does that sound good to you?

You said a lot of different things, so I'm not sure which part I'd be agreeing with.  :-)

gdbreplay is a separate program that happens share a few bits of code with gdbserver, but
not that much.  It currently includes common-defs.h at the top of its source file.
So I think the really simplest is to make the rule for gdbreplay.o you were
adding use "-include common-defs.h".  Or add a new gdbreplay.h header that includes
common-defs.h and -include that one.

Anything else than that seems like a distraction at this point, and I wouldn't call those
other options simpler.  The look certainly more debatable to me.
  

On 3/26/24 11:31 AM, Pedro Alves wrote:
> You said a lot of different things, so I'm not sure which part I'd be agreeing with.  :-)

Sorry about that.

> gdbreplay is a separate program that happens share a few bits of code with gdbserver, but
> not that much.  It currently includes common-defs.h at the top of its source file.
> So I think the really simplest is to make the rule for gdbreplay.o you were
> adding use "-include common-defs.h".  Or add a new gdbreplay.h header that includes
> common-defs.h and -include that one.
> 
> Anything else than that seems like a distraction at this point, and I wouldn't call those
> other options simpler.  The look certainly more debatable to me.

Ok, I'll keep the special rule for gdbreplay.o and make it -include
common-defs.h.

It's a bit weird because gdbreplay is made of gdbreplay.o, utils.o and
version.o.  The former includes common-defs.h, while the last two
include server.h.  But it works like that today, and I don't need to
change it.

Simon