[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb7bdf1996fd1: [-Wunsafe-buffer-usage] Hide 
fixits/suggestions behind an extra flag. (authored by Artem Dergachev 
).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-crashes.c
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-no-fixits.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-misuse.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-source-ranges.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20  -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -frtti -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,66 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-fsaf

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 523601.
NoQ added a comment.

Rebase *correctly*!


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

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-crashes.c
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-unevaluated-context.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-no-fixits.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-misuse.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-source-ranges.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-warning-unevaluated-context.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20  -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -frtti -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,66 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// Passing through -Xclang.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// -Xclang

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 523595.
NoQ added a comment.

Rebase!


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

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-crashes.c
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-no-fixits.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-misuse.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-source-ranges.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,66 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// Passing through -Xclang.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// -Xclang flags take precedence over driver flags.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+[[clang::unsafe_buffer_usage]] void bar(int *);
+
+void foo(int *x) { // \
+  // ON-warning{{'x' is an unsafe pointer used for buffer access}}
+  // FIXME: Better "OFF" warning?
+  x[5] = 10; // \
+  // ON-note{{used in buffer access here}} \
+  // OFF-warning{{unsafe buffer access}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+
+  x += 5;  // \
+  // ON-note{{used in pointer arithmetic here}} \
+  // OFF-warnin

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 515110.
NoQ added a comment.

Before I forget: Update tests that didn't fail due to my patch because they 
were testing absence of things (warnings or fixits) and my patch only made them 
more absent. They need to keep testing absence of these things even when the 
flag is passed.


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

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-crashes.c
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-no-fixits.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-misuse.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-source-ranges.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,66 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// Passing through -Xclang.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// -Xclang flags take precedence over driver flags.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+[[clang::unsafe_buffer_usage]] void bar(int *);
+
+void foo(int *x) { // \
+  // ON-warning{{'x' is an unsafe pointer used for buffer access}}
+  // FIXME: Better "OFF" warning?
+  x[5] = 10; // \
+  // ON-note{{used in buffer ac

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 510933.
NoQ added a comment.

- Rebase! (I'll update related revisions soon but not immediately, need to make 
sense out of them first.)
- Eliminate the `EmitFixits` mode as discussed above.


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

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-addressof-arraysubscript.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pre-increment.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,66 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// Passing through -Xclang.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// -Xclang flags take precedence over driver flags.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+[[clang::unsafe_buffer_usage]] void bar(int *);
+
+void foo(int *x) { // \
+  // ON-warning{{'x' is an unsafe pointer used for buffer access}}
+  // FIXME: Better "OFF" warning?
+  x[5] = 10; // \
+  // ON-note{{used in buffer access here}} \
+  // OFF-warning{{unsafe buffer access}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+
+  x += 5;  // \
+  // ON-note{{used in pointer arithmetic here}} \
+  // OFF-warning{{unsafe pointer arithmetic}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

So in a nutshell, this is the intended behavior:

| code | diagnostic 
 | EmitFixits && EmitSuggestions | !EmitFixits && 
EmitSuggestions | !EmitFixits && !EmitSuggestions |
|  | 
---
 | - | -- | 
--- |
| `void foo(int *p)` { | note: 'p' declared here
 | yes   | yes  
  | no  |
| `  int *q;`  | warning: 'q' is unsafe pointer to raw buffer   
 | yes   | yes  
  | no  |
|  | note with fixit: change type of 'q' and 'p' to span to 
propagate bounds information | yes   | yes (no fixit)   
  | no  |
| `  q = p;`   | note: 'bounds information propagates from 'p' to 'q' 
here   | yes   | yes
| no  |
| `  q[5] = 10;`   | note: unsafe raw buffer operation here 
 | yes   | yes  
  | yes (warning)   |
|  | note: pass -fsafe-buffer-usage-suggestions to receive 
code hardening suggestions| no| no  
   | yes |
| `}`  |
 |   |  
  | |
|

The first mode, "**EmitFixits && EmitSuggestions**", is the default behavior 
before this patch, and the behavior under `-fsafe-buffer-usage-suggestions` 
after this patch. The third mode, "**!EmitFixits && !EmitSuggestions**", is the 
default behavior after the patch.

The second mode, "**!EmitFixits && EmitSuggestions**" (the special behavior 
under `-fno-diagnostics-fixit-info`) was implemented for two purposes:

- Recover some performance when `-fno-diagnostics-fixit-info` is passed (don't 
run the sophisticated fixit machinery if fixits are going to be discarded);
- As a possible workaround for potential crashes in the fixit machinery.

We initially hoped that this mode will make "**!EmitSuggestions**" unnecessary, 
but as the table above demonstrates, a very large portion of the fixit 
machine's logic is still being invoked for the purposes of building notes, even 
when fixits aren't attached. And we cannot really disable this logic under that 
compiler flag, given that  `-fno-diagnostics-fixit-info` already has a 
well-defined meaning which doesn't allow us to change the shape of the 
diagnostics in response. So we needed a new, more powerful flag, so here we 
are. I suspect that explicit support for `!EmitFixits` is no longer necessary. 
It is now useful for performance purposes only, and we don't any concrete data 
to support the claim that it's a valuable optimization, nor do we believe that 
this flag is ever intentionally passed in practice, so I guess I'll go ahead 
and remove it.


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

https://reviews.llvm.org/D146669

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

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 507912.
NoQ added a comment.

Don't recommend enabling suggestions when suggestions are impossible (eg. due 
to lack of C++20).


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

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,66 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// Passing through -Xclang.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// -Xclang flags take precedence over driver flags.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+[[clang::unsafe_buffer_usage]] void bar(int *);
+
+void foo(int *x) { // \
+  // ON-warning{{'x' is an unsafe pointer used for buffer access}}
+  // FIXME: Better "OFF" warning?
+  x[5] = 10; // \
+  // ON-note{{used in buffer access here}} \
+  // OFF-warning{{unsafe buffer access}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+
+  x += 5;  // \
+  // ON-note{{used in pointer arithmetic here}} \
+  // OFF-warning{{unsafe pointer arithmetic}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+
+  bar(x);  // \
+  // ON-warning{{function introduces unsafe buffer manipulation}} \
+  // OFF-warning{{function introduces unsafe buffer manipulation}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
===

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11792
   "change type of '%0' to '%select{std::span|std::array|std::span::iterator}1' 
to preserve bounds information">;
+def note_safe_buffer_usage_suggestions_disabled : Note<
+  "pass -fsafe-buffer-usage-suggestions to receive code hardening 
suggestions">;

jkorous wrote:
> I like this idea!
> 
> Thinking about the questions you raised:
> > Do we really need that? Maybe we should somehow throttle it to reduce noise?
> 
> The only case I can see value in not emitting this note is when the user 
> passes `-fno-safe-buffer-usage-suggestions`. In that case it can be argued 
> that the user is well aware of the flag since they explicitly turned it off.
> 
> That being said I am fine with the current behavior and in either case expect 
> we will tweak this once we get feedback from the users.
> The only case I can see value in not emitting this note is when the user 
> passes -fno-safe-buffer-usage-suggestions. In that case it can be argued that 
> the user is well aware of the flag since they explicitly turned it off.

Yeah fair enough. This will need some rework though, because right now 
`-fno-...` is purely a Driver flag, so `-cc1` invocations aren't even aware of 
it being passed this way. So it turns from a normal flag to a somewhat quirky 
flag.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2159
   Sema &S;
+  bool SuggestSuggestions;
 

t-rasmud wrote:
> Was there a reason for naming this variable SuggestSuggestions? Can this be 
> called EmitSuggestions? I think that would make it uniform and easy to follow 
> the code.
It means "Should I emit a note reminding the user to enable 
`-fsafe-buffer-usage-suggestions` if they want suggestions?", so it's quite 
different from `EmitSuggestions` (in fact it's the opposite). Added a comment.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2203
+if (IsRelatedToDecl) {
+  assert(!SuggestSuggestions &&
+ "Variables blamed for unsafe buffer usage without suggestions!");

ziqingluo-90 wrote:
> nitpick: 
> I was a bit confused by the name of the variable.  My understand of 
> `!SuggestSuggestions ` here means "we are suggesting suggestions now".  
> Then what does `SuggestSuggestions ` mean?
Yeah it means "Should I emit a note reminding the user to enable 
`-fsafe-buffer-usage-suggestions` if they want suggestions?". Added a comment 
to the variable.


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

https://reviews.llvm.org/D146669

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

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 507903.
NoQ added a comment.

Explain the weird variable name.

Confirm that even though `warn_unsafe_buffer_operation` and 
`note_unsafe_buffer_operation` have a different number of modes, the mismatch 
doesn't cause problems yet. Add an assert to catch the problem when it starts 
happening. Add tests to confirm that messages make sense.


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

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,66 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// Passing through -Xclang.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// -Xclang flags take precedence over driver flags.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+[[clang::unsafe_buffer_usage]] void bar(int *);
+
+void foo(int *x) { // \
+  // ON-warning{{'x' is an unsafe pointer used for buffer access}}
+  // FIXME: Better "OFF" warning?
+  x[5] = 10; // \
+  // ON-note{{used in buffer access here}} \
+  // OFF-warning{{unsafe buffer access}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+
+  x += 5;  // \
+  // ON-note{{used in pointer arithmetic here}} \
+  // OFF-warning{{unsafe pointer arithmetic}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+
+  bar(x);  // \
+  // ON-warning{{function introduces unsafe buffer manipulation}} \
+  // OFF-warning{{fu

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Rashmi Mudduluru via Phabricator via cfe-commits]

t-rasmud added inline comments.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2159
   Sema &S;
+  bool SuggestSuggestions;
 

Was there a reason for naming this variable SuggestSuggestions? Can this be 
called EmitSuggestions? I think that would make it uniform and easy to follow 
the code.


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

https://reviews.llvm.org/D146669

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

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2203
+if (IsRelatedToDecl) {
+  assert(!SuggestSuggestions &&
+ "Variables blamed for unsafe buffer usage without suggestions!");

nitpick: 
I was a bit confused by the name of the variable.  My understand of 
`!SuggestSuggestions ` here means "we are suggesting suggestions now".  
Then what does `SuggestSuggestions ` mean?


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

https://reviews.llvm.org/D146669

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

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1139
UnsafeBufferUsageHandler &Handler,
+   bool EmitSuggestions,
bool EmitFixits) {

nitpick: I'm afraid that we will forgot the difference between 
`EmitSuggestions` and `EmitFixits` later if there is no document about it.


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

https://reviews.llvm.org/D146669

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

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Jan Korous via Phabricator via cfe-commits]

jkorous accepted this revision.
jkorous added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11792
   "change type of '%0' to '%select{std::span|std::array|std::span::iterator}1' 
to preserve bounds information">;
+def note_safe_buffer_usage_suggestions_disabled : Note<
+  "pass -fsafe-buffer-usage-suggestions to receive code hardening 
suggestions">;

I like this idea!

Thinking about the questions you raised:
> Do we really need that? Maybe we should somehow throttle it to reduce noise?

The only case I can see value in not emitting this note is when the user passes 
`-fno-safe-buffer-usage-suggestions`. In that case it can be argued that the 
user is well aware of the flag since they explicitly turned it off.

That being said I am fine with the current behavior and in either case expect 
we will tweak this once we get feedback from the users.


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

https://reviews.llvm.org/D146669

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

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 507549.
NoQ added a comment.

Add some nice assertions.


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

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,54 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// Passing through -Xclang.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+// -Xclang flags take precedence over driver flags.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+
+void foo(int *x) { // \
+  // ON-warning{{'x' is an unsafe pointer used for buffer access}}
+  // FIXME: Better "OFF" warning?
+  x[5] = 10; // \
+  // ON-note{{used in buffer access here}} \
+  // OFF-warning{{unsafe buffer access}} \
+  // OFF-note   {{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+}
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -Wno-unused-value -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Wno-unused-value -verify %s
 
 void basic(int * x) {// expected-warning{{'x' is an unsafe pointer used for buffer access}}
   int *p1 = new int[10]; // not to warn
Index: clang/test

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: jkorous, t-rasmud, ziqingluo-90, malavikasamak, 
aaron.ballman, gribozavr, ymandel, sgatev.
Herald added subscribers: steakhal, martong.
Herald added a project: All.
NoQ requested review of this revision.
Herald added a subscriber: MaskRay.

This patch implements a new clang driver flag `-fsafe-buffer-usage-suggestions` 
which allows turning the smart suggestion machine on and off (defaults to off). 
This is valuable for stability reasons, as the machine is being rapidly 
improved and we don't want accidental breakages to ruin the build for innocent 
users. It is also arguably useful in general because it enables separation of 
concerns between project contributors: some users will actively update the code 
to conform to the programming model, while others simply want to make sure that 
they aren't regressing it. Finally, there could be other valid reasons to opt 
out of suggestions entirely on some codebases (while continuing to enforce 
`-Wunsafe-buffer-usage` warnings), such as lack of access to hardened libc++ 
(or even to the C++ standard library in general) on the target platform.

When the flag is disabled, the unsafe buffer usage analysis is reduced to an 
extremely minimal mode of operation that contains virtually no smarts: not only 
it doesn't offer automatic fixits, but also textual suggestions such as "change 
the type of this variable to `std::span` to preserve bounds information" are 
not displayed*, and in fact the machine doesn't even try to blame specific 
variables in the first place, it simply warns on the operations and leaves 
everything else to the user. So this flag turns off a lot more of our complex 
machinery than what we already turn off in presence of say 
`-fno-diagnostic-fixit-info`.

The flag is discoverable: when it's off, the warnings are accompanied by a 
`note:` telling the user that there's a flag they can use. (Do we really need 
that? Maybe we should somehow throttle it to reduce noise?)


Repository:
  rC Clang

https://reviews.llvm.org/D146669

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticOptions.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  
clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-assign-to-array-subscr-on-ptr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-deref-simple-ptr-arith.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma-fixit.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-pragma.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fblocks -include %s -verify %s
 
 // RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
 // RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-suggestions-flag.cpp
@@ -0,0 +1,54 @@
+// Test the -cc1 flag. There's no -fno-... option in -cc1 invocations,
+// just the positive option.
+
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=OFF %s
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify=ON %s \
+// RUN:-fsafe-buffer-usage-suggestions
+
+// Test driver flags. Now there's both -f... and -fno-... to worry about.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=ON %s
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+
+// In case of driver flags, last flag takes precedence.
+
+// RUN: %clang -fsyntax-only -std=c++20 -Wunsafe-buffer-usage \
+// RUN:-fsafe-buffer-usage-suggestions \
+// RUN:-fno-safe-buffer-usage-suggestions \
+// RUN:-Xclang -verify=OFF %s
+// RUN: %cla