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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
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.
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). | |
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: |
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.