[Lldb-commits] [PATCH] D53435: [x86] Fix issues with a realigned stack in MSVC compiled applications

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks Aleksandr, this all looks good.  I'd like to see a definition of AFA (and we can say "CFA is DWARF's Canonical Frame Address" in the same spot) maybe in UnwindPlan.h, non-Windows people (ok, me) will not know what that is referring to.  I was a little worried about and_rsp_pattern_p() but we're not recording the and'ed value.  One long standing problem of the UnwindPlan intermediate representation is that it doesn't have any way to represent an alignment involved in the location computation (short of a DWARF expression).  This is a problem on AArch32 / armv7 where the NEON registers are saved on higher alignment boundaries than the stack pointer on function call.  (if the alignment is less-than or equal-to the stack frame, then there's no align instructions needed in the prologue.)  But you're not doing anything like that, no worries.



================
Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.h:195
 
   // Get the CFA register for a given frame.
+  bool ReadFrameAddress(lldb::RegisterKind register_kind,
----------------
Comment update, e.g. Get the Frame Address register for a given frame.


================
Comment at: source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp:399
+// movq %rsp, %rbx [0x48 0x8b 0xdc] or [0x48 0x89 0xe3]
+// movl %esp, %ebx [0x8b 0xdc] or [0x89 0xe3]
+bool x86AssemblyInspectionEngine::mov_rsp_rbx_pattern_p() {
----------------
thanks for using with at&t syntax in these comments ;) 


================
Comment at: source/Symbol/UnwindPlan.cpp:202
+  m_afa_value.Dump(s, unwind_plan, thread);
+
   s.Printf(" => ");
----------------
What do you think about only printing the AFA= entry if it is != LLDB_INVALID_ADDRESS?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53435