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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D74169#1863026 <https://reviews.llvm.org/D74169#1863026>, @MaskRay wrote:

> `test/ELF/gc-debuginfo.s` works without the code change (after deleting `--gc-debuginfo`). How many changes do you expect will be needed to make the feature work?


(Disclaimer: As a person who is interested in this feature implicitly (Alexey and I do work for the same company))

I wonder if that is really important to split patches that much (this patch seems have no functionality at all?). It would be good to see at least some benefit, but perhaps
it is not that easy to do... Anyways:
Personally I'd probably prefer to see a full set of patches for the feature. A reviewer should be able to apply them and debug (if he/she wants to) probably.



================
Comment at: lld/ELF/CMakeLists.txt:38
   MarkLive.cpp
+  LLDDwarfLinker.cpp
   OutputSections.cpp
----------------
I'd not add "LLD" prefix. All if sources here belong to "LLD" actually.


================
Comment at: lld/ELF/Driver.cpp:2004
+
+  // If -gc-debuginfo specified remove unused debug data
+  if (config->gcSections && config->gcDebuginfo)
----------------
Missing a full stop.


================
Comment at: lld/ELF/LLDDwarfLinker.cpp:12
+// such abandoned debug info LLDDwarfLinker analyzes relocations and removes
+// debug info related to deleted sections.
+//
----------------
Is it a final version of the comment? I'd expect to habe more information here probably about what we exactly do.


================
Comment at: lld/test/ELF/gc-debuginfo.s:1
+# REQUIRES: x86
+	
----------------
I'd ask to add a comment:

```
## This is a test for blah blah..

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74169