[PATCH] D140179: [WIP][-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added inline comments.


================
Comment at: clang/include/clang/Basic/Diagnostic.h:1040-1043
+  // An ordered sequence of "-Wunsafe-buffer-usage" opt-out regions in one
+  // translation unit. Each region is represented by a pair of start and end
+  // locations.
+  SmallVector<std::pair<SourceLocation, SourceLocation>, 8> SafeBufferOptOutMap;
----------------
NoQ wrote:
> Ok, now I no longer see why this data should live in DiagnosticEngine. It's mostly about analysis, right? The pragma simply makes our analysis produce different results, regardless of whether these results are used for producing diagnostics or something else. Maybe let's keep it all in Preprocessor?
make sense to me!


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:543
 #define GADGET(x)                                                              \
-        x ## Gadget::matcher().bind(#x),
+        allOf(x ## Gadget::matcher().bind(#x), notInSafeBufferOptOut()),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
----------------
NoQ wrote:
> This prevents safe fixable gadgets from being found in the opt-out zone. I think this clause should only apply to warning gadgets.
You are right!  Fixables should be found regardless of whether they are in an opt-out zone.  A Fixable could later be immediately discarded once we know that the variable declaration associated to the Fixable is in an opt-out zone.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:551-555
+        allOf(declStmt().bind("any_ds"), notInSafeBufferOptOut())
+        // We match all DREs regardless of whether they are in safe-buffer
+        // opt-out region. Because an unclaimed DRE 'd', regardless of where it is,
+        // should prevent a Fixable associated to the same variable as 'd'
+        // from being emitting.
----------------
NoQ wrote:
> I think we should match all DeclStmts as well, because otherwise we may be unable to find the variable to fix.
In case we are unable to find the variable to fix,  it means that the variable declaration is in an opt-out zone.  So we don't fix the variable anyway, right?

Or do you mean that a variable may still get fixed even if its declaration is in an opt-out zone?   I could imagine it is possible if the variable is involved in some assignments that we want to fix.


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

https://reviews.llvm.org/D140179