[clang] [Safe Buffers] Serialize unsafe_buffer_usage pragmas (PR #92031)

[Artem Dergachev via cfe-commits]

================
@@ -1483,26 +1484,40 @@ void Preprocessor::emitFinalMacroWarning(const Token &Identifier,
 }
 
 bool Preprocessor::isSafeBufferOptOut(const SourceManager &SourceMgr,
-                                           const SourceLocation &Loc) const {
-  // Try to find a region in `SafeBufferOptOutMap` where `Loc` is in:
-  auto FirstRegionEndingAfterLoc = llvm::partition_point(
-      SafeBufferOptOutMap,
-      [&SourceMgr,
-       &Loc](const std::pair<SourceLocation, SourceLocation> &Region) {
-        return SourceMgr.isBeforeInTranslationUnit(Region.second, Loc);
-      });
+                                      const SourceLocation &Loc) const {
+  // The lambda that tests if a `Loc` is in an opt-out region given one opt-out
+  // region map:
+  auto TestInMap = [&SourceMgr](const SafeBufferOptOutRegionsTy &Map,
+                                const SourceLocation &Loc) -> bool {
+    // Try to find a region in `SafeBufferOptOutMap` where `Loc` is in:
+    auto FirstRegionEndingAfterLoc = llvm::partition_point(
+        Map, [&SourceMgr,
+              &Loc](const std::pair<SourceLocation, SourceLocation> &Region) {
+          return SourceMgr.isBeforeInTranslationUnit(Region.second, Loc);
+        });
+
+    if (FirstRegionEndingAfterLoc != Map.end()) {
+      // To test if the start location of the found region precedes `Loc`:
+      return SourceMgr.isBeforeInTranslationUnit(
+          FirstRegionEndingAfterLoc->first, Loc);
+    }
+    // If we do not find a region whose end location passes `Loc`, we want to
+    // check if the current region is still open:
+    if (!Map.empty() && Map.back().first == Map.back().second)
+      return SourceMgr.isBeforeInTranslationUnit(Map.back().first, Loc);
+    return false;
+  };
 
----------------
haoNoQ wrote:

Ok @ziqingluo-90 has just walked me through this offline and I think I understand what's happening now.

What was unobvious to me: shouldn't we `TestInMap` over the entire stack of includes? And the answer is:
- Regular textual `#include`s will just show up in the current map normally; there's no need to go up the stack.
- PCHes are going to be included with the `-include-pch` flag, not directly. There's not really much of a stack going on at this point and there's no way to "put pragmas around the compiler flag".
- In case of modules, we don't care what this function returns deep inside a module, because we probably shouldn't be analyzing that code in the first place. Instead, the warning should be naturally enabled in the clang invocation that builds the module itself, and that's where the warning would be emitted, taking only the current module into account.
  - As a matter of fact, as of today we're analyzing that nested code. But we shouldn't be, that's not how modules are intended to work. We consider it a bug and have a separate plan to fix this. Right now this means that the warning will be emitted multiple times for the same line of code (both by module compilation and by .cpp file compilation, and possibly by everything in between). And the worst that could happen if this function misbehaves, is that some of these duplicates will disappear. Which is annoying but harmless.

Let's add a comment explaining this!

https://github.com/llvm/llvm-project/pull/92031