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

[jyknight]

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

[dim]

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

s/unilateraly/unilaterally/

[t.p.northover]

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

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

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]

lib/IR/Globals.cpp

170–173

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.

include/llvm/IR/GlobalValue.h

/// @}
   }
 
   /// Returns true if this global's definition will be the one chosen by the
   /// linker.
   bool isStrongDefinitionForLinker() const {
     return !(isDeclarationForLinker() || isWeakForLinker());
   }
 
+  // Returns true if the alignment of the value can be unilateraly
+  // increased.
+  bool canIncreaseAlignment() const;
+
   /// This method unlinks 'this' from the containing module, but does not delete
   /// it.
   virtual void removeFromParent() = 0;
 
   /// This method unlinks 'this' from the containing module and deletes it.
   virtual void eraseFromParent() = 0;
 
   /// Get the module that this global value is contained inside of...

lib/CodeGen/CodeGenPrepare.cpp

for (auto &Arg : CI->arg_operands()) {
       if ((AI = dyn_cast<AllocaInst>(Val)) && AI->getAlignment() < PrefAlign &&
           DL->getTypeAllocSize(AI->getAllocatedType()) >= MinSize + Offset2)
         AI->setAlignment(PrefAlign);
       // Global variables can only be aligned if they are defined in this
       // object (i.e. they are uniquely initialized in this object), and
       // over-aligning global variables that have an explicit section is
       // forbidden.
       GlobalVariable *GV;
-      if ((GV = dyn_cast<GlobalVariable>(Val)) && GV->hasUniqueInitializer() &&
-          !GV->hasSection() && GV->getAlignment() < PrefAlign &&
+      if ((GV = dyn_cast<GlobalVariable>(Val)) && GV->canIncreaseAlignment() &&
+          GV->getAlignment() < PrefAlign &&
           DL->getTypeAllocSize(GV->getType()->getElementType()) >=
               MinSize + Offset2)
         GV->setAlignment(PrefAlign);
     }
     // If this is a memcpy (or similar) then we may be able to improve the
     // alignment
     if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(CI)) {
       unsigned Align = getKnownAlignment(MI->getDest(), *DL);

lib/IR/Globals.cpp

 //===-- Globals.cpp - Implement the GlobalValue & GlobalVariable class ----===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the GlobalValue & GlobalVariable classes for the IR
 // library.
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/IR/GlobalValue.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/GlobalAlias.h"
+#include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/Support/ErrorHandling.h"
 using namespace llvm;
 
 //===----------------------------------------------------------------------===//
 //                            GlobalValue Class
bool GlobalValue::isDeclaration() const {
   if (const Function *F = dyn_cast<Function>(this))
     return F->empty() && !F->isMaterializable();
 
   // Aliases are always definitions.
   assert(isa<GlobalAlias>(this));
   return false;
 }
 
+bool GlobalValue::canIncreaseAlignment() const {
+  // Firstly, can only increase the alignment of a global if it
+  // is a strong definition.
+  if (!isStrongDefinitionForLinker())
+    return false;
+
+  // It also has to either not have a section defined, or, not have
+  // alignment specified. (If it is assigned a section, the global
+  // could be densely packed with other objects in the section, and
+  // increasing the alignment could cause padding issues.)
+  if (hasSection() && getAlignment() > 0)
+    return false;
+
+  // On ELF platforms, we're further restricted in that we can't
+  // increase the alignment of any variable which might be emitted
+  // into a shared library, and which is exported. If the main
+  // executable accesses a variable found in a shared-lib, the main
+  // exe actually allocates memory for, and exports the symbol ITSELF,
+  // taking precendence over the symbol found in the library. At build
+  // time, the observed alignment of the variable is copied into the
+  // executable. (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.).
+  //
+  // And thus, even though you might think you are defining the
+  // global, and allocating the memory for the global in your object
+  // file, and thus should be able to set the alignment arbitrarily,
+  // that's not actually true. Doing so can cause an ABI breakage; an
+  // executable might have already been built with the previous
+  // alignment of the variable, and then assuming an increased
+  // alignment will be incorrect.
+  //
+  // TODO: is it possible to determine if the relocation mode is PIC
+  // at this point in the code? I think it'd be correct to skip this
+  // check in non-PIC mode. We'd need something like:
+  //  TargetMachine.getRelocationModel() == Reloc::PIC_;
+
+  // Conservatively assume ELF if there's no parent pointer.
+  bool isELF =
+      (!Parent || Triple(Parent->getTargetTriple()).isOSBinFormatELF());
+  if (isELF && hasDefaultVisibility() && !hasLocalLinkage())
+    return false;
+
+  return true;
+}
+
 //===----------------------------------------------------------------------===//
 // GlobalVariable Implementation
 //===----------------------------------------------------------------------===//
 
 GlobalVariable::GlobalVariable(Type *Ty, bool constant, LinkageTypes Link,
                                Constant *InitVal, const Twine &Name,
                                ThreadLocalMode TLMode, unsigned AddressSpace,
                                bool isExternallyInitialized)

lib/Transforms/Utils/Local.cpp

 /// we control, modify the object's alignment to PrefAlign. This isn't
 /// often possible though. If alignment is important, a more reliable approach
 /// is to simply align all global variables and allocation instructions to
 /// their preferred alignment from the beginning.
 ///
 static unsigned enforceKnownAlignment(Value *V, unsigned Align,
                                       unsigned PrefAlign,
                                       const DataLayout &DL) {
+  assert(PrefAlign > Align);
+
   V = V->stripPointerCasts();
 
   if (AllocaInst *AI = dyn_cast<AllocaInst>(V)) {
+    assert(AI->getAlignment() <= Align);
     // If the preferred alignment is greater than the natural stack alignment
     // then don't round up. This avoids dynamic stack realignment.
     if (DL.exceedsNaturalStackAlignment(PrefAlign))
       return Align;
-    // If there is a requested alignment and if this is an alloca, round up.
-    if (AI->getAlignment() >= PrefAlign)
-      return AI->getAlignment();
     AI->setAlignment(PrefAlign);
     return PrefAlign;
   }
 
   if (auto *GO = dyn_cast<GlobalObject>(V)) {
+    assert(GO->getAlignment() <= Align);
     // If there is a large requested alignment and we can, bump up the alignment
     // of the global.  If the memory we set aside for the global may not be the
     // memory used by the final program then it is impossible for us to reliably
     // enforce the preferred alignment.
-    if (!GO->isStrongDefinitionForLinker())
+    if (!GO->canIncreaseAlignment())
       return Align;
 
-    if (GO->getAlignment() >= PrefAlign)
-      return GO->getAlignment();
-    // We can only increase the alignment of the global if it has no alignment
-    // specified or if it is not assigned a section.  If it is assigned a
-    // section, the global could be densely packed with other objects in the
-    // section, increasing the alignment could cause padding issues.
-    if (!GO->hasSection() || GO->getAlignment() == 0)
-      GO->setAlignment(PrefAlign);
-    return GO->getAlignment();
+    return PrefAlign;
   }
 
   return Align;
 }
 
 /// getOrEnforceKnownAlignment - If the specified pointer has an alignment that
 /// we can determine, return it, otherwise return 0.  If PrefAlign is specified,
 /// and it is more than the alignment of the ultimate object, see if we can

test/CodeGen/ARM/memfunc.ll

-; RUN: llc < %s -mtriple=armv7-apple-ios -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-IOS
-; RUN: llc < %s -mtriple=thumbv7m-none-macho -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-DARWIN
-; RUN: llc < %s -mtriple=arm-none-eabi -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-EABI
-; RUN: llc < %s -mtriple=arm-none-eabihf -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-EABI
-; RUN: llc < %s -mtriple=arm-none-androideabi -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-EABI
-; RUN: llc < %s -mtriple=arm-none-gnueabi -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-GNUEABI
-; RUN: llc < %s -mtriple=arm-none-gnueabihf -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-GNUEABI
+; RUN: llc < %s -mtriple=armv7-apple-ios -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-IOS --check-prefix=CHECK
+; RUN: llc < %s -mtriple=thumbv7m-none-macho -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-DARWIN --check-prefix=CHECK
+; RUN: llc < %s -mtriple=arm-none-eabi -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-EABI --check-prefix=CHECK
+; RUN: llc < %s -mtriple=arm-none-eabihf -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-EABI --check-prefix=CHECK
+; RUN: llc < %s -mtriple=arm-none-androideabi -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-EABI --check-prefix=CHECK
+; RUN: llc < %s -mtriple=arm-none-gnueabi -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-GNUEABI --check-prefix=CHECK
+; RUN: llc < %s -mtriple=arm-none-gnueabihf -disable-post-ra -o - | FileCheck %s --check-prefix=CHECK-GNUEABI --check-prefix=CHECK
 
 define void @f1(i8* %dest, i8* %src) {
 entry:
   ; CHECK-LABEL: f1
 
   ; CHECK-IOS: bl _memmove
   ; CHECK-DARWIN: bl _memmove
   ; CHECK-EABI: bl __aeabi_memmove
entry:
   unreachable
 }
 
 ; CHECK: {{\.data|\.section.+data}}
 ; CHECK-NOT: .align
 ; CHECK: arr1:
 ; CHECK-IOS: .align 3
 ; CHECK-DARWIN: .align 2
-; CHECK-EABI: .align 2
-; CHECK-GNUEABI: .align 2
+; CHECK-EABI-NOT: .align
+; CHECK-GNUEABI-NOT: .align
 ; CHECK: arr2:
 ; CHECK: {{\.section.+foo,bar}}
 ; CHECK-NOT: .align
 ; CHECK: arr3:
 ; CHECK-NOT: .align
 ; CHECK: arr4:
 ; CHECK: {{\.data|\.section.+data}}
 ; CHECK-NOT: .align
 ; CHECK: arr5:
 ; CHECK-NOT: .align
 ; CHECK: arr6:
 ; CHECK-NOT: arr7:
 
 declare void @llvm.memmove.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1) nounwind
 declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture, i8* nocapture, i32, i32, i1) nounwind
 declare void @llvm.memset.p0i8.i32(i8* nocapture, i8, i32, i32, i1) nounwind