-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Add source file name for template instantiations in -ftime-trace #98320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-clang-driver Author: Utkarsh Saxena (usx95) ChangesFull diff: https://github.com/llvm/llvm-project/pull/98320.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index a7bc6749c5852..69f46a77d8067 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -3430,6 +3430,7 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation,
llvm::raw_string_ostream OS(Name);
Instantiation->getNameForDiagnostic(OS, getPrintingPolicy(),
/*Qualified=*/true);
+ OS << "@" << SourceMgr.getFilename(Instantiation->getLocation());
return Name;
});
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 88f6af80cbc55..d835a217802ac 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4966,6 +4966,7 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
llvm::raw_string_ostream OS(Name);
Function->getNameForDiagnostic(OS, getPrintingPolicy(),
/*Qualified=*/true);
+ OS << "@" << SourceMgr.getFilename(Function->getLocation());
return Name;
});
diff --git a/clang/test/Driver/ftime-trace-sections.py b/clang/test/Driver/ftime-trace-sections.py
index 02afa4ac54eb7..cfec77fc97a8e 100644
--- a/clang/test/Driver/ftime-trace-sections.py
+++ b/clang/test/Driver/ftime-trace-sections.py
@@ -1,6 +1,6 @@
#!/usr/bin/env python
-import json, sys, time
+import json, sys, time, re
def is_inside(range1, range2):
@@ -16,12 +16,18 @@ def is_before(range1, range2):
c = range2["ts"]
return b <= c
+instantiation_pattern = re.compile("^.*<.*>@.*.cpp$")
+
+def is_valid_instantiation(instantiation):
+ return instantiation_pattern.match(instantiation["args"]["detail"])
+
log_contents = json.loads(sys.stdin.read())
events = log_contents["traceEvents"]
codegens = [event for event in events if event["name"] == "CodeGen Function"]
frontends = [event for event in events if event["name"] == "Frontend"]
backends = [event for event in events if event["name"] == "Backend"]
+instantiations = [event for event in events if event["name"].startswith("Instantiate")]
beginning_of_time = log_contents["beginningOfTime"] / 1000000
seconds_since_epoch = time.time()
@@ -48,3 +54,11 @@ def is_before(range1, range2):
]
):
sys.exit("Not all Frontend section are before all Backend sections!")
+
+if not all(
+ [
+ is_valid_instantiation(instantiation)
+ for instantiation in instantiations
+ ]
+):
+ sys.exit("Not all Frontend section are before all Backend sections!")
|
@llvm/pr-subscribers-clang Author: Utkarsh Saxena (usx95) ChangesFull diff: https://github.com/llvm/llvm-project/pull/98320.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index a7bc6749c5852..69f46a77d8067 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -3430,6 +3430,7 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation,
llvm::raw_string_ostream OS(Name);
Instantiation->getNameForDiagnostic(OS, getPrintingPolicy(),
/*Qualified=*/true);
+ OS << "@" << SourceMgr.getFilename(Instantiation->getLocation());
return Name;
});
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 88f6af80cbc55..d835a217802ac 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4966,6 +4966,7 @@ void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
llvm::raw_string_ostream OS(Name);
Function->getNameForDiagnostic(OS, getPrintingPolicy(),
/*Qualified=*/true);
+ OS << "@" << SourceMgr.getFilename(Function->getLocation());
return Name;
});
diff --git a/clang/test/Driver/ftime-trace-sections.py b/clang/test/Driver/ftime-trace-sections.py
index 02afa4ac54eb7..cfec77fc97a8e 100644
--- a/clang/test/Driver/ftime-trace-sections.py
+++ b/clang/test/Driver/ftime-trace-sections.py
@@ -1,6 +1,6 @@
#!/usr/bin/env python
-import json, sys, time
+import json, sys, time, re
def is_inside(range1, range2):
@@ -16,12 +16,18 @@ def is_before(range1, range2):
c = range2["ts"]
return b <= c
+instantiation_pattern = re.compile("^.*<.*>@.*.cpp$")
+
+def is_valid_instantiation(instantiation):
+ return instantiation_pattern.match(instantiation["args"]["detail"])
+
log_contents = json.loads(sys.stdin.read())
events = log_contents["traceEvents"]
codegens = [event for event in events if event["name"] == "CodeGen Function"]
frontends = [event for event in events if event["name"] == "Frontend"]
backends = [event for event in events if event["name"] == "Backend"]
+instantiations = [event for event in events if event["name"].startswith("Instantiate")]
beginning_of_time = log_contents["beginningOfTime"] / 1000000
seconds_since_epoch = time.time()
@@ -48,3 +54,11 @@ def is_before(range1, range2):
]
):
sys.exit("Not all Frontend section are before all Backend sections!")
+
+if not all(
+ [
+ is_valid_instantiation(instantiation)
+ for instantiation in instantiations
+ ]
+):
+ sys.exit("Not all Frontend section are before all Backend sections!")
|
✅ With the latest revision this PR passed the Python code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We might want a release note for that.
Also, do we not have tests for -ftime-trace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we worried about the increase in the output file sizes?
@@ -3430,6 +3430,7 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation, | |||
llvm::raw_string_ostream OS(Name); | |||
Instantiation->getNameForDiagnostic(OS, getPrintingPolicy(), | |||
/*Qualified=*/true); | |||
OS << ", file:" << SourceMgr.getFilename(Instantiation->getLocation()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add it in a structured manner rather than as a substring?
We are outputting JSON after all, it would be much more convenient to keep args.detail
as is and add another one (e.g. args.detail_file
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it as a json parameter.
@@ -3430,6 +3430,7 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation, | |||
llvm::raw_string_ostream OS(Name); | |||
Instantiation->getNameForDiagnostic(OS, getPrintingPolicy(), | |||
/*Qualified=*/true); | |||
OS << ", file:" << SourceMgr.getFilename(Instantiation->getLocation()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to get the spelling location / expansion location first.
Could you check for instantiations coming from various complicaged source locations and make sure we're doing the right thing??
- from macro expansions,
- from macro arguments,
- from concatenation operator
##
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -3430,6 +3430,7 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation, | |||
llvm::raw_string_ostream OS(Name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do this for other things?
Most of the nodes are likely to have a source location they can be attributed to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do this on a need basis. Most source actions (like Parse*) already have this information (although not structured). See the unit test for examples.
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding an optional flag for this.
Given that the profiles are 2-3x larger now, this seems very much warranted.
Mostly LG, but I did have some questions and suggestions. PTAL.
std::string Name = TraceEventObj->getString("name").value_or("").str(); | ||
std::string Metadata = GetMetadata(TraceEventObj); | ||
|
||
if (Name == "Source") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure why we need this change.
Could you explain what happens here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Source" events are async "b", "e" events and do not have a duration attached. So we need to compute that separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why do we do this in the unit-tests, and not production code, and why do we need to do this in this change?
I am still vague on the motivation for this diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, these are due to new headers which are added to the tests.
Decided to ignore Source events as they are not 100% accurate because of being async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but this could use a comment on why we are ignoring them.
Something like Source events do not have a correct duration, so we skip testing them altogether
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more suggestions, and happy to approve as soon as the question about source events gets resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I suggest to add a comment explaining the motivation to drop source events.
So that if anyone else wants to re-add them, they'll have some base for understanding what caused us to drop them in the first place.
std::string Name = TraceEventObj->getString("name").value_or("").str(); | ||
std::string Metadata = GetMetadata(TraceEventObj); | ||
|
||
if (Name == "Source") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, but this could use a comment on why we are ignoring them.
Something like Source events do not have a correct duration, so we skip testing them altogether
.
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/2507 Here is the relevant piece of the build log for the reference:
|
I highly suspect that this change is also causing the test failure on my Windows bot: https://lab.llvm.org/buildbot/#/builders/46/builds/1870 |
Hi, This test introduced TimeProfilerTest and it is currently breaking on windows. It looks like related to the path separator. Error message:
Could you revert your change and fix it please? |
…ace" (#99534) Reverts #98320 Breaks windows tests: ``` Step 8 (test-build-unified-tree-check-clang-unit) failure: test (failure) ******************** TEST 'Clang-Unit :: Support/./ClangSupportTests.exe/1/3' FAILED ******************** Script(shard): -- GTEST_OUTPUT=json:C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\tools\clang\unittests\Support\.\ClangSupportTests.exe-Clang-Unit-4296-1-3.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=3 GTEST_SHARD_INDEX=1 C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\tools\clang\unittests\Support\.\ClangSupportTests.exe -- Script: -- C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\tools\clang\unittests\Support\.\ClangSupportTests.exe --gtest_filter=TimeProfilerTest.TemplateInstantiations -- C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\clang\unittests\Support\TimeProfilerTest.cpp(278): error: Expected equality of these values: R"( Frontend | ParseFunctionDefinition (fooB) | ParseFunctionDefinition (fooMTA) | ParseFunctionDefinition (fooA) | ParseDeclarationOrFunctionDefinition (test.cc:3:5) | | ParseFunctionDefinition (user) | PerformPendingInstantiations | | InstantiateFunction (fooA<int>, ./a.h:7) | | | InstantiateFunction (fooB<int>, ./b.h:3) | | | InstantiateFunction (fooMTA<int>, ./a.h:4) )" Which is: "\nFrontend\n| ParseFunctionDefinition (fooB)\n| ParseFunctionDefinition (fooMTA)\n| ParseFunctionDefinition (fooA)\n| ParseDeclarationOrFunctionDefinition (test.cc:3:5)\n| | ParseFunctionDefinition (user)\n| PerformPendingInstantiations\n| | InstantiateFunction (fooA<int>, ./a.h:7)\n| | | InstantiateFunction (fooB<int>, ./b.h:3)\n| | | InstantiateFunction (fooMTA<int>, ./a.h:4)\n" buildTraceGraph(Json) Which is: "\nFrontend\n| ParseFunctionDefinition (fooB)\n| ParseFunctionDefinition (fooMTA)\n| ParseFunctionDefinition (fooA)\n| ParseDeclarationOrFunctionDefinition (test.cc:3:5)\n| | ParseFunctionDefinition (user)\n| PerformPendingInstantiations\n| | InstantiateFunction (fooA<int>, .\\a.h:7)\n| | | InstantiateFunction (fooB<int>, .\\b.h:3)\n| | | InstantiateFunction (fooMTA<int>, .\\a.h:4)\n" With diff: @@ -7,5 +7,5 @@ | | ParseFunctionDefinition (user) | PerformPendingInstantiations -| | InstantiateFunction (fooA<int>, ./a.h:7) -| | | InstantiateFunction (fooB<int>, ./b.h:3) -| | | InstantiateFunction (fooMTA<int>, ./a.h:4)\n +| | InstantiateFunction (fooA<int>, .\\a.h:7) +| | | InstantiateFunction (fooB<int>, .\\b.h:3) +| | | InstantiateFunction (fooMTA<int>, .\\a.h:4)\n C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\clang\unittests\Support\TimeProfilerTest.cpp:278 Expected equality of these values: R"( Frontend | ParseFunctionDefinition (fooB) | ParseFunctionDefinition (fooMTA) | ParseFunctionDefinition (fooA) | ParseDeclarationOrFunctionDefinition (test.cc:3:5) | | ParseFunctionDefinition (user) | PerformPendingInstantiations | | InstantiateFunction (fooA<int>, ./a.h:7) ```
) Summary: This is helpful in identifying file and location which contain the particular template declaration. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250901
…ace" (#99534) Summary: Reverts #98320 Breaks windows tests: ``` Step 8 (test-build-unified-tree-check-clang-unit) failure: test (failure) ******************** TEST 'Clang-Unit :: Support/./ClangSupportTests.exe/1/3' FAILED ******************** Script(shard): -- GTEST_OUTPUT=json:C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\tools\clang\unittests\Support\.\ClangSupportTests.exe-Clang-Unit-4296-1-3.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=3 GTEST_SHARD_INDEX=1 C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\tools\clang\unittests\Support\.\ClangSupportTests.exe -- Script: -- C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\tools\clang\unittests\Support\.\ClangSupportTests.exe --gtest_filter=TimeProfilerTest.TemplateInstantiations -- C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\clang\unittests\Support\TimeProfilerTest.cpp(278): error: Expected equality of these values: R"( Frontend | ParseFunctionDefinition (fooB) | ParseFunctionDefinition (fooMTA) | ParseFunctionDefinition (fooA) | ParseDeclarationOrFunctionDefinition (test.cc:3:5) | | ParseFunctionDefinition (user) | PerformPendingInstantiations | | InstantiateFunction (fooA<int>, ./a.h:7) | | | InstantiateFunction (fooB<int>, ./b.h:3) | | | InstantiateFunction (fooMTA<int>, ./a.h:4) )" Which is: "\nFrontend\n| ParseFunctionDefinition (fooB)\n| ParseFunctionDefinition (fooMTA)\n| ParseFunctionDefinition (fooA)\n| ParseDeclarationOrFunctionDefinition (test.cc:3:5)\n| | ParseFunctionDefinition (user)\n| PerformPendingInstantiations\n| | InstantiateFunction (fooA<int>, ./a.h:7)\n| | | InstantiateFunction (fooB<int>, ./b.h:3)\n| | | InstantiateFunction (fooMTA<int>, ./a.h:4)\n" buildTraceGraph(Json) Which is: "\nFrontend\n| ParseFunctionDefinition (fooB)\n| ParseFunctionDefinition (fooMTA)\n| ParseFunctionDefinition (fooA)\n| ParseDeclarationOrFunctionDefinition (test.cc:3:5)\n| | ParseFunctionDefinition (user)\n| PerformPendingInstantiations\n| | InstantiateFunction (fooA<int>, .\\a.h:7)\n| | | InstantiateFunction (fooB<int>, .\\b.h:3)\n| | | InstantiateFunction (fooMTA<int>, .\\a.h:4)\n" With diff: @@ -7,5 +7,5 @@ | | ParseFunctionDefinition (user) | PerformPendingInstantiations -| | InstantiateFunction (fooA<int>, ./a.h:7) -| | | InstantiateFunction (fooB<int>, ./b.h:3) -| | | InstantiateFunction (fooMTA<int>, ./a.h:4)\n +| | InstantiateFunction (fooA<int>, .\\a.h:7) +| | | InstantiateFunction (fooB<int>, .\\b.h:3) +| | | InstantiateFunction (fooMTA<int>, .\\a.h:4)\n C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\clang\unittests\Support\TimeProfilerTest.cpp:278 Expected equality of these values: R"( Frontend | ParseFunctionDefinition (fooB) | ParseFunctionDefinition (fooMTA) | ParseFunctionDefinition (fooA) | ParseDeclarationOrFunctionDefinition (test.cc:3:5) | | ParseFunctionDefinition (user) | PerformPendingInstantiations | | InstantiateFunction (fooA<int>, ./a.h:7) ``` Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250832
This is helpful in identifying files which contain the particular template decl.