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

[Joe Loser via Phabricator via cfe-commits]

jloser 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");
 
----------------
Izaron wrote:
> 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 =)
That was my takeaway as well — no obvious lingering bugs. :)


================
Comment at: clang/unittests/Support/TimeProfilerTest.cpp:209
+  setupProfiler();
+  ASSERT_TRUE(compileFromString(Code, "-std=c99", "test.c"));
+  std::string Json = teardownProfiler();
----------------
Izaron wrote:
> 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
Makes sense — thanks for pointing out the relevant code block in Sema!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136549