[PATCH] D136549: [clang] Fix time profile in "isIntegerConstantExpr"

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron marked 2 inline comments as done.
Izaron added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:15908
 
-  llvm::TimeTraceScope TimeScope("isIntegerConstantExpr", [&] {
-    return Loc->printToString(Ctx.getSourceManager());
-  });
+  ExprTimeTraceScope TimeScope(this, Ctx, "isIntegerConstantExpr");
 
----------------
jloser wrote:
> **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.
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 =)


================
Comment at: clang/unittests/Support/TimeProfilerTest.cpp:209
+  setupProfiler();
+  ASSERT_TRUE(compileFromString(Code, "-std=c99", "test.c"));
+  std::string Json = teardownProfiler();
----------------
jloser wrote:
> **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?
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136549/new/

https://reviews.llvm.org/D136549