Skip to content

Backport "[clang][analyzer] Fix #embed crash (#107764)" #107841

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

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Backport "[clang][analyzer] Fix #embed crash (#107764)" #107841

merged 1 commit into from
Sep 10, 2024

Conversation

steakhal
Copy link
Contributor

@steakhal steakhal commented Sep 9, 2024

Backports the fix for #107724.

(cherry picked from commit d84d955)

@steakhal steakhal added this to the LLVM 19.X Release milestone Sep 9, 2024
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Sep 9, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 9, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balazs Benics (steakhal)

Changes

Backports the fix for #107724.

(cherry picked from commit d84d955)


Full diff: https://github.com/llvm/llvm-project/pull/107841.diff

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+1-4)
  • (added) clang/test/Analysis/embed.c (+12)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 62a240ecbc6003..c11468a08ae5ca 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1928,6 +1928,7 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
     case Stmt::CXXRewrittenBinaryOperatorClass:
     case Stmt::RequiresExprClass:
     case Expr::CXXParenListInitExprClass:
+    case Stmt::EmbedExprClass:
       // Fall through.
 
     // Cases we intentionally don't evaluate, since they don't need
@@ -2430,10 +2431,6 @@ void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,
       Bldr.addNodes(Dst);
       break;
     }
-
-    case Stmt::EmbedExprClass:
-      llvm::report_fatal_error("Support for EmbedExpr is not implemented.");
-      break;
   }
 }
 
diff --git a/clang/test/Analysis/embed.c b/clang/test/Analysis/embed.c
new file mode 100644
index 00000000000000..32f6c130325740
--- /dev/null
+++ b/clang/test/Analysis/embed.c
@@ -0,0 +1,12 @@
+// RUN: %clang_analyze_cc1 -std=c23 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_dump_ptr(const unsigned char *ptr);
+void clang_analyzer_dump(unsigned char val);
+
+int main() {
+    const unsigned char SelfBytes[] = {
+        #embed "embed.c"
+    };
+    clang_analyzer_dump_ptr(SelfBytes); // expected-warning {{&Element{SelfBytes,0 S64b,unsigned char}}}
+    clang_analyzer_dump(SelfBytes[0]); // expected-warning {{Unknown}} FIXME: This should be the `/` character.
+}

@steakhal
Copy link
Contributor Author

steakhal commented Sep 9, 2024

There is no need to update the release notes as #embed is a new feature, so the crash is not a regression that needs to be highlighted.

Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Collaborator

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tru
Copy link
Collaborator

tru commented Sep 10, 2024

Hi, since we are wrapping up LLVM 19.1.0 we are very strict with the fixes we pick at this point. Can you please respond to the following questions to help me understand if this has to be included in the final release or not.

Is this PR a fix for a regression or a critical issue?

What is the risk of accepting this into the release branch?

What is the risk of NOT accepting this into the release branch?

@NagyDonat
Copy link
Contributor

Is this PR a fix for a regression or a critical issue?

#embed support is a new feature introduced in Clang 19, so this is not a regression. However, without this PR Clang Static Analyzer (and clang-tidy which calls Clang Static Analyzer) will crash on any project that contains an #embed directive.

What is the risk of accepting this into the release branch?

Instead of crashing outright, the static analysis will continue after the #embed directive. This could theoretically lead to incorrect analyzer behavior (other crashes, bad analysis results etc.), but there is no reason to expect any issues.

What is the risk of NOT accepting this into the release branch?

Clang Static Analyzer and clang-tidy will crash on code that contain #embed directives.

@tru tru merged commit bb79e7f into llvm:release/19.x Sep 10, 2024
7 of 9 checks passed
Copy link

@steakhal (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@steakhal steakhal deleted the backport-embed-crash-fix branch December 15, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category release:backport
Projects
Development

Successfully merging this pull request may close these issues.

6 participants