[PATCH] D125036: [RISCV] Alignment relaxation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

On lld/ELF/Relocations.cpp:1439, R_RELAX_HINT should not trigger `sym.hasDirectReloc = true;`, otherwise an ifunc may unnecessarily be converted to a canonical PLT. You may read `*-ifunc-nonpreemptible*.s` tests for what they do. I have some notes on https://maskray.me/blog/2021-01-18-gnu-indirect-function#address-significance



================
Comment at: lld/ELF/Arch/RISCV.cpp:40
                 uint64_t val) const override;
+  void finalizeSections() const override;
 };
----------------
The name is unnecessarily the same as `Writer::finalizeSections`. `relaxSections` may be a better name.


================
Comment at: lld/ELF/Arch/RISCV.cpp:488
+constexpr int alignNopBytesWidth = 16;
+constexpr int alignNopBytesMask = ((1 << alignNopBytesWidth) - 1);
+
----------------



================
Comment at: lld/ELF/Arch/RISCV.cpp:520
+  r.addend = (r.addend & ~alignNopBytesMask) | newNopBytes;
+  int incr = newNopBytes - oldNopBytes;
+  if (incr) {
----------------



================
Comment at: lld/ELF/Arch/RISCV.cpp:522
+  if (incr) {
+    adjustRanges.push_back({r.offset, incr});
+    delta += incr;
----------------
emplace_back


================
Comment at: lld/ELF/Arch/RISCV.cpp:573
+
+      AdjustRanges adjustRanges;
+      int64_t delta = 0;
----------------
The variable and the member function have the same name. Rename the variable to `ranges`?

Move it outside the outer loop and use a `ranges.clear()` here. Try avoiding constructing/destructing a vector in a loop.


================
Comment at: lld/ELF/Arch/RISCV.cpp:576
+      for (Relocation &r : isec->relocations)
+        if (r.type == R_RISCV_ALIGN && r.addend)
+          relaxAlign(isec, r, delta, adjustRanges);
----------------
r.addend is guaranteed to be non-zero


================
Comment at: lld/ELF/Arch/RISCV.cpp:591
+void RISCV::finalizeSections() const {
+  // Can't perform relaxation if it is not a final link.
+  if (config->relocatable)
----------------
Don't perform relaxation for a relocatable link.


================
Comment at: lld/ELF/InputSection.cpp:156
+// we can inspect and adjust individually.
+class SymbolAddr {
+public:
----------------
struct


================
Comment at: lld/ELF/InputSection.cpp:188
+  SmallVector<SymbolAddr, 0> symbolAddrs;
+  for (Symbol *sym : file->getSymbols())
+    if (Defined *d = dyn_cast<Defined>(sym))
----------------
If a file compiled with -ffunction-sections has N text sections/function symbols, this takes O(N^2). I think we should build a map to decrease the time complexity. You can add a map to `InputFile`. It's unfortunate to increase the size of `InputFile` which it is acceptable.


================
Comment at: lld/ELF/InputSection.cpp:191
+      if (d->section == this) {
+        symbolAddrs.push_back({d->value, d});
+        symbolAddrs.push_back({d->value + d->size, d});
----------------
emplace_back


================
Comment at: lld/ELF/InputSection.cpp:194
+      }
+  llvm::sort(symbolAddrs, [](const SymbolAddr &a, const SymbolAddr &b) {
+    return a.offset < b.offset;
----------------
stable_sort

Two symbols may be defined at the same offset. 


================
Comment at: lld/ELF/InputSection.cpp:223
+  const auto *ar = ranges.begin();
+  for (auto &r : relocations) {
+    for (; ar != ranges.end() && ar->offset < r.offset; ++ar)
----------------



================
Comment at: lld/ELF/InputSection.cpp:238
+    size_t length = srcN - src0;
+    if (dest < src0)
+      std::copy_backward(src0, srcN, dest + length);
----------------
The condition isn't correct. It may trigger the undefined behavior src0 < dest+length < srcN.

---

We should avoid `std::copy` when

> The behavior is undefined if d_first is within the range [first, last)

Otherwise we should use `std::copy`.

---

I am not sure the code works with adding bytes. In that case we need to get the NOP pattern.


================
Comment at: lld/ELF/InputSection.h:170
 
+  MutableArrayRef<uint8_t> mutableData() const {
+    if (!copiedData) {
----------------
Then remove `mutable` from `copiedData`


================
Comment at: lld/ELF/InputSection.h:172
+    if (!copiedData) {
+      size_t size = data().size();
+      uint8_t *mutData = context().bAlloc.Allocate<uint8_t>(size);
----------------



================
Comment at: lld/test/ELF/riscv-relax-align-rvc.s:38
+  c.add a0, a1
+.balign 16
+  c.add s0, s1
----------------
The test needs many `.balign {8,16}` directives to test `InputSectionBase::adjustRanges`.

The current `InputSectionBase::adjustRanges` is flawed but the tests aren't able to detect that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125036