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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
