[PATCH] D123623: [Debuginfo][llvm-dwarfutil] Add check for unsupported debug sections.

Tue Apr 12 23:47:56 PDT 2022

jhenderson added a comment.

> The error would be displayed for critical(such that could not be removed) sections and the source file would be skipped. Other unsupported sections would be removed and warning message should be displayed

I have concerns about this difference in behaviour between "critical" and "non-critical" sections:

1. I assume this code has a DWARF version check, but if not, the assumption of what is "critical"/"non-critical" is going to fall down the next time a DWARF standard is published (potentially). Even then, developers of this tool will have to remember to update this list when adding DWARFv6 etc support. It feels fragile.
2. There's an assumption here that all .debug_* sections are part of the DWARF standard. I know of at least one downstream usage within my company where we've extended the DWARF spec by adding a new section. This was deliberately called .debug_* to ensure it got stripped by tools when stripping debug information, but is, if I'm not mistaken, otherwise independent of the debug information. By having a hard-coded list, we'll have to carry a private downstream patch around or anybody using this tool will end up having their debug data corrupted. Whilst I acknowledge that we don't need to worry about downstream users that much, I do think it's pointing to the fact that this is the wrong approach again, although I don't have a concrete answer to what the right approach should be.
3. What determines what is a "critical" versus a "non-critical" section? Surely that depends upon the consumer as well as the structure of the DWARF?

================
Comment at: llvm/test/tools/llvm-dwarfutil/ELF/error-unsupported-types.test:4
+
+# RUN: llvm-dwarfutil --garbage-collection --tombstone=maxpc %p/Inputs/type-units.o %t1 2>&1 | FileCheck %s -DFILE=%p/Inputs/type-units.o
+
----------------
Why does this use a canned binary rather than yaml2obj?

================
Comment at: llvm/tools/llvm-dwarfutil/DebugInfoLinker.cpp:276

+  // Unknown debug sections would be removed. Display warning
+  // for such sections.
----------------
This comment needs reflowing too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123623