Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP][-Wunsafe-buffer-usage] Start emitting std::array fixits #68037

Conversation

jkorous-apple
Copy link
Contributor

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:analysis labels Oct 2, 2023
@github-actions
Copy link

github-actions bot commented Oct 2, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 8ebd226375488426fd834239cb5c51b3ea0c8004 553173411b33b4439d6d6c458c31e08ab0a08e28 -- clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h clang/lib/Analysis/UnsafeBufferUsage.cpp clang/lib/Sema/AnalysisBasedWarnings.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
View the diff from clang-format here.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index a2f7f84109b3..fe097cc679b9 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -57,10 +57,7 @@ public:
 #endif
 
 public:
-  enum class TargetType {
-    Span,
-    Array
-  };
+  enum class TargetType { Span, Array };
 
   UnsafeBufferUsageHandler() = default;
   virtual ~UnsafeBufferUsageHandler() = default;
@@ -81,7 +78,8 @@ public:
   /// and all of its group mates.
   virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
                                          const VariableGroupsManager &VarGrpMgr,
-                                         FixItList &&Fixes, const Decl *D, TargetType Type) = 0;
+                                         FixItList &&Fixes, const Decl *D,
+                                         TargetType Type) = 0;
 
 #ifndef NDEBUG
 public:
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 61da10b74f39..b38f11c10464 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -2220,22 +2220,23 @@ static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx) {
   if (auto CAT = dyn_cast<clang::ConstantArrayType>(D->getType())) {
     const QualType &ArrayEltT = CAT->getElementType();
     assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
-    // Producing fix-it for variable declaration---make `D` to be of std::array type:
+    // Producing fix-it for variable declaration---make `D` to be of std::array
+    // type:
     SmallString<32> Replacement;
     raw_svector_ostream OS(Replacement);
-    OS << "std::array<" << ArrayEltT.getAsString() << ", " << getAPIntText(CAT->getSize()) << "> " << D->getName();
+    OS << "std::array<" << ArrayEltT.getAsString() << ", "
+       << getAPIntText(CAT->getSize()) << "> " << D->getName();
     FixIts.push_back(FixItHint::CreateReplacement(
-        SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc() }, OS.str()));
+        SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc()}, OS.str()));
   }
 
-
   return FixIts;
 }
 
 static FixItList fixVariableWithArray(const VarDecl *VD,
-                                     const DeclUseTracker &Tracker,
-                                     const ASTContext &Ctx,
-                                     UnsafeBufferUsageHandler &Handler) {
+                                      const DeclUseTracker &Tracker,
+                                      const ASTContext &Ctx,
+                                      UnsafeBufferUsageHandler &Handler) {
   const DeclStmt *DS = Tracker.lookupDecl(VD);
   assert(DS && "Fixing non-local variables not implemented yet!");
   if (!DS->isSingleDecl()) {
@@ -2806,14 +2807,14 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
 
   for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {
     auto FixItsIt = FixItsForVariableGroup.find(VD);
-    Handler.handleUnsafeVariableGroup(VD, VarGrpMgr,
-                                      FixItsIt != FixItsForVariableGroup.end()
-                                      ? std::move(FixItsIt->second)
-                                      : FixItList{},
-                                      D,
-                                      NaiveStrategy.lookup(VD) == Strategy::Kind::Span
-                                          ? UnsafeBufferUsageHandler::TargetType::Span
-                                          : UnsafeBufferUsageHandler::TargetType::Array);
+    Handler.handleUnsafeVariableGroup(
+        VD, VarGrpMgr,
+        FixItsIt != FixItsForVariableGroup.end() ? std::move(FixItsIt->second)
+                                                 : FixItList{},
+        D,
+        NaiveStrategy.lookup(VD) == Strategy::Kind::Span
+            ? UnsafeBufferUsageHandler::TargetType::Span
+            : UnsafeBufferUsageHandler::TargetType::Array);
     for (const auto &G : WarningGadgets) {
       Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true);
     }
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 8a76b5531f67..50454ba98b07 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2267,7 +2267,8 @@ public:
 
   void handleUnsafeVariableGroup(const VarDecl *Variable,
                                  const VariableGroupsManager &VarGrpMgr,
-                                 FixItList &&Fixes, const Decl *D, TargetType Type) override {
+                                 FixItList &&Fixes, const Decl *D,
+                                 TargetType Type) override {
     assert(!SuggestSuggestions &&
            "Unsafe buffer usage fixits displayed without suggestions!");
     S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)

@haoNoQ
Copy link
Collaborator

haoNoQ commented Oct 11, 2023

We will need a new warning text here. "To preserve bounds information" isn't our goal; the traditional C array already preserves bounds information as part of the type. Theoretically, we could teach the compiler to harden all raw C array operations without converting it to std::array; the bounds information is already there. The message should say that std::array is beneficial because it takes advantage of library-level hardening.

@jkorous-apple
Copy link
Contributor Author

Deprecated by a proper implementation:
#80084

Going to address the comment about warning text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:analysis clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants