Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 10, 2025

impGetNodeAddr returns GTF_IND_COPYABLE_FLAGS (volatile, unaligned, nonfaulting, initclass) from the original access, but five call sites were ignoring these flags, potentially leading to incorrect semantics for volatile/unaligned/initclass operations.

Changes:

  • Added allowConfiguredDerefs parameter to impGetNodeAddr: When false, indirections with volatile, unaligned, or initclass flags fall through to temp creation instead of returning the address directly. Defaults to true for backward compatibility.

  • Return buffers (3 sites): Set allowConfiguredDerefs = false when obtaining addresses for return buffer arguments passed to helpers. VM helpers cannot handle addresses with volatile, unaligned, or initclass flags.

  • Struct field access on instance: Combine flags from impGetNodeAddr with prefix flags when creating the field indirection:

    GenTreeFlags objAddrFlags = GTF_EMPTY;
    obj = impGetNodeAddr(obj, CHECK_SPILL_ALL, &objAddrFlags);
    indirFlags |= objAddrFlags;  // Combine with prefix flags
  • Box-isinst optimization: Transfer flags to the hasValue field indirection:

    objToBox = impGetNodeAddr(objToBox, CHECK_SPILL_ALL, &indirFlags);
    impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox, indirFlags), ...);
  • Fixed recursive call: Preserve allowConfiguredDerefs in GT_COMMA case to maintain semantics through comma expressions.

All five TODO-Bug?: verify if flags matter here comments resolved.

Original prompt

This section details on the original issue you should resolve

<issue_title>Review usages of impGetNodeAddr with ignored flags</issue_title>
<issue_description>#85562 made impGetNodeAddr return flags the original access would use. Places that use the API but ignore the returned flags have been marked with // TODO-Bug?: verify if flags matter here.
All such cases should be reviewed and either documented to be fine or fixed to not ignore the flags.
cc @SingleAccretion </issue_description>

<agent_instructions>For uses that end up indirecting the address, simply transfer the flags on to that indirection.
For uses that do not indirect (e.g. passing the address to a VM helper), add a new argument to impGetNodeAddr about whether volatile and unaligned addresses should be allowed, and if not, fall through to the case that copies to a local.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@ Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch See info in [area-owners.md](https://github.com/dotnet/runtime/blob/main/docs/area-owners.md) if you want to be subscribed.
Issue Details

#85562 made impGetNodeAddr return flags the original access would use. Places that use the API but ignore the returned flags have been marked with // TODO-Bug?: verify if flags matter here.
All such cases should be reviewed and either documented to be fine or fixed to not ignore the flags.
cc @SingleAccretion

Author: MichalPetryka
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 10, 2025
@jakobbotsch jakobbotsch added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 10, 2025
Comment on lines 1129 to 1132
if (!allowVolatileUnaligned && ((flags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED)) != 0))
{
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

@copilot On second thought this needs to check GTF_IND_INITCLASS as well. Can you rename allowVolatileUnaligned to allowConfiguredDerefs at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 7c2aedd. The parameter is now renamed to allowConfiguredDerefs and the check now includes GTF_IND_INITCLASS in addition to volatile and unaligned flags.

Copilot AI changed the title [WIP] Review usages of impGetNodeAddr with ignored flags Fix impGetNodeAddr flag handling for volatile and unaligned accesses Dec 10, 2025
Copilot AI requested a review from jakobbotsch December 10, 2025 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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.

Review usages of impGetNodeAddr with ignored flags

2 participants