Skip to content

[mono] Move basic w32file* and w32process* functions to eventpipe - #66731

Merged
akoeplinger merged 4 commits into
dotnet:mainfrom
akoeplinger:w32-cleanup
Mar 18, 2022
Merged

[mono] Move basic w32file* and w32process* functions to eventpipe#66731
akoeplinger merged 4 commits into
dotnet:mainfrom
akoeplinger:w32-cleanup

Conversation

@akoeplinger

@akoeplingerakoeplinger commented Mar 16, 2022

Copy link
Copy Markdown
Member

They are not used by the rest of the runtime and we only need a bare minimum of w32process and w32file in eventpipe (e.g. no file share/access logic).

Fix ep-test build and warnings, it was missing linking against libz and monoapi.

Fix host vs. target confusion in ep-rt-mono.c

@ghostghost added the area-VM-meta-mono label Mar 16, 2022
@akoeplinger
[mono] Move basic w32file* and w32process* functions to eventpipe
e82c20a
They are not used by the rest of the runtime and we only need a bare minimum of w32process and w32file in eventpipe (e.g. no file share/access logic).
Fix ep-test build and warnings, it was missing linking against libz and monoapi.
Fix host vs. target confusion in ep-rt-mono.c
lambdageek
lambdageek reviewed Mar 17, 2022
View reviewed changes
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.c

/* clock_gettime () is found by configure on Apple builds, but its only present from ios 10, macos 10.12, tvos 10 and watchos 3 */
#if defined (HAVE_CLOCK_MONOTONIC) && (defined(TARGET_IOS) || defined(TARGET_OSX) || defined(TARGET_WATCHOS) || defined(TARGET_TVOS))
#if defined (HAVE_CLOCK_MONOTONIC) && (defined(HOST_IOS) || defined(HOST_OSX) || defined(HOST_WATCHOS) || defined(HOST_TVOS))

@lambdageeklambdageekMar 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isn't HOST_DARWIN supposed to encompass all of these?

@lambdageeklambdageekMar 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also: ios 10, macos 10.12, etc are pretty old. Do we still need this?

@am11am11Mar 17, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minimum supported macOS version is 10.15 in .NET 6, per https://github.com/dotnet/core/blob/main/release-notes/6.0/supported-os.md#macos.

@akoeplingerakoeplingerMar 17, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

yes HOST_DARWIN would work too, good point. I can change that in a separate cleanup since we have similar code elsewhere.

Regarding removing this altogether, we have #58737 which tracks removing it from Android/iOS but last time we tried we hit various issues (#62978) so it needs a bit more care.

lateralusX
lateralusX reviewed Mar 17, 2022
View reviewed changes

@lateralusXlateralusX left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall I think it looks great, nice work!. Most comments are formatting and some around adapting ported code to EventPipe types where possible.

Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.c Outdated
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.c Outdated
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.c Outdated
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.c Outdated
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.c Outdated
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.c Outdated
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.h Outdated
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.h Outdated
Comment threadsrc/mono/mono/eventpipe/test/ep-buffer-tests.c Outdated
Comment threadsrc/mono/mono/eventpipe/ep-rt-mono.c Outdated
akoeplingerand others added 3 commits March 17, 2022 21:56
@akoeplinger@lateralusX
Formatting suggestions from code review
2afdde5
Co-authored-by: Johan Lorensson <lateralusx.github@gmail.com>
@akoeplinger
More code review feedback
167273e
Fix more formatting.
Move initialization of num_main_args after setting main_args so we can use it as a cheap guard value (not totally thread-safe but good enough for our purposes)
@akoeplinger
Addressing feedback
a11a791
Move cmdline formatting back into object.c
Fix bug return INVALID_HANDLE_VALUE instead of null in ep_rt_file_open_write
lateralusX
lateralusX approved these changes Mar 18, 2022
View reviewed changes

@lateralusXlateralusX left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@runfoapprunfoappBot mentioned this pull request Mar 18, 2022
Closed
@akoeplinger
akoeplinger merged commit 2637241 into dotnet:mainMar 18, 2022
@akoeplinger
akoeplinger deleted the w32-cleanup branch March 18, 2022 15:36
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
@akoeplinger@lateralusX@radekdoulik
[mono] Move basic w32file* and w32process* functions to eventpipe (do…
25ba70d
…tnet#66731)
They are not used by the rest of the runtime and we only need a bare minimum of w32process and w32file in eventpipe (e.g. no file share/access logic).
Fix ep-test build and warnings, it was missing linking against libz and monoapi.
Fix host vs. target confusion in ep-rt-mono.c
Move initialization of num_main_args after setting main_args in object.c so we can use it as a cheap guard value (not totally thread-safe but good enough for our purposes).
Co-authored-by: Johan Lorensson <lateralusx.github@gmail.com>
@ghostghost locked as resolved and limited conversation to collaborators Apr 17, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lambdageeklambdageeklambdageek left review comments
@lateralusXlateralusXlateralusX approved these changes
@vargazvargazAwaiting requested review from vargaz
@SamMonoRTSamMonoRTAwaiting requested review from SamMonoRT
@BrzVladBrzVladAwaiting requested review from BrzVladBrzVlad is a code owner
@nariccnariccAwaiting requested review from naricc
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner
@marek-safarmarek-safarAwaiting requested review from marek-safar
+1 more reviewer
@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@akoeplingerakoeplinger

Labels

area-VM-meta-mono

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@akoeplinger@lambdageek@am11@lateralusX