Closed Bug 886140 Opened 7 years ago Closed 7 years ago

Use IWYU on jsobj and jsinfer to untangle them and fix the final include guard

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 886205

People

(Reporter: ehoogeveen, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

While working on bug 883697, I found that I couldn't move the include guard in jsinferinlines.h to the top of the file, required to make the gcc and clang include guard optimizations work, without causing compilation errors in jsobjinlines.h. The patches in this bug apply Include What You Use (IWYU) to the jsobj and jsinfer modules in order to untangle them, then fix up other files just enough to make compilation work again.

To do this I wrote a little shell script that takes the place of the compiler and sends the passed arguments to IWYU, then clang, so that IWYU doesn't interfere with the build system. In addition, the script finds the filename of the file being compiled and runs IWYU on its header and any inlines files if they exist. It outputs the results to appropriately named files (e.g. jsobj_cpp_iwyu.txt).

I then ran the script for various configurations of the JS shell, and two browser builds (release and debug). I compared the output of the results and compiled the most inclusive set of includes (no pun intended) for each of them. That way, I hopefully avoided bug 885511. There were some differences between the generated files, though looking back I probably could have gone about it more efficiently by selecting all the options that enable various parts of the code at once.

Since this is my first patch based on the results of IWYU and I don't know if there are any particular conventions for how to apply them, I basically followed IWYU to the letter. I did structure the files so that the .cpp file includes the inlines.h file, and the inlines.h file includes the .h file (following the SpiderMonkey Coding Guide), and so that the .cpp and inlines.h files didn't contain any redundant includes. I also didn't include the automatically generated files, since those should be passed on the command line.

After applying IWYU, a lot of .cpp files needed to include an extra header such as jsfuninlines.h. Although this seems unfortunate at first glance, it makes sense: these files use functions from headers like jsfun.h, which do not define the functions they declare. I didn't want to put these new includes in header files, where there might have been less duplication, because that could have lead to new include cycles. Placing them directly in the .cpp files puts us a step closer to IWYU for SM, and these files *do* after all use functions from the included files.
I deviated from IWYU by including jsinferinlines.h in jsobjinlines.h. I'm not sure why IWYU didn't think this was needed, but including only jsinfer.h caused more problems than I was willing to investigate.
Attachment #766463 - Flags: review?(n.nethercote)
In addition to applying IWYU, this also moves the include guard in jsinferinlines.h to the top so that gcc and clang's include guard optimization can kick in.
Attachment #766464 - Flags: review?(n.nethercote)
As described in the first comment, this fixes up all the compilation and linker errors that resulted from part 2. I did this with the help of -Werror=undefined-inline and DXR :)
Attachment #766465 - Flags: review?(n.nethercote)
Blocks: 634839, 883697
Compiling the browser found a couple more issues. That's a bit worrying, since I'm not sure how good the coverage is at this point. I can't think of a way around it though, short of including the inlines files everywhere (and defeating the point of having them separate in the first place).
Attachment #766465 - Attachment is obsolete: true
Attachment #766465 - Flags: review?(n.nethercote)
Attachment #766493 - Flags: review?(n.nethercote)
Comment on attachment 766463 [details] [diff] [review]
Part 1 - Apply IWYU to jsobj

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

Can you attach the output from IWYU?  That would make reviewing these patches much easier.  You're adding a lot of #includes to jsobjinlines.h that I removed in bug 880565, and I'd like to understand why.

In general, I've been removing the #includes that IWYU suggests, but haven't bothered adding the ones it says are missing, except when it causes compile errors.  (I.e. transitive #includes are ok.)  Perhaps you weren't doing this.  Also, in my experience IWYU is simply wrong about 5% of the time when it says you can remove a #include.

It's interesting to hear how you ran IWYU.  I've been running it over the whole codebase and it reliably gives me info about .cpp files, but the .h file coverage is patchy, for no obvious reason.  I'll attach the script I've been using, it might be interesting to compare with your script.

You should use jorendorff's script (https://bugzilla.mozilla.org/attachment.cgi?id=749760) to make sure your patches don't introduce any new cycles.  And beware that the script's output is non-deterministic, and the same cycle(s) can be printed out in multiple different ways... so if you run it twice in a row it might give different output, but if you look closely the results are actually equivalent.

::: js/src/jsobj.h
@@ +15,5 @@
>   * values, called slots.  The map/slot pointer pair is GC'ed, while the map
>   * is reference counted and the slot vector is malloc'ed.
>   */
> +#include "mozilla/Assertions.h"     // for MOZ_STATIC_ASSERT
> +#include "mozilla/Attributes.h"     // for MOZ_DELETE

I haven't been including the "for" comments... though they are helpful for reviewing (hence my request for the IWYU output) they're likely to get out of date.
Comment on attachment 766493 [details] [diff] [review]
Part 3 - Fix up compilation after the jsinfer changes

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

I'm really surprised how many places you've had to add jsfuninlines.h.  Which function or functions are needed in so many places?  Lots of the functions in that file (all the ones that only look at |u|) look like they could be put in jsfun.h instead...
> Lots of the
> functions in that file (all the ones that only look at |u|) look like they
> could be put in jsfun.h instead...

I've started bug 886205 to look into this.  I haven't got to jsfuninlines.h yet, but I've moved lots of things out of vm/ObjectImpl.h and jsobjinlines.h, and I expect jsfuninlines.h will be similar.
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Can you attach the output from IWYU?  That would make reviewing these
> patches much easier.  You're adding a lot of #includes to jsobjinlines.h
> that I removed in bug 880565, and I'd like to understand why.
> 
> In general, I've been removing the #includes that IWYU suggests, but haven't
> bothered adding the ones it says are missing, except when it causes compile
> errors.  (I.e. transitive #includes are ok.)  Perhaps you weren't doing
> this.  Also, in my experience IWYU is simply wrong about 5% of the time when
> it says you can remove a #include.
Ah, yes, I haven't been doing this. I'll admit, in this bug it was my intent mainly to get things working, so I didn't want to look at what includes weren't strictly necessary until after everything compiled. As well, I was worried about includes that aren't needed now but will be after we apply IWYU to more things - but after writing part 3 of this patch, I see that we can just deal with that when we come to it (at the header level though).

Unfortunately I only saved the total list of includes, not the 'what to add' and 'what to remove' sections. I should be able to get them again without too much difficulty, but I'll combine some more config options so I don't have to compare as many files.

> It's interesting to hear how you ran IWYU.  I've been running it over the
> whole codebase and it reliably gives me info about .cpp files, but the .h
> file coverage is patchy, for no obvious reason.  I'll attach the script I've
> been using, it might be interesting to compare with your script.

I've inluded the script I used. It's pretty ghetto, but it works :) Most of the .cpp file IWYU reports also cover the .h file, but most don't report on the inlines.h or -inl.h file for some reason. Likewise, most inlines.h and -inl.h IWYU reports also cover the .h file (usually with the same outcome as the .cpp report). The .h file reports were usually pretty redundant as a result, but it was nice to have them separate as a place to start (I worked through them in the order .h, inlines.h/-inl.h, .cpp).

> > +#include "mozilla/Assertions.h"     // for MOZ_STATIC_ASSERT
> > +#include "mozilla/Attributes.h"     // for MOZ_DELETE
> 
> I haven't been including the "for" comments... though they are helpful for
> reviewing (hence my request for the IWYU output) they're likely to get out
> of date.

Oops, I didn't mean to include those in the patch.

(In reply to Nicholas Nethercote [:njn] from comment #6)
> I'm really surprised how many places you've had to add jsfuninlines.h. 
> Which function or functions are needed in so many places?  Lots of the
> functions in that file (all the ones that only look at |u|) look like they
> could be put in jsfun.h instead...

isHeavyWeight() was the first linker error I got, but most of them are either isCallable() or writeBarrierPre(). I can get you a list of the linker errors with just parts 1 and 2 applied (and some spot fixes for compile errors, which I can list as well if they add any functions). I'll put that in bug 886205 when I have time to get it.
> I've inluded the script I used.

Or I was meaning to, but then I forgot. Here it is :) I also made a C version that just has a few minor changes to accommodate .c instead of .cpp, but of course nothing in SM uses that. I simply set CXX to the location of this script when calling make (or in .mozconfig for a browser build), and it works. Note the '-x c++' build flag - this is needed so clang won't compile the .h files as Objective-C. Adding '-w' just reduces the spew (it still prints errors above the IWYU lists though, and there's usually a bunch of those).
Let's see... I'm most interested in this bug to fix the misplaced #ifndef wrapper in jsinferinlines.h.  Do you think you could come up with a patch (or patches) that do that in a minimal way?  I'm gradually removing unneeded headers with IWYU in bug 634839, and that's work that doesn't parallelize easily.
Yes, I'm planning to go through (with the current part 3 in hand) and figure out which includes aren't needed as things stand. I'm not in any real hurry to land this patch, and if you can land fixes for the funinlines.h and Shape-inl.h stuff in bug 886205 that should greatly reduce part 3.

Speaking of which, I just went through the compile errors (without part 3) and they look like this:

Uses  Function                    File
5     IsConstructing              jsfuninlines.h
5     js::ion::BaselineFrame      ion/BaselineFrame.h
2     MarkNonNativePropertyFound  vm/Shape-inl.h
2     JS_sprintf_append           jsprf.h
2     JS_snprintf                 jsprf.h
1     Function                    jsfuninlines.h
1     IsImplicitDenseElement      vm/Shape-inl.h
1     JS_vsmprintf                jsprf.h

The list of linker errors is a lot longer though. I'll see if I can parse all the undefined function warnings in the morning to give a better picture.
Attachment #766463 - Flags: review?(n.nethercote)
Attachment #766464 - Flags: review?(n.nethercote)
I didn't set out to do so, but by fixing the cyclic header dependencies (in bug 886205) I also managed to fix the misplaced #ifndef wrapper in jsinferinlines.h.  So I don't think there's anything left to do in this bug! :)
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 886205
I'll take it! :)
Attachment #766493 - Flags: review?(n.nethercote)
You need to log in before you can comment on or make changes to this bug.