[lldb] Add a static_assert that g_core_definitions matches the Core enum

[JDevlieghere]

This PR uses the same trick as 7ced9ff to ensure the g_core_definitions table is correctly indexed by the Core enum. It's easy to make a mistake. Case in point: this caught two entries that appeared in the wrong order.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

This PR uses the same trick as 7ced9ff to ensure the g_core_definitions table is correctly indexed by the Core enum. It's easy to make a mistake. Case in point: this caught two entries that appeared in the wrong order.

---

Full diff: https://github.com/llvm/llvm-project/pull/159452.diff

1 Files Affected:

• (modified) lldb/source/Utility/ArchSpec.cpp (+12-3)

diff --git a/lldb/source/Utility/ArchSpec.cpp b/lldb/source/Utility/ArchSpec.cpp
index 2a87cc6bf7de9..079a77690c745 100644
--- a/lldb/source/Utility/ArchSpec.cpp
+++ b/lldb/source/Utility/ArchSpec.cpp
@@ -41,7 +41,7 @@ struct CoreDefinition {
 } // namespace lldb_private
 
 // This core information can be looked using the ArchSpec::Core as the index
-static const CoreDefinition g_core_definitions[] = {
+static constexpr const CoreDefinition g_core_definitions[] = {
     {eByteOrderLittle, 4, 2, 4, llvm::Triple::arm, ArchSpec::eCore_arm_generic,
      "arm"},
     {eByteOrderLittle, 4, 2, 4, llvm::Triple::arm, ArchSpec::eCore_arm_armv4,
@@ -90,12 +90,12 @@ static const CoreDefinition g_core_definitions[] = {
      "thumbv6m"},
     {eByteOrderLittle, 4, 2, 4, llvm::Triple::thumb, ArchSpec::eCore_thumbv7,
      "thumbv7"},
-    {eByteOrderLittle, 4, 2, 4, llvm::Triple::thumb, ArchSpec::eCore_thumbv7f,
-     "thumbv7f"},
     {eByteOrderLittle, 4, 2, 4, llvm::Triple::thumb, ArchSpec::eCore_thumbv7s,
      "thumbv7s"},
     {eByteOrderLittle, 4, 2, 4, llvm::Triple::thumb, ArchSpec::eCore_thumbv7k,
      "thumbv7k"},
+    {eByteOrderLittle, 4, 2, 4, llvm::Triple::thumb, ArchSpec::eCore_thumbv7f,
+     "thumbv7f"},
     {eByteOrderLittle, 4, 2, 4, llvm::Triple::thumb, ArchSpec::eCore_thumbv7m,
      "thumbv7m"},
     {eByteOrderLittle, 4, 2, 4, llvm::Triple::thumb, ArchSpec::eCore_thumbv7em,
@@ -257,6 +257,15 @@ static_assert(sizeof(g_core_definitions) / sizeof(CoreDefinition) ==
                   ArchSpec::kNumCores,
               "make sure we have one core definition for each core");
 
+template <int I> struct ArchSpecValidator : ArchSpecValidator<I + 1> {
+  static_assert(g_core_definitions[I].core == I,
+                "g_core_definitions order doesn't match Core enumeration");
+};
+
+template <> struct ArchSpecValidator<ArchSpec::eCore_wasm32> {};
+
+ArchSpecValidator<ArchSpec::eCore_arm_generic> validator;
+
 struct ArchDefinitionEntry {
   ArchSpec::Core core;
   uint32_t cpu;

[JDevlieghere]

This is what the compiler tells you when you make a mistake:

/Users/jonas/llvm/llvm-project/lldb/source/Utility/ArchSpec.cpp:263:34: note: expression evaluates to '24 == 25'
  263 |       g_core_definitions[I].core == I,
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
/Users/jonas/llvm/llvm-project/lldb/source/Utility/ArchSpec.cpp:263:7: error: static assertion failed due to requirement 'g_core_definitions[24].core == 24': g_core_definitions order doesn't match Core enumeration
  263 |       g_core_definitions[I].core == I,
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[jasonmolenda]

pretty cool, I don't understand templates enough to really get how it works, but definitely a nice safety net.

[DavidSpickett]

ArchSpecValidator<ArchSpec::eCore_arm_generic> starts instantiating a chain of ArchSpecValidator<I + 1> until it gets to ArchSpecValidator<ArchSpec::eCore_wasm32> which does not inherit from anything so it stops there.

arm_generic being the first core value and wasm32 being the last. At least right now.

Could you use kNum_Cores so we don't have to remember to update this when new ones are added?

Also the eCore_wasm32 one has an empty body, so does that mean it does not get checked?

And you say 2 cores were in the wrong order but only one is moved in the diff. Maybe the other is a downstream thing?

[JDevlieghere]

Could you use kNum_Cores so we don't have to remember to update this when new ones are added?

Also the eCore_wasm32 one has an empty body, so does that mean it does not get checked?

Good catch, that's an unintentional off-by-one. Using kNum_Cores addresses both.

And you say 2 cores were in the wrong order but only one is moved in the diff. Maybe the other is a downstream thing?

Haha, I guess it depends on how you (or the diff algorithm) looks at it. thumbv7s and thumbv7k showed up before thumbv7f which is why I said 2, which of course can be be fixed by moving just thumbv7f which is equivalent to moving the other two down.

[DavidSpickett]

1 moves up or 2 move down, it's the same picture.

LGTM.