[PATCH] D81988: [ELF] Refactor ObjFile<ELFT>::initializeSymbols to keep the invariant: InputFile::symbols has non null entry

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D81988#2100226 <https://reviews.llvm.org/D81988#2100226>, @grimar wrote:

> In D81988#2098351 <https://reviews.llvm.org/D81988#2098351>, @MaskRay wrote:
>
> > In D81988#2097630 <https://reviews.llvm.org/D81988#2097630>, @grimar wrote:
> >
> > > In D81988#2097485 <https://reviews.llvm.org/D81988#2097485>, @jhenderson wrote:
> > >
> > > > would we be better off emitting some kind of warning or error about the out-of-order list earlier on?
> > >
> > >
> > > +1 to this question.
> >
> >
> > This should be a separate change. llvm-readobj -s should emit a warning as well (like GNU readelf).
>
>
> Shouldn't we just error out early?


Yes.

> We have a few more places where we have this code pattern:
> 
> 1. MapFile.cpp https://github.com/llvm/llvm-project/blob/master/lld/ELF/MapFile.cpp#L54
> 2. InputSectionBase::getEnclosingFunction: https://github.com/llvm/llvm-project/blob/master/lld/ELF/InputSection.cpp#L285
> 
>   That code also uses `dyn_cast<Defined>(b)` and makes me wondering why do you want to support this broken case instead of reporting an error much earlier? This ways seems we will not need to fix all other places.

The two functions are called in the relocation pass, which is very late and is not subject to the bug.
I do think we have a more serious issue which my original description failed to capture.
Updating...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81988