[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:
> 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). 
I found it was answered actually:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170731/474875.html
Then it is OK I think.


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

https://reviews.llvm.org/D74169