[PATCH] D78389: [ELF] Keep local symbols when both --emit-relocs and --discard-all are specified

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: lld/ELF/Writer.cpp:649-650
   // copied because they may be referenced by relocations.
   if (config->emitRelocs)
     return true;
 
----------------
MaskRay wrote:
> ikudrin wrote:
> > MaskRay wrote:
> > > ikudrin wrote:
> > > > For the purpose of this fix, it would be simpler to extend the condition at the beginning of `getDiscard()` in `Driver.cpp` and remove this condition. The other changes are not required.
> > > Do you mean
> > > ```
> > > -  if (args.hasArg(OPT_relocatable))
> > > + if (args.hasArg(OPT_relocatable) || config->emitRelocs)
> > >     return DiscardPolicy::None;
> > > ```
> > > 
> > > ? That would increase the amount of differences for D77807.
> > Yes, that is true, but I believe that every patch should solve its issue in the right way and the change in `Driver.cpp` looks like that. We emit relocations in two cases, `-r` and `--emit-relocs`, and both of them should be handled similarly.
> I have a different opinion, though. I prefer the current version despite the -r --emit-relocs similarity.
> 
> ```
> -  if (config->discard != DiscardPolicy::All)
>     copyLocalSymbols();
> ```
> 
> This removes the logic from the caller and makes the intricate logic local to the body of copyLocalSymbols.
My main concern is the unjustified difference in handling the `-r` and `--emit-relocs` cases. What about removing the condition in `getDiscard()` in `Driver.cpp` and changing `config->emitRelocs` to `config->copyRelocs` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78389