Clean up the hazard analysis implementation

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(23 attachments, 4 obsolete attachments)

8.26 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.85 KB, patch
terrence
: review+
Details | Diff | Splinter Review
10.52 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.46 KB, patch
terrence
: review+
Details | Diff | Splinter Review
12.31 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.42 KB, patch
terrence
: review+
Details | Diff | Splinter Review
8.15 KB, patch
terrence
: review+
Details | Diff | Splinter Review
5.95 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.41 KB, patch
terrence
: review+
Details | Diff | Splinter Review
18.22 KB, patch
terrence
: review+
Details | Diff | Splinter Review
7.32 KB, patch
terrence
: review+
Details | Diff | Splinter Review
4.91 KB, patch
terrence
: review+
Details | Diff | Splinter Review
2.03 KB, patch
terrence
: review+
Details | Diff | Splinter Review
29.67 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.16 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.26 KB, patch
terrence
: review+
Details | Diff | Splinter Review
7.00 KB, patch
Details | Diff | Splinter Review
9.61 KB, patch
terrence
: review+
Details | Diff | Splinter Review
5.56 KB, patch
terrence
: review+
Details | Diff | Splinter Review
3.98 KB, patch
terrence
: review+
Details | Diff | Splinter Review
10.27 KB, patch
terrence
: review+
Details | Diff | Splinter Review
61.52 KB, patch
jonco
: review+
Details | Diff | Splinter Review
2.26 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
Making a bug for a large grab-bag of changes I've been working on. Some are just refactoring. Some are actual fixes for things uncovered while creating a test suite. Then there's the test suite itself.
Assignee

Comment 1

3 years ago
It seems like Map is now the "right" solution for... well, maps. Dicts. Whatever you want to call them. This moves over some but not all of them.

I think this was motivated by a case where there was a C function named... uh, "push"? Hm, I doubt I would be doing x.push(). But it was something like that -- it broke the analysis because it shadowed something on Object.prototype that I was unwisely calling.

Clearly, it wasn't a *real* problem, since the current code is not failing. But this patch adds a bunch of comments too.
Attachment #8734955 - Flags: review?(terrence)
Assignee

Comment 2

3 years ago
Honestly, I'm not sure why I'm wasting your time with this one. I should've rolled it into my other refactoring patch.
Attachment #8734956 - Flags: review?(terrence)
Assignee

Comment 3

3 years ago
I think I'm the only one who ever looks at them, but in hazard reports it gives you a patch from a point early in a variable's live range, through a GC call, to a use of the variable after the GC call that proves it's still live. It can be very helpful in deciphering tricky hazards without having to go back to the XDB files.

But it was sometimes cutting off the first link in that chain, which made it seem like it was just spewing a bunch of internal goop. Perhaps with this fixed, other people will get some use out of it too?

I meant to write a test for this, but ran out of steam. I guess I'll wait until I find the next problem. It's noncritical stuff anyway.

Oh, crud. I just noticed this patch has a bunch of other stuff mixed in.
Attachment #8734961 - Flags: review?(terrence)
Assignee

Comment 4

3 years ago
Another trivial one, from trying to split out the important stuff a little better.
Attachment #8734967 - Flags: review?(terrence)
Assignee

Updated

3 years ago
Attachment #8734961 - Attachment is obsolete: true
Attachment #8734961 - Flags: review?(terrence)
Assignee

Comment 5

3 years ago
Larger chunk of refactoring goop, including a somewhat controversial Map.prototype.update definition.
Attachment #8734970 - Flags: review?(terrence)
Assignee

Updated

3 years ago
Attachment #8734967 - Attachment is obsolete: true
Attachment #8734967 - Flags: review?(terrence)
Assignee

Comment 6

3 years ago
Previously, this mostly "worked" purely by chance -- it started gathering method definitions at the 'csu' variable, which JS helpfully hoisted up to the toplevel. It had the last value assigned to csu within the loop, which would have been the basest base class (don't think too hard about the case of multiple inheritance, it was wrong). Then we find all descendants, which was *too* much, but it ended up just making the analysis conservative.

The new version gets the exact set of virtual methods that are possible, starting at the static type. (So all descendants' definitions, plus the static type itself or anything it might have inherited from.)
Attachment #8734991 - Flags: review?(terrence)
Assignee

Updated

3 years ago
Attachment #8734970 - Attachment is obsolete: true
Attachment #8734970 - Flags: review?(terrence)
Assignee

Comment 7

3 years ago
No more passing a 2nd return value back through a mutable 1-element array. Some of the comments were outright lies.
Attachment #8734997 - Flags: review?(terrence)
Assignee

Updated

3 years ago
Attachment #8734991 - Attachment is obsolete: true
Attachment #8734991 - Flags: review?(terrence)
Assignee

Comment 8

3 years ago
Seeing the exact commands executed (together with necessary environment variable changes) is really handy for debugging, but it also obscures what's going on.
Attachment #8734998 - Flags: review?(terrence)
Assignee

Comment 9

3 years ago
Whoa, math is hard and tests are good.

So I have w workers that want to divide n tasks between themselves. While that means it'll be *about* Math.floor(n/w) tasks each, the specifics are important. With my previous logic, you'd drop n % w jobs on the floor. Which is not good.

A different approach would be to give each worker Math.floor(n/w) tasks, and then give one of the remaining n%w tasks to each of the workers in order. That gets a nice even distribution, but it seemed a little messy. I guess that would be num_tasks[i] = Math.floor(n/w) + (i < n%w ? 1 : 0), which isn't bad. Hm, maybe I should have done that. Although I really care about finding the start and end points, which means iterating through all previous workers' (the workers figure this out on startup). I guess that's still fixable without a loop, with some logic.

Anyway, what I ended up with was finding the w (floating point) points at which to break up the n tasks. Then each worker's tasks go from its start point (floored to the previous whole index) up to just before the beginning of the next worker's task start point. (If start < end, then that worker gets no tasks.) The n%w "extra" tasks get evenly spread across workers this way, and each worker just needs to compute its start position and the next worker's start position. (There is a virtual worker that starts at the task just past the end, assuming weird rounding that would break Math.floor(k/k*N)==N, so the final worker need not be handled specially.)
Attachment #8734999 - Flags: review?(terrence)
Assignee

Comment 10

3 years ago
I switched lots of stuff over to in-source annotations, but things like AutoSuppressGC were still done with an explicit list in annotations.js. This switches them over to in-source annotations too. That requires reordering the analysis phases, since we need to know the GC suppressors while building the callgraph.

That may seem weird, but edges in the callgraph include information about whether the call is within the scope of a GC suppression.
Attachment #8735001 - Flags: review?(terrence)
Assignee

Comment 11

3 years ago
A pretty small grab-bag of minor fixes.
Attachment #8735002 - Flags: review?(terrence)
Assignee

Comment 12

3 years ago
I knew the compiler sometimes generated "C4" constructors and then internally made all of the regular constructors call them. Now, it's doing the same thing with "D4" destructors. In pretty common cases, too. I have a test that hits this in a later patch.

This fixes a real problem that bz noticed. I tried removing some annotations that should have revealed a hazard, and it did not report one because it couldn't see that a destructor was GC'ing.
Attachment #8735003 - Flags: review?(terrence)
Assignee

Comment 13

3 years ago
The analysis attempts to give example stacks demonstrating how a function could GC, and some of my hazards were missing them. It turned out to be because of a wacky case where the mangled representation of a function was identical between two different functions -- gcFunctions.txt only had a record of the "real" one.

Fortunately, it's pretty easy to fix, as I was already collecting the set of unmangled names for each mangled name, and they *do* differ in the unmangled name.
Attachment #8735004 - Flags: review?(terrence)
Assignee

Comment 14

3 years ago
Rewrite the test suite, and then test a bunch of stuff that this patch stack deals with, along with some "vanilla" sorts of hazards.
Attachment #8735005 - Flags: review?(terrence)
Assignee

Comment 15

3 years ago
Comment on attachment 8734961 [details] [diff] [review]
The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further

Uh oh. bzexport is flaking out on me and obsoleting things that should not be obsoleted. Reinstating review requests.

On the bright side, this lets me flood terrence's inbox even more!
Attachment #8734961 - Attachment is obsolete: false
Attachment #8734961 - Flags: review?(terrence)
Assignee

Updated

3 years ago
Attachment #8734967 - Attachment is obsolete: false
Attachment #8734967 - Flags: review?(terrence)
Assignee

Updated

3 years ago
Attachment #8734970 - Attachment is obsolete: false
Attachment #8734970 - Flags: review?(terrence)
Assignee

Updated

3 years ago
Attachment #8734991 - Attachment is obsolete: false
Attachment #8734991 - Flags: review?(terrence)
Comment on attachment 8734955 [details] [diff] [review]
Switch to using a Map for visited points within a function, and heavily comment

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

Nice improvement!
Attachment #8734955 - Flags: review?(terrence) → review+
Attachment #8734956 - Flags: review?(terrence) → review+
Comment on attachment 8734961 [details] [diff] [review]
The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further

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

::: js/src/devtools/rootAnalysis/analyzeRoots.js
@@ +630,5 @@
>          loadPrintedLines(functionName);
>  
>      while (entry) {
>          var ppoint = entry.ppoint;
> +        var lineText = findLocation(entry.body, ppoint, "brief");

Oh wow. That's just special. "brief" is "true", but I don't know that this abuse of the type system is called for. How about we pass {brief:true} and have findLocation take options={brief:false} instead?
Attachment #8734961 - Flags: review?(terrence) → review+
Attachment #8734967 - Flags: review?(terrence) → review+
Attachment #8734970 - Flags: review?(terrence) → review+
Comment on attachment 8734991 [details] [diff] [review]
Start searching from the most specialized csu

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

Wow.
Attachment #8734991 - Flags: review?(terrence) → review+
Attachment #8734997 - Flags: review?(terrence) → review+
Attachment #8734998 - Flags: review?(terrence) → review+
Attachment #8734999 - Flags: review?(terrence) → review+
Comment on attachment 8735001 [details] [diff] [review]
In-source annotations for GC suppression

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

Neat!
Attachment #8735001 - Flags: review?(terrence) → review+
Attachment #8735002 - Flags: review?(terrence) → review+
Attachment #8735003 - Flags: review?(terrence) → review+
Attachment #8735004 - Flags: review?(terrence) → review+
Comment on attachment 8735005 [details] [diff] [review]
Rewrite the test suite, add several tests

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

::: js/src/devtools/rootAnalysis/t/testlib.py
@@ +37,5 @@
> +            print("Running %s" % cmd)
> +        subprocess.check_call(["sh", "-c", cmd])
> +
> +    def load_db_entry(self, dbname, pattern):
> +        if not isinstance(pattern, basestring):

Please add a comment that pattern must be str or regexp.

@@ +40,5 @@
> +    def load_db_entry(self, dbname, pattern):
> +        if not isinstance(pattern, basestring):
> +            output = subprocess.check_output([self.binpath("xdbkeys"), dbname + ".xdb"])
> +            entries = output.splitlines()
> +            matches = [f for f in entries if re.search(pattern, f)]

s/f/_/. Underscore is the expected throwaway in these cases.

@@ +57,5 @@
> +sixgill_bin = '{bindir}'
> +'''.format(scriptdir=scriptdir, bindir=self.cfg.sixgill_bin))
> +        cmd = [ os.path.join(scriptdir, "analyze.py"), phase ]
> +        if upto:
> +            cmd += [ "--upto", upto ]

Strip extra space after and before [ and ] respectively; here and 2 lines up.

@@ +72,5 @@
> +
> +    def computeHazards(self):
> +        self.run_analysis_script("callgraph")
> +
> +    def load_text_file(self, filename, key=lambda l: l):

It looks like we're using |key| for filtering. In python, a functor key usually indicates a sort key rather than a filter function. How about we call it |filter_func| instead?

@@ +75,5 @@
> +
> +    def load_text_file(self, filename, key=lambda l: l):
> +        return [processed for processed in (
> +            key(line.strip()) for line in file(os.path.join(self.outdir, filename))
> +        ) if processed is not None]

Ouch, that spacing. How about we name the intermediate generator object instead:

keyed = (key(line.strip()) for line in file(os.path.join(self.outdir, filename)))
return [_ for _ in keyed if _ is not None]

@@ +84,5 @@
> +    def load_gcTypes(self):
> +        def grab_type(line):
> +            m = re.match(r'^(GC\w+): (.*)', line)
> +            if m:
> +                return (m.group(1) + 's', m.group(2))

Please explicitly |return None| in the failing case here so that readers don't think it might be accidental.

@@ +101,5 @@
> +            if m:
> +                info = list(m.groups())
> +                info[0] = info[0].split("$")[-1]
> +                info[3] = info[3].split("$")[-1]
> +                return HazardSummary(*info)

And here.
Attachment #8735005 - Flags: review?(terrence) → review+
Assignee

Comment 21

3 years ago
This was a fun one to track down. The analysis thought the *constructor* of AutoFoo here can GC:

class GCOnDestruction {
  public:
    ~GCOnDestruction() { GC(); }
};

extern void foo();

class AutoFoo {
    GCOnDestruction killmenow;
  public:
    AutoFoo() : killmenow() { foo(); }
};

Why? Because if exceptions were allowed, then theoretically foo() could throw an exception, and the half-constructed AutoFoo would get immediately torn down, running killmenow's destructor.

Which is fine and all, except that we're compiling with -fno-exceptions. For whatever reason, the CFG passed to the plugin contains exception handling cleanup nodes, and the analysis took that very seriously.

Fortunately, it also marks those cleanup nodes as being only valid for exception handling, so we can manually ignore them. (Apparently, it uses cleanup nodes for other sorts of failure as well, and we do need to take those into account.)
Attachment #8746783 - Flags: review?(terrence)
Assignee

Comment 22

3 years ago
This was ugly. js::ReportOutOfMemory(ExclusiveContext*) was getting called, and yet it had no callees. Why? Because only js::ReportOutOfMemory(js::ExclusiveContext*) had any callees (notice the js:: namespace qualification).

It actually got it right most of the time, but there was one recently-added usage with a forward declaration of ExclusiveContext was tricking it, and when it did a lookup from the mangled name => the demangled name, it happened to pick the unqualified version and everything fell apart.

The fix appeared to be easy -- just use decl_as_string instead of pulling out the internal char* pointer. Except that decl_as_string is C++ only, so we get a link failure against cc1 (eg when compiling conftest.c). Turns out sixgill already had something set up for this, where it defines a bunch of dummy symbols that just abort() when they're called. They're weak, so they get overridden by the real ones, and the code dynamically chooses to only call them in the configuration where they exist.

I changed decl_as_string to do the right thing when called for C code instead, and can now call decl_as_string unconditionally for either language.
Attachment #8746787 - Flags: review?(terrence)
Assignee

Comment 23

3 years ago
This is pointing to the binary resulting from the sixgill patches uploaded to this bug. No need to review this separately (and in fact I should prune down the diff as there are nuisance whitespace changes in it now... ugh).

MozReview-Commit-ID: 7WSYmLAVb7M
Assignee

Comment 24

3 years ago
So... an earlier patch in this bug fixed a breakage in the callgraph related to internally generated constructors and destructors. Unfortunately, that made all pre barriers into GC functions. With a very plausible-looking stack. :(

Fortunately, it turns out to be a false positive, The GC happened in AutoClearTypeInferenceStateOnOOM, which is passed in to various sweep() functions by pointer. In most cases, this pointer is actually null, and so the GC is impossible. I split out separate function overloads to say whether it was really possible. They actually just feed into the original function, but the nogc variant suppresses the analysis. (If I got it wrong, it'll be a dynamic MOZ_CRASH.)

The details are definitely arguable. I used the same name for all 3 methods, which perhaps isn't the best choice. Lemme know what you think.

MozReview-Commit-ID: CLrEs3OODOu
Attachment #8746795 - Flags: review?(terrence)
Assignee

Comment 25

3 years ago
The code was previously doing some very very heuristic-y checks for identifying constructors. This broke when sixgill started properly qualifying type names, so I did the verbose ugly thing that is much much more precise. It still does some nasty regexp parsing, and it's possible to fool, but very unlikely.

MozReview-Commit-ID: 46064amRbCQ
Attachment #8746796 - Flags: review?(terrence)
Assignee

Comment 26

3 years ago
Here's an easy one. Get rid of some unused test files.

MozReview-Commit-ID: sBZct85zDJ
Attachment #8746798 - Flags: review?(terrence)
Assignee

Comment 27

3 years ago
Add a test for the new fixes.

MozReview-Commit-ID: H5NZlyS2rx5
Attachment #8746799 - Flags: review?(terrence)
Attachment #8746783 - Flags: review?(terrence) → review+
Comment on attachment 8746787 [details] [diff] [review]
Use decl_as_string to properly namespace-qualify types for C++

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

O_o
Attachment #8746787 - Flags: review?(terrence) → review+
Comment on attachment 8746795 [details] [diff] [review]
Expose when sweeping can or cannot GC to the analysis

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

I think this is clearer to the human reader too.

::: js/src/jsscript.h
@@ +1725,5 @@
>  
>      inline js::TypeScript* types();
>  
> +    void maybeSweepTypes(js::AutoClearTypeInferenceStateOnOOM& oom) {
> +        maybeSweepTypes(&oom);

This is a definite double-take. We should call the target maybeSweepTypesInner or somesuch to disambiguate.

::: js/src/vm/ObjectGroup.h
@@ +409,1 @@
>      inline void maybeSweep(AutoClearTypeInferenceStateOnOOM* oom);

And here as well, please.
Attachment #8746795 - Flags: review?(terrence) → review+
Attachment #8746796 - Flags: review?(terrence) → review+
Attachment #8746798 - Flags: review?(terrence) → review+
Comment on attachment 8746799 [details] [diff] [review]
Test for sixgill exception handling + analysis constructor/destructor matching

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

::: js/src/devtools/rootAnalysis/t/exceptions/source.cpp
@@ +22,5 @@
> +class AutoSomething {
> +    RAII_GC gc;
> +  public:
> +    AutoSomething() : gc() {
> +        asm(""); // Ooh, scary, this might throw an exception

:-)
Attachment #8746799 - Flags: review?(terrence) → review+
Assignee

Comment 31

3 years ago
Well, this got a little out of hand.

I wanted to create a new ZoneCellIter that knew statically that it would not GC during its constructor. I can't make a templateized ZoneCellIter<T>, because you can't have both templatized and non-templatized classes with the same name.

So I used ZoneNonNurseryCellIter<T>, where T is an AllocKind, but it's fugly.

I also noticed that a lot of these did some wonky

  JSScript* script = i.get<JSScript>();

stuff, which felt sort of redundant with ZoneNonNurseryCellIter<AllocKind::SCRIPT>.

And then I thought I'd be clever and use MapTypeToFinalizeKind. Especially since then maybe I could use SFINAE to make it work for all GC types.

But it still had the ugly type name. So I thought maybe it should be a method on Zone anyway.

But Zone doesn't include jsgcinlines.h, so it doesn't have the types. Ooh, but this is template-land, so it doesn't need them.

So I switched stuff over to eg zone->cellIter<JSScript>(), it looked kinda cool. And I realized that zone->cellIter<JSObject>() is kind of pointless, since JSObject does not correspond to a single AllocKind anyway. That meant I could leave it as a zone->cellIter(kind) overload.

And life was good. Except now ZoneCellIterUnderGC stuck out like a sore thumb.

So I switched it over to a similar setup.

And then I thought that if I'm changing the way iteration happens, I may as well go all the way and support new-style C++11 iteration.

And then I looked at the time. And said "no".

MozReview-Commit-ID: 77GjtXY0QUf
Attachment #8749460 - Flags: review?(terrence)
Comment on attachment 8749460 [details] [diff] [review]
Make ZoneNonNurseryCellIter to communicate nogc to the hazard analysis

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

At first I was bewildered by the many new iter types, but |auto| makes the usage sites quite clean while giving us the type flexibility to cleanly represent the annoying plurality of potential states! Quite nice. I'd r+ this except that I want Jon to take a look too, in case he sees something I missed.
Attachment #8749460 - Flags: review?(terrence) → review?(jcoppeard)
Assignee

Comment 33

3 years ago
Actually, Terrence was totally wrote about the explosion of ZoneCellIter types. I mapped out what each one was doing, and I *think* I got all the variants properly contained within two types: ZoneCellUntypedIter and ZoneCellIter<T> (where T is a GC type, not an AllocKind).

I am submitting this as a separate patch because I think it may be easier to follow the logic of introducing the ZoneNonNurseryCellIter nonsense first, and then reducing it down. But I'm not sure. A squashed patch might actually be simpler to review.

MozReview-Commit-ID: AxRTTUfWrND
Attachment #8749896 - Flags: review?(jcoppeard)
Assignee

Comment 34

3 years ago
err... "totally *right*".
Assignee

Comment 35

3 years ago
Also create accessors on gc::Zone for a nicer API to ZoneCellIters.

Here's a version that actually compiles and stuff. It also forces you to pass through a token to discardJitCode that proves that you've emptied the nursery somehow. It does not eliminate any hazards.

MozReview-Commit-ID: AxRTTUfWrND
Attachment #8751115 - Flags: review?(jcoppeard)
Comment on attachment 8751115 [details] [diff] [review]
Make ZoneCellIter variants to communicate nogc to the hazard analysis

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

Thanks for posting the combined patch.  It's a nice cleanup for zone iteration.

::: js/src/gc/Iteration.cpp
@@ +103,4 @@
>  
>      if (compartment) {
> +        Zone* zone = compartment->zone();
> +        for (auto script = zone->cellIter<JSScript>(underGC); !script.done(); script.next()) {

This is not called under GC as far as I can see - it's used for Debugger::findScripts.  Can we just call the normal iterator?

@@ +125,3 @@
>  
>      for (auto thingKind : ObjectAllocKinds()) {
> +        for (auto obj = zone->cellIter<JSObject>(thingKind, underGC); !obj.done(); obj.next()) {

It looks like this is called by the CC so that would not be under GC either.  Maybe just call the normal iterator.

::: js/src/gc/Zone.h
@@ +159,5 @@
> +    // Iterate over all cells in the zone. See the definition of ZoneCellIter
> +    // in jsgcinlines.h for the possible arguments and documentation.
> +    template <typename T, typename... Args>
> +    js::gc::ZoneCellIter<T> cellIter(Args... args) {
> +        return js::gc::ZoneCellIter<T>(this, args...);

I think we should mozilla::Forward the args here, and below.

@@ +172,5 @@
> +    // dependent on the type T, so does not need any special handling.)
> +    template <typename... Args>
> +    js::gc::ZoneCellUntypedIter_Dependent<JS::Zone*, Args...> untypedCellIter(Args... args) {
> +        return js::gc::ZoneCellUntypedIter_Dependent<JS::Zone*, Args...>(this, args...);
> +    }

Do we need this for anything other than objects?  If not could we just have an objectIter() that returns ZoneCellIter<JSObject*>?

::: js/src/jsgc.cpp
@@ +2440,5 @@
>      AutoClearTypeInferenceStateOnOOM oom(zone);
> +    AutoAssertUnderGC underGC(rt);
> +
> +    for (auto obj = zone->cellIter<JSObject>(AllocKind::OBJECT0, underGC); !obj.done(); obj.next())
> +        obj->isNative();

Not sure this is meant to be here.

::: js/src/jsgcinlines.h
@@ +190,5 @@
> +    JSRuntime* rt;
> +
> +  public:
> +    AutoAssertUnderGC(JSRuntime* rt) : AutoAssertEmptyNursery(rt) {
> +        MOZ_ASSERT(rt->isHeapBusy());

This doesn't check we're under GC as it also allows for heapState_ to be Tracing.  I think this should be |rt->isHeapCollecting()|.  Ditto below.

@@ +284,2 @@
>    public:
> +    ZoneCellUntypedIter_Dependent(Args... args) : ZoneCellUntypedIter(args...) {}

Needs Forward if we keep this.
Attachment #8751115 - Flags: review?(jcoppeard) → review+
Attachment #8749460 - Attachment is obsolete: true
Attachment #8749460 - Flags: review?(jcoppeard)
Attachment #8749896 - Attachment is obsolete: true
Attachment #8749896 - Flags: review?(jcoppeard)
Assignee

Comment 37

3 years ago
(In reply to Jon Coppeard (:jonco) from comment #36)
> ::: js/src/gc/Iteration.cpp
> @@ +103,4 @@
> >  
> >      if (compartment) {
> > +        Zone* zone = compartment->zone();
> > +        for (auto script = zone->cellIter<JSScript>(underGC); !script.done(); script.next()) {
> 
> This is not called under GC as far as I can see - it's used for
> Debugger::findScripts.  Can we just call the normal iterator?

Oh. Yeah, it turns out that the ZoneCellIterUnderGC was really ZoneCellIterWhileHeapIsBusy.

So at the moment, zone->cellIter<JSScript>() is just as good, since it'll skip the evictNursery. The busy-heap version doesn't seem very useful, since afaict the only effect is to skip a dynamic if (!rt->isHeapBusy) check?

> ::: js/src/gc/Zone.h
> @@ +159,5 @@
> > +    // Iterate over all cells in the zone. See the definition of ZoneCellIter
> > +    // in jsgcinlines.h for the possible arguments and documentation.
> > +    template <typename T, typename... Args>
> > +    js::gc::ZoneCellIter<T> cellIter(Args... args) {
> > +        return js::gc::ZoneCellIter<T>(this, args...);
> 
> I think we should mozilla::Forward the args here, and below.

Oh, yeah.

> @@ +172,5 @@
> > +    // dependent on the type T, so does not need any special handling.)
> > +    template <typename... Args>
> > +    js::gc::ZoneCellUntypedIter_Dependent<JS::Zone*, Args...> untypedCellIter(Args... args) {
> > +        return js::gc::ZoneCellUntypedIter_Dependent<JS::Zone*, Args...>(this, args...);
> > +    }
> 
> Do we need this for anything other than objects?  If not could we just have
> an objectIter() that returns ZoneCellIter<JSObject*>?

I eventually realized that I could eliminate not only this, but also ZoneCellUntypedIter, by using ZoneCellIter<TenuredCell> as a base class and specializing it to be pretty much what ZoneCellUntypedIter was. Then I don't need to play any special tricks to get Zone.h to compile without the type definitions, and it should continue working properly as we make more stuff nursery-allocable (including things with multiple AllocKinds, if we were to create such a thing.)

If you want to iterate over cells of arbitrary type (eg because you're looping over all AllocKinds), then you can just use ZoneCellIter<TenuredCell>.

> ::: js/src/jsgc.cpp
> @@ +2440,5 @@
> >      AutoClearTypeInferenceStateOnOOM oom(zone);
> > +    AutoAssertUnderGC underGC(rt);
> > +
> > +    for (auto obj = zone->cellIter<JSObject>(AllocKind::OBJECT0, underGC); !obj.done(); obj.next())
> > +        obj->isNative();
> 
> Not sure this is meant to be here.

Oops, sorry. That's debugging code because it turns out there weren't any instances of iterating over objects, and my first cut didn't compile. So this was to force an instantiation. Gone now.

> ::: js/src/jsgcinlines.h
> @@ +190,5 @@
> > +    JSRuntime* rt;
> > +
> > +  public:
> > +    AutoAssertUnderGC(JSRuntime* rt) : AutoAssertEmptyNursery(rt) {
> > +        MOZ_ASSERT(rt->isHeapBusy());
> 
> This doesn't check we're under GC as it also allows for heapState_ to be
> Tracing.  I think this should be |rt->isHeapCollecting()|.  Ditto below.

Yeah, I've eliminated AutoAssertUnderGC in favor of AutoAssertHeapBusy and AutoAssertEmptyNursery. The latter is what I actually care about with respect to hazards. I'm not sure if the former is useful? Currently, I don't pay attention to it, as it would only allow skipping what looks to be a cheap dynamic check. But I may be missing something. I can always add overloads that take it if there's something useful to be done with it; I just didn't see the need for the added complexity right now.
Attachment #8752306 - Flags: review?(jcoppeard)
Assignee

Updated

3 years ago
Attachment #8751115 - Attachment is obsolete: true
Assignee

Comment 38

3 years ago
Oops. Didn't wait for the compile to finish. What could I possibly break by adding mozilla::Forward<>?

Heh. It triggered a *nasty* template compilation error message, which ended up being due to bad const hygiene.

This version compiles for me. (Hopefully it will on Windows too.)
Attachment #8752314 - Flags: review?(jcoppeard)
Assignee

Updated

3 years ago
Attachment #8752306 - Attachment is obsolete: true
Attachment #8752306 - Flags: review?(jcoppeard)
Comment on attachment 8752314 [details] [diff] [review]
Make ZoneCellIter variants to communicate nogc to the hazard analysis

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

Looking good.

::: js/src/gc/Iteration.cpp
@@ +124,2 @@
>              if (obj->asTenured().isMarked(GRAY))
> +                cellCallback(data, JS::GCCellPtr(static_cast<JSObject*>(obj)));

Can we call the get() method here and get rid of the cast?

::: js/src/jsgc.h
@@ +1352,5 @@
> +    AutoAssertHeapBusy() : rt(nullptr) {
> +    }
> +
> +  public:
> +    AutoAssertHeapBusy(JSRuntime* rt) {

This is going to warn about implicit conversion from JSRuntime* to AutoAssertHeapBusy.  You probably want to mark this |explicit|.  Ditto for AutoAssertEmptyNursery.

::: js/src/jsgcinlines.h
@@ +297,3 @@
>    public:
> +    // (Possibly) outside of a GC, non-nursery allocated
> +    template <typename = MapTypeToFinalizeKind<GCType>>

Wow, I've never see this syntax before!  It seems the template parameter name is optional...

More importantly, is this necessary?  I tried removing this line and the one below and the code still compiles.

::: js/src/vm/TypeInference.cpp
@@ +4438,2 @@
>          zone->setPreservingCode(false);
> +        zone->discardJitCode(rt->defaultFreeOp(), rt);

This relies on implicit conversion from JSRuntime* to AutoAssertEmpty nursery.  I think this should just pass |empty| here.
Attachment #8752314 - Flags: review?(jcoppeard) → review+
Assignee

Comment 40

3 years ago
A pair of recently introduced hazards are only visible with the fixes in this bug. Annotate them.
Attachment #8756633 - Flags: review?(terrence)
Attachment #8756633 - Flags: review?(terrence) → review+

Comment 42

3 years ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15de0d1d0b05
Switch to using a Map for visited points within a function, and heavily comment, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8544b072ee6
Rename function, add a few comments, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d2730ada95
The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d58f8529b86
Convert memoizer to Map, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6108a65dc38
Refactoring: more Set/Map usage, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/196ac1f813f9
Start searching from the most specialized csu, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6615c7b0083
Refactor findVirtualFunctions and improve comments, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/266befcb8acd
Hide command output behind --verbose flag, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/d00b9c8a7984
Fix chunking implementation, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/129559d4ac62
In-source annotations for GC suppression, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/5762a8fba027
Various refactorings, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/457cb29cad55
Handle D4 destructors, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7373c6c29e
Provide GC stacks for synthesized functions, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa434447a11b
Rewrite the test suite, add several tests, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/b641d01138ab
Update sixgill to ignore exception cleanup and properly qualify some type names
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9dedcf7163
Fix constructor recognition, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/19c13aa9b5ad
Remove unused test files, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/788ac18818c9
Test for sixgill exception handling + analysis constructor/destructor matching, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/95107c3ad9cf
Comments
https://hg.mozilla.org/integration/mozilla-inbound/rev/a73f74f718e7
Annotate mFree function pointer as not GCing, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/c95bdd426ced
Make ZoneCellIter variants to communicate nogc to the hazard analysis, r=jonco
Various GC crashes started after this landed, all backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/69518db96a4d
Flags: needinfo?(sphink)

Comment 45

3 years ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae7a6263603
Switch to using a Map for visited points within a function, and heavily comment, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9f523c94cf0
Rename function, add a few comments, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/a122f2f80ea9
The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/d91691bc2a5d
Convert memoizer to Map, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7b3fc568de
Refactoring: more Set/Map usage, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/46da36d5dd6b
Start searching from the most specialized csu, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d28cf9c0cb
Refactor findVirtualFunctions and improve comments, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab887355547d
Hide command output behind --verbose flag, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8730c90dd1
Fix chunking implementation, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fac61d360ab
In-source annotations for GC suppression, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4246cb8c2ba
Various refactorings, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5817a915a31
Handle D4 destructors, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d85ed16c20e
Provide GC stacks for synthesized functions, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/e51f08275a66
Rewrite the test suite, add several tests, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/810a121696ab
Update sixgill to ignore exception cleanup and properly qualify some type names
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a346f93e840
Fix constructor recognition, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/7876b3fd15e6
Remove unused test files, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd1752a35f57
Test for sixgill exception handling + analysis constructor/destructor matching, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/a682c8517601
Comments
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd10307c591
Annotate mFree function pointer as not GCing, r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/47dfe258f4b4
Make ZoneCellIter variants to communicate nogc to the hazard analysis, r=jonco
Assignee

Comment 46

3 years ago
(In reply to Wes Kocher (:KWierso) from comment #44)
> Various GC crashes started after this landed, all backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69518db96a4d

Yes, my warning fix was invalid. I put #ifdef DEBUG around the code, but one part of it wasn't debug-only. :(
Flags: needinfo?(sphink)

Comment 47

3 years ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f17ed092065
trail of tears - more bogus reasons for calloc() to GC found. Suppress them.
You need to log in before you can comment on or make changes to this bug.