[ubsan] Make the alignment check work with some extern decls (PR32630)

[vsk]

Summary

UBSan's alignment check does not detect unaligned loads and stores from
extern struct declarations. Example:

struct S { int i; };
extern struct S g_S; // &g_S may not be aligned properly.

int main() {
  return g_S.i; // No alignment check for &g_S.i is emitted.
}

The frontend skips the alignment check for &g_S.i because the check is
constant-folded to 'NOT NULL' by the IR builder.

If we drop the alignment information from extern struct declarations,
the IR builder would no longer be able to constant-fold the checks away.
That would make the alignment check more useful.

Note: it would still not be able to catch the following case --

extern int i; // &i is not aligned properly.

PR: https://bugs.llvm.org/show_bug.cgi?id=32630

Testing: check-clang, check-ubsan.

[efriedma]

Note: it would still not be able to catch the following case --

Do you know why?

lib/CodeGen/CodeGenModule.cpp

2319

If we don't explicitly set the alignment, is it 1? Or is it picking up the alignment from the type somehow?

[vsk]

In D32456#736120, @efriedma wrote:

Note: it would still not be able to catch the following case --

Do you know why?

As-written, the alignment check doesn't apply to loads and stores on non-pointer POD types. The reason why is probably that most of these loads/stores happen on the stack, where the data should be aligned.

lib/CodeGen/CodeGenModule.cpp

2319

If the alignment isn't explicitly set, it is initialized to 0. The LangRef states: 'Either global variable definitions or declarations may have an explicit section to be placed in and may have an optional explicit alignment specified'. It should be OK to not specify the alignment.

[efriedma]

If the alignment isn't explicitly set, it is initialized to 0.

That's what I thought. I don't like this; clang should explicitly set an alignment for every global.

[vsk]

In D32456#737228, @efriedma wrote:

If the alignment isn't explicitly set, it is initialized to 0.

That's what I thought. I don't like this; clang should explicitly set an alignment for every global.

Ok, I will set this aside for now and think of an alternate approach to address PR32630.

[efriedma]

Err, that's not what I meant...

If the alignment of a global in LLVM IR is "zero", it doesn't really mean zero. It means "guess the alignment I want based on the specified type of the global". And that's not a game we ever want to play when generating IR from clang.

[vsk]

In D32456#737273, @efriedma wrote:

Err, that's not what I meant...

If the alignment of a global in LLVM IR is "zero", it doesn't really mean zero. It means "guess the alignment I want based on the specified type of the global". And that's not a game we ever want to play when generating IR from clang.

The alternative mentioned in PR32630 is to decrease the alignment of the global when -fsanitize=alignment is enabled. Is this less risky than not specifying the alignment at all? I assumed that it was not.

Hm, giving this more thought I'm not happy with how fragile this patch seems. Changing the way we set alignment to hack around constant folding doesn't seem great. And this patch does not interact properly with extern definitions which have 'attribute((aligned(N)))' set.

[efriedma]

This should work correctly with alignment attributes, I think? IIRC those just change the return value of getDeclAlign().

I agree it's a little fragile, but I don't have a good suggestion to avoid that.

lib/CodeGen/CodeGenModule.cpp

CodeGenModule::GetOrCreateLLVMGlobal(StringRef MangledName,
   }
 
   // Handle things which are present even on external declarations.
   if (D) {
     // FIXME: This code is overly simple and should be merged with other global
     // handling.
     GV->setConstant(isTypeConstant(D->getType(), false));
 
+    if (LangOpts.Sanitize.has(SanitizerKind::Alignment) && !IsForDefinition &&
+        (D->getStorageClass() == SC_Extern ||
+         D->getStorageClass() == SC_PrivateExtern)) {
+      // The alignment sanitizer can catch more bugs if we don't attach
+      // alignment info to extern globals.
+    } else {
       GV->setAlignment(getContext().getDeclAlign(D).getQuantity());
+    }
 
     setLinkageAndVisibilityForGV(GV, D);
 
     if (D->getTLSKind()) {
       if (D->getTLSKind() == VarDecl::TLS_Dynamic)
         CXXThreadLocals.push_back(D);
       setTLSMode(GV, *D);
     }

test/CodeGenCXX/ubsan-global-alignment.cpp

 // RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=alignment | FileCheck %s
 
 struct S {
   int I;
 };
 
 extern S g_S;
+__private_extern__ S pe_S;
 extern S array_S[];
 
-// CHECK-LABEL: define i32 @_Z18load_extern_global
-int load_extern_global() {
-  // FIXME: The IR builder constant-folds the alignment check away to 'true'
-  // here, so we never call the diagnostic. This is PR32630.
-  // CHECK-NOT: ptrtoint i32* {{.*}} to i32, !nosanitize
+// CHECK-LABEL: define i32 @_Z19load_extern_global1
+int load_extern_global1() {
+  // CHECK: br i1 icmp eq (i64 and (i64 ptrtoint (%struct.S* @g_S to i64), i64 3), i64 0), {{.*}}, !nosanitize
+  // CHECK: call void @__ubsan_handle_type_mismatch
   // CHECK: [[I:%.*]] = load i32, i32* getelementptr inbounds (%struct.S, %struct.S* @g_S, i32 0, i32 0), align 4
   // CHECK-NEXT: ret i32 [[I]]
   return g_S.I;
 }
 
+// CHECK-LABEL: define i32 @_Z19load_extern_global2
+int load_extern_global2() {
+  // CHECK: br i1 icmp eq (i64 and (i64 ptrtoint (%struct.S* @pe_S to i64), i64 3), i64 0), {{.*}}, !nosanitize
+  // CHECK: call void @__ubsan_handle_type_mismatch
+  // CHECK: [[I:%.*]] = load i32, i32* getelementptr inbounds (%struct.S, %struct.S* @pe_S, i32 0, i32 0), align 4
+  // CHECK-NEXT: ret i32 [[I]]
+  return pe_S.I;
+}
+
 // CHECK-LABEL: define i32 @_Z22load_from_extern_array
 int load_from_extern_array(int I) {
   // CHECK: [[I:%.*]] = getelementptr inbounds %struct.S, %struct.S* {{.*}}, i32 0, i32 0
   // CHECK-NEXT: [[PTRTOINT:%.*]] = ptrtoint i32* [[I]] to i64, !nosanitize
   // CHECK-NEXT: [[AND:%.*]] = and i64 [[PTRTOINT]], 3, !nosanitize
   // CHECK-NEXT: [[ICMP:%.*]] = icmp eq i64 [[AND]], 0, !nosanitize
   // CHECK-NEXT: br i1 [[ICMP]]
   // CHECK: call void @__ubsan_handle_type_mismatch
   return array_S[I].I;
 }