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

[Alexey Lapshin via Phabricator via llvm-commits]

avl marked an inline comment as done.
avl 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)
----------------
grimar wrote:
> 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.
I see. I also did not find any reference to documentation which mentioned that relocations must be sorted(nor ELF file specification, nor DWARF specification). 


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

https://reviews.llvm.org/D74169