Bug 1861973 - Signedness confusion in js::wasm::PackedTypeCode::pack.…

… r=rhunt.

`PackedTypeCode::pack` stores a pointer in an unsigned 48-bit field,
PackedRepr typeDef_ : TypeDefBits, where TypeDefbits == 48.

This is problematic, although in release builds OK, on both 32- and 64-bit
targets:

* on an arm32 bit target (native, not simulator), the assertion
  `MOZ_ASSERT((uint64_t)typeDef <= ((uint64_t)1 << TypeDefBits) - 1)` has been
  observed to fail. The assertion assumes that `(uint64_t)typeDef` unsignedly
  widens typeDef to 64 bits; but that is implementation dependent and on this
  target it is signedly widened, which causes the assertion to fail if bit 31
  of `typeDef` is 1.

* on 64 bit targets, TypeDef::typeDef reconstitutes the pointer by unsignedly
  widening the 48 bit field back out to 64 bits. That works OK because, at
  least for "canonical addresses" on x86_64, only kernel addresses exist in
  the high-half canonical space. So we'll never encounter them. But it's
  conceptually confusing because existing literature, and our own practices
  (eg ds/PointerAndUint7) regard 64-bit canonical addresses as split equally
  high and low -- that is, as signed.

This patch:

* on 32 bit targets, removes the failing assertion (it is pointless)

* on 64 bit targets, fixes up the assertion and also `PackedTypeCode::typeDef`
  to treat the stored value as a sighed 48 bit entity.  Despite the presence
  of more code, this routine becomes cheaper on x86_64: previously it required
  3 insns and 2 regs.  Now it requires 3 insns and 1 reg.

As a passing observation, if `PackedTypeCode::typeCodeAbstracted` is really as
hot as its comment claims, it would be better -- at least on x86_64 - to place
`typeCode_` either at the start or end of the union.  Extracting bits from the
middle of a 64-bit word requires 2 insns, a shift and a mask; but if the field
is at the top or bottom it would require respectively only a shift (top) or a
mask (bottom).

Differential Revision: https://phabricator.services.mozilla.com/D192354

UltraBlame original commit: 6f3be95d65116af346971c9a559f77dbb8e9e14a

js/src/wasm/WasmValType.h

@@ -577,47 +577,134 @@ typeDef
 nullptr
 )
 ;
+#
+if
+defined
+(
+JS_64BIT
+)
+&
+&
+defined
+(
+DEBUG
+)
 /
 /
 Double
 check
 that
+typeDef
+points
+inside
 the
-type
-definition
-was
-allocated
-within
+"
+canonical
+"
 48
 -
+bit
+/
+/
+address
+space
+by
+checking
+that
+the
+lower
+48
 bits
-as
+sign
+extend
+back
+to
+/
+/
+the
+original
+.
+This
+is
+necessary
+since
+we
+will
+only
+store
+the
+lowest
+48
 /
 /
+bits
+of
+it
+as
 noted
 above
 .
-MOZ_ASSERT
-(
-(
-uint64_t
-)
-typeDef
-<
+There
+'
+s
+no
+equivalent
+check
+on
+32
+bit
+/
+/
+targets
+since
+we
+can
+store
+the
+whole
+pointer
+.
+int64_t
+w
 =
+int64_t
 (
-(
-uint64_t
+typeDef
 )
-1
+;
+w
 <
 <
+=
+(
+64
+-
 TypeDefBits
 )
+;
+w
+>
+>
+=
+(
+64
 -
-1
+TypeDefBits
 )
 ;
+MOZ_ASSERT
+(
+w
+=
+=
+int64_t
+(
+typeDef
+)
+)
+;
+#
+endif
 PackedTypeCode
 ptc
 =
@@ -961,6 +1048,96 @@ isValid
 )
 )
 ;
+#
+if
+defined
+(
+JS_64BIT
+)
+/
+/
+Reconstitute
+the
+pointer
+by
+sign
+-
+extending
+the
+lowest
+TypeDefBits
+bits
+.
+static_assert
+(
+sizeof
+(
+int64_t
+)
+=
+=
+sizeof
+(
+uintptr_t
+)
+)
+;
+int64_t
+w
+=
+int64_t
+(
+typeDef_
+)
+;
+w
+<
+<
+=
+(
+64
+-
+TypeDefBits
+)
+;
+w
+>
+>
+=
+(
+64
+-
+TypeDefBits
+)
+;
+return
+(
+const
+TypeDef
+*
+)
+(
+uintptr_t
+)
+w
+;
+#
+else
+/
+/
+The
+pointer
+is
+stored
+exactly
+in
+the
+lowest
+32
+bits
+of
+typeDef_
+.
 return
 (
 const
@@ -972,6 +1149,8 @@ uintptr_t
 )
 typeDef_
 ;
+#
+endif
 }
 bool
 isNullable