Skip to content
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

Left align demangled stacktrace output. #6191

Merged
merged 7 commits into from
Jun 9, 2023

Conversation

vbrunini
Copy link
Contributor

@vbrunini vbrunini commented Jun 6, 2023

Closes #6190.

@dalg24
Copy link
Member

dalg24 commented Jun 7, 2023

From the clang-format check on Jenkins CI

diff --git a/core/src/impl/Kokkos_Stacktrace.cpp b/core/src/impl/Kokkos_Stacktrace.cpp
index 6ce8d77a4..577e4cfb1 100644
--- a/core/src/impl/Kokkos_Stacktrace.cpp
+++ b/core/src/impl/Kokkos_Stacktrace.cpp
@@ -170,9 +170,10 @@ main_column_info find_main_column(const std::vector<std::string>& traceback) {
   return main_column_info{found_main, main_col};
 }
 
-void demangle_and_print_traceback_entry(
-    std::ostream& out, const std::string& traceback_entry,
-    const bool found_main, const size_t main_col) {
+void demangle_and_print_traceback_entry(std::ostream& out,
+                                        const std::string& traceback_entry,
+                                        const bool found_main,
+                                        const size_t main_col) {
   std::vector<std::string> tokens;
   size_t cur_col = 0;
   for_each_token(traceback_entry, [&](const std::string& s, bool last) {

@fnrizzi
Copy link
Contributor

fnrizzi commented Jun 7, 2023

can you please post a screenshot of what the output looks like after this PR vs what was before ?
It would help having that part of the PR, and i know there is one in the issue but would like to see the actual output of this PR

@vbrunini
Copy link
Contributor Author

vbrunini commented Jun 7, 2023

Before:

demangled test_f1:                                                                                                          Kokkos::Impl::save_stacktrace()       [0x494bb3]
                                                                                                 Test::stacktrace_test_f1(std::ostream&)       [0x43cf69]
                                                                                                       Test::test_stacktrace(bool, bool)       [0x438463]
                                                              Test::defaultdevicetype_DeathTest_stacktrace_generic_term_Test::TestBody()       [0x43a11c]
void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)       [0x472aad]
                                                                                                                                               [0x4649ca]
                                                                                                                                               [0x464e1d]
                                                                                                                                               [0x465631]
                                                                                          testing::internal::UnitTestImpl::RunAllTests()       [0x466d47]
                                                                                                                testing::UnitTest::Run()       [0x46507b]
                                                                                                                                    main       [0x4373f2]
                                                                                                                       __libc_start_main [0x7f3ece93a555]
                                                                                                                                               [0x438359]

After:

demangled test_f1:
Kokkos::Impl::save_stacktrace() [0x495713]
Test::stacktrace_test_f1(std::ostream&) [0x43dac9]
Test::test_stacktrace(bool, bool) [0x438543]
void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) [0x47360d]
 [0x46552a]
 [0x46597d]
 [0x466191]
testing::internal::UnitTestImpl::RunAllTests() [0x4678a7]
testing::UnitTest::Run() [0x465bdb]
main [0x4374a2]
__libc_start_main [0x7f9d87f9c555]
 [0x438409]

@fnrizzi
Copy link
Contributor

fnrizzi commented Jun 7, 2023

thanks @vbrunini
I think reading left-to-right makes sense, but IMO those addresses somehow get in the way.
What about this:

image

maybe more people will vote on this today at the meeting

@crtrott
Copy link
Member

crtrott commented Jun 7, 2023

Regarding the proposal for address alignment: is the address a separate string right now? It looks to me like its just part of the entire name?

@fnrizzi
Copy link
Contributor

fnrizzi commented Jun 7, 2023

Regarding the proposal for address alignment: is the address a separate string right now? It looks to me like its just part of the entire name?

i have not checked that, but seems so. I was just proposing what looked "nice" to my eye...

@masterleinad
Copy link
Contributor

We agreed on having the addresses first, then line numbers, and everything left-aligned.

@cz4rs cz4rs self-requested a review June 7, 2023 18:27
@vbrunini
Copy link
Contributor Author

vbrunini commented Jun 7, 2023

Now looks like:

demangled test_f1:
 [0x495713] Kokkos::Impl::save_stacktrace()
 [0x43dac9] Test::stacktrace_test_f1(std::ostream&)
 [0x438543] Test::test_stacktrace(bool, bool)
 [0x47360d] void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
 [0x46552a] 
 [0x46597d] 
 [0x466191] 
 [0x4678a7] testing::internal::UnitTestImpl::RunAllTests()
 [0x465bdb] testing::UnitTest::Run()
 [0x4374a2] main
 [0x7f16e9f60555] __libc_start_main
 [0x438409] 

Comment on lines 183 to 186
out << s;
}
if (!last) {
out << " ";
Copy link
Contributor

@cz4rs cz4rs Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably simplify even further and not worry about an extra space in the last line:

Suggested change
out << s;
}
if (!last) {
out << " ";
out << s << " " << demangle(s);

This would get rid of bool last parameter and the second loop.

@fnrizzi
Copy link
Contributor

fnrizzi commented Jun 8, 2023

CI seems unrelated

@masterleinad masterleinad added this to the Release 4.2 milestone Jun 9, 2023
@crtrott crtrott merged commit 43a797b into kokkos:develop Jun 9, 2023
27 of 28 checks passed
@dalg24
Copy link
Member

dalg24 commented Jun 9, 2023

Ignored HIP timeouts

@dalg24 dalg24 mentioned this pull request Jun 9, 2023
@nmm0 nmm0 modified the milestones: Release 4.2, Release 4.1 Jun 16, 2023
nliber pushed a commit to nliber/kokkos that referenced this pull request Jun 22, 2023
* Left align demangled stacktrace output.

Closes kokkos#6190.

* Remove now unused parameter to fix CI.

* Fix clang-format for CI.

* Print the address first

* Fix unused parameter Werror.

* Remove bool last parameter from for_each_token.

* Make clang-format happy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants