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

Fix assert for ARM shuffle thunk #66642

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Conversation

BruceForstall
Copy link
Member

Handle case where gap exists at the beginning of the stack
range.

Fix #13241

Handle case where gap exists at the beginning of the stack
range.

Fix dotnet#13241
@BruceForstall
Copy link
Member Author

This is a possible fix for #13241. It looks to me like the assert didn't handle the case of the "gap" at the beginning, even though the code did.

For the test case at #13241 (comment), the generated thunk is:

0511d0bc b570     push        {r4-r6,lr}
0511d0be f8d0c010 ldr         r12,[r0,#0x10]
0511d0c2 f20d0410 addw        r4,sp,#0x10
0511d0c6 f20d0510 addw        r5,sp,#0x10
0511d0ca 4608     mov         r0,r1
0511d0cc 3420     adds        r4,r4,#0x20   // gap
0511d0ce f8541b04 ldr         r1,[r4],#4
0511d0d2 f8542b04 ldr         r2,[r4],#4
0511d0d6 f8543b04 ldr         r3,[r4],#4
0511d0da f8546b04 ldr         r6,[r4],#4
0511d0de 3520     adds        r5,r5,#0x20  // gap
0511d0e0 f8456b04 str         r6,[r5],#4
0511d0e4 f8546b04 ldr         r6,[r4],#4
0511d0e8 f8456b04 str         r6,[r5],#4
0511d0ec f8546b04 ldr         r6,[r4],#4
0511d0f0 f8456b04 str         r6,[r5],#4
0511d0f4 f8cdc00c str         r12,[sp,#0xC]
0511d0f8 bd70     pop         {r4-r6,pc}

with the "// gap" lines being generated when the assertion was previously hitting.

Now, I don't know enough about shuffle thunks to know if the gaps are supposed to be there or not...

@BruceForstall
Copy link
Member Author

@janvorli Who knows about (ARM) shuffle thunks?

cc @jakobbotsch

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, I've ran 50000 callers in the pinvoke stress test without any mismatches so seems the existing code does handle it correctly as you suspected.
You can either reenable the stress test here or I can do it in #66553 after you merge this.

@BruceForstall BruceForstall merged commit 9f2ff3f into dotnet:main Mar 15, 2022
@BruceForstall BruceForstall deleted the Fix13241 branch March 15, 2022 18:41
@BruceForstall
Copy link
Member Author

You can either reenable the stress test here or I can do it in #66553 after you merge this.

@jakobbotsch I went ahead and merged; I'll let you re-enable the stress test in #66553

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
Handle case where gap exists at the beginning of the stack
range.

Fix dotnet#13241
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[arm32/linux] Assert failure: dwSrcIndex > dwLastSrcIndex in ABI stress
3 participants