[PATCH] D58698: [DWARFFormValue] Don't consider DW_FORM_data4/8 to be section offsets.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp:200
+    // In DWARF3 DW_FORM_data4 and DW_FORM_data8 served also as a section
+    // offset. If we don't have a DWARFUnit, default to the old behavior.
+    if (Form == DW_FORM_data4 || Form == DW_FORM_data8)
----------------
aprantl wrote:
> aprantl wrote:
> > "some producers" here means older versions of clang, but since older versions of clang also default to DWARF 2 (at least on Darwin) I think this is a safe change.
> To clarify: what we are dropping here is support for compilers that claim to emit DWARF 3+ but still is a FORM_data4 for a section offset. I'm fine with dropping support for these.
> 
> I'm not a fan of DWARFFormValue optionally having a DWARFUnit pointer. This is asking for trouble. If we could assert that we have on in this condition (or get rid of the optionality) I'd be much happier.
I mean... we could drop support for this convenience feature when dumping DWARFv3 and older entirely?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/formclass2.s:3-4
+#   struct e {
+#     enum {} f[16384];
+#     short g;
+#   };
----------------
I'd consider making these both chars, just to make it clear that their type isn't interesting, perhaps.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/formclass2.s:6-9
+#   e foo() {
+#     auto E = new e;
+#     return *E;
+#   }
----------------
I see this is necessary to produce a debug_loc section in the output. Perhaps that could be commented and/or maybe there's a more direct way to do it? (and/or maybe it'd be worth separating the need for a debug_loc section (& the code that tickles it sufficiently to occur) from the code that ensures 'e' is emitted in the DWARF)

Also - any idea why there's on an error emitted when debug_loc is present but too small? Should we emit an error even when it's not present? I mean, I can see the counterargument (could've stripped/objcopied this one section for test purposes, etc)

Also - 16384 seems rahter large. Any idea why the offset of 'g' must be /that/ large before the warning is emitted? oh... because it turns it into a data4, rather than a data2, etc. OK


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58698/new/

https://reviews.llvm.org/D58698