Enable XplatEventLogger on all non-Windows platforms #3879

manu-st · 2016-03-23T07:09:37Z

Ensure that even if FEATURE_EVENTSOURCE_XPLAT is not defined it, the
implementation provides a dummy implementation. This is required because
mscorlib defines FEATURE_EVENTSOURCE_XPLAT even if this is not supported
by the underlying platform.

jkotas · 2016-03-23T11:38:26Z

@manu-silicon Could you please take a look at the OSX build break? Looks like the condition for XplatEventLogger.cs in mscorlib.shared.sources.props needs to be fixed up.

manu-st · 2016-03-23T13:42:52Z

@jkotas Just pushed a change to always include XplatEventLogger.cs.

jkotas · 2016-03-23T16:41:03Z

LGTM. Could you please squash into single commit?

manu-st · 2016-03-23T23:33:18Z

Squashed.

ramarag · 2016-03-24T01:15:08Z

Hey instead of this big change, you can just conditionally turn on FEATURE_EVENTSOURCE_XPLAT in the VM for non windows,
currently it is turned on only if FEATURE_EVENT_TRACE is specified, look at https://github.com/dotnet/coreclr/blob/master/CMakeLists.txt#L786

This would keep the feature self contained

ramarag · 2016-03-24T01:16:23Z

Correct me if i am wrong, this would not have raised if we had ways to trun off features for different flavors of linux in mscorlib

manu-st · 2016-03-24T01:38:59Z

The issue was that on Linux ARM mscorlib assumed that Xplat was available but the native part had been disabled due to poor support (see #2824). And indeed if we could have turned on/off some features in mscorlib, it would have matched the native part and there would not have been an issue.

ramarag · 2016-03-24T01:44:48Z

That is exactly the above mentioned solution would do, instead of matching mscorlib to native, we would be changing native to match mscorlib ,only for this instance
so
if(CLR_CMAKE_PLATFORM_UNIX)
add_definitions(-DFEATURE_EVENTSOURCE_XPLAT=1)
endif(CLR_CMAKE_PLATFORM_UNIX)

needs to be moved out of
if(FEATURE_EVENT_TRACE)

jkotas · 2016-03-24T01:45:00Z

Yes, that would work too - but it would required keeping the managed and unmanaged settings in sync somehow.

The typical approach taken in corefx is to build the managed code general purpose, and have the fine-grained differences in unmanaged C++ wrappers only. Here is one example from many: https://github.com/dotnet/corefx/blob/1db0c74f3f525365c2573458aa1f454cacf00453/src/Native/System.Security.Cryptography.Native/pal_ssl.cpp#L49.

This corefx approach make it easier to maintain different configurations. It is why I have suggested it here as well.

jkotas · 2016-03-24T01:50:45Z

I see - you are basically suggesting to use FEATURE_EVENTSOURCE_XPLAT instead of FEATURE_PAL to ifdef this, because of FEATURE_EVENTSOURCE_XPLAT is almost unused with this change. I do not have opinion what looks better. @manu-silicon What do you think?

ramarag · 2016-03-24T01:53:17Z

Ok, it makes sense from that point of view, although composition is lost.
@manu-silicon : Can you remove all occurrences of EATURE_EVENTSOURCE_XPLAT, as it no longer has any meaning

manu-st · 2016-03-24T02:06:25Z

@jkotas @ramarag Looks like I'm receiving mix-messages here and I'm not entirely following the reasoning of keeping either or of FEATURE_PAL or FEATURE_EVENTSOURCE_XPLAT. The former is to distinguish between Unix and Windows and the later for Unix only to see if something is available or not.

ramarag · 2016-03-24T02:11:24Z

The reason behind is FEATURE_EVENTSOURCE_XPLAT is to compose functionality for logging EventSource Events distinctly from ETW period. This could potentially be reused on windows too [ not gonna happen- but still saying]

FEATURE_PAL implies non windows to be exact

For the record - I would like to keep FEATURE_EVENTSOURCE_XPLAT, as it a nice way to turn of the feature. If the current change is going to be merged,please clean all usages of FEATURE_EVENTSOURCE_XPLAT, as it no longer makes any sense

jkotas · 2016-03-24T02:52:54Z

Ok. @manu-silicon Could you please make these changes:

Turn on FeatureXplatEventSource in clr.coreclr.props for all Unix (move it couple of lines down to the Unix block)
Turn on DFEATURE_EVENTSOURCE_XPLAT=1 in CMakeLists.txt for all Unix
Use FEATURE_EVENTSOURCE_XPLAT instead of FEATURE_PAL for ifdefing this

Sorry about the back and forth on this one...

jkotas · 2016-03-24T02:56:26Z

The ifdef for real implementation vs. the dummy implementation in eventtracepriv.h should be #ifdef FEATURE_EVENT_TRACE

manu-st · 2016-03-24T03:49:25Z

@jkotas Thanks for the clarification. I'll have a second pass at it tomorrow.

ramarag · 2016-03-24T05:34:55Z

by turning on FEATURE_EVENTSOURCE_XPLAT for all Linux, you do not need to have dummy implementation anywhere, this was the case when the feature got checked in, the current status is due to consolidation from this change dd9c20c in CMakeLists.txt [The piece I pointed above]

jkotas · 2016-03-24T05:41:06Z

I think you still need to have the dummy implementation even when FEATURE_EVENTSOURCE_XPLAT is turned on for all Unix. Lttng will be still available only when FEATURE_EVENT_TRACE is defined.

ramarag · 2016-03-24T05:46:19Z

Almost all of the functionality defined by FEATURE_EVENT_TRACE and not used by FEATURE_EVENTSOURCE_XPLAT should have dummy default implementations already if it is not defined.

manu-st · 2016-03-28T09:34:39Z

@jkotas @ramarag Here is my latest take on the matter. Compared to the original code, the only change is that now you can disable FEATURE_EVENTSOURCE_XPLAT when FEATURE_EVENT_TRACE is enabled, which was not possible before.

However, disabling FEATURE_EVENT_TRACE on Unix requires the set of changes I made originally if we do not want to change the way mscorlib is compiled. So I gave up doing that, which means that if someone disable it in CMakeLists.txt, it has to disable it in mscorlib too.

Let me know what you think.

jkotas · 2016-03-28T14:39:56Z

src/vm/eventtracepriv.h

@@ -408,13 +408,17 @@ class BulkStaticsLogger

 #endif // __EVENTTRACEPRIV_H__

-#if defined(FEATURE_EVENTSOURCE_XPLAT)
+#if defined(FEATURE_EVENT_TRACE)


I think this should stay FEATURE_EVENTSOURCE_XPLAT

jkotas · 2016-03-28T14:48:08Z

FEATURE_EVENTSOURCE_XPLAT should control only whether the two internal calls from mscorlib to VM exists.

Please let me know if there is anything that I am missing...

manu-st · 2016-04-18T07:48:40Z

Seems to work for me. @myungjoo @leemgs Could you try this to see if this addresses all issues?

myungjoo · 2016-04-18T08:05:28Z

@prajwal-aithal , Could you please test this one? (Me and @leemgs are away for a week and cannot test ARM devices)

jkotas · 2016-04-18T09:07:14Z

The OSX build is failing. The problem is that FEATURE_EVENTSOURCE_XPLAT has to be defined for Unix unconditionally in the root CMakefile.txt.

manu-st · 2016-04-18T10:25:59Z

You mean regardless of the value of FEATURE_EVENT_TRACE?

manu-st · 2016-04-18T12:49:37Z

Do you mean having something like that in the CMakeLists.txt

if(CLR_CMAKE_PLATFORM_LINUX AND CLR_CMAKE_PLATFORM_ARCH_AMD64)
  set(FEATURE_EVENT_TRACE 1)
endif()
if(CLR_CMAKE_PLATFORM_UNIX)
  add_definitions(-DFEATURE_EVENTSOURCE_XPLAT=1)
endif()

?

I just tried on my Mac and it works. I did not test if that would work for arm but will try tomorrow.

jkotas · 2016-04-18T13:35:27Z

Yes, exactly that.

manu-st · 2016-04-18T23:52:53Z

I pushed the changes. It works for me on Mac and Linux ARM. Did not test anything else.

ramarag · 2016-04-19T00:52:14Z

If it is not too much too ask can you build on top of ramarag@84a8d4d ?

jkotas · 2016-04-19T01:13:07Z

@ramarag I would like to get this done in steps. Make one small step to get it working - because of there are people waiting for this, and then work on better factoring. The implementation of the event tracing is very entangled and the history of this PR shows that it is hard to do bigger steps here (e.g. there are still build breaks).

ramarag · 2016-04-19T01:23:05Z

src/vm/eventtracepriv.h

@@ -412,9 +412,13 @@ class BulkStaticsLogger
 class XplatEventSourceLogger
 {
 public:
+#ifdef FEATURE_EVENT_TRACE


This conditional compilation is done for FireEtw* and XplatEventLogger::IsEventLoggingEnabled under the hoods. If it is not pressing to check in the change as it is please do look to build on top of ramarag@84a8d4d

Do you mean compiling just your commit? or Mine and yours combined?

With your commit, it seems to work.

yes i meant add your changes on top of ramarag/coreclr@84a8d4d

jkotas · 2016-04-19T02:01:05Z

@manu-silicon The global CMakefile is sensitive to order in which things are defined. You have moved them earlier and it broke the build. Could you please keep it where it was, and just tweak the ifdef?

manu-st · 2016-04-19T02:12:16Z

@jkotas I wanted to put them together. I've pushed the change

jkotas · 2016-04-19T04:06:35Z

@dotnet-bot test Ubuntu x64 Checked Build and Test please

jkotas · 2016-04-19T05:31:16Z

@manu-silicon I am sorry - I have just merged a number of other PRs while the CI was running on this one, and it has conflict now. Could you please resolve it?

jkotas · 2016-04-19T05:37:58Z

@ramarag I have created issue #4403 about the cleanup that you are suggesting.

manu-st · 2016-04-19T05:45:48Z

@jkotas Just pushed the changed.

Because on non-Windows platform, we assume event tracing, we also requires it to compile native code for non-Windows platform. Event tracing is only enabled on AMD64 devices for now on Unix platforms.

jkotas · 2016-04-20T03:22:10Z

@dotnet-bot test Ubuntu x64 Checked Build and Test please

jkotas · 2016-04-20T05:02:49Z

@manu-silicon Thank you for all work you have put into this one!

manu-st · 2016-04-20T05:07:21Z

@jkotas Not much work, but it was definitely time consuming. Glad we can now on.
@ramarag I can try to test your code, but could you update your branch to the latest master to make the process easier on me. Thanks!

ramarag · 2016-04-20T23:36:11Z

@manu-silicon : i have updated the changes to ramarag@0ec237e

Enable XplatEventLogger on all non-Windows platforms Commit migrated from dotnet/coreclr@0eff733

dnfclas added the cla-already-signed label Mar 23, 2016

manu-st changed the title ~~OXplat~~ Enable XplatEventLogger on all non-Windows platforms Mar 23, 2016

manu-st force-pushed the xplat branch from 6c60e59 to 5296a59 Compare March 23, 2016 23:33

manu-st force-pushed the xplat branch 2 times, most recently from 74aa690 to 89d31c6 Compare March 28, 2016 09:29

jkotas reviewed Mar 28, 2016
View reviewed changes

manu-st force-pushed the xplat branch from bf5691f to c2c077f Compare April 18, 2016 23:52

ramarag reviewed Apr 19, 2016
View reviewed changes

manu-st force-pushed the xplat branch from c2c077f to fa2c684 Compare April 19, 2016 02:12

manu-st force-pushed the xplat branch from fa2c684 to 05de9a9 Compare April 19, 2016 05:45

Enable XplatEventLogger on all non-Windows platforms

5c6abe0

Because on non-Windows platform, we assume event tracing, we also requires it to compile native code for non-Windows platform. Event tracing is only enabled on AMD64 devices for now on Unix platforms.

manu-st force-pushed the xplat branch from 05de9a9 to 5c6abe0 Compare April 19, 2016 05:47

jkotas merged commit 0eff733 into dotnet:master Apr 20, 2016

hseok-oh mentioned this pull request Dec 8, 2016

First step to create nuget packages for ARM32/Linux #8421

Merged

manu-st mentioned this pull request Jan 30, 2020

ARM 32-bit progress dotnet/runtime#5460

Closed

jkotas mentioned this pull request Jan 30, 2020

FEATURE_EVENTSOURCE_XPLAT cleanup dotnet/runtime#5642

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#3879 from manu-silicon/xplat

acf40bf

Enable XplatEventLogger on all non-Windows platforms Commit migrated from dotnet/coreclr@0eff733

Enable XplatEventLogger on all non-Windows platforms #3879

Enable XplatEventLogger on all non-Windows platforms #3879

Conversation

manu-st commented Mar 23, 2016

jkotas commented Mar 23, 2016

manu-st commented Mar 23, 2016

jkotas commented Mar 23, 2016

manu-st commented Mar 23, 2016

ramarag commented Mar 24, 2016

ramarag commented Mar 24, 2016

manu-st commented Mar 24, 2016

ramarag commented Mar 24, 2016

jkotas commented Mar 24, 2016

jkotas commented Mar 24, 2016

ramarag commented Mar 24, 2016

manu-st commented Mar 24, 2016

ramarag commented Mar 24, 2016

jkotas commented Mar 24, 2016

jkotas commented Mar 24, 2016

manu-st commented Mar 24, 2016

ramarag commented Mar 24, 2016

jkotas commented Mar 24, 2016

ramarag commented Mar 24, 2016

manu-st commented Mar 28, 2016

jkotas Mar 28, 2016

Choose a reason for hiding this comment

jkotas commented Mar 28, 2016

manu-st commented Apr 18, 2016

myungjoo commented Apr 18, 2016

jkotas commented Apr 18, 2016

manu-st commented Apr 18, 2016 via email

manu-st commented Apr 18, 2016

jkotas commented Apr 18, 2016

manu-st commented Apr 18, 2016

ramarag commented Apr 19, 2016

jkotas commented Apr 19, 2016

ramarag Apr 19, 2016

Choose a reason for hiding this comment

manu-st Apr 19, 2016

Choose a reason for hiding this comment

manu-st Apr 19, 2016

Choose a reason for hiding this comment

ramarag Apr 19, 2016

Choose a reason for hiding this comment

jkotas commented Apr 19, 2016

manu-st commented Apr 19, 2016

jkotas commented Apr 19, 2016

jkotas commented Apr 19, 2016

jkotas commented Apr 19, 2016

manu-st commented Apr 19, 2016

jkotas commented Apr 20, 2016

jkotas commented Apr 20, 2016

manu-st commented Apr 20, 2016

ramarag commented Apr 20, 2016