[PATCH] D40026: [ELF] - Allow applying SHF_MERGE optimization for relocatable output.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/InputSection.cpp:414
       if (Config->IsRela) {
-        P->r_addend += Sym.getVA() - Section->getOutputSection()->Addr;
+        int64_t Addend = getAddend<ELFT>(Rel);
+        P->r_addend = Sym.getVA(Addend) - Section->getOutputSection()->Addr;
----------------
grimar wrote:
> ruiu wrote:
> > Do you need this temporary variable?
> No, but then code will be 2 lines anyways because of clang-format, IMO looks better with it.
I don't think so. Can you inline?


================
Comment at: ELF/Writer.cpp:494
+    // which is used for -r.
+    if (isa<SyntheticSection>(IS) && !(IS->Flags & SHF_MERGE))
+      continue;
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > I wonder if you really need to handle this as a special case.
> > > Do you mean we can create symbols for all synthetic sections instead ? I think that is at least confusing to see in output and excessive.
> > Well, I mean I actually do not understand what this code is for in the first place.
> It is for this place:
> https://github.com/llvm-mirror/lld/blob/master/ELF/SyntheticSections.cpp#L1529
> 
> When we rewrite relocations we updating info field and want to set proper section index.
> Before this patch we had regular SHF_MERGE sections in output, so symbols were created for them.
> But now them are synthetic and there is no symbol, so we emit wrong zero index and relocation is broken.
> 
> BTW, I think that affects regular output + --emit-relocs as well. I am going to check soon and will split that change to different patch if so.
This is probably okay, but let me think about it, as it still feels somewhat ad-hoc. There might be some simpler way.


https://reviews.llvm.org/D40026