[PATCH] D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local

Mon Jan 6 14:26:00 PST 2020

dblaikie added a comment.

In D72197#1806523 <https://reviews.llvm.org/D72197#1806523>, @MaskRay wrote:

> In D72197#1806495 <https://reviews.llvm.org/D72197#1806495>, @dblaikie wrote:
>
> > I think the question I have: What is the value (I don't mean to assume there is no value, but I want to understand the specific use case) of supporting this if the full generality of symbol interposition is not supported? (is there a well defined use case that is usable with this patch and doesn't need the full generality? Or is the use case going to be subtly broken if it changes even slighlty and happens to trip over another part of LLVM that doesn't support symbol interposition? (in which case this is a great patch, but only as part of an effort to fully support the general case))
>
>
> @dblaikie I came from a ppc64 use case https://reviews.llvm.org/D71639#1803561 where I could not construct a good test due to the MC issue this patch intends to fix. The advantages include at least the following:
>
> - Improve compatibility with GNU as. This improves portability of hand-written assembly.

That's fair - though I'd still wonder if this is a consistently applied idea, that llvm's assembler does support symbol interposition, or is this one of many ways in which it assumes that is not going to happen? (in which case fixing one bug without a plan to test & fix the full surface area seems like not a good idea to me)

> - Resolve a program behavior difference (-ffunction-sections vs w/o). See @peter.smith's comment above. The new behavior is consistent with the -ffunction-sections case before.

This seems like a red herring/not relevant & more reflective of LLVM's more general lack of support for function interposition - if functions can't be interposed then it makes sense not to use a relocation between two functions in the same section, I think?

> - Make MC simpler (it deleted `isWeak`)

I'm not too concerned about the code complexity (I haven't looked at the implementation of the patch at all) - only wondering about the semantic choices.

> Lack of `-fsemantic-interposition` is a larger issue that we only have a partial solution currently. Many aspects of that issue is irrelevant to this patch but I believe this patch fixes the assembler side issue any efforts on that feature will face.

I'm concerned about partial solutions that might make someone start depending on this in ways that aren't actually supported.

But if this is a full solution to the issue of hand-written assembly code, that seems like a coherent specific supported/supportable feature without needing to address the broader issue.

In D72197#1806540 <https://reviews.llvm.org/D72197#1806540>, @jyknight wrote:

> In D72197#1806495 <https://reviews.llvm.org/D72197#1806495>, @dblaikie wrote:
>
> > I think the question I have: What is the value (I don't mean to assume there is no value, but I want to understand the specific use case) of supporting this if the full generality of symbol interposition is not supported? (is there a well defined use case that is usable with this patch and doesn't need the full generality? Or is the use case going to be subtly broken if it changes even slighlty and happens to trip over another part of LLVM that doesn't support symbol interposition? (in which case this is a great patch, but only as part of an effort to fully support the general case))
>
>
> IMO it's useful to differentiate the high-level desires, vs the low-level implementation details. Whatever clang/LLVM-IR wants its policy to be for semantic interposition, that choice needs to be represented somehow in assembly.
>
> And it seems a reasonable choice (especially given gnu as compatibility) that on ELF platforms, referencing a global symbol in assembly should always use a relocation, as this patch implements. If we want to have the other behavior in some case, we can emit a local symbol alias for a function, and call via that, instead.
>
> I hope the jmp difference doesn't cause an issue -- I think it makes more sense as you have, but that kind of thing always worries me. :)

Yeah, fair enough - if no one knows of other major outstanding bugs/unimplemented functionality needed to support symbol interposition at the assembly level, I'm good with this patch/direction. [if there are other major outstanding things - then I'd worry about this bug being fixed in isolation & letting some project limp along expecting full support where it doesn't actually exist/where there are traps]

I'm bowing out for now then - thanks for explaining & sorry for the noise.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72197