Fix implicit null check with negative offset

Summary:
It seems that negative offset was accidentally allowed in D17967.
AFAICT small negative offset should be valid (always raise segfault) on all archs that I'm aware of (especially x86, which is the only one with this optimization enabled) and such case can be useful when loading hiden metadata from an object.

However, like the positive side, it should only be done within a certain limit.
For now, use the same limit on the positive side for the negative side.
A separate option can be added if needs appear.

Reviewers: mcrosier, skatkov

Reviewed By: skatkov

Subscribers: sanjoy, llvm-commits

Differential Revision: https://reviews.llvm.org/D38925

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@315991 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/CodeGen/ImplicitNullChecks.cpp b/lib/CodeGen/ImplicitNullChecks.cpp
index 19c52d013749ec826176833be178710d42f84f6f..bf0f88d49a82ca412ca23971cbafe964ebe59869 100644 (file)
--- a/lib/CodeGen/ImplicitNullChecks.cpp
+++ b/lib/CodeGen/ImplicitNullChecks.cpp
@@ -369,7 +369,7 @@ ImplicitNullChecks::isSuitableMemoryOp(MachineInstr &MI, unsigned PointerReg,
   // We want the mem access to be issued at a sane offset from PointerReg,
   // so that if PointerReg is null then the access reliably page faults.
   if (!((MI.mayLoad() || MI.mayStore()) && !MI.isPredicable() &&
-        Offset < PageSize))
+        -PageSize < Offset && Offset < PageSize))
     return SR_Unsuitable;
 
   // Finally, check whether the current memory access aliases with previous one.

diff --git a/test/CodeGen/X86/implicit-null-check-negative.ll b/test/CodeGen/X86/implicit-null-check-negative.ll
index c8d425c3889fe9edfc51ff2105a48223eba882f0..c05b4a072adfd455fe7695635f7e2ac3017efa58 100644 (file)
--- a/test/CodeGen/X86/implicit-null-check-negative.ll
+++ b/test/CodeGen/X86/implicit-null-check-negative.ll
@@ -37,6 +37,22 @@ define i32 @imp_null_check_gep_load(i32* %x) {
   ret i32 %t
 }
 
+define i32 @imp_null_check_neg_gep_load(i32* %x) {
+ entry:
+  %c = icmp eq i32* %x, null
+  br i1 %c, label %is_null, label %not_null, !make.implicit !0
+
+ is_null:
+  ret i32 42
+
+ not_null:
+; null - 5000 * sizeof(i32) lies outside the null page and hence the
+; load to %t cannot be assumed to be reliably faulting.
+  %x.gep = getelementptr i32, i32* %x, i32 -5000
+  %t = load i32, i32* %x.gep
+  ret i32 %t
+}
+
 define i32 @imp_null_check_load_no_md(i32* %x) {
 ; This is fine, except it is missing the !make.implicit metadata.
  entry:

diff --git a/test/CodeGen/X86/implicit-null-check.ll b/test/CodeGen/X86/implicit-null-check.ll
index ee795667cdb19832119b9166d0656c8b798a2525..8cfc9c669ad03beee8bfb0c22faa0aec1f147401 100644 (file)
--- a/test/CodeGen/X86/implicit-null-check.ll
+++ b/test/CodeGen/X86/implicit-null-check.ll
@@ -182,6 +182,28 @@ define void @imp_null_check_store(i32* %x) {
   ret void
 }
 
+define i32 @imp_null_check_neg_gep_load(i32* %x) {
+; CHECK-LABEL: _imp_null_check_neg_gep_load:
+; CHECK: [[BB0_imp_null_check_neg_gep_load:L[^:]+]]:
+; CHECK: movl -128(%rdi), %eax
+; CHECK: retq
+; CHECK: [[BB1_imp_null_check_neg_gep_load:LBB7_[0-9]+]]:
+; CHECK: movl $42, %eax
+; CHECK: retq
+
+ entry:
+  %c = icmp eq i32* %x, null
+  br i1 %c, label %is_null, label %not_null, !make.implicit !0
+
+ is_null:
+  ret i32 42
+
+ not_null:
+  %x.gep = getelementptr i32, i32* %x, i32 -32
+  %t = load i32, i32* %x.gep
+  ret i32 %t
+}
+
 !0 = !{}
 
 ; CHECK-LABEL: __LLVM_FaultMaps:
@@ -194,7 +216,7 @@ define void @imp_null_check_store(i32* %x) {
 ; CHECK-NEXT: .short 0
 
 ; # functions:
-; CHECK-NEXT: .long 7
+; CHECK-NEXT: .long 8
 
 ; FunctionAddr:
 ; CHECK-NEXT: .quad _imp_null_check_add_result
@@ -261,6 +283,19 @@ define void @imp_null_check_store(i32* %x) {
 ; Fault[0].HandlerOffset:
 ; CHECK-NEXT: .long [[BB1_imp_null_check_load]]-_imp_null_check_load
 
+; FunctionAddr:
+; CHECK-NEXT: .quad _imp_null_check_neg_gep_load
+; NumFaultingPCs
+; CHECK-NEXT: .long 1
+; Reserved:
+; CHECK-NEXT: .long 0
+; Fault[0].Type:
+; CHECK-NEXT: .long 1
+; Fault[0].FaultOffset:
+; CHECK-NEXT: .long [[BB0_imp_null_check_neg_gep_load]]-_imp_null_check_neg_gep_load
+; Fault[0].HandlerOffset:
+; CHECK-NEXT: .long [[BB1_imp_null_check_neg_gep_load]]-_imp_null_check_neg_gep_load
+
 ; FunctionAddr:
 ; CHECK-NEXT: .quad _imp_null_check_store
 ; NumFaultingPCs
@@ -289,7 +324,7 @@ define void @imp_null_check_store(i32* %x) {
 
 ; OBJDUMP: FaultMap table:
 ; OBJDUMP-NEXT: Version: 0x1
-; OBJDUMP-NEXT: NumFunctions: 7
+; OBJDUMP-NEXT: NumFunctions: 8
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
 ; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 5
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
@@ -301,6 +336,8 @@ define void @imp_null_check_store(i32* %x) {
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
 ; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 3
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
+; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 4
+; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
 ; OBJDUMP-NEXT: Fault kind: FaultingStore, faulting PC offset: 0, handling PC offset: 7
 ; OBJDUMP-NEXT: FunctionAddress: 0x000000, NumFaultingPCs: 1
 ; OBJDUMP-NEXT: Fault kind: FaultingLoad, faulting PC offset: 0, handling PC offset: 11