-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[GCStress] JIT/Methodical/tailcall/_il_dbgreference_i Assert failure SanityCheck() #41555
Comments
Between two runs of gcstress0x3-gcstress0xc and gcstress-extra, I grabbed four core dumps. I was able to repro on Windows arm32, but not under a debugger (at least not yet). I finally got a stack trace from gdb from a core dump:
|
This test has to pass by-refs through the arg buffer. Could it be the following assumption that is not ok on ARM32 when we are loading the by-ref from the arg buffer? runtime/src/coreclr/src/vm/tailcallhelp.cpp Lines 635 to 640 in 2d463ee
Note also that since #41206 we are no longer loading from the arg buffer into a by-ref typed local first. I wonder if this could have any adverse effects as well. cc @jkotas |
Is this assumption ever ok? This looks super fragile at best. This may need to use |
I can look into making that change this weekend. IIRC I was initially using |
I do not think you need |
Aha, this would probably have been my mistake last time around then. Is there a similar possibility for the store into the arg buffer? |
I think your best bet is to stay close to the patterns that |
fwiw, it appears we're looking for GC refs in the "rems" function ad address 0xe5f51dbd with code:
Maybe untracked slot 5. |
(btw, is the |
@jakobbotsch With your suggestion above about a possible problem, wouldn't I expect to see the GC problem in the generated stub code? Assuming the stack I show is correct, and we're reporting GC refs in the |
I got it to fail on Windows arm32 under windbg (after 143 runs), with a better stack:
code:
In this case, the value of
StoreTailCall stub:
store tail call args 2
|
@jakobbotsch I annotated a few places in the |
No, looks like old leftover code. This one looks unused too: runtime/src/coreclr/src/vm/tailcallhelp.cpp Line 375 in 341b8b2
I can clean this up.
Right, the issue would be in |
Switch to using ByReference instead of using stind.i/ldind.i and relying on the JIT to report the locations being moved between. Fixes dotnet#41555
* Properly handle byrefs in tailcall helper stubs Switch to using ByReference instead of using stind.i/ldind.i and relying on the JIT to report the locations being moved between. Fixes #41555 * Move NextCallReturnAddress call back
Switch to using ByReference instead of using stind.i/ldind.i and relying on the JIT to report the locations being moved between. Fixes #41555
@jakobbotsch Thanks a lot for fixing this issue. |
Reopening this issue in order to wait for completing backport to RC2. |
…1875) * Properly handle byrefs in tailcall helper stubs Switch to using ByReference instead of using stind.i/ldind.i and relying on the JIT to report the locations being moved between. Fixes #41555 * Move NextCallReturnAddress call back Co-authored-by: Jakob Botsch Nielsen <[email protected]>
RC2 merge is now done. |
https://dev.azure.com/dnceng/public/_build/results?buildId=793659&view=results
https://helix.dot.net/api/2019-06-17/jobs/d2e0c1f9-e5dc-4d0c-9252-d4f2d7fe271c/workitems/JIT.Methodical/console
The text was updated successfully, but these errors were encountered: