Add support for user created buffers for raw profiler hooks by wiktork · Pull Request #451 · microsoft/CLRInstrumentationEngine

Wiktor Kopec (wiktork) · 2021-10-22T06:01:02Z

Adds support for 3 different scenarios in raw profiler hooks:

SetILFunctionBody is called right after DefineMethod during ModuleLoad.
SetILFunctionBody is called right after DefineMethod during JitCompilationStarted.
SetILFunctionBody is called with a custom buffer (one not allocated with our FunctionBodyAllocator)

TODO:

GetInstrumentationResults (test only) may result on having a non-zero buffer.

Del Myers (delmyers) · 2021-10-22T18:31:23Z

HRESULT MicrosoftInstrumentationEngine::CCorProfilerInfoWrapper::SetILFunctionBody(

Do we need to do something similar for SetILInstrumentedCodeMap? I.E. if there is no CMethodInfoInstance, and user buffers are enabled, then just directly set the code map?

Refers to: src/InstrumentationEngine/CorProfilerInfoWrapper.cpp:434 in c259b82. [](commit_id = c259b82, deletion_comment = False)

Del Myers (delmyers)

🕐

William Xie (willxie-eng) · 2021-10-25T17:32:11Z

Please remember to update the changelog.md file

In reply to: 951147810

Wiktor Kopec (wiktork) · 2021-10-26T17:31:33Z

Do we need to do something similar for SetILInstrumentedCodeMap?

I don't think we need to set these for brand new methods, since there's no mapping from old il to new il. But you are correct, we will fail this call since we won't find the existing method.

Del Myers (delmyers) · 2021-10-26T17:35:38Z

Yeah, I don't know what the CLR will do in this case. Technically, I don't think that you are limited to setting the IL for a method at module load only if it is a brand new method. I can imagine a world where someone reads the IL from disk, creates a new method, and sets the code map before the first jit... but that is probably not a common use case.

In reply to: 952157974

Wiktor Kopec (wiktork) · 2021-10-29T06:26:47Z

/azp run

azure-pipelines · 2021-10-29T06:27:03Z

Azure Pipelines successfully started running 1 pipeline(s).

Wiktor Kopec (wiktork) · 2021-11-03T18:15:19Z

/azp run

azure-pipelines · 2021-11-03T18:15:33Z

Azure Pipelines successfully started running 1 pipeline(s).

Wiktor Kopec (wiktork) · 2021-11-03T21:31:34Z


 PVOID MicrosoftInstrumentationEngine::CCorMethodMalloc::Alloc(_In_ ULONG cb)
 {
+    //TODO Any consecutive calls to Alloc will destroy the previous buffer.


Consider failing subsequent calls or leaking the buffer

It looks like this is the default code flow; we don't have independent cleanup for this buffer, we rely on subsequent AllocCalls or ref counting to cleanup. I will leave this as is for now.

Wiktor Kopec (wiktork) · 2021-11-03T21:54:08Z

+    if (moduleId == m_testModule)
+    {
+        if (m_firstJit)
+        {


Extra tests:

Double Alloc

Order between IM and Raw maintained

Order inversion with both modifying the method.

Make sure repeat calls to SetILFunctionBody behave as expected.

Wiktor Kopec (wiktork) · 2021-11-11T00:06:12Z

/azp run

azure-pipelines · 2021-11-11T00:06:29Z

Azure Pipelines successfully started running 1 pipeline(s).

William Xie (willxie-eng)

Wiktor Kopec (wiktork) · 2021-11-12T17:31:11Z

/azp run

azure-pipelines · 2021-11-12T17:31:26Z

Azure Pipelines successfully started running 1 pipeline(s).

Wiktor Kopec (wiktork) changed the title ~~Dev/wiktork/custom buffer4~~ Add support for user created buffers for raw profiler hooks Oct 22, 2021

Wiktor Kopec (wiktork) commented Oct 22, 2021

View reviewed changes

Comment thread src/InstrumentationEngine/MethodInfo.cpp

Wiktor Kopec (wiktork) commented Oct 22, 2021

View reviewed changes

Comment thread src/InstrumentationEngine/MethodInfo.cpp Outdated

Del Myers (delmyers) reviewed Oct 22, 2021

View reviewed changes

Comment thread src/InstrumentationEngine/MethodInfo.cpp

Del Myers (delmyers) reviewed Oct 22, 2021

View reviewed changes

Comment thread src/InstrumentationEngine/CorProfilerInfoWrapper.cpp Outdated

Del Myers (delmyers) suggested changes Oct 22, 2021

View reviewed changes

William Xie (willxie-eng) reviewed Oct 25, 2021

View reviewed changes

Comment thread src/InstrumentationEngine/CorProfilerInfoWrapper.cpp

Del Myers (delmyers) approved these changes Oct 25, 2021

View reviewed changes

Wiktor Kopec (wiktork) linked an issue Oct 29, 2021 that may be closed by this pull request

Raw profiler hook doesn't work with a profiler that adds methods #370

Open

Wiktor Kopec (wiktork) force-pushed the dev/wiktork/customBuffer4 branch from c259b82 to 69a533d Compare October 29, 2021 06:21

Wiktor Kopec (wiktork) commented Oct 29, 2021

View reviewed changes

Comment thread InstrumentationEngine.sln

Wiktor Kopec (wiktork) commented Nov 3, 2021

View reviewed changes

Comment thread src/InstrumentationEngine/MethodInfo.cpp Outdated

Wiktor Kopec (wiktork) marked this pull request as ready for review November 3, 2021 18:22

Wiktor Kopec (wiktork) requested a review from a team as a code owner November 3, 2021 18:22

Wiktor Kopec (wiktork) commented Nov 3, 2021

View reviewed changes

William Xie (willxie-eng) reviewed Nov 8, 2021

View reviewed changes

Comment thread src/InstrumentationEngine.Lib/InstrumentationEngineFeatures.h

Wiktor Kopec (wiktork) added 4 commits November 10, 2021 10:18

Add support for custom user buffers when using raw profiler hooks

56fb656

Unit tests + PR feedback

e562b4b

Fix up tests and configurations

3567a66

PR feedback + unnecessary code removal

ef32601

Wiktor Kopec (wiktork) force-pushed the dev/wiktork/customBuffer4 branch from 4185b6c to ef32601 Compare November 10, 2021 23:51

William Xie (willxie-eng) approved these changes Nov 11, 2021

View reviewed changes

William Xie (willxie-eng) reviewed Nov 11, 2021

View reviewed changes

Comment thread src/InstrumentationEngine/CorProfilerInfoWrapper.cpp

William Xie (willxie-eng) reviewed Nov 11, 2021

View reviewed changes

Comment thread tests/RawProfilerHook/RawProfilerHook/CoRawProfilerHook.cpp

Revert code analysis builds back to release

fa44d82

Wiktor Kopec (wiktork) merged commit 0d1cfa7 into microsoft:main Nov 16, 2021

Uh oh!

Conversation

Wiktor Kopec (wiktork) commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Del Myers (delmyers) commented Oct 22, 2021

Uh oh!

Uh oh!

Uh oh!

Del Myers (delmyers) left a comment

Choose a reason for hiding this comment

Uh oh!

William Xie (willxie-eng) commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Wiktor Kopec (wiktork) commented Oct 26, 2021

Uh oh!

Del Myers (delmyers) commented Oct 26, 2021

Uh oh!

Wiktor Kopec (wiktork) commented Oct 29, 2021

Uh oh!

azure-pipelines Bot commented Oct 29, 2021

Uh oh!

Uh oh!

Uh oh!

Wiktor Kopec (wiktork) commented Nov 3, 2021

Uh oh!

azure-pipelines Bot commented Nov 3, 2021

Uh oh!

Wiktor Kopec (wiktork) Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

Wiktor Kopec (wiktork) Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

Wiktor Kopec (wiktork) Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

Wiktor Kopec (wiktork) Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wiktor Kopec (wiktork) commented Nov 11, 2021

Uh oh!

azure-pipelines Bot commented Nov 11, 2021

Uh oh!

William Xie (willxie-eng) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Wiktor Kopec (wiktork) commented Nov 12, 2021

Uh oh!

azure-pipelines Bot commented Nov 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Wiktor Kopec (wiktork) commented Oct 22, 2021 •

edited

Loading

William Xie (willxie-eng) commented Oct 25, 2021 •

edited

Loading

Wiktor Kopec (wiktork) Nov 3, 2021 •

edited

Loading