[PATCH] D44202: [DebugInfo/AccelTable] Fix inconsistency in getDIEOffset implementations

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:282
+    if (Optional<uint64_t> Off = OffForm->getAsSectionOffset())
+      return *Off + HdrData->DIEOffsetBase;
+  }
----------------
labath wrote:
> JDevlieghere wrote:
> > labath wrote:
> > > 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.
> > 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?
> 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).
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. 


Repository:
  rL LLVM

https://reviews.llvm.org/D44202