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

JIT optimization breaks pointer/ref arithmetic #61510

Closed
AndrejStanisev opened this issue Nov 12, 2021 · 6 comments · Fixed by #61518
Closed

JIT optimization breaks pointer/ref arithmetic #61510

AndrejStanisev opened this issue Nov 12, 2021 · 6 comments · Fixed by #61518
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug
Milestone

Comments

@AndrejStanisev
Copy link

Description

We allocate some unmanaged memory that we then manipulate via references rather than raw pointers. We use the following trick to get a ref byte from an address stored as nint.

public static ref byte GetReference(nint addr) => ref Unsafe.AddByteOffset(ref Unsafe.NullRef<byte>(), addr);

This works correctly in .NET 5, but returns a null pointer in .NET 6, where this code gets JITed to return 0 unconditionally, as follows:

00007FFF71040FB0  xor         eax,eax  
00007FFF71040FB2  ret  

Reproduction Steps

class Program
{

    public static void Main()
    {
        ref var a = ref GetReference(10_000);
        Console.WriteLine(a);
    }

    [MethodImpl(MethodImplOptions.AggressiveOptimization)]
    public static ref byte GetReference(nint addr) => ref Unsafe.AddByteOffset(ref Unsafe.NullRef<byte>(), addr);
}

Expected behavior

The JIT should output code performing arithmetic with the given parameter.

Actual behavior

JIT outputs code returning 0 unconditionally.

Regression?

The example works correctly in .NET 5.

Known Workarounds

No response

Configuration

.NET Version 6.0.100
Windows Version 10.0.19042 Build 19042
x64

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Nov 12, 2021
@ghost
Copy link

ghost commented Nov 12, 2021

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

We allocate some unmanaged memory that we then manipulate via references rather than raw pointers. We use the following trick to get a ref byte from an address stored as nint.

public static ref byte GetReference(nint addr) => ref Unsafe.AddByteOffset(ref Unsafe.NullRef<byte>(), addr);

This works correctly in .NET 5, but returns a null pointer in .NET 6, where this code gets JITed to return 0 unconditionally, as follows:

00007FFF71040FB0  xor         eax,eax  
00007FFF71040FB2  ret  

Reproduction Steps

class Program
{

    public static void Main()
    {
        ref var a = ref GetReference(10_000);
        Console.WriteLine(a);
    }

    [MethodImpl(MethodImplOptions.AggressiveOptimization)]
    public static ref byte GetReference(nint addr) => ref Unsafe.AddByteOffset(ref Unsafe.NullRef<byte>(), addr);
}

Expected behavior

The JIT should output code performing arithmetic with the given parameter.

Actual behavior

JIT outputs code returning 0 unconditionally.

Regression?

The example works correctly in .NET 5.

Known Workarounds

No response

Configuration

.NET Version 6.0.100
Windows Version 10.0.19042 Build 19042
x64

Other information

No response

Author: Nefron
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@SingleAccretion SingleAccretion added this to the 7.0.0 milestone Nov 12, 2021
@Sergio0694
Copy link
Contributor

Sergio0694 commented Nov 12, 2021

Not related to the actual codegen issue, but... Wouldn't it be more compact and also easier to read to implement that GetReference method as just ref *(byte*)addr? It has less extra steps and it's also a workaround for this issue:

public static unsafe ref byte M(nint p)
{
    return ref *(byte*)p;
}
L0000: mov rax, rcx
L0003: ret

@AndrejStanisev
Copy link
Author

Not related to the actual codegen issue, but... Wouldn't it be more compact and also easier to read to implement that GetReference method as just ref *(byte*)addr? It has less extra steps and it's also a workaround for this issue:

I went with ref Unsafe.AsRef<byte>((void*)addr); but your way works too. We were initially avoiding having to designated the method as unsafe.

@Sergio0694
Copy link
Contributor

Having fake "safe" code using APIs like this instead of properly using built-in unsafe syntax is objectively worse and should be avoided. It makes it harder to see when code is actually doing unsafe things. It effectively makes your code more unsafe. It's an ongoing problem that has been here forever, certainly not helped by the existence of some APIs (looking at you Marshal...).

There was a good comment by @tannergooding with some guidance on this (or was it a Twitter thread?), can't find it now 😄

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Nov 12, 2021

So the immediate cause of this is the following morph transform:

runtime/src/coreclr/jit/morph.cpp

Lines 12402 to 12423 in 35704e4

if (op2->IsCnsIntOrI() && varTypeIsIntegralOrI(typ))
{
CLANG_FORMAT_COMMENT_ANCHOR;
// Fold (x + 0).
if ((op2->AsIntConCommon()->IconValue() == 0) && !gtIsActiveCSE_Candidate(tree))
{
// If this addition is adding an offset to a null pointer,
// avoid the work and yield the null pointer immediately.
// Dereferencing the pointer in either case will have the
// same effect.
if (!optValnumCSE_phase && varTypeIsGC(op2->TypeGet()) &&
((op1->gtFlags & GTF_ALL_EFFECT) == 0))
{
op2->gtType = tree->gtType;
DEBUG_DESTROY_NODE(op1);
DEBUG_DESTROY_NODE(tree);
return op2;
}

I do not know why this wasn't an issue in 5.0, this code is fairly ancient.

There is a "bug in a bug" here: when we swap operands just above the quoted code, we may lose the GC-ness of a byref: ADD(byref 0, long V00) => ADD(long V00, byref 0) => long V00 (you will see this in diff dumps if you disable the optimization).

Edit: I will take this then.

@SingleAccretion SingleAccretion self-assigned this Nov 12, 2021
@SingleAccretion SingleAccretion removed the untriaged New issue has not been triaged by the area owner label Nov 12, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 12, 2021
@JulieLeeMSFT
Copy link
Member

Thanks @SingleAccretion for taking on this one.

CC @dotnet/jit-contrib

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 15, 2021
@SingleAccretion SingleAccretion removed their assignment Nov 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants