[Lldb-commits] [PATCH] D129814: Fix stepping over watchpoints in architectures that raise the exception before executing the instruction

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 3 inline comments as done.
jingham added inline comments.


================
Comment at: lldb/source/Target/StopInfo.cpp:730
+        StopInfoWatchpoint *as_wp_stop_info 
+            = reinterpret_cast<StopInfoWatchpoint *>(m_stop_info_sp.get());
+        as_wp_stop_info->SetStepOverPlanComplete();
----------------
labath wrote:
> Change type of `m_stop_info_sp` (and the relevant constructor argument) to `shared_ptr<StopInfoWatchpoint>` to avoid the cast here.
> 
> (btw, the right cast for upcasts is `static_cast`. reinterpret_cast can return wrong results in more [[ https://godbolt.org/z/5W7rz3fKK | complicated scenarios ]])
> Change type of `m_stop_info_sp` (and the relevant constructor argument) to `shared_ptr<StopInfoWatchpoint>` to avoid the cast here.

Cute, I didn't know the voodoo to change the type of the pointer inside a shared pointer while retaining its reference counts.  

> 
> (btw, the right cast for upcasts is `static_cast`. reinterpret_cast can return wrong results in more [[ https://godbolt.org/z/5W7rz3fKK | complicated scenarios ]])

Huh, interesting.  The names seem backwards to me, static_cast sounds like it takes the pointer you gave it and casts it to the target type, whereas reinterpret sounds like the one that figures out what the class hierarchy actually is and does the right thing, but it looks like it's the other way round.


================
Comment at: lldb/source/Target/StopInfo.cpp:805
+      ThreadPlanSP step_over_wp_sp(new ThreadPlanStepOverWatchpoint(
+          *(thread_sp.get()), shared_from_this(), wp_sp));
+      Status error;
----------------
labath wrote:
> And use `static_pointer_cast<StopInfoWatchpoint>(shared_from_this())` here. That way the casting remains confined within the origin class.
I didn't know that voodoo, looks like we use it in other places, however, so I switched to doing it that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129814