[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)
efriedma added a comment. 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. https://reviews.llvm.org/D32456 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)
vsk added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D32456 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)
efriedma added a comment. 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. https://reviews.llvm.org/D32456 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)
vsk added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D32456 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)
efriedma added a comment. > 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. https://reviews.llvm.org/D32456 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)
vsk added a comment.
In https://reviews.llvm.org/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.
Comment at: lib/CodeGen/CodeGenModule.cpp:2319
+ // The alignment sanitizer can catch more bugs if we don't attach
+ // alignment info to extern globals.
+} else {
efriedma wrote:
> If we don't explicitly set the alignment, is it 1? Or is it picking up the
> alignment from the type somehow?
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.
https://reviews.llvm.org/D32456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)
efriedma added a comment.
> Note: it would still not be able to catch the following case --
Do you know why?
Comment at: lib/CodeGen/CodeGenModule.cpp:2319
+ // The alignment sanitizer can catch more bugs if we don't attach
+ // alignment info to extern globals.
+} else {
If we don't explicitly set the alignment, is it 1? Or is it picking up the
alignment from the type somehow?
https://reviews.llvm.org/D32456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32456: [ubsan] Make the alignment check work with some extern decls (PR32630)
vsk created this revision.
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; // _S may not be aligned properly.
int main() {
return g_S.i; // No alignment check for _S.i is emitted.
}
The frontend skips the alignment check for _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; // is not aligned properly.
PR: https://bugs.llvm.org/show_bug.cgi?id=32630
Testing: check-clang, check-ubsan.
https://reviews.llvm.org/D32456
Files:
lib/CodeGen/CodeGenModule.cpp
test/CodeGenCXX/ubsan-global-alignment.cpp
Index: test/CodeGenCXX/ubsan-global-alignment.cpp
===
--- test/CodeGenCXX/ubsan-global-alignment.cpp
+++ test/CodeGenCXX/ubsan-global-alignment.cpp
@@ -5,18 +5,27 @@
};
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
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -2312,7 +2312,14 @@
// handling.
GV->setConstant(isTypeConstant(D->getType(), false));
-GV->setAlignment(getContext().getDeclAlign(D).getQuantity());
+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);
Index: test/CodeGenCXX/ubsan-global-alignment.cpp
===
--- test/CodeGenCXX/ubsan-global-alignment.cpp
+++ test/CodeGenCXX/ubsan-global-alignment.cpp
@@ -5,18 +5,27 @@
};
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
Index: lib/CodeGen/CodeGenModule.cpp
