Page MenuHomePhabricator

Stop increasing alignment of externally-visible globals on ELF platforms.
ClosedPublic

Authored by jyknight on Jan 13 2016, 8:55 AM.

Details

Summary

With ELF, the alignment of a global variable in a shared library will
get copied into an executables linked against it, if the executable even
accesss the variable. So, it's not possible to implicitly increase
alignment based on access patterns, or you'll break existing binaries.

This happened to affect libc++'s std::cout symbol, for example. See
thread: http://thread.gmane.org/gmane.comp.compilers.clang.devel/45311

Diff Detail

Repository
rL LLVM

Event Timeline

jyknight updated this revision to Diff 44756.Jan 13 2016, 8:55 AM
jyknight retitled this revision from to Stop increasing alignment of externally-visible globals on ELF platforms..
jyknight updated this object.
jyknight added reviewers: t.p.northover, dim.
jyknight added a subscriber: llvm-commits.
emaste added a subscriber: emaste.Jan 13 2016, 9:31 AM
dim accepted this revision.Jan 13 2016, 2:20 PM
dim edited edge metadata.

LGTM. The assembly output of the cout-align.cpp sample, compiled before and after this change:

--- cout-align-r257663-before.s 2016-01-13 23:09:24.266652000 +0100
+++ cout-align-r257663-after.s  2016-01-13 22:17:26.781312000 +0100
@@ -19,7 +19,7 @@ _Z7do_initv:
        movl    $_ZTV1BIciE+24, %eax
        movd    %rax, %xmm1
        punpcklqdq      %xmm0, %xmm1    # xmm1 = xmm1[0],xmm0[0]
-       movdqa  %xmm1, cout(%rip)
+       movdqu  %xmm1, cout(%rip)
        popq    %rbp
        retq
 .Lfunc_end0:
@@ -205,7 +205,7 @@ GCC_except_table4:
        .type   cout,@object            # @cout
        .bss
        .globl  cout
-       .align  16
+       .align  8
 cout:
        .zero   24
        .size   cout, 24
include/llvm/IR/GlobalValue.h
349 ↗(On Diff #44756)

s/unilateraly/unilaterally/

This revision is now accepted and ready to land.Jan 13 2016, 2:20 PM
t.p.northover edited edge metadata.Jan 13 2016, 2:23 PM

Code looks good to me too, and the comments despite me piling on with the nits.

I don't really care about the sentence, but we should probably be a little more conservative with the PIC in case someone actually comes along and does it without investigating properly.

lib/IR/Globals.cpp
153–156 ↗(On Diff #44756)

Minor nit, but this isn't actually a sentence.

I'd also probably be more explicit about it being "link time" that the copying happens.

170–173 ↗(On Diff #44756)

It depends what we want to support, but non-PIC shared libraries certainly used to be a thing. They're now strongly discouraged, but we'd probably still have to think carefully before breaking them.

jyknight marked 2 inline comments as done.Jan 13 2016, 3:27 PM
jyknight added inline comments.
lib/IR/Globals.cpp
170–173 ↗(On Diff #44756)

Ahhh, right, forgot about that. Now, for some archs (x86-64, any others?) it's not possible to include non-PIC code in a shared-library. But, I'll just take out that TODO.

This revision was automatically updated to reflect the committed changes.