This is an archive of the discontinued LLVM Phabricator instance.

[IR] Allow increasing the alignment of dso-local globals.
ClosedPublic

Authored by efriedma on Oct 29 2018, 6:01 PM.

Details

Summary

I think this is the actual important property; the previous visibility check was an approximation.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Oct 29 2018, 6:01 PM
rnk added inline comments.Oct 29 2018, 7:24 PM
lib/IR/Globals.cpp
240–242 ↗(On Diff #171615)

What about copy relocations? I think if you have some DSO a.so and a binary b.exe, b.exe could copy visible global variables from a.so into b.exe. Will it keep the alignment if it does?

efriedma added inline comments.Oct 30 2018, 11:13 AM
lib/IR/Globals.cpp
240–242 ↗(On Diff #171615)

A global variable defined in a shared library isn't dso_local.

jyknight added inline comments.Oct 30 2018, 1:21 PM
lib/IR/Globals.cpp
240–242 ↗(On Diff #171615)

While I think this change is probably going to result in correct behavior at the moment, I'm a bit afraid of it causing incorrect behavior in the future, as we fix the rest of the compiler to have internally-consistent behavior around symbol preemption. (for example, if we fix the behavior where we inline all function definitions despite them being interposable, and have clang actually annotate definitions dso_local by default.)

I think my basic question is: does the "dso_local" attribute make a promise on the part of the USER that they will not do any symbol interposition? Or is it a requirement on the user PLUS the technical underpinnings of the implementation?

I would kind of expect the former -- that as an IR user, I could mark a global variable dso_local, if I can assure the compiler that nobody will _semantically_ attempt to intercept the definition. However, the issue is that on ELF platforms, the linker will generate an symbol interception behind the scenes without me asking for or wanting it, simply to implement normal _reads_ of a global in a dso. That seems like something I shouldn't have to know/care about.

But in such cases, we'd still need to not increase the alignment of a global, even when a promise that it won't be semantically interposed has been made (which is what I'm assuming the dso_local ought to mean).

efriedma added inline comments.Oct 30 2018, 1:50 PM
lib/IR/Globals.cpp
240–242 ↗(On Diff #171615)

From the previous discussion, I was under the impression that dso_local was supposed to model the address of the underlying bits, not the semantic properties. That's useful by itself because we can avoid accessing the GOT to compute the address.

If we want to support semantic interposition, I think we would add a new linkage type (e.g. introduce a new linkage "interposable"; it's the same as "weak" from the perspective of the optimizer, but we don't mark it weak in the ELF file).

I can propose a patch to LangRef to clarify, if that makes sense.

rnk accepted this revision.Oct 30 2018, 2:17 PM
rnk added inline comments.
lib/IR/Globals.cpp
240–242 ↗(On Diff #171615)

I think of dso_local as relating to what kind of mechanical relocation you need to generate, not whether the definition can be interposed semantically. So, for example, if you have a constant array of data in a shared library, you can fold loads from it even if it's not marked dso_local, as long as it has *_odr or external linkage.

I had forgotten how dso_local relates to -fPIC vs. -fPIE, which is silly, since they were part of the motivation for adding it. I think this patch is correct. In the case I described, clang will not mark the global dso_local.

The net effect of this change is going to be that we can increase the alignment of vanilla, default visibility, external globals in executables.

This revision is now accepted and ready to land.Oct 30 2018, 2:17 PM
This revision was automatically updated to reflect the committed changes.