[Lldb-commits] [PATCH] D140067: Fix an ASAN bug I introduced in debugserver, accessing off the end of an array intentionally

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.


================
Comment at: lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:2026-2037
+        if (reg == gpr_pc)
+          value->value.uint64 = clear_pac_bits(
+              reinterpret_cast<uint64_t>(m_state.context.gpr.__opaque_pc));
+        else if (reg == gpr_lr)
+          value->value.uint64 = clear_pac_bits(
+              reinterpret_cast<uint64_t>(m_state.context.gpr.__opaque_lr));
+        else if (reg == gpr_sp)
----------------
aprantl wrote:
> jasonmolenda wrote:
> > JDevlieghere wrote:
> > > `reg` is a uint32_t so you could use a switch here:
> > > 
> > > ```
> > > switch(reg) {
> > >   case gpr_pc: 
> > >     ...
> > > }
> > > ```
> > I thought about that, but then I'd need a `break;` after each element and it was one extra line.  I don't have a real preference, and this was shorter.
> If we are already micro-optimizing, I guess the resulting code would be shorter (and 100% UB-free) if it were
> ```
> uint64_t val = 0;
> switch(reg) {
>   case gpr_pc: 
>       memcpy(&val, m_state.context.gpr.__opaque_pc);
>       break;
>   ...
> }
> value->value.uint64 = clear_pac_bits(val);
> ```
I don't have anything against using memcpy instead of the reinterpret_cast, but I don't see it being any clearer (IMO), and it doesn't reduce the code duplication - we still need separate lines for when the members are named __opaque_ and when not.  If anyone feels it's clearer to use a switch statement and/or memcpy, I genuinely don't care either way, but I don't see it myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140067