Skip to content

EventPipe support for ProcessInfo - #87562

Merged
LakshanF merged 10 commits into
dotnet:mainfrom
LakshanF:ProcessInfo2
Jul 14, 2023
Merged

EventPipe support for ProcessInfo#87562
LakshanF merged 10 commits into
dotnet:mainfrom
LakshanF:ProcessInfo2

Conversation

@LakshanF

@LakshanFLakshanF commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

Missing process information - work by @elinor-fung to support commandLine and added managedEntrypointAssemblyName. Enabled the existing diagnostic tests related to this.

@LakshanF
LakshanF requested review from elinor-fung and removed request for MichalStrehovskyJune 14, 2023 16:48
@LakshanFLakshanF self-assigned this Jun 14, 2023
@ghost

ghost commented Jun 14, 2023

Copy link
Copy Markdown

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

Issue Details

Missing process information - work by @elinor-fung to support commandLine and added managedEntrypointAssemblyName. Enabled the existing diagnostic tests related to this.

Author:LakshanF
Assignees:-
Labels:

area-Diagnostics-coreclr

Milestone:-

@ghost

ghost commented Jun 14, 2023

Copy link
Copy Markdown

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

Issue Details

Missing process information - work by @elinor-fung to support commandLine and added managedEntrypointAssemblyName. Enabled the existing diagnostic tests related to this.

Author:LakshanF
Assignees:LakshanF
Labels:

area-Diagnostics-coreclr, area-NativeAOT-coreclr

Milestone:-

@LakshanFLakshanF added this to the 8.0.0 milestone Jun 14, 2023
@LakshanF
LakshanF marked this pull request as draft June 14, 2023 16:48
@build-analysisbuild-analysisBot mentioned this pull request Jun 14, 2023
Closed
@runfoapprunfoappBot mentioned this pull request Jun 14, 2023
Closed
jkotas
jkotas reviewed Jun 14, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
MichalStrehovsky
MichalStrehovsky reviewed Jun 15, 2023
View reviewed changes
Comment threadsrc/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs
Comment threadsrc/tests/tracing/eventpipe/processinfo/processinfo.cs Outdated
Comment threadsrc/tests/tracing/eventpipe/processinfo2/processinfo2.csproj Outdated
@LakshanF
LakshanF marked this pull request as ready for review June 15, 2023 20:33
elinor-fung
elinor-fung reviewed Jul 6, 2023
View reviewed changes
Comment threadsrc/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
if (PalInterlockedCompareExchangePointer((void**)(&entrypoint_assembly_name), (void*)(entrypoint_assembly_name_local), nullptr) != nullptr)
delete[] entrypoint_assembly_name_local;
}
return reinterpret_cast<const char*>(entrypoint_assembly_name);

@elinor-fungelinor-fungJul 6, 2023

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.

Is always returning the process name (minus extension) what we want? So if someone renames the executable or if someone uses a nativeaot library, they would get the name of the process which would not match the assembly.

@LakshanFLakshanFJul 7, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think for the first iteration, using the process name without extension is fine. We can enhance if user feedback requests this.

@ericstjericstj mentioned this pull request Jul 6, 2023
Closed
This was referenced Jul 7, 2023
Open
Closed
Closed
elinor-fung
elinor-fung reviewed Jul 10, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
jkotas
jkotas reviewed Jul 11, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
This was referenced Jul 12, 2023
Closed
Closed
Closed
Closed
Closed
Closed
elinor-fung
elinor-fung approved these changes Jul 13, 2023
View reviewed changes
Comment threadsrc/tests/tracing/eventpipe/diagnosticport/diagnosticport.cs
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
Comment on lines +113 to +121
const wchar_t* process_name_const = wcsrchr(wszModuleFileName, DIRECTORY_SEPARATOR_CHAR);
if (process_name_const != NULL) {
process_name_const++;
}
size_t len = -1;
const wchar_t* extension = wcsrchr(process_name_const, '.');
if (extension != NULL) {
len = extension - process_name_const;
}

@elinor-fungelinor-fungJul 13, 2023

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.

You could define something like _tcslen / _tcsrchr based on the platform and then share this logic for finding the start of the name and its length.

@LakshanFLakshanFJul 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Added this to the tracking issue, #87069

Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp Outdated
Comment threadsrc/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp
This was referenced Jul 14, 2023
Closed
Closed
@LakshanF
LakshanF merged commit 9cf89bf into dotnet:mainJul 14, 2023
@ghostghost locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas left review comments
@MichalStrehovskyMichalStrehovskyMichalStrehovsky left review comments
@elinor-fungelinor-fungelinor-fung approved these changes

Assignees

@LakshanFLakshanF

Labels

area-NativeAOT-coreclr

Projects

AppModel
Status: Done

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@LakshanF@jkotas@MichalStrehovsky@elinor-fung