1461821 - Fix places in the parser where we memset/memcpy the BindingName array associated with a {Global,Module,...}Scope::Data despite BindingName being non-trivial (causing gcc compiler warnings)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Convert the trailing array of BindingNames at the end of the various kinds of scope data into raw unsigned chars into which those BindingNames are placement-new'd

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Call the relevant scope-data constructor when allocating it, and poison/mark as undefined the memory for the trailing array of BindingNames, ratther than impermissibly PodZero-ing non-trivial classes willy-nilly

Guh, this is all such a mess.  :-|

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8975986 [details] [diff] [review]
Convert the trailing array of BindingNames at the end of the various kinds of scope data into raw unsigned chars into which those BindingNames are placement-new'd

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

Hm, I'm wondering if TrailingNamesArray could at some point become a template class we could use in other places.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8975987 [details] [diff] [review]
Call the relevant scope-data constructor when allocating it, and poison/mark as undefined the memory for the trailing array of BindingNames, ratther than impermissibly PodZero-ing non-trivial classes willy-nilly

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

::: js/src/vm/Scope.h
@@ +158,5 @@
> +    // Explicitly ensure no one accidentally allocates scope data without
> +    // poisoning its trailing names.
> +    TrailingNamesArray() = delete;
> +
> +    TrailingNamesArray(size_t nameCount) {

Nit: explicit, and Data constructors below

Comment 5

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> I'm wondering if TrailingNamesArray could at some point become a
> template class we could use in other places.

I pondered that.  There might even be a case for it in further memset-POD fixing.  Given this is not particularly a favored pattern, IMO, I left it alone for this very firstest use case.  Maybe I end up having to do this later today or tomorrow, but eh.

(In reply to Jan de Mooij [:jandem] from comment #4)
> Nit: explicit, and Data constructors below

This is what happens when I start building with gcc instead of clang.  :-(

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c36389c5f7
Convert the trailing array of BindingNames at the end of the various kinds of scope data into raw unsigned chars into which those BindingNames are placement-new'd, rather than memcpy-ing non-trivial classes around and failing to comply with the C++ object model.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ddbcf79bc48
Call the relevant scope-data constructor when allocating it, and poison/mark as undefined the memory for the trailing array of BindingNames, ratther than impermissibly PodZero-ing non-trivial classes willy-nilly.  r=jandem

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/73c36389c5f7
https://hg.mozilla.org/mozilla-central/rev/2ddbcf79bc48