[Sema] Warn about possible stack exhaution

[Mordante]

Summary

When a long set of expressions is chained it may overflow the stack. This warns about the issue.

Note I'm not sure whether AnalyzeImplicitConversions is the best place to add this test, but it was the best I could find. Also not sure what the naming convention for these helpers is. Another option is to put the original code in a lambda in AnalyzeImplicitConversions and thus remove the extra function.

Fixes https://bugs.llvm.org/show_bug.cgi?id=14030

Depends on D69478 (if this patch is unwanted it is possible to remove the dependency.)

[xbolva00]

clang/lib/Sema/SemaChecking.cpp

12339

Please do not change function names.

[xbolva00]

clang/lib/Sema/SemaChecking.cpp

12339

Ah, you had to change it.

Can this be solved maybe via new bool argument for AnalyzeImplicitConversions(.., arg)? If arg is false, we could just call S.runWithSufficientStackSpace(AnalyzeImplicitConversions(...., true)), otherwise continue in normal flow.

[Mordante]

clang/lib/Sema/SemaChecking.cpp

12339

I like this idea better than my original approach. Thanks for the suggestion.

[Mordante]

Add an extra argument to AnalyzeImplicitConversions as suggested by @xbolva00 .

[rsmith]

We should resist using runWithSufficientStackSpace where possible. Have you tried making this process data-recursive instead? It looks fairly straightforward to form a worklist of expressions to which we want to apply AnalyseImplicitConversions, and run through that list until no new entries are added.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3481

The note should explain what the problem is. If you want to suggest a solution, that could be OK, but should be secondary and non-obvious from the description of the problem. In this case, a note saying the expression is too complex would cover both explaining what the problem is, and implying the solution (make the expression less complex).

Also, Clang diagnostics start with lowercase letters.

clang/lib/Sema/SemaChecking.cpp

12339

How about renaming the original function to AnalyzeImplicitConversionsImpl and using the original name for the runWithSufficientStackSpace version?

clang/test/Sema/stack-overflow-expr-int.c

3

You can use not --crash if you want to allow crashes. But testing that we do crash seems inappropriate here; if we keep using runWithSufficientStackSpace, it would seem preferable to test that we produce the diagnostic, and make sure the test passes regardless of whether we crash.

[Mordante]

I'll look at a worklist first, before fixing the other issues. However it seems it's not as trivial as I hoped. The recursion occurs in the SequenceChecker which has a WorkList but that's not used for this part. I'll try to find a solution.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 // for their workflow.
 def warn_int_to_pointer_cast : Warning<
   "cast to %1 from smaller integer type %0">,
   InGroup<IntToPointerCast>;
 def warn_int_to_void_pointer_cast : Warning<
   "cast to %1 from smaller integer type %0">,
   InGroup<IntToVoidPointerCast>;
 
+def note_reduce_expression_chaining : Note<
+  "Reduce the number of chained expressions, using helper variables or parens">;
+
 def warn_attribute_ignored_for_field_of_type : Warning<
   "%0 attribute ignored for field of type %1">,
   InGroup<IgnoredAttributes>;
 def warn_no_underlying_type_specified_for_enum_bitfield : Warning<
   "enums in the Microsoft ABI are signed integers by default; consider giving "
   "the enum %0 an unsigned underlying type to make this code portable">,
   InGroup<SignedEnumBitfield>, DefaultIgnore;
 def warn_attribute_packed_for_bitfield : Warning<

clang/lib/Sema/SemaChecking.cpp

if (value.isVector()) {
     return true;
   }
 
   assert(value.isComplexFloat());
   return (IsSameFloatAfterCast(value.getComplexFloatReal(), Src, Tgt) &&
           IsSameFloatAfterCast(value.getComplexFloatImag(), Src, Tgt));
 }
 
-static void AnalyzeImplicitConversions(Sema &S, Expr *E, SourceLocation CC,
-                                       bool IsListInit = false);
+static void
+AnalyzeImplicitConversions(Sema &S, Expr *E, SourceLocation CC,
+                           bool IsListInit = false,
+                           bool CalledWithSufficientStackSpace = false);
 
 static bool IsEnumConstOrFromMacro(Sema &S, Expr *E) {
   // Suppress cases where we are comparing against an enum constant.
   if (const DeclRefExpr *DR =
       dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts()))
     if (isa<EnumConstantDecl>(DR->getDecl()))
       return true;
 
static void CheckBoolLikeConversion(Sema &S, Expr *E, SourceLocation CC) {
   if (E->IgnoreParenImpCasts()->getType()->isAtomicType())
     return;
   CheckImplicitConversion(S, E->IgnoreParenImpCasts(), S.Context.BoolTy, CC);
 }
 
 /// AnalyzeImplicitConversions - Find and report any interesting
 /// implicit conversions in the given expression.  There are a couple
 /// of competing diagnostics here, -Wconversion and -Wsign-compare.
-static void AnalyzeImplicitConversions(Sema &S, Expr *OrigE, SourceLocation CC,
-                                       bool IsListInit/*= false*/) {
+///
+/// \param CalledWithSufficientStackSpace is the function called from
+/// \ref Sema::runWithSufficientStackSpace if not it calls itself as lamba to
+/// that function.
+static void
+AnalyzeImplicitConversions(Sema &S, Expr *OrigE, SourceLocation CC,
+                           bool IsListInit /*= false*/,
+                           bool CalledWithSufficientStackSpace /*= false*/) {
+  if (!CalledWithSufficientStackSpace) {
+    S.runWithSufficientStackSpace(
+        CC, [&] { AnalyzeImplicitConversions(S, OrigE, CC, IsListInit, true); },
+        diag::note_reduce_expression_chaining);
+    return;
+  }
+
   QualType T = OrigE->getType();
   Expr *E = OrigE->IgnoreParenImpCasts();
 
   // Propagate whether we are in a C++ list initialization expression.
   // If so, we do not issue warnings for implicit int-float conversion
   // precision loss, because C++11 narrowing already handles it.
   IsListInit =
       IsListInit || (isa<InitListExpr>(OrigE) && S.getLangOpts().CPlusPlus);
if (E->isTypeDependent() || E->isValueDependent())
     return;
 
   // Check for array bounds violations in cases where the check isn't triggered
   // elsewhere for other Expr types (like BinaryOperators), e.g. when an
   // ArraySubscriptExpr is on the RHS of a variable initialization.
   CheckArrayAccess(E);
 
   // This is not the right CC for (e.g.) a variable initialization.
   AnalyzeImplicitConversions(*this, E, CC);
 }
 
 /// CheckBoolLikeConversion - Check conversion of given expression to boolean.
 /// Input argument E is a logical expression.
 void Sema::CheckBoolLikeConversion(Expr *E, SourceLocation CC) {
   ::CheckBoolLikeConversion(*this, E, CC);
 }
 

clang/test/Sema/stack-overflow-expr-bool.cpp

+// The segmentation fault gives exit code 139
+// RUN: not not %clang_cc1 %s -fsyntax-only 2>&1 | FileCheck %s
+// REQUIRES: thread_support
+
+#define A10 ARG ARG ARG ARG ARG ARG ARG ARG ARG ARG
+#define A100 A10 A10 A10 A10 A10 A10 A10 A10 A10 A10
+#define A1000 A10 A10 A10 A10 A10 A10 A10 A10 A10 A10
+#define A10000 A100 A100 A100 A100 A100 A100 A100 A100 A100 A100
+#define A100000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000
+#define A1000000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000
+#define A10000000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000
+#define A100000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000
+
+#define ARG true &&
+void foo() {
+  bool b = A100000000 true;
+}
+
+// CHECK: warning: stack nearly exhausted; compilation time may suffer, and crashes due to stack overflow are likely
+// CHECK: note: Reduce the number of chained expressions, using helper variables or parens
+// CHECK: Stack dump:

clang/test/Sema/stack-overflow-expr-double.c

+// The segmentation fault gives exit code 139
+// RUN: not not %clang_cc1 %s -fsyntax-only 2>&1 | FileCheck %s
+// REQUIRES: thread_support
+
+#define A10 ARG ARG ARG ARG ARG ARG ARG ARG ARG ARG
+#define A100 A10 A10 A10 A10 A10 A10 A10 A10 A10 A10
+#define A1000 A10 A10 A10 A10 A10 A10 A10 A10 A10 A10
+#define A10000 A100 A100 A100 A100 A100 A100 A100 A100 A100 A100
+#define A100000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000
+#define A1000000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000
+#define A10000000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000
+#define A100000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000
+
+#define ARG 1. +
+void foo() {
+  double f = A100000000 1.;
+}
+
+// CHECK: warning: stack nearly exhausted; compilation time may suffer, and crashes due to stack overflow are likely
+// CHECK: note: Reduce the number of chained expressions, using helper variables or parens
+// CHECK: Stack dump:

clang/test/Sema/stack-overflow-expr-int.c

+// The segmentation fault gives exit code 139
+// RUN: not not %clang_cc1 %s -fsyntax-only 2>&1 | FileCheck %s
+// REQUIRES: thread_support
+
+#define A10 ARG ARG ARG ARG ARG ARG ARG ARG ARG ARG
+#define A100 A10 A10 A10 A10 A10 A10 A10 A10 A10 A10
+#define A1000 A10 A10 A10 A10 A10 A10 A10 A10 A10 A10
+#define A10000 A100 A100 A100 A100 A100 A100 A100 A100 A100 A100
+#define A100000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000 A1000
+#define A1000000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000 A10000
+#define A10000000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000 A100000
+#define A100000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000 A1000000
+
+#define ARG 1 +
+void foo() {
+  int i = A100000000 1;
+}
+
+// CHECK: warning: stack nearly exhausted; compilation time may suffer, and crashes due to stack overflow are likely
+// CHECK: note: Reduce the number of chained expressions, using helper variables or parens
+// CHECK: Stack dump: