1500247 - HashSet<gcpointer> should be treated as a gcpointer by the hazard analysis

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P1)

Description

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

From bug 1500064. HashSet and HashMap should be annotated to inherit GC pointeredness from their template parameters.

Comment 1

[Jason Orendorff [:jorendorff]]

Is this really P1? That is, are we going to work on it during the current cycle (FF66, soft freeze on 21 January)?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
This can hide a potential crashes / security vulnerabilities so I think it should be P1.  Whether Steve has time to work in it by the end of the next train is another matter.

Comment 3

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

Attachment: Make GCHash{Map,Set} inherit annotations from template params

This just worked on try, and found no problems.

Which makes me suspicious. So I am working on a self-test, where I will add uses that *should* fail with the hazard analysis, and add a new annotation mechanism saying "you should get n hazards reported in this source file." That'll require some plumbing, though.

In the meantime, I ran a try build that should fail: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=217062657&revision=5ce94f37e50affb4fc59722cf3a01a450bb4c1df

...and it didn't, so it looks like I have more work to do. Argh.

Comment 4

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

Ugh. The usual hazard thing. I had a bug where it was only inheriting GC ptrdness from explicitly listed GC pointer types, which are very rare these days, rather than inferred GC pointer types. Which means that the feature wasn't working (for anything but my too-simple test.)

Fixing that bug revealed 12 hazards. 4 are from adding the annotation to GCHash{Map,Set}. 1 of those 4 was an intentionally injected error. That means I have 3 hazards from this bug to fix, plus 8 hazards from the other types that were supposedly getting checked but actually weren't, in this case UniquePtr<T> and Maybe<T>.

Which means I need to fix those and backport them before landing the test. And I really do want to also implement a self-check as part of this. I just can't do it the way I was trying to, which was to add a JS_HAZ_EXPECT_FAILURES(n) that I'd look for whenever detecting a hazard to see if it was expected, because that will not catch the case that actually matters: no hazards detected, expectation of 1 or more.

I'm thinking I'll add an __attribute__ annotation for either the function containing the hazard (which may be inconvenient for jsapi-tests), or for a dummy type that is interpreted at the file scope. I already gather up all attribute values for all types, so that should give me what I need.

Comment 5

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

Oh yeah, and that's 12 hazards just in the JS shell. I haven't checked the full browser yet. That'll be fun.

Comment 6

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

Just an update, since I may have to pause work on this for a bit. The 12 hazards seemed to have 1 true positive, 10 false positives, and 1 intentional error. I have a fix for the real bug, and analysis improvements to remove all but 4 of the false positives. I also have the annotation working to ignore the intentional error, though I just realized that I do *not* have the critical check implemented where it ensures that the error is caught. Oops.

The full browser is showing 11 additional hazards. Most of them appear to be due to WeakCache, which inherits from GCHashMap and so is considered to need rooting. I have an annotation for it so it doesn't need to be rooted (it's swept, not traced, so it should NOT be rooted), but the annotation is processed in the wrong order and so is getting overridden. Still need to fix that.

The other thing I know of that I need to fix before landing this is some hairy UniquePtr handling. I'm still thinking through that one. It's kind of messy.

Comment 7

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

Attachment: Fix bug in handling MOZ_INHERIT_ATTRIBUTE_FROM_TEMPLATE_PARAM for GC ptrdness

I was only looking at the specifically labeled GC things, not the inferred ones. Which made the feature do basically nothing.

Comment 8

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

Attachment: Make GCHash{Map,Set} inherit annotations from template params

Bleh. This requires a number of annotations to get right: the ones for GCHashMap and GCHashSet, plus ones for explicit instantiations of those, but then that makes WeakCache<GCPointer> into something that needs rooting, which it doesn't, so more annotations to turn that off for WeakCache as well as specific instantiations of WeakCache.

Comment 9

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

Attachment: Add a mechanism to embed known hazards into test files and verify that they are caught

Use it to verify that MOZ_INHERIT_ATTRIBUTE_FROM_TEMPLATE_PARAM works.

This also cleans up some useless output from the hazard task.

Comment 10

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

Attachment: [hazards] Do not report hazards when a variable is known to hold a safe value

There are some places where we use a Maybe<AutoCheckCannotGC> to dynamically control whether GC is allowed. Now that the analysis treats Maybe<T> the same as T for the purpose of the hazard analysis, we need to teach the analysis enough data flow to be able to make this work.

Also, this allows setting GC pointers to nullptr and not have the analysis complain about the known constant value living across a GC. (This can be easily extended to NumberValue etc. if needed.)

Comment 11

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

Attachment: [hazards] End the live range of a variable passed to a move constructor via std::move()

{
      extern void foo(UniquePtr<T> u);
      UniquePtr<T> uptr = MakeUnique<T>();
      foo(std::move(uptr));
      gc();
    }

should NOT be a hazard; uptr goes through the move constructor and transfers ownership of the pointer. This is kind of complicated because what actually happens is that std::move() returns a temporary that gets passed to the move constructor, so we have to handle ending the live ranges of both the original UniquePtr as well as the temporary.

Comment 12

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

Attachment: Make GCHash{Map,Set} inherit annotations from template params

The point of this bug.

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 9033614 [details] [diff] [review]
Add a mechanism to embed known hazards into test files and verify that they are caught

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

This testUnrootedGCHashMap looks pretty unsafe.  Can we test this by adding one of the rootAnalysis/t tests or is this attribute required for other reasons?  I guess it's the intentional error you mentioned.  Could we remove that instead?

::: js/src/jsapi-tests/testGCExactRooting.cpp
@@ +163,5 @@
>    return true;
>  }
>  END_TEST(testGCRootedHashMap)
>  
> +BEGIN_TEST_WITH_ATTRIBUTES(testUnrootedGCHashMap,JS_EXPECT_HAZARDS) {

Is the idea of this to test the hazard analysis?  This doesn't look very safe.

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 9033615 [details] [diff] [review]
[hazards] Do not report hazards when a variable is known to hold a safe value

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

This looks fine but can you add a test for this?

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 9033620 [details] [diff] [review]
[hazards] End the live range of a variable passed to a move constructor via std::move()

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

This looks good but it's non-trivial so I'd like to see some tests.

Comment 16

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

Comment on attachment 9033614 [details] [diff] [review]
Add a mechanism to embed known hazards into test files and verify that they are caught

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

Sorry, I should have commented this.

Yes, this is an intentional hazard. The JS_EXPECT_HAZARD annotation will cause the job to fail if the hazard is not detected.

I will add a comment.

Comment 17

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

Attachment: [hazards] Do not report hazards when a variable is known to hold a safe value

But if I write tests, then they will find bugs! Why would I do that?

I did, and they did.

Comment 18

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

Attachment: [hazards] End the live range of a variable passed to a move constructor via std::move()

More tests, more bugs. Well, missing features this time, I guess.

Comment 19

[Jon Coppeard (:jonco)]

Comment on attachment 9034084 [details] [diff] [review]
[hazards] Do not report hazards when a variable is known to hold a safe value

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

Great, thanks for adding tests.

::: js/src/devtools/rootAnalysis/t/hazards/test.py
@@ +53,5 @@
>  assert('haz8' in hazmap)
> +
> +# safevals hazards. See comments in source.
> +assert('unsafe1' in hazmap)
> +assert('safe2' in hazmap)

Shouldn't this one be not in hazmap as it's safe?

Comment 20

[Jon Coppeard (:jonco)]

Comment on attachment 9034085 [details] [diff] [review]
[hazards] End the live range of a variable passed to a move constructor via std::move()

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

Looks good to me.

::: js/src/devtools/rootAnalysis/analyzeRoots.js
@@ +345,5 @@
> +    {
> +        const { Variable: { Name: [ fullname, shortname ] } } = callee;
> +        const [ mangled, unmangled ] = splitFunction(fullname);
> +        // Match a UniquePtr move constructor.
> +        if (unmangled.match(/::UniquePtr<[^>]*>::UniquePtr\((\w+::)*UniquePtr<[^>]*>&&/))

Is it worth factoring out a predicates for things like 'edge is UniquePtr move constructor' and similar?

Comment 21

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

For the record, the shell analysis is now green, but the browser analysis has 11 hazards. 8 of them are in generated bindings code -- false positives, but very difficult for the analysis to figure out. Example:

  StringObjectRecordOrLong arg0; // GC pointer
  StringObjectRecordOrLongArgument arg0_holder(arg0); // Conditionally roots, eventually, if needed
  done = (failed = !arg0_holder.TrySetToLong(cx, args[0], tryNext)) || !tryNext; // Can GC
  self->PassUnion22(cx, Constify(arg0)); // Uses arg0

arg0 contains bare pointers. arg0_holder roots it via a Maybe<RecordRooter<...>> that is emplaced as soon as you set the "union" to an object pointer. There's no way for the analysis to work this all out. Hm... though come to think of it, if that last line could pull from arg0_holder so that nothing ever uses arg0, this would probably all just work. That's not possible at the moment, but could be. I don't know what effect it would have on performance or code size.

Otherwise, I'll just need to annotate the heck out of this special case.

Comment 22

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

(In reply to Jon Coppeard (:jonco) from comment #19)
> > +# safevals hazards. See comments in source.
> > +assert('unsafe1' in hazmap)
> > +assert('safe2' in hazmap)
> 
> Shouldn't this one be not in hazmap as it's safe?

Er, yes. Yes, it should (not).

Test is valid, code is not. You would think that the name "safe" would imply something to me...

Comment 23

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

> Test is valid, code is not. You would think that the name "safe" would imply
> something to me...

I just realized I got that wrong too. Test is wrong, code is wrong. Test should say what you said it should, code should implement that. I've fixed it now. (It was checking for 'return nullptr' only, which shows up as '<returnval> = nullptr', when it should have been checking for 'variable = nullptr'.)

Thanks!

Comment 24

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

Attachment: Add a mechanism to embed known hazards into test files and verify that they are caught

This is identical to the previous version of this patch, except for the comment explaining what the test is for. I wasn't totally clear on whether you were ok with this self-test mechanism. I would like to have one, so I can embed tests like this using our actual types and annotations, instead of only testing with unit tests where I fake up the actual types and things.

Comment 25

[Jon Coppeard (:jonco)]

Comment on attachment 9034481 [details] [diff] [review]
Add a mechanism to embed known hazards into test files and verify that they are caught

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

Ah I see.  Yes it's good to have end-to-end testing for this.  Thanks.

Comment 26

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

Attachment: Move special-cased types to annotations

Hardcoding it in the analysis script seemed ugly even for me.

Comment 27

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

Attachment: [hazards] Ignore difficult-to-handle autogenerated DOM bindings code

Sorry, this is truly ugly.

Comment 28

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

Attachment: [hazards] Annotate and make use of the fact that some classes' zero-arg constructor (eg Maybe<T>) does not populate the instance with any GC pointers, and thus it is safe to GC until something changes

Argh! I'm an idiot. I just spent a ton of time implementing fancy fixes for what I stupidly took to be a false positive (bug 1519284) but is actually a real hazard.

I guess I'd still like to land this code? You can see the sort of case it handles from the test. It would allow a crazy person to write code like:

    struct Danger {
        Maybe<JSObject*> x;
        Optional<JSObject*> y;
    }

    Danger danger;
    gc();
    if (v.isObject()) {
      danger.x.emplace(&v.toObject());
    }

This patch plus the next will make bug 1519284 report the real hazard instead of getting distracted by one that doesn't actually matter.

Comment 29

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

Attachment: [hazards] Implement an annotation to mark that destructors will not use invalidated GC pointers

Yet more foolish effort. This patch implements an annotation for types like

    struct BoringDeath {
      Rooted<JSObject> foo;
    };

to say that the destructor never accesses any pointers stored in the instance, so variables' live ranges can be treated as ending earlier and avoiding the bizarro hazard-at-end-of-scope issue that sometimes comes up. You would do it with

    struct BoringDeath {
      Rooted<JSObject> foo;
      ~BoringDeath JS_HAZ_DOES_NOT_USE_THIS {}
    };

Comment 30

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

Attachment: [hazards] Implement an annotation to mark that destructors will not use invalidated GC pointers

Sorry, I guess I should have implemented the annotation in this patch too.

Comment 31

[Jon Coppeard (:jonco)]

Comment on attachment 9035821 [details] [diff] [review]
[hazards] Ignore difficult-to-handle autogenerated DOM bindings code

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

Well this is horrible.  We really need to sort out the rooting situation for these arguments so that we don't need to rely on this.  r+ but only because we have a plan to do that.

Comment 32

[Jon Coppeard (:jonco)]

Comment on attachment 9035828 [details] [diff] [review]
[hazards] Annotate and make use of the fact that some classes' zero-arg constructor (eg Maybe<T>) does not populate the instance with any GC pointers, and thus it is safe to GC until something changes

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

This is pretty complicated and I'd rather not add it if it's not actually needed.  Can we talk about this later?

Comment 33

[Jon Coppeard (:jonco)]

Comment on attachment 9035830 [details] [diff] [review]
[hazards] Implement an annotation to mark that destructors will not use invalidated GC pointers

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

Can we not work this out ourselves?  It strikes me that this annotation would be very easy to get wrong (e.g. if you don't realise that a Heap<T>'s destructor will call the post barrier), or for subsequent changes to invalidate it.

Comment 34

[Jon Coppeard (:jonco)]

Comment on attachment 9035828 [details] [diff] [review]
[hazards] Annotate and make use of the fact that some classes' zero-arg constructor (eg Maybe<T>) does not populate the instance with any GC pointers, and thus it is safe to GC until something changes

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

Cancelling review for now.

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e7f4d97d178cd14d693c56b925036df69fab86f
https://hg.mozilla.org/integration/mozilla-inbound/rev/57206fca017df883b2580c6c5e0eb109cd926c27
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8867abaa0f79b9ec10fb057485191f0564160c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0e97add682c71d36dbe895efad729307638f14f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3f61ce9366b8e616f6bf4d27f1e679b11d4749
https://hg.mozilla.org/integration/mozilla-inbound/rev/a55571003fbde93cb82e5ff6eeadb954ea1b7a8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/89fc662c01ee95a86213a20e89312b7925122bd4
https://hg.mozilla.org/mozilla-central/rev/89fc662c01ee
https://hg.mozilla.org/mozilla-central/rev/a55571003fbd
https://hg.mozilla.org/mozilla-central/rev/fd3f61ce9366
https://hg.mozilla.org/mozilla-central/rev/f0e97add682c
https://hg.mozilla.org/mozilla-central/rev/2a8867abaa0f
https://hg.mozilla.org/mozilla-central/rev/5e40a711ed0a
https://hg.mozilla.org/mozilla-central/rev/7e7f4d97d178