[clang] Fix time profile in "isIntegerConstantExpr"

[Izaron]

Summary

The time profiler in Expr::isIntegerConstantExpr used to
call Loc->printToString, it was inconsistent with other time
profiles in the file and caused segfaults if Loc was nullptr.

Fixes https://github.com/llvm/llvm-project/issues/58551

[jloser]

LGTM. Thanks for the fix.

I did leave an open question or two though, but it is non-blocking.

clang/lib/AST/ExprConstant.cpp

15908

Question This looks like the right fix for this call site. Are the other two uses of llvm::TimeTraceScope as a local variable subject to similar problems? I don't think so since we don't try to get the location explicitly, but want to confirm.

clang/unittests/Support/TimeProfilerTest.cpp

209

Question Is adding the ability to plumb the standards mode just useful for this bug fix in the sense of reducing the trace graph output of the AST?

[dyung]

I'm not really familiar with this code, so I'll leave others to review the actual change, but I can confirm that when I built a compiler with this change and ran it against the failing tests they all now pass. So in that regard, LGTM.

[Izaron]

clang/lib/AST/ExprConstant.cpp

15908

That's a good question! We use custom llvm::TimeTraceScope in three places:

isPotentialConstantExpr
EvaluateAsInitializer
EvaluateWithSubstitution

The problem with isIntegerConstantExpr (which I fixed in this patch) was that the Loc was nullptr (and there were no tests that would catch it).

The three functions with custom time traces use either const VarDecl *VD or const FunctionDecl *FD. These variables are surely not nullptr because the methods bravely use them (VD->doSmth()/FD->doSmth()).

Also our unit test covers isPotentialConstantExpr and EvaluateAsInitializer (you can see them in ASSERT_TRUE).
So I think there is no obvious problems that I can think of =)

clang/unittests/Support/TimeProfilerTest.cpp

209

This is useful for bug fix, because some ExprConstant.cpp methods are called only for C code (not for C++ code). C and C++ have a somewhat different constant evaluations.

The segfault in Expr::isIntegerConstantExpr was only discoverable when compiling C code, because there is a explicit condition for calling this method only for C code:
https://github.com/llvm/llvm-project/blob/08d1c43c7023a2e955c43fbf4c3f1635f91521e0/clang/lib/Sema/SemaExpr.cpp#L17318

[jloser]

clang/lib/AST/ExprConstant.cpp

15908

That was my takeaway as well — no obvious lingering bugs. :)

clang/unittests/Support/TimeProfilerTest.cpp

209

Makes sense — thanks for pointing out the relevant code block in Sema!

clang/lib/AST/ExprConstant.cpp

static bool EvaluateCPlusPlus11IntegralConstantExpr(const ASTContext &Ctx,
   return true;
 }
 
 bool Expr::isIntegerConstantExpr(const ASTContext &Ctx,
                                  SourceLocation *Loc) const {
   assert(!isValueDependent() &&
          "Expression evaluator can't be called on a dependent expression.");
 
-  llvm::TimeTraceScope TimeScope("isIntegerConstantExpr", [&] {
-    return Loc->printToString(Ctx.getSourceManager());
-  });
+  ExprTimeTraceScope TimeScope(this, Ctx, "isIntegerConstantExpr");
 
   if (Ctx.getLangOpts().CPlusPlus11)
     return EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, nullptr, Loc);
 
   ICEDiag D = CheckICE(this, Ctx);
   if (D.Kind != IK_ICE) {
     if (Loc) *Loc = D.Loc;
     return false;

clang/unittests/Support/TimeProfilerTest.cpp

std::string teardownProfiler() {
   raw_svector_ostream OS(SmallVec);
   timeTraceProfilerWrite(OS);
   timeTraceProfilerCleanup();
   return OS.str().str();
 }
 
 // Returns true if code compiles successfully.
 // We only parse AST here. This is enough for constexpr evaluation.
-bool compileFromString(StringRef Code) {
+bool compileFromString(StringRef Code, StringRef Standard, StringRef FileName) {
   CompilerInstance Compiler;
   Compiler.createDiagnostics();
 
   auto Invocation = std::make_shared<CompilerInvocation>();
   Invocation->getPreprocessorOpts().addRemappedFile(
-      "test.cc", MemoryBuffer::getMemBuffer(Code).release());
-  const char *Args[] = {"-std=c++20", "test.cc"};
+      FileName, MemoryBuffer::getMemBuffer(Code).release());
+  const char *Args[] = {Standard.data(), FileName.data()};
   CompilerInvocation::CreateFromArgs(*Invocation, Args,
                                      Compiler.getDiagnostics());
   Compiler.setInvocation(std::move(Invocation));
 
   class TestFrontendAction : public ASTFrontendAction {
   private:
     std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                    StringRef InFile) override {
for (const auto &Event : Events) {
     }
     Stream << "\n";
   }
   return Stream.str();
 }
 
 } // namespace
 
-TEST(TimeProfilerTest, ConstantEvaluation) {
+TEST(TimeProfilerTest, ConstantEvaluationCxx20) {
   constexpr StringRef Code = R"(
 void print(double value);
 
 namespace slow_namespace {
 
 consteval double slow_func() {
     double d = 0.0;
     for (int i = 0; i < 100; ++i) { // 8th line

 
 int slow_arr[12 + 34 * 56 +                                  // 22nd line
              static_cast<int>(slow_namespace::slow_func())]; // 23rd line
 
 constexpr int slow_init_list[] = {1, 1, 2, 3, 5, 8, 13, 21}; // 25th line
     )";
 
   setupProfiler();
-  ASSERT_TRUE(compileFromString(Code));
+  ASSERT_TRUE(compileFromString(Code, "-std=c++20", "test.cc"));
   std::string Json = teardownProfiler();
   std::string TraceGraph = buildTraceGraph(Json);
   ASSERT_TRUE(TraceGraph == R"(
 Frontend
 | EvaluateAsRValue (<test.cc:8:21>)
 | EvaluateForOverflow (<test.cc:8:21, col:25>)
 | EvaluateAsRValue (<test.cc:9:14>)
 | EvaluateForOverflow (<test.cc:9:9, col:14>)
 | isPotentialConstantExpr (slow_namespace::slow_func)
 | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)
 | | EvaluateAsRValue (<test.cc:8:21, col:25>)
 | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)
 | | EvaluateAsRValue (<test.cc:8:21, col:25>)
 | EvaluateAsInitializer (slow_value)
 | EvaluateAsConstantExpr (<test.cc:17:33, col:59>)
 | EvaluateAsConstantExpr (<test.cc:18:11, col:37>)
 | EvaluateAsRValue (<test.cc:22:14, line:23:58>)
 | EvaluateAsInitializer (slow_init_list)
 | PerformPendingInstantiations
 )");
 
   // NOTE: If this test is failing, run this test with
   // `llvm::errs() << TraceGraph;` and change the assert above.
 }
+
+TEST(TimeProfilerTest, ConstantEvaluationC99) {
+  constexpr StringRef Code = R"(
+struct {
+  short quantval[4]; // 3rd line
+} value;
+    )";
+
+  setupProfiler();
+  ASSERT_TRUE(compileFromString(Code, "-std=c99", "test.c"));
+  std::string Json = teardownProfiler();
+  std::string TraceGraph = buildTraceGraph(Json);
+  ASSERT_TRUE(TraceGraph == R"(
+Frontend
+| isIntegerConstantExpr (<test.c:3:18>)
+| EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)
+| PerformPendingInstantiations
+)");
+
+  // NOTE: If this test is failing, run this test with
+  // `llvm::errs() << TraceGraph;` and change the assert above.
+}