Make bitfields only as wide as necessary for enums

nico · V8 LUCI CQ · commit d15d49b09dc7 · 2022-08-01T15:09:04.000Z
clang now complains when a BitField for an enum is too wide. We could suppress this, but it seems kind of useful from an uninformed distance, so I made a few bitfields smaller instead. (For AddressingMode, since its size is target-dependent, I added an explicit underlying type to the enum instead, which suppresses the diag on a per-enum basis.) This is without any understanding of the code I'm touching. Especially the change in v8-internal.h feels a bit risky to me. Bug: chromium:1348574 Change-Id: I73395de593045036b72dadf4e3147b5f7e13c958 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3794708 Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Leszek Swirski <leszeks@chromium.org> Reviewed-by: Hannes Payer <hpayer@chromium.org> Auto-Submit: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/main@{#82109}
diff --git a/include/v8-internal.h b/include/v8-internal.h
@@ -574,7 +574,7 @@ class Internals {
 
   static const int kNodeClassIdOffset = 1 * kApiSystemPointerSize;
   static const int kNodeFlagsOffset = 1 * kApiSystemPointerSize + 3;
-  static const int kNodeStateMask = 0x7;
+  static const int kNodeStateMask = 0x3;
   static const int kNodeStateIsWeakValue = 2;
 
   static const int kFirstNonstringType = 0x80;
diff --git a/src/ast/ast.h b/src/ast/ast.h
@@ -1006,7 +1006,7 @@ class Literal final : public Expression {
   friend class AstNodeFactory;
   friend Zone;
 
-  using TypeField = Expression::NextBitField<Type, 4>;
+  using TypeField = Expression::NextBitField<Type, 3>;
 
   Literal(int smi, int position) : Expression(position, kLiteral), smi_(smi) {
     bit_field_ = TypeField::update(bit_field_, kSmi);
diff --git a/src/base/bit-field.h b/src/base/bit-field.h
@@ -40,6 +40,11 @@ class BitField final {
   static constexpr U kNumValues = U{1} << kSize;
 
   // Value for the field with all bits set.
+  // If clang complains
+  // "constexpr variable 'kMax' must be initialized by a constant expression"
+  // on this line, then you're creating a BitField for an enum with more bits
+  // than needed for the enum values. Either reduce the BitField size,
+  // or give the enum an explicit underlying type.
   static constexpr T kMax = static_cast<T>(kNumValues - 1);
 
   template <class T2, int size2>
diff --git a/src/compiler/backend/instruction-codes.h b/src/compiler/backend/instruction-codes.h
@@ -198,7 +198,7 @@ V8_EXPORT_PRIVATE std::ostream& operator<<(std::ostream& os,
   V(None)                       \
   TARGET_ADDRESSING_MODE_LIST(V)
 
-enum AddressingMode {
+enum AddressingMode : uint8_t {
 #define DECLARE_ADDRESSING_MODE(Name) kMode_##Name,
   ADDRESSING_MODE_LIST(DECLARE_ADDRESSING_MODE)
 #undef DECLARE_ADDRESSING_MODE
@@ -309,7 +309,7 @@ using MiscField = base::BitField<int, 22, 10>;
 // LaneSizeField and AccessModeField are helper types to encode/decode a lane
 // size, an access mode, or both inside the overlapping MiscField.
 using LaneSizeField = base::BitField<int, 22, 8>;
-using AccessModeField = base::BitField<MemoryAccessMode, 30, 2>;
+using AccessModeField = base::BitField<MemoryAccessMode, 30, 1>;
 // TODO(turbofan): {HasMemoryAccessMode} is currently only used to guard
 // decoding (in CodeGenerator and InstructionScheduler). Encoding (in
 // InstructionSelector) is not yet guarded. There are in fact instructions for
diff --git a/src/compiler/backend/instruction.h b/src/compiler/backend/instruction.h
@@ -591,8 +591,8 @@ class LocationOperand : public InstructionOperand {
   }
 
   static_assert(KindField::kSize == 3);
-  using LocationKindField = base::BitField64<LocationKind, 3, 2>;
-  using RepresentationField = base::BitField64<MachineRepresentation, 5, 8>;
+  using LocationKindField = base::BitField64<LocationKind, 3, 1>;
+  using RepresentationField = LocationKindField::Next<MachineRepresentation, 8>;
   using IndexField = base::BitField64<int32_t, 35, 29>;
 };
 
diff --git a/src/handles/global-handles.cc b/src/handles/global-handles.cc
@@ -575,7 +575,7 @@ class GlobalHandles::Node final : public NodeBase<GlobalHandles::Node> {
 
   // This stores three flags (independent, partially_dependent and
   // in_young_list) and a State.
-  using NodeState = base::BitField8<State, 0, 3>;
+  using NodeState = base::BitField8<State, 0, 2>;
   // Tracks whether the node is contained in the set of young nodes. This bit
   // persists across allocating and freeing a node as it's only cleaned up
   // when young nodes are proccessed.
diff --git a/src/maglev/maglev-ir.h b/src/maglev/maglev-ir.h
@@ -315,7 +315,7 @@ class OpProperties {
     return kNeedsRegisterSnapshotBit::decode(bitfield_);
   }
   constexpr bool is_pure() const {
-    return (bitfield_ | kPureMask) == kPureValue;
+    return (bitfield_ & kPureMask) == kPureValue;
   }
   constexpr bool is_required_when_unused() const {
     return can_write() || non_memory_side_effects();
diff --git a/src/wasm/wasm-code-manager.h b/src/wasm/wasm-code-manager.h
@@ -487,7 +487,7 @@ class V8_EXPORT_PRIVATE WasmCode final {
   int trap_handler_index_ = -1;
 
   // Bits encoded in {flags_}:
-  using KindField = base::BitField8<Kind, 0, 3>;
+  using KindField = base::BitField8<Kind, 0, 2>;
   using ExecutionTierField = KindField::Next<ExecutionTier, 2>;
   using ForDebuggingField = ExecutionTierField::Next<ForDebugging, 2>;
 

Original file line number	Diff line number	Diff line change
`@@ -591,8 +591,8 @@ class LocationOperand : public InstructionOperand {`
`591`	`591`	`}`
`592`	`592`
`593`	`593`	`static_assert(KindField::kSize == 3);`
`594`		`- using LocationKindField = base::BitField64<LocationKind, 3, 2>;`
`595`		`- using RepresentationField = base::BitField64<MachineRepresentation, 5, 8>;`
	`594`	`+ using LocationKindField = base::BitField64<LocationKind, 3, 1>;`
	`595`	`+ using RepresentationField = LocationKindField::Next<MachineRepresentation, 8>;`
`596`	`596`	`using IndexField = base::BitField64<int32_t, 35, 29>;`
`597`	`597`	`};`
`598`	`598`
Original file line number	Diff line number	Diff line change
`@@ -315,7 +315,7 @@ class OpProperties {`
`315`	`315`	`return kNeedsRegisterSnapshotBit::decode(bitfield_);`
`316`	`316`	`}`
`317`	`317`	`constexpr bool is_pure() const {`
`318`		`- return (bitfield_ \| kPureMask) == kPureValue;`
	`318`	`+ return (bitfield_ & kPureMask) == kPureValue;`
`319`	`319`	`}`
`320`	`320`	`constexpr bool is_required_when_unused() const {`
`321`	`321`	`return can_write() \|\| non_memory_side_effects();`