Skip to content

Conversation

@jakobbotsch
Copy link
Member

Physical promotion was working under the assumption that reinterpreting GC pointers is undefined behavior, and would happily promote GC pointers as integers if it saw such accesses. However, physical promotion is function wide while the UB accesses can be happening in a restricted (dynamically unreachable) scope. This exact situation happens in MemoryExtensions.Contains. The issue was uncovered under jit stress where we did not fold away the guard early enough, meaning that promotion then saw a TYP_LONG access of a struct { object, int } and proceeded to promote it as such.

Fix #90602

No diffs are expected.

Physical promotion was working under the assumption that reinterpreting
GC pointers is undefined behavior, and would happily promote GC pointers
as integers if it saw such accesses. However, physical promotion is
function wide while the UB accesses can be happening in a restricted
(dynamically unreachable) scope. This exact situation happens in
MemoryExtensions.Contains. The issue was uncovered under jit stress
where we did not fold away the guard early enough, meaning that
promotion then saw a `TYP_LONG` access of a `struct { object, int }` and
proceeded to promote it as such.

Fix dotnet#90602
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 16, 2023
@ghost ghost assigned jakobbotsch Aug 16, 2023
@ghost
Copy link

ghost commented Aug 16, 2023

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

Issue Details

Physical promotion was working under the assumption that reinterpreting GC pointers is undefined behavior, and would happily promote GC pointers as integers if it saw such accesses. However, physical promotion is function wide while the UB accesses can be happening in a restricted (dynamically unreachable) scope. This exact situation happens in MemoryExtensions.Contains. The issue was uncovered under jit stress where we did not fold away the guard early enough, meaning that promotion then saw a TYP_LONG access of a struct { object, int } and proceeded to promote it as such.

Fix #90602

No diffs are expected.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch reopened this Aug 16, 2023
@jakobbotsch jakobbotsch marked this pull request as ready for review August 17, 2023 07:58
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @EgorBo

No diffs.

@jakobbotsch jakobbotsch requested a review from EgorBo August 17, 2023 08:48
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Does it need some fast path when lcl's layout is known to not contain any gc pointers? Although, I guess zero tp diffs say no.

@jakobbotsch
Copy link
Member Author

Does it need some fast path when lcl's layout is known to not contain any gc pointers? Although, I guess zero tp diffs say no.

Can't hurt to add it to the general helper function.

@jakobbotsch jakobbotsch merged commit 45acd38 into dotnet:main Aug 17, 2023
@jakobbotsch jakobbotsch deleted the fix-90602 branch August 17, 2023 14:16
@jakobbotsch
Copy link
Member Author

/azp backport to release/8.0

@jakobbotsch
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2023
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assert failure: !CREATE_CHECK_STRING(pMT && pMT->Validate())

3 participants