[PATCH] D127581: [ELF] Relax R_RISCV_ALIGN

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/RISCV.cpp:554-562
+      // Restore original st_value for symbols relative to this section.
+      ArrayRef<SymbolAnchor> sa = makeArrayRef(aux.anchors);
+      int32_t delta = 0;
+      for (auto it : llvm::enumerate(sec->relocations)) {
+        for (; sa.size() && sa[0].offset <= it.value().offset; sa = sa.slice(1))
+          if (!sa[0].end)
+            sa[0].d->value += delta;
----------------
luismarques wrote:
> Can't we skip this for the first pass?
This code takes nearly no time. I don't think we should special case the first pass.


================
Comment at: lld/ELF/Arch/RISCV.cpp:610
+      // Tell assignAddresses that the size has changed.
+      sec->bytesDropped = delta;
+    }
----------------
kito-cheng wrote:
> I hit overflow here as @gkm concern, and the fixed by changing `bytesDropped` to `uint16_t` (yeah, I tested the uint8_t version), maybe we can put an `assert (delta <= numeric_limits<uint16_t>::max());` here to make sure this could catch earlier?
> 
> I saw there are assertions for `byteDropped` in other place, so I think that should be reasonable?
> ```
> [kitoc at xxxx llvm-project]$ grpe bytesDropped  * -R
> ...
> lld/ELF/InputSection.h:  uint8_t bytesDropped = 0;
> lld/ELF/InputSection.h:    assert(bytesDropped + num < 256);
> lld/ELF/InputSection.h:    bytesDropped += num;
> lld/ELF/InputSection.h:    assert(bytesDropped >= num);
> lld/ELF/InputSection.h:    bytesDropped -= num;
> ...
> ```
> 
> Gonna run second round of testing.
push_back/drop_back is a code problem of the basic block sections feature. I don't intend to touch the functions for this patch.
Changed RISCV.cpp:611 instead.


================
Comment at: lld/ELF/Arch/RISCV.cpp:593
+      // Tell assignAddresses that the size has changed.
+      sec->bytesDropped = delta;
+    }
----------------
gkm wrote:
> `InputSectionBase::bytesDropped` is merely `uint8_t`, and feels vulnerable to overflow. The comment on the decl says it is intended for basic-block sections, for which 8 bits is reasonable, but this new use, it might be inadequate. Perhaps `uint16_t` ?
Thanks!


================
Comment at: lld/ELF/InputSection.h:147
   void drop_back(unsigned num) {
     assert(bytesDropped + num < 256);
     bytesDropped += num;
----------------
gkm wrote:
> 
push_back/drop_back is a code problem of the basic block sections feature. I don't intend to touch the functions for this patch.
Changed RISCV.cpp:611 instead.


================
Comment at: lld/ELF/Writer.cpp:1637
+                                       : target->relaxOnce(tc.pass);
+    ++tc.pass;
 
----------------
peter.smith wrote:
> Although more source changes. Would it be cleaner to have the passes variable here, and pass it into createThunks as a parameter?
ThunkCreator::pass needs to be retained, otherwise `uint32_t pass` needs to be threading though most of its member functions. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127581