[PATCH] D77807: [LLD][ELF] Implement --discard-* for cases when -r or --emit-relocs are used.
Igor Kudrin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 10 05:21:43 PDT 2020
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
More information about the llvm-commits
mailing list