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

Wed Jun 2 16:39:27 PDT 2021

dblaikie added a comment.

In D102296#2794777 <https://reviews.llvm.org/D102296#2794777>, @Amir wrote:

> 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)

Ah, I see that the change to test `isRelocatableObject` in DWARFContext is compensating for this ^ issue. So any other callers would need to make similar adjustments if they are assuming that an "relocated section" is meant to have relocations applied to it.

Might be a bit subtle - maybe there's a chance renaming the function would be suitable if there's a name that might better capture that "this is the section that these relocations are related to, but they might've already been applied" - or I guess there could be a parameter to the function, or two different functions, etc, to better differentiate the situations.

but if it's just the one DWARF fix... I'm not /super/ fussed about it.

> 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.

I'll keep an eye out to see what you come up with there.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102296