[PATCH] D101116: [ELF] Support .rela.eh_frame with unordered r_offset values

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lld/ELF/InputSection.cpp:1329
 void EhInputSection::split(ArrayRef<RelTy> rels) {
+  // getReloc expects the relocations to be sorted by r_offset. See the comment
+  // in scanRelocs.
----------------
MaskRay wrote:
> jhenderson wrote:
> > MaskRay wrote:
> > > It is unfortunate that there are two pieces of code.
> > > `RelTy` is parametric so we cannot add a member in `EhInputSection` caching the input relocations.
> > > 
> > I might be missing something, but surely you could have some templated function that would allow you to avoid the code duplication at least?
> Not clear the trade-off is worthwhile. The template function needs to live in one header (our usage are in two .cpp files). On the other hand, there are only few lines and allocating a SmallVector and `rels = sorted;` cannot be dropped with a common function.
The assignment can be folded in with the function call easily enough, I think. The advantage is that we don't have two places doing the same thing, potentially resulting in the code diverging in future patches. It also allows for future code reuse, and it seems not implausible we'll need this sorting elsewhere in the future.

```
template <typename RelTy>
ArrayRef<RelTy> sortRels(ArrayRef<RelTy> rels, ArrayRef<RelTy> storage) {
  auto cmp = [](const RelTy &a, const RelTy &b) {
    return a.r_offset < b.r_offset;
  };
  if (!llvm::is_sorted(rels, cmp)) {
    sorted.assign(rels.begin(), rels.end());
    llvm::stable_sort(storage, cmp);
    rels = sorted;
  }
  return rels;
}

template <class ELFT, class RelTy>
void EhInputSection::split(ArrayRef<RelTy> rels) {
  SmallVector<RelTy, 0> sorted;
  rels = sortRels(rels, sorted);
  ...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101116