[PATCH] D125036: [RISCV] Alignment relaxation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Using just one buffer with the copy_backward usage is unsafe.

Consider a case when all `AdjustRange::delta` values are positive (inflating), I feel that one iteration may overwrite some bytes which will be used later.

Currently the content move is done in `InputSectionBase::adjustRanges` `MutableArrayRef<uint8_t> buf = this->mutableData();`. So every relax iteration shuffles the content.
I think a better way is to do the shuffle only once, after the relaxation finishes.



================
Comment at: lld/ELF/InputSection.cpp:164
+
+void lld::elf::fillSectionSymbolAddrs(SectionSymbolAddrs &sectionSymbolAddrs) {
+  // Collect the input files that contain the code sections.
----------------



================
Comment at: lld/ELF/InputSection.cpp:183
+  for (auto &kv : sectionSymbolAddrs)
+    llvm::sort(kv.second, [](const SymbolAddr &a, const SymbolAddr &b) {
+      return a.offset < b.offset;
----------------
stable_sort


================
Comment at: lld/ELF/InputSection.cpp:197
+                                    SymbolAddrs &symbolAddrs) {
+  if (ranges.empty())
+    return false;
----------------
Delete since the caller has checked emptiness.


================
Comment at: lld/ELF/InputSection.cpp:209
+    for (; i < ranges.size() && ranges[i].offset < sa.offset; i++) {
+      // An AdjustRange should not span a symbol start/end offset
+      assert(!ranges[i].contains(sa.offset));
----------------
The invariant on `ranges` is checked by `symbolAddrs`. Ranges after the end of `symbolAddrs` is not tested.
I'd remove all `assert` in the for loop.

```
    if (i > 0)
      // An AdjustRange should not span a symbol start/end offset.
      assert(!ranges[i - 1].contains(sa.offset));
```
below is sufficient.

If there is a need to check

> are all disjoint, i.e., do not overlap; and ...

Use a separate loop which calls `fatal` and do this after the `ranges` object is constructed.


================
Comment at: lld/ELF/InputSection.cpp:234
+  uint8_t *dest = buf.begin() + ranges.begin()->offset;
+  for (size_t i = 0; i < ranges.size(); i++) {
+    const AdjustRange &ar = ranges[i];
----------------



================
Comment at: lld/ELF/InputSection.cpp:245
+    else
+      std::copy_backward(src0, srcN, dest + length);
+    dest += length;
----------------
Using just one buffer with the copy_backward usage is unsafe.

Consider a case when all `AdjustRange::delta` values are positive (inflating), I feel that one iteration may overwrite some bytes which will be used later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125036