[PATCH] D29920: [ELF] Add test for relocations with section group

Tue Feb 14 02:35:02 PST 2017

grimar added a comment.

In https://reviews.llvm.org/D29920#675826, @ruiu wrote:

> I do not really understand the issue yet, so I don't know how to fix this, but the basic rule is that if a change introduced a crash bug, it should be reverted and then re-submitted with a fix.

That probably would be bad idea to revert it. That is the case when change revealed bug, but not introduced.
Bug is next just in case:

1. We have 2 object with SHT_GROUP sections foo. So that means "only one of the duplicate groups will be retained by the link-editor, and the members of the remaining groups will be discarded.".
2. r294469 made the next change. During copyRelocations, it changed calculated offset for relocation from

  P->r_offset = RelocatedSection->getOffset(Rel.r_offset);

to

  P->r_offset = RelocatedSection->OutSec->Addr + RelocatedSection->getOffset(Rel.r_offset);

3. And here is a problem RelocatedSection in the place of crash is InputSection<ELFT>::Discarded, because it is the member of one of duplicate groups which was discarded earlier by LLD in ObjectFile<ELFT>::initializeSections().

That way OurSec is null. Previous logic called getOffset of discarded section and did not crash. I believe that also was incorrect. Probably we should place asserts to check that methods of discarded sections are not called.

In https://reviews.llvm.org/D29920#675850, @ruiu wrote:

> George,
>
> Can you please take a look at this test and fix the issue? Petr tried to revert your change, but since other changes have been made on top of it, it couldn't be reverted cleanly, so we decided to not revert to avoid risk of regression. The crash bug is not super important but needs fixing. Thanks!

Fix is https://reviews.llvm.org/D29929. Looks caused by GNU as bug.

Repository:
  rL LLVM

https://reviews.llvm.org/D29920