[PATCH] D38554: Fixed ppc32 function relocations in non-pic mode

[vit9696 via Phabricator via llvm-commits]

vit9696 added a comment.

Right, I could finally understand your opinion and perhaps even the reasons behind it. However, I cannot agree with them due to the following:
— a working linker you describe is something subjective, in my opinion it is a linker capable of translating relocations defined by a standard or a specification, not necessarily all, and efficiency is not a necessity.
— the assembler indeed does not know what binding should be used, however, the extension to the compiler is incorrect. The compiler, which is responsible for emitting the relocations, is aware of the relocation model, and thus is expected to emit relocations suitable for this model. Since PLT is a PIC-specific feature, which is known to the compiler, the compiler should decide whether to generate PLT relocations or not.
— while you state that two relocation models is a historic artifact, LLVM currently implements a check whether to use PLT or not, so if this code is here, then it should work properly, otherwise it is a bug.
— your statement regarding more common GNU Power-PC ABIs being towards generating position independent may be correct, however, it has nothing to do with embedded ABI we are using here. Furthermore, if the "working" linker generates PLT relocations for a static model, it will simply crash a loader that has no support for PLT.

And to recite the former reasons already mentioned above:
— gcc does not generate PLT relocations in such case
— llvm until the aforementioned two commits did not either
— this change fixes lld compatibility
— the fix is one line long (!)

Taking all this into account the "complexity" as the only last reason you are talking about is not a valid cause to reject the proposed change, which brings actual benefit and no issues. If you think that it is unclear why a PIC check should exist in the PLT logic (although I expect one knowing what PLT is to also be aware of how PIC works), I am fine to edit the diff and include a small comment like "PLT relocations are only necessary for PIC model".

If not, then I ask this diff to be approved and committed upstream.


https://reviews.llvm.org/D38554