Skip to content

Conversation

kg
Copy link
Member

@kg kg commented Sep 20, 2025

Thanks to @jkotas for noticing this

@Copilot Copilot AI review requested due to automatic review settings September 20, 2025 15:47
Copy link
Contributor

@Copilot Copilot AI left a 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 fixes inverted garbage collector (GC) contract conditions in functions that may create threads in the CoreCLR runtime. The conditions were incorrectly specifying GC triggers for when a thread exists and disabled GC for when no thread exists, when it should be the opposite.

  • Corrects the logic in CONTRACTL blocks across multiple CoreCLR VM files
  • Ensures proper GC contract specifications for thread creation and management functions
  • Affects functions in thread management, application domain operations, and host interface components

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/coreclr/vm/threadsuspend.cpp Fixed GC contracts in UserAbort and ResumeAllThreads functions
src/coreclr/vm/threads.cpp Corrected GC contracts in multiple thread lifecycle functions including constructors, setup, and cleanup
src/coreclr/vm/corhost.cpp Updated GC contracts in host interface methods for app domain and delegate operations
src/coreclr/vm/appdomain.cpp Fixed GC contracts in AppDomain friendly name operations

Copy link
Contributor

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

@kg
Copy link
Member Author

kg commented Sep 21, 2025

I'll follow up on your comments and examine each method carefully, thanks

@kg
Copy link
Member Author

kg commented Sep 22, 2025

Should CorHost2::ExecuteInAppDomain be GC_TRIGGERS since it invokes arbitrary code that might trigger GC? Isn't NOTHROW wrong for it too?

@jkotas
Copy link
Member

jkotas commented Sep 22, 2025

Should CorHost2::ExecuteInAppDomain be GC_TRIGGERS since it invokes arbitrary code that might trigger GC?

Yes, that sounds right.

Isn't NOTHROW wrong for it too?

It catches all exceptions and converts them to HRESULT so it is NOTHROW. (NOTHROW means that no exception escapes. It does not prevent the implementation from throwing and caching.)

@kg kg requested a review from jkotas September 22, 2025 20:43
@kg
Copy link
Member Author

kg commented Sep 22, 2025

Did a pass over everything and tried to update most of them so they aren't conditional. LMK if I got anything wrong.

@kg
Copy link
Member Author

kg commented Sep 22, 2025

Following up on the non-contractL comments (I missed them originally)

@kg
Copy link
Member Author

kg commented Sep 22, 2025

Think I got everything now. Sorry for the churn.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@kg kg merged commit 3feaf3a into dotnet:main Sep 23, 2025
98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants