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

[llvm-gsymutil] Fix flaky test #123814

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

alx32
Copy link
Contributor

@alx32 alx32 commented Jan 21, 2025

The macho-gsym-merged-callsites-dsym.yaml test was failing with expensive checks on. This is because we still can't reliably associate line table information to functions until #123391 is merged.

Example failure: https://lab.llvm.org/buildbot/#/builders/16/builds/12374

@alx32 alx32 marked this pull request as ready for review January 21, 2025 20:23
@llvmbot
Copy link
Member

llvmbot commented Jan 21, 2025

@llvm/pr-subscribers-debuginfo

Author: None (alx32)

Changes

The macho-gsym-merged-callsites-dsym.yaml test was failing with expensive checks on. This is because we still can't reliably associate line table information to functions until #123391 is merged.


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

1 Files Affected:

  • (modified) llvm/test/tools/llvm-gsymutil/ARM_AArch64/macho-gsym-merged-callsites-dsym.yaml (+6-4)
diff --git a/llvm/test/tools/llvm-gsymutil/ARM_AArch64/macho-gsym-merged-callsites-dsym.yaml b/llvm/test/tools/llvm-gsymutil/ARM_AArch64/macho-gsym-merged-callsites-dsym.yaml
index b9e18dee3238f6..bba92befc089b0 100644
--- a/llvm/test/tools/llvm-gsymutil/ARM_AArch64/macho-gsym-merged-callsites-dsym.yaml
+++ b/llvm/test/tools/llvm-gsymutil/ARM_AArch64/macho-gsym-merged-callsites-dsym.yaml
@@ -1,5 +1,7 @@
 ## Test that reconstructs a dSYM file from YAML and generates a gsym from it. The gsym has callsite info and merged functions.
 
+# TODO: Add line numbers instead of ':{{.}}' below - after introducing accurate DIE-based lookups: https://github.com/llvm/llvm-project/pull/123391
+
 # RUN: split-file %s %t
 # RUN: yaml2obj %t/merged_callsites.dSYM.yaml -o %t/merged_callsites.dSYM
 
@@ -61,12 +63,12 @@
 
 # RUN: llvm-gsymutil %t/dwarf_call_sites_dSYM.gsym --merged-functions --address=0x000000010000035c --merged-functions-filter="function3_copy1" | FileCheck --check-prefix=CHECK-C3 %s
 # CHECK-C3:       Found 1 function at address 0x000000010000035c:
-# CHECK-C3-NEXT:     0x000000010000035c: function3_copy1 + 16 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:28
+# CHECK-C3-NEXT:     0x000000010000035c: function3_copy1 + 16 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:{{.}}
 # CHECK-C3-NEXT:        CallSites: function4_copy1
 
 # RUN: llvm-gsymutil %t/dwarf_call_sites_dSYM.gsym --merged-functions --address=0x0000000100000340 --merged-functions-filter="function4_copy1" | FileCheck --check-prefix=CHECK-C4 %s
 # CHECK-C4:       Found 1 function at address 0x0000000100000340:
-# CHECK-C4-NEXT:     0x0000000100000340: function4_copy1 + 8 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:14
+# CHECK-C4-NEXT:     0x0000000100000340: function4_copy1 + 8 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:{{.}}
 
 ### ----------------------------------------------------------------------------------------------------------------------------------
 ### Resolve the 2nd call stack - the 2nd and 3rd addresses are the same but they resolve to a different function because of the filter
@@ -78,12 +80,12 @@
 
 # RUN: llvm-gsymutil %t/dwarf_call_sites_dSYM.gsym --merged-functions --address=0x000000010000035c --merged-functions-filter="function3_copy2" | FileCheck --check-prefix=CHECK-C6 %s
 # CHECK-C6:       Found 1 function at address 0x000000010000035c:
-# CHECK-C6-NEXT:     0x000000010000035c: function3_copy2 + 16 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:28
+# CHECK-C6-NEXT:     0x000000010000035c: function3_copy2 + 16 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:{{.}}
 # CHECK-C6-NEXT:        CallSites: function4_copy2
 
 # RUN: llvm-gsymutil %t/dwarf_call_sites_dSYM.gsym --merged-functions --merged-functions-filter="function4_copy2" --address=0x0000000100000340 | FileCheck --check-prefix=CHECK-C7 %s
 # CHECK-C7:       Found 1 function at address 0x0000000100000340:
-# CHECK-C7-NEXT:     0x0000000100000340: function4_copy2 + 8 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:14
+# CHECK-C7-NEXT:     0x0000000100000340: function4_copy2 + 8 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:{{.}}
 
 
 #--- merged_funcs_test.cpp

# CHECK-C6-NEXT: CallSites: function4_copy2

# RUN: llvm-gsymutil %t/dwarf_call_sites_dSYM.gsym --merged-functions --merged-functions-filter="function4_copy2" --address=0x0000000100000340 | FileCheck --check-prefix=CHECK-C7 %s
# CHECK-C7: Found 1 function at address 0x0000000100000340:
# CHECK-C7-NEXT: 0x0000000100000340: function4_copy2 + 8 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:14
# CHECK-C7-NEXT: 0x0000000100000340: function4_copy2 + 8 @ /tmp/tst{{[/\\]}}out/merged_funcs_test.cpp:{{.}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully we can eventually use [[#@LINE+N]] and --leading-lines to test these line numbers. Also, where does the /tmp/tst file path prefix come from?

https://llvm.org/docs/TestingGuide.html#extra-files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the paths and line numbers come from merged_callsites.dSYM.yaml - which is binary encoded. So the input is hard-coded in binary format effectively.
[[#@LINE+N]] won't work in this case - we can update the lines in the test but the line numbers to test won't change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but presumably those line numbers come from merged_funcs_test.cpp? Then we can in fact use the [[#@LINE+N]] trick if we make sure to use --leading-lines. We may have to regenerate the files so they are correct, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if we move #--- merged_funcs_test.cpp towards the beginning of the file it makes it less likely that the line numbers will change when we update the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see - one issue is that this test isn't really easily manually updatable because of hardcoded values in callsites.yaml. So using [[#@LINE+N]] we could have that part auto-match but we still have to manually edit callsites.yaml.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any line numbers in callsites.yaml. What would you have to update? Is there anyway it could be generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The callsites.yaml has return_offset: [0x10] which is a binary offset of a callsite into the function. There's no way to generate this right now - it's supposed to be test data generated from looking at the binary.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it sounds like callsites.yaml needs to be changed every time we regenerate the input files, which seems to be an orthogonal problem to using @LINE+N.

@alx32 alx32 merged commit bfafbe3 into llvm:main Jan 21, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants