1461556 - Don't use mozilla::PodZero in a bunch of places to initialize values of non-trivial type

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jeff Walden [:Waldo]]

More of the same memset warning dumb you already fixed in one place or another.  Engine is littered with this stuff, and basically it makes gcc useless for building for me, so time to clean up a bunch.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch for bunches of easy cases

There remains more to do on this front, but some remaining cases are (wait for it) non-trivial to fix.  For example, New{Global,Module,...}ScopeData wants to PodCopy arrays of BindingNames onto the ends of {Global,Module,...}Scope::Data instances, but BindingName is non-trivial so this is no good.  That *particular* concern, I'm going to deal with in another bug and patch, 'cause it'll be a chunk of work not to mix with other things.  I don't know how many other cases are left past that; warnings in this gunk tend to litter compiles to the point of making logs unreadable.

But these changes are all basically simple -- mostly just relying on in-class initializers to zero things, leaning on braced-init to zero stuff (I can cite spec chapter and verse to you if required) the easy/lazy way.

This patch isn't tryservered yet -- obviously I'll do that before landing any of it.  I would expect compilers to get this stuff right now, but you never know.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8975716 [details] [diff] [review]
Patch for bunches of easy cases

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

::: js/src/vm/Caches.h
@@ +153,3 @@
>  
> +    NewObjectCache()
> +      : entries{} // zeroes out the array

I didn't know there was this difference between entries() and entries{}. Also, the |x_ = 0| initialization syntax is much nicer than using a separate x_(0) in the initializer list :)

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> I didn't know there was this difference between entries() and entries{}.

There mostly isn't, actually.

Value-initializing per C++11 [dcl.init]p7 either calls the default constructor of a class, zero-initializes classes without user-provided constructors (that is, fills in all members with 0 converted to the member type), recursively value-initializes array members, or zero-initializes other types.  Which basically means fill in the whole value with zeroes, in practice (literal zero bytes, unless there's some pointer-to-member or pointer-to-virtual-member or pointer-to-virtual-member-function or derived-class null pointer that isn't represented by all zeroes -- or some non-IEEE-754 floating point representation where 0 isn't represented by all zeroes).

Then, for what () versus {} in member-init lists does:

If the initialization is of an *object*, per C++11 [dcl.init]p10, "An object whose initializer is an empty set of parentheses, i.e., (), shall be value-initialized."  And per C++11 [dcl.init.list]p3, "If the initializer list has no elements and T is a class type with a default constructor, the object is value-initialized."

If the initialization is of an aggregate -- that is, an array, or a class with no base class and without any special construction behavior (vtable, must invoke any user-defined constructor, some member must be initialized, etc.) whose non-static data members are all public -- then per C++11 [dcl.init.aggr]p7, "If there are fewer initializer-clauses in the list than there are members in the aggregate, then each member not explicitly initialized shall be initialized from an empty initializer list", which per above means value-initialize.

Or if it's of a class, (x, y, z) and {x, y, z} go through overload resolution and both find/invoke the corresponding constructor.

Or "if the initializer list has no elements, the object is value-initialized", or () will value-initialize.

(And some silly obscure cases ignored that don't matter here.)

There is no real difference between () and {} here, just {} by virtue of being distinct/new/different might stick in the reader's mind as actually doing stuff where you might not have thought of () as doing so.

> Also, the |x_ = 0| initialization syntax is much nicer than using a separate
> x_(0) in the initializer list :)

Troof.

Comment 4

[Jan de Mooij [:jandem]]

Thanks for the explanation!

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Initialize various asm.js structures using in-class initializers, not PodZero

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Call memset on a void*, not a T*, in js_delete_poison to avoid memset-on-nontrivial warnings with gcc that don't matter for an object whose lifetime is about to end

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Don't memset-zero the BacktrackingAllocator::vregs array of non-trivial VirtualRegister instances

Plus some driveby style-fixes.

Comment 8

[Jeff Walden [:Waldo]]

There are still roughly a handful of other issues to go.  I'm not sure how many of them have relatively simple fixes -- a couple looked tricky on first try as my day was winding down earlier.  (This now is just me popping on quickly to at least keep the queue going, tho I remain in no great hurry overall on fixing this/landing these.)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ead72cce7f0e
Don't use mozilla::PodZero in a bunch of places to initialize values of non-trivial type.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/19d2aace5b3c
Initialize various asm.js structures using in-class initializers, not PodZero.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8411d78d58a
Call memset on a void*, not a T*, in js_delete_poison to avoid memset-on-nontrivial warnings with gcc that don't matter for an object whose lifetime is about to end.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/e016aa76775e
Don't memset-zero the BacktrackingAllocator::vregs array of non-trivial VirtualRegister instances.  r=jandem

Comment 11

[Jeff Walden [:Waldo]]

More patches of moderate simplicity coming here, so leaving open.  A couple will be their own bugs, but the easier stuff I'll just keep dumpin' here.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Add a JSScript constructor to initialize JSScript inside JSScript::Create without using PodZero

I'm sorry.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Give uint8_clamped a defaulted (and also trivial) default constructor, copy constructor, and copy-assignment operator

Right now the typed array element ops stuff in SharedOps/UnsharedOps in TypedArrayObject-inl.h uses Pod* functions between same-type elements.  Works C++-compatibly with everything but uint8_clamped, which has good reason to be weird.  But for things that deal with triviality, uint8_clamped doesn't have to be weird, and defaulted functions will do the trick exactly.  So, do that.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Don't use PodCopy/PodMove to implement typed-array element-to-element copying

Bog-standard std::copy and std::copy_n are readily optimized to the same thing, and they don't have a non-obvious requirement that the type being copied be trivial.

Technically this isn't necessary and only the prior patch is to eliminate warnings.  Or you could take just this patch, and suddenly triviality worries would not exist.  Either way.

But this seems like a good change in its own right, so taking both seems ideal to me.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Rename TypeSet::clone to TypeSet::cloneIntoUninitialized to indicate that it freshly initializes the TemporaryTypeSet* provided to it. Also removes existing code that, quite unnecessarily, partly initialized that argument

It turns out this clone function already placement-news, and there's not really any point to having the pointer passed in be negligibly initialized, so there was basically no change needed here to *actually* fix the problem.  Stop zeroing explicitly, remove the now-bad assert, and the rest is alpha-renames for clarity.

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Add member initializers to the function pointers in JS::AsmJSCacheOps so that JSRuntime::asmJSCacheOps, a js::UnprotectedData<JS::AsmJSCacheOps>, will have its members nulled out automatically when the JSRuntime field is initialized

I dislike greatly how difficult it is to determine that js::UnprotectedData<JS::AsmJSCacheOps> when not explicitly initialized in the member list will default-construct the contained type within it.  But that's where we are, if you grunge hard enough and know spec enough.

C++ initialization rules really are the worst.  Not least because you can't even assert their guaranteed behaviors, because you can't *evaluate* memory if it has undefined contents, so any assertion that "this should contain zeroes" is absolutely allowed to pass even when the memory has never had zeroes written to it.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Replace a PodZero of js::gcstats::Statistics::totalTimes_ with a loop asserting every element was default-initialized to zero

Completely unnecessary PodZero, woo!  The assertion things are a little anal-retentive, but better safe than sorry as long as it's so difficult to understand that these fields actually were zeroed out.

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Replace a PodZero of js::gcstats::Statistics::phaseTimes with a loop overwriting every element value with a default-initialized (i.e. zeroed) value

Comment 19

[Jan de Mooij [:jandem]]

Comment on attachment 8976438 [details] [diff] [review]
Don't use PodCopy/PodMove to implement typed-array element-to-element copying

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

::: js/src/vm/TypedArrayObject-inl.h
@@ +217,5 @@
>  
>      template<typename T>
>      static void podCopy(SharedMem<T*> dest, SharedMem<T*> src, size_t nelem) {
> +        // std::copy_n better matches the argument values/types of this
> +        // function,, but as noted below it allows the input/output ranges to

s/,,/,/

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8976441 [details] [diff] [review]
Rename TypeSet::clone to TypeSet::cloneIntoUninitialized to indicate that it freshly initializes the TemporaryTypeSet* provided to it.  Also removes existing code that, quite unnecessarily, partly initialized that argument

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

Nice

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ead72cce7f0e
https://hg.mozilla.org/mozilla-central/rev/19d2aace5b3c
https://hg.mozilla.org/mozilla-central/rev/d8411d78d58a
https://hg.mozilla.org/mozilla-central/rev/e016aa76775e

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8976435 [details] [diff] [review]
Add a JSScript constructor to initialize JSScript inside JSScript::Create without using PodZero

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

I just realized I forgot to review the big patch. It's nice to initialize fields explicitly, but I'm sorry too, for landing conflicting changes to this code :/

::: js/src/vm/JSScript.cpp
@@ +2644,5 @@
> +#endif
> +    // NOTE: This field must be list-initialized in this manner, with umpteen
> +    //       trailing bit-fields not provided values, so that those trailing
> +    //       bit-fields are value-initialized, i.e. zero-initialized, i.e. set
> +    //       to 0 or false as appropriate.

As discussed on IRC, we could zero initialize and then do |bitFields_.x = x| for these 4 fields.

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Add a JSScript constructor to initialize JSScript inside JSScript::Create without using PodZero (with bit-fields all zeroed plus manual overrides, and no field-reordering)

I found this command helpful in looking at this diff character-level-wise, since so much of it is just bitFields_-prefixing and occasionally _-suffixing things:

dwdiff --punctuation --color --diff-input /tmp/script-changes.diff | less -R

I looked that through pretty carefully and am confident nothing's reordered and the trivial additions are so trivial.

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 8976710 [details] [diff] [review]
Add a JSScript constructor to initialize JSScript inside JSScript::Create without using PodZero (with bit-fields all zeroed plus manual overrides, and no field-reordering)

...oh, you did actually review this.  Maybe Thunderbird hadn't polled recently enough to find this grant at the time I looked, or something.

The changes from old to this are not big -- just undoing reorderings of fields in a couple places where I tried to keep reorderings of field consistent with orders of field-sets elsewhere, plus the constructor change -- so I will go ahead with this on typical r+-with-small-enough-changes grounds.

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Don't PodZero ObjectGroup instances inside the ObjectGroup constructor -- set most fields using member-initializers in the constructor, and add initializers in the class body for the remaining two fields

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Don't PodZero ObjectGroupCompartment -- just add member initializers inside the class definition

IT IS FINISHED (if you count all the patches posted across various bugs):

[jwalden@find-waldo-now src]$ BUILD_VERBOSE_LOG=1 make -s -C dbg -j3
Elapsed: 0.00s; From dist/private: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories.
Elapsed: 0.01s; From dist/public: Kept 0 existing; Added/updated 0; Removed 0 files and 0 directories.
Elapsed: 0.11s; From dist/include: Kept 346 existing; Added/updated 0; Removed 0 files and 0 directories.
Elapsed: 0.05s; From _tests: Kept 352 existing; Added/updated 0; Removed 0 files and 0 directories.
Elapsed: 0.00s; From dist/bin: Kept 1 existing; Added/updated 0; Removed 0 files and 0 directories.
backend.mk:2161: warning: overriding recipe for target '../dist/system_wrappers/pixman.h'
backend.mk:1279: warning: ignoring old recipe for target '../dist/system_wrappers/pixman.h'
backend.mk:2161: warning: overriding recipe for target '../dist/system_wrappers/pixman.h'
backend.mk:1279: warning: ignoring old recipe for target '../dist/system_wrappers/pixman.h'
Array.o
RegExp.o
backend.mk:2161: warning: overriding recipe for target '../dist/system_wrappers/pixman.h'
backend.mk:1279: warning: ignoring old recipe for target '../dist/system_wrappers/pixman.h'
BinSource-auto.o
BinSource.o
BinToken.o
BinTokenReaderBase.o
BinTokenReaderMultipart.o
BinTokenReaderTester.o
Parser.o
StoreBuffer.o
Disassembler-x86-shared.o
jsmath.o
jsutil.o
Interpreter.o
JSAtom.o
TraceLogging.o
VTuneWrapper.o
Unified_cpp_js_src0.o
Unified_cpp_js_src1.o
Unified_cpp_js_src10.o
Unified_cpp_js_src11.o
Unified_cpp_js_src12.o
Unified_cpp_js_src13.o
Unified_cpp_js_src14.o
Unified_cpp_js_src15.o
Unified_cpp_js_src16.o
Unified_cpp_js_src17.o
Unified_cpp_js_src18.o
Unified_cpp_js_src19.o
Unified_cpp_js_src2.o
Unified_cpp_js_src20.o
Unified_cpp_js_src21.o
Unified_cpp_js_src22.o
Unified_cpp_js_src23.o
Unified_cpp_js_src24.o
Unified_cpp_js_src25.o
Unified_cpp_js_src26.o
Unified_cpp_js_src27.o
Unified_cpp_js_src28.o
Unified_cpp_js_src29.o
Unified_cpp_js_src3.o
Unified_cpp_js_src30.o
Unified_cpp_js_src31.o
Unified_cpp_js_src32.o
Unified_cpp_js_src33.o
Unified_cpp_js_src34.o
Unified_cpp_js_src35.o
Unified_cpp_js_src36.o
Unified_cpp_js_src37.o
Unified_cpp_js_src38.o
Unified_cpp_js_src39.o
Unified_cpp_js_src4.o
Unified_cpp_js_src40.o
Unified_cpp_js_src41.o
Unified_cpp_js_src42.o
Unified_cpp_js_src43.o
Unified_cpp_js_src44.o
Unified_cpp_js_src5.o
Unified_cpp_js_src6.o
Unified_cpp_js_src7.o
Unified_cpp_js_src8.o
Unified_cpp_js_src9.o
libjs_static.a
libmozjs-62a1.so
Unified_cpp_js_src_gdb0.o
Unified_cpp_js_src_shell0.o
testAssemblerBuffer.o
Unified_cpp_js_src_jsapi-tests0.o
Unified_cpp_js_src_jsapi-tests1.o
js
gdb-tests
Unified_cpp_js_src_jsapi-tests2.o
Unified_cpp_js_src_jsapi-tests3.o
Unified_cpp_js_src_jsapi-tests4.o
Unified_cpp_js_src_jsapi-tests5.o
Unified_cpp_js_src_jsapi-tests6.o
jsapi-tests
spidermonkey_checks.stub
TEST-PASS | check_spidermonkey_style.py | ok
TEST-PASS | check_macroassembler_style.py | ok
TEST-PASS | check_js_opcode.py | ok
backend.mk:2161: warning: overriding recipe for target '../dist/system_wrappers/pixman.h'
backend.mk:1279: warning: ignoring old recipe for target '../dist/system_wrappers/pixman.h'
[jwalden@find-waldo-now src]$

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 8976801 [details] [diff] [review]
Don't PodZero ObjectGroupCompartment -- just add member initializers inside the class definition

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

\o/ Thanks for doing this.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a053ded7f80
Give uint8_clamped a defaulted (and also trivial) default constructor, copy constructor, and copy-assignment operator.  (This also allows uint8_clamped to be permissibly memmove'd and memcpy'd.)  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/04a9320069d7
Don't use PodCopy/PodMove to implement typed-array element-to-element copying: bog-standard std::copy and std::copy_n are readily optimized to the same thing, and they don't have a non-obvious requirement that the type being copied be trivial.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/d642657c6d7a
Rename TypeSet::clone to TypeSet::cloneIntoUninitialized to indicate that it freshly initializes the TemporaryTypeSet* provided to it.  Also removes existing code that, quite unnecessarily, partly initialized that argument.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c09010d054e
Add member initializers to the function pointers in JS::AsmJSCacheOps so that JSRuntime::asmJSCacheOps, a js::UnprotectedData<JS::AsmJSCacheOps>, will have its members nulled out automatically when the JSRuntime field is initialized.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ceb0b9f6c80
Replace a PodZero of js::gcstats::Statistics::totalTimes_ with a loop asserting every element was default-initialized to zero.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/1df7d4219611
Replace a PodZero of js::gcstats::Statistics::phaseTimes with a loop overwriting every element value with a default-initialized (i.e. zeroed) value.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c45180cea08
Add a JSScript constructor to initialize JSScript inside JSScript::Create without using PodZero.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/441f59473bfa
Don't PodZero ObjectGroup instances inside the ObjectGroup constructor -- set most fields using member-initializers in the constructor, and add initializers in the class body for the remaining two fields.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/7658d2d1e0d7
Don't PodZero ObjectGroupCompartment -- just add member initializers inside the class definition.  r=jandem

Comment 29

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7a053ded7f80
https://hg.mozilla.org/mozilla-central/rev/04a9320069d7
https://hg.mozilla.org/mozilla-central/rev/d642657c6d7a
https://hg.mozilla.org/mozilla-central/rev/5c09010d054e
https://hg.mozilla.org/mozilla-central/rev/6ceb0b9f6c80
https://hg.mozilla.org/mozilla-central/rev/1df7d4219611
https://hg.mozilla.org/mozilla-central/rev/7c45180cea08
https://hg.mozilla.org/mozilla-central/rev/441f59473bfa
https://hg.mozilla.org/mozilla-central/rev/7658d2d1e0d7

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:Waldo, maybe it's time to close this bug?

Comment 31

[Jeff Walden [:Waldo]]

PodZero really needs to be completely removed, and there are still uses of it in the JS engine.  But we don't have any of the compiler warning here about PodZero happening on non-trivial stuff in the JS engine, so I guess this *as a JS engine* bug is done.