[PATCH] D115993: [ELF] Optimize RelocationSection<ELFT>::writeTo

Tue Dec 21 09:46:28 PST 2021

arichardson added a comment.

This looks like a nice improvement to me. I also like @ikudrin's suggestion of validating internal consistency in debug builds, but if this has a measurable performance impact the current patch LGTM.

================
Comment at: lld/ELF/SyntheticSections.cpp:1668
+  addend = computeAddend();
+  kind = AddendOnly; // Catch errors
 }
----------------
MaskRay wrote:
> ikudrin wrote:
> > And now we have an inconsistency if `kind` was `AddendOnlyWithTargetVA` or `AgainstSymbolWithTargetVA` because `sym` is not `nullptr` and an `assert()` in `computeAddend()` can trigger. Maybe add a new kind `PostComputeRaw` and add `assert`s to `computeAddend()` and `needsDynSymIndex()` to ensure that they are not called after `computeRaw()`?
> Every redundant operation costs. We have comprehensive tests so I do not worry much about inconsistency caused problems. Adding `PostComputeRaw` seems to sacrifice too much for the check.
Not sure if that makes any difference in a release build since we already have the store in computeRaw().
One thing that might also make it more resilient to bugs introduced while refactoring (or to help modified downstream consumers with less comprehensive test coverage) would be to keep the `addend` field private and add a `r_addend()` accessor that asserts that we are in the `PostComputeRaw`/`ContentsFinalized`/`SomeOtherName` state.

================
Comment at: lld/ELF/SyntheticSections.cpp:1775
     Elf_Rela r;
-    encodeDynamicReloc<ELFT>(getPartition().dynSymTab, &r, rel);
+    r.r_offset = rel.getOffset();
+    r.setSymbolAndType(rel.getSymIndex(getPartition().dynSymTab), rel.type,
----------------
Does it make sense to also parallelize this or is this not meaningful for the relocation counts encountered in practise?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115993