Page MenuHomePhabricator

[MC] Sort DWARF FDEs by the associated CIE before emitting them.
ClosedPublic

Authored by efriedma on Feb 14 2019, 5:09 PM.

Details

Summary

This isn't technically necessary according to the DWARF standard, but the Android libunwindstack rejects eh_frame sections where an FDE refers to a CIE other than the closest previous CIE.

I think I would consider this a bug in libunwindstack... as far as I can tell, the DWARF standard doesn't contain any requirement like this. And
I'm not sure we should be adding workarounds for specific unwind implementations. But I have the patch anyway, so I'm posting it.

I'd appreciate it if someone from Google could contact the owners of libstackunwind to see if they have any opinion on this.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Feb 14 2019, 5:09 PM
Herald added a reviewer: jfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I asked Chris to comment here about libunwindstack. Hopefully we can get the problem fixed, but I think this workaround might be necessary for code running on older devices in any case (i.e. NDK apps).

compnerd accepted this revision.Feb 19 2019, 8:47 AM

Actually, I'm not sure if that is a bug in libstackunwind, but rather in LLVM. I think that this is the behaviour that GCC has specifically for the eh_frame section. This constraint is not part of the DWARF spec, but eh_frame even though it is similar to debug_frame, is not part of the DWARF spec, it just uses a similar encoding. The change itself looks good, but, please wait for the discussion to complete before committing this.

lib/MC/MCDwarf.cpp
1857 ↗(On Diff #186943)

While you are in the area, mind just switching this over to a range-based for loop?

This revision is now accepted and ready to land.Feb 19 2019, 8:47 AM

I asked my local expert for an opinion.

"The comments about the DWARF standard are correct. I don’t think it imposes any order or restriction on the placement of FDE’s and CIE’s.

However, the Linux Standard Base design is such that FDE’s can only refer to prior CIE’s, not ones that appear later in .eh_frame. This restriction arises because the CIE pointer in FDE’s are self-relative offsets, and the offset is an unsigned quantity which is always subtracted from the FDE location.

Seems to me that, given this, there’s only two ways you’d want to organize FDE’s and CIE’s: either bunch all the CIE’s at the beginning, followed by all the FDE’s; or each CIE is followed by all its FDE’s. This proposal is suggesting eliminating the former as an option. I wasn’t there when the LSB was formalized, but it seems to me that if they had wanted to have each CIE followed by all its FDE’s, they wouldn’t have put a pointer to the CIE in the FDE in the first place. Would have saved some space and complexity."

Any other comments on this?

efriedma marked an inline comment as done.Mar 14 2019, 4:05 PM
efriedma added inline comments.
lib/MC/MCDwarf.cpp
1857 ↗(On Diff #186943)

I looked at it briefly, but the iterators are used inside the loop to check whether the current CFE is the last one, so there isn't any easy way to express this as a range-based loop.

This revision was automatically updated to reflect the committed changes.
lhames added a subscriber: lhames.Apr 23 2019, 5:57 PM

Late to the party here, but if the DWARF spec doesn’t require FDEs to refer to the most recent CIE, is it worth making this optional, even if it’s on by default? Otherwise we risk writing code that assumes this sanitized layout when the spec doesn’t guarantee it.

I actually just hit this issue in JITLink: its eh-frame parser used to assume that FDEs referred to the most recent CIE (because of course they would, right?), but then broke on output from older clangs. I relaxed JITLink’s eh-frame parser to support references to any previous CIE, only to discover that I could no longer write a test case now that LLVM unconditionally does the “right” thing. :P

Actually never mind: The right thing to do is add the test with a checked in binary (or yaml2obj test case, if I can make that work).