From b4aed63f5758b955e84840c5871b1301ccb6968f Mon Sep 17 00:00:00 2001 From: trav90 Date: Sun, 2 Sep 2018 18:45:56 -0500 Subject: 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 --- js/src/vm/Scope.cpp | 14 +++++++------- js/src/vm/Scope.h | 45 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 46 insertions(+), 13 deletions(-) (limited to 'js/src/vm') diff --git a/js/src/vm/Scope.cpp b/js/src/vm/Scope.cpp index 112b34586..25e389d8a 100644 --- a/js/src/vm/Scope.cpp +++ b/js/src/vm/Scope.cpp @@ -273,7 +273,7 @@ Scope::XDRSizedBindingNames(XDRState* xdr, Handle scope, } for (uint32_t i = 0; i < length; i++) { - if (!XDRBindingName(xdr, &data->names[i])) { + if (!XDRBindingName(xdr, &data->trailingNames[i])) { if (mode == XDR_DECODE) { DeleteScopeData(data.get()); data.set(nullptr); @@ -1250,7 +1250,7 @@ BindingIter::init(LexicalScope::Data& data, uint32_t firstFrameSlot, uint8_t fla init(0, 0, 0, 0, 0, 0, CanHaveEnvironmentSlots | flags, firstFrameSlot, JSSLOT_FREE(&LexicalEnvironmentObject::class_), - data.names, data.length); + data.trailingNames.start(), data.length); } else { // imports - [0, 0) // positional formals - [0, 0) @@ -1262,7 +1262,7 @@ BindingIter::init(LexicalScope::Data& data, uint32_t firstFrameSlot, uint8_t fla init(0, 0, 0, 0, 0, data.constStart, CanHaveFrameSlots | CanHaveEnvironmentSlots | flags, firstFrameSlot, JSSLOT_FREE(&LexicalEnvironmentObject::class_), - data.names, data.length); + data.trailingNames.start(), data.length); } } @@ -1283,7 +1283,7 @@ BindingIter::init(FunctionScope::Data& data, uint8_t flags) init(0, data.nonPositionalFormalStart, data.varStart, data.varStart, data.length, data.length, flags, 0, JSSLOT_FREE(&CallObject::class_), - data.names, data.length); + data.trailingNames.start(), data.length); } void @@ -1299,7 +1299,7 @@ BindingIter::init(VarScope::Data& data, uint32_t firstFrameSlot) init(0, 0, 0, 0, data.length, data.length, CanHaveFrameSlots | CanHaveEnvironmentSlots, firstFrameSlot, JSSLOT_FREE(&VarEnvironmentObject::class_), - data.names, data.length); + data.trailingNames.start(), data.length); } void @@ -1343,7 +1343,7 @@ BindingIter::init(EvalScope::Data& data, bool strict) // consts - [data.length, data.length) init(0, 0, 0, data.varStart, data.length, data.length, flags, firstFrameSlot, firstEnvironmentSlot, - data.names, data.length); + data.trailingNames.start(), data.length); } void @@ -1359,7 +1359,7 @@ BindingIter::init(ModuleScope::Data& data) init(data.varStart, data.varStart, data.varStart, data.varStart, data.letStart, data.constStart, CanHaveFrameSlots | CanHaveEnvironmentSlots, 0, JSSLOT_FREE(&ModuleEnvironmentObject::class_), - data.names, data.length); + data.trailingNames.start(), data.length); } PositionalFormalParameterIter::PositionalFormalParameterIter(JSScript* script) diff --git a/js/src/vm/Scope.h b/js/src/vm/Scope.h index 5304d6713..387eb30eb 100644 --- a/js/src/vm/Scope.h +++ b/js/src/vm/Scope.h @@ -111,6 +111,39 @@ class BindingName void trace(JSTracer* trc); }; +/** + * The various {Global,Module,...}Scope::Data classes consist of always-present + * bits, then a trailing array of BindingNames. The various Data classes all + * end in a TrailingNamesArray that contains sized/aligned space for *one* + * BindingName. Data instances that contain N BindingNames, are then allocated + * in sizeof(Data) + (space for (N - 1) BindingNames). Because this class's + * |data_| field is properly sized/aligned, the N-BindingName array can start + * at |data_|. + * + * This is concededly a very low-level representation, but we want to only + * allocate once for data+bindings both, and this does so approximately as + * elegantly as C++ allows. + */ +class TrailingNamesArray +{ + private: + alignas(BindingName) unsigned char data_[sizeof(BindingName)]; + + private: + // Some versions of GCC treat it as a -Wstrict-aliasing violation (ergo a + // -Werror compile error) to reinterpret_cast<> |data_| to |T*|, even + // through |void*|. Placing the latter cast in these separate functions + // breaks the chain such that affected GCC versions no longer warn/error. + void* ptr() { + return data_; + } + + public: + BindingName* start() { return reinterpret_cast(ptr()); } + + BindingName& operator[](size_t i) { return start()[i]; } +}; + class BindingLocation { public: @@ -346,7 +379,7 @@ class LexicalScope : public Scope // The array of tagged JSAtom* names, allocated beyond the end of the // struct. - BindingName names[1]; + TrailingNamesArray trailingNames; void trace(JSTracer* trc); }; @@ -462,7 +495,7 @@ class FunctionScope : public Scope // The array of tagged JSAtom* names, allocated beyond the end of the // struct. - BindingName names[1]; + TrailingNamesArray trailingNames; void trace(JSTracer* trc); }; @@ -556,7 +589,7 @@ class VarScope : public Scope // The array of tagged JSAtom* names, allocated beyond the end of the // struct. - BindingName names[1]; + TrailingNamesArray trailingNames; void trace(JSTracer* trc); }; @@ -645,7 +678,7 @@ class GlobalScope : public Scope // The array of tagged JSAtom* names, allocated beyond the end of the // struct. - BindingName names[1]; + TrailingNamesArray trailingNames; void trace(JSTracer* trc); }; @@ -745,7 +778,7 @@ class EvalScope : public Scope // The array of tagged JSAtom* names, allocated beyond the end of the // struct. - BindingName names[1]; + TrailingNamesArray trailingNames; void trace(JSTracer* trc); }; @@ -846,7 +879,7 @@ class ModuleScope : public Scope // The array of tagged JSAtom* names, allocated beyond the end of the // struct. - BindingName names[1]; + TrailingNamesArray trailingNames; void trace(JSTracer* trc); }; -- cgit v1.2.3 From 36cb80d1cac66a1511bec4ad97947a2deeab2e08 Mon Sep 17 00:00:00 2001 From: trav90 Date: Sun, 2 Sep 2018 18:59:51 -0500 Subject: 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. --- js/src/vm/Scope.cpp | 4 +-- js/src/vm/Scope.h | 75 ++++++++++++++++++++++++++++++++++++----------------- 2 files changed, 53 insertions(+), 26 deletions(-) (limited to 'js/src/vm') diff --git a/js/src/vm/Scope.cpp b/js/src/vm/Scope.cpp index 25e389d8a..8bb1702ea 100644 --- a/js/src/vm/Scope.cpp +++ b/js/src/vm/Scope.cpp @@ -191,12 +191,12 @@ template static UniquePtr NewEmptyScopeData(ExclusiveContext* cx, uint32_t length = 0) { - uint8_t* bytes = cx->zone()->pod_calloc(ConcreteScope::sizeOfData(length)); + uint8_t* bytes = cx->zone()->pod_malloc(ConcreteScope::sizeOfData(length)); if (!bytes) ReportOutOfMemory(cx); auto data = reinterpret_cast(bytes); if (data) - new (data) typename ConcreteScope::Data(); + new (data) typename ConcreteScope::Data(length); return UniquePtr(data); } diff --git a/js/src/vm/Scope.h b/js/src/vm/Scope.h index 387eb30eb..1d04fd9f6 100644 --- a/js/src/vm/Scope.h +++ b/js/src/vm/Scope.h @@ -12,6 +12,7 @@ #include "jsobj.h" #include "jsopcode.h" +#include "jsutil.h" #include "gc/Heap.h" #include "gc/Policy.h" @@ -139,6 +140,14 @@ class TrailingNamesArray } public: + // Explicitly ensure no one accidentally allocates scope data without + // poisoning its trailing names. + TrailingNamesArray() = delete; + + explicit TrailingNamesArray(size_t nameCount) { + if (nameCount) + JS_POISON(&data_, 0xCC, sizeof(BindingName) * nameCount); + } BindingName* start() { return reinterpret_cast(ptr()); } BindingName& operator[](size_t i) { return start()[i]; } @@ -370,17 +379,20 @@ class LexicalScope : public Scope // // lets - [0, constStart) // consts - [constStart, length) - uint32_t constStart; - uint32_t length; + uint32_t constStart = 0; + uint32_t length = 0; // Frame slots [0, nextFrameSlot) are live when this is the innermost // scope. - uint32_t nextFrameSlot; + uint32_t nextFrameSlot = 0; // The array of tagged JSAtom* names, allocated beyond the end of the // struct. TrailingNamesArray trailingNames; + explicit Data(size_t nameCount) : trailingNames(nameCount) {} + Data() = delete; + void trace(JSTracer* trc); }; @@ -466,11 +478,11 @@ class FunctionScope : public Scope // The canonical function of the scope, as during a scope walk we // often query properties of the JSFunction (e.g., is the function an // arrow). - GCPtrFunction canonicalFunction; + GCPtrFunction canonicalFunction = {}; // If parameter expressions are present, parameters act like lexical // bindings. - bool hasParameterExprs; + bool hasParameterExprs = false; // Bindings are sorted by kind in both frames and environments. // @@ -485,18 +497,21 @@ class FunctionScope : public Scope // positional formals - [0, nonPositionalFormalStart) // other formals - [nonPositionalParamStart, varStart) // vars - [varStart, length) - uint16_t nonPositionalFormalStart; - uint16_t varStart; - uint32_t length; + uint16_t nonPositionalFormalStart = 0; + uint16_t varStart = 0; + uint32_t length = 0; // Frame slots [0, nextFrameSlot) are live when this is the innermost // scope. - uint32_t nextFrameSlot; + uint32_t nextFrameSlot = 0; // The array of tagged JSAtom* names, allocated beyond the end of the // struct. TrailingNamesArray trailingNames; + explicit Data(size_t nameCount) : trailingNames(nameCount) {} + Data() = delete; + void trace(JSTracer* trc); }; @@ -581,16 +596,19 @@ class VarScope : public Scope struct Data { // All bindings are vars. - uint32_t length; + uint32_t length = 0; // Frame slots [firstFrameSlot(), nextFrameSlot) are live when this is // the innermost scope. - uint32_t nextFrameSlot; + uint32_t nextFrameSlot = 0; // The array of tagged JSAtom* names, allocated beyond the end of the // struct. TrailingNamesArray trailingNames; + explicit Data(size_t nameCount) : trailingNames(nameCount) {} + Data() = delete; + void trace(JSTracer* trc); }; @@ -671,15 +689,18 @@ class GlobalScope : public Scope // vars - [varStart, letStart) // lets - [letStart, constStart) // consts - [constStart, length) - uint32_t varStart; - uint32_t letStart; - uint32_t constStart; - uint32_t length; + uint32_t varStart = 0; + uint32_t letStart = 0; + uint32_t constStart = 0; + uint32_t length = 0; // The array of tagged JSAtom* names, allocated beyond the end of the // struct. TrailingNamesArray trailingNames; + explicit Data(size_t nameCount) : trailingNames(nameCount) {} + Data() = delete; + void trace(JSTracer* trc); }; @@ -769,17 +790,20 @@ class EvalScope : public Scope // // top-level funcs - [0, varStart) // vars - [varStart, length) - uint32_t varStart; - uint32_t length; + uint32_t varStart = 0; + uint32_t length = 0; // Frame slots [0, nextFrameSlot) are live when this is the innermost // scope. - uint32_t nextFrameSlot; + uint32_t nextFrameSlot = 0; // The array of tagged JSAtom* names, allocated beyond the end of the // struct. TrailingNamesArray trailingNames; + explicit Data(size_t nameCount) : trailingNames(nameCount) {} + Data() = delete; + void trace(JSTracer* trc); }; @@ -860,7 +884,7 @@ class ModuleScope : public Scope struct Data { // The module of the scope. - GCPtr module; + GCPtr module = {}; // Bindings are sorted by kind. // @@ -868,19 +892,22 @@ class ModuleScope : public Scope // vars - [varStart, letStart) // lets - [letStart, constStart) // consts - [constStart, length) - uint32_t varStart; - uint32_t letStart; - uint32_t constStart; - uint32_t length; + uint32_t varStart = 0; + uint32_t letStart = 0; + uint32_t constStart = 0; + uint32_t length = 0; // Frame slots [0, nextFrameSlot) are live when this is the innermost // scope. - uint32_t nextFrameSlot; + uint32_t nextFrameSlot = 0; // The array of tagged JSAtom* names, allocated beyond the end of the // struct. TrailingNamesArray trailingNames; + explicit Data(size_t nameCount) : trailingNames(nameCount) {} + Data() = delete; + void trace(JSTracer* trc); }; -- cgit v1.2.3 From 47c5bba17a1049ca69e01981aebe858fb50c8d69 Mon Sep 17 00:00:00 2001 From: trav90 Date: Sun, 2 Sep 2018 21:01:45 -0500 Subject: Fix build bustage --- js/src/vm/Scope.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'js/src/vm') diff --git a/js/src/vm/Scope.cpp b/js/src/vm/Scope.cpp index 8bb1702ea..a71c03695 100644 --- a/js/src/vm/Scope.cpp +++ b/js/src/vm/Scope.cpp @@ -1315,7 +1315,7 @@ BindingIter::init(GlobalScope::Data& data) init(0, 0, 0, data.varStart, data.letStart, data.constStart, CannotHaveSlots, UINT32_MAX, UINT32_MAX, - data.names, data.length); + data.trailingNames.start(), data.length); } void -- cgit v1.2.3