1013531 - Add AutoCheckCannotGC and split up meaning of AutoAssertNoGC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

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

On IRC, we decided to make:

AutoCheckCannotGC
 - the static analysis will fail if anything within its scope can GC

AutoSuppressAnalysis
 - an annotation that tells the analysis that it can assume nothing within its scope can GC

AutoAssertOnGC
 - MOZ_ASSERT if something GCs within its scope GCs

Only I'm currently bikeshedding on the AutoSuppressAnalysis name.

Comment 1

[Andrew McCreight [:mccr8]]

AutoSuppressAnalysis is a generic name, given that we're already running multiple static analyses on TBPL alone.  At least throw a GC in there somewhere. :)

Comment 2

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

I was thinking AutoAnnotateNoGC or AutoAnnotateCannotGC, but wasn't sure if "annotate" means anything to most people. (Ok, it means *something*, but I'm not sure if it implies informing a static analysis about some characteristic of the code.)

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] from comment #2)

Can we make these assert on allocation, not just check whether a GC occurred after the fact?  This would give us a much stronger degree of confidence that these assertions were correct, and also point us at the problem if it happened.

I had a go at doing this myself but there was some complication (I think it was interaction with suppressed GC), so it may be separate assertions for no allocation / no gc is the way to go.

Comment 4

[Terrence Cole [:terrence]]

Taking. Looks like it is going to be useful for bug 1012742 as well, since we're relying on the fact that void* is "not a gc thing" across all the random callbacks in the CC.

Comment 5

[Terrence Cole [:terrence]]

Attachment: clean_up_hazard_suppression-v0.diff

Passes shell tests, but probably broke something in the hazard analysis. Let's find out:
https://tbpl.mozilla.org/?tree=Try&rev=f352d1ad28bf

Comment 6

[Terrence Cole [:terrence]]

Seems I also forgot to move the #endif:
https://tbpl.mozilla.org/?tree=Try&rev=679df735642b

Comment 7

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

Comment on attachment 8430174 [details] [diff] [review]
clean_up_hazard_suppression-v0.diff

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

::: dom/bindings/CallbackObject.cpp
@@ +80,5 @@
>    JSContext* cx = nullptr;
>    nsIGlobalObject* globalObject = nullptr;
>  
>    {
> +    JS::AutoSuppressGCAnalysis nogc;

The new name has a good result: seeing it makes me immediately think "why? why? why?"

This needs a comment. Maybe bz should write it?

::: js/public/GCAPI.h
@@ +402,3 @@
>  
> +/*
> + * This is recognized as a GC thing pointer by the static analsysis. It is

Ttoo manny ssses.

@@ +403,5 @@
> +/*
> + * This is recognized as a GC thing pointer by the static analsysis. It is
> + * useful when dealing with internal pointers to GC things where the GC thing
> + * itself may not be present for the static analysis: e.g. acquiring inline
> + * chars from a JSString* on the heap.

I think the comment needs to be more clear.

Place AutoCheckCannotGC in scopes that you believe can never GC. These annotations will be verified both dynamically via AutoAssertOnGC, and statically with the rooting hazard analysis (implemented by making the analysis consider AutoCheckCannotGC to be a GC pointer, and therefore complain if it is live across a GC call.) It is useful...

::: js/src/gc/GCRuntime.h
@@ +384,5 @@
>      ConservativeGCData    conservativeGC;
> +
> +    /*
> +     * Some regions of code are hard for the static rooting hazard analysis to
> +     * understand. In those cases, we switch the static analysis for a dynamic

s/switch/trade/

(switch switch with trade?)

::: js/src/jsgc.cpp
@@ +5478,5 @@
> +    if (rt->gc.inUnsafeRegion > 0) {
> +        char msgbuf[1024];
> +        JS_snprintf(msgbuf, sizeof(msgbuf), "[AutoAssertOnGC] possible GC in GC-unsafe region");
> +        MOZ_ReportAssertionFailure(msgbuf, __FILE__, __LINE__);
> +        MOZ_CRASH();

You don't want to use MOZ_CRASH("[AutoAssertOnGC] possible GC in GC-unsafe region")?

If there's really a reason to do your own string printing, then should the final MOZ_CRASH  be MOZ_REALLY_CRASH? I guess I'm not sure exactly what you want the output to be.

Comment 8

[Terrence Cole [:terrence]]

Green try run minus hazards at:
https://tbpl.mozilla.org/?tree=Try&rev=679df735642b
And with hazards fixed:
https://tbpl.mozilla.org/?tree=Try&rev=7d3b6095e178

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d61bcf08362

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/29ca8bc78484

https://tbpl.mozilla.org/php/getParsedLog.php?id=40651664&tree=Mozilla-Inbound

Comment 10

[Terrence Cole [:terrence]]

A new usage of AutoAssertNoGC got added to the browser between my implementation and the rebase for landing.

According to try, this is no longer needed:
https://tbpl.mozilla.org/?tree=Try&rev=30be6c0e7c43

https://hg.mozilla.org/integration/mozilla-inbound/rev/57014191cb43

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/57014191cb43

Comment 12

[Jon Coppeard (:jonco)]

Reopening for build errors with clang 3.3 on linux:

In file included from /home/jon/work/dev/js/src/jsarray.cpp:7:
In file included from /home/jon/work/dev/js/src/jsarray.h:12:
In file included from /home/jon/work/dev/js/src/jsobj.h:21:
In file included from /home/jon/work/dev/js/src/gc/Barrier.h:14:
In file included from /home/jon/work/dev/js/src/gc/StoreBuffer.h:22:
In file included from /home/jon/work/dev/js/src/gc/Nursery.h:19:
../../dist/include/js/GCAPI.h:419:7: error: visibility does not match previous declaration
class JS_PUBLIC_API(AutoCheckCannotGC) : public AutoAssertOnGC
      ^
/home/jon/work/dev/js/src/jstypes.h:73:30: note: expanded from macro 'JS_PUBLIC_API'
#  define JS_PUBLIC_API(t)   MOZ_EXPORT t
                             ^
../../dist/include/mozilla/Types.h:44:45: note: expanded from macro 'MOZ_EXPORT'
#    define MOZ_EXPORT       __attribute__((visibility("default")))
                                            ^
/home/jon/work/dev/config/gcc_hidden.h:6:13: note: previous attribute is here
#pragma GCC visibility push(hidden)
            ^
error generated.
make[3]: *** [jsarray.o] Error 1

Comment 13

[Jon Coppeard (:jonco)]

Attachment: fix-forward-declaration

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8432434 [details] [diff] [review]
fix-forward-declaration

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

Stealing as requested.

Comment 15

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4957e3ceafb

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Kill build warnings

Seems it introduced some build warnings (order of init in the ctor), not sure they're all due to this patch, but at least one is.

In file included from /home/ben/code/moz/inbound/js/src/vm/Runtime.h:29:0,
                 from /home/ben/code/moz/inbound/js/src/jscntxt.h:15,
                 from /home/ben/code/moz/inbound/js/src/jsbool.cpp:15,
                 from /home/ben/code/moz/builds/wd64/js/src/Unified_cpp_js_src7.cpp:2:
/home/ben/code/moz/inbound/js/src/gc/GCRuntime.h: In constructor ‘js::gc::GCRuntime::GCRuntime(JSRuntime*)’:
/home/ben/code/moz/inbound/js/src/gc/GCRuntime.h:560:19: warning: ‘js::gc::GCRuntime::helperState’ will be initialized after [-Wreorder]
     GCHelperState helperState;
                   ^
/home/ben/code/moz/inbound/js/src/gc/GCRuntime.h:550:10: warning:   ‘bool js::gc::GCRuntime::alwaysPreserveCode’ [-Wreorder]
     bool                  alwaysPreserveCode;
          ^
In file included from /home/ben/code/moz/builds/wd64/js/src/Unified_cpp_js_src7.cpp:119:0:
/home/ben/code/moz/inbound/js/src/jsgc.cpp:1038:1: warning:   when initialized here [-Wreorder]
 GCRuntime::GCRuntime(JSRuntime *rt) :
 ^
In file included from /home/ben/code/moz/inbound/js/src/vm/Runtime.h:29:0,
                 from /home/ben/code/moz/inbound/js/src/jscntxt.h:15,
                 from /home/ben/code/moz/inbound/js/src/jsbool.cpp:15,
                 from /home/ben/code/moz/builds/wd64/js/src/Unified_cpp_js_src7.cpp:2:
/home/ben/code/moz/inbound/js/src/gc/GCRuntime.h:558:36: warning: ‘js::gc::GCRuntime::lockOwner’ will be initialized after [-Wreorder]
     mozilla::DebugOnly<PRThread *>   lockOwner;
                                    ^
/home/ben/code/moz/inbound/js/src/gc/GCRuntime.h:546:9: warning:   ‘int js::gc::GCRuntime::inUnsafeRegion’ [-Wreorder]
     int inUnsafeRegion;
         ^
In file included from /home/ben/code/moz/builds/wd64/js/src/Unified_cpp_js_src7.cpp:119:0:
/home/ben/code/moz/inbound/js/src/jsgc.cpp:1038:1: warning:   when initialized here [-Wreorder]
 GCRuntime::GCRuntime(JSRuntime *rt) :
 ^

Comment 17

[Jon Coppeard (:jonco)]

Comment on attachment 8432499 [details] [diff] [review]
Kill build warnings

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

Great, thanks for fixing!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4957e3ceafb
https://hg.mozilla.org/mozilla-central/rev/bb39c677d650