Page MenuHomePhabricator

[llvm][IR] Add dso_local_equivalent Constant
Needs ReviewPublic

Authored by leonardchan on Apr 1 2020, 3:35 PM.

Details

Summary

The dso_local_equivalent constant is a wrapper for functions that represents a value which is functionally equivalent to the global passed to this. That is, if this accepts a function, calling this constant should have the same effects as calling the function directly. This could be a direct reference to the function, the @plt modifier on X86/AArch64, a thunk, or anything that's equivalent to the resolved function as a call target.

When lowered, the returned address must have a constant offset at link time from some other symbol defined within the same binary. The address of this value is also insignificant. The name is leveraged from dso_local where use of a function or variable is resolved to a symbol in the same linkage unit.

In this patch:

  • Addition of dso_local_equivalent and handling it
  • Update Constant::needsRelocation() to strip constant inbound GEPs and take advantage of dso_local_equivalent for relative references
  • Update LLVM-C API

See RFC for more details.

Diff Detail

Event Timeline

leonardchan created this revision.Apr 1 2020, 3:35 PM
leonardchan retitled this revision from [IR] Strip constant inbound GEPs when checking Constant::needsRelocation() to [IR] Update Constant::needsRelocation() to strip constant inbound GEPs and PLT relocations.
leonardchan edited the summary of this revision. (Show Details)
leonardchan added reviewers: phosek, mcgrathr.
leonardchan retitled this revision from [IR] Update Constant::needsRelocation() to strip constant inbound GEPs and PLT relocations to [llvm][IR] Add a new Constant, PLTEntry, which may represent the PLT entry for a function.
leonardchan edited the summary of this revision. (Show Details)
arsenm added a subscriber: arsenm.Aug 18 2020, 12:27 PM

Was there an RFC for this?

Missing GlobalISel support

leonardchan planned changes to this revision.Aug 18 2020, 12:29 PM

Was there an RFC for this?

Missing GlobalISel support

There hasn't, but I can send one out. The idea of a new constant was from discussing with @pcc offline.

Missing RFC + langref changes.

llvm/include/llvm/IR/Constants.h
892

What's PLT entry?

leonardchan edited the summary of this revision. (Show Details)

Clean up some of the code.

Also sent out an RFC at http://lists.llvm.org/pipermail/llvm-dev/2020-August/144469.html.

Still need to add and test GlobalISel support.

leonardchan planned changes to this revision.Aug 20 2020, 11:35 AM
leonardchan retitled this revision from [llvm][IR] Add a new Constant, PLTEntry, which may represent the PLT entry for a function to [llvm][IR] Add a new Constant (UnnamedFunc).
leonardchan edited the summary of this revision. (Show Details)

Update to reflect comments in the RFC.

Rather than "equivalent to the resolved function at runtime", I'd say "equivalent to the resolved function as a call target". It's not equivalent to the function at runtime when the runtime use is "ptr == &func".

This is just a naming suggestion, but how about naming it unnamed_func (instead of unnamedfunc) so it's closer to unnamed_addr.

leonardchan edited the summary of this revision. (Show Details)
  • Renamed unnamedfunc to unnamed_func.
  • Replaced "equivalent to the resolved function at runtime" with "equivalent to the resolved function as a call target".
  • Added section to LangRef.
leonardchan retitled this revision from [llvm][IR] Add a new Constant (UnnamedFunc) to [llvm][IR] Add dso_local_equivalent Constant.
leonardchan edited the summary of this revision. (Show Details)
leonardchan added a subscriber: rjmccall.

Updated to reflect the most recent comments at http://lists.llvm.org/pipermail/llvm-dev/2020-September/145177.html.

leonardchan edited the summary of this revision. (Show Details)Wed, Sep 23, 5:25 PM
rjmccall added inline comments.Wed, Sep 23, 10:03 PM
llvm/docs/LangRef.rst
3771

There has to be a type for the global in the syntax, right? Or is that an assumed prefix on this, and then we don't need it separately on the global because it's the same type?

Also, do we actually need the parens? I think the other places we use parens here, it actually does meaningful grouping.

On wording, allow me to suggest:

A '``dso_local_equivalent``' constant represents a global value which is functionally equivalent to a given global value, but is always defined in the current linkage unit.  The resulting pointer has the same type as the underlying global.  The resulting pointer is permitted, but not required, to be different from a pointer to the global, and it may have different values in different translation units.

The target global may not have ``extern_weak`` linkage.

``dso_local_equivalent`` can be implemented in several ways:

- If the global has private or internal linkage, has hidden visibility, or is ``dso_local``, ``dso_local_equivalent`` can be implemented as simply a pointer to the global.

- If the global is a function, ``dso_local_equivalent`` can be implemented with a stub that tail-calls the function.  Many targets support relocations that resolve at link time to either a function or a stub for it, depending on if the function is defined within the linkage unit; LLVM will use this when available.  (This is commonly called a "PLT stub".)  On other targets, the stub may have to be emitted explicitly.

- If the global is a variable, this can be implemented by copying the contents of the variable into the current linkage unit (as if by `memcpy`).  This is not supported on all targets, and target-specific restrictions may apply.  For example, on ELF this can be implemented as a copy relocation, which requires that the global has the same size at load time as it did at at link time.

I don't think you need to keep anything from the last paragraph.

leonardchan added a reviewer: rjmccall.
leonardchan removed a subscriber: rjmccall.
leonardchan added inline comments.
llvm/docs/LangRef.rst
3771

There has to be a type for the global in the syntax, right? Or is that an assumed prefix on this, and then we don't need it separately on the global because it's the same type?

The latter. I was aiming for a syntax similar to blockaddress, but we would know the global type since we still need to write it before dso_local_equivalent.

Also, do we actually need the parens? I think the other places we use parens here, it actually does meaningful grouping.

You're right. They can be removed.

Thanks for the suggestion also! The only thing I'd probably clarify on is if this constant accepts "global values" vs "global objects" since global value implies that this could also accept global aliases or global ifuncs. I suppose there's nothing wrong with this: ifuncs can have the same handling as functions, and depending on the aliasee, an alias can have the same handling as a function or variable. It could be that this is already implied, but I'll update to explicitly mention them also.

leonardchan edited the summary of this revision. (Show Details)Thu, Sep 24, 12:28 PM
leonardchan edited the summary of this revision. (Show Details)Thu, Sep 24, 12:28 PM

Previous diff was against HEAD rather than master. Updated.

Add generic lowering for targets that do not support PLTs.

I wasn't sure what the best way to lower the constant was, so I ended up making a module pass that runs before ISel and replaces dso_local_equivalents with dso_local stubs that tail call to the original function. I'm open to changing it if there's a cleaner way. I thought the easiest approach was to provide the generic implementation as IR that could be lowered naturally than through SDNodes in ISel.

pcc added inline comments.Thu, Oct 1, 11:22 AM
llvm/docs/LangRef.rst
3797

I would make this specific to functions. I don't think this can be made to work with globals at least with existing ELF linkers because I don't think you can get the copy relocation behavior without the symbol's value being overridden by the linkage unit with the copy relocation, and it doesn't make sense to have more than one linkage unit override the address of the same global.

rjmccall added inline comments.Thu, Oct 1, 3:37 PM
llvm/docs/LangRef.rst
3797

Oh, does ELF's copy relocation necessarily steal the symbol so that it resolves into the copying linkage unit? I thought it might be separable so that you could just get a local copy of the data without changing symbol resolution. That would be a much more generically useful feature.

pcc added inline comments.Thu, Oct 1, 3:46 PM
llvm/docs/LangRef.rst
3797

I think the behaviors may be separable at the binary level (i.e. you could hand-craft an executable that only copies but doesn't override), but at the linker level the copying and overriding happen at the same time when the linker sees a non-GOT-generating relocation pointing to the symbol.

leonardchan added inline comments.Thu, Oct 1, 7:08 PM
llvm/docs/LangRef.rst
3797

I'm fine with applying this to just functions for now since that was what I was initially planning to support. If possible, support for variables could be rolled out in the future. It looks like, off the bat, we could just use a GOT entry if we know the symbol is dso_local, but I think we can already achieve this by having a hidden global that points to your symbol.

Took me some time to research copy relocations, so correct me if I'm wrong, but should it be R_*_GLOB_DATA that overrides the symbol address during linking, whereas the copy relocation (R_*_COPY) just does the copying as @rjmccall suggests? I think it's R_*_GLOB_DATA that the GOT uses for getting addresses(?).

pcc added inline comments.Thu, Oct 1, 7:22 PM
llvm/docs/LangRef.rst
3797

The R_*_COPY is normally generated by the linker and only present in the executable or shared object. It isn't present in an object file. The symbol address override is implemented by a symbol table entry, not by a relocation.

I'm not sure what would happen if you had an R_*_COPY in an object file without a definition of the symbol. You would need to make sure that it's passed through to the binary correctly and that dynamic loaders have the expected behavior (because normally dynamic loaders expect the symbol that's being copied to also be defined locally). The size of the symbol would also need to be known at compile time (as opposed to at link time in the normal case where the linker is inserting the relocation) in order to allocate enough space for the copy.

MaskRay added a subscriber: MaskRay.Thu, Oct 1, 7:55 PM
MaskRay added inline comments.
llvm/docs/LangRef.rst
3797

I agree with pcc that the variable case should be dropped.

R_*_COPY could be used for shared objects in some ld.so (e.g. musl, which just skips the first definition; but not in glibc, which special cases that a COPY skips an executable definition). Generally it does not make sense for a shared object.

R_*_COPY on an executable copying its own symbol does not work -> the behavior is like RTLD_NEXT.

rjmccall added inline comments.Thu, Oct 1, 10:55 PM
llvm/docs/LangRef.rst
3797

Is that avoidable? What happens if you ask for a copy relocation in an object file, or is that not expressible?

3797

I'm fine with just supporting functions given the absence of a target that can support this for variables.

3797

I thought I deleted this comment, but apparently not; please ignore it.

Update to only support functions since we don't have a fully flushed out way of supporting global variables.

leonardchan edited the summary of this revision. (Show Details)Thu, Oct 15, 12:03 PM

ping, any more comments for this?