[LLDB] Missing break in a switch statement alters the execution flow.

[fixathon]

Summary

Looks like a typo from the past code changes.

[fixathon]

Added in-code comments with details

lldb/source/Plugins/Process/Utility/ARMUtils.h

48–49

These lines were unreachable prior to the fix due to the default label with no break skipping to case 0. Past code history suggests that was accidental.

314–317

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.

[DavidSpickett]

lldb/source/Plugins/Process/Utility/ARMUtils.h

29

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)

314–317

This part LGTM. This "bits" function is returning uint32 so the compiler can't know it'll only be in a 2 bit range.

[fixathon]

Addressed comments

[fixathon]

Code update to address the comments

Differential Revision: https://reviews.llvm.org/D131244

[DavidSpickett]

lldb/source/Plugins/Process/Utility/ARMUtils.h

49

This is now dead code?

[fixathon]

lldb/source/Plugins/Process/Utility/ARMUtils.h

29

Added the assert back

49

This will execute for the default case after its break.

[fixathon]

closing as already patched.

lldb/source/Plugins/Process/Utility/ARMUtils.h

 static inline uint32_t Align(uint32_t val, uint32_t alignment) {
   return alignment * (val / alignment);
 }
 
 static inline uint32_t DecodeImmShift(const uint32_t type, const uint32_t imm5,
                                       ARM_ShifterType &shift_t) {
   switch (type) {
   default:
-  // assert(0 && "Invalid shift type");
+    assert(0 && "Invalid shift type");
+    break;
   case 0:
     shift_t = SRType_LSL;
     return imm5;
   case 1:
     shift_t = SRType_LSR;
     return (imm5 == 0 ? 32 : imm5);
   case 2:
     shift_t = SRType_ASR;
     return (imm5 == 0 ? 32 : imm5);
   case 3:
     if (imm5 == 0) {
       shift_t = SRType_RRX;
       return 1;
     } else {
       shift_t = SRType_ROR;
       return imm5;
     }
   }
   shift_t = SRType_Invalid;
   return UINT32_MAX;
 }
 
 // A8.6.35 CMP (register) -- Encoding T3
 // Convenience function.
 static inline uint32_t DecodeImmShiftThumb(const uint32_t opcode,
                                            ARM_ShifterType &shift_t) {
   return DecodeImmShift(Bits32(opcode, 5, 4),
                         Bits32(opcode, 14, 12) << 2 | Bits32(opcode, 7, 6),
static inline uint32_t ARMExpandImm(uint32_t opcode) {
   uint32_t carry_in = 0;
   uint32_t carry_out;
   return ARMExpandImm_C(opcode, carry_in, carry_out);
 }
 
 // (imm32, carry_out) = ThumbExpandImm_C(imm12, carry_in)
 static inline uint32_t ThumbExpandImm_C(uint32_t opcode, uint32_t carry_in,
                                         uint32_t &carry_out) {
-  uint32_t imm32; // the expanded result
+  uint32_t imm32 = 0; // the expanded result
   const uint32_t i = bit(opcode, 26);
   const uint32_t imm3 = bits(opcode, 14, 12);
   const uint32_t abcdefgh = bits(opcode, 7, 0);
   const uint32_t imm12 = i << 11 | imm3 << 8 | abcdefgh;
 
   if (bits(imm12, 11, 10) == 0) {
     switch (bits(imm12, 9, 8)) {
     default: // Keep static analyzer happy with a default case
+      break;
+
     case 0:
       imm32 = abcdefgh;
       break;
 
     case 1:
       imm32 = abcdefgh << 16 | abcdefgh;
       break;
 
     case 2: