Closed Bug 1013531 Opened 6 years ago Closed 6 years ago

Add AutoCheckCannotGC and split up meaning of AutoAssertNoGC


(Core :: JavaScript: GC, defect)

Not set





(Reporter: sfink, Assigned: terrence)


(Blocks 1 open bug)



(3 files)

On IRC, we decided to make:

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

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

 - MOZ_ASSERT if something GCs within its scope GCs

Only I'm currently bikeshedding on the AutoSuppressAnalysis name.
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. :)
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.)
(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.
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.
Assignee: nobody → terrence
Passes shell tests, but probably broke something in the hazard analysis. Let's find out:
Attachment #8430174 - Flags: review?(sphink)
Seems I also forgot to move the #endif:
Comment on attachment 8430174 [details] [diff] [review]

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


(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.
Attachment #8430174 - Flags: review?(sphink) → review+
Depends on: 1005978
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:
Depends on: 1018555
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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
Resolution: FIXED → ---
Attachment #8432434 - Flags: review?(terrence)
Comment on attachment 8432434 [details] [diff] [review]

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

Stealing as requested.
Attachment #8432434 - Flags: review?(terrence) → review+
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) :
Attachment #8432499 - Flags: review?(jcoppeard)
Comment on attachment 8432499 [details] [diff] [review]
Kill build warnings

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

Great, thanks for fixing!
Attachment #8432499 - Flags: review?(jcoppeard) → review+
Duplicate of this bug: 1018555
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.