Closed Bug 1437533 Opened 2 years ago Closed Last year

NativeObject-inl.h:564:87: warning: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'class js::HeapSlot'; use assignment instead [-Wclass-memaccess]

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: sylvestre, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

from /root/firefox-gcc-last/js/src/vm/Interpreter-inl.h:25,
                  from /root/firefox-gcc-last/js/src/vm/Interpreter.cpp:11:
 /root/firefox-gcc-last/js/src/vm/NativeObject-inl.h: In static member function 'static JS::Result<js::NativeObject*, JS::OOM&> js::NativeObject::create(JSContext*, js::gc::AllocKind, js::gc::InitialHeap, js::HandleShape, js::HandleObjectGroup)':
 /root/firefox-gcc-last/js/src/vm/NativeObject-inl.h:564:87: warning: 'void* memset(void*, int, size_t)' clearing an object of non-trivial type 'class js::HeapSlot'; use assignment instead [-Wclass-memaccess]
          memset(nobj->as<JSFunction>().fixedSlots(), 0, size - sizeof(js::NativeObject));
Jeff, can you take a look? I'm not sure what will be best here. JSFunction is weird.

Surprisingly, we call memset with a variable size here, so the existing code might not be particularly fast...
Priority: -- → P2
I'm always skeptical that functions this big, with this many if-statements, can possibly benefit from being inline. Really??
Flags: needinfo?(jwalden+bmo)
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> I'm always skeptical that functions this big, with this many if-statements,
> can possibly benefit from being inline. Really??

This one isn't MOZ_ALWAYS_INLINE so may not be inlined anyway, but a lot of the perf work I'm doing these days is making sure the right things get inlined and it can easily make a 20% or more perf difference on micro-benchmarks, even for pretty large functions...
If I don't have this patch, I can't build a non-unified build at all.  (And I need to be able to do that to verify inline-function moves in subsequent patches don't create a time bomb.)
Attachment #8957992 - Flags: review?(jorendorff)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
The memset that has to go away, basically initializes everything in JSFunction or js::FunctionExtended that's not in NativeObject.  And it does so disregarding field types and other things.  (It's not quite really a variable memset-count -- it's one of two constant values.  Probably compiles into something constant with a branch somewhere.)  That really should live inside JSFunction, not in NativeObject, IMO.  Moving those field-inits into a JSFunction member function, that ordinarily would live in JSFunction-inl.h, requires that NativeObject::create move into JSFunction-inl.h.  (Plus all the things that depend on it, and the things that depend on them, &c.)

This is just code-move, not code-change.  I'll introduce the JSFunction::initFunctionSpecificFields once these functions have all moved over.

(This all is very ugh -- people should create objects using the actual type they want, not depending on a NativeObject::create workhorse that has to do garbage things like this beyond NativeObject's ambit -- but in the current system, this is the incrementally best change.)
Attachment #8957993 - Flags: review?(jorendorff)
As I said, this is mostly very silly, and really this garbage should be initializing Rust-style -- computing the values of all the relevant fields of JSFunction or js::FunctionExtended, then writing them directly into the newly-allocated JSFunction.  But that's a bigger change cutting across a bunch more things.

Hopefully this increased use of the C++ object model, doesn't make us fall over from the fact that js::Allocate<T>() does *not* allocate a T by calling a T constructor.  Not calling a T constructor, means the compiler doesn't know there's a T there.  Which should really make all subsequent use of the T, as T, basically UB.  Treeherder of debug builds suggests we're probably okay:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef91ffd4b790805582ab2f7b955ef9362a3ac42c

Will do an opt-push as well, because that sort of failure mode is certainly a ton likelier to show up in an opt build.
Attachment #8957994 - Flags: review?(jorendorff)
Personally I think it's nicer/simpler/faster to have JSFunction::create. There's some code duplication in this patch, but I think it's acceptable - if needed NativeObject::createInternal or something could be added. This passes all shell tests.
Attachment #8958039 - Flags: feedback?(jwalden+bmo)
Attachment #8958039 - Flags: feedback?(jorendorff)
Attachment #8958039 - Attachment description: Alternative fix → Alternative fix (prototype, parts stolen from Waldo's patch)
Comment on attachment 8958039 [details] [diff] [review]
Alternative fix (prototype, parts stolen from Waldo's patch)

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

Try-pushes of my patches here indicate...well, I dunno if they indicate anything, exactly, but the failure count in the pushes *may* indicate reason for concern.  Who can say any more?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef91ffd4b790805582ab2f7b955ef9362a3ac42c&selectedJob=167148024
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65d7d6cae9ce932c53a92eb7e959cc77ec7af0c1&selectedJob=167335328

The Windows g2 failures in that second push seem possibly most worrisome -- this push with none of the relevant patches may eventually indicate if there was something wrong with my approach:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8095e71dfb5325f4c1f0569448e656d6d84dcc87

Anyway -- basic idea looks fine to me.

::: js/src/vm/JSFunction-inl.h
@@ +130,5 @@
> +    const js::Class* clasp = group->clasp();
> +    MOZ_ASSERT(clasp->isJSFunction());
> +    MOZ_ASSERT(dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan(), clasp) == 0);
> +
> +    JSObject* obj = js::Allocate<JSObject>(cx, kind, /* nDynamicSlots = */ 0, heap, clasp);

Add a constexpr local for nDynamicSlots.

@@ +134,5 @@
> +    JSObject* obj = js::Allocate<JSObject>(cx, kind, /* nDynamicSlots = */ 0, heap, clasp);
> +    if (!obj)
> +        return cx->alreadyReportedOOM();
> +
> +    JSFunction* fun = static_cast<JSFunction*>(obj);

I might perhaps still cast to NativeObject here to do all the calls up to the initFunctionSpecificFields call, *then* do the as<JSFunction>() to cast after that point.  That makes clear that all these calls/inits up to here affect NativeObject fields, then the call affects the remaining JSFunction-specific fields.

Given there's only one caller, *and* given this caller is clearly JSFunction-centric, probably initFunctionSpecificFields can just be folded directly into this, IMO.

@@ +146,5 @@
> +    MOZ_ASSERT(shape->slotSpan() == 0);
> +
> +    fun->initFunctionSpecificFields(kind);
> +
> +    fun = SetNewObjectMetadata(cx, fun);

Assert

  MOZ_ASSERT(!clasp->shouldDelayMetadataBuilder(),
             "Function has no extra data hanging off it, that wouldn't be "
             "allocated at this point, that would require delaying the "
             "building of metadata for it");
Attachment #8958039 - Flags: feedback?(jwalden+bmo) → feedback+
Flags: needinfo?(jwalden+bmo)
Mm, so

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8095e71dfb5325f4c1f0569448e656d6d84dcc87&selectedJob=167505146

suggests the Windows g2 failures may be unrelated to this change.  Maybe I tryservered on an unfortunate rev, don't have time to check now tho.
Comment on attachment 8957992 [details] [diff] [review]
Properly declare JSTracer respecting JS_PUBLIC_API to avoid compile errors about visibility mismatch in non-unified builds

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

Ew
Attachment #8957992 - Flags: review?(jorendorff) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e15ed6f5914
Properly declare JSTracer respecting JS_PUBLIC_API to avoid compile errors about visibility mismatch in non-unified builds.  r=jorendorff
Keywords: leave-open
Comment on attachment 8958039 [details] [diff] [review]
Alternative fix (prototype, parts stolen from Waldo's patch)

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

Easy yes.
Attachment #8958039 - Flags: feedback?(jorendorff) → feedback+
Comment on attachment 8957994 [details] [diff] [review]
Initialize JSFunction-specific fields and potential extended slots in NativeObject::create without using memset to zero out fields that are non-trivial classes

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

(clearing r? in favor of jandem's patch)
Attachment #8957994 - Flags: review?(jorendorff)
Attachment #8957993 - Flags: review?(jorendorff)
Attached patch Updated patchSplinter Review
Attachment #8958039 - Attachment is obsolete: true
Attachment #8973631 - Flags: review?(jwalden+bmo)
Comment on attachment 8973631 [details] [diff] [review]
Updated patch

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

The various fun-> initializations added here technically aren't C++-kosher, if I hadn't mentioned it, because we haven't instantiated values of those types at those locations.  (Just as we haven't for JSObject or JSFunction.)  But if this works, at least for now it's the best we can do.

::: js/src/vm/JSFunction-inl.h
@@ +102,5 @@
> +    const js::Class* clasp = group->clasp();
> +    MOZ_ASSERT(clasp->isJSFunction());
> +
> +    static constexpr size_t NumDynamicSlots = 0;
> +    MOZ_ASSERT(dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan(), clasp) == 0);

Not == NumDynamicSlots?

::: js/src/vm/NativeObject-inl.h
@@ +532,5 @@
>      debugCheckNewObject(group, shape, kind, heap);
>  
>      const js::Class* clasp = group->clasp();
>      MOZ_ASSERT(clasp->isNative());
> +    MOZ_ASSERT(!clasp->isJSFunction(), "Should use JSFunction::create");

Don't caps the reason-string.
Attachment #8973631 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] from comment #16)
> > +    static constexpr size_t NumDynamicSlots = 0;
> > +    MOZ_ASSERT(dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan(), clasp) == 0);
> 
> Not == NumDynamicSlots?

I was thinking about doing that when I wrote this but wasn't sure about it. Maybe using NumDynamicSlots is a bit more explicit about the invariant we're asserting; I'll change it.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/064597ea4795
Don't use memset to initialize JSFunction extended slots. Parts of this patch written by Waldo. r=jwalden
https://hg.mozilla.org/mozilla-central/rev/064597ea4795
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.