[PATCH] D74169: [WIP][LLD][ELF][DebugInfo] Remove obsolete debug info.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/DWARF.cpp:166
+  // skip relocations, started from nextReloc until relocPos < startPos.
+  uint64_t relocPos = rels[nextReloc].r_offset;
+  while (relocPos < startPos && nextReloc < rels.size() - 1)
----------------
avl wrote:
> avl wrote:
> > grimar wrote:
> > > Seems this assumes that relocations are sorted by r_offset? This is probably a wrong assumpting.
> > > We have the code, for example, which uses the same thing (and perhaps could be reused somehow, not sure):
> > > 
> > > ```
> > > // Returns the index of the first relocation that points to a region between
> > > // Begin and Begin+Size.
> > > template <class IntTy, class RelTy>
> > > static unsigned getReloc(IntTy begin, IntTy size, const ArrayRef<RelTy> &rels,
> > >                          unsigned &relocI) {
> > >   // Start search from RelocI for fast access. That works because the
> > >   // relocations are sorted in .eh_frame.
> > >   for (unsigned n = rels.size(); relocI < n; ++relocI) {
> > >     const RelTy &rel = rels[relocI];
> > >     if (rel.r_offset < begin)
> > >       continue;
> > > ```
> > > 
> > > But it says that relocations are sorted in .eh_frame and I believe it is true,
> > > but generally there is no requirement in ELF specification about that relocations must be sorted by offset I think.
> > right. It assumes that relocations are sorted by r_offset.
> > It is sorted in scanRelocs(). But I missed that it is sorted not for all types of sections.
> > Would fix it , Thanks!
> @grimar @ruiu  It looks like it was considered safe to rely on sorted order of relocations - D36079. What do you think ? 
I do not know. ELF specification does not says they must be. Perhaps we can at least check this with `std::is_sorted`.
I see lld/Wasm does it:

```
  ArrayRef<WasmRelocation> relocs = section->Relocations;
  assert(std::is_sorted(relocs.begin(), relocs.end(),
                        [](const WasmRelocation &r1, const WasmRelocation &r2) {
                          return r1.Offset < r2.Offset;
                        }));
```

`assert` will not help in release build though.
Perhaps other people can comment on this, as you see my concern about D36079 was not answered that time it seems.


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

https://reviews.llvm.org/D74169