Retry: Enable EventPipe across Unix and Windows #15611

nategraf · 2017-12-21T20:42:36Z

This PR Enables the native components of EventPipe and SampleProfiler to be used on Unix and Windows (expanded from only Linux).

The two largest components are the creation of native interfaces to ETW which match those used to LTTng, and the fixing of contracts within EventPipe code. Additionally bug fixes, build changes, and modification to the SampleProfiler were done to address errors and enable windows builds

Follow-up #14772
Resolves #15607

…" (dotnet#15609)" This reverts commit 302005c.

brianrob

LTGM modulo addressing the build and test issues that were found on x64 Pri1 and ARM.

jkotas · 2017-12-21T20:49:47Z

@dotnet test Windows_NT arm Cross Checked Innerloop Build and Test please
@dotnet test Windows_NT arm64 Cross Checked Innerloop Build and Test please
@dotnet test Windows_NT armlb Cross Checked Innerloop Build and Test please

jkotas · 2017-12-21T21:23:58Z

arm build failing with:

13:09:58 CMake Error at src/vm/CMakeLists.txt:523 (add_subdirectory):
13:09:58   add_subdirectory given source
13:09:58   "D:/j/workspace/arm_cross_che---9fe37e18/bin/obj/Windows_NT.arm.Checked/eventing/eventpipe"
13:09:58   which is not an existing directory.

benaadams · 2017-12-21T22:38:47Z

Revert fixed the ETW failures https://github.com/dotnet/coreclr/issues/15607

@dotnet-bot test Windows_NT x64 Checked corefx_baseline

nategraf · 2017-12-21T23:51:45Z

@dotnet-bot test Windows_NT pri1 please

benaadams · 2017-12-22T00:25:52Z

https://github.com/dotnet/coreclr/issues/15607 occurs again

Test Result (29 failures / +29)
--
System.Net.Mail.Tests.LoggingTest.EventSource_EventsRaisedAsExpected
System.Threading.Tasks.Tests.EtwTests.TestEtw
System.Diagnostics.Tests.DiagnosticSourceEventSourceBridgeTests.LinuxNewLineConventions
BasicEventSourceTests.FuzzyTests.Test_Write_Fuzzy
BasicEventSourceTests.TestEventCounter.Test_Write_Metric_EventListener
BasicEventSourceTests.TestsManifestGeneration.Test_EventSource_NamedEventSource
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_ByteArray_Manifest_EventListener
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_ComplexData_SelfDescribing_EventListener
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_Manifest_EventListener_UseEvents
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_SelfDescribing_EventListener
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_ByteArray_Manifest_EventListener_UseEvents
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_ByteArray_SelfDescribing_EventListener
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_SelfDescribing_EventListener_UseEvents
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_NoAttribute
BasicEventSourceTests.TestsWriteEvent.Test_WriteEvent_Manifest_EventListener
BasicEventSourceTests.TestsWriteEventToListener.Test_WriteEvent_InvalidCalls
BasicEventSourceTests.TestsWriteEventToListener.Test_WriteEvent_ArgsCornerCases
BasicEventSourceTests.TestsWriteEventToListener.Test_WriteEvent_ArgsBasicTypes
BasicEventSourceTests.TestsWrite.Test_Write_T_In_Manifest_Serialization
BasicEventSourceTests.TestsWrite.Test_Write_T_EventListener
BasicEventSourceTests.TestsWrite.Test_Write_T_EventListener_UseEvents
System.Buffers.ArrayPool.Tests.ArrayPoolUnitTests.ReturnBufferFiresDiagnosticEvent
System.Threading.Tests.EtwTests.TestEtw
System.Net.Security.Tests.LoggingTest.EventSource_EventsRaisedAsExpected
System.Data.Tests.DataCommonEventSourceTest.InvokeCodeThatShouldFirEvents_EnsureEventsFired
System.Linq.Parallel.Tests.EtwTests.TestEtw
System.Collections.Concurrent.Tests.EtwTests.TestEtw
System.Net.Primitives.Functional.Tests.LoggingTest.EventSource_EventsRaisedAsExpected
System.Threading.Tasks.Dataflow.Tests.EtwTests.TestEtw

nategraf · 2017-12-22T00:32:26Z

Thanks @benaadams I'm working on it

benaadams · 2017-12-22T00:38:22Z

FYI there's also https://github.com/dotnet/coreclr/issues/15537 in the errors for the run which is definitely unrelated

benaadams · 2017-12-27T07:53:50Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline

…eclr into universal-eventpipe

…rsal-eventpipe

nategraf · 2017-12-28T02:14:31Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline

nategraf · 2017-12-28T15:59:18Z

The change originally causing #15607, is that I enabled FeaturePerfTracing in the managed areas, assuming it would be a purely additive and compatible change. I have since figured out that certain places within managed code treat the use of EventPipe and EventListener logging as mutually exclusive, and will require work to make both usable at once.

In order to avoid expanding this PR further, I am opening https://github.com/dotnet/coreclr/issues/15656 and disabling tests which require use of the EventPipe's managed surface.

nategraf · 2017-12-28T16:01:02Z

@jkotas could you please kick off the ARM builds?

jkotas · 2017-12-28T16:19:43Z

@dotnet test Windows_NT arm Cross Checked Innerloop Build and Test please
@dotnet test Windows_NT arm64 Cross Checked Innerloop Build and Test please
@dotnet test Windows_NT armlb Cross Checked Innerloop Build and Test please

jkotas · 2017-12-28T16:20:38Z

@dotnet-bot test Ubuntu arm Cross Debug Innerloop Build please

nategraf · 2017-12-28T18:40:55Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline

nategraf · 2017-12-28T23:39:00Z

@BruceForstall @jkotas @benaadams all the concerns (Failures in ARM builds, failures in pri1 Windows builds, failed corefx tests, and erroneously introduced python2.7 dependency) of this PR's first iteration have been addressed. With your signoff I'll merge this PR.

jkotas · 2017-12-29T00:00:18Z

src/mscorlib/src/System/Diagnostics/Eventing/EventPipe.cs

@@ -15,12 +15,12 @@ internal struct EventPipeProviderConfiguration
        [MarshalAs(UnmanagedType.LPWStr)]
        private string m_providerName;
        private UInt64 m_keywords;
-        private uint m_loggingLevel;
+        private UInt32 m_loggingLevel;


Nit: Our convention is to use language keywords instead of BCL types (i.e. int, string, float instead of Int32, String, Single, etc). https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md . So this should have been rather changed other way around.

This was changed to fix a marshalling error on x86. Is there perhaps another solution to the marshalling error?

I do not see how changing uint to UInt32 can fix marshalling error in this file. uint and UInt32 are different names for the same thing in C#. It is just a matter of style convention which one to use.

Same with long and Int64. These are also different names for the same thing.

Here is the commit which fixed the marshalling error. If those are irrelevant, what fixed it?
Just the changes in the C++ side?
97d5c0b

Just the changes in the C++ side?

I think so.

Alright. I'll try reverting the changes to the C# code and test this again

jkotas · 2017-12-29T00:01:35Z

src/scripts/utilities.py

@@ -0,0 +1,108 @@
+from filecmp import dircmp


License header?

Added now. Thanks

benaadams · 2017-12-29T00:06:17Z

Add to PR summary

Resolves https://github.com/dotnet/coreclr/issues/15607

Then should auto close on merge?

BruceForstall · 2017-12-29T00:18:39Z

@dotnet-bot test Windows_NT arm Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm Cross Checked jitstress1 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked jitstress2 Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress1 Build and Test
@dotnet-bot test Windows_NT arm64 Cross Checked jitstress2 Build and Test
@dotnet-bot test Windows_NT x64 Checked jitstress1
@dotnet-bot test Windows_NT x64 Checked jitstress2
@dotnet-bot test Windows_NT x86 Checked jitstress1
@dotnet-bot test Windows_NT x86 Checked jitstress2
@dotnet-bot test Windows_NT x86 Checked corefx_baseline
@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Windows_NT x86_arm_altjit Checked Build and Test
@dotnet-bot test Windows_NT x86_arm_altjit Checked jitstress1
@dotnet-bot test Windows_NT armlb Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT armlb Cross Checked jitstress1 Build and Test
@dotnet-bot test Windows_NT armlb Cross Checked jitstress2 Build and Test

BruceForstall · 2017-12-29T00:22:50Z

@nategraf If you can, you should avoid pushing any changes until the jobs I triggered finished, otherwise we'll lose their results and they'll need to get re-triggered.

nategraf · 2017-12-29T15:47:35Z

If you click on the ❌ or ✔ next to commit hash in between comments you can see any old results

nategraf · 2017-12-29T15:54:05Z

Jitstress2

20:20:25 Pass: 9436 Fail: 0 Complete: 84% Start:20:20:24 Test:[_relstress3.cmd_9615] JIT\Methodical\refany\_relstress3\_relstress3.cmd
20:20:25 Pass: 9436 Fail: 1 Complete: 84% Start:20:20:24 Test:[_dbglcs_gcref.cmd_9606] JIT\Methodical\VT\port\_dbglcs_gcref\_dbglcs_gcref.cmd

Jitstress1

19:34:51 Pass: 4106 Fail: 0 Complete: 36% Start:19:34:51 Test:[fieldExprUnchecked1.cmd_4187] JIT\jit64\opt\cse\fieldExprUnchecked1\fieldExprUnchecked1.cmd
19:34:51 Pass: 4106 Fail: 1 Complete: 36% Start:19:34:51 Test:[_rellcs_gcref.cmd_4177] JIT\Methodical\VT\port\_rellcs_gcref\_rellcs_gcref.cmd
...
19:48:46 Pass: 9434 Fail: 1 Complete: 84% Start:19:48:47 Test:[_dbglcs.cmd_9614] JIT\Methodical\Arrays\lcs\_dbglcs\_dbglcs.cmd
19:48:46 Pass: 9434 Fail: 2 Complete: 84% Start:19:48:47 Test:[_dbglcs_gcref.cmd_9606] JIT\Methodical\VT\port\_dbglcs_gcref\_dbglcs_gcref.cmd

Do you think these failures could be related to these changes, or might they be unrelated?

BruceForstall · 2017-12-29T18:29:18Z

@nategraf Looks like those armlb failures have been occurring for a while. You can ignore them.

BruceForstall

Some questions about the build.cmd changes

BruceForstall · 2017-12-29T18:41:54Z

build.cmd

+set __IntermediatesIncDir=%__IntermediatesDir%\src\inc
+set __IntermediatesEventingDir=%__IntermediatesDir%\eventing
+
+echo Laying out dynamically generated files consumed by the build system


I appreciate you have these "echo" status messages. Can you please make sure they all start with echo %__MsgPrefix%, so we know where the message came from?

Fixed in %__MsgPrefix%

BruceForstall · 2017-12-29T18:43:39Z

build.cmd

+%PYTHON% -B -Wall  %__SourceDir%\scripts\genEventing.py --inc %__IntermediatesIncDir% --dummy %__IntermediatesIncDir%\etmdummy.h --man %__SourceDir%\vm\ClrEtwAll.man --nonextern || exit /b 1
+
+echo Laying out dynamically generated EventPipe Implementation
+%PYTHON% -B -Wall %__SourceDir%\scripts\genEventPipe.py --man %__SourceDir%\vm\ClrEtwAll.man --intermediate %__IntermediatesEventingDir%\eventpipe --nonextern || exit /b 1


If one of the python scripts fails, will it output an appropriate error message, so we know what failed? Because you're going to immediate exit the build script with no additional error message.

Yes. It lets python print out the stack trace like normal

BruceForstall · 2017-12-29T18:46:08Z

build.cmd

@@ -380,6 +390,45 @@ for /f "tokens=*" %%s in ('%DotNetCli% msbuild "%OptDataProjectFilePath%" /t:Dum
    set __IbcOptDataVersion=%%s
 )

+REM =========================================================================================
+REM ===
+REM === Generate source files for eventing


This phase looks unconditional, whether we're building native, corelib, etc. Is that appropriate?

I'll take another look at what types of builds actually need these files

I figured out better conditioning. Thanks for catching this

BruceForstall · 2017-12-29T18:47:24Z

build.cmd

@@ -380,6 +390,45 @@ for /f "tokens=*" %%s in ('%DotNetCli% msbuild "%OptDataProjectFilePath%" /t:Dum
    set __IbcOptDataVersion=%%s
 )

+REM =========================================================================================
+REM ===
+REM === Generate source files for eventing


How does this affect incremental build? If I run "build.cmd" twice in a row, will it generate new files that will cause the subsequent builds to build things they shouldn't need to?

No. I created a utility which ensures files are only written if the copy on disk is different

BruceForstall

The build.cmd changes look good to me. I don't have any comments on the rest of it.

nategraf · 2017-12-29T23:32:29Z

@dotnet-bot test Ubuntu arm Cross Debug Innerloop Build please

ghost · 2018-01-03T17:00:46Z

This commit is breaking the build for people who keep their pythons in directories named "C:\Program Files". The pathname needs to be quoted.

k15tfu · 2020-01-04T14:23:08Z

btw, seems that it also fixed ICorProfilerInfo::ForceGC on macOS because it always returned E_FAIL w/o FEATURE_EVENT_TRACE: https://github.com/dotnet/coreclr/blob/v1.1.13/src/vm/proftoeeinterfaceimpl.cpp#L4825

Revert "Revert "Enable EventPipe across Unix and Windows (dotnet#14772)…

c602a37

…" (dotnet#15609)" This reverts commit 302005c.

nategraf requested a review from brianrob December 21, 2017 20:42

brianrob approved these changes Dec 21, 2017

View reviewed changes

Fix ARM build break

f6bceea

Victor Nate Graf added 2 commits December 22, 2017 16:51

Use more explicit references to resolve build failures

a7e6820

Fix compat with python3

54a362c

Victor Nate Graf added 3 commits December 27, 2017 18:11

Disable FeaturePerfTracing on Windows as it is not ready

d1b3052

Merge branch 'universal-eventpipe' of https://github.com/nategraf/cor…

87f7810

…eclr into universal-eventpipe

Merge branch 'master' of https://github.com/dotnet/coreclr into unive…

7bb40eb

…rsal-eventpipe

Disable test for incomplete functionality

fd97a3e

Fix test diabled patterns

24efd34

jkotas reviewed Dec 29, 2017

View reviewed changes

Add license header

663cae2

Use keyword types for managed code

dec360d

BruceForstall reviewed Dec 29, 2017

View reviewed changes

Victor Nate Graf added 3 commits December 29, 2017 11:00

Add message prefix

d094b13

More precisly condition generation of eventing sources

a00d866

Remove erroneously added changes

0a37839

BruceForstall approved these changes Dec 29, 2017

View reviewed changes

nategraf merged commit c1bbdae into dotnet:master Jan 2, 2018

tannergooding mentioned this pull request Jan 3, 2018

Updating the build script to work when python is installed to a path containing a space. #15707

Merged

nategraf mentioned this pull request Jan 10, 2018

Fix incremental build by preventing nonchanging writes #15817

Merged

brianrob mentioned this pull request Mar 6, 2018

Fix Automatic Loading of Tracepoint Provider #16771

Merged

echesakov mentioned this pull request Mar 9, 2018

Fix etmdummy.h file not found during Linux/arm crosscomponent build #16858

Merged

omajid mentioned this pull request Aug 9, 2018

Build with python3 on unix #19356

Merged

Retry: Enable EventPipe across Unix and Windows #15611

Retry: Enable EventPipe across Unix and Windows #15611

Conversation

nategraf commented Dec 21, 2017 • edited

brianrob left a comment

Choose a reason for hiding this comment

jkotas commented Dec 21, 2017

jkotas commented Dec 21, 2017

benaadams commented Dec 21, 2017

nategraf commented Dec 21, 2017

benaadams commented Dec 22, 2017

nategraf commented Dec 22, 2017

benaadams commented Dec 22, 2017

benaadams commented Dec 27, 2017

nategraf commented Dec 28, 2017

nategraf commented Dec 28, 2017

nategraf commented Dec 28, 2017

jkotas commented Dec 28, 2017

jkotas commented Dec 28, 2017

nategraf commented Dec 28, 2017

nategraf commented Dec 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Dec 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Dec 29, 2017 • edited

BruceForstall commented Dec 29, 2017

BruceForstall commented Dec 29, 2017

nategraf commented Dec 29, 2017

nategraf commented Dec 29, 2017

BruceForstall commented Dec 29, 2017

BruceForstall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

nategraf commented Dec 29, 2017

ghost commented Jan 3, 2018

k15tfu commented Jan 4, 2020

nategraf commented Dec 21, 2017 •

edited

jkotas Dec 29, 2017 •

edited

benaadams commented Dec 29, 2017 •

edited