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

Update MethodTable::IsDynamicStatics and DacpMethodTableData::bIsDynamic to return 0/1 instead of flag value #106504

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Aug 15, 2024

MethodTable::IsDynamicStatics just gets the flag

inline DWORD IsDynamicStatics()
{
LIMITED_METHOD_DAC_CONTRACT;
return GetFlag(enum_flag_DynamicStatics);
}

So if it is set, it returns the flag value (0x2)

DAC just sets DacpMethodTableData::bIsDynamic to MethodTable::IsDynamicStatics.

Change both to return TRUE/FALSE instead of the flag value. Per #106504 (comment), this was the behaviour until somewhat recently.

Copy link
Contributor

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

@lambdageek
Copy link
Member

@elinor-fung before eb8f54d MethodTable::IsDynamicStatics used to be:

   return !TestFlagWithMask(enum_flag_StaticsMask, enum_flag_StaticsMask_NonDynamic);

which is always 0 or 1 because of how ! works. So I think it's probably safe to update the runtime (and indirectly fix the value returned by the brittle DAC)

@elinor-fung elinor-fung changed the title Update assert in GetMethodTableData for cDAC/DAC bIsDynamic values Update MethodTable::IsDynamicStatics and DacpMethodTableData::bIsDynamic to return 0/1 instead of flag value Aug 15, 2024
@elinor-fung
Copy link
Member Author

I went with both - switching the runtime back to always return 0/1 and also switching the DAC to always return 0/1 (so it won't be affected by any inadvertent change in the value from the runtime).

@lambdageek
Copy link
Member

lambdageek commented Aug 15, 2024

I'm not sure whether this meets the bar for backports to RC1, but this now leaves net9.0 as the only runtime that returns non-0/1.

@hoyosjs
Copy link
Member

hoyosjs commented Aug 15, 2024

It likely is OK for rc2. Easy enough to do a bar check.

@elinor-fung elinor-fung merged commit dab77ac into dotnet:main Aug 16, 2024
90 checks passed
@elinor-fung elinor-fung deleted the cdac-mt-data-assert branch August 16, 2024 16:29
@elinor-fung
Copy link
Member Author

/backport to release/9.0

Copy link
Contributor

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10457525842

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.

5 participants