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

[Amir Ayupov via Phabricator via llvm-commits]

Amir added a comment.

In D102296#2787857 <https://reviews.llvm.org/D102296#2787857>, @dblaikie wrote:

> Sorry to jump in here, hope I'm not derailing a perfectly functional review.
>
> Could you provide some more context in the patch description? Might be worth knowing what that ET_REL check was originally implemented for? (could you check the patch history of why that was added)
>
> So this getRelocatedSection is meant to retrieve the section that the specified REL or RELA section applies to? But if the ELF file is already relocated (ie: e_type != ET_REL) then the REL/RELA section sort of doesn't apply to the section - no relocations need to be applied to that section, right? But maybe it's the wrong abstraction/interface design? (or callers should be using the interface in some different way) - this REL or RELA section is for whatever section its for, but that section has already had the relocations applied?
>
> I worry that with only this change the callers might be getting the wrong idea - the idea that the relocations do need to be applied, when in fact they don't, because the file isn't relocatable? (previously/witohut this patch they were relying on this function not returning the relocated section to indicate that no relocations needed to be applied)

The check was added in the initial implementation of getRelocatedSection https://github.com/llvm/llvm-project/commit/4f60a38f18ee68c4abd4cfefb5a4bba0f0917f16.
So your reasoning is most likely correct, that the original intent was to skip non-relocatable files and return section_end().

But we believe that this interface should just return the relocated section and not have this implicit check. As a side effect, this can make `llvm-objdump -d -r` work the same as `objdump -dr` which is helpful at times. If you believe that LLVM should rather keep the current interface, we can make the change on the BOLT side.

> re:
>
>> But it's expected that this change may break downstream tests, e.g. for llvm-dwarfdump.
>
> Then there should be an llvm-dwarfdump test that demonstrates this fix changes behavior. That can include a linked object file that still has relocations - if such a file can't be produced by LLVM tools (well, a linked file can't be produced by LLVM itself - needs a linker like lld, and LLVM tests can't depend on lld) - then a checked in binary can be used for testing.
>
> Or a libObject unit test might be an option too. Though I personally wouldn't mind seeing how this affects some tool, rather than only a narrow unit test.

The test that broke in llvm-dwarfdump is this one: https://reviews.llvm.org/harbormaster/unit/view/755844/. Looking into how exactly did this change affect XCOFF test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102296