[clang][ExprConst] Add diagnostics for invalid binary arithmetic #118475

tbaederr · 2024-12-03T11:53:29Z

... between unrelated declarations or literals.

Leaving this small (I haven't run the whole test suite locally) to get some feedback on the wording and implementation first.

The output of the sample in #117409 is now:

./array.cpp:57:6: warning: expression result unused [-Wunused-value]
   57 |   am - aj.af();
      |   ~~ ^ ~~~~~~~
./array.cpp:70:8: error: call to consteval function 'L::L<bx>' is not a constant expression
   70 |   q(0, [] {
      |        ^
./array.cpp:57:6: note: arithmetic on addresses of literals has unspecified value
   57 |   am - aj.af();
      |      ^
./array.cpp:62:5: note: in call to 'al(&""[0], {&""[0]})'
   62 |     al(bp.af(), k);
      |     ^~~~~~~~~~~~~~
./array.cpp:70:8: note: in call to 'L<bx>({})'
   70 |   q(0, [] {
      |        ^~~~
   71 |     struct bx {
      |     ~~~~~~~~~~~
   72 |       constexpr operator ab<g<l<decltype(""[0])>::e>::e>() { return t(""); }
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   73 |     };
      |     ~~
   74 |     return bx();
      |     ~~~~~~~~~~~~
   75 |   }());
      |   ~~~

The output for

int a, b;
constexpr int n = &b - &a

is now:

./array.cpp:80:15: error: constexpr variable 'n' must be initialized by a constant expression
   80 | constexpr int n = &b - &a;
      |               ^   ~~~~~~~
./array.cpp:80:22: note: arithmetic involving '&b' and '&a' has unspecified value
   80 | constexpr int n = &b - &a;
      |                      ^
1 error generated.

llvmbot · 2024-12-03T11:54:05Z

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

... between unrelated declarations or literals.

Leaving this small (I haven't run the whole test suite locally) to get some feedback on the wording and implementation first.

The output of the sample in #117409 is now:

./array.cpp:57:6: warning: expression result unused [-Wunused-value]
   57 |   am - aj.af();
      |   ~~ ^ ~~~~~~~
./array.cpp:70:8: error: call to consteval function 'L::L&lt;bx&gt;' is not a constant expression
   70 |   q(0, [] {
      |        ^
./array.cpp:57:6: note: arithmetic on addresses of literals has unspecified value
   57 |   am - aj.af();
      |      ^
./array.cpp:62:5: note: in call to 'al(&amp;""[0], {&amp;""[0]})'
   62 |     al(bp.af(), k);
      |     ^~~~~~~~~~~~~~
./array.cpp:70:8: note: in call to 'L&lt;bx&gt;({})'
   70 |   q(0, [] {
      |        ^~~~
   71 |     struct bx {
      |     ~~~~~~~~~~~
   72 |       constexpr operator ab&lt;g&lt;l&lt;decltype(""[0])&gt;::e&gt;::e&gt;() { return t(""); }
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   73 |     };
      |     ~~
   74 |     return bx();
      |     ~~~~~~~~~~~~
   75 |   }());
      |   ~~~

The output for

int a, b;
constexpr int n = &amp;b - &amp;a

is now:

./array.cpp:80:15: error: constexpr variable 'n' must be initialized by a constant expression
   80 | constexpr int n = &amp;b - &amp;a;
      |               ^   ~~~~~~~
./array.cpp:80:22: note: arithmetic involving '&amp;b' and '&amp;a' has unspecified value
   80 | constexpr int n = &amp;b - &amp;a;
      |                      ^
1 error generated.

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

2 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticASTKinds.td (+4)
(modified) clang/lib/AST/ExprConstant.cpp (+15-2)

diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index f630698757c5fb..4d078fc9ca6bb7 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -91,11 +91,15 @@ def note_constexpr_pointer_subtraction_zero_size : Note<
   "subtraction of pointers to type %0 of zero size">;
 def note_constexpr_pointer_comparison_unspecified : Note<
   "comparison between '%0' and '%1' has unspecified value">;
+def note_constexpr_pointer_arith_unspecified : Note<
+  "arithmetic involving '%0' and '%1' has unspecified value">;
 def note_constexpr_pointer_constant_comparison : Note<
   "comparison of numeric address '%0' with pointer '%1' can only be performed "
   "at runtime">;
 def note_constexpr_literal_comparison : Note<
   "comparison of addresses of literals has unspecified value">;
+def note_constexpr_literal_arith : Note<
+  "arithmetic on addresses of literals has unspecified value">;
 def note_constexpr_opaque_call_comparison : Note<
   "comparison against opaque constant address '%0' can only be performed at "
   "runtime">;
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 6b5b95aee35522..2aefcd870ebaf8 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -14548,8 +14548,21 @@ bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
         return Error(E);
       const Expr *LHSExpr = LHSValue.Base.dyn_cast<const Expr *>();
       const Expr *RHSExpr = RHSValue.Base.dyn_cast<const Expr *>();
-      if (!LHSExpr || !RHSExpr)
-        return Error(E);
+      if (!LHSExpr || !RHSExpr) {
+        std::string LHS = LHSValue.toString(Info.Ctx, E->getLHS()->getType());
+        std::string RHS = RHSValue.toString(Info.Ctx, E->getRHS()->getType());
+        Info.FFDiag(E, diag::note_constexpr_pointer_arith_unspecified)
+            << LHS << RHS;
+        return false;
+      }
+
+      if (ArePotentiallyOverlappingStringLiterals(Info, LHSValue, RHSValue)) {
+        std::string LHS = LHSValue.toString(Info.Ctx, E->getLHS()->getType());
+        std::string RHS = RHSValue.toString(Info.Ctx, E->getRHS()->getType());
+        Info.FFDiag(E, diag::note_constexpr_literal_arith) << LHS << RHS;
+        return false;
+      }
+
       const AddrLabelExpr *LHSAddrExpr = dyn_cast<AddrLabelExpr>(LHSExpr);
       const AddrLabelExpr *RHSAddrExpr = dyn_cast<AddrLabelExpr>(RHSExpr);
       if (!LHSAddrExpr || !RHSAddrExpr)

zygoloid

Thanks! I think this is an improvement as-is, but while we're looking at this it'd be useful to make the existing diagnostics (and the new ones that are mirroring them) a little bit more precise about what the problem is.

clang/include/clang/Basic/DiagnosticASTKinds.td

clang/lib/AST/ExprConstant.cpp

zygoloid · 2024-12-03T19:00:41Z

clang/lib/AST/ExprConstant.cpp

+        return false;
+      }
+
+      if (ArePotentiallyOverlappingStringLiterals(Info, LHSValue, RHSValue)) {


It might be useful -- both here and in the comparison logic -- to detect the case where LHSExpr == RHSExpr and produce a different diagnostic (or maybe a note attached to the literal_arith diagnostic) that mentions that repeated evaluation of the same literal can produce different objects.

I liked the idea and did it for the arithmetic case, but I'm not sure about the order, i.e. the new note is printed after the backtrace of the first error:

./array.cpp:70:8: error: call to consteval function 'L::L<bx>' is not a constant expression 70 | q(0, [] { | ^ ./array.cpp:57:6: note: arithmetic on addresses of potentially overlapping literals has unspecified value 57 | am - aj.af(); | ^ ./array.cpp:62:5: note: in call to 'al(&""[0], {&""[0]})' 62 | al(bp.af(), k); | ^~~~~~~~~~~~~~ ./array.cpp:70:8: note: in call to 'L<bx>({})' 70 | q(0, [] { | ^~~~ 71 | struct bx { | ~~~~~~~~~~~ 72 | constexpr operator ab<g<l<decltype(""[0])>::e>::e>() { return t(""); } | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 73 | }; | ~~ 74 | return bx(); | ~~~~~~~~~~~~ 75 | }()); | ~~~ ./array.cpp:72:71: note: repeated evaluation of the same literal expression can produce different objects 72 | constexpr operator ab<g<l<decltype(""[0])>::e>::e>() { return t(""); } | ^~

Yeah, that's a bit unfortunate. But it's how we order diagnostics in general: the "context" notes for an error (eg, template instantiation, macro expansion) appear between the error diagnostic and its proper notes. It'd be nice to address that in general somehow, but I think the order is correct, because the context notes explain how we got to the immediately preceding diagnostic. (Another weird thing: the "In file included from" context appears before the diagnostic whereas other context appears afterwards.)

I've been wondering if we should split the "note" diagnostic category into "notes" that provide additional information about the diagnostic, and "context" diagnostics that provide additional information about the location of the previous diagnostic. We could then de-emphasize the context diagnostics in some way -- perhaps using a less bright color for them, and perhaps omitting the snippet -- to make it easier to scan from a diagnostic to its notes.

But obviously none of that is in scope for this PR :)

shafik

Am I missing something, it seems like several of the new diagnostics don't have tests that cover them.

tbaederr · 2024-12-16T12:03:24Z

Ping

tbaederr · 2025-01-07T16:01:54Z

Ping

cor3ntin

Can you add a release note?
LGTM otherwise

... between unrelated declarations or literals.

tbaederr added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Dec 3, 2024

tbaederr requested review from zygoloid, cor3ntin, AaronBallman and shafik December 3, 2024 11:53

llvmbot added the clang Clang issues not falling into any other category label Dec 3, 2024

zygoloid reviewed Dec 3, 2024

View reviewed changes

tbaederr force-pushed the arith-diagnostics branch from 4f58533 to dd5e19b Compare December 5, 2024 05:57

shafik reviewed Dec 7, 2024

View reviewed changes

tbaederr force-pushed the arith-diagnostics branch from dd5e19b to 798c3e6 Compare December 10, 2024 10:40

cor3ntin approved these changes Jan 7, 2025

View reviewed changes

tbaederr added 4 commits January 8, 2025 14:52

[clang][ExprConst] Add diagnostics for invalid binary arithmetic

0d631c5

... between unrelated declarations or literals.

Address review comments

9574928

Fix tests

fbb9dc9

Add tests for new diagnostics

9178a28

tbaederr force-pushed the arith-diagnostics branch from 798c3e6 to 9178a28 Compare January 9, 2025 09:20

tbaederr merged commit fd6baa4 into llvm:main Jan 9, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][ExprConst] Add diagnostics for invalid binary arithmetic #118475

[clang][ExprConst] Add diagnostics for invalid binary arithmetic #118475

tbaederr commented Dec 3, 2024

llvmbot commented Dec 3, 2024

zygoloid left a comment

zygoloid Dec 3, 2024

tbaederr Dec 4, 2024

zygoloid Dec 4, 2024

shafik left a comment

tbaederr commented Dec 16, 2024

tbaederr commented Jan 7, 2025

cor3ntin left a comment

[clang][ExprConst] Add diagnostics for invalid binary arithmetic #118475

[clang][ExprConst] Add diagnostics for invalid binary arithmetic #118475

Conversation

tbaederr commented Dec 3, 2024

llvmbot commented Dec 3, 2024

zygoloid left a comment

Choose a reason for hiding this comment

zygoloid Dec 3, 2024

Choose a reason for hiding this comment

tbaederr Dec 4, 2024

Choose a reason for hiding this comment

zygoloid Dec 4, 2024

Choose a reason for hiding this comment

shafik left a comment

Choose a reason for hiding this comment

tbaederr commented Dec 16, 2024

tbaederr commented Jan 7, 2025

cor3ntin left a comment

Choose a reason for hiding this comment