[PATCH] D74169: [WIP][LLD][ELF][DebugInfo] Skeleton implementation of removing obsolete debug info.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

A few comments from me.



================
Comment at: lld/ELF/Config.h:158
   bool gcSections;
+  bool gcDebuginfo;
   bool gdbIndex;
----------------
Unsorted (shold be placed before `gcSections` + should be `gcDebugInfo` I think.




================
Comment at: lld/ELF/Driver.cpp:2006
+  if (config->gcSections && config->gcDebuginfo)
+    linkDebugInfo<ELFT>();
+
----------------
Looking at this piece I think that 915, 916 lines should just be:

```
config->gcDebuginfo = config->gcSections &&
      args.hasFlag(OPT_gc_debuginfo, OPT_no_gc_debuginfo, false);
```

Perhaps worth to report an error when we have --gc-debug info, but not --gc-sections, but I am not sure here.


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:66
+template <class ELFT> void linkDebugInfo() {
+  assert(config->gcDebuginfo);
+
----------------
We usually do not do such things in LLD. I.e. do not assert on options when the code is trivial like in your case:

```
  if (config->gcSections && config->gcDebuginfo)
    linkDebugInfo<ELFT>();
```


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:83
+      dwarfContexts.push_back(std::unique_ptr<DWARFContext>(
+          new DWARFContext(std::make_unique<LLDDwarfObj<ELFT>>(obj))));
+
----------------
Please do not use `new`. Is should be something like:

`dwarfContexts.push_back(std::make_unique...`


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:88
+
+      objectsForLinking.push_back(
+          std::make_unique<DwarfLinkerObjFile>(
----------------
It would probably be a bit more natural to:

```
std::unique_ptr<DwarfLinkerObjFile> Obj = std::make_unique<DwarfLinkerObjFile>(
              file->getName(), dwarfObjFile, emptyWarnings);
Obj->Addresses = ;
debugInfoLinker.addObjectFile(*Obj);
objectsForLinking.push_back(std::move(Obj));
```




================
Comment at: lld/ELF/LLDDwarfLinker.cpp:93
+      objectsForLinking.back()->Addresses =
+          std::unique_ptr<AddressesMap>(new ObjFileAddressMap);
+
----------------
`Addresses = std::make_unique<ObjFileAddressMap>();`


================
Comment at: lld/ELF/LLDDwarfLinker.h:17
+// and related relocations to remove parts of debug info data
+// which points to sections to be removed.
+template <class ELFT> void linkDebugInfo();
----------------
I am not an expert in English, though would rewrite to something like:

```
// This function is used to remove an unused parts of debug data
// which belongs to garbage collected sections.
```

(I guess othes might have a better variant though).


================
Comment at: lld/test/ELF/gc-debuginfo.s:7
+
+# RUN: echo '.global _start; .global main;  _start: jmp main; main: ' \
+# RUN:   | llvm-mc --filetype=obj --triple=x86_64-unknown-linux - -o %t.o
----------------
No need to use that much asm:

`llvm-mc /dev/null -o %t.o -filetype=obj -triple=aarch64-pc-linux`


================
Comment at: lld/test/ELF/gc-debuginfo.s:15
+# CHECK: file format ELF64-x86-64
+# CHECK-NOT: {{.}}
----------------
I'd add a test for `-r --gc-debuginfo` error too since you have this code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74169