[PATCH] D59553: [LLD][ELF][DebugInfo] llvm-symbolizer shows incorrect source line info if --gc-sections used
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 21 02:50:23 PDT 2019
grimar added inline comments.
================
Comment at: lld/ELF/InputSection.cpp:785
void InputSection::relocateNonAlloc(uint8_t *Buf, ArrayRef<RelTy> Rels) {
+ assert(!Config->Relocatable);
+
----------------
avl wrote:
> avl wrote:
> > grimar wrote:
> > > avl wrote:
> > > > ruiu wrote:
> > > > > What does this assert mean? If you pass `-r` option to the linker, this function is not indeed called, but that's also true to many other options. Why is this option special?
> > > > >
> > > > That UINT64_MAX thing should be done for only !Config->Relocatable case.
> > > > So I added this assertion to be sure that routine could not be called for other cases.
> > > > If it does not fit with the design - would it be better to move that assertion closer to
> > > > Target->relocateOne(BufLoc, Type, SignExtend64<Bits>(UINT64_MAX - 1)); ?
> > > We do not need this assert at all. -r output must not trigger relocations resolving.
> > > It contradicts with its nature.
> > >
> > > If that routine would be called for some "other cases" with -r, LLD would be just broken :)
> > so if that routine could be called for some "other cases" with -r then "!Config->Relocatable" should be checked later, before
> >
> > Target->relocateOne(BufLoc, Type, SignExtend64<Bits>(UINT64_MAX - 1));
> >
> > , right ?
> >If that routine would be called for some "other cases" with -r, LLD would be just broken :)
>
> I misunderstood a bit that sentence. so you are saying that LLD would be just broken if we would be in this routine and -r specified. Then assertion looks to be correctly placed - it will signal that LLD is broken.
>
> But I see that LLD does not like to have that kind of assertions. I would remove it.
We have the following code:
```
auto *Sec = cast<InputSection>(this);
if (Config->Relocatable)
relocateNonAllocForRelocatable(Sec, Buf);
else if (Sec->AreRelocsRela)
Sec->relocateNonAlloc<ELFT>(Buf, Sec->template relas<ELFT>());
else
Sec->relocateNonAlloc<ELFT>(Buf, Sec->template rels<ELFT>());
```
This is the only place where LLD calls `relocateNonAlloc`. So you do not need this assert.
It is just excessive to check the conditions that are already tested at the upper-level explicitly.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59553/new/
https://reviews.llvm.org/D59553
More information about the llvm-commits
mailing list