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

ProcessInfo2 Diagnostics IPC Command #52258

Merged
17 commits merged into from
Jun 9, 2021

Conversation

josalem
Copy link
Contributor

@josalem josalem commented May 4, 2021

closes #51592
related dotnet/diagnostics#2198

Adds the ProcessInfo2 command to the Diagnostics IPC Protocol.

New ver of the command includes the entrypoint assembly path and product version string.

I'm opening this in draft mode for some additional test coverage while I hunt down how to retrieve the entrypoint assembly path in Mono (@lateralusX any ideas?).

CC @jander-msft

@josalem josalem added this to the 6.0.0 milestone May 4, 2021
@josalem josalem self-assigned this May 4, 2021
@ghost
Copy link

ghost commented May 4, 2021

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

Issue Details

closes #51592
related dotnet/diagnostics#2198

Adds the ProcessInfo2 command to the Diagnostics IPC Protocol.

New ver of the command includes the entrypoint assembly path and product version string.

I'm opening this in draft mode for some additional test coverage while I hunt down how to retrieve the entrypoint assembly path in Mono (@lateralusX any ideas?).

CC @jander-msft

Author: josalem
Assignees: josalem
Labels:

area-Diagnostics-coreclr

Milestone: 6.0.0

@runfoapp runfoapp bot mentioned this pull request May 5, 2021
src/coreclr/vm/ceeload.cpp Outdated Show resolved Hide resolved
@josalem josalem marked this pull request as ready for review May 7, 2021 23:13
@josalem josalem requested a review from marek-safar as a code owner May 7, 2021 23:13
src/coreclr/vm/ceeload.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/ceeload.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/ceeload.cpp Outdated Show resolved Hide resolved
src/tests/tracing/eventpipe/common/IpcUtils.cs Outdated Show resolved Hide resolved
@hoyosjs
Copy link
Member

hoyosjs commented May 11, 2021

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

John Salem added 11 commits May 26, 2021 14:51
 * TODO: get Mono's entrypoint asm
 * command includes everything the first one did + product ver
   and entrypoint asm path
* Add a test too
* fetch mono info from main assembly
* remove ref naming
* remove lazy method
* need to test still
* simplify access patterns
* change API to utf8
* updated test
add missing break statement from merge conflict fix
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM

@runfoapp runfoapp bot mentioned this pull request May 28, 2021
Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

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

Other than a few minor things it looks good to me

src/native/eventpipe/ds-process-protocol.c Show resolved Hide resolved
src/native/eventpipe/ds-process-protocol.c Show resolved Hide resolved
@josalem
Copy link
Contributor Author

josalem commented Jun 1, 2021

I don't believe the failures are related to these changes. @lateralusX can you take a look at the Mono changes?

@josalem
Copy link
Contributor Author

josalem commented Jun 2, 2021

@steveisok or @lambdageek can someone take a look at the Mono changes. I'm pretty confident that the wasm and android failures are unrelated to these changes, but not 100%.

src/mono/mono/eventpipe/ep-rt-mono.c Outdated Show resolved Hide resolved
src/mono/mono/eventpipe/ep-rt-mono.h Show resolved Hide resolved
src/mono/mono/eventpipe/ep-rt-mono.h Show resolved Hide resolved
@ghost
Copy link

ghost commented Jun 8, 2021

Hello @josalem!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 197cfb6 into dotnet:main Jun 9, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 9, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
.NET Core Diagnostics
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Allow getting entrypoint assembly via diagnostics IPC protocol
7 participants