Closed Bug 1461821 Opened 2 years ago Closed 2 years ago

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)

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(2 files)

No description provided.
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.
Attachment #8975986 - Flags: review?(jdemooij) → review+
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
Attachment #8975987 - Flags: review?(jdemooij) → review+
(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.  :-(
Pushed by jwalden@mit.edu:
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
https://hg.mozilla.org/mozilla-central/rev/73c36389c5f7
https://hg.mozilla.org/mozilla-central/rev/2ddbcf79bc48
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1494752
You need to log in before you can comment on or make changes to this bug.