Closed Bug 1467632 Opened 2 years ago Closed 2 years ago

"Warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct AsmJSGlobal::CacheablePod’" since bug AsmJSGlobal::CacheablePOD became non-trivial

Categories

(Core :: Javascript: WebAssembly, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: Waldo, Assigned: lth)

References

Details

Attachments

(2 files, 3 obsolete files)

Compiling SpiderMonkey with modern GCC warns when compiling AsmJS.cpp:

In file included from /home/jwalden/moz/after/js/src/dbg/dist/include/js/HashTable.h:18,
                 from /home/jwalden/moz/after/js/src/dbg/dist/include/js/TracingAPI.h:11,
                 from /home/jwalden/moz/after/js/src/dbg/dist/include/js/GCPolicyAPI.h:44,
                 from /home/jwalden/moz/after/js/src/dbg/dist/include/js/RootingAPI.h:22,
                 from /home/jwalden/moz/after/js/src/dbg/dist/include/js/CallArgs.h:73,
                 from /home/jwalden/moz/after/js/src/jsapi.h:29,
                 from /home/jwalden/moz/after/js/src/dbg/dist/include/js/UbiNodeCensus.h:15,
                 from /home/jwalden/moz/after/js/src/vm/UbiNodeCensus.cpp:7,
                 from /home/jwalden/moz/after/js/src/dbg/js/src/Unified_cpp_js_src40.cpp:2:
/home/jwalden/moz/after/js/src/dbg/dist/include/mozilla/PodOperations.h: In instantiation of ‘void mozilla::PodZero(T*) [with T = AsmJSGlobal::CacheablePod]’:
/home/jwalden/moz/after/js/src/wasm/AsmJS.cpp:167:30:   required from here
/home/jwalden/moz/after/js/src/dbg/dist/include/mozilla/PodOperations.h:32:9: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘struct AsmJSGlobal::CacheablePod’; use assignment or value-initialization instead [-Wclass-memaccess]
   memset(aT, 0, sizeof(T));
   ~~~~~~^~~~~~~~~~~~~~~~~~
In file included from /home/jwalden/moz/after/js/src/dbg/js/src/Unified_cpp_js_src40.cpp:47:
/home/jwalden/moz/after/js/src/wasm/AsmJS.cpp:133:12: note: ‘struct AsmJSGlobal::CacheablePod’ declared here
     struct CacheablePod {
            ^~~~~~~~~~~~

The warning occurs because the AsmJSGlobal constructor (well, the non-default one) does PodZero(this).  But when CacheablePod became *non*-POD, by dint of becoming non-trivial when CacheablePod::V::U gained a user-defined default constructor, it became non-kosher to memset AsmJSGlobal to zeroes.

Fundamentally the problem is that CacheablePod is *not* POD any more (because it's non-trivial because it has a user-defined default constructor), and so you can't do things like memcpy and memset and so on on it.  The correct fix would be something along the lines of renaming CacheablePod because it isn't, giving the |pod| field WASM_DECLARE_SERIALIZABLE support that does proper field-by-field transliteration, then having AsmJSGlobal's support invoke that -- combined with creating AsmJSGlobals with their fields correctly initialized.

I started on this briefly, but it's a bit of a mess of code -- you either need to add umpteen constructors to initialize fields just so (percolating into the various subtypes within CacheablePod, CacheablePod::U, and CacheablePod::V), or you need to use initializer lists.  But I don't think you can do the latter when the init-list contains a non-trivial type (like Val), so it sprawls some.  Maybe you can see your way through this better than I can.

Anyway.  Ball's in your racket -- could you please fix this?  We were warning-free with modern gcc til this landed, thanks to a bunch of fixes for this warnings by me a bit ago.  http://whereswalden.com/2018/05/21/psa-stop-using-mozillapodzero-and-mozillapodarrayzero/
Flags: needinfo?(lhansen)
Sigh, yes.
Flags: needinfo?(lhansen)
Attached patch bug1467632-repoddify.patch (obsolete) — Splinter Review
The simplest fix is really to turn CacheablePod back into a Pod type.  This turns out to be easy now and not very ugly.  WIP patch here.  I need to repro the problem with a recent GCC and just check that this patch fixes it.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
This patch does fix the warning.

(On the other hand, the fixed code causes gcc8 to crash with an internal compiler error.  But once the rockets are up ...)
Attached patch bug1467632-repoddify.patch (obsolete) — Splinter Review
Attachment #8984359 - Attachment is obsolete: true
Attachment #8984373 - Flags: review?(luke)
Attachment #8984373 - Flags: feedback?(jwalden+bmo)
Comment on attachment 8984373 [details] [diff] [review]
bug1467632-repoddify.patch

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

::: js/src/wasm/WasmTypes.h
@@ +616,5 @@
>  {
> +    // We hold the Code here, not the ValType, so that Val can be POD.  Should
> +    // we need to also hold the refTypeIndex for a Ref type, we can turn this
> +    // into typeBits_, or we can add a separate field for the refTypeIndex.
> +    ValType::Code type_;

Sorry if this is basic but: Val itself has a ctor; does that already make it non-POD?  Or does the new expanded definition of POD allow some degree of trivial ctor and then could ValType have such a trivial ctor making it POD too?
It seems that there is some degree of freedom for the default constructor for Val as all the members are POD types; the default constructor for Val need do nothing.

I'm pretty sure ValType could have a similar trivial default constructor, but at the cost of error checking everywhere - its default constructor now creates a unique invalid bit pattern, with an invalid code and an invalid payload.  I did think about this, and it seemed to me that it it would be better to have a local, and minor, abstraction break where we need data to be POD than to have uninitialized ValType data everywhere else.
So a data type can't be POD if it has a default ctor with an initializer?  It seems like, for the same reason as ValType, we'd want Val to keep its initializing ctor.  Could we just remove the PodZero() from AsmJSGlobal() and add a few more default ctors to AsmJSGlobal::CacheablePod?
Flags: needinfo?(lhansen)
Comment on attachment 8984373 [details] [diff] [review]
bug1467632-repoddify.patch

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

::: js/src/wasm/WasmTypes.h
@@ +616,5 @@
>  {
> +    // We hold the Code here, not the ValType, so that Val can be POD.  Should
> +    // we need to also hold the refTypeIndex for a Ref type, we can turn this
> +    // into typeBits_, or we can add a separate field for the refTypeIndex.
> +    ValType::Code type_;

If Val has a user-defined default or copy constructor, it is non-trivial and therefore not POD.  Basically the idea of trivial initialization is "you don't have to do anything to create an instance", i.e. no vtable ptr to set, no members to set, etc.

So to comment 7's point, no, we can't just add more default constructors.  :-|
Attachment #8984373 - Flags: feedback?(jwalden+bmo) → feedback+
Let's block this until Benjamin's changes to Val have landed (this week or early next week, is my guess) and then reassess the situation.

I also think the change I proposed using uint32_t or ValType::Code can probably be improved by wrapping the "representation" of whatever value we want to serialize in some POD type.
Depends on: 1450261
Comment on attachment 8984373 [details] [diff] [review]
bug1467632-repoddify.patch

sgtm
Attachment #8984373 - Flags: review?(luke)
Clearing ni? until the dust settles.
Sigh.
Flags: needinfo?(lhansen)
Component: JavaScript Engine → Javascript: Web Assembly
This is similar to the previous patch but keeps the changes local to AsmJS.cpp by using the already-introduced representation type for ValType, PackedTypeCode, to represent a ValType, and by specializing LitVal within AsmJS.cpp as LitValPOD.
Attachment #8984373 - Attachment is obsolete: true
Attachment #9002494 - Flags: review?(luke)
Attachment #9002494 - Flags: review?(jwalden+bmo)
Priority: -- → P3
Comment on attachment 9002494 [details] [diff] [review]
bug1467632-repoddify-v2.patch

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

Thanks; nice to have this AsmJS.cpp-local.
Attachment #9002494 - Flags: review?(luke) → review+
Comment on attachment 9002494 [details] [diff] [review]
bug1467632-repoddify-v2.patch

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

::: js/src/wasm/AsmJS.cpp
@@ +110,5 @@
> +{
> +    PackedTypeCode valType_;
> +    union U {
> +        uint32_t  i32_;
> +        uint64_t  i64_;

Name the fields |u|-prefixed because they're unsigned?

@@ +138,5 @@
> +            MOZ_CRASH("Can't happen");
> +        }
> +    }
> +};
> +

static_assert(std::is_pod<LitValPod>::value,
              "must be POD to be simply serialized/deserialized");

Technically we could relax this to some form of triviality, but I'm much too lazy to think through precisely what aspects of triviality could be asserted.  Plus some STLs' support for std::is_trivially_* is bad, so at best you could only be std::is_trivial or something instead of fully precise.
Attachment #9002494 - Flags: review?(jwalden+bmo) → review+
There was actually another problem like this, with ReadScalar, fairly easily fixed by specializing ReadScalar in the case of ExprType.
Attachment #9002672 - Flags: review?(luke)
Get the syntax right for specializing a function defined within a namespace.

(I had some candidate pop-culture references to insert here to make fun of this adjustment, but they all turned out to be pop-culture references to the Old Testament, which I think says something about my reaction every time I run into this particular wrinkle of C++.  I'm sure there's a Good Reason I can't just namespace-qualify the identifier.  I'm sure there's a Good Reason why compilers will have to be asked specially to tell me that I will be sorry, later, when I try to commit the patch.)
Attachment #9002672 - Attachment is obsolete: true
Attachment #9002672 - Flags: review?(luke)
Attachment #9002773 - Flags: review?(luke)
Comment on attachment 9002773 [details] [diff] [review]
bug1467632-specialize-readscalar-v2.patch

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

Haha, who can really ever know.
Attachment #9002773 - Flags: review?(luke) → review+
(In reply to Lars T Hansen [:lth] from comment #17)
> (I had some candidate pop-culture references to insert here to make fun of
> this adjustment, but they all turned out to be pop-culture references to the
> Old Testament, which I think says something about my reaction every time I
> run into this particular wrinkle of C++.

All the cool kids are doing it!  https://bugzilla.mozilla.org/show_bug.cgi?id=761723#c91
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0883c99c897
Make AsmJSGlobal's pod field be POD. r=luke, r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3d1b33b0e9
Specialize ReadScalar<ExprType>.  r=luke
https://hg.mozilla.org/mozilla-central/rev/a0883c99c897
https://hg.mozilla.org/mozilla-central/rev/aa3d1b33b0e9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.