[PATCH] D54759: [LLD][ELF] Use more specific method to calculate DT_PLTRELSZ

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

It feels like some corner case. Though it still a bug we could fix.
I have a few suggestions/questions below.



================
Comment at: ELF/SyntheticSections.cpp:1255
+// section. When this occurs we cannot just use the OutputSection Size.
+// Moreover the [DT_JMPTREL, DT_JMPREL + DT_PLTRELSZ) is permitted to overlap
+// with the [DT_RELA, DT_RELA + DT_RELASZ).
----------------
`DT_JMPTREL` -> `DT_JMPREL`


================
Comment at: ELF/SyntheticSections.cpp:1267
+             In.RelaIplt->getParent() == In.RelaPlt->getParent() &&
+             In.RelaIplt->OutSecOff == In.RelaPlt->OutSecOff + Size)
+           Size += In.RelaIplt->getSize();
----------------
I truncated this to

```
  Entries.push_back({DT_PLTRELSZ, [=] {
                       size_t Size = In.RelaPlt->getSize();
                       if (In.RelaIplt->getParent() == In.RelaPlt->getParent())
                         Size += In.RelaIplt->getSize();
                       return Size;
                     }});
```

And all your tests pass.
That will break the case when `!Config->AndroidPackDynReloc` (so, `In.RelaIplt` is placed to `.rel.dyn`),
but do we really need to support it? I guess such script is a rare case and perhaps not worth to support all corner cases? I am not sure.


================
Comment at: ELF/SyntheticSections.cpp:1359
     addInSec(DT_JMPREL, In.RelaPlt);
-    addSize(DT_PLTRELSZ, In.RelaPlt->getParent());
+    addPltRelSize();
     switch (Config->EMachine) {
----------------
So I suggest to inline `addPltRelSize` here and just update the existent comment above.


================
Comment at: ELF/SyntheticSections.h:465
   void addSize(int32_t Tag, OutputSection *Sec);
+  void addPltRelSize();
   void addSym(int32_t Tag, Symbol *Sym);
----------------
Perhaps I would not create the additional method, because it is very specific.
I think it can be inlined, see how `DT_PPC64_GLINK` is handled for example.


================
Comment at: test/ELF/arm-combined-dynrel-ifunc.s:12
+
+// See comment in aarch64-combined-dynrel.s for an explanation of why we
+// are checking that the DT_PLTRELSZ only accounts for the PLT relocs.
----------------
I would just include the comment here instead of referencing it. No need to save space so hard in test cases I think:)
The same applies to another test.


https://reviews.llvm.org/D54759