[clang] Output location info in separate fields for -ftime-trace

[usx95]

Uniformly use file and line metadata information in -ftime-trace profiles. This makes postprocessing simpler and not fragile.

This removes the column information. This can be added later if the need be as a separate field.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

Uniformly use file and line metadata information in -ftime-trace profiles. This makes post processing simpler and not fragile.

---

Full diff: https://github.com/llvm/llvm-project/pull/106277.diff

3 Files Affected:

• (modified) clang/lib/Parse/Parser.cpp (+7-2)
• (modified) clang/test/Driver/check-time-trace-ParseDeclarationOrFunctionDefinition.cpp (+2-1)
• (modified) clang/unittests/Support/TimeProfilerTest.cpp (+7-7)

diff --git a/clang/lib/Parse/Parser.cpp b/clang/lib/Parse/Parser.cpp
index 5ebe71e496a2e8..cc16a9d98d0ca6 100644
--- a/clang/lib/Parse/Parser.cpp
+++ b/clang/lib/Parse/Parser.cpp
@@ -16,6 +16,7 @@
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
@@ -1255,8 +1256,12 @@ Parser::DeclGroupPtrTy Parser::ParseDeclarationOrFunctionDefinition(
   // Add an enclosing time trace scope for a bunch of small scopes with
   // "EvaluateAsConstExpr".
   llvm::TimeTraceScope TimeScope("ParseDeclarationOrFunctionDefinition", [&]() {
-    return Tok.getLocation().printToString(
-        Actions.getASTContext().getSourceManager());
+    llvm::TimeTraceMetadata M;
+    const SourceManager &SM = Actions.getASTContext().getSourceManager();
+    auto Loc = SM.getExpansionLoc(Tok.getLocation());
+    M.File = SM.getFilename(Loc);
+    M.Line = SM.getExpansionLineNumber(Loc);
+    return M;
   });
 
   if (DS) {
diff --git a/clang/test/Driver/check-time-trace-ParseDeclarationOrFunctionDefinition.cpp b/clang/test/Driver/check-time-trace-ParseDeclarationOrFunctionDefinition.cpp
index f854cddadbfcc1..8a1127752b325b 100644
--- a/clang/test/Driver/check-time-trace-ParseDeclarationOrFunctionDefinition.cpp
+++ b/clang/test/Driver/check-time-trace-ParseDeclarationOrFunctionDefinition.cpp
@@ -4,7 +4,8 @@
 // RUN:   | FileCheck %s
 
 // CHECK-DAG: "name": "ParseDeclarationOrFunctionDefinition"
-// CHECK-DAG: "detail": "{{.*}}check-time-trace-ParseDeclarationOrFunctionDefinition.cpp:15:1"
+// CHECK-DAG: "file": "{{.*}}check-time-trace-ParseDeclarationOrFunctionDefinition.cpp"
+// CHECK-DAG: "line": 16
 // CHECK-DAG: "name": "ParseFunctionDefinition"
 // CHECK-DAG: "detail": "foo"
 // CHECK-DAG: "name": "ParseFunctionDefinition"
diff --git a/clang/unittests/Support/TimeProfilerTest.cpp b/clang/unittests/Support/TimeProfilerTest.cpp
index f53fe71d630bf5..6d8b4e0453294f 100644
--- a/clang/unittests/Support/TimeProfilerTest.cpp
+++ b/clang/unittests/Support/TimeProfilerTest.cpp
@@ -210,8 +210,8 @@ constexpr int slow_init_list[] = {1, 1, 2, 3, 5, 8, 13, 21}; // 25th line
   std::string Json = teardownProfiler();
   ASSERT_EQ(R"(
 Frontend (test.cc)
-| ParseDeclarationOrFunctionDefinition (test.cc:2:1)
-| ParseDeclarationOrFunctionDefinition (test.cc:6:1)
+| ParseDeclarationOrFunctionDefinition (test.cc:2)
+| ParseDeclarationOrFunctionDefinition (test.cc:6)
 | | ParseFunctionDefinition (slow_func)
 | | | EvaluateAsRValue (<test.cc:8:21>)
 | | | EvaluateForOverflow (<test.cc:8:21, col:25>)
@@ -223,15 +223,15 @@ Frontend (test.cc)
 | | | | EvaluateAsRValue (<test.cc:8:21, col:25>)
 | | | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)
 | | | | EvaluateAsRValue (<test.cc:8:21, col:25>)
-| ParseDeclarationOrFunctionDefinition (test.cc:16:1)
+| ParseDeclarationOrFunctionDefinition (test.cc:16)
 | | ParseFunctionDefinition (slow_test)
 | | | EvaluateAsInitializer (slow_value)
 | | | EvaluateAsConstantExpr (<test.cc:17:33, col:59>)
 | | | EvaluateAsConstantExpr (<test.cc:18:11, col:37>)
-| ParseDeclarationOrFunctionDefinition (test.cc:22:1)
+| ParseDeclarationOrFunctionDefinition (test.cc:22)
 | | EvaluateAsConstantExpr (<test.cc:23:31, col:57>)
 | | EvaluateAsRValue (<test.cc:22:14, line:23:58>)
-| ParseDeclarationOrFunctionDefinition (test.cc:25:1)
+| ParseDeclarationOrFunctionDefinition (test.cc:25)
 | | EvaluateAsInitializer (slow_init_list)
 | PerformPendingInstantiations
 )",
@@ -270,7 +270,7 @@ Frontend (test.cc)
 | ParseFunctionDefinition (fooB)
 | ParseFunctionDefinition (fooMTA)
 | ParseFunctionDefinition (fooA)
-| ParseDeclarationOrFunctionDefinition (test.cc:3:5)
+| ParseDeclarationOrFunctionDefinition (test.cc:3)
 | | ParseFunctionDefinition (user)
 | PerformPendingInstantiations
 | | InstantiateFunction (fooA<int>, a.h:7)
@@ -292,7 +292,7 @@ struct {
   std::string Json = teardownProfiler();
   ASSERT_EQ(R"(
 Frontend (test.c)
-| ParseDeclarationOrFunctionDefinition (test.c:2:1)
+| ParseDeclarationOrFunctionDefinition (test.c:2)
 | | isIntegerConstantExpr (<test.c:3:18>)
 | | EvaluateKnownConstIntCheckOverflow (<test.c:3:18>)
 | PerformPendingInstantiations

[ilya-biryukov]

Suggestion: could we update the PR title to say something like "Output location in separate fields of -ftime-trace"?

"properly" is open to interpretation and requires reading the full change description.

[ilya-biryukov]

This is guarded by isTimeTraceVerbose in other places, but not here.
The only reason I can see for that is historical accidents, although perhaps the parsing events are less frequent too.

That being said, I don't think we should necessarily change it in this patch, but could we add a FIXME here saying that other places only log locations in verbose mode and an explanation why this one does not?

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 35dfe8013c2e100ffa159ca183430422a370d3a4 9fa462d60262c4bed03e95f71d3e17dc2788343f --extensions h,cpp -- clang/include/clang/Basic/SourceManager.h clang/lib/AST/ExprConstant.cpp clang/lib/Basic/SourceManager.cpp clang/lib/Parse/Parser.cpp clang/lib/Sema/SemaTemplateInstantiate.cpp clang/lib/Sema/SemaTemplateInstantiateDecl.cpp clang/test/Driver/check-time-trace-ParseDeclarationOrFunctionDefinition.cpp clang/unittests/Support/TimeProfilerTest.cpp

View the diff from clang-format here.

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 9087572d0c..cc7605538b 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -673,7 +673,8 @@ namespace {
   };
 
   // A shorthand time trace scope struct, prints source range, for example
-  // {"name":"EvaluateAsRValue","args":{"detail":"test.cc", "line":8, "col":21"}}}
+  // {"name":"EvaluateAsRValue","args":{"detail":"test.cc", "line":8,
+  // "col":21"}}}
   class ExprTimeTraceScope {
   public:
     ExprTimeTraceScope(const Expr *E, const ASTContext &Ctx, StringRef Name)

[ilya-biryukov]

The new behavior LG, but I have a few NITs and a major comment about the placement of the helper.

[ilya-biryukov]

Could you move outside of source manager?

The method can be implemented independent of source manager and there's no reason for source manager to depend on TimeTraceMetadata.
We might need a separate header for this, but that's ok.

[ilya-biryukov]

Could you mention what the previous behavior was, i.e. that it was reported as a string in detail. It might be read as if we are keeping the old behavior too, but it may actually be a change that breaks compatibility and it's useful to mention that.

[ilya-biryukov]

Suggestion: getLineNumber should produce the same result since we've called the getExpansionLoc above already.
Will read simpler and should skip some redundant work.