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

[Pedro Alves <pedro@palves.net>]

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.