Page MenuHomePhabricator

Add debug_frame section support
ClosedPublic

Authored by labath on Jun 26 2017, 4:44 AM.

Details

Summary

This is a beefed-up version of D33504, which adds support for dwarf 4
debug_frame section format.

The main difference here is that the decision whether to use eh_frame or
debug_frame is done on a per-function basis instead of per-object file.
This is necessary because one module can contain both sections (for
example, the start files added by the linker will typically pull in
eh_frame), but we want to be able to access both, for maximum
information.

I also add unit test for parsing various CFI formats (eh_frame,
debug_frame v3 and debug_frame v4).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jun 26 2017, 4:44 AM

Works well for me. Thank you a lot for bringing it up to scratch!

clayborg requested changes to this revision.Jun 26 2017, 9:04 AM

Glad this is happening. Does this mean we won't see the "bad eh frame" warnings anymore on linux? See inlined comments.

include/lldb/Symbol/DWARFCallFrameInfo.h
38 ↗(On Diff #103922)

Remove "reg_kind" and "is_eh_frame" and replace with DWARFCallFrameInfo::Type. See inlined comment below by the CFIVersion enum.

77 ↗(On Diff #103922)

How about we remove "is_eh_frame" from constructor and make CFIVersion into CFIType and make it public. We went from 2 flavors (DWARF and EH frame) to now 4, so we should just use an enum to define the exact flavor.

enum Type {
  EHFrame,
  DWARFv2,
  DWARFv3,
  DWARFv4
};

The enum is already in DWARFCallFrameInfo so no need for a prefix.

source/Symbol/DWARFCallFrameInfo.cpp
306 ↗(On Diff #103922)

Update to use DWARFCallFrameInfo::Type

315 ↗(On Diff #103922)

Update to use DWARFCallFrameInfo::Type

485 ↗(On Diff #103922)

Update to use DWARFCallFrameInfo::Type

499 ↗(On Diff #103922)

Update to use DWARFCallFrameInfo::Type

567 ↗(On Diff #103922)

Update to use DWARFCallFrameInfo::Type

source/Symbol/FuncUnwinders.cpp
219–221 ↗(On Diff #103922)

What compiler suddenly started including complete unwind info in .debug_frame? They are all supposed, but none ever have. MacOSX x86 and x86_64 has never done this right. I think this if statement is too inclusive. Can you narrow it down? I would love to see an example of this in action. I have never seen the x86 PIC bump code be properly described in any compiler. Are we trying to use this for unwinding frame zero? I would hope not unless we really and carefully know the exact compiler that produced the info. It would be preferable marked with an augmentation code that says "yes, I am really complete everywhere".

source/Symbol/UnwindTable.cpp
62 ↗(On Diff #103922)

Update to use DWARFCallFrameInfo::Type

This revision now requires changes to proceed.Jun 26 2017, 9:04 AM

Glad this is happening. Does this mean we won't see the "bad eh frame" warnings anymore on linux? See inlined comments.

Unfortunately, I doubt it. This does nothing about eh_frame parsing, it just adds debug_frame support. So, you may start seeing more of those, as we will be parsing more stuff. :)

That said, I haven't seen that warning happen, at least not on x86. If you have a simple repro case, I could take a look.

include/lldb/Symbol/DWARFCallFrameInfo.h
38 ↗(On Diff #103922)

I think the enum could be used more prominently internally (I haven't checked that more closely yet). However, I think that using it here is wrong. The reason is that the decision which debug_frame version we are parsing does not happen at this level. This is a per-CIE property -- the same debug_frame section can contain CIE's with different version numbers (e.g. if they were compiled with different compilers or flags -- in fact, that's how I built my test module). At this level, all we need to know is whether we are parsing an eh_frame or debug_frame section, which is a still boolean value.

Theoretically, we could have a separate two-valued (eh, dwarf) enum here, and then, internally, when speaking about a specific CIE, use the 4-valued enum you proposed.

The register kind argument could probably be removed though.

source/Symbol/FuncUnwinders.cpp
219–221 ↗(On Diff #103922)

This is just copied from the equivalent eh_frame function, line 147 (which seems to have been added in r224689). I think what's that trying to say is that eh_frame/debug_frame augmentation is only supported on x86.

clayborg accepted this revision.Jun 26 2017, 10:22 AM

If the CIEs can change per CIE,l then this patch is fine.

include/lldb/Symbol/DWARFCallFrameInfo.h
38 ↗(On Diff #103922)

Ah, interesting. Might be nice to make a DWARF/EHFrame enum later to make the code more readable when DWARFCallFrameInfo are created. No need to do that now if you need to get this in.

This revision is now accepted and ready to land.Jun 26 2017, 10:22 AM
labath added inline comments.Jun 27 2017, 4:08 AM
include/lldb/Symbol/DWARFCallFrameInfo.h
38 ↗(On Diff #103922)

Thanks for the quick review. I'm in a moderate hurry, as an android compiler change caused us to fail to unwind on x86 without debug_frame, but I'll spin up a separate patch for that.

This revision was automatically updated to reflect the committed changes.