[Lldb-commits] [PATCH] D112069: [lldb][AArch64] Add UnwindPlan for Linux sigreturn

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for doing this. Just a couple of comments inline.



================
Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:288
+
+  unwind_plan_sp = std::make_shared<UnwindPlan>(eRegisterKindGeneric);
+  unwind_plan_sp->AppendRow(row);
----------------
I have a feeling that this eRegisterKind enum should correspond with the register numbers used in the unwind plan. It'd be great if it did, as then you could replace arm64_dwarf::{pc,sp} with LLDB_REGNUM_GENERIC_{PC,SP}, and avoid including the arm64 header.  Though if it does work as I remember, then I guess the question is how does this work in the first place.


================
Comment at: lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp:303-305
+  if ((machine == llvm::Triple::ArchType::aarch64 ||
+       machine == llvm::Triple::ArchType::aarch64_be ||
+       machine == llvm::Triple::ArchType::aarch64_32))
----------------
If you passed in a `llvm::Triple`, you could replace this with `triple.isAArch64()`.

And one could imagine that e.g. the environment field of the triple could be important in determining the precise unwind plan.


================
Comment at: lldb/source/Target/RegisterContextUnwind.cpp:900
   // section, so prefer that if available. On other platforms we may need to
   // provide a platform-specific UnwindPlan which encodes the details of how to
   // unwind out of sigtramp.
----------------
labath wrote:
> this comment
I guess we should also update this comment now. Maybe:
```
On some platforms the unwind information for signal handlers is not present or correct. Give the platform plugins a chance to provide substitute plan. Otherwise, use eh_frame.
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112069