Closed Bug 929200 Opened 6 years ago Closed 6 years ago

MOZ_NONHEAP_CLASS analysis doesn't know that JSContext::new_ is a heap allocation function

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: jimb, Assigned: jcranmer)

Details

(Whiteboard: [qa-])

Attachments

(2 files)

Static analysis doesn't enforce the MOZ_NONHEAP_CLASS attribute in SpiderMonkey code, because SpiderMonkey uses its own new_ operator.

The MOZ_NONHEAP_CLASS attribute is inherited by classes that contain members whose type has that attribute. Thus, because JS::Handle is non-heap, JS::CompileOptions and js::ParseTask are too. However, js::ParseTask is allocated on the heap here:

http://dxr.mozilla.org/mozilla-central/source/js/src/jsworkers.cpp#l270

The problem is that JSContext::new_ is defined in terms of placement new here:

http://dxr.mozilla.org/mozilla-central/source/js/public/Utility.h#l246

and the analysis does not recognize placement 'new' as a heap allocation operation. However, SpiderMonkey code is not permitted to use plain 'new'; placement 'new', in the form of JSContext::new_ and JSRuntime::new_, is the only variety ever used. So the analysis has no effect on SpiderMonkey code.
The __attribute__ positioning for C++ functions is annoying due to -Wgcc-compat, but that's why C99 introduced _Pragma, isn't it? :-)

I didn't enable the analysis for JS yet because that makes things go fail:
/src/trunk/mozilla/js/src/jsworkers.cpp:270:9: error: variable of type
      'js::ParseTask' is not valid on the heap
        cx->new_<ParseTask>(workercx.get(), options, chars, length,
        ^
/src/trunk/mozilla/js/src/jsworkers.h:394:20: note: 'js::ParseTask' is a
      non-heap class because member 'options' is a non-heap class
      'JS::CompileOptions'
    CompileOptions options;
                   ^
/src/trunk/mozilla/js/src/jsapi.h:3450:23: note: 'JS::CompileOptions' is a
      non-heap class because member 'element' is a non-heap class
      'Handle<JSObject *>'
    Handle<JSObject*> element;
                      ^

I see no easy way to fix that. jimb?
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #823182 - Flags: review?(jwalden+bmo)
Flags: needinfo?(jimb)
Yes; those need to be fixed before the analysis will pass.

Bug 887077 takes care of CompileOptions. There is no bug for the spewers, but bug 892643 will provide the type needed for that.
Flags: needinfo?(jimb)
Comment on attachment 823182 [details] [diff] [review]
Add MOZ_HEAP_ALLOCATOR

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

I sort of tried to understand the clang plugin bits, but that part's somewhat rubberstampy.

::: mfbt/Attributes.h
@@ +413,5 @@
>  #ifdef MOZ_CLANG_PLUGIN
>  # define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))
>  # define MOZ_STACK_CLASS __attribute__((annotate("moz_stack_class")))
>  # define MOZ_NONHEAP_CLASS __attribute__((annotate("moz_nonheap_class")))
> +/* It turns out that clang doesn't like void func() __attribute__ {} without a

/*
 * It turns out...
 * warning...
 * anyways...
 */

@@ +417,5 @@
> +/* It turns out that clang doesn't like void func() __attribute__ {} without a
> + * warning, so use pragmas to disable the warning. This code won't work on GCC
> + * anyways, so the warning is safe to ignore.
> + */
> +# define MOZ_HEAP_ALLOCATOR \

Looks pre-existing, but style would have this (and the ones in the other arm) indented two spaces, not one:

#ifdef MOZ_CLANG_PLUGIN
#  define ...
#else
#  define ...
#endif

@@ +421,5 @@
> +# define MOZ_HEAP_ALLOCATOR \
> +  _Pragma("GCC diagnostic push") \
> +  _Pragma("GCC diagnostic ignored \"-Wgcc-compat\"") \
> +  __attribute__((annotate("moz_heap_allocator"))) \
> +  _Pragma("GCC diagnostic pop")

If this is purely clang-targeted, using "clang diagnostic blah" seems slightly preferable to "GCC" for clarity.
Attachment #823182 - Flags: review?(jwalden+bmo) → review+
It seems that the static analysis now passes, so it's time to add the part that makes it actually used.
Attachment #8340898 - Flags: review?(jimb)
Comment on attachment 8340898 [details] [diff] [review]
Annotate JS allocation failures with MOZ_HEAP_ALLOCATOR

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

I don't really know what MOZ_HEAP_ALLOCATOR does, but if it's an attribute that marks a function as a heap allocation function, then this looks right.
Attachment #8340898 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/910ea4847f84
https://hg.mozilla.org/mozilla-central/rev/baf7b0f169a1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.