Page MenuHomePhabricator

[AsmPrinter][ELF] Define local aliases (.Lfoo$local) for GlobalObjects
ClosedPublic

Authored by MaskRay on Jan 22 2020, 2:11 PM.

Details

Summary

For MC_GlobalAddress operands referencing certain GlobalObjects,
we can lower them to STB_LOCAL aliases to avoid costs brought by
assembler/linker's conservative decisions about symbol interposition:

  • An assembler conservatively assumes a global default visibility symbol interposable (ELF semantics). So relocations in object files are needed even if the code generator assumed the definition exact and non-interposable.
  • The relocations can cause the creation of PLT entries on some targets for -shared links. A linker conservatively assumes a global default visibility symbol interposable (if not otherwise constrained by -Bsymbolic/--dynamic-list/VER_NDX_LOCAL/etc).

"certain" refers to GlobalObjects in the intersection of
hasExactDefinition() and !isInterposable(): external, appending, internal, private.
Local linkages (internal and private) cannot be interposed. appending is for very
few objects LLVM interpret specially. So the set just includes external.

This patch emits STB_LOCAL aliases (.Lfoo$local) for such GlobalObjects, so that targets can lower
MC_GlobalAddress operands to STB_LOCAL aliases if applicable.
We may extend the scope and include GlobalAlias in the future.

LLVM's existing -fno-semantic-interposition behaviors give us license to do such optimizations:

  • Various optimizations (ipconstprop, inliner, sccp, sroa, etc) treat normal ExternalLinkage GlobalObjects as non-interposable.
  • Before D72197, MC resolved a PC-relative VK_None fixup to a non-local symbol at assembly time (no outstanding relocation), if the target is defined in the same section. Put it simply, even if IR optimizations failed to optimize and allowed interposition for the function call in void foo() {} void bar() { foo(); }, the assembler would disallow it.

This patch sets up AsmPrinter infrastructure to make -fno-semantic-interposition more so.
With and without the patch, the object file output should be identical:
.Lfoo$local does not take a symbol table entry.

Diff Detail

Event Timeline

MaskRay created this revision.Jan 22 2020, 2:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 2:11 PM

Unit tests: pass. 62112 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Ping D73228 and D73230 (tested on a large code base).

Patch makes sense to me.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
454

I think you explained why we only are doing this for ExternalLinkage well in the patch description, do you think the reasoning should be here as a comment?

456

Ideally GV.isDSOLocal() should fully determine if a symbol is replaceable at runtime or not. Not sure if its safer to use TargetMachine::shouldAssumeDSOLocal instead. I believe the only difference right now for ELF would be the copy relocation case.

That said, this conditional is only true if the symbol is not preemptable. IIUC the compiler shouldn't be accessing data GOT-indirect, and the linker won't be emitting PLTs for the functions. Which means:

costs brought by potential symbol interpositionloc
(relocations in object files, PLT/GOT in executables/shared objects).

There shouldn't be any saving related to PLT/GOT. I'm not very familiar with linking on archs other then Power though so I could definitely be wrong here.

To make sure I understand, this is adding a local alias as at assembly time that can be used as a fixup target. This is necessary as at assembly time all we have is a global default visibility symbol which we have to assume can be interposed, even if the code-generator has already assumed it. Are there other uses that I'm missing?
As it stands this sounds reasonable to me, I'm trying to think if there are any other alternatives and what the trade-offs would be. For example is there no way that the assembler could find out the information at assembly time, or is that more complex than adding local aliases?

llvm/test/CodeGen/AMDGPU/r600-constant-array-fixup.ll
4 ↗(On Diff #239696)

As I understand it, this patch should just add local aliases for some known DSO local global symbols. This looks to have changed the relocation target and the value of arr. Intuitively I wouldn't have expected this from adding the aliases alone. There must be something I'm missing either in the patch, or this could be something to do with the AMDGPU backend. I'd be grateful if you could explain the difference?

MaskRay updated this revision to Diff 240942.Jan 28 2020, 10:53 AM

Improve comment for AsmPrinter::getSymbolPreferLocal()

MaskRay marked 2 inline comments as done.Jan 28 2020, 11:37 AM
MaskRay added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
456

It is not convenient to access Module in this function. We probably should improve GlobalVariable::maybeSetDsoLocal to take account of TargetMachine::shouldAssumeDSOLocal, so that isDSOLocal() and TargetMachine::shouldAssumeDSOLocal will return the same value.

We require a definition (!GV.isDeclaration()). So for copy relocation, e.g.

// x86 x86-64
// or aarch64 -mpie-copy-relocations
extern int var;
int foo() { return var; }

We don't use a local alias (no issue here).

MaskRay marked 2 inline comments as done.Jan 28 2020, 11:43 AM
MaskRay added a subscriber: arsenm.
MaskRay added inline comments.
llvm/test/CodeGen/AMDGPU/r600-constant-array-fixup.ll
4 ↗(On Diff #239696)

After rebase, this change goes away.

It seemed an issue fixed by @arsenm in rG7dc49f77ee508b4152f9291c8e804e4eda3653d3 . It does look weird to me: adding a new symbol somehow alters relocations.

MaskRay marked an inline comment as done.Jan 28 2020, 12:05 PM

To make sure I understand, this is adding a local alias as at assembly time that can be used as a fixup target. This is necessary as at assembly time all we have is a global default visibility symbol which we have to assume can be interposed, even if the code-generator has already assumed it. Are there other uses that I'm missing?

Yes. STV_DEFAULT STB_GLOBAL preemptivity depends on whether the link uses -no-pie/-pie (executable) or -shared (shared object). Referencing a local alias makes -shared similar to the executable case.

(STV_PROTECTED/STV_INTERNAL/STV_HIDDEN) STB_GLOBAL does not need a local alias. But for simplicity, leave it to the linker.

As it stands this sounds reasonable to me, I'm trying to think if there are any other alternatives and what the trade-offs would be. For example is there no way that the assembler could find out the information at assembly time, or is that more complex than adding local aliases?

The downside is that textual assembly output will be slightly larger, but it is acceptable. Object assembly output can only be smaller because of elimination of certain relocations. .L symbols do not take space.

An alternative is to make the assembler aware of executable/shared use cases. For example, gas RISC-V has two directives .option nopic and .option pic. I believe that is a wrong direction.

The compiler has had to know the eventual use case (-fno-pic / -fpie / -fpic). The linker has to know the use case (-no-pie / -pie / -shared). The combinations already provide sufficient flexibility that makes a mode switch on the assembler side unnecessary.

Unit tests: pass. 62275 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Thanks for the update, I agree that informing the assembler of pic status isn't the right way to go, particularly if the assembler can't check that the code that you have written is actually pic and also the potentially incompatibility with GNU as. I've no objections.

sfertile accepted this revision.Jan 29 2020, 8:29 AM

LGTM.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
456

Good point, thanks.

This revision is now accepted and ready to land.Jan 29 2020, 8:29 AM
MaskRay updated this revision to Diff 241226.Jan 29 2020, 10:56 AM
MaskRay edited the summary of this revision. (Show Details)

Update description

This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 62303 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Hi @MaskRay,

This change has broke the update_llc_test_checks.py tool's functionality for any test that includes a local function.
The tool now removes all "; CHECK" lines from such tests and emits nothing instead.
You can check this by adding "dso_local" attribute to any of the X86 existing tests (for example /llvm/CodeGen/X86/add-i64.ll) and run "update_llc_test_checks.py on it.

I think that it's crucial to fix this (hint: the regex strings defined at the beginning of llvm-org/llvm/utils/UpdateTestChecks/asm.py don't capture these test cases anymore).

Thanks,
Ayman