[PATCH] D102296: [ELF] getRelocatedSection: remove the check for ET_REL object file

[Amir Ayupov via Phabricator via llvm-commits]

Amir added a comment.

@MaskRay: agree with the title wording change.

Note that it's a second attempt to make getRelocatedSection work regardless of object file type. The previous attempt was reverted in commit 9219fe79b9842; it's the one that re-introduced the check we want to remove:

  commit 9219fe79b9842f3be3524c2e9aeb807ddffdfe3e                                                                                                                 
  Author: Rafael Espindola <rafael.espindola at gmail.com>
  Date:   Mon Mar 21 20:59:15 2016 +0000
  
      Revert "[llvm-objdump] Printing relocations in executable and shared object files.  This partially reverts r215844 by removing test objdump-reloc-shared.test which s
  tated GNU objdump doesn't print relocations, it does."
  
      This reverts commit r263971.
      It produces the wrong results for .rela.dyn. I will add a test.
  
      llvm-svn: 263987
  
  ...
  
  diff --git a/llvm/include/llvm/Object/ELFObjectFile.h b/llvm/include/llvm/Object/ELFObjectFile.h
  index 5ee90ee9ea71..b01fa1da4b3e 100644
  --- a/llvm/include/llvm/Object/ELFObjectFile.h
  +++ b/llvm/include/llvm/Object/ELFObjectFile.h
  @@ -640,6 +640,9 @@ ELFObjectFile<ELFT>::section_rel_end(DataRefImpl Sec) const {
   template <class ELFT>
   section_iterator
   ELFObjectFile<ELFT>::getRelocatedSection(DataRefImpl Sec) const {
  +  if (EF.getHeader()->e_type != ELF::ET_REL)
  +    return section_end();
  +

Regarding tests: none of the tests in LLVM and subprojects have executable files with relocations, so all tests are passing. But it's expected that this change may break downstream tests, e.g. for llvm-dwarfdump. 
llvm-dwarfdump is relying on getRelocatedSection() to return section_end() for ELF files of types other than relocatable objects. We've changed the function to return relocatable section for other types of ELF files. As a result, llvm-dwarfdump started re-processing relocations for sections that already had relocations applied, e.g. in executable files, and this resulted in wrong values reported.
As a workaround/solution, we can make this function return relocated section for executable (and any non-relocatable objects) files only if the section is allocatable:

    Expected<const Elf_Shdr *> SecOrErr = EF.getSection(EShdr->sh_info);
    if (!SecOrErr)
      return SecOrErr.takeError();
  +  if (EF.getHeader()->e_type != ELF::ET_REL &&
  +      !((*R)->sh_flags & ELF::SHF_ALLOC))
  +    return section_end();
    return section_iterator(SectionRef(toDRI(*SecOrErr), this));
  }

Another potential solution would be to introduce a flag (e.g. CheckFileRelocatable) for getRelocatedSection or another function with intended behavior.

There are options for adding the test, too:

1. As a unit test for getRelocatedSection that would check that this function works for executable files with relocations.
2. As a test in lld folder that would run llvm-mc; ld.lld with --emit-relocs and then a client of getRelocatedSection (`llvm-objdump -dr` or something similar). Cons: test is unrelated to lld.
3. As a prepared ELF binary test for LLVM clients of getRelocatedSection.

What do you think would be an appropriate solution?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102296