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

Move Internal.Console to shared CoreLib & port to Unix #42983

Merged
merged 6 commits into from
Oct 11, 2020

Conversation

MichalStrehovsky
Copy link
Member

Internal.Console is a helper class to print stuff out of CoreLib where System.Console is unavailable. Internal.Console was only available on CoreCLR/Windows, but I needed it for Linux, hence this pull request.

@ghost
Copy link

ghost commented Oct 2, 2020

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

@jkotas
Copy link
Member

jkotas commented Oct 2, 2020

You should be also able to delete GetStdHandle and WriteFile from src\coreclr\src\vm\ecalllist.h.

@ghost
Copy link

ghost commented Oct 2, 2020

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

@jkotas
Copy link
Member

jkotas commented Oct 2, 2020

Internal.Console was only available on CoreCLR/Windows

FWIW, it is available on CoreCLR/Unix too, but it depends on Win32 PAL. This PR makes it cleaner and simpler.

@jkotas
Copy link
Member

jkotas commented Oct 2, 2020

Note that this is not a public API. It is an internal aid for printf-style debugging used frequently by the runtime engineers to debug low-level problems in CoreLib and in the VM.

Copy link
Contributor

@CoffeeFlux CoffeeFlux left a comment

Choose a reason for hiding this comment

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

As part of this, you'll have to remove the existing minimal Mono implementation.

Also worth noting: this will not work on Android. I don't think that's a problem, and if we want it to work there later I can always add the necessary code to SystemNative_Log.

Lastly, thanks a bunch! Looks good to me from the Mono side, and having proper support for this should be a nice improvement 😊

@@ -104,7 +104,6 @@

<!-- Sources -->
<ItemGroup>
<Compile Include="$(BclSourcesRoot)\Internal\Console.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

Delete <Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetStdHandle.cs"> and friends in this file, and them to the Windows-specific section of the shared CoreLib project instead.

Comment on lines 9 to 12
void SystemNative_Log(uint8_t* buffer, int32_t length)
{
fwrite(buffer, 1, (size_t)length, stdout);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful to flush stdout here. Otherwise log messages might not show up on crashes etc.

@jkotas jkotas merged commit 58f28f6 into dotnet:master Oct 11, 2020
@MichalStrehovsky MichalStrehovsky deleted the commonConsole branch October 11, 2020 17:30
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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.

6 participants