Skip to content

Conversation

@jkotas
Copy link
Member

@jkotas jkotas commented Dec 12, 2020

The C11 standard treats trivial infinite loops as undefined behavior and allows
compilers to optimize them out completely.

Add "volatile" on FC_NO_TAILCALL to defeat this optimization

The C11 standard treats trivial infinite loops as undefined behavior and allows
compilers to optimize them out completely.

Add "volatile" on FC_NO_TAILCALL to defeat this optimization
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas
Copy link
Member Author

jkotas commented Dec 12, 2020

@trylek This should fix the intermittent crash that you have seen on x86

@jkotas
Copy link
Member Author

jkotas commented Dec 12, 2020

From C standard, 6.8.5:
An iteration statement whose controlling expression is not a constant expression, that performs no input/output operations, does not access volatile objects, and performs no synchronization or atomic operations in its body, controlling expression, or (in the case of a for statement) its expression-3, may be assumed by the implementation to terminate.

@jkotas
Copy link
Member Author

jkotas commented Dec 12, 2020

coreclr!ObjectNative::GetClass on Win x86

Before this change:

...
6210ec3d 8b4614          mov     eax,dword ptr [esi+14h]
6210ec40 8b401c          mov     eax,dword ptr [eax+1Ch]
6210ec43 8b4024          mov     eax,dword ptr [eax+24h]
6210ec46 85c9            test    ecx,ecx
6210ec48 7508            jne     coreclr!ObjectNative::GetClass+0x32 (6210ec52)
6210ec4a 8bca            mov     ecx,edx
6210ec4c 5e              pop     esi
6210ec4d e917000000      jmp     coreclr!GetClassHelper (6210ec69) <- This is bug. It cannot be a tailcall for the helper frame unwinder to work
...

After this change (matches what shipped in .NET 5):

...
7990018d 8b4614          mov     eax,dword ptr [esi+14h]
79900190 8b401c          mov     eax,dword ptr [eax+1Ch]
79900193 8b4024          mov     eax,dword ptr [eax+24h]
79900196 85c9            test    ecx,ecx
79900198 7512            jne     CoreCLR!ObjectNative::GetClass+0x3c (799001ac)
7990019a 8bca            mov     ecx,edx
7990019c e822000000      call    CoreCLR!GetClassHelper (799001c3) <- No tail call
799001a1 833d9477be7900  cmp     dword ptr [CoreCLR!FC_NO_TAILCALL (79be7794)],0
799001a8 75e1            jne     CoreCLR!ObjectNative::GetClass+0x1b (7990018b)
799001aa ebf5            jmp     CoreCLR!ObjectNative::GetClass+0x31 (799001a1)
...

@jkotas
Copy link
Member Author

jkotas commented Dec 12, 2020

cc @hoyosjs . This is fixing a different problem, but it is in the same area as #45906

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@trylek trylek merged commit b8be103 into dotnet:master Dec 13, 2020
@jkotas jkotas deleted the FC_NO_TAILCALL branch December 13, 2020 22:27
@ghost ghost locked as resolved and limited conversation to collaborators Jan 12, 2021
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.

3 participants