[Lldb-commits] [PATCH] D13245: Add support for .ARM.exidx unwind information

[Jason Molenda via lldb-commits]

jasonmolenda added a comment.

Looks good to me, I don't have anything beyond Pavel's suggestions.  I don't know the format of the ARM .exidx .extab unwind info so I don't have any feedback on the parsing of that.

I forgot about that code in RegisterContextLLDB::GetFullUnwindPlanForFrame() for cases where we have no FuncUnwinders object.  The comment on that,

  // No FuncUnwinders available for this pc (i.e. a stripped function symbol and -fomit-frame-pointer).
  // Try using the eh_frame or the .ARM.exidx information relative to the current PC,
  // and finally fall back on the architectural default unwind.

could be a little clearer.  I'd describe it more like

// No FuncUnwinders available for this pc (stripped function symbols, lldb could not augment
// its function table with another source, like LC_FUNCTION_STARTS or eh_frame in ObjectFileMachO).
// See if eh_frame or the .ARM.exidx tables have unwind information for this address, else
// fall back to the architectural default unwind.

ObjectFileMachO.cpp reads the symbols for the object file and then augments those with the
LC_FUNCTION_STARTS (a raw list of start addresses for the the functions in the binary) or the
eh_frame function ranges.  Having accurate function start addresses is critical for the instruction
profile unwinding.

Compact unwind format is not suitable for a source of function start addresses -- if two functions
have the same unwind signature and exist next to each other in the binary, compact unwind may 
represent them as a single entry, etc.  The ARM unwind formats may have the same behavior.

Anyway, that's something the ObjectFile reader for your platform would need to do and not
the topic of this patch.

BTW "Implemnted based on" -> "Implemented based on".  And thanks for including the 
reference name / revision / URL - that kind of pointer is a huge help for long-term maintenance.


http://reviews.llvm.org/D13245