Add flag to suppress overflow errors in C++ constant expressions. #102390
Conversation
The latest Android NDK is broken on 32-bit because it contains an invalid shift, and older versions of clang didn't catch this. Demote the error to a default-error with a flag so it can be suppressed. Not sure if this is really worth doing if it only affects the Android NDK; presumably most people will use the compiler from the NDK itself, so they'll update their headers before they ever run into this.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Eli Friedman (efriedma-quic) ChangesRecent Android NDK headers are broken on 32-bit because they contain an invalid shift, and older versions of clang didn't catch this. Demote the error to a default-error with a flag so it can be suppressed. See discussion on #70307. Not sure if this is really worth doing if it only affects the Android NDK; Android trunk has been fixed. And presumably most people will use the compiler from the NDK itself, so they'll update their headers before they ever run into this. But posting so we can discuss... CC @glandium Full diff: https://github.com/llvm/llvm-project/pull/102390.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 581434d33c5c9..e51f882856221 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -74,6 +74,14 @@ def err_expr_not_ice : Error<
def ext_expr_not_ice : Extension<
"expression is not an %select{integer|integral}0 constant expression; "
"folding it to a constant is a GNU extension">, InGroup<GNUFoldingConstant>;
+def ext_expr_ice_overflow : Extension<
+ "expression is not an integer constant expression "
+ "because of arithmetic overflow; folding it to a constant is a GNU "
+ "extension">, InGroup<GNUFoldingConstant>;
+def ext_expr_ice_overflow_cxx : Extension<
+ "expression is not an integral constant expression "
+ "because of arithmetic overflow">,
+ InGroup<DiagGroup<"constant-overflow">>, DefaultError, SFINAEFailure;
def err_typecheck_converted_constant_expression : Error<
"value of type %0 is not implicitly convertible to %1">;
def err_typecheck_converted_constant_expression_disallowed : Error<
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 2ec6367eccea0..96af6427b38ea 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -7242,6 +7242,7 @@ class Sema final : public SemaBase {
virtual SemaDiagnosticBuilder diagnoseNotICE(Sema &S,
SourceLocation Loc) = 0;
virtual SemaDiagnosticBuilder diagnoseFold(Sema &S, SourceLocation Loc);
+ virtual SemaDiagnosticBuilder diagnoseOverflow(Sema &S, SourceLocation Loc);
virtual ~VerifyICEDiagnoser() {}
};
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 74c0e01705905..84bc1e37215cd 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -16913,6 +16913,13 @@ Sema::VerifyICEDiagnoser::diagnoseFold(Sema &S, SourceLocation Loc) {
return S.Diag(Loc, diag::ext_expr_not_ice) << S.LangOpts.CPlusPlus;
}
+Sema::SemaDiagnosticBuilder
+Sema::VerifyICEDiagnoser::diagnoseOverflow(Sema &S, SourceLocation Loc) {
+ if (S.LangOpts.CPlusPlus)
+ return S.Diag(Loc, diag::ext_expr_ice_overflow_cxx);
+ return S.Diag(Loc, diag::ext_expr_ice_overflow);
+}
+
ExprResult
Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
VerifyICEDiagnoser &Diagnoser,
@@ -17031,6 +17038,7 @@ Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
}
Diagnoser.diagnoseFold(*this, DiagLoc) << E->getSourceRange();
+
for (const PartialDiagnosticAt &Note : Notes)
Diag(Note.first, Note.second);
@@ -17045,8 +17053,7 @@ Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
// not a constant expression as a side-effect.
bool Folded =
E->EvaluateAsRValue(EvalResult, Context, /*isConstantContext*/ true) &&
- EvalResult.Val.isInt() && !EvalResult.HasSideEffects &&
- (!getLangOpts().CPlusPlus || !EvalResult.HasUndefinedBehavior);
+ EvalResult.Val.isInt() && !EvalResult.HasSideEffects;
if (!isa<ConstantExpr>(E))
E = ConstantExpr::Create(Context, E, EvalResult.Val);
@@ -17079,7 +17086,10 @@ Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
return ExprError();
}
- Diagnoser.diagnoseFold(*this, DiagLoc) << E->getSourceRange();
+ if (EvalResult.HasUndefinedBehavior)
+ Diagnoser.diagnoseOverflow(*this, DiagLoc) << E->getSourceRange();
+ else
+ Diagnoser.diagnoseFold(*this, DiagLoc) << E->getSourceRange();
for (const PartialDiagnosticAt &Note : Notes)
Diag(Note.first, Note.second);
diff --git a/clang/test/C/drs/dr2xx.c b/clang/test/C/drs/dr2xx.c
index ffdf5aac377d9..5df6f056e5c50 100644
--- a/clang/test/C/drs/dr2xx.c
+++ b/clang/test/C/drs/dr2xx.c
@@ -287,7 +287,7 @@ void dr261(void) {
* but we fold it as a constant expression anyway as a GNU extension.
*/
enum e2 {
- ex2 = __INT_MAX__ + (0, 1) /* expected-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
+ ex2 = __INT_MAX__ + (0, 1) /* expected-warning {{expression is not an integer constant expression because of arithmetic overflow; folding it to a constant is a GNU extension}}
expected-note {{value 2147483648 is outside the range of representable values of type 'int'}}
expected-warning {{left operand of comma operator has no effect}}
*/
diff --git a/clang/test/Sema/shift-count-overflow.c b/clang/test/Sema/shift-count-overflow.c
index b5186586c2272..69890d8d6cb28 100644
--- a/clang/test/Sema/shift-count-overflow.c
+++ b/clang/test/Sema/shift-count-overflow.c
@@ -1,6 +1,9 @@
// RUN: %clang_cc1 -fsyntax-only -verify=expected,c -pedantic %s
// RUN: %clang_cc1 -x c++ -std=c++98 -fsyntax-only -verify=expected,cpp %s
// RUN: %clang_cc1 -x c++ -std=c++11 -fsyntax-only -verify=expected,cpp %s
+// RUN: %clang_cc1 -x c++ -std=c++11 -fsyntax-only -Wno-constant-overflow -verify=suppress %s
+
+// suppress-no-diagnostics
enum shiftof {
X = (1<<32) // c-warning {{expression is not an integer constant expression; folding it to a constant is a GNU extension}}
|
|
CC @zygoloid @hubert-reinterpretcast @cor3ntin @shafik for more opinions regarding diagnostic behavior
The NDK code is ill-formed per https://eel.is/c++draft/expr.shift#1.sentence-4 and the fix is already applied from looking at https://android.googlesource.com/platform/frameworks/native/+/master/libs/nativewindow/include/android/hardware_buffer.h#329 I've not spotted any issues opened against 19.x rc1 or rc2 regarding this (let alone something suggesting it's breaking a system header), so unless we get feedback very quickly, I don't see a compelling reason to let this be downgraded to a warning. |
|
I agree with @AaronBallman that, given there hasn't been a lot of outstanding complaints following #70307, we don't want to allow that sort of relaxation of constexpr evaluation as it's both non conforming (and potentially problematic for people who rely on constant evaluation to enforce the soundness of their code) If that change was in fact a lot more disruptive beyond the already patched NDK, I think we could consider a more targeted error for shifts only (rather than allowing any overflow to be ignored). |
|
I targeted all overflows with this fix because we were missing diagnostics for all overflows. For example, "enum{x=999999*999999};" only produces a pedantic warning on clang 18. So if we need the extra flexibility, we should just add it for everything. I think a default-error, sfinae-failing diagnostic should be pretty safe; everyone will get the correct diagnostics by default, and some narrow subset of people can disable them if they cause issues. MSVC ignores most forms of overflow by default (/W2 enables warnings); gcc errors by default, but allows degrading the error to a warning with -fpermissive. |
it's not part of any released NDK, and it's common to be stuck with older NDKs. Most people are probably using the compiler that comes with the NDK, but Mozilla doesn't. Well, I guess we can just patch our clang to make this a non-error on Android targets or something. |
Clang community stance has consistently been that we have no interest in supporting I don't see anyone arguing that this is a good language extension to C++ for Clang to carry, so because this mostly impacts people who are stuck on an older NDK (which suggests that they'll be just as stuck on an older NDK next year or the year after, etc), I'm concerned we'll have to carry this language extension forever. So while I can see a case either way, my preference (personal, not code owner) is to not turn this into a warning that defaults to an error. But maybe I'm misunderstanding the situation, too. |
|
I don't see a compelling reason for the extension and I find its behaviour somewhat arbitrary: why is it okay to allow the overflow in the definition of an enumerator but not okay to allow for GCC's |
|
Note I'm not really attached to merging this either. I mostly posted this to have a space to discuss the consequences of #70307/#100452. If the decision is that we don't want this, I'm fine with that. Also, if we change our mind based on user reports, this would be pretty safe to cherry-pick to a patch release. |
|
Thank you for putting together the experiment @efriedma-quic, it's greatly appreciated! I agree with closing it for now; we can resurrect it later if necessary. |
Recent Android NDK headers are broken on 32-bit because they contain an invalid shift, and older versions of clang didn't catch this. Demote the error to a default-error with a flag so it can be suppressed. See discussion on #70307.
Not sure if this is really worth doing if it only affects the Android NDK; Android trunk has been fixed. And presumably most people will use the compiler from the NDK itself, so they'll update their headers before they ever run into this. But posting so we can discuss...
CC @glandium