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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
