[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka requested changes to this revision.
vitalybuka added a comment.
This revision now requires changes to proceed.

Abandon?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[luxufan via Phabricator via cfe-commits]

StephenFan added a comment.

In D129448#3669296 , @vitalybuka 
wrote:

> I am not sure what to do with this patch. I really prefer to avoid it to 
> minimize the risk of regressions.
>
> We probably considered/prototyped to insert starts like in this solution then 
> in 2016. But at the time of the solution was a comment by @rsmith 
> https://reviews.llvm.org/D24693#559609
> Even if it works for asan, I still don't know how this patch will affect 
> miss-compiles in other places. Dropping intrinsics completely as is must not 
> miscompile.
>
> If the goal to fix asan, then it's extremely rare case, I measured it on llvm 
> codebase and entire Google code. This is not worth of effort to me.
> Also HWAsan and MTE, which can be considered asan successors, does not even 
> bother to handle allocas with multiple lifetime start.

Ok. I'm just fixing this out of interest, and it doesn't look like it's a rigid 
demand to fix it. If this patch may cause potential regression, I respect your 
decision.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

I am not sure what to do with this patch. I really prefer to avoid it to 
minimize the risk of regressions.

We probably considered/prototyped to insert starts like in this solution then 
in 2016. But at the time of the solution was a comment by @rsmith 
https://reviews.llvm.org/D24693#559609
Even if it works for asan, I still don't know how this patch will affect 
miss-compiles in other places. Dropping intrinsics completely as is must not 
miscompile.

If the goal to fix asan, then it's extremely rare case, I measured it on llvm 
codebase and entire Google code. This is not worth of effort to me.
Also HWAsan and MTE, which can be considered asan successors, does not even 
bother to handle allocas with multiple lifetime start.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[luxufan via Phabricator via cfe-commits]

StephenFan added inline comments.



Comment at: clang/test/CodeGen/lifetime2.c:42
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
+// O2: @llvm.lifetime.end.p0i8(i64 1

StephenFan wrote:
> vitalybuka wrote:
> > It assume this will break Msan 
> > Transforms/Instrumentation/MemorySanitizer.cpp:1298 as it assume variable 
> > is not initialized on start
> > 
> > ```
> > void goto_bypass(void) {
> >   {
> > char x;
> >   l1:
> > bar(, 1);
> >if (x)
> >  goto l1
> >   }
> >   goto l1;
> > }
> > ```
> Yes. I still need some time to see how to deal with it.
Candidate solution: D129991


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[luxufan via Phabricator via cfe-commits]

StephenFan added inline comments.



Comment at: clang/test/CodeGen/lifetime2.c:78
 break;
   case 2:
 bar(, 1);

vitalybuka wrote:
> vitalybuka wrote:
> > StephenFan wrote:
> > > vitalybuka wrote:
> > > > vitalybuka wrote:
> > > > > Please check for lifetime markers, I assume case 2 will have a new one
> > > > > Please check for lifetime markers, I assume case 2 will have a new one
> > > > Please check for *all* lifetime markers
> > > > 
> > > > you can add use "FileCheck --implicit-check-not llvm.lifetime" so it 
> > > > will fail if something has no corresponding match
> > > > 
> > > I have checked for all lifetime markers in `Diff 444701`. What's the 
> > > point of adding `--implicit-check-not llvm.lifetime`?
> > Point is to have en error, if lifetime emitted, but we don't have a check 
> > for that.
> > Which is very reasonable for the test.
> actually converting the test into --implicit-check-not in a separate patch 
> could be nice, so we would be be confused by previously missed markers. 
> https://reviews.llvm.org/D129789
Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[luxufan via Phabricator via cfe-commits]

StephenFan updated this revision to Diff 445420.
StephenFan added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

1. Rebase.
2. Add a test case of goto_bypass.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/lifetime2.c
  llvm/test/Instrumentation/MemorySanitizer/alloca.ll

Index: llvm/test/Instrumentation/MemorySanitizer/alloca.ll
===
--- llvm/test/Instrumentation/MemorySanitizer/alloca.ll
+++ llvm/test/Instrumentation/MemorySanitizer/alloca.ll
@@ -209,7 +209,42 @@
 ; KMSAN-NOT: call void @__msan_poison_alloca(i8* {{.*}}, i64 4,
 ; CHECK: call void @llvm.lifetime.end
 
+define void @goto_bypass() sanitize_memory {
+entry:
+  %x = alloca i8, align 1
+  call void @llvm.lifetime.start.p0i8(i64 1, i8* %x)
+  br label %l1
+
+l1:
+  call void @llvm.lifetime.start.p0i8(i64 1, i8* %x)
+  call void @bar(i8* noundef %x, i32 noundef 1)
+  %0 = load i8, i8* %x
+  %tobool = icmp ne i8 %0, 0
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:
+  br label %l1
+
+if.end:
+  call void @llvm.lifetime.end.p0i8(i64 1, i8* %x)
+  br label %l1
+}
 
-
+; CHECK-LABEL: define void @goto_bypass(
+; CHECK-LABEL: entry:
+; CHECK: %x = alloca i8
+; INLINE: call void @llvm.memset.p0i8.i64(i8* align 1 {{.*}}, i8 -1, i64 1, i1 false)
+; CALL: call void @__msan_poison_stack(i8* {{.*}}, i64 1)
+; ORIGIN: call void @__msan_set_alloca_origin4(i8* {{.*}}, i64 1,
+; KMSAN: call void @__msan_poison_alloca(i8* {{.*}}, i64 1,
+
+; FIXME: It is invalid to instrument here Since variable x may be initialized by callee bar.
+; CHECK-LABEL: l1:
+; INLINE: call void @llvm.memset.p0i8.i64(i8* align 1 {{.*}}, i8 -1, i64 1, i1 false)
+; CALL: call void @__msan_poison_stack(i8* {{.*}}, i64 1)
+; ORIGIN: call void @__msan_set_alloca_origin4(i8* {{.*}}, i64 1,
+; KMSAN: call void @__msan_poison_alloca(i8* {{.*}}, i64 1,
+
+declare void @bar(i8* noundef, i32 noundef)
 declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
Index: clang/test/CodeGen/lifetime2.c
===
--- clang/test/CodeGen/lifetime2.c
+++ clang/test/CodeGen/lifetime2.c
@@ -35,8 +35,11 @@
 void goto_bypass(void) {
   {
 char x;
+// O2: @llvm.lifetime.start.p0i8(i64 1
   l1:
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
+// O2: @llvm.lifetime.end.p0i8(i64 1
   }
   goto l1;
 }
@@ -67,21 +70,30 @@
   case 1:
 n = n;
 char x;
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
 break;
   case 2:
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
 break;
+// O2: @llvm.lifetime.end.p0i8(i64 1
   }
 }
 
 // CHECK-LABEL: @indirect_jump
 void indirect_jump(int n) {
   char x;
+  // O2: @llvm.lifetime.start.p0i8(i64 1
   void *T[] = {&};
+  // O2: @llvm.lifetime.start.p0i8(i64 8
   goto *T[n];
 L:
+  // O2: @llvm.lifetime.start.p0i8(i64 1
+  // O2: @llvm.lifetime.start.p0i8(i64 8
   bar(, 1);
+  // O2: @llvm.lifetime.end.p0i8(i64 8
+  // O2: @llvm.lifetime.end.p0i8(i64 1
 }
 
 extern void foo2(int p);
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -38,6 +38,7 @@
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
@@ -1926,6 +1927,14 @@
   /// The current lexical scope.
   LexicalScope *CurLexicalScope = nullptr;
 
+  /// Map from lexical socpe to vector of bypassed lifetime start markers.
+  /// NOTE: If a lifetime start marker isn't defined in any lexical scope(
+  /// defining while CurLexicalScope is nullptr), its key is nullptr;
+  std::map>
+  BypassedLifetimeStartMarkers;
+
+  void restartBypassedVariable();
+
   /// The current source location that should be used for exception
   /// handling code.
   SourceLocation CurEHLocation;
@@ -2922,7 +2931,8 @@
   void EmitSehTryScopeBegin();
   void EmitSehTryScopeEnd();
 
-  llvm::Value *EmitLifetimeStart(llvm::TypeSize Size, llvm::Value *Addr);
+  llvm::Value *EmitLifetimeStart(llvm::TypeSize Size, llvm::Value *Addr,
+ bool isBypassed = false);
   void EmitLifetimeEnd(llvm::Value *Size, llvm::Value *Addr);
 
   llvm::Value *EmitCXXNewExpr(const CXXNewExpr *E);
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- 

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/test/CodeGen/lifetime2.c:78
 break;
   case 2:
 bar(, 1);

vitalybuka wrote:
> StephenFan wrote:
> > vitalybuka wrote:
> > > vitalybuka wrote:
> > > > Please check for lifetime markers, I assume case 2 will have a new one
> > > > Please check for lifetime markers, I assume case 2 will have a new one
> > > Please check for *all* lifetime markers
> > > 
> > > you can add use "FileCheck --implicit-check-not llvm.lifetime" so it will 
> > > fail if something has no corresponding match
> > > 
> > I have checked for all lifetime markers in `Diff 444701`. What's the point 
> > of adding `--implicit-check-not llvm.lifetime`?
> Point is to have en error, if lifetime emitted, but we don't have a check for 
> that.
> Which is very reasonable for the test.
actually converting the test into --implicit-check-not in a separate patch 
could be nice, so we would be be confused by previously missed markers. 
https://reviews.llvm.org/D129789


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/test/CodeGen/lifetime2.c:78
 break;
   case 2:
 bar(, 1);

StephenFan wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > Please check for lifetime markers, I assume case 2 will have a new one
> > > Please check for lifetime markers, I assume case 2 will have a new one
> > Please check for *all* lifetime markers
> > 
> > you can add use "FileCheck --implicit-check-not llvm.lifetime" so it will 
> > fail if something has no corresponding match
> > 
> I have checked for all lifetime markers in `Diff 444701`. What's the point of 
> adding `--implicit-check-not llvm.lifetime`?
Point is to have en error, if lifetime emitted, but we don't have a check for 
that.
Which is very reasonable for the test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[luxufan via Phabricator via cfe-commits]

StephenFan added inline comments.



Comment at: clang/test/CodeGen/lifetime2.c:42
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
+// O2: @llvm.lifetime.end.p0i8(i64 1

vitalybuka wrote:
> It assume this will break Msan 
> Transforms/Instrumentation/MemorySanitizer.cpp:1298 as it assume variable is 
> not initialized on start
> 
> ```
> void goto_bypass(void) {
>   {
> char x;
>   l1:
> bar(, 1);
>if (x)
>  goto l1
>   }
>   goto l1;
> }
> ```
Yes. I still need some time to see how to deal with it.



Comment at: clang/test/CodeGen/lifetime2.c:78
 break;
   case 2:
 bar(, 1);

vitalybuka wrote:
> vitalybuka wrote:
> > Please check for lifetime markers, I assume case 2 will have a new one
> > Please check for lifetime markers, I assume case 2 will have a new one
> Please check for *all* lifetime markers
> 
> you can add use "FileCheck --implicit-check-not llvm.lifetime" so it will 
> fail if something has no corresponding match
> 
I have checked for all lifetime markers in `Diff 444701`. What's the point of 
adding `--implicit-check-not llvm.lifetime`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[luxufan via Phabricator via cfe-commits]

StephenFan updated this revision to Diff 444701.
StephenFan added a comment.

1. Record bypassed lifetime start markers in codegenfunction class instead of 
lexicalscope. Since bypassed variables may be defined when lexicalscope is 
nullptr. This can also fix test fail of use-after-scope-goto.cpp.
2. Check more lifetime.start or end intrinsics in lifetime2.c test file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/lifetime2.c

Index: clang/test/CodeGen/lifetime2.c
===
--- clang/test/CodeGen/lifetime2.c
+++ clang/test/CodeGen/lifetime2.c
@@ -35,11 +35,12 @@
 // CHECK-LABEL: @goto_bypass
 void goto_bypass(void) {
   {
-// O2-NOT: @llvm.lifetime.start.p0i8(i64 1
-// O2-NOT: @llvm.lifetime.end.p0i8(i64 1
+// O2: @llvm.lifetime.start.p0i8(i64 1
 char x;
   l1:
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
+// O2: @llvm.lifetime.end.p0i8(i64 1
   }
   goto l1;
 }
@@ -69,24 +70,26 @@
   switch (n) {
   case 1:
 n = n;
-// O2-NOT: @llvm.lifetime.start.p0i8(i64 1
-// O2-NOT: @llvm.lifetime.end.p0i8(i64 1
+// O2: @llvm.lifetime.start.p0i8(i64 1
 char x;
 bar(, 1);
 break;
   case 2:
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
 break;
+// O2: @llvm.lifetime.end.p0i8(i64 1
   }
 }
 
 // CHECK-LABEL: @indirect_jump
 void indirect_jump(int n) {
   char x;
-  // O2-NOT: @llvm.lifetime
+  // O2: @llvm.lifetime.start
   void *T[] = {&};
   goto *T[n];
 L:
+  // O2: @llvm.lifetime.start
   bar(, 1);
 }
 
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -38,6 +38,7 @@
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
@@ -1926,6 +1927,14 @@
   /// The current lexical scope.
   LexicalScope *CurLexicalScope = nullptr;
 
+  /// Map from lexical socpe to vector of bypassed lifetime start markers.
+  /// NOTE: If a lifetime start marker isn't defined in any lexical scope(
+  /// defining while CurLexicalScope is nullptr), its key is nullptr;
+  std::map>
+  BypassedLifetimeStartMarkers;
+
+  void restartBypassedVariable();
+
   /// The current source location that should be used for exception
   /// handling code.
   SourceLocation CurEHLocation;
@@ -2922,7 +2931,8 @@
   void EmitSehTryScopeBegin();
   void EmitSehTryScopeEnd();
 
-  llvm::Value *EmitLifetimeStart(llvm::TypeSize Size, llvm::Value *Addr);
+  llvm::Value *EmitLifetimeStart(llvm::TypeSize Size, llvm::Value *Addr,
+ bool isBypassed = false);
   void EmitLifetimeEnd(llvm::Value *Size, llvm::Value *Addr);
 
   llvm::Value *EmitCXXNewExpr(const CXXNewExpr *E);
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1233,6 +1233,8 @@
 EmitBranch(SkipCountBB);
   }
   EmitBlock(BB);
+  restartBypassedVariable();
+
   uint64_t CurrentCount = getCurrentProfileCount();
   incrementProfileCounter(S);
   setCurrentProfileCount(getCurrentProfileCount() + CurrentCount);
@@ -1299,6 +1301,14 @@
   return ResTy;
 }
 
+void CodeGenFunction::restartBypassedVariable() {
+  if (BypassedLifetimeStartMarkers.count(CurLexicalScope))
+llvm::for_each(BypassedLifetimeStartMarkers[CurLexicalScope],
+   [this](const llvm::CallInst *LifetimeStartMarker) {
+ Builder.Insert(LifetimeStartMarker->clone());
+   });
+}
+
 void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
const CGFunctionInfo ) {
   assert(Fn && "generating code for null Function");
@@ -1454,6 +1464,8 @@
   // a quick pass now to see if we can.
   if (!CurFn->doesNotThrow())
 TryMarkNoThrow(CurFn);
+
+  BypassedLifetimeStartMarkers.clear();
 }
 
 /// ContainsLabel - Return true if the statement contains a label in it.  If
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -646,6 +646,7 @@
   }
 
   EmitBlock(Dest.getBlock());
+  restartBypassedVariable();
 
   // Emit debug info for labels.
   if (CGDebugInfo *DI = getDebugInfo()) {
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- 

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/test/CodeGen/lifetime2.c:78
 break;
   case 2:
 bar(, 1);

vitalybuka wrote:
> Please check for lifetime markers, I assume case 2 will have a new one
> Please check for lifetime markers, I assume case 2 will have a new one
Please check for *all* lifetime markers

you can add use "FileCheck --implicit-check-not llvm.lifetime" so it will fail 
if something has no corresponding match



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:935
 SmallVector Labels;
+SmallVector LifetimeStartMarkers;
 LexicalScope *ParentScope;

LifetimeStartMarkers -> BypassedLifetimeStartMarkers
and below

if I read this correctly this is not any start marker



Comment at: clang/test/CodeGen/lifetime2.c:42
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
+// O2: @llvm.lifetime.end.p0i8(i64 1

It assume this will break Msan 
Transforms/Instrumentation/MemorySanitizer.cpp:1298 as it assume variable is 
not initialized on start

```
void goto_bypass(void) {
  {
char x;
  l1:
bar(, 1);
   if (x)
 goto l1
  }
  goto l1;
}
```



Comment at: clang/test/CodeGen/lifetime2.c:78
 break;
   case 2:
 bar(, 1);

Please check for lifetime markers, I assume case 2 will have a new one


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

looks like check-asan tests fail


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129448/new/

https://reviews.llvm.org/D129448

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129448: [CodeGen][Asan] Emit lifetime intrinsic for bypassed label

[luxufan via Phabricator via cfe-commits]

StephenFan created this revision.
StephenFan added reviewers: MaskRay, vitalybuka, rsmith.
Herald added a project: All.
StephenFan requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix https://github.com/llvm/llvm-project/issues/56356
For following test case:

  extern int bar(char *A, int n);
  void goto_bypass(void) {
{
  char x;
l1:
  bar(, 1);
}
goto l1;
  }

And using `clang -cc1 -O2 -S -emit-llvm` to compile it.

  In the past, due to the existence of bypassed label, the lifetime

intrinsic would not be generated. This was also the cause of pr56356.

  In this patch, if the variable is bypassed, we do variable

allocation, emit lifetime-start intrinsic and record the lifetime-start
intrinsic in LexicalScope. Then When emitting the bypass label, we emit
the lifetime instrinsic again to make sure the lifetime of the bypassed
variable is start again. Address sanitizer will capture these lifetime
intrinsics and instrument poison and unpoison code. Finally pr56356 can
be resolved.

Here is the new llvm-ir of the test case above.

  define dso_local void @goto_bypass() local_unnamed_addr #0 {
  entry:
%x = alloca i8, align 1
call void @llvm.lifetime.start.p0(i64 1, ptr nonnull %x) #3
br label %l1
  
  l1:   ; preds = %l1, %entry
call void @llvm.lifetime.start.p0(i64 1, ptr nonnull %x) #3
%call = call i32 @bar(ptr noundef nonnull %x, i32 noundef 1) #3
call void @llvm.lifetime.end.p0(i64 1, ptr nonnull %x) #3
br label %l1
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129448

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/lifetime2.c

Index: clang/test/CodeGen/lifetime2.c
===
--- clang/test/CodeGen/lifetime2.c
+++ clang/test/CodeGen/lifetime2.c
@@ -35,11 +35,12 @@
 // CHECK-LABEL: @goto_bypass
 void goto_bypass(void) {
   {
-// O2-NOT: @llvm.lifetime.start.p0i8(i64 1
-// O2-NOT: @llvm.lifetime.end.p0i8(i64 1
+// O2: @llvm.lifetime.start.p0i8(i64 1
 char x;
   l1:
+// O2: @llvm.lifetime.start.p0i8(i64 1
 bar(, 1);
+// O2: @llvm.lifetime.end.p0i8(i64 1
   }
   goto l1;
 }
@@ -69,8 +70,8 @@
   switch (n) {
   case 1:
 n = n;
-// O2-NOT: @llvm.lifetime.start.p0i8(i64 1
-// O2-NOT: @llvm.lifetime.end.p0i8(i64 1
+// O2: @llvm.lifetime.start.p0i8(i64 1
+// O2: @llvm.lifetime.end.p0i8(i64 1
 char x;
 bar(, 1);
 break;
@@ -83,7 +84,7 @@
 // CHECK-LABEL: @indirect_jump
 void indirect_jump(int n) {
   char x;
-  // O2-NOT: @llvm.lifetime
+  // O2: @llvm.lifetime
   void *T[] = {&};
   goto *T[n];
 L:
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -38,6 +38,7 @@
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
@@ -931,6 +932,7 @@
   class LexicalScope : public RunCleanupsScope {
 SourceRange Range;
 SmallVector Labels;
+SmallVector LifetimeStartMarkers;
 LexicalScope *ParentScope;
 
 LexicalScope(const LexicalScope &) = delete;
@@ -950,6 +952,19 @@
   Labels.push_back(label);
 }
 
+void addLifetimeStartMarker(const llvm::CallInst *LifetimeStartMarker) {
+  assert(isa(LifetimeStartMarker) &&
+ cast(LifetimeStartMarker)->getIntrinsicID() ==
+ llvm::Intrinsic::lifetime_start &&
+ "LifetimeStartMarker Is not a lifetime start intrinsic");
+  LifetimeStartMarkers.push_back(LifetimeStartMarker);
+}
+
+const SmallVector &
+getLifetimeStartMarkers() const {
+  return LifetimeStartMarkers;
+}
+
 /// Exit this cleanup scope, emitting any accumulated
 /// cleanups.
 ~LexicalScope() {
@@ -2922,7 +2937,8 @@
   void EmitSehTryScopeBegin();
   void EmitSehTryScopeEnd();
 
-  llvm::Value *EmitLifetimeStart(llvm::TypeSize Size, llvm::Value *Addr);
+  llvm::Value *EmitLifetimeStart(llvm::TypeSize Size, llvm::Value *Addr,
+ bool isBypassed = false);
   void EmitLifetimeEnd(llvm::Value *Size, llvm::Value *Addr);
 
   llvm::Value *EmitCXXNewExpr(const CXXNewExpr *E);
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -647,6 +647,12 @@
 
   EmitBlock(Dest.getBlock());
 
+  if (CurLexicalScope)
+llvm::for_each(CurLexicalScope->getLifetimeStartMarkers(),
+   [this](const llvm::CallInst *LifetimeStartMarker) {
+