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

Unify corerun's implementation of the native debugger check with the minipal for all platforms #109599

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

jkoritzinsky
Copy link
Member

Deduplicate the non-Windows path so new platforms only need to add support in one place.

Copy link
Contributor

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

#endif

bool minipal_can_check_for_native_debugger(void)
{
#if defined(MINIPAL_DEBUGGER_PRESENT_CHECK)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like an always-true condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s true for every platform we support, but if you build on an unsupported platform, we’ll hit the false case.

Copy link
Member

Choose a reason for hiding this comment

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

IMO, just delete minipal_can_check_for_native_debugger and add #else\n#error "Not yet implemented" on line 47 to let the person/team consciously make a determination during the porting time (whether they want to ignore this feature by adding #elif defined(__someOS__) /* ignored */ or implement it as part of the port).

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT Nov 7, 2024

Choose a reason for hiding this comment

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

@am11 I agree with these thoughts for most features, but the debugging scenario seems different. It doesn't need to exist for new platforms and not having a very basic "debugger supported check" means we then may need to add additional compile time checks in other places, which I think we'd like to avoid. The pattern here seems to be very simple, even if clunky, so new platform bring up can focus on "Hello, World" or just ignore debugging if desired.

Am I missing some mechanic of your proposal that would make this easier?

Copy link
Member

Choose a reason for hiding this comment

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

@AaronRobinsonMSFT, without the additional API: main...am11:runtime:dedup-debugger-check, it wouldn't block HelloWorld during the port to new platform. To get by, the person will add an empty elif branch likely with a TODO (as they would need to adjust tons of other places) and obviously not use -d or --debug options with corerun, as wait_for_debugger is only called in that case.

No big deal, but that build-time error is the more prevalent pattern used in coreclr (I remembered hitting them in past adventures 😅).

@jkoritzinsky
Copy link
Member Author

/ba-g debugger helix workitem deadletter (which we can't use known issues for today)

@jkoritzinsky jkoritzinsky merged commit ce6d835 into dotnet:main Nov 7, 2024
155 of 159 checks passed
@jkoritzinsky jkoritzinsky deleted the dedup-debugger-check branch November 7, 2024 19:52
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.

3 participants