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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: lld/ELF/Symbols.h:241
         isInIplt(false), gotInIgot(false), isPreemptible(false),
-        used(!config->gcSections), needsTocRestore(false),
-        scriptDefined(false) {}
+        used(!config->gcSections && binding != llvm::ELF::STB_LOCAL),
+        needsTocRestore(false), scriptDefined(false) {}
----------------
grimar wrote:
> So, when `!gcSections` and the symbol is local, it bow sets `used` to false,
> though the previous code set it to `true` in this case.
> 
> Seems this doesn't break anything in the current implementation,
> though I wonder if we might want to have `config->copyRelocs` involved in the condition.
> It perhaps would be a bit cleaner way to work with this flag.
> 
> Also, perhaps it can be initialized in the constructor body now, since the condition is not trivial.
> 
> nit: You can probably use `isLocal()` instead of checking the `binding` field directly.
> 
> 
> Seems this doesn't break anything in the current implementation, though I wonder if we might want to have `config->copyRelocs` involved in the condition. It perhaps would be a bit cleaner way to work with this flag.

If we add `config->copyRelocs` we will also have to add `config->discard` and the expression will easily become overcomplicated. Long complex expressions are usually hard to understand, so I would prefer this to be as short and simple as possible.

> Also, perhaps it can be initialized in the constructor body now, since the condition is not trivial.

As `used` is a bit field, moving the initialization into the body might confuse the optimizer.

> nit: You can probably use `isLocal()` instead of checking the `binding` field directly.

Using a method that depends on the state of a class in the member initializer list makes the constructor fragile. Better to avoid.


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

https://reviews.llvm.org/D77807