[asan] Do not remove empty lifetime.start/lifetime.end ranges

[vitalybuka]

Summary

Asan stack-use-after-scope check should poison alloca even if there is
no access between start and end.

This is possible for code like this:
for (int i = 0; i < 3; i++) {

int x;
p = &x;

}

"Loop Invariant Code Motion" will move "p = &x;" out of the loop, making
start/end range empty.

PR27453

[eugenis]

lib/Transforms/InstCombine/InstCombineCalls.cpp

2246 ↗

(On Diff #65645)

code style: "F"

[vitalybuka]

naming

[majnemer]

test/Transforms/InstCombine/lifetime-asan.ll

10–11 ↗

(On Diff #65783)

Instead of alloca + gep, why not just make you alloca have type i8?

27–28 ↗

(On Diff #65783)

Ditto.

[vitalybuka]

Simplified ll test

[majnemer]

lib/Transforms/InstCombine/InstCombineCalls.cpp

2249 ↗

(On Diff #65786)

Is it really possible for Func to be null?

[vitalybuka]

lib/Transforms/InstCombine/InstCombineCalls.cpp

2249 ↗

(On Diff #65786)

I have no confident answer, but seems I can compile entire clang without null check.

[vitalybuka]

removed null check

[majnemer]

I am satisfied with this change from a code and test point of view but have no idea if this is correct behavior for the sanitizer. Can somebody with more experience on that side of things comment?

lib/Transforms/InstCombine/InstCombineCalls.cpp

2246–2248 ↗

(On Diff #65812)

I'd prefer that this check get split out from the other.

[vitalybuka]

"if" split

[eugenis]

I think this is correct from the sanitizer standpoint. It would help asan report use-after-scope for accesses after lifetime.end.

This may have performance implications (lots of poisoning/unpoisoning of locals in tight loops), but that can be fixed on the ASan side.

llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp

case Intrinsic::stackrestore: {
     // If the stack restore is in a return, resume, or unwind block and if there
     // are no allocas or calls between the restore and the return, nuke the
     // restore.
     if (!CannotRemove && (isa<ReturnInst>(TI) || isa<ResumeInst>(TI)))
       return eraseInstFromFunction(CI);
     break;
   }
   case Intrinsic::lifetime_start:
+    // Asan needs to poison memory to detect invalid access which is possible
+    // even for empty lifetime range.
+    if (II->getFunction()->hasFnAttribute(Attribute::SanitizeAddress))
+      break;
+
     if (removeTriviallyEmptyRange(*II, Intrinsic::lifetime_start,
                                   Intrinsic::lifetime_end, *this))
       return nullptr;
     break;
   case Intrinsic::assume: {
     Value *IIOperand = II->getArgOperand(0);
     // Remove an assume if it is immediately followed by an identical assume.
     if (match(II->getNextNode(),

llvm/trunk/test/Transforms/InstCombine/lifetime-asan.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+declare void @llvm.lifetime.start(i64, i8* nocapture)
+declare void @llvm.lifetime.end(i64, i8* nocapture)
+declare void @foo(i8* nocapture)
+
+define void @asan() sanitize_address {
+entry:
+  ; CHECK-LABEL: @asan(
+  %text = alloca i8, align 1
+
+  call void @llvm.lifetime.start(i64 1, i8* %text)
+  call void @llvm.lifetime.end(i64 1, i8* %text)
+  ; CHECK: call void @llvm.lifetime.start
+  ; CHECK-NEXT: call void @llvm.lifetime.end
+
+  call void @foo(i8* %text) ; Keep alloca alive
+
+  ret void
+}
+
+
+define void @no_asan() {
+entry:
+  ; CHECK-LABEL: @no_asan(
+  %text = alloca i8, align 1
+
+  call void @llvm.lifetime.start(i64 1, i8* %text)
+  call void @llvm.lifetime.end(i64 1, i8* %text)
+  ; CHECK-NO: call void @llvm.lifetime
+
+  call void @foo(i8* %text) ; Keep alloca alive
+
+  ret void
+}