Reapply "Replace CordbProcess::GetSharedDomain with GetAppDomain"#117224
Merged
elinor-fung merged 2 commits intodotnet:mainfrom Jul 3, 2025
Merged
Reapply "Replace CordbProcess::GetSharedDomain with GetAppDomain"#117224elinor-fung merged 2 commits intodotnet:mainfrom
elinor-fung merged 2 commits intodotnet:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the ID cookie passed to the CordbBase constructor in CordbAssembly by always using vmAssembly instead of conditionally choosing between vmAssembly and vmDomainAssembly.
- Removed the
vmDomainAssembly.IsNull()check in the constructor initializer list. - Now always calls
VmPtrToCookie(vmAssembly)for the assembly ID.
Comments suppressed due to low confidence (2)
src/coreclr/debug/di/rsassembly.cpp:29
- Add a unit test scenario where the same assembly is loaded into multiple AppDomains to ensure the new ID generation logic produces unique and consistent cookies.
VMPTR_DomainAssembly vmDomainAssembly)
src/coreclr/debug/di/rsassembly.cpp:32
- By removing the null check on
vmDomainAssembly, we should verify thatvmAssemblyis always valid here; consider adding an assertion (_ASSERTE(!vmAssembly.IsNull())) or early guard to prevent passing a null pointer toVmPtrToCookie.
VmPtrToCookie(vmAssembly),
elinor-fung
added
area-Diagnostics-coreclr
and removed
area-AssemblyLoader-coreclr
labels
Contributor
|
Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag |
…otnet#117037)" (dotnet#117221) This reverts commit a3929e2.
elinor-fung
force-pushed
the
fix-cordbassembly-id
branch
from
6856cf5 to
89a04fa
Compare
elinor-fung
changed the title
hoyosjs
approved these changes
Jul 2, 2025
Member
|
I'll run some tests before merging |
Member
Author
|
|
Member
Author
|
/ba-g failure is #111922
Chatted offline that we should be good. |
MihaZupan
pushed a commit
to MihaZupan/runtime
that referenced
this pull request
Jul 4, 2025
…tnet#117224) Fix on top of the original: use Assembly as the ID for CordbAssembly instead of DomainAssembly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Original change: #117037
Fix on top of the original: 1002a9d. Use Assembly as the ID instead of DomainAssembly. See #117221