[PATCH] D77807: [LLD][ELF] Implement --discard-* for cases when -r or --emit-relocs are used.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/Writer.cpp:565
+  if (config->copyRelocs && config->discard != DiscardPolicy::None)
+    markUsedLocalSymbols();
+  copyLocalSymbols();
----------------
Perhaps move it inside `copyLocalSymbols()`?

At least because this work can be avoided when `!in.symTab`:

```
template <class ELFT> void Writer<ELFT>::copyLocalSymbols() {
  if (!in.symTab)
    return;
...
```


================
Comment at: lld/ELF/Writer.cpp:649
-  // If -emit-reloc is given, all symbols including local ones need to be
-  // copied because they may be referenced by relocations.
-  if (config->emitRelocs)
----------------
I think it is better to update the comment, not remove it.


================
Comment at: lld/ELF/Writer.cpp:657
+    for (Symbol *b : f->getLocalSymbols())
+      b->used = false;
+    for (InputSectionBase *s : f->getSections()) {
----------------
`Symbol::used` is initialized to something in the constructor of the `Symbol`:

```
  Symbol(Kind k, InputFile *file, StringRefZ name, uint8_t binding,
         uint8_t stOther, uint8_t type)
      :  .... used(!config->gcSections), ....
```

LLD usually tries to initialize variables to a proper value from the begining.
Given all above I think more consistent would to change the initializer condition right there.
Then you should be able to remove this loop.


================
Comment at: lld/test/ELF/discard-locals-preserve-used.s:5
+
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 -save-temp-labels %s -o %t
----------------
Usually `REQURES` is placed on the first line.


================
Comment at: lld/test/ELF/discard-locals-preserve-used.s:9
+# RUN: ld.lld --discard-none -shared --emit-relocs %t -o - | \
+# RUN:   llvm-nm - | FileCheck %s --check-prefix=DISCARD_NONE
+
----------------
`DISCARD_NONE` -> `DISCARD-NONE` here and below.
(We don't use underbars in check prefixes in LLD tests)


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

https://reviews.llvm.org/D77807