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

[Simon Marchi <simark@simark.ca>]

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