Re: The defs.h / common-defs.h / server.h rule

Re: The defs.h / common-defs.h / server.h rule

[Andrew Burgess]

Simon Marchi <simon.marchi@polymtl.ca> writes:

> Hi all,
>
> We currently have a rule [1] saying that all .c/.cc file must include
> one of defs.h, common-defs.h or server.h, depending on where they are,
> and that no header should include them.
>
> I was wondering about why that rule exists, and if it is still relevant
> today.
>
> The reason I am asking is that I'm usually working on GDB in an editor
> that uses clangd behind the scenes, which is really good at showing
> accurate errors and warnings in real time.  Unfortunately, the
> experience is bad for headers when headers don't include what they use.
> Since headers in GDB don't include defs.h, but use a lot of things
> defined there (or in files included transitively), editing headers in
> GDB is really bad.
>
> Could we switch to having headers include what they use?

+1 from me.

>                                                           There might be
> some edge cases where we can't do that with header files used both in
> GDB and GDBserver context,

We might be able to solve these problems by moving the "whatever"
feature into a 'whatever.h' header and then having gdbserver/whatever.h
and gdb/whatever.h, then depending on how the file is being compiled
we'd pick up the correct version of the 'whatever.h' header.

Thought this might not help with your editor as I guess it's searching
the whole tree for header files, and would likely go with the first one
it found...  but ignoring you're editor for a moment, having this split
might avoid the need for the .c/.cc file to pull the header for us.

Thanks,
Andrew

>                             but it would still be nice to make the
> experience better for all header files used in only one (which is the
> majority).
>
> As an example, I'm attaching some before / after screenshots of what it
> looks like when editing objfiles.h before including defs.h at the top
> and after.
>
> Before someome says "it's your tools' fault, your tools are bad": I
> know.  There has been some requests [2] to improve clangd for
> non-self-contained headers.  Having a feature like that would help here,
> but I don't think I can expect it any time soon, and I don't have the
> skills required to do it.  OTOH, I think that GDB's style here is a bit
> odd and goes against the typical Good Practices (tm).
>
> Simon
>
> [1] https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Include_Files
> [2] https://github.com/clangd/clangd/issues/1534

Re: The defs.h / common-defs.h / server.h rule

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> We currently have a rule [1] saying that all .c/.cc file must include
Simon> one of defs.h, common-defs.h or server.h, depending on where they are,
Simon> and that no header should include them.

Simon> I was wondering about why that rule exists, and if it is still relevant
Simon> today.

Every source file has to include config.h first.  In other GNU-ish
projects this is done in a straightforward way, but gdb (like gcc) used
a kind of prologue header file instead.

Simon> Could we switch to having headers include what they use?

Do you mean just adding defs.h to every header, or something else?

Simon> Before someome says "it's your tools' fault, your tools are bad": I
Simon> know.

In addition to that, I wonder how to enforce this rule.
For example I'm not likely to start using clangd.
Or if clangd is changed, will the rule change?

Tom

Re: The defs.h / common-defs.h / server.h rule

[Tom Tromey]

Simon> Could we switch to having headers include what they use?

Tom> Do you mean just adding defs.h to every header, or something else?

To be clear here, I think adding defs.h (or whatever) to every header
seems relatively harmless.  So if that would help you, it seems ok.

Simon> Before someome says "it's your tools' fault, your tools are bad": I
Simon> know.

Tom> In addition to that, I wonder how to enforce this rule.
Tom> For example I'm not likely to start using clangd.
Tom> Or if clangd is changed, will the rule change?

... just that I suspect you'll end up having to remind me of the rule
from time to time.

Tom

Re: The defs.h / common-defs.h / server.h rule

[Simon Marchi]

On 2024-02-20 09:52, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> We currently have a rule [1] saying that all .c/.cc file must include
> Simon> one of defs.h, common-defs.h or server.h, depending on where they are,
> Simon> and that no header should include them.
> 
> Simon> I was wondering about why that rule exists, and if it is still relevant
> Simon> today.
> 
> Every source file has to include config.h first.  In other GNU-ish
> projects this is done in a straightforward way, but gdb (like gcc) used
> a kind of prologue header file instead.

Ok, so through defs.h, every source file in gdb/ for instance ends up
including:

 - gdbsupport/config.h
 - gnulib/config.h
 - gdb/config.h

Plus, in gdbsupport/common-defs.h (or transitively included) are some
definitions that we want all source files to see all the time.  For
instance, the stuff in poison.h, or our override of ATTRIBUTE_PRINTF.

I suppose the other classic way to do this is with an -include flag,
passed to the compiler?  Actually that might be a better way to solve my
problem.  In order to correctly analyze some headers in a standalone
way, it is indeed necessary for clangd to include the various config.h
files first (and perhaps other stuff from defs.h & co).  If we used
-include to include whatever we want all code to see all the time, then
clangd would see the -include flag in the compilation flags and include
those "prologue" headers when analyzing any other header.

I gave it a try just for fun, it seems to work nicely:

  - include config.h files with -include
    https://review.lttng.org/c/binutils-gdb/+/11857/1

  - move more things to gdbsupport.inc.h
    https://review.lttng.org/c/binutils-gdb/+/11858/2

I like this solution because (a) it helps my problem and (b) it makes
defs.h / common-defs.h / server.h no longer special.  All things that we
want all source files to see all the time are included by that -include
flag, so enforced by the build system, and for the rest, source files /
header files include what they need (e.g. if they don't need defs.h,
they don't have to include it).

defs.h & co pull in a ton of stuff that is not needed everywhere, so I
think it would be nice to gradually remove some includes from them, and
include things where they are really needed.  This could help build
times and avoid recompiling files unnecessarily.

I'm not sure my argument for such a change is very strong, but I would
be happy to see a change like that.

> Simon> Could we switch to having headers include what they use?
> 
> Do you mean just adding defs.h to every header, or something else?

That's what I thought at first (either defs.h, or whatever include files
they use).  But it's not possible to include defs.h / common-defs.h /
server.h in the header files that defs.h / common-defs.h / server.h
themselves include.  So for those include files, I would be stuck.

> Simon> Before someome says "it's your tools' fault, your tools are bad": I
> Simon> know.
> 
> In addition to that, I wonder how to enforce this rule.
> For example I'm not likely to start using clangd.
> Or if clangd is changed, will the rule change?

I don't think what I propose is really clangd specific.  I think it's
just to design headers that can be analyzed in a self-contained way, to
the extent possible, which I think is just good software design.

Do people use other editors / IDEs that give accurate compilation errors
in real time like clangd does?  How do they do for header files that
assume they've been included in some context?

Simon

Re: The defs.h / common-defs.h / server.h rule

[Tom Tromey]

Simon> I suppose the other classic way to do this is with an -include flag,
Simon> passed to the compiler?

I don't know if any projects do this, but traditionally it's not done by
GNU-ish programs, I guess because -include was GCC-specific, i.e., not
portable.

Simon> defs.h & co pull in a ton of stuff that is not needed everywhere, so I
Simon> think it would be nice to gradually remove some includes from them, and
Simon> include things where they are really needed.  This could help build
Simon> times and avoid recompiling files unnecessarily.

Yeah, that'd be great.  gdb's header files are a crazy mess.

Tom

Re: The defs.h / common-defs.h / server.h rule

[Simon Marchi]

On 2024-02-23 16:06, Tom Tromey wrote:
> Simon> I suppose the other classic way to do this is with an -include flag,
> Simon> passed to the compiler?
> 
> I don't know if any projects do this, but traditionally it's not done by
> GNU-ish programs, I guess because -include was GCC-specific, i.e., not
> portable.

Do you think it would be acceptable today?  As far as I know, people
only build gdb with gcc and clang.  But I checked other compilers I
know:

 - icc has -include (it uses the clang frontend now, I think)
 - xlc has -qinclude (although it seems like there's a version with a
   clang frontend as well which supports -include too)

So, in this day and age, I'd be surprised if anyone compiled gdb with a
compiler that doesn't have some equivalent of -include.

Simon

Re: The defs.h / common-defs.h / server.h rule

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> On 2024-02-23 16:06, Tom Tromey wrote:
Simon> I suppose the other classic way to do this is with an -include flag,
Simon> passed to the compiler?

>> I don't know if any projects do this, but traditionally it's not done by
>> GNU-ish programs, I guess because -include was GCC-specific, i.e., not
>> portable.

Simon> Do you think it would be acceptable today?  As far as I know, people
Simon> only build gdb with gcc and clang.

I've tried to think of a reason we shouldn't do it, but I haven't really
come up with anything compelling.

It's hard to really know if it would cause problems for someone.
However it seems to me that you could do the experiment.  Worst case,
we'd have to back it out any maybe add some #includes here and there.

Tom