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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/Writer.cpp:649-650
   // copied because they may be referenced by relocations.
   if (config->emitRelocs)
     return true;
 
----------------
ikudrin wrote:
> 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?
I think I am slightly in favor of keeping it here, because the argument about hiding "the intricate logic" probably makes sense to me.


================
Comment at: lld/ELF/Writer.cpp:653
+  if (config->discard == DiscardPolicy::All)
+    return false;
+
----------------
Can this be moved closer to the first `config->discard` check to group them?

```
  if (config->discard == DiscardPolicy::None)
    return true;
```

(Seems `if (config->discard == DiscardPolicy::None)` can be moved here).


================
Comment at: lld/test/ELF/emit-relocs-discard-locals.s:2
+# REQUIRES: x86
+## Test that --emit-relocs keeps local symbols and overrids --discard-{locals,all}.
+
----------------
ikudrin wrote:
> overrid__e__s?
`overrides`


================
Comment at: lld/test/ELF/emit-relocs-discard-locals.s:7
+# RUN: ld.lld --emit-relocs --discard-locals %t.o -o %tlocal
+# RUN: llvm-nm %tlocal | FileCheck --check-prefixes=NM,NM_NOGC %s
+# RUN: llvm-readobj -r %tlocal | FileCheck --check-prefix=REL %s
----------------
`NM_NOGC` -> `NM-NOGC` (we don't use underbars in tests I think)


================
Comment at: lld/test/ELF/emit-relocs-discard-locals.s:9
+# RUN: llvm-readobj -r %tlocal | FileCheck --check-prefix=REL %s
+# RUN: ld.lld --emit-relocs --discard-locals --gc-sections %t.o -o %tlocal.gc
+# RUN: llvm-nm %tlocal.gc | FileCheck --check-prefix=NM %s
----------------
Why it is important to test `--gc-sections`/`--no-gc-sections` too?


================
Comment at: lld/test/ELF/emit-relocs-discard-locals.s:17
+
+## -r keeps symbols and relocations similar to --emit-relocs.
+# RUN: ld.lld -r --discard-locals %t.o -o %tlocal.ro
----------------
The `emit-relocs-discard-locals.s` test case name reflects the `--emit-relocs`, but not `-r`.
I think this test could be splitted into two perhaps. Or renamed properly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78389