888088 - #include statement ordering is all over the map

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

The SpiderMonkey style guide now specifies the proper order for #include statements, which we violate all over the place.

(Admittedly there are some corner cases that the style guide doesn't cover, such as #includes within #ifdef, and #inclusion of some weird files.  I'll come up with a reasonable policy for them when I add the order checking to the script in bug 880088.)

Fixing it should be a barrel o' laughs.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

As a heads up, I've started writing a python script to do this - right now it's mostly a lesson in python for me, but I've gotten it to the point where it can deal with stripping // and /**/ comments (admittedly a detail, but I don't want it stumbling over some commented out #ifdef later). I have some ideas for how to deal with (possibly nested) #ifdefs, but I'll have to see how these pan out.

Comment 2

[Nicholas Nethercote [inactive]]

Ok.  I've just put up a draft script in bug 880088, which doesn't do #include statement order checking.  It might be worth thinking about how your code could fit into that script.

If I were doing it myself I'd try to get away with a dumb line-by-line scanner rather than a proper parser.

Comment 3

[Nicholas Nethercote [inactive]]

Emanuel, since you've started looking at this I've assigned this bug to you.  Let me know if you're not happy with that.

If you want to add checking for the #ifndef wrapper in each header at the same time, feel free! :)

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

Sure, that's fine. I'll be busy until Thursday, and I'm not sure how far I'll get tonight, but it's starting to come together.

Comment 5

[Nicholas Nethercote [inactive]]

There's no great rush -- they've been out of order for a long time, they can remain that way for a little longer :)  And writing the script will be the easy/fun part;  then you'll have to actually put them all in order...

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

Ah, but I'm writing it to put them in order for us ;) Of course we'll have to check that it doesn't mess up any comments, but that should be much less work.

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: WIP output

Just to prove that I'm still working on this, here's some early output from the script :) This is just a quick dump of the includes, so it doesn't take preprocessor directives into account - placing these in the context of the actual file is my next step.

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Somewhat less broken WIP output

That one was a bit more broken than I intended. Here's fixed WIP output.

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Automatically generated WIP patch

Alright, after a marathon night of debugging, here's a patch automatically generated by the first version of the script! There are various details that the script simply can't deal with, so the output is a bit more verbose and pedantic than a human could do, but with a few exceptions the include logic should be equivalent to what was there before.

The exceptions are:
1) When a preprocessor directive covers the entire file, includes outside that scope are automatically pulled in. This is a simple heuristic to make the output for js/src/assembler/ less convoluted, but it might not be valid in all cases.
2) The prerequisite includes (-inl.h/inlines.h for .cpp files, .h for -inl.h/inlines.h files, if they exist) are always placed at the top of the file. In general the script avoids moving includes across #define and #undef statements since they might impact what is included, but this is an exception to that rule.
3) Some macros used in preprocessor directives might depend on earlier includes - the script doesn't know about that, so it will happily move the required include down to its lexicographically correct position. This could perhaps be solved by annotating the relevant includes with some sort of comment.

I'm holding off on posting the script itself until I've gotten some sleep and taken the time to do some general cleanup. I spent a lot of time fixing bugs last night, and that hasn't done the code clarity any favors.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 778957 [details] [diff] [review]
Automatically generated WIP patch

Review of attachment 778957 [details] [diff] [review]:
-----------------------------------------------------------------

I've taken a look at a tiny fraction of these files and it looks very much like what I expected, which is good.  Since I'll be the one landing these changes, here's what I'll do.

- I'll wait until tomorrow, to give you time to post the script and an updated patch, if necessary.

- Then I'll review and do any necessary changes myself.

- I'll split it up into a number of patches to make bisection easier later if there are any problems caused.

- I'll try server and land it.

Sound ok?  Thanks again for doing this!  It's not glamorous work, but it'll be really nice to have it fixed.

::: js/src/jscntxt.cpp
@@ +26,2 @@
>  # include <string>
>  #endif  // ANDROID

This is an interesting case.  The style guide is silent on this issue, but breaking up #ifdef blocks containing multiple #includes probably isn't the right way to go -- it's what a script would do, not a human.  Perhaps the rule for such block should be that the #includes within them should be ordered as normal, and should go in the position of the first #include.  E.g.

#include "jsfoo.h"
#ifdef ANDROID
#include "jsgoo.h"
#include "jstoo.h"
#endif
#include "jskoo.h"

::: js/src/jscompartment.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "jscompartmentinlines.h"

So our rule is to put jsfooinlines.h at the top of jsfoo.cpp, but this is a case where jsfoo.cpp doesn't actually *need* jsfooinlines.h.  Hmm.

::: js/src/jscpucfg.h
@@ +68,4 @@
>  # include <sys/types.h>
> +#endif
> +
> +#if !defined(JS_HAVE_ENDIAN_H) && defined(JS_HAVE_MACHINE_ENDIAN_H) && !defined(XP_OS2) && !defined(_WIN32) && !defined(_WIN64) && !defined(__APPLE__)

The changes in this file make me really nervous.  We shouldn't be fiddling with #ifdefs this much.  Sometimes bending the rules of the style guide is best, and this file is an example.

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

I'd like to update the script with your suggestion first, if I can. Aside from that, I think the output is correct (if not always ideal) and should hopefully only take a few manual tweaks to get it into a landable state. I'll try to get an early version of it up today so you can try it out, though I think it'll need a bit more work before it can be used as an automated tool (but I guess that's okay for now).

(In reply to Nicholas Nethercote [:njn] from comment #10)
> ::: js/src/jscntxt.cpp
> @@ +26,2 @@
> >  # include <string>
> >  #endif  // ANDROID
> 
> This is an interesting case.  The style guide is silent on this issue, but
> breaking up #ifdef blocks containing multiple #includes probably isn't the
> right way to go -- it's what a script would do, not a human.  Perhaps the
> rule for such block should be that the #includes within them should be
> ordered as normal, and should go in the position of the first #include.  E.g.
> 
> #include "jsfoo.h"
> #ifdef ANDROID
> #include "jsgoo.h"
> #include "jstoo.h"
> #endif
> #include "jskoo.h"

I'll have to think about how to integrate this.. I don't think it'll be that hard to do though - all it should take is a small pass to group includes with the same parents together. Potentially tougher are #else, #elif and nested #ifs, but we can expand on that later if necessary.

> ::: js/src/jscompartment.cpp
> @@ +3,5 @@
> >   * This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +#include "jscompartmentinlines.h"
> 
> So our rule is to put jsfooinlines.h at the top of jsfoo.cpp, but this is a
> case where jsfoo.cpp doesn't actually *need* jsfooinlines.h.  Hmm.

A potential advantage might be that jsfoo.cpp doesn't need to include anything that jsfooinlines.h includes, and jsfooinlines.h doesn't need to include anything that jsfoo.h includes. Sort of counter to IWYU, though.

> ::: js/src/jscpucfg.h
> @@ +68,4 @@
> >  # include <sys/types.h>
> > +#endif
> > +
> > +#if !defined(JS_HAVE_ENDIAN_H) && defined(JS_HAVE_MACHINE_ENDIAN_H) && !defined(XP_OS2) && !defined(_WIN32) && !defined(_WIN64) && !defined(__APPLE__)
> 
> The changes in this file make me really nervous.  We shouldn't be fiddling
> with #ifdefs this much.  Sometimes bending the rules of the style guide is
> best, and this file is an example.

Yeah, the problem is finding the right compromise. Right now the script goes all out on include ordering - it's possible to make it more conservative, but it might be hard to do without losing some of the more reasonable changes. What we could do is take its current output, fix up the extreme cases by hand, then make it more conservative for future runs.

Comment 12

[Nicholas Nethercote [inactive]]

But once the big reordering is done, the script won't be needed any more, right?  It'll still be good to have a script that checks ordering, but I suspect that can be substantially simpler than one that does reordering.  That's why I'm happy to fix up minor problems by hand rather than tweaking the script until it's perfect.

Comment 13

[Emanuel Hoogeveen [:ehoogeveen]]

Yes, that's true. It'd be interesting to see if we can apply this outside SM as well, though (eventually). I think all the infrastructure is in place, so it should just be a matter of applying the right include rules and finding a reviewer.

Comment 14

[Nicholas Nethercote [inactive]]

I suggest Ms2ger as a reviewer for Gecko.

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize include order across js/src/

I added grouping based on parents for includes of the same type (e.g. jsfoo.h and jsbar.h, or mozilla/foo.h and mozilla/bar.h). Then I also added a pass to ensure that comment lines (i.e. lines that contain only whitespace and a comment) match the parents of surrounding includes. Both of these only come up a few times, but I think it's a nice addition.

Other than those changes, which only affect a few files, this should be identical to the WIP patch, but I didn't go through all of it to make sure.

Comment 16

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Console output after applying script to js/src/

Here's the console output after applying the script to js/src/. This certainly isn't a comprehensive list of things the script might have gotten wrong, but it might help as an indication of what files to review more carefully.

Comment 17

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize includes script, v1

And finally, here's the current version of the script. It's still a bit rough, and there's more code duplication than I would like (parse_strip_comments, transcribe_comment and update_include in particular share most of their code), but it runs without errors on the entire tree (which is not to say all the output is necessarily correct, since I only checked js/src/).

Its command line syntax is as follows:
./sanitizeIncludes.py MODE PATH

... where MODE must be either 'check' or 'fix' (without the quotes), and PATH should be a relative or absolute path to a directory or a file. Processing is the same in both modes - the only difference between the two is that if MODE is check, the script displays a unified diff using Python's difflib module. If MODE is fix, it writes the processed input to the file directly.

Comment 18

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> ::: js/src/jscpucfg.h
> @@ +68,4 @@
> >  # include <sys/types.h>
> > +#endif
> > +
> > +#if !defined(JS_HAVE_ENDIAN_H) && defined(JS_HAVE_MACHINE_ENDIAN_H) && !defined(XP_OS2) && !defined(_WIN32) && !defined(_WIN64) && !defined(__APPLE__)
> 
> The changes in this file make me really nervous.  We shouldn't be fiddling
> with #ifdefs this much.  Sometimes bending the rules of the style guide is
> best, and this file is an example.

Good catch, I'll have to look into what's going on in this file - the output looks pretty suspicious. As is, it's probably better not to take any changes to it, but I think the script can probably do better (assuming the current output is even correct).

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 779512 [details] [diff] [review]
Sanitize include order across js/src/

Review of attachment 779512 [details] [diff] [review]:
-----------------------------------------------------------------

I'm most of the way through fixing up your previous patch for landing.  Lots of small changes.  More details about them later.

BTW, you haven't actually compiled the code after running your script, have you?  There are lots of problems to fix up.  In particular, some additional #includes are needed, and assembler/ is a huge mess -- it turns out the ordering of #includes relative to #ifdef ENABLE_ASSEMBLER statements is rather important.  Anyway, I'm working through them, and your patch was an excellent start that had 90% of the work already done.

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: (part 0) - Some minor #include fix-ups.

As a result of some #include moving-around in subsequent patches, I need to:

- #include "js/Utility.h" in HeapAPI.h, for JS_ASSERT.

- Change some |js::Zone| decls to |JS::Zone|.  I'm not sure how the former
  worked, since |js::Zone| doesn't appear to be declared anywhere.

- Explicitly qualify some |JS::Zone| uses.

Comment 21

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> BTW, you haven't actually compiled the code after running your script, have
> you?

Yeah, I figured I'd do that after completing the script, but since you offered to fix them up I just checked if the output *looked* sane.

> and assembler/ is a huge mess -- it turns out the
> ordering of #includes relative to #ifdef ENABLE_ASSEMBLER statements is
> rather important.

I was afraid of that. I'm guessing assembler/wtf/Platform.h has to be included outside that scope? I wasn't sure how to fix that short of adding an exception for that include, but maybe I should have.

I also found and fixed the problem that was messing up js/src/jscpucfg.h, which was related to how the script handled #define and #undef statements. I'll upload a new script with that fix after I finish debugging a somewhat obscure issue.

Comment 22

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize includes script, v1.1

This version of the script fixes the problem with js/src/jscpucfg.h and adds some tweaks for better include ordering. In particular, I made a new sorting function that penalizes more nested directories, so e.g. ion/IonAllocPolicy.h will now precede ion/arm/Lowering-arm.h, which lets it group files like ion/arm/Lowering-arm.h and ion/x64/Lowering-x64.h together.

I'll also attach the generated output, though I don't expect you to use it at this point. I'd like to keep it up to date with the script as an example.

Comment 23

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Automatically generated patch (v1.1)

Comment 24

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Sanitize include order in js/src/jscpucfg.h

Here's the updated output for js/src/jscpucfg.h - I think you'll agree it looks much saner (separated this out because I assume you ignored the earlier changes to this file).

Comment 25

[Nicholas Nethercote [inactive]]

Parts 0--9:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf993171121
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab30741c5654
https://hg.mozilla.org/integration/mozilla-inbound/rev/695e5051cad2
https://hg.mozilla.org/integration/mozilla-inbound/rev/055b807e6a4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1609288cc7aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/12b1fc69cf4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cdcc55c9dde
https://hg.mozilla.org/integration/mozilla-inbound/rev/20fa9344c91a
https://hg.mozilla.org/integration/mozilla-inbound/rev/58309e4d1700
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb768efd61a

Those patches fix all files except for those in ion/ (which I'll do soon) and assembler/ and yarr/ (which are still giving me trouble and I might not end up bothering with).

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4bf993171121
https://hg.mozilla.org/mozilla-central/rev/ab30741c5654
https://hg.mozilla.org/mozilla-central/rev/695e5051cad2
https://hg.mozilla.org/mozilla-central/rev/055b807e6a4b
https://hg.mozilla.org/mozilla-central/rev/1609288cc7aa
https://hg.mozilla.org/mozilla-central/rev/12b1fc69cf4f
https://hg.mozilla.org/mozilla-central/rev/4cdcc55c9dde
https://hg.mozilla.org/mozilla-central/rev/20fa9344c91a
https://hg.mozilla.org/mozilla-central/rev/58309e4d1700
https://hg.mozilla.org/mozilla-central/rev/2cb768efd61a

Comment 27

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 779669 [details]
Sanitize includes script, v1.1

Obsoleted by newer versions in bug 897553.

Comment 28

[Nicholas Nethercote [inactive]]

Part 10:
https://hg.mozilla.org/integration/mozilla-inbound/rev/973361ec4fb5

That one fixes all of ion/ except for ion/arm/, which was giving me difficulties and I will return to tomorrow.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/973361ec4fb5

Comment 30

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Automatically generated patch

Here's the latest generated output, using version 1.4 of the script and on top of the latest m-i.

Comment 31

[Nicholas Nethercote [inactive]]

Comment on attachment 781367 [details] [diff] [review]
Automatically generated patch

Review of attachment 781367 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, looks pretty good overall.  I see about five cases that I got wrong;  I'll fix them up.  Of the remainder:

- I haven't landed the ion/arm/ changes yet;  hopefully today.

- The system header changes all look like cases where we don't have a clear ordering due to #ifdefs.  I'm inclined to not touch those, to avoid causing any problems.

- Your script still messes up the common Ion idiom where we include platform-specific x86/x64/arm headers.

Next step is to implement order checking, which should be much simpler, and depends on bug 880088 (which is still awaiting review, sigh).  It should be much simpler than actually doing the rearranging.

::: js/src/gc/Memory.cpp
@@ +25,1 @@
>  #include "jswin.h"

So it turns out that jswin.h must precede psapi.h otherwise compile errors happen on Windows.

::: js/src/ion/BaselineCompiler.h
@@ +14,5 @@
>  #include "jsinfer.h"
>  
> +#if !defined(JS_CPU_X64) && !defined(JS_CPU_X86)
> +# include "ion/arm/BaselineCompiler-arm.h"
> +#endif

Still doing this ugly thing, huh? :)

::: js/src/ion/ExecutionModeInlines.h
@@ +8,5 @@
>  #define ion_ExecutionModeInlines_h
>  
>  #ifdef JS_ION
>  
> +#include "ion/ExecutionModeInlines.h"

#including itself?

::: js/src/ion/IonCaches.h
@@ +9,5 @@
>  
> +#ifdef JS_CPU_ARM
> +# include "ion/arm/Assembler-arm.h"
> +#endif
> +

Yeah, I put that in the wrong spot.

::: js/src/ion/LIR.h
@@ +17,5 @@
>  #include "ion/Bailouts.h"
>  #include "ion/InlineList.h"
>  #include "ion/IonAllocPolicy.h"
>  #include "ion/LOpcodes.h"
> +#include "ion/MIR.h"

Got that wrong too.

::: js/src/ion/shared/Assembler-x86-shared.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "ion/shared/Assembler-x86-shared.h"

So this actually causes problems.  Similarly for some of the other platform-specific ion headers.  Turns out that Assembler-{x86,x64}.h #include this one, and it shouldn't be #included anywhere else.

@@ -141,5 @@
>          return;
>  
>      if (runtime_->flusher() == this)
>          runtime_->setFlusher(NULL);
> -}

ORLY? O_o

This happens a couple of times.

::: js/src/ion/shared/IonFrames-x86-shared.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "ion/shared/IonFrames-x86-shared.h"

As above, this causes problems.

::: js/src/jsclone.cpp
@@ +32,5 @@
>  #include "mozilla/Endian.h"
>  #include "mozilla/FloatingPoint.h"
>  
>  #include "jsdate.h"
> +#include "jswrapper.h"

I got that wrong too.

::: js/src/jsmemorymetrics.cpp
@@ +13,5 @@
>  #include "jsscript.h"
>  
>  #include "ion/BaselineJIT.h"
>  #include "ion/Ion.h"
> +#include "js/MemoryMetrics.h"

I know it doesn't follow the usual pattern, but js/MemoryMetrics.h is actually the header for this module.  jsmemorymetrics.cpp should be renamed vm/MemoryMetrics.cpp.

::: js/src/prmjtime.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "prmjtime.h"
> +
> +#include "mozilla/MathAlgorithms.h"

I missed this.

@@ +18,4 @@
>  #include <string.h>
>  #include <time.h>
>  
> +#include "jslock.h"

And this.

::: js/src/vm/ForkJoin.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "vm/ForkJoin.h"

Missed this too.

::: js/src/vm/PropertyKey.cpp
@@ +9,4 @@
>  #include "jsatom.h"
>  #include "jscntxt.h"
>  
> +#include "js/PropertyKey.h"

Again, this is the module header for this .cpp files, so it should come first.

Comment 32

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Automatically generated patch

(In reply to Nicholas Nethercote [:njn] from comment #31)
> - Your script still messes up the common Ion idiom where we include
> platform-specific x86/x64/arm headers.

Can't do much about that - the script simply can't know that these macros are mutually exclusive, so it does the conservative thing and requires the preceding #if/#ifdef and #elif are negated. Of course, it could leave the ordering alone altogether, but I'm not sure that's always desirable.

> > +#if !defined(JS_CPU_X64) && !defined(JS_CPU_X86)
> > +# include "ion/arm/BaselineCompiler-arm.h"
> > +#endif
> 
> Still doing this ugly thing, huh? :)

Same deal as above - just negating the #if further down ;)

> > +#include "ion/ExecutionModeInlines.h"
> 
> #including itself?

Fixed. This was because of the upper case Inlines.h - took a bit of refactoring to make it not case sensitive.

> > -}
> 
> ORLY? O_o
> 
> This happens a couple of times.

Good catch, fixed! This was an oversight introduced by a fix for a different problem (and has nothing to do with the fact that the file doesn't end in a newline, as I initially thought).

> > +#include "ion/shared/IonFrames-x86-shared.h"
> 
> As above, this causes problems.

This seems like a peculiar exception, probably not worth handling in the script since we won't be running it automatically anyway.

> > +#include "js/MemoryMetrics.h"
> 
> I know it doesn't follow the usual pattern, but js/MemoryMetrics.h is
> actually the header for this module.  jsmemorymetrics.cpp should be renamed
> vm/MemoryMetrics.cpp.

> > +#include "js/PropertyKey.h"
> 
> Again, this is the module header for this .cpp files, so it should come
> first.

Weird. I'm not sure these exceptions are worth handling in the script either.

Comment 33

[Nicholas Nethercote [inactive]]

Parts 11 and 12:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df8423fdcad8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7fcbc8fd27

These did ion/arm/ and the follow-up fixes identified in comment 31.

Comment 34

[Nicholas Nethercote [inactive]]

I filed bug 898274 for adding checking to make sure the #include statement stay in order.

Thanks for all your hard work on this, Emanuel!

Comment 35

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 13 - Fix #include ordering in a few places that fell through the cracks

After fixing the (inlines.h, -inl.h) matching to be case insensitive, "ion/ExecutionModeInlines.h" popped up as an include that should be in the inlines section. There were also a few remaining issues that slipped through the cracks.

I fixed this one up myself - the shell compiles on Linux, and I sent it to try to check the rest:
https://tbpl.mozilla.org/?tree=Try&rev=63d027c48556

Comment 36

[Nicholas Nethercote [inactive]]

Comment on attachment 781582 [details] [diff] [review]
Part 13 - Fix #include ordering in a few places that fell through the cracks

Review of attachment 781582 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for this.  It's Friday night here;  I'll do these on Monday.

I'm uncertain about ExecutionModeInlines.h -- I don't know if it really should be ExecutionMode-inl.h.  Because I couldn't tell, I assumed not, and decided to treat it *not* as a -inl.h file.  So I might just leave it unchanged.

::: js/src/ion/shared/MacroAssembler-x86-shared.h
@@ +18,3 @@
>  #ifdef JS_CPU_X86
>  # include "ion/x86/Assembler-x86.h"
> +#elif defined(JS_CPU_X64)

Oh, good catch on that one.

Comment 37

[Nicholas Nethercote [inactive]]

Comment on attachment 778957 [details] [diff] [review]
Automatically generated WIP patch

I'll mark this patch as non-obsolete and r+'d, for posterity, since it's the one I used as a basis for the landed patches.

Comment 38

[Carsten Book [:Tomcat]]

backed out part 11 in https://hg.mozilla.org/integration/mozilla-inbound/rev/63887fd246cc seems it have caused bug 898387 as prema-orange on inbound

Comment 39

[Carsten Book [:Tomcat]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=25768929&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=25769286&tree=Mozilla-Inbound are 2 examples of the crash

Comment 40

[Nicolas B. Pierron [:nbp]]

(In reply to Carsten Book [:Tomcat] from comment #38)
> backed out part 11 in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/63887fd246cc seems it
> have caused bug 898387 as prema-orange on inbound

This also seems to caused huge regressions which were visible on benchmarks running on B2G.
  http://arewefastyet.com/#machine=14

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f7fcbc8fd27

Comment 42

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Part 11 v2 - Fix #include ordering in js/src/ion/arm/.

I did some experimentation using try. It seems ion/arm/Architecture-arm.cpp needs to include ion/arm/Assembler-arm.h, or things break. I don't know why, but Mochitest 1 is green on ARM if that include isn't removed. I also put the top define with the others, since it will be defined regardless. The comment that was there about the kernel version check doesn't look like it applies to the current #if block anymore.

Comment 43

[Nicholas Nethercote [inactive]]

Comment on attachment 782153 [details] [diff] [review]
Part 11 v2 - Fix #include ordering in js/src/ion/arm/.

Review of attachment 782153 [details] [diff] [review]:
-----------------------------------------------------------------

Great!  How did you determine it was that #include that was the problem?

I'll try and land this (and part 13) on Monday.

Comment 44

[Emanuel Hoogeveen [:ehoogeveen]]

I tried moving the #defines to ion/arm/Architecture-arm.h, but that still went orange. Then I reverted the changes to ion/arm/Architecture-arm.cpp, since those looked the most suspicious to me, and got a green run on try. Then I tried a few permutations of partial changes to that file, and those proved that only removing ion/arm/Assembler-arm.h made a difference. Only took a few pushes to try, so I may have gotten a bit lucky :)

Comment 45

[Nicholas Nethercote [inactive]]

Part 13, and part 11 attempt 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2388401dc340
https://hg.mozilla.org/integration/mozilla-inbound/rev/77337bcdfd68

Fingers crossed that's the end!

Comment 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2388401dc340
https://hg.mozilla.org/mozilla-central/rev/77337bcdfd68