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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D123623#3447455 <https://reviews.llvm.org/D123623#3447455>, @jhenderson wrote:

>> 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.

There is no check for DWARF version at the moment. I think it would be good to add.

At the moment DWARFLinker has partial support for DWARF v5 and partial support for DWARF v4 sections(it does not update .debug_macinfo, does not handle debug_types).

> 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.

The same assumption is used in other llvm parts f.e. ELFObjcopy.cpp:

  static bool isDebugSection(const SectionBase &Sec) {
    return StringRef(Sec.Name).startswith(".debug") ||
           StringRef(Sec.Name).startswith(".zdebug") || Sec.Name == ".gdb_index";
  }

To resolve that case with custom sections we can add option --keep-section=debug_custom.

> 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?

The difference is whether section might be safely dropped or not(in other words whether section is important for optimizations done by tool). f,e,

a) the tool need to handle address ranges for garbage collection. Since the tool does not able to process .debug_rnglists section then it could not do garbage collection at all. This is critical section.

b) .debug_cu_index is not used for garbage collection. But it references debug_info which rebuilt by tool. the tool does not update .debug_cu_index so it becomes invalid after garbage collection. Though it may be easily dropped. This is not critical section.



================
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
+
----------------
jhenderson wrote:
> Why does this use a canned binary rather than yaml2obj?
yaml2obj does not support debug_types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123623