Bug 1549340 - Part 2: Use union instead of reinterpret_cast to initialize const char* with symbol for JSPropertySpec.name. r=jandem

Differential Revision: https://phabricator.services.mozilla.com/D30493 --HG-- extra : moz-landing-system : lando
2019-05-10 14:08:07 +00:00 · 2019-05-10 14:08:07 +00:00 · 9fa8a62e9e
--- a/dom/bindings/BindingUtils.cpp
+++ b/dom/bindings/BindingUtils.cpp
@ -1143,6 +1143,17 @@ static int CompareIdsAtIndices(const void* aElement1, const void* aElement2,
  return JSID_BITS(infos[index1].Id()) < JSID_BITS(infos[index2].Id()) ? -1 : 1;
 }

+// {JSPropertySpec,JSFunctionSpec} use {JSPropertySpec,JSFunctionSpec}::Name
+// and ConstantSpec uses `const char*` for name field.
+static inline JSPropertySpec::Name ToPropertySpecName(
+    JSPropertySpec::Name name) {
+  return name;
+}
+
+static inline JSPropertySpec::Name ToPropertySpecName(const char* name) {
+  return JSPropertySpec::Name(name);
+}
+
 template <typename SpecT>
 static bool InitIdsInternal(JSContext* cx, const Prefable<SpecT>* pref,
                            PropertyInfo* infos, PropertyType type) {
@ -1161,7 +1172,8 @@ static bool InitIdsInternal(JSContext* cx, const Prefable<SpecT>* pref,
    uint32_t specIndex = 0;
    do {
      jsid id;
-      if (!JS::PropertySpecNameToPermanentId(cx, spec->name, &id)) {
+      if (!JS::PropertySpecNameToPermanentId(cx, ToPropertySpecName(spec->name),
+                                             &id)) {
        return false;
      }
      infos->SetId(id);
--- a/js/public/PropertySpec.h
+++ b/js/public/PropertySpec.h
@ -19,7 +19,7 @@
 #include "js/CallArgs.h"            // JSNative
 #include "js/PropertyDescriptor.h"  // JSPROP_*
 #include "js/RootingAPI.h"          // JS::MutableHandle
-#include "js/Symbol.h"              // JS::SymbolCode
+#include "js/Symbol.h"              // JS::SymbolCode, PropertySpecNameIsSymbol
 #include "js/Value.h"               // JS::Value

 struct JSContext;
@ -148,7 +148,34 @@ struct JSPropertySpec {
    }
  };

-  const char* name;
+  union Name {
+   private:
+    const char* string_;
+    uintptr_t symbol_;
+
+   public:
+    Name() = delete;
+
+    explicit constexpr Name(const char* str) : string_(str) {}
+    explicit constexpr Name(JS::SymbolCode symbol)
+        : symbol_(uint32_t(symbol) + 1) {}
+
+    explicit operator bool() const { return !!symbol_; }
+
+    bool isSymbol() const { return JS::PropertySpecNameIsSymbol(symbol_); }
+    JS::SymbolCode symbol() const {
+      MOZ_ASSERT(isSymbol());
+      return JS::SymbolCode(symbol_ - 1);
+    }
+
+    bool isString() const { return !isSymbol(); }
+    const char* string() const {
+      MOZ_ASSERT(isString());
+      return string_;
+    }
+  };
+
+  Name name;
  uint8_t flags;
  AccessorsOrValue u;

@ -157,6 +184,9 @@ struct JSPropertySpec {

  constexpr JSPropertySpec(const char* name, uint8_t flags, AccessorsOrValue u)
      : name(name), flags(flags), u(u) {}
+  constexpr JSPropertySpec(JS::SymbolCode name, uint8_t flags,
+                           AccessorsOrValue u)
+      : name(name), flags(flags), u(u) {}

 public:
  JSPropertySpec(const JSPropertySpec& other) = default;
@ -172,6 +202,17 @@ struct JSPropertySpec {
            JSPropertySpec::Accessor::nativeAccessor(setter, setterInfo)));
  }

+  static constexpr JSPropertySpec nativeAccessors(
+      JS::SymbolCode name, uint8_t flags, JSNative getter,
+      const JSJitInfo* getterInfo, JSNative setter = nullptr,
+      const JSJitInfo* setterInfo = nullptr) {
+    return JSPropertySpec(
+        name, flags,
+        AccessorsOrValue::fromAccessors(
+            JSPropertySpec::Accessor::nativeAccessor(getter, getterInfo),
+            JSPropertySpec::Accessor::nativeAccessor(setter, setterInfo)));
+  }
+
  static constexpr JSPropertySpec selfHostedAccessors(
      const char* name, uint8_t flags, const char* getterName,
      const char* setterName = nullptr) {
@ -184,6 +225,18 @@ struct JSPropertySpec {
                : JSPropertySpec::Accessor::noAccessor()));
  }

+  static constexpr JSPropertySpec selfHostedAccessors(
+      JS::SymbolCode name, uint8_t flags, const char* getterName,
+      const char* setterName = nullptr) {
+    return JSPropertySpec(
+        name, flags | JSPROP_GETTER | (setterName ? JSPROP_SETTER : 0),
+        AccessorsOrValue::fromAccessors(
+            JSPropertySpec::Accessor::selfHostedAccessor(getterName),
+            setterName
+                ? JSPropertySpec::Accessor::selfHostedAccessor(setterName)
+                : JSPropertySpec::Accessor::noAccessor()));
+  }
+
  static constexpr JSPropertySpec int32Value(const char* name, uint8_t flags,
                                             int32_t n) {
    return JSPropertySpec(name, flags | JSPROP_INTERNAL_USE_BIT,
@ -191,6 +244,13 @@ struct JSPropertySpec {
                              JSPropertySpec::ValueWrapper::int32Value(n)));
  }

+  static constexpr JSPropertySpec int32Value(JS::SymbolCode name, uint8_t flags,
+                                             int32_t n) {
+    return JSPropertySpec(name, flags | JSPROP_INTERNAL_USE_BIT,
+                          AccessorsOrValue::fromValue(
+                              JSPropertySpec::ValueWrapper::int32Value(n)));
+  }
+
  static constexpr JSPropertySpec stringValue(const char* name, uint8_t flags,
                                              const char* s) {
    return JSPropertySpec(name, flags | JSPROP_INTERNAL_USE_BIT,
@ -198,6 +258,13 @@ struct JSPropertySpec {
                              JSPropertySpec::ValueWrapper::stringValue(s)));
  }

+  static constexpr JSPropertySpec stringValue(JS::SymbolCode name,
+                                              uint8_t flags, const char* s) {
+    return JSPropertySpec(name, flags | JSPROP_INTERNAL_USE_BIT,
+                          AccessorsOrValue::fromValue(
+                              JSPropertySpec::ValueWrapper::stringValue(s)));
+  }
+
  static constexpr JSPropertySpec sentinel() {
    return JSPropertySpec(nullptr, 0,
                          AccessorsOrValue::fromAccessors(
@ -249,20 +316,6 @@ struct JSPropertySpec {
  }
 };

-// JSPropertySpec::{nativeAccessors, selfHostedAccessors,int32Value,
-// stringValue} methods require symbol names to be casted to `const char*`,
-// and the cast is `reinterpret_cast`.
-//
-// Provide a macro for the cast because of the following reasons:
-//
-//   * `reinterpret_cast` cannot be used in constexpr
-//   * using non-constexpr static method in parameter disables constexpr of
-//     above methods
-//   * top-level `reinterpret_cast` doesn't disable constexpr of above methods
-//
-#define SYMBOL_TO_PROPERTY_NAME(symbol) \
-  reinterpret_cast<const char*>(uint32_t(symbol) + 1)
-
 #define JS_CHECK_ACCESSOR_FLAGS(flags)                                         \
  (static_cast<std::enable_if<((flags) & ~(JSPROP_ENUMERATE |                  \
                                           JSPROP_PERMANENT)) == 0>::type>(0), \
@ -274,10 +327,10 @@ struct JSPropertySpec {
 #define JS_PSGS(name, getter, setter, flags)                            \
  JSPropertySpec::nativeAccessors(name, JS_CHECK_ACCESSOR_FLAGS(flags), \
                                  getter, nullptr, setter, nullptr)
-#define JS_SYM_GET(symbol, getter, flags)                \
-  JSPropertySpec::nativeAccessors(                       \
-      SYMBOL_TO_PROPERTY_NAME(::JS::SymbolCode::symbol), \
-      JS_CHECK_ACCESSOR_FLAGS(flags), getter, nullptr)
+#define JS_SYM_GET(symbol, getter, flags)                                 \
+  JSPropertySpec::nativeAccessors(::JS::SymbolCode::symbol,               \
+                                  JS_CHECK_ACCESSOR_FLAGS(flags), getter, \
+                                  nullptr)
 #define JS_SELF_HOSTED_GET(name, getterName, flags)                         \
  JSPropertySpec::selfHostedAccessors(name, JS_CHECK_ACCESSOR_FLAGS(flags), \
                                      getterName)
@ -286,13 +339,11 @@ struct JSPropertySpec {
                                      getterName, setterName)
 #define JS_SELF_HOSTED_SYM_GET(symbol, getterName, flags) \
  JSPropertySpec::selfHostedAccessors(                    \
-      SYMBOL_TO_PROPERTY_NAME(::JS::SymbolCode::symbol),  \
-      JS_CHECK_ACCESSOR_FLAGS(flags), getterName)
+      ::JS::SymbolCode::symbol, JS_CHECK_ACCESSOR_FLAGS(flags), getterName)
 #define JS_STRING_PS(name, string, flags) \
  JSPropertySpec::stringValue(name, flags, string)
 #define JS_STRING_SYM_PS(symbol, string, flags) \
-  JSPropertySpec::stringValue(                  \
-      SYMBOL_TO_PROPERTY_NAME(::JS::SymbolCode::symbol), flags, string)
+  JSPropertySpec::stringValue(::JS::SymbolCode::symbol, flags, string)
 #define JS_INT32_PS(name, value, flags) \
  JSPropertySpec::int32Value(name, flags, value)
 #define JS_PS_END JSPropertySpec::sentinel()
@ -303,7 +354,9 @@ struct JSPropertySpec {
 * compiled during JSRuntime::initSelfHosting.
 */
 struct JSFunctionSpec {
-  const char* name;
+  using Name = JSPropertySpec::Name;
+
+  Name name;
  JSNativeWrapper call;
  uint16_t nargs;
  uint16_t flags;
@ -339,11 +392,9 @@ struct JSFunctionSpec {
  JS_FNSPEC(name, nullptr, nullptr, nargs, flags, selfHostedName)
 #define JS_SELF_HOSTED_SYM_FN(symbol, selfHostedName, nargs, flags) \
  JS_SYM_FNSPEC(symbol, nullptr, nullptr, nargs, flags, selfHostedName)
-#define JS_SYM_FNSPEC(symbol, call, info, nargs, flags, selfHostedName)      \
-  JS_FNSPEC(                                                                 \
-      reinterpret_cast<const char*>(uint32_t(::JS::SymbolCode::symbol) + 1), \
-      call, info, nargs, flags, selfHostedName)
+#define JS_SYM_FNSPEC(symbol, call, info, nargs, flags, selfHostedName) \
+  JS_FNSPEC(::JS::SymbolCode::symbol, call, info, nargs, flags, selfHostedName)
 #define JS_FNSPEC(name, call, info, nargs, flags, selfHostedName) \
-  { name, {call, info}, nargs, flags, selfHostedName }
+  { JSFunctionSpec::Name(name), {call, info}, nargs, flags, selfHostedName }

 #endif  // js_PropertySpec_h
--- a/js/public/Symbol.h
+++ b/js/public/Symbol.h
@ -103,9 +103,8 @@ extern JS_PUBLIC_API Symbol* GetWellKnownSymbol(JSContext* cx,
 * Return true if the given JSPropertySpec::name or JSFunctionSpec::name value
 * is actually a symbol code and not a string. See JS_SYM_FN.
 */
-inline bool PropertySpecNameIsSymbol(const char* name) {
-  uintptr_t u = reinterpret_cast<uintptr_t>(name);
-  return u != 0 && u - 1 < WellKnownSymbolLimit;
+inline bool PropertySpecNameIsSymbol(uintptr_t name) {
+  return name != 0 && name - 1 < WellKnownSymbolLimit;
 }

 }  // namespace JS
--- a/js/src/builtin/TypedObject.cpp
+++ b/js/src/builtin/TypedObject.cpp
@ -338,8 +338,12 @@ const Class js::ReferenceTypeDescr::class_ = {

 const JSFunctionSpec js::ReferenceTypeDescr::typeObjectMethods[] = {
    JS_SELF_HOSTED_FN("toSource", "DescrToSource", 0, 0),
-    {"array", {nullptr, nullptr}, 1, 0, "ArrayShorthand"},
-    {"equivalent", {nullptr, nullptr}, 1, 0, "TypeDescrEquivalent"},
+    {JSFunctionSpec::Name("array"), {nullptr, nullptr}, 1, 0, "ArrayShorthand"},
+    {JSFunctionSpec::Name("equivalent"),
+     {nullptr, nullptr},
+     1,
+     0,
+     "TypeDescrEquivalent"},
    JS_FS_END};

 static const uint32_t ReferenceSizes[] = {
@ -477,9 +481,13 @@ const Class ArrayTypeDescr::class_ = {
 const JSPropertySpec ArrayMetaTypeDescr::typeObjectProperties[] = {JS_PS_END};

 const JSFunctionSpec ArrayMetaTypeDescr::typeObjectMethods[] = {
-    {"array", {nullptr, nullptr}, 1, 0, "ArrayShorthand"},
+    {JSFunctionSpec::Name("array"), {nullptr, nullptr}, 1, 0, "ArrayShorthand"},
    JS_SELF_HOSTED_FN("toSource", "DescrToSource", 0, 0),
-    {"equivalent", {nullptr, nullptr}, 1, 0, "TypeDescrEquivalent"},
+    {JSFunctionSpec::Name("equivalent"),
+     {nullptr, nullptr},
+     1,
+     0,
+     "TypeDescrEquivalent"},
    JS_SELF_HOSTED_FN("build", "TypedObjectArrayTypeBuild", 3, 0),
    JS_SELF_HOSTED_FN("from", "TypedObjectArrayTypeFrom", 3, 0),
    JS_FS_END};
@ -487,8 +495,12 @@ const JSFunctionSpec ArrayMetaTypeDescr::typeObjectMethods[] = {
 const JSPropertySpec ArrayMetaTypeDescr::typedObjectProperties[] = {JS_PS_END};

 const JSFunctionSpec ArrayMetaTypeDescr::typedObjectMethods[] = {
-    {"forEach", {nullptr, nullptr}, 1, 0, "ArrayForEach"},
-    {"redimension", {nullptr, nullptr}, 1, 0, "TypedObjectArrayRedimension"},
+    {JSFunctionSpec::Name("forEach"), {nullptr, nullptr}, 1, 0, "ArrayForEach"},
+    {JSFunctionSpec::Name("redimension"),
+     {nullptr, nullptr},
+     1,
+     0,
+     "TypedObjectArrayRedimension"},
    JS_SELF_HOSTED_FN("map", "TypedObjectArrayMap", 2, 0),
    JS_SELF_HOSTED_FN("reduce", "TypedObjectArrayReduce", 2, 0),
    JS_SELF_HOSTED_FN("filter", "TypedObjectArrayFilter", 1, 0),
@ -718,9 +730,13 @@ const Class StructTypeDescr::class_ = {
 const JSPropertySpec StructMetaTypeDescr::typeObjectProperties[] = {JS_PS_END};

 const JSFunctionSpec StructMetaTypeDescr::typeObjectMethods[] = {
-    {"array", {nullptr, nullptr}, 1, 0, "ArrayShorthand"},
+    {JSFunctionSpec::Name("array"), {nullptr, nullptr}, 1, 0, "ArrayShorthand"},
    JS_SELF_HOSTED_FN("toSource", "DescrToSource", 0, 0),
-    {"equivalent", {nullptr, nullptr}, 1, 0, "TypeDescrEquivalent"},
+    {JSFunctionSpec::Name("equivalent"),
+     {nullptr, nullptr},
+     1,
+     0,
+     "TypeDescrEquivalent"},
    JS_FS_END};

 const JSPropertySpec StructMetaTypeDescr::typedObjectProperties[] = {JS_PS_END};
--- a/js/src/ctypes/CTypes.cpp
+++ b/js/src/ctypes/CTypes.cpp
@ -1921,7 +1921,7 @@ static bool InitTypeConstructor(
    const JSFunctionSpec* instanceFns, const JSPropertySpec* instanceProps,
    MutableHandleObject typeProto, MutableHandleObject dataProto) {
  JSFunction* fun = js::DefineFunctionWithReserved(
-      cx, parent, spec.name, spec.call.op, spec.nargs, spec.flags);
+      cx, parent, spec.name.string(), spec.call.op, spec.nargs, spec.flags);
  if (!fun) {
    return false;
  }
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@ -2922,19 +2922,13 @@ JS_PUBLIC_API bool JSPropertySpec::getValue(JSContext* cx,
  return true;
 }

-static JS::SymbolCode PropertySpecNameToSymbolCode(const char* name) {
-  MOZ_ASSERT(JS::PropertySpecNameIsSymbol(name));
-  uintptr_t u = reinterpret_cast<uintptr_t>(name);
-  return JS::SymbolCode(u - 1);
-}
-
-bool PropertySpecNameToId(JSContext* cx, const char* name, MutableHandleId id,
+bool PropertySpecNameToId(JSContext* cx, JSPropertySpec::Name name,
+                          MutableHandleId id,
                          js::PinningBehavior pin = js::DoNotPinAtom) {
-  if (JS::PropertySpecNameIsSymbol(name)) {
-    JS::SymbolCode which = PropertySpecNameToSymbolCode(name);
-    id.set(SYMBOL_TO_JSID(cx->wellKnownSymbols().get(which)));
+  if (name.isSymbol()) {
+    id.set(SYMBOL_TO_JSID(cx->wellKnownSymbols().get(name.symbol())));
  } else {
-    JSAtom* atom = Atomize(cx, name, strlen(name), pin);
+    JSAtom* atom = Atomize(cx, name.string(), strlen(name.string()), pin);
    if (!atom) {
      return false;
    }
@ -2944,7 +2938,7 @@ bool PropertySpecNameToId(JSContext* cx, const char* name, MutableHandleId id,
 }

 JS_PUBLIC_API bool JS::PropertySpecNameToPermanentId(JSContext* cx,
-                                                     const char* name,
+                                                     JSPropertySpec::Name name,
                                                     jsid* idp) {
  // We are calling fromMarkedLocation(idp) even though idp points to a
  // location that will never be marked. This is OK because the whole point
@ -4667,10 +4661,11 @@ JS_PUBLIC_API JS::Symbol* JS::GetWellKnownSymbol(JSContext* cx,
 }

 #ifdef DEBUG
-static bool PropertySpecNameIsDigits(const char* s) {
-  if (JS::PropertySpecNameIsSymbol(s)) {
+static bool PropertySpecNameIsDigits(JSPropertySpec::Name name) {
+  if (name.isSymbol()) {
    return false;
  }
+  const char* s = name.string();
  if (!*s) {
    return false;
  }
@ -4683,18 +4678,19 @@ static bool PropertySpecNameIsDigits(const char* s) {
 }
 #endif  // DEBUG

-JS_PUBLIC_API bool JS::PropertySpecNameEqualsId(const char* name, HandleId id) {
-  if (JS::PropertySpecNameIsSymbol(name)) {
+JS_PUBLIC_API bool JS::PropertySpecNameEqualsId(JSPropertySpec::Name name,
+                                                HandleId id) {
+  if (name.isSymbol()) {
    if (!JSID_IS_SYMBOL(id)) {
      return false;
    }
    Symbol* sym = JSID_TO_SYMBOL(id);
-    return sym->isWellKnownSymbol() &&
-           sym->code() == PropertySpecNameToSymbolCode(name);
+    return sym->isWellKnownSymbol() && sym->code() == name.symbol();
  }

  MOZ_ASSERT(!PropertySpecNameIsDigits(name));
-  return JSID_IS_ATOM(id) && JS_FlatStringEqualsAscii(JSID_TO_ATOM(id), name);
+  return JSID_IS_ATOM(id) &&
+         JS_FlatStringEqualsAscii(JSID_TO_ATOM(id), name.string());
 }

 JS_PUBLIC_API bool JS_Stringify(JSContext* cx, MutableHandleValue vp,
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@ -37,6 +37,7 @@
 #include "js/OffThreadScriptCompilation.h"
 #include "js/Principals.h"
 #include "js/PropertyDescriptor.h"
+#include "js/PropertySpec.h"
 #include "js/Realm.h"
 #include "js/RealmOptions.h"
 #include "js/RefCounted.h"
@ -2547,7 +2548,8 @@ MOZ_MUST_USE JS_PUBLIC_API bool JS_EncodeStringToBuffer(JSContext* cx,

 namespace JS {

-JS_PUBLIC_API bool PropertySpecNameEqualsId(const char* name, HandleId id);
+JS_PUBLIC_API bool PropertySpecNameEqualsId(JSPropertySpec::Name name,
+                                            HandleId id);

 /**
 * Create a jsid that does not need to be marked for GC.
@ -2558,7 +2560,8 @@ JS_PUBLIC_API bool PropertySpecNameEqualsId(const char* name, HandleId id);
 * during GC marking.
 */
 JS_PUBLIC_API bool PropertySpecNameToPermanentId(JSContext* cx,
-                                                 const char* name, jsid* idp);
+                                                 JSPropertySpec::Name name,
+                                                 jsid* idp);

 } /* namespace JS */

--- a/js/src/vm/JSObject.cpp
+++ b/js/src/vm/JSObject.cpp
@ -38,7 +38,7 @@
 #include "js/CharacterEncoding.h"
 #include "js/MemoryMetrics.h"
 #include "js/PropertyDescriptor.h"  // JS::FromPropertyDescriptor
-#include "js/PropertySpec.h"
+#include "js/PropertySpec.h"        // JSPropertySpec
 #include "js/Proxy.h"
 #include "js/UbiNode.h"
 #include "js/UniquePtr.h"
@ -3014,7 +3014,7 @@ bool js::GetPropertyDescriptor(JSContext* cx, HandleObject obj, HandleId id,

 /* * */

-extern bool PropertySpecNameToId(JSContext* cx, const char* name,
+extern bool PropertySpecNameToId(JSContext* cx, JSPropertySpec::Name name,
                                 MutableHandleId id,
                                 js::PinningBehavior pin = js::DoNotPinAtom);