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