[RFC] Sort #includes in gdb

[RFC] Sort #includes in gdb

[Tom Tromey]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

As discussed previously on the list, here is the patch to sort
includes in gdb.  Include files are put into up to 4 stanzas; stanzas
are separated by newlines.  The stanzas are:

1. For non-header files, the "introductory" headers.  First either
   defs.h, common/common-defs.h, or server.h, depending on the
   location of the source file.  Next, if the .c file has a
   corresponding .h, that .h.  (There is one exception to this rule,
   thread-iter.c.)

2. System header files, which are determined by the use of <> in the
   include.

3. Header files that are part of binutils-gdb but not in the gdb
   subdirectory -- mostly things coming from libiberty or BFD.

4. gdb header files.


I've attached the patch compressed, as it is very large.
Even the ChangeLog (which was auto-generated) is large; perhaps it
should be replaced by something like "All files: sort headers"?

Should the stanzas have introductory comments?
Because the patch is auto-generated, this would be easy to do.


If you want to try the script yourself, it is here:

    https://github.com/tromey/gdb-refactoring-scripts

Run it like:

    cd $srcdir/gdb
    emacs --script ~/gdb/gdb-refactoring-scripts/rewriter.el includes


I did not push this to the buildbot, as I believe it is too large for
that as well.  If you want to try building it without running the
script, it is the branch "submit/sort-includes" in my github.

I did test that the script produces the same output if run twice.
(Actually it has a buglet where it still updates the ChangeLog the
second time, but I didn't feel like fixing this.)

Let me know what you think.  This does have some possibility of breaking
the build.

Tom


[-- Attachment #2: sort includes patch --]
[-- Type: application/gzip, Size: 104002 bytes --]

Re: [RFC] Sort #includes in gdb

[Joel Brobecker]

> As discussed previously on the list, here is the patch to sort
> includes in gdb.  Include files are put into up to 4 stanzas; stanzas
> are separated by newlines.  The stanzas are:
> 
> 1. For non-header files, the "introductory" headers.  First either
>    defs.h, common/common-defs.h, or server.h, depending on the
>    location of the source file.  Next, if the .c file has a
>    corresponding .h, that .h.  (There is one exception to this rule,
>    thread-iter.c.)
> 
> 2. System header files, which are determined by the use of <> in the
>    include.
> 
> 3. Header files that are part of binutils-gdb but not in the gdb
>    subdirectory -- mostly things coming from libiberty or BFD.
> 
> 4. gdb header files.

It might be FUD on my part, but with C & C++, I've always been very
nervous about changing the order of #include-s. This is because some
headers sometimes define the same macros with different values, and
so order can make a difference for those (without really knowing
which is more correct, if any, and whether we are in fact getting
the one that we should). And of course, this is all highly platform-
dependent.

I really like, however, the idea of overcoming that fear, and
evaluate in practice what the real impact of order is. But since
we are getting close to 8.3 branching, could we hold the actual
puth until after the 8.3 branch is created?

-- 
Joel

Re: [RFC] Sort #includes in gdb

[Pedro Alves]

Hi!

On 01/26/2019 03:40 PM, Tom Tromey wrote:

> I did not push this to the buildbot, as I believe it is too large for
> that as well.

ISTR that you could point the bot at some branch instead of a patch?

This would seem particularly useful for this patch, given
the potential for breaking native/host-only code.

> If you want to try building it without running the
> script, it is the branch "submit/sort-includes" in my github.
> 
> I did test that the script produces the same output if run twice.
> (Actually it has a buglet where it still updates the ChangeLog the
> second time, but I didn't feel like fixing this.)
> 
> Let me know what you think.  This does have some possibility of breaking
> the build.

Yeah, there's a likely chance that this will break some native builds -- there are
some headers that are (or used to be) order dependent.  I remember a small number of
patches over the years moving header include order particularly in the
architecture-specific Linux native files, around asm/foo.h, sys/foo.h, headers to
fix the build in some particular kernel/libc version.  ISTR <asm/ptrace.h>
as a particular trouble maker, but I could well be misremembering that one.
I wish I could point at actual code / comments or commits, but I'm not
finding much.  :-/

See this one in gdbserver/linux-arm-low.c at least:

 /* Don't include elf.h if linux/elf.h got included by gdb_proc_service.h.
    On Bionic elf.h and linux/elf.h have conflicting definitions.  */


The fix would be trivial of course, so it doesn't sound very alarming to
me.

BTW, skimming the patch I noticed that the script didn't move
this conditional include (HAVE_GETAUXVAL):

 --- a/gdb/gdbserver/linux-aarch64-ipa.c
 +++ b/gdb/gdbserver/linux-aarch64-ipa.c
 @@ -19,9 +19,11 @@
     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
  
  #include "server.h"
 +
 +#include <elf.h>
  #include <sys/mman.h>
 +
  #include "tracepoint.h"
 -#include <elf.h>
  #ifdef HAVE_GETAUXVAL
  #include <sys/auxv.h>
  #endif

but seems to have handled the conditional include in many other cases.

> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> 
>  #include "defs.h"
> +#include "ada-lang.h"
> +
> +#include <algorithm>
>  #include <ctype.h>
> +#include <sys/stat.h>

What do you think of having separate stanzas for C and C++ system includes?

So we'd have include sections similar to:

 #include "defs.h"
 #include "ada-lang.h"

 #include <ctype.h>
 #include <sys/stat.h>

 #include <algorithm>
 #include <map>
 #include <string>
 #include <vector>

 ...

C++ headers are easily detectable as those without a file extension (no .h).


A minor one: personally, I think I would prefer that all files from the
same directory were sorted together.  I.e., instead of:

 #include "c-lang.h"
 #include "cli/cli-utils.h"
 #include "common/byte-vector.h"
 #include "common/function-view.h"
 #include "common/gdb_vecs.h"
 #include "common/vec.h"
 #include "completer.h"
 #include "dictionary.h"
 #include "symfile.h"
 #include "symtab.h"

This would look clearer / more organized to me:

 #include "cli/cli-utils.h"
 #include "common/byte-vector.h"
 #include "common/function-view.h"
 #include "common/gdb_vecs.h"
 #include "common/vec.h"

 #include "c-lang.h"
 #include "completer.h"
 #include "dictionary.h"
 #include "symfile.h"
 #include "symtab.h"

The above shows directories before non-directory files, because
it seems like the include order goes along with the 
'more general -> more specific', or 'further away -> closer' order.
Something like:

 system headers
 "external" libraries/utilities (libiberty/BFD)
 "internal" libraries/utilities (common / nat / cli / arch)
 gdb/ headers

No firm opinion on comments vs no comments.

Thanks,
Pedro Alves

Re: [RFC] Sort #includes in gdb

[Matt Rice]

On Mon, Jan 28, 2019 at 11:08 AM Pedro Alves <palves@redhat.com> wrote:
>
> Hi!
>
> On 01/26/2019 03:40 PM, Tom Tromey wrote:
>
> > I did not push this to the buildbot, as I believe it is too large for
> > that as well.
>
> ISTR that you could point the bot at some branch instead of a patch?
>
> This would seem particularly useful for this patch, given
> the potential for breaking native/host-only code.
>
> > If you want to try building it without running the
> > script, it is the branch "submit/sort-includes" in my github.
> >
> > I did test that the script produces the same output if run twice.
> > (Actually it has a buglet where it still updates the ChangeLog the
> > second time, but I didn't feel like fixing this.)
> >
> > Let me know what you think.  This does have some possibility of breaking
> > the build.
>
> Yeah, there's a likely chance that this will break some native builds -- there are
> some headers that are (or used to be) order dependent.  I remember a small number of
> patches over the years moving header include order particularly in the
> architecture-specific Linux native files, around asm/foo.h, sys/foo.h, headers to
> fix the build in some particular kernel/libc version.  ISTR <asm/ptrace.h>
> as a particular trouble maker, but I could well be misremembering that one.
> I wish I could point at actual code / comments or commits, but I'm not
> finding much.  :-/

I remember the macros defining PTRACE_GETREGS et al, because they are
sometimes defined as an enum,
sometimes as a macro, and somewhere that one of those constants was
wrong due to a copy/paste error in an ancient version of glibc, so on
top of the enum vs define thing, there was some fixing it up, and that
fixing up worked somewhat until c++, type checking of enum vs int.  I
did a bit of looking through the history of glibc, but wasn't able to
find this wrongly defined constant.

here is at least a bit of stuff referencing the enum/macro constants mayhem.

https://reviews.llvm.org/D4366

Re: [RFC] Sort #includes in gdb

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> It might be FUD on my part, but with C & C++, I've always been very
Joel> nervous about changing the order of #include-s. This is because some
Joel> headers sometimes define the same macros with different values, and
Joel> so order can make a difference for those (without really knowing
Joel> which is more correct, if any, and whether we are in fact getting
Joel> the one that we should). And of course, this is all highly platform-
Joel> dependent.

Yeah, it's a concern; though I already pushed a series that fixes the
fallout from the first draft of this series.  Basically it was some
minor latent bugs.  I'll answer a bit more in reply to Pedro's note.

Joel> I really like, however, the idea of overcoming that fear, and
Joel> evaluate in practice what the real impact of order is. But since
Joel> we are getting close to 8.3 branching, could we hold the actual
Joel> puth until after the 8.3 branch is created?

Definitely.

Tom

Re: [RFC] Sort #includes in gdb

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> I did not push this to the buildbot, as I believe it is too large for
>> that as well.

Pedro> ISTR that you could point the bot at some branch instead of a patch?

I don't know but I will ask Sergio when he's back.

Pedro> Yeah, there's a likely chance that this will break some native builds -- there are
Pedro> some headers that are (or used to be) order dependent.  I remember a small number of
Pedro> patches over the years moving header include order particularly in the
Pedro> architecture-specific Linux native files, around asm/foo.h, sys/foo.h, headers to
Pedro> fix the build in some particular kernel/libc version.  ISTR <asm/ptrace.h>
Pedro> as a particular trouble maker, but I could well be misremembering that one.
Pedro> I wish I could point at actual code / comments or commits, but I'm not
Pedro> finding much.  :-/

One thing worth noting is that the rewriting script is not very
sophisticated: it understands #include and #if (to a limited extent) but
it does not actually handle comments.

So, something like:

   #include <z.h>
   /* whatever */
   #include <a.h>

... will not result in any reordering.

This is somewhat lame, and I didn't check to see how many files this
leaves un-re-written yet; but on the other hand it provides a simple out
when ordering is needed.

Pedro> BTW, skimming the patch I noticed that the script didn't move
Pedro> this conditional include (HAVE_GETAUXVAL):

I fixed this.

Pedro> What do you think of having separate stanzas for C and C++ system includes?

I implemented this.

Pedro> A minor one: personally, I think I would prefer that all files from the
Pedro> same directory were sorted together.  I.e., instead of:

I implemented this as well.

Pedro> No firm opinion on comments vs no comments.

For now at least, I'm sticking with "no comments" because the lack of
comment-handling in the script means that adding comments makes the
script non-idempotent.  But, idempotency is a good quality to have,
because it means the script can be re-run at any time to fix any
"regressions" that have crept in.

Tom

Re: [RFC] Sort #includes in gdb

[Tom Tromey]

Pedro> ISTR that you could point the bot at some branch instead of a patch?

Tom> I don't know but I will ask Sergio when he's back.

I think it can't be done.

Pedro> No firm opinion on comments vs no comments.

Tom> For now at least, I'm sticking with "no comments" because the lack of
Tom> comment-handling in the script means that adding comments makes the
Tom> script non-idempotent.  But, idempotency is a good quality to have,
Tom> because it means the script can be re-run at any time to fix any
Tom> "regressions" that have crept in.

I thought about this some more and I went back and implemented limited
comment-scanning, to make this work.  I think this is nicer because it
provides an in-source guide to developers saying where to add a new
#include.

I've appended the relevant bits from the rewritten objfiles.c.

Let me know what you think.

Tom

#include "defs.h"
#include "objfiles.h"

/* Standard C includes.  */
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>

/* Standard C++ includes.  */
#include <vector>

/* Local non-gdb includes.  */
#include "bfd.h"
#include "hashtab.h"

/* Local subdirectory includes.  */
#include "common/pathstuff.h"

/* Local includes.  */
#include "addrmap.h"
#include "arch-utils.h"
#include "bcache.h"
#include "block.h"
#include "breakpoint.h"
#include "btrace.h"
#include "complaints.h"
#include "dictionary.h"
#include "exec.h"
#include "expression.h"
#include "gdb-stabs.h"
#include "gdb_bfd.h"
#include "gdb_obstack.h"
#include "observable.h"
#include "parser-defs.h"
#include "psymtab.h"
#include "solist.h"
#include "source.h"
#include "symfile.h"
#include "symtab.h"
#include "target.h"

Re: [RFC] Sort #includes in gdb

[Pedro Alves]

On 03/15/2019 11:13 PM, Tom Tromey wrote:
> Pedro> ISTR that you could point the bot at some branch instead of a patch?
> 
> Tom> I don't know but I will ask Sergio when he's back.
> 
> I think it can't be done.

Isn't that what "try --branch" is for?

> 
> Pedro> No firm opinion on comments vs no comments.
> 
> Tom> For now at least, I'm sticking with "no comments" because the lack of
> Tom> comment-handling in the script means that adding comments makes the
> Tom> script non-idempotent.  But, idempotency is a good quality to have,
> Tom> because it means the script can be re-run at any time to fix any
> Tom> "regressions" that have crept in.
> 
> I thought about this some more and I went back and implemented limited
> comment-scanning, to make this work.  I think this is nicer because it
> provides an in-source guide to developers saying where to add a new
> #include.
> 
> I've appended the relevant bits from the rewritten objfiles.c.
> 
> Let me know what you think.
Seems fine to me.

BTW, I noticed that the ChangeLog said "Sort headers."
twice for every entry.

Thanks,
Pedro Alves

Re: [RFC] Sort #includes in gdb

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> I think it can't be done.

Pedro> Isn't that what "try --branch" is for?

No idea, but I guess the idea is to push a branch, then use "try" to do
a try run based on that branch?

I can try that.  I'm a little reluctant to push a temporary branch like this.
Will sourceware let me delete the branch when I'm done?

Tom

Re: [RFC] Sort #includes in gdb

[Simon Marchi]

On 2019-03-29 16:52, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> I think it can't be done.
> 
> Pedro> Isn't that what "try --branch" is for?
> 
> No idea, but I guess the idea is to push a branch, then use "try" to do
> a try run based on that branch?
> 
> I can try that.  I'm a little reluctant to push a temporary branch like 
> this.
> Will sourceware let me delete the branch when I'm done?
> 
> Tom

We can freely push, force-push and delete the "users/*" branches.  For 
example, I just deleted an old branch of mine that I didn't need 
anymore:

$ git push upstream :users/simark/autotools-bump
remote: 
----------------------------------------------------------------------
remote: --  The hooks.no-emails config option contains 
`refs/heads/users/.*',
remote: --  which matches the name of the reference being updated
remote: --  (refs/heads/users/simark/autotools-bump).
remote: --
remote: --  Commit emails will therefore not be sent.
remote: 
----------------------------------------------------------------------
To ssh://sourceware.org/git/binutils-gdb.git
  - [deleted]                   users/simark/autotools-bump


It would be fine if we did the cleanup in multiple chunks, it doesn't 
have to be one big patch.

I don't think I've mentioned it, but I really like this change.  I don't 
like the mental weight of having to choose where to add my include in an 
unorganized list :).

Simon

Re: [RFC] Sort #includes in gdb

[Tom Tromey]

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

Simon> It would be fine if we did the cleanup in multiple chunks, it doesn't
Simon> have to be one big patch.

Yeah, I may try that next.  That will require some hacks to my script.

I tried pushing my branch to sourceware and then using buildbot --branch
(with a trivial patch on top of the main one).  But, the builds all
report:

    *** Failed to update master GDB git repository.  The build can continue. ***

Tom

Re: [RFC] Sort #includes in gdb

[Pedro Alves]

Adding Sergio.
On 03/30/2019 06:34 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> It would be fine if we did the cleanup in multiple chunks, it doesn't
> Simon> have to be one big patch.
> 
> Yeah, I may try that next.  That will require some hacks to my script.
> 
> I tried pushing my branch to sourceware and then using buildbot --branch
> (with a trivial patch on top of the main one).  But, the builds all
> report:
> 
>     *** Failed to update master GDB git repository.  The build can continue. ***

Sergio, do you know whether this is something that could be easily fixed?

Thanks,
Pedro Alves

Re: [RFC] Sort #includes in gdb

[Pedro Alves]

On 03/29/2019 09:05 PM, Simon Marchi wrote:
> 
> I don't think I've mentioned it, but I really like this change.  I don't like the mental weight of having to choose where to add my include in an unorganized list :).

Me too.  I'm waiting for this to land before touching the "namespace gdb"
work again, since a not-insignificant part of that work revolves around
moving "#include"s in the middle of files to the top of the file.

Thanks,
Pedro Alves

Re: [RFC] Sort #includes in gdb

[Sergio Durigan Junior]

On Wednesday, April 03 2019, Pedro Alves wrote:

> Adding Sergio.
> On 03/30/2019 06:34 PM, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>> 
>> Simon> It would be fine if we did the cleanup in multiple chunks, it doesn't
>> Simon> have to be one big patch.
>> 
>> Yeah, I may try that next.  That will require some hacks to my script.
>> 
>> I tried pushing my branch to sourceware and then using buildbot --branch
>> (with a trivial patch on top of the main one).  But, the builds all
>> report:
>> 
>>     *** Failed to update master GDB git repository.  The build can continue. ***
>
> Sergio, do you know whether this is something that could be easily fixed?

Tom pinged me yesterday about this issue.  Sorry, I got distracted with
something else.  I'll take a look into this now.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [RFC] Sort #includes in gdb

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 03/29/2019 09:05 PM, Simon Marchi wrote:
>> 
>> I don't think I've mentioned it, but I really like this change.  I don't like the mental weight of having to choose where to add my include in an unorganized list :).

Pedro> Me too.  I'm waiting for this to land before touching the "namespace gdb"
Pedro> work again, since a not-insignificant part of that work revolves around
Pedro> moving "#include"s in the middle of files to the top of the file.

This patch doesn't touch those includes -- it only touches the ones at
the top of a file.  If you want to land yours first, it's no trouble for
me.  The change is just running a script.

On irc the other day, Simon had the idea that I could land the patch in
pieces: say, convert [a-f]*, test that, check it in.  I modified my
script to let me do this and the first batch went through.  So, if you'd
really rather this go first, let me know and I can do it that way.  It
may take a few days to get it all in.

Tom

Re: [RFC] Sort #includes in gdb

[Sergio Durigan Junior]

On Wednesday, April 03 2019, Pedro Alves wrote:

> Adding Sergio.
> On 03/30/2019 06:34 PM, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>> 
>> Simon> It would be fine if we did the cleanup in multiple chunks, it doesn't
>> Simon> have to be one big patch.
>> 
>> Yeah, I may try that next.  That will require some hacks to my script.
>> 
>> I tried pushing my branch to sourceware and then using buildbot --branch
>> (with a trivial patch on top of the main one).  But, the builds all
>> report:
>> 
>>     *** Failed to update master GDB git repository.  The build can continue. ***
>
> Sergio, do you know whether this is something that could be easily fixed?

I'm still investigating, but I think the "--branch" option doesn't
specify a branch to be tested; rather, it specified a branch where the
test should be performed.  I.e., you still have to provide a patch.  We
currently just support testing against origin/master (that's why, on
.buildbot/options, we specify the "try_branch" variable as being
"origin/master").

I'll keep investigating.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [RFC] Sort #includes in gdb

[Pedro Alves]

On 04/03/2019 09:11 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> On 03/29/2019 09:05 PM, Simon Marchi wrote:
>>>
>>> I don't think I've mentioned it, but I really like this change.  I don't like the mental weight of having to choose where to add my include in an unorganized list :).
> 
> Pedro> Me too.  I'm waiting for this to land before touching the "namespace gdb"
> Pedro> work again, since a not-insignificant part of that work revolves around
> Pedro> moving "#include"s in the middle of files to the top of the file.
> 
> This patch doesn't touch those includes -- it only touches the ones at
> the top of a file.  If you want to land yours first, it's no trouble for
> me.  The change is just running a script.
> 
> On irc the other day, Simon had the idea that I could land the patch in
> pieces: say, convert [a-f]*, test that, check it in.  I modified my
> script to let me do this and the first batch went through.  So, if you'd
> really rather this go first, let me know and I can do it that way.  It
> may take a few days to get it all in.
Nope, please don't hold up for me.  I don't have things ready enough.
I meant more that I'm waiting to rebase on top of your work before
continuing with the work of cleaning up the branch to bring it
to mergeable state.

Thanks,
Pedro Alves

Re: [RFC] Sort #includes in gdb

[Tom Tromey]

Sergio> I'm still investigating, but I think the "--branch" option doesn't
Sergio> specify a branch to be tested; rather, it specified a branch where the
Sergio> test should be performed.  I.e., you still have to provide a patch.  We
Sergio> currently just support testing against origin/master (that's why, on
Sergio> .buildbot/options, we specify the "try_branch" variable as being
Sergio> "origin/master").

Yeah, what I did is push my branch, then commit a trivial change and use
"buildbot try --branch origin/users/tromey/...".

I think it's fine to just test & land this patch in pieces.  Normally
patches aren't so big.

Tom

Re: [RFC] Sort #includes in gdb

[Sergio Durigan Junior]

On Wednesday, April 03 2019, Tom Tromey wrote:

> Sergio> I'm still investigating, but I think the "--branch" option doesn't
> Sergio> specify a branch to be tested; rather, it specified a branch where the
> Sergio> test should be performed.  I.e., you still have to provide a patch.  We
> Sergio> currently just support testing against origin/master (that's why, on
> Sergio> .buildbot/options, we specify the "try_branch" variable as being
> Sergio> "origin/master").
>
> Yeah, what I did is push my branch, then commit a trivial change and use
> "buildbot try --branch origin/users/tromey/...".

The problem with that is that BuildBot needs to know which branch to use
when comparing builds.  If you specify a branch that's not
origin/master, then there are no previous results to compare against.

Maybe it could work if you created a new branch, waited 1 hour or so
(because our BuildBot doesn't fetch directly from sourceware.org;
instead, it uses a mirror, so you have to wait until your branch
propagates), commited a trivial change, built it, then put the huge
patch on top of it, then put another trivial patch on top, and built it
again.  Not very cool.

> I think it's fine to just test & land this patch in pieces.  Normally
> patches aren't so big.

Again, sorry about the limitations of BuildBot.  I remember spending a
considerable amount of time trying to increase the patch size that's
accepted, but unfortunately the change would have to be propagated to
every buildslave, which is not feasible.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/