Skip to content

JIT: unpin locals fed by non-GC typed values #130002

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.

Sign up for GitHub

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

Closed
AndyAyersMS wants to merge 1 commit into from
+6 −0
Closed

JIT: unpin locals fed by non-GC typed values #130002

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,6 +1222,12 @@ struct GenTree

bool IsNotGcDef() const
{
// A non-GC typed value (eg native int reinterpreted as byref) cannot designate a movable object.
if (!varTypeIsGC(TypeGet()))

@EgorBo EgorBo Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume this sort of marks any untracked GC pointer as UB even if previously it worked, like

fixed (... = Unsafe.AsPointer(gcRef))

cc @dotnet/jit-contrib in case if you see any concern

@SingleAccretion SingleAccretion Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

any concern

This is a breaking change. I don't see how it can be justified, since the pinned state of a value is a dynamic property. Here we only know that the value is pinned at the def point, but we need to know that it "would be" pinned for at least the lifetime the caller is concerned about.

@jkotas jkotas Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ECMA spec explicitly talks about "Start GC tracking" and "Stop GC tracking" for conversions.

@AndyAyersMS AndyAyersMS Jun 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes this is too broad.

Seem like though that a non-gc parameter could possibly be handled here. If the parameter actually referred to GC memory whoever passed the argument would need to have pinned or they'd already be broken.

@SingleAccretion SingleAccretion Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the parameter actually referred to GC memory whoever passed the argument would need to have pinned or they'd already be broken.

I don't think parameters are special in any way. The pinning state can be dynamically altered for them just as for normal variables (through opaque means such as calls, inter-thread sync, GC handles, aliased pointers, etc).

@SingleAccretion SingleAccretion Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems to me the analysis is also reliant on the "global" (or at least per-method) perspective, since a "true" return value is treated as representing a value that can't be made movable 'no matter what'. It would require a very sophisticated analysis indeed to prove that the pinned state of a value [that may be movable] doesn't change beyond its use in a def, in the general case.

@hamarb123 hamarb123 Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As others have seemingly already mentioned here 😅: #129993 (comment)

@AndyAyersMS AndyAyersMS Jun 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The global analysis is flow insensitive so def vs use ordering shouldn't matter?

@SingleAccretion SingleAccretion Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The global analysis is flow insensitive so def vs use ordering shouldn't matter?

What I meant is basically this:

void* p = GetPin(); // 'p' pinned here via e. g. a handle, the virtual 'pin ref count' increases: 0 -> 1

pinned j = p; // It increases again: 1 -> 2
Unpin(p); // Decreases: 2 -> 1

// ...
// 'j's value still pinned here
// ...

return; // Decreases again: 1 -> 0, unpinned.

as the situation where the pinned state (analogous to a ref count) "changes beyond its use in a def [here the j = p def]". We know that the value represented by p at the point of the def must be pinned [otherwise we have UB], but beyond that point it must be proven by some other means.

@AndyAyersMS AndyAyersMS Jun 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks all. Didn't appreciate that we have these non-lexical pinning mechanisms which makes any kind of local inference unsound.

{
return true;
}
Comment on lines +1225 to +1229
Comment on lines +1225 to +1229

if (IsIntegralConst(0) || OperIs(GT_LCL_ADDR, GT_LCLHEAP))
{
return true;
Expand Down
Loading