[Lldb-commits] [PATCH] D124198: [LLDB][Unwind] Add stack scanning as fallback unwind plan if no symbol file is available.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

It is unclear how this patch works. Was there support for searching for the return address on the stack already? And this patch is just adding the a few register unwind rules for when/if the return address _is_ found for x86?

This will need tests.



================
Comment at: lldb/include/lldb/Symbol/UnwindPlan.h:225
+          // Not the first search.
+          m_value.ra_search.search_offset |= 1;
+        }
----------------
What is this magic number/bit here? Is it ok to clobber bit zero here?


================
Comment at: lldb/include/lldb/Symbol/UnwindPlan.h:266
+      int32_t GetRaSearchOffset() const {
+        return m_type == isRaSearch ? m_value.ra_search.search_offset & ~1 : 0;
+      }
----------------
Are we assuming "search_offset" must be aligned to at least a 4 bit boundary so that we can put something in bit zero?


================
Comment at: lldb/include/lldb/Symbol/UnwindPlan.h:270
+      bool IsFirstRaSearch() const {
+        return m_value.ra_search.search_offset % 2 == 0;
+      }
----------------
You are using & and | in other locations, and now we are using modulo? I would just keep with the & operators


================
Comment at: lldb/include/lldb/Target/ABI.h:99
 public:
+  virtual bool CreateStackWalkingUnwindPlan(UnwindPlan &unwind_plan) = 0;
+
----------------
Add a default implementation that returns false so you don't have to change all of the classes that don't implement this.


================
Comment at: lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.h:30-33
+  bool
+  CreateStackWalkingUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override {
+    return false;
+  }
----------------
Don't make the CreateStackWalkingUnwindPlan pure virtual if most of the implementations are just "return false;". I would make a default implementation in the base class that just returns false. 


================
Comment at: lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp:734
 
+bool ABIWindows_x86_64::CreateStackWalkingUnwindPlan(UnwindPlan &unwind_plan) {
+  unwind_plan.Clear();
----------------
What code actually does the search for a return address? Is that already available somewhere in the unwind plans? Reading through this plan I see that it sets the CFA to be RA search but I fail to see any searching going on.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1258
   std::vector<clang::ParmVarDecl *> params;
-  while (begin != end && param_count > 0) {
+  for (uint32_t i = 0; i < param_count && begin != end;) {
     uint32_t record_offset = begin.offset();
----------------
all changes in this file don't seem related to this patch? Revert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124198