[2/3] gdb, gdbserver, gdbsupport: include config.h 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 source 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, 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 copiles, it's just a convenience thing.

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.

Change-Id: If3e345d00a9fc42336322f1d8286687d22134340
---
 gdb/Makefile.in             |  1 +
 gdb/defs.h                  |  7 -------
 gdb/gdb.inc.h               | 22 ++++++++++++++++++++++
 gdbserver/Makefile.in       |  3 ++-
 gdbserver/gdbserver.inc.h   | 22 ++++++++++++++++++++++
 gdbserver/server.h          |  8 --------
 gdbsupport/Makefile.am      |  1 +
 gdbsupport/Makefile.in      | 16 ++++++++++++----
 gdbsupport/common-defs.h    | 10 ----------
 gdbsupport/gdbsupport.inc.h | 35 +++++++++++++++++++++++++++++++++++
 10 files changed, 95 insertions(+), 30 deletions(-)
 create mode 100644 gdb/gdb.inc.h
 create mode 100644 gdbserver/gdbserver.inc.h
 create mode 100644 gdbsupport/gdbsupport.inc.h
  

Am Montag, 18. März 2024 um 21:03:47 MEZ hat Simon Marchi <simon.marchi@efficios.com> Folgendes geschrieben:

> diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
> index 7c360aa413ef..db1eee88059a 100644
> --- a/gdbsupport/Makefile.am
> +++ b/gdbsupport/Makefile.am
> @@ -35,6 +35,7 @@ AM_CPPFLAGS = \
>     $(INCINTL) \
>     -I../bfd \
>     -I$(srcdir)/../bfd \
> +    -include $(srcdir)/gdbsupport.inc.h \
>     @LARGEFILE_CPPFLAGS@
>
> override CXX += $(CXX_DIALECT)
> diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
> index 6f8dacc157f9..9b1063f31ab3 100644
> --- a/gdbsupport/Makefile.in
> +++ b/gdbsupport/Makefile.in
> @@ -393,10 +393,18 @@ ACLOCAL_AMFLAGS = -I . -I ../config
> # from Automake, as gdbsupport uses AM_GNU_GETTEXT through
> # ZW_GNU_GETTEXT_SISTER_DIR, but doesn't have any translations (currently).
> SUBDIRS =
> -AM_CPPFLAGS = -I$(srcdir)/../include -I$(srcdir)/../gdb \
> -    -I../gnulib/import -I$(srcdir)/../gnulib/import \
> -    -I.. -I$(srcdir)/.. $(INCINTL) -I../bfd -I$(srcdir)/../bfd \
> -    @LARGEFILE_CPPFLAGS@
> +AM_CPPFLAGS = \
> +    -I$(srcdir)/../include \
> +    -I$(srcdir)/../gdb \
> +    -I../gnulib/import \
> +    -I$(srcdir)/../gnulib/import \
> +    -I.. \
> +    -I$(srcdir)/.. \
> +    $(INCINTL) \
> +    -I../bfd \
> +    -I$(srcdir)/../bfd \
> +    -include $(srcdir)/gdbsupport.inc.h \
> +    @LARGEFILE_CPPFLAGS@
>
> AM_CXXFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS)
> noinst_LIBRARIES = libgdbsupport.a

Shouldn't most of this hunk be in the 'one entry per line' commit?


Hannes
  

On 3/19/24 07:18, Hannes Domani wrote:
>  Am Montag, 18. März 2024 um 21:03:47 MEZ hat Simon Marchi <simon.marchi@efficios.com> Folgendes geschrieben:
> 
>> diff --git a/gdbsupport/Makefile.am b/gdbsupport/Makefile.am
>> index 7c360aa413ef..db1eee88059a 100644
>> --- a/gdbsupport/Makefile.am
>> +++ b/gdbsupport/Makefile.am
>> @@ -35,6 +35,7 @@ AM_CPPFLAGS = \
>>      $(INCINTL) \
>>      -I../bfd \
>>      -I$(srcdir)/../bfd \
>> +    -include $(srcdir)/gdbsupport.inc.h \
>>      @LARGEFILE_CPPFLAGS@
>>
>> override CXX += $(CXX_DIALECT)
>> diff --git a/gdbsupport/Makefile.in b/gdbsupport/Makefile.in
>> index 6f8dacc157f9..9b1063f31ab3 100644
>> --- a/gdbsupport/Makefile.in
>> +++ b/gdbsupport/Makefile.in
>> @@ -393,10 +393,18 @@ ACLOCAL_AMFLAGS = -I . -I ../config
>> # from Automake, as gdbsupport uses AM_GNU_GETTEXT through
>> # ZW_GNU_GETTEXT_SISTER_DIR, but doesn't have any translations (currently).
>> SUBDIRS =
>> -AM_CPPFLAGS = -I$(srcdir)/../include -I$(srcdir)/../gdb \
>> -    -I../gnulib/import -I$(srcdir)/../gnulib/import \
>> -    -I.. -I$(srcdir)/.. $(INCINTL) -I../bfd -I$(srcdir)/../bfd \
>> -    @LARGEFILE_CPPFLAGS@
>> +AM_CPPFLAGS = \
>> +    -I$(srcdir)/../include \
>> +    -I$(srcdir)/../gdb \
>> +    -I../gnulib/import \
>> +    -I$(srcdir)/../gnulib/import \
>> +    -I.. \
>> +    -I$(srcdir)/.. \
>> +    $(INCINTL) \
>> +    -I../bfd \
>> +    -I$(srcdir)/../bfd \
>> +    -include $(srcdir)/gdbsupport.inc.h \
>> +    @LARGEFILE_CPPFLAGS@
>>
>> AM_CXXFLAGS = $(WARN_CFLAGS) $(WERROR_CFLAGS)
>> noinst_LIBRARIES = libgdbsupport.a
> 
> Shouldn't most of this hunk be in the 'one entry per line' commit?

Ah yeah, I was missing regenerating gdbsupport/Makefile.in in the first
patch.  Fixed locally.  Thanks for pointing it out.

Simon
  

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

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


> 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


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


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

Pedro Alves
  

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
  

On 2024-03-21 02:11, Simon Marchi wrote:
> On 3/20/24 16:32, Pedro Alves wrote:
>> On 2024-03-18 20:01, Simon Marchi wrote:
>> 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.
> 

I think you're looking at this all backwards, TBH.  

Currently, defs.h (and common-defs.h/server.h...) is a special header, and we need to include it first.

We all agree that defs.h includes too much, and that several things in it should be moved to other headers, and included
only where they're needed.  If we imagine we've already done that, then defs.h is left with only the things that
need to be included by all source files, and all headers.

That's the end goals that seems super obvious to me.

Yet, your patch takes a different route, and creates yet another set of special headers, and moves some things there.

It then leaves us in a partial transition state, where it's very likely that some headers needed more things that are
included by defs.h, but they aren't included in the new magic header, nor in the said headers that needed the things.
I.e., it likely regresses "make check-headers" even further.  Some headers use bfd_vma for example.  Are they all including
libbfd.h explicitly or transitively?  I don't know.  Currently they get it from defs.h.  Some headers make use
of "enum block_enum", which is defined in defs.h today, so when you edit such headers you'll will not have that
enum defined.  Likewise probably other enums in defs.h.  We use gdb::array_view in a lot of headers, etc.

I really see no reason for the new headers, and switching to a different set of magic headers, and moving things around
from defs.h to the new headers.  It makes me a bit nervous even, as order of some things in defs.h (and friends) is important.
E.g., the GCC_GENERATED_STDINT_H thing.

What I'm saying is that the patch that I was expecting to see, was one that simply does one thing -- makes gdb use "-include" to
force-include defs.h everywhere (and common-defs.h/server.h, you get the point).  It's then just one single, atomic, change.  We go
from having to put defs.h at the top of source files, to not having to do that because we use "-include defs.h".  That's it.  Nothing more.
So defs.h is still the same special header, just the mechanism by which we include it changes.  The code that ends up compiled is
_exactly_ the same.  Documentation explaining that that is the special header doesn't have to change.

And your use case, editing the header, will end up with headers including _exactly_ what they include today when any source
file is compiled, not some subset of defs.h.  Any oddity with editing the file is an oddity normal compilation would have too.

And then, over time, we move things out of defs.h.  E.g., move to including array-view.h in headers that need it.  Move
the definitions of enum block_enum, enum user_selected_what_flag, etc. to some other headers that make more sense.  Move the inline
functions and function prototypes to other headers.  Etc.  But we take it from the top down.  Ultimately still end up with defs.h,
but a smaller defs.h.  

And at each step of the way, editing a header file always sees the exact same set of pre-included files/symbols as when the same header
is compiled normally.

Pedro Alves
  

On 2024-03-21 12:50, Pedro Alves wrote:
> And at each step of the way, editing a header file always sees the exact
>  same set of pre-included files/symbols as when the same header
> is compiled normally.

Let me clarify this.  Here I was generally referring to the rule that source files should include their
module header right after defs.h.  Like:

 foo.c:
 #include "defs.h"
 #include "foo.h"
 * other includes *

So with that, there's one compilation unit that compiles "foo.h" exactly the same as what clangd sees when editing foo.h.

That even enforces "include what you need" in foo.h (other than the things defs.h already includes, of course).

We don't do that presently in many places, but we should do it throughout.  IIRC, Tromey even had a series to
normalize that throughout the tree a few years ago.  I don't recall why it didn't land.
  

On 3/21/24 08:50, Pedro Alves wrote:
> On 2024-03-21 02:11, Simon Marchi wrote:
>> On 3/20/24 16:32, Pedro Alves wrote:
>>> On 2024-03-18 20:01, Simon Marchi wrote:
>>> 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.
>>
> 
> I think you're looking at this all backwards, TBH.  

Thanks a lot for the feedback.  TLDR, I agree with you.

> 
> Currently, defs.h (and common-defs.h/server.h...) is a special header, and we need to include it first.
> 
> We all agree that defs.h includes too much, and that several things in it should be moved to other headers, and included
> only where they're needed.  If we imagine we've already done that, then defs.h is left with only the things that
> need to be included by all source files, and all headers.
> 
> That's the end goals that seems super obvious to me.
> 
> Yet, your patch takes a different route, and creates yet another set of special headers, and moves some things there.

To be clear, after my series, defs.h is no longer special.  It just
happens that headers that need it rely on it having been included
before.  I didn't see it as an immediate problem (it still builds),
because there are a lot more instances of that problem anyway (cases
that make check-headers would catch).  I saw it as something we would
fix little by little.

> It then leaves us in a partial transition state, where it's very likely that some headers needed more things that are
> included by defs.h, but they aren't included in the new magic header, nor in the said headers that needed the things.
> I.e., it likely regresses "make check-headers" even further.  Some headers use bfd_vma for example.  Are they all including
> libbfd.h explicitly or transitively?  I don't know.  Currently they get it from defs.h.  Some headers make use
> of "enum block_enum", which is defined in defs.h today, so when you edit such headers you'll will not have that
> enum defined.  Likewise probably other enums in defs.h.  We use gdb::array_view in a lot of headers, etc.

Right, my series doesn't actually make it better when I look at header.
But it makes it so the errors make sense, and I can start fixing them.
Whereas right now, the errors given in the headers sometimes don't make
sense, because we're missing some magic things from the various config.h
files, for instance.

> I really see no reason for the new headers, and switching to a different set of magic headers, and moving things around
> from defs.h to the new headers.  It makes me a bit nervous even, as order of some things in defs.h (and friends) is important.
> E.g., the GCC_GENERATED_STDINT_H thing.

My hope was that we could identify in one go all the stuff that needs to
be included or defined early, and put it in a new, clean header.  But
of course that's risky, it looks like GCC_GENERATED_STDINT_H would
belong in the early include file too.

> What I'm saying is that the patch that I was expecting to see, was one that simply does one thing -- makes gdb use "-include" to
> force-include defs.h everywhere (and common-defs.h/server.h, you get the point).  It's then just one single, atomic, change.  We go
> from having to put defs.h at the top of source files, to not having to do that because we use "-include defs.h".  That's it.  Nothing more.
> So defs.h is still the same special header, just the mechanism by which we include it changes.  The code that ends up compiled is
> _exactly_ the same.  Documentation explaining that that is the special header doesn't have to change.

I think that makes sense.  It's clearly less risky than my approach.  It
has the advantage that editing headers should work fine right away.

> And your use case, editing the header, will end up with headers including _exactly_ what they include today when any source
> file is compiled, not some subset of defs.h.  Any oddity with editing the file is an oddity normal compilation would have too.
> 
> And then, over time, we move things out of defs.h.  E.g., move to including array-view.h in headers that need it.  Move
> the definitions of enum block_enum, enum user_selected_what_flag, etc. to some other headers that make more sense.  Move the inline
> functions and function prototypes to other headers.  Etc.  But we take it from the top down.  Ultimately still end up with defs.h,
> but a smaller defs.h.  
> 
> And at each step of the way, editing a header file always sees the exact same set of pre-included files/symbols as when the same header
> is compiled normally.

Ack, thanks!

Simon
  

On 3/22/24 10:55, Simon Marchi wrote:> Ack, thanks!
> 
> Simon

One question: files such as gdb/arch/arc.c currently include
gdbsupport/common-defs.h, since they are compiled separately in the gdb
and gdbserver context.  With my changes (even when I'll update my patch
to take the approach you suggest), the gdb-compiled version will use
`-include defs.h`, while the gdbserver-compiled version will use
`-include server.h`.  Both defs.h and server.h include common-defs.h, so
it works.  But is it a problem?  Should I modify the build system so
that these shared files use `-include gdbsupport/common-defs.h`?

Another (more involved) option would be to see if these files could get
moved to gdbsupport.

Simon
  

On 2024-03-22 15:08, Simon Marchi wrote:
> On 3/22/24 10:55, Simon Marchi wrote:> Ack, thanks!
>>
>> Simon
> 
> One question: files such as gdb/arch/arc.c currently include
> gdbsupport/common-defs.h, since they are compiled separately in the gdb
> and gdbserver context.  With my changes (even when I'll update my patch
> to take the approach you suggest), the gdb-compiled version will use
> `-include defs.h`, while the gdbserver-compiled version will use
> `-include server.h`.  Both defs.h and server.h include common-defs.h, so
> it works.  But is it a problem?  Should I modify the build system so
> that these shared files use `-include gdbsupport/common-defs.h`?

I think we can go either way at this point, I don't think it's a real problem unless
we move the files to gdbsupport/.  Using common-defs.h helps a little with avoiding adding
gdb-only or gdbserver-only code, but then OTOH, some of the files under gdb/arch/ already have
conditional compilation with #ifdef GDBSERVER, unfortunately, so we're not ready to move them
somewhere where they're compiled once.

> 
> Another (more involved) option would be to see if these files could get
> moved to gdbsupport.

Yeah.