[PATCH] D46628: [ELF] Add --strip-debug-non-line option

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Driver.cpp:1853
+          .EndsWith("debug_str", false)
+          .StartsWith(".debug", true)
+          .StartsWith(".zdebug", true)
----------------
```
if (S->Name.consume_front(".debug_") || S->Name.consume_front(".zdebug_")) {
  ...
}
```


================
Comment at: lld/ELF/OutputSections.cpp:205
 
+  ReducedDebugData.clear();
+
----------------
Neither `vector::clear` nor `SmallVector::clear` deallocates the storage. Did you intend to call `shrink_to_fit()`?


================
Comment at: lld/ELF/Writer.cpp:1073
+template <typename value_type>
+void writeInt(raw_ostream &OS, value_type value, endianness endian) {
+  value = byte_swap<value_type>(value, endian);
----------------
Delete this helper.

Use `endian::write(OS, UINT32_C(0xffffffff), Endian);` below.

(this utility is defined in llvm/Support/EndianStream.h)


================
Comment at: lld/ELF/Writer.cpp:1078
+
+template <class ELFT> void Writer<ELFT>::reduceNonlineDwarfDebug() {
+  OutputSection *AbbrSec = nullptr;
----------------
`NonLine`. The option name treats `non-line` separately.


================
Comment at: lld/ELF/Writer.cpp:1083
+  for (OutputSection *Sec : OutputSections)
+    if (Sec->Name == ".debug_abbrev") {
+      AbbrSec = Sec;
----------------
```
for (OutputSection *Sec : OutputSections) {
  if (Sec->Name == ".debug_abbrev")
    AbbrSec = Sec;
  else if ...
}


================
Comment at: lld/ELF/Writer.cpp:1103
+    uint8_t AddressSize = Config->Is64 ? 8 : 4;
+    return llvm::DataExtractor(Data, IsLE, AddressSize);
+  };
----------------
`Config->IsLE`


================
Comment at: lld/ELF/Writer.cpp:1161
+  std::unordered_map<uint32_t, uint32_t> InfoOffMap;
+  SmallVector<char, 1> ReducedInfo;
+  // Reduce .debug_info
----------------
SmallString<0>

The allocation is likely unavoidable. Don't waste stack space.


================
Comment at: lld/ELF/Writer.cpp:1181
+
+      uint32_t NextInfoOffset = InfoOffset + Length;
+      uint16_t Version = InfoData.getU16(&InfoOffset);
----------------
`NextInfoOffset` is only used once. Just `InfoOffset += Length` below.


================
Comment at: lld/ELF/Writer.cpp:1219
+      if (IsDWARF64) {
+        writeInt(OS, uint32_t(0xFFFFFFFF), Endian);
+      }
----------------
delete `{}`


================
Comment at: lld/test/ELF/strip-debug-non-line-multi-cu.s:5
+# RUN: ld.lld %t %tbar -o %t2 --strip-debug-non-line
+# RUN: llvm-readobj -sections -symbols %t2 | FileCheck %s --check-prefix=CHECK-READOBJ
+# RUN: llvm-dwarfdump -verify %t2 | FileCheck %s --check-prefix=CHECK-DWARFDUMP-VERIFY
----------------
`--long` (instead of `-long`) is preferred in `llvm-readobj`.


================
Comment at: lld/test/ELF/strip-debug-non-line-multi-cu.s:72
+	.byte	37                      # DW_AT_producer
+	.byte	14                      # DW_FORM_strp
+	.byte	19                      # DW_AT_language
----------------
`.byte 8 # DW_FORM_string` allows you to inline the DW_AT_producer string. Or delete it if it is not necessary (the `clang version` string is too long but probably not relevant to this feature)


================
Comment at: lld/test/ELF/strip-debug-non-line.s:2
+# REQUIRES: x86
+# RUN: llvm-mc -g -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: ld.lld %t -o %t2 --strip-debug-non-line
----------------
`-o %t.o`

It is more natural to use `.o` for object files. Then use `%t` for the executable.


================
Comment at: lld/test/ELF/strip-debug-non-line.s:5
+# RUN: llvm-readobj -sections -symbols %t2 | FileCheck %s --check-prefix=CHECK-READOBJ
+# RUN: llvm-dwarfdump -verify %t2 | FileCheck %s --check-prefix=CHECK-DWARFDUMP-VERIFY
+# RUN: llvm-dwarfdump -debug-info %t2 | FileCheck %s --check-prefix=CHECK-DWARFDUMP-INFO
----------------
`llvm-dwarfdump -verify` returns 1 if there is any error, so you can omit `| FileCheck`.


================
Comment at: lld/test/ELF/strip-debug-non-line.s:10
+
+# CHECK-READOBJ: Name: .{{z?}}debug_str
+# CHECK-READOBJ: Name: .{{z?}}debug_abbrev
----------------
`.zdebug_str` is not possible in the output. So you can just check `Name: .debug_str`


================
Comment at: lld/test/ELF/strip-debug-non-line.s:15
+# CHECK-READOBJ: Name: .{{z?}}debug_line
+# CHECK-READOBJ-NOT: Name: .{{z?}}debug_macinfo
+# CHECK-READOBJ-NOT: Name: .{{z?}}debug_types
----------------
The `*-NOT` directive checks a string doesn’t occur between two matches. This doesn't guarantee `.debug_macinfo` is absent.

You may need: `--implicit-check-not=.debug_macinfo --implicit-check-not=.debug_types`


================
Comment at: lld/test/ELF/strip-debug-non-line.s:71
+.Linfo_string1:
+	.asciz	"-"                     # string offset=100
+.Linfo_string2:
----------------
The `# string offset=` comments are not necessary - the literal offsets are not referenced in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D46628