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

pkg/stack/unwind: Handle undefined return addresses in arm64 compact unwind table #2015

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

Sylfrena
Copy link
Contributor

@Sylfrena Sylfrena commented Sep 14, 2023

rbpType is not set correctly in case of Undefined Return Address in Link Register causing unwinding an extra frame despite reaching bottom frame(main()).

This was not caught by any tests in #1953(#1953) so add tests as well.

TEST PLAN

Before:

=== RUN   TestCompactUnwindTableArm64/RA_Rule_undefined
compact_unwind_table_test.go:217:                                                                                        
                Error Trace:    /Users/icebear/Snow/polar/parca-agent/pkg/stack/unwind/compact_unwind_table_test.go:217      
                Error:          Not equal:                                                                                   
                                expected: unwind.CompactUnwindTable{unwind.CompactUnwindTableRow{pc:0x7b, lrOffset:0, cfaType
:0x1, rbpType:0x0, cfaOffset:0, rbpOffset:0}}                                                                                
                                actual  : unwind.CompactUnwindTable{unwind.CompactUnwindTableRow{pc:0x7b, lrOffset:0, cfaType
:0x2, rbpType:0x4, cfaOffset:0, rbpOffset:0}}
--- FAIL: TestCompactUnwindTableArm64/RA_Rule_undefined (0.00s)

=== RUN   TestCompactUnwindTableX86/RA_Rule_undefined
compact_unwind_table_test.go:407: 
Error Trace:    /Users/icebear/Snow/polar/parca-agent/pkg/stack/unwind/compact_unwind_table_test.go:407
Error:          Not equal: 
                expected: unwind.CompactUnwindTable{unwind.CompactUnwindTableRow{pc:0x7b, lrOffset:-16, cfaType
:0x2, rbpType:0x4, cfaOffset:0, rbpOffset:0}}
                actual  : unwind.CompactUnwindTable{unwind.CompactUnwindTableRow{pc:0x7b, lrOffset:0, cfaType
:0x1, rbpType:0x0, cfaOffset:0, rbpOffset:0}}
--- FAIL: TestCompactUnwindTableX86/RA_Rule_undefined (0.00s)  

After:

- PASS: TestCompactUnwindTableX86 (0.00s)                                                                                  
	- PASS: TestCompactUnwindTableX86/CFA_with_Offset_on_x86_64_stack_pointer (0.00s)
	- PASS: TestCompactUnwindTableX86/CFA_with_Offset_on_x86_64_frame_pointer (0.00s)
	- PASS: TestCompactUnwindTableX86/CFA_known_expression_PLT_1 (0.00s)
	- PASS: TestCompactUnwindTableX86/CFA_known_expression_PLT_2 (0.00s)
	- PASS: TestCompactUnwindTableX86/CFA_not_known_expression (0.00s)     
 	- PASS: TestCompactUnwindTableX86/RA_Rule_undefined (0.00s)            
 	- PASS: TestCompactUnwindTableX86/RBP_offset (0.00s)                   
 	- PASS: TestCompactUnwindTableX86/RBP_register (0.00s)                 
 	- PASS: TestCompactUnwindTableX86/RBP_expression (0.00s)               
 	- PASS: TestCompactUnwindTableX86/Invalid_CFA_rule_returns_error (0.00s)

-- PASS: TestCompactUnwindTableArm64 (0.00s)
	- PASS: TestCompactUnwindTableArm64/CFA_with_Offset_on_Arm64_stack_pointer (0.00s)
	- PASS: TestCompactUnwindTableArm64/CFA_with_Offset_on_Arm64_frame_pointer (0.00s)                       
	- PASS: TestCompactUnwindTableArm64/CFA_known_expression_PLT_1 (0.00s)                                   
	- PASS: TestCompactUnwindTableArm64/CFA_known_expression_PLT_2 (0.00s)                                   
	- PASS: TestCompactUnwindTableArm64/CFA_not_known_expression (0.00s)                                     
	- PASS: TestCompactUnwindTableArm64/RA_Rule_undefined (0.00s)                                            
	- PASS: TestCompactUnwindTableArm64/RBP_offset (0.00s)                                                   
	- PASS: TestCompactUnwindTableArm64/RBP_register (0.00s)
	- PASS: TestCompactUnwindTableArm64/RBP_expression (0.00s)
	- PASS: TestCompactUnwindTableArm64/Invalid_CFA_rule_returns_error (0.00s)

@Sylfrena Sylfrena requested a review from a team as a code owner September 14, 2023 07:26
@Sylfrena Sylfrena changed the title pkg/stack/unwind: Add regression test for unwind table generation for… pkg/stack/unwind: Add regression test for unwind table generation for Arm64 Sep 14, 2023
… Arm64

`rbpType` is not set correctly in case of Undefined Return Address in Link Register
causing unwinding an extra frame despite reaching bottom frame(main()).

This was not caught by any tests in #1953(#1953)
so add tests as well.

TEST PLAN
=========
Before:
```
=== RUN   TestCompactUnwindTableArm64/RA_Rule_undefined
compact_unwind_table_test.go:217:                                                                                        
                Error Trace:    /Users/icebear/Snow/polar/parca-agent/pkg/stack/unwind/compact_unwind_table_test.go:217      
                Error:          Not equal:                                                                                   
                                expected: unwind.CompactUnwindTable{unwind.CompactUnwindTableRow{pc:0x7b, lrOffset:0, cfaType
:0x1, rbpType:0x0, cfaOffset:0, rbpOffset:0}}                                                                                
                                actual  : unwind.CompactUnwindTable{unwind.CompactUnwindTableRow{pc:0x7b, lrOffset:0, cfaType
:0x2, rbpType:0x4, cfaOffset:0, rbpOffset:0}}
--- FAIL: TestCompactUnwindTableArm64/RA_Rule_undefined (0.00s)

=== RUN   TestCompactUnwindTableX86/RA_Rule_undefined
compact_unwind_table_test.go:407: 
Error Trace:    /Users/icebear/Snow/polar/parca-agent/pkg/stack/unwind/compact_unwind_table_test.go:407
Error:          Not equal: 
                expected: unwind.CompactUnwindTable{unwind.CompactUnwindTableRow{pc:0x7b, lrOffset:-16, cfaType
:0x2, rbpType:0x4, cfaOffset:0, rbpOffset:0}}
                actual  : unwind.CompactUnwindTable{unwind.CompactUnwindTableRow{pc:0x7b, lrOffset:0, cfaType
:0x1, rbpType:0x0, cfaOffset:0, rbpOffset:0}}
--- FAIL: TestCompactUnwindTableX86/RA_Rule_undefined (0.00s)  
```

After:
```
- PASS: TestCompactUnwindTableX86 (0.00s)                                                                                  
	- PASS: TestCompactUnwindTableX86/CFA_with_Offset_on_x86_64_stack_pointer (0.00s)
	- PASS: TestCompactUnwindTableX86/CFA_with_Offset_on_x86_64_frame_pointer (0.00s)
	- PASS: TestCompactUnwindTableX86/CFA_known_expression_PLT_1 (0.00s)
	- PASS: TestCompactUnwindTableX86/CFA_known_expression_PLT_2 (0.00s)
	- PASS: TestCompactUnwindTableX86/CFA_not_known_expression (0.00s)     
 	- PASS: TestCompactUnwindTableX86/RA_Rule_undefined (0.00s)            
 	- PASS: TestCompactUnwindTableX86/RBP_offset (0.00s)                   
 	- PASS: TestCompactUnwindTableX86/RBP_register (0.00s)                 
 	- PASS: TestCompactUnwindTableX86/RBP_expression (0.00s)               
 	- PASS: TestCompactUnwindTableX86/Invalid_CFA_rule_returns_error (0.00s)

-- PASS: TestCompactUnwindTableArm64 (0.00s)
	- PASS: TestCompactUnwindTableArm64/CFA_with_Offset_on_Arm64_stack_pointer (0.00s)
	- PASS: TestCompactUnwindTableArm64/CFA_with_Offset_on_Arm64_frame_pointer (0.00s)                       
	- PASS: TestCompactUnwindTableArm64/CFA_known_expression_PLT_1 (0.00s)                                   
	- PASS: TestCompactUnwindTableArm64/CFA_known_expression_PLT_2 (0.00s)                                   
	- PASS: TestCompactUnwindTableArm64/CFA_not_known_expression (0.00s)                                     
	- PASS: TestCompactUnwindTableArm64/RA_Rule_undefined (0.00s)                                            
	- PASS: TestCompactUnwindTableArm64/RBP_offset (0.00s)                                                   
	- PASS: TestCompactUnwindTableArm64/RBP_register (0.00s)
	- PASS: TestCompactUnwindTableArm64/RBP_expression (0.00s)
	- PASS: TestCompactUnwindTableArm64/Invalid_CFA_rule_returns_error (0.00s)
```

Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
Copy link
Contributor

@javierhonduco javierhonduco left a comment

Choose a reason for hiding this comment

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

Good stuff, thanks for adding the regression tests for x86 as well.

nit: Could you rename the commit to explain the "main" change, something "Handle undefined return addresses in arm64 compact unwind table"

@Sylfrena Sylfrena changed the title pkg/stack/unwind: Add regression test for unwind table generation for Arm64 pkg/stack/unwind: Handle undefined return addresses in arm64 compact unwind table Sep 14, 2023
@Sylfrena Sylfrena enabled auto-merge (squash) September 14, 2023 07:48
@Sylfrena Sylfrena merged commit d7ca617 into main Sep 14, 2023
22 checks passed
@javierhonduco javierhonduco deleted the fix-undefined-ra branch September 14, 2023 08:17
Sylfrena added a commit to parca-dev/testdata that referenced this pull request Sep 14, 2023
xref: parca-dev/parca-agent#2015

Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
Sylfrena added a commit to parca-dev/testdata that referenced this pull request Sep 14, 2023
…ies (#20)

* testdata: Add Arm64 libc.so binaries and updated tables for cpp binaries

xref: parca-dev/parca-agent#2015

Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
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

2 participants