[PATCH] D66686: [LifetimeAnalysis] Make it possible to disable the new warnings

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6651
   visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
-Visit);
+Visit, true);
 else

mgehre wrote:
> Could we have argument comments here `/*EnableLifetimeWarnings=*/true,
> and especially below where multiple bool literals are passed?
Done in r369928.


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

https://reviews.llvm.org/D66686



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

[PATCH] D66686: [LifetimeAnalysis] Make it possible to disable the new warnings

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6651
   visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
-Visit);
+Visit, true);
 else

Could we have argument comments here `/*EnableLifetimeWarnings=*/true,
and especially below where multiple bool literals are passed?


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

https://reviews.llvm.org/D66686



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

[PATCH] D66686: [LifetimeAnalysis] Make it possible to disable the new warnings

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun abandoned this revision.
xazax.hun added a comment.

Committed in https://reviews.llvm.org/rG6379e5c8a441 due to it was urgent for 
some users. Will address any comments post-commit.


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

https://reviews.llvm.org/D66686



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

[PATCH] D66686: [LifetimeAnalysis] Make it possible to disable the new warnings

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 216964.
xazax.hun added a comment.

- Added a test.


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

https://reviews.llvm.org/D66686

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -fsyntax-only -Wno-dangling -Wreturn-stack-address -verify %s
+
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int &operator*();
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  MyIntPointer(const MyIntOwner &);
+  int &operator*();
+  MyIntOwner toOwner();
+};
+
+int &f() {
+  int i;
+  return i; // expected-warning {{reference to stack memory associated with local variable 'i' returned}}
+}
+
+MyIntPointer g() {
+  MyIntOwner o;
+  return o; // No warning, it is disabled.
+}
\ No newline at end of file
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6553,11 +6553,13 @@
 
 static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
  Expr *Init, LocalVisitor Visit,
- bool RevisitSubinits);
+ bool RevisitSubinits,
+ bool EnableLifetimeWarnings);
 
 static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
   Expr *Init, ReferenceKind RK,
-  LocalVisitor Visit);
+  LocalVisitor Visit,
+  bool EnableLifetimeWarnings);
 
 template  static bool isRecordWithAttr(QualType Type) {
   if (auto *RD = Type->getAsCXXRecordDecl())
@@ -6646,9 +6648,9 @@
 Path.push_back({IndirectLocalPathEntry::GslPointerInit, Arg, D});
 if (Arg->isGLValue())
   visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
-Visit);
+Visit, true);
 else
-  visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
+  visitLocalsRetainedByInitializer(Path, Arg, Visit, true, true);
 Path.pop_back();
   };
 
@@ -6723,9 +6725,9 @@
 Path.push_back({IndirectLocalPathEntry::LifetimeBoundCall, Arg, D});
 if (Arg->isGLValue())
   visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
-Visit);
+Visit, false);
 else
-  visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
+  visitLocalsRetainedByInitializer(Path, Arg, Visit, true, false);
 Path.pop_back();
   };
 
@@ -6744,7 +6746,8 @@
 /// glvalue expression \c Init.
 static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
   Expr *Init, ReferenceKind RK,
-  LocalVisitor Visit) {
+  LocalVisitor Visit,
+  bool EnableLifetimeWarnings) {
   RevertToOldSizeRAII RAII(Path);
 
   // Walk past any constructs which we can lifetime-extend across.
@@ -6781,7 +6784,8 @@
   else
 // We can't lifetime extend through this but we might still find some
 // retained temporaries.
-return visitLocalsRetainedByInitializer(Path, Init, Visit, true);
+return visitLocalsRetainedByInitializer(Path, Init, Visit, true,
+EnableLifetimeWarnings);
 }
 
 // Step into CXXDefaultInitExprs so we can diagnose cases where a
@@ -6796,11 +6800,12 @@
   if (auto *MTE = dyn_cast(Init)) {
 if (Visit(Path, Local(MTE), RK))
   visitLocalsRetainedByInitializer(Path, MTE->GetTemporaryExpr(), Visit,
-   true);
+   true, EnableLifetimeWarnings);
   }
 
   if (isa(Init)) {
-handleGslAnnotatedTypes(Path, Init, Visit);
+if (EnableLifetimeWarnings)
+  handleGslAnnotatedTypes(Path, Init, Visit);
 return visitLifetimeBoundArguments(Path, Init, Visit);
   }
 
@@ -6821,7 +6826,8 @@
   } else if (VD->getInit() && !isVarOnPath(Path, VD)) {
 Path.push_back({IndirectLocalPathEntry::VarInit, DRE, VD});
 visitLocalsRetainedByReferenceBinding(Path, VD->getInit(),
- 

[PATCH] D66686: [LifetimeAnalysis] Make it possible to disable the new warnings

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 216962.
xazax.hun added a comment.

- Add the actual diff.


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

https://reviews.llvm.org/D66686

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp

Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6553,11 +6553,13 @@
 
 static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
  Expr *Init, LocalVisitor Visit,
- bool RevisitSubinits);
+ bool RevisitSubinits,
+ bool EnableLifetimeWarnings);
 
 static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
   Expr *Init, ReferenceKind RK,
-  LocalVisitor Visit);
+  LocalVisitor Visit,
+  bool EnableLifetimeWarnings);
 
 template  static bool isRecordWithAttr(QualType Type) {
   if (auto *RD = Type->getAsCXXRecordDecl())
@@ -6646,9 +6648,9 @@
 Path.push_back({IndirectLocalPathEntry::GslPointerInit, Arg, D});
 if (Arg->isGLValue())
   visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
-Visit);
+Visit, true);
 else
-  visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
+  visitLocalsRetainedByInitializer(Path, Arg, Visit, true, true);
 Path.pop_back();
   };
 
@@ -6723,9 +6725,9 @@
 Path.push_back({IndirectLocalPathEntry::LifetimeBoundCall, Arg, D});
 if (Arg->isGLValue())
   visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
-Visit);
+Visit, false);
 else
-  visitLocalsRetainedByInitializer(Path, Arg, Visit, true);
+  visitLocalsRetainedByInitializer(Path, Arg, Visit, true, false);
 Path.pop_back();
   };
 
@@ -6744,7 +6746,8 @@
 /// glvalue expression \c Init.
 static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
   Expr *Init, ReferenceKind RK,
-  LocalVisitor Visit) {
+  LocalVisitor Visit,
+  bool EnableLifetimeWarnings) {
   RevertToOldSizeRAII RAII(Path);
 
   // Walk past any constructs which we can lifetime-extend across.
@@ -6781,7 +6784,8 @@
   else
 // We can't lifetime extend through this but we might still find some
 // retained temporaries.
-return visitLocalsRetainedByInitializer(Path, Init, Visit, true);
+return visitLocalsRetainedByInitializer(Path, Init, Visit, true,
+EnableLifetimeWarnings);
 }
 
 // Step into CXXDefaultInitExprs so we can diagnose cases where a
@@ -6796,11 +6800,12 @@
   if (auto *MTE = dyn_cast(Init)) {
 if (Visit(Path, Local(MTE), RK))
   visitLocalsRetainedByInitializer(Path, MTE->GetTemporaryExpr(), Visit,
-   true);
+   true, EnableLifetimeWarnings);
   }
 
   if (isa(Init)) {
-handleGslAnnotatedTypes(Path, Init, Visit);
+if (EnableLifetimeWarnings)
+  handleGslAnnotatedTypes(Path, Init, Visit);
 return visitLifetimeBoundArguments(Path, Init, Visit);
   }
 
@@ -6821,7 +6826,8 @@
   } else if (VD->getInit() && !isVarOnPath(Path, VD)) {
 Path.push_back({IndirectLocalPathEntry::VarInit, DRE, VD});
 visitLocalsRetainedByReferenceBinding(Path, VD->getInit(),
-  RK_ReferenceBinding, Visit);
+  RK_ReferenceBinding, Visit,
+  EnableLifetimeWarnings);
   }
 }
 break;
@@ -6833,13 +6839,15 @@
 // handling all sorts of rvalues passed to a unary operator.
 const UnaryOperator *U = cast(Init);
 if (U->getOpcode() == UO_Deref)
-  visitLocalsRetainedByInitializer(Path, U->getSubExpr(), Visit, true);
+  visitLocalsRetainedByInitializer(Path, U->getSubExpr(), Visit, true,
+   EnableLifetimeWarnings);
 break;
   }
 
   case Stmt::OMPArraySectionExprClass: {
-visitLocalsRetainedByInitializer(
-Path, cast(Init)->getBase(), Visit, true);
+visitLocalsRetainedByInitializer(Path,
+ cast(Init)->getBase(),
+ 

[PATCH] D66686: [LifetimeAnalysis] Make it possible to disable the new warnings

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: rsmith, mgehre.
xazax.hun added a project: clang.
Herald added subscribers: llvm-commits, Szelethus, Charusso, gamesh411, jfb, 
dkrupp, rnkovacs, hiraditya, javed.absar.
Herald added a project: LLVM.

This patch introduces quite a bit of plumbing, but I took this approach because 
it seems to be the safest, do not run any related code at all when the warnings 
are turned off.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66686

Files:
  llvm/lib/Target/AArch64/AArch64InstrFormats.td
  llvm/lib/Target/AArch64/AArch64InstructionSelector.cpp
  llvm/test/CodeGen/AArch64/GlobalISel/load-addressing-modes.mir
  llvm/test/CodeGen/AArch64/GlobalISel/store-addressing-modes.mir
  llvm/test/CodeGen/AArch64/arm64-fastisel-gep-promote-before-add.ll

Index: llvm/test/CodeGen/AArch64/arm64-fastisel-gep-promote-before-add.ll
===
--- llvm/test/CodeGen/AArch64/arm64-fastisel-gep-promote-before-add.ll
+++ llvm/test/CodeGen/AArch64/arm64-fastisel-gep-promote-before-add.ll
@@ -1,6 +1,6 @@
 ; fastisel should not fold add with non-pointer bitwidth
 ; sext(a) + sext(b) != sext(a + b)
-; RUN: llc -mtriple=arm64-apple-darwin %s -O0 -o - | FileCheck %s
+; RUN: llc -fast-isel -mtriple=arm64-apple-darwin %s -O0 -o - | FileCheck %s
 
 define zeroext i8 @gep_promotion(i8* %ptr) nounwind uwtable ssp {
 entry:
Index: llvm/test/CodeGen/AArch64/GlobalISel/store-addressing-modes.mir
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/GlobalISel/store-addressing-modes.mir
@@ -0,0 +1,168 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=aarch64-unknown-unknown -run-pass=instruction-select -verify-machineinstrs %s -o - | FileCheck %s
+
+--- |
+  define void @strxrox(i64* %addr) { ret void }
+  define void @strdrox(i64* %addr) { ret void }
+  define void @strwrox(i64* %addr) { ret void }
+  define void @strsrox(i64* %addr) { ret void }
+  define void @strhrox(i64* %addr) { ret void }
+  define void @strqrox(i64* %addr) { ret void }
+  define void @shl(i64* %addr) { ret void }
+...
+
+---
+name:strxrox
+alignment:   2
+legalized:   true
+regBankSelected: true
+tracksRegLiveness: true
+machineFunctionInfo: {}
+body: |
+  bb.0:
+liveins: $x0, $x1, $x2
+; CHECK-LABEL: name: strxrox
+; CHECK: liveins: $x0, $x1, $x2
+; CHECK: [[COPY:%[0-9]+]]:gpr64sp = COPY $x0
+; CHECK: [[COPY1:%[0-9]+]]:gpr64 = COPY $x1
+; CHECK: [[COPY2:%[0-9]+]]:gpr64 = COPY $x2
+; CHECK: STRXroX [[COPY2]], [[COPY]], [[COPY1]], 0, 0 :: (store 8 into %ir.addr)
+%0:gpr(p0) = COPY $x0
+%1:gpr(s64) = COPY $x1
+%ptr:gpr(p0) = G_GEP %0, %1
+%3:gpr(s64) = COPY $x2
+G_STORE %3, %ptr :: (store 8 into %ir.addr)
+...
+---
+name:strdrox
+alignment:   2
+legalized:   true
+regBankSelected: true
+tracksRegLiveness: true
+machineFunctionInfo: {}
+body: |
+  bb.0:
+liveins: $x0, $x1, $d2
+; CHECK-LABEL: name: strdrox
+; CHECK: liveins: $x0, $x1, $d2
+; CHECK: [[COPY:%[0-9]+]]:gpr64sp = COPY $x0
+; CHECK: [[COPY1:%[0-9]+]]:gpr64 = COPY $x1
+; CHECK: [[COPY2:%[0-9]+]]:fpr64 = COPY $d2
+; CHECK: STRDroX [[COPY2]], [[COPY]], [[COPY1]], 0, 0 :: (store 8 into %ir.addr)
+%0:gpr(p0) = COPY $x0
+%1:gpr(s64) = COPY $x1
+%ptr:gpr(p0) = G_GEP %0, %1
+%3:fpr(s64) = COPY $d2
+G_STORE %3, %ptr :: (store 8 into %ir.addr)
+...
+---
+name:strwrox
+alignment:   2
+legalized:   true
+regBankSelected: true
+tracksRegLiveness: true
+machineFunctionInfo: {}
+body: |
+  bb.0:
+liveins: $x0, $x1, $w2
+; CHECK-LABEL: name: strwrox
+; CHECK: liveins: $x0, $x1, $w2
+; CHECK: [[COPY:%[0-9]+]]:gpr64sp = COPY $x0
+; CHECK: [[COPY1:%[0-9]+]]:gpr64 = COPY $x1
+; CHECK: [[COPY2:%[0-9]+]]:gpr32 = COPY $w2
+; CHECK: STRWroX [[COPY2]], [[COPY]], [[COPY1]], 0, 0 :: (store 4 into %ir.addr)
+%0:gpr(p0) = COPY $x0
+%1:gpr(s64) = COPY $x1
+%ptr:gpr(p0) = G_GEP %0, %1
+%3:gpr(s32) = COPY $w2
+G_STORE %3, %ptr :: (store 4 into %ir.addr)
+...
+---
+name:strsrox
+alignment:   2
+legalized:   true
+regBankSelected: true
+tracksRegLiveness: true
+machineFunctionInfo: {}
+body: |
+  bb.0:
+liveins: $x0, $x1, $s2
+; CHECK-LABEL: name: strsrox
+; CHECK: liveins: $x0, $x1, $s2
+; CHECK: [[COPY:%[0-9]+]]:gpr64sp = COPY $x0
+; CHECK: [[COPY1:%[0-9]+]]:gpr64 = COPY $x1
+; CHECK: [[COPY2:%[0-9]+]]:fpr32 = COPY $s2
+; CHECK: STRSroX [[COPY2]], [[COPY]], [[COPY1]], 0, 0 :: (store 4 into %ir.addr)
+%0:gpr(p0) = COPY $x0
+%1:gpr(s64) = COPY $x1
+%ptr:gpr(p0) = G_GEP %0, %1
+%3:fpr(s32) = COPY $s2
+G_STORE %3, %ptr :: (store 4 into %ir.addr)
+...
+---
+name:strhrox
+alignment: