[PATCH] D53852: [IR] Allow increasing the alignment of dso-local globals.

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/IR/Globals.cpp:240-242
   // executable binary. (A COPY relocation is also generated, to copy
   // the initial data from the shadowed variable in the shared-lib
   // into the location in the main binary, before running code.)
----------------
efriedma wrote:
> jyknight wrote:
> > efriedma wrote:
> > > rnk wrote:
> > > > 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?
> > > A global variable defined in a shared library isn't dso_local.
> > 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).
> > 
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D53852