[Lldb-commits] [PATCH] D131244: [LLDB] Missing break in a switch statement alters the execution flow.

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:29
+    // assert(0 && "Invalid shift type");
+    break;
   case 0:
----------------
Looking at the codepaths and the reason this function exists, it's either called with a 2 bit number which is the "type" field of an instruction, or it's not called because the type is known to be SRType_RRX already.

In the enum ARM_ShifterType SRType_RRX would be 4, which wouldn't fit the 2 bit expectation.

So I think we can safely re-enable this assert and move the two unreachable lines up here. I didn't get any test failures doing so and I don't see a codepath that could hit it.

(but it would be fairly easy for future changes to make this mistake since it's not very obvious from the types, so an assert is good here)


================
Comment at: lldb/source/Plugins/Process/Utility/ARMUtils.h:314-315
     switch (bits(imm12, 9, 8)) {
     default: // Keep static analyzer happy with a default case
+      break;
+
----------------
fixathon wrote:
> Same here. Past code history suggests the break was missing by accident, i.e the **default** case was added to satisfy a static code checker without the intent to modify the execution flow. 
This part LGTM. This "bits" function is returning uint32 so the compiler can't know it'll only be in a 2 bit range.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131244/new/

https://reviews.llvm.org/D131244