This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo/AccelTable] Fix inconsistency in getDIEOffset implementations
ClosedPublic

Authored by labath on Mar 7 2018, 4:41 AM.

Details

Summary

Even though the getDIEOffset offset function was common for the two
accelerator table implementations, it was doing two different things:
for the Apple tables, it was returning the die offset relative to the
start of the section, whereas for DWARF v5 tables, it was relative to
the start of the CU.

I resolve this by renaming the function to getDIESectionOffset to make
it obvious what the function returns, and change the DWARF
implementation to return the section offset. I also keep the CU-relative
accessor, but only in the DWARF implementation (there is no way to get
this information for the Apple tables). This was not caught by existing
tests because the hand-written inputs also erroneously used section
offsets instead of CU-relative ones.

While looking at this, I noticed that the Apple implementation was not
fully correct either -- the header contains a DIEOffsetBase field, which
should be added to all offsets (but it wasn't being used). This went unnoticed
because all current writers set this field to zero anyway. I fix this as well
and add a hand-written test which demonstrates the issue.

Diff Detail

Event Timeline

labath created this revision.Mar 7 2018, 4:41 AM
labath added inline comments.Mar 7 2018, 4:52 AM
lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
282

Actually, after reading the "spec" https://llvm.org/docs/SourceLevelDebugging.html#name-accelerator-tables, I'm starting to be unsure about this part. The document says: "base DIE offset that should be added to any atoms that are encoded using the DW_FORM_ref1, DW_FORM_ref2, DW_FORM_ref4, DW_FORM_ref8 or DW_FORM_ref_udata", but we our producers encode this field as DW_FORM_data4.

JDevlieghere requested changes to this revision.Mar 7 2018, 7:52 AM

Great catch Pavel, I can imagine this preventing quite some confusion in the future. Thanks!

Requesting changes because I also don't think the DIEOffsetBase is correct, as explained inline.

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
282

Indeed, I don't think this is correct. While we currently don't have any atoms using this encoding, technically nothing prevents you from defining one with DW_FORM_ref* which would require using the DIEOffsetBase. I don't know the history behind this though, maybe @aprantl knows more?

This revision now requires changes to proceed.Mar 7 2018, 7:52 AM

+@aprantl to weigh in on the comment.

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
282

So, to be correct, do you think we should check explicitly check for the specified forms here?
(Also, whatever the solution, it should probably be also applied to the getCUOffset function below).

JDevlieghere added inline comments.Mar 8 2018, 2:59 AM
lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
282

Yes, I think so. In reality the relationship between the atom type (i.e. DW_ATOM_die_offset) and its form (ie. DW_FORM_data4) is going to be pretty much fixed, but as the form is part of the atom in the header, theoretically it could be anything.

labath updated this revision to Diff 137560.Mar 8 2018, 5:39 AM
  • Add the DIEOffsetBase only for the DW_FORM_ref*** family. There's a function in DWARFFormValue that could be of use for this, but it assumes you have the entire DWARFUnit around, and not just it's offset. As it stands now, I just hard-code the list of forms and perform the addition manually.
  • Treat CU and die offsets identically.
This revision is now accepted and ready to land.Mar 8 2018, 9:54 AM
This revision was automatically updated to reflect the committed changes.