-
Notifications
You must be signed in to change notification settings - Fork 5.2k
fix backcompat issue for standalone GC #117793
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a backward compatibility issue with the standalone GC introduced by a previous change. The issue occurred because compilers (both MSVC and Clang) rearranged overloaded methods with the same name (IsPromoted
) in the vtable, breaking compatibility when loading a newer standalone GC DLL with older runtime versions.
- Renames the newer overloaded
IsPromoted
method toIsPromoted2
to prevent vtable ordering issues - Moves the
IsPromoted2
declaration toIGCHeapInternal
interface since it's only used by GC-internal code - Updates all call sites to use the renamed method
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/coreclr/gc/gcinterface.h | Removes the IsPromoted(Object*, bool) method from the public IGCHeap interface |
src/coreclr/gc/gc.h | Adds IsPromoted2(Object*, bool) method to the internal IGCHeapInternal interface |
src/coreclr/gc/gcimpl.h | Updates method declaration from IsPromoted to IsPromoted2 |
src/coreclr/gc/gc.cpp | Renames method implementation and updates internal call |
src/coreclr/gc/gcbridge.cpp | Updates call site to use IsPromoted2 method |
Comments suppressed due to low confidence (1)
src/coreclr/gc/gcimpl.h:129
- The method name 'IsPromoted2' is not descriptive and suggests this is a temporary workaround. Consider a more meaningful name like 'IsPromotedWithVerification' or 'IsPromotedEx' to better indicate its purpose.
bool IsPromoted2 (Object *object, bool bVerifyNextHeader);
Tagging subscribers to this area: @dotnet/gc |
/ba-g dotnet/dnceng#6004 |
#116310 breaks the backward compatibility for the standalone GC - it can no longer work with previous versions of the runtime on both windows and linux. this is easily reproducible by just loading a standalone GC dll built from main with say 8.0 runtime because it will fail as soon as it hits
FinalizeLoad
when coreclr is trying to load the standalone dll.both msvc and clang put the 2 methods, both named
IsPromoted
next to each other instead of maintaining the order as declared. renaming the new one toIsPromoted2
worked. but I also just madeIsPromoted2
onIGCHeapInternal
instead since it's only used by the GC side.