885511 - Avoid breaking nondefault build options with IWYU (Include What You Use)

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

So the recent IWYU-based #include pruning ended up breaking at least JS_MORE_DETERMINISTIC and JSGC_GENERATIONAL. Rather than periodically re-fixing these whenever we do an IWYU pass, is there a more general solution?

One option would be to just pick compile options for IWYU that will compile as much code as possible. Obviously, this has holes (eg a number of code chunks are either/or), but might be good enough.

A different way of accomplishing the same thing would be to add #ifdef IWYU || ... everywhere (or #ifdef COMPILE_ALL_THE_THINGS), to manually label stuff that's necessary, but that seems... well, kind of horrible.

I don't know how IWYU works, but can it gather required includes from multiple passes with different #define settings? Or if it's just looking at the code, can it assume all #ifdef and #ifndef are true, and just look at everything?

See bug 885502 for the 2nd installement of the JS_MORE_DETERMINISTIC breakage.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> See bug 885502 for the 2nd installement of the JS_MORE_DETERMINISTIC
> breakage.

And it came without warning too, unfortunately, I realised this after updating on my end and saw that the builds were busted.

Comment 2

[Nicholas Nethercote [inactive]]

IWYU is a clang plug-in, so it's seeing the post-CPP code, so there's no chance it can somehow read all the code.

This is a general problem with having lots of different builds and not having them all tested on test machines.  It's just that IWYU clean-ups are likely to hit this problem more than most changes.

I'll do deterministic  builds for future IWYU patches, since that's hit a problem.

The only preventive measure I can think of is to use #ifndefs to document when a header is only needed for a particular build.  For example, in jsiter.cpp:

  #ifdef JS_MORE_DETERMINISTIC
  #include "ds/Sort.h"
  #endif

Comment 3

[Jeff Walden [:Waldo]]

You could also throw the relevant code behind templates:

enum Determinism { Deterministic = true, UnspecifiedDeterminism = false };

template<Determinism IsDeterministic>
struct ComputeAsmJSLoadTime;

template<>
struct ComputeAsmJSLoadTime<Deterministic>
{
    size_t loadTime() { ... };
};

template<>
struct ComputeAsmJSLoadTime<UnspecifiedDeterminism>
{
    size_t loadTime() { return 0; }
};

and use ComputeAsmJSLoadTime<JS_MORE_DETERMINISTIC>::loadTime().  Same for all the other places.

That fixes syntax-based issues.  It won't fix semantic issues, due to lazy instantiation.  But maybe that solves enough problems to be worth the refactoring.

Comment 4

[Jeff Walden [:Waldo]]

Oh, I guess you could declare both specializations to get both things compiled:

template<>
struct ComputeAsmJSLoadTime<Deterministic>;
template<>
struct ComputeAsmJSLoadTime<UnspecifiedDeterminism>;

Of course this is all moderately hackish, but so's anything else that comes to mind here.  :-)

Comment 5

[Nicholas Nethercote [inactive]]

I've been building with --enable-gcgenerational and --enable-more-deterministic when doing my recent IWYU work.  I think that's enough.  Doesn't seem worth doing anything more here.