Enablelttng #1598

ramarag · Sep 21, 2015

This enables Event logging for the VM

@brianrob @janvorli @sergiy-k : can you take a look

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/dlls/mscoree/coreclr/CMakeLists.txt

@@ -42,6 +42,7 @@ else()

 endif (WIN32)

+set(CMAKE_ADDITIONAL_LINKEROPTIONS "" )


Could you please remove the CMAKE_ prefix from the variable name? This prefix is used by cmake for its own variables.

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

CMakeLists.txt

 if (WIN32)
  add_definitions(-DFEATURE_EVENT_TRACE=1)
 endif (WIN32)

+if (CLR_CMAKE_PLATFORM_LINUX)


I'd prefer using "OR" in the condition instead of haveing two ifs for the same definition.

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/gc/gcee.cpp

@@ -392,7 +392,7 @@ size_t GCHeap::GetNow()

 void ProfScanRootsHelper(Object** ppObject, ScanContext *pSC, DWORD dwFlags)
 {
-#if defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)
+#if defined(GC_PROFILING) && defined(FEATURE_EVENT_TRACE) 


Is this change really correct? I can see the original form of the condition used at about 50 places in CoreCLR sources.

ScanRootsHelper defined in proftoeeinterfaceimpl.cpp is under define GC_PROFILING

coreclr/src/vm/proftoeeinterfaceimpl.cpp

Line 921 in 4add761

#ifdef PROFILING_SUPPORTED

I am also removing FEATURE_EVENT_TRACE from all such checks

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/gc/gcpriv.h

@@ -1116,9 +1116,9 @@ struct no_gc_region_info
 };

 //class definition of the internal class
-#if defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)
+#if defined(GC_PROFILING) && defined(FEATURE_EVENT_TRACE)


Is this change really correct? I can see the original form of the condition used at about 50 places in CoreCLR sources.

Oops sorry i seem to have got carried away with the conditional, will undo

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/inc/eventtracebase.h

+// Special Handling of Startup events
+//
+
+// "mc.exe -MOF" already generates this block for XP-suported builds inside ClrEtwAll.h;


The same comment is at line 922, is it intentional?

Yes this is is ETW specific code which we have not found use so far

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/pal/src/eventprovider/CMakeLists.txt

+
+
+   add_library(eventprovider1
+"lttngeventprovMicrosoft_Linux_DotNETRuntime.cpp"


The file naming convention in PAL / CoreCLR is all lowercase characters and no underscores. Do we need to have these files different for each OS in such a way that we cannot handle it by a bunch of ifdefs? If it is really the case, then I'd suggest creating a subfolder for each supported OS (and maybe even for the lltng if we plan to support other providers later) and using short names.

Yes we need them to be separate for different logging systems, presumably each OS can have multiple logging systems. I will move these files under a folder named after a logging system

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/vm/CMakeLists.txt

@@ -264,6 +264,9 @@ set(VM_SOURCES_WKS
    weakreferencenative.cpp
 )

+list(APPEND VM_SOURCES_WKS 


Can you please move this source into the previous list of sources (keeping the list alphabetically ordered)

Oops sorry this was the reason for build break on mac and freebsd. This needs to be conditioned on linux and windows

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/vm/eventtrace.cpp

+#else
+void BulkTypeEventLogger::FireBulkTypeEvent()
+{
+//   _ASSERTE(!"Eventing Not Implemented");


Please remove the commented out code. If its purpose was to indicate that we will need to implement this in the future, please use // UNIXTODO: xxxx instead.
.

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/vm/eventtrace.cpp

@@ -1488,6 +1499,9 @@ void BulkStaticsLogger::FireBulkStaticsEvent()

    ULONG result = EventWrite(Microsoft_Windows_DotNETRuntimeHandle, &GCBulkRootStaticVar, _countof(eventData), eventData);
    _ASSERTE(result == ERROR_SUCCESS);
+#else
+//    _ASSERTE(!"Eventing Not Implemented");


Please remove the commented out code. If its purpose was to indicate that we will need to implement this in the future, please use // UNIXTODO: xxxx instead.

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/vm/eventtrace.cpp

@@ -4875,7 +4900,7 @@ VOID ETW::InfoLog::RuntimeInformation(INT32 type)

            LPCGUID comGUID=&g_EEComObjectGuid;

-            LPWSTR lpwszCommandLine = W("");
+            LPWSTR lpwszCommandLine = (LPWSTR)W("");


Please don't cast away constness and rather make the variable LPCWSTR (and change the signature of the functions that get that as parameter to accept LPCWSTR).

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/vm/eventtrace.cpp

@@ -4875,7 +4900,7 @@ VOID ETW::InfoLog::RuntimeInformation(INT32 type)

            LPCGUID comGUID=&g_EEComObjectGuid;

-            LPWSTR lpwszCommandLine = W("");
+            LPWSTR lpwszCommandLine = (LPWSTR)W("");
            LPWSTR lpwszRuntimeDllPath = (LPWSTR)dllPath;


Please don't cast away constness and rather make the variable LPCWSTR (and change the signature of the functions that get that as parameter to accept LPCWSTR).

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/vm/eventtrace.cpp

@@ -5560,7 +5585,9 @@ VOID ETW::LoaderLog::SendAssemblyEvent(Assembly *pAssembly, DWORD dwEventOptions
    }
 }

+#if !defined(FEATURE_PAL)


Please define ETW_INLINE as empty for FEATURE_PAL instead

janvorli · Sep 21, 2015

janvorli reviewed Sep 21, 2015

View changes

janvorli · Sep 21, 2015

src/vm/excep.cpp

@@ -5403,7 +5403,7 @@ LONG InternalUnhandledExceptionFilter_Worker(
 #endif // DEBUGGING_SUPPORTED


-#ifdef FEATURE_EVENT_TRACE    
+#if defined(FEATURE_EVENT_TRACE) && !defined(FEATURE_PAL)  //Enable once Eventrepoter is ported Cross Plat


Please add UNIXTODO to the comment, we use it to quickly locate changes that are planned to be made for Unix

janvorli · Sep 21, 2015

Please fix the OSX / FreeBSD build errors, you can see them here:
http://dotnet-ci.cloudapp.net/job/dotnet_coreclr_mac_release_prtest/1476/consoleFull#-81325627274b77ed1-74ea-41e6-b970-9241fe4855f3

brianrob · Sep 21, 2015

brianrob reviewed Sep 21, 2015

View changes

brianrob · Sep 21, 2015

src/gc/gc.cpp

@@ -31551,7 +31551,7 @@ void gc_heap::descr_generations_to_profiler (gen_walk_fn fn, void *context)
                assert (seg == hp->ephemeral_heap_segment);
                assert (curr_gen_number0 <= max_generation);
                //
-                if ((curr_gen_number0 == max_generation))
+                if (curr_gen_number0 == max_generation)


Can you put this back so that it shows up as no diff (there isn't an actual change here).

No, Not unless we want to stop passing -Wparentheses-equality flag to clang compiler.
With the flag this is reported as an error

Gotcha - thanks for the clarification.

brianrob · Sep 21, 2015

brianrob reviewed Sep 21, 2015

View changes

brianrob · Sep 21, 2015

src/inc/genXplatEtw.pl

+
+    my $stdprolog=<<Copy_Right;
+//
+// Copyright (c) Microsoft. All rights reserved.


I think you also need this header at the top of this file

Yes you are correct, added

brianrob · Sep 21, 2015

brianrob reviewed Sep 21, 2015

View changes

brianrob · Sep 21, 2015

Documentation/building/linux-instructions.md

@@ -36,7 +36,7 @@ ellismg@linux:~$ sudo apt-get update

 Then install the packages you need:

-`ellismg@linux:~$ sudo apt-get install cmake llvm-3.5 clang-3.5 lldb-3.6 lldb-3.6-dev libunwind8 libunwind8-dev gettext`
+`ellismg@linux:~$ sudo apt-get install cmake llvm-3.5 clang-3.5 lldb-3.6 lldb-3.6-dev libunwind8 libunwind8-dev gettext liblttng-ust-dev`


liblttng-ust-dev will be required to build. Have you been able to make LTTng a soft dependency, so that running CoreCLR does not require it?

Nope soft dependency at run time needs non trivial amount of work

brianrob · Sep 21, 2015

brianrob reviewed Sep 21, 2015

View changes

brianrob · Sep 21, 2015

src/gc/gcee.cpp

@@ -408,7 +408,7 @@ void ProfScanRootsHelper(Object** ppObject, ScanContext *pSC, DWORD dwFlags)
    }
 #endif //INTERIOR_POINTERS
    ScanRootsHelper(&pObj, pSC, dwFlags);
-#endif // defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)
+#endif // defined(GC_PROFILING)&& defined(FEATURE_EVENT_TRACE)  


As with @janvorli's comments, I'm surprised that you needed to change any of these ifdefs. Have the semantic meanings of them changed?

see implementation at

coreclr/src/vm/proftoeeinterfaceimpl.cpp

Line 921 in 4add761

#ifdef PROFILING_SUPPORTED

brianrob · Sep 21, 2015

brianrob reviewed Sep 21, 2015

View changes

brianrob · Sep 21, 2015

src/inc/eventtracebase.h

+
+#elif defined(PLATFORM_UNIX)
+
+#include "clrallevents.h"


clrallevents.h should probably be called something a bit more descriptive like clrlttngtracepoints.h so that it's clear that this file is LTTng specific.

Actually this header is for every platform except windows and is not specific to LTTng, Also it has nothing to do with tracepoints,it is a place holder for all the FireEtw* and EventEnabled* functions. Hence i am leaving it as it is

So is this the UNIX, etc. file going forward? If so, then this sounds good.

yes that is the idea

brianrob · Sep 21, 2015

brianrob reviewed Sep 21, 2015

View changes

brianrob · Sep 21, 2015

src/inc/genXplatEtw.pl

+    "win:Double"        =>"ctf_float",
+    "win:Int32"         =>"ctf_integer",
+    "win:Boolean"       =>"ctf_integer",
+    "win:UInt64"        =>"ctf_integer",


Per http://lttng.org/docs/#doc-liblttng-ust-tp-fields, ctf_integer allows you to specify the actual type of the data so that it's size can be taken into consideration. It be good to take that mapping into account here so that the parser doesn't have to handle different size data types for the same field when looking at Windows or Linux traces.

Actually the generated code has the specific datatype, i use the lookup-table instead of checking for each individual type in an elaborate if condition !!! see line 454 in updated script

brianrob · Sep 21, 2015

brianrob reviewed Sep 21, 2015

View changes

brianrob · Sep 21, 2015

src/inc/profilepriv.h

 // This is the helper callback that the gc uses when walking the heap.
 BOOL HeapWalkHelper(Object* pBO, void* pv);
 void ScanRootsHelper(Object** ppObj, ScanContext *pSC, DWORD dwUnused);
 BOOL AllocByClassHelper(Object* pBO, void* pv);
+#endif // defined(PROFILING_SUPPORTED_DATA) || defined(PROFILING_SUPPORTED)


Why does this ifdef need to be moved? I would not have expected you to need to change this.

see implementation at

coreclr/src/vm/proftoeeinterfaceimpl.cpp

Line 921 in 4add761

#ifdef PROFILING_SUPPORTED

ramarag · Sep 28, 2015

ramarag reviewed Sep 28, 2015

View changes

ramarag · Sep 28, 2015

Documentation/project-docs/event-logging-guide.md

@@ -0,0 +1,25 @@
+#Event Logging in CoreCLR


@brianrob can you take a look at the event-logging-guide ?

brianrob · Sep 28, 2015

brianrob reviewed Sep 28, 2015

View changes

brianrob · Sep 28, 2015

Documentation/project-docs/event-logging-guide.md

@@ -0,0 +1,25 @@
+#Event Logging in CoreCLR
+
+For event logging mechanism in coreclr, we are using the [ETW](https://msdn.microsoft.com/en-us/library/windows/desktop/bb968803%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396)- Event Tracing for Windows as a template for all logging mechanism


I think I get what you're saying here, but it would be good to clarify the text a bit. Specifically, it should say that we're using the ETW manifest as the basis for the event definitions on all platforms. Then, we run various scripts to generate the platform specific code that is used for logging.

brianrob · Sep 28, 2015

brianrob reviewed Sep 28, 2015

View changes

brianrob · Sep 28, 2015

Documentation/project-docs/event-logging-guide.md

+
+**Step 2**:
+
+-*For Windows*: TBD


It would be good to track down whoever did this the first time and get them to add instructions for Windows.

brianrob · Sep 28, 2015

brianrob reviewed Sep 28, 2015

View changes

brianrob · Sep 28, 2015

Documentation/project-docs/event-logging-guide.md

+-*For Linux*:
+Run the script at [<Root>/src/inc/genXplatEtw.pl](https://github.com/dotnet/coreclr/blob/master/src/inc/genXplatEtw.pl)
+
+This will modify all the files involved in generating the code for the underlying Event logging system


It would be good to list the files so that developers can ensure their changes are complete. Also, perhaps mention the technology being used for each platform along with the instructions as well.

gkhanna79 · Oct 3, 2015

gkhanna79 reviewed Oct 3, 2015

View changes

gkhanna79 · Oct 3, 2015

src/gc/gcee.cpp

@@ -390,6 +390,7 @@ size_t GCHeap::GetNow()
    return hp->get_time_now();
 }

+#if defined(GC_PROFILING) //UNIXTODO: Enable this for FEATURE_EVENT_TRACE
 void ProfScanRootsHelper(Object** ppObject, ScanContext *pSC, DWORD dwFlags)
 {
 #if defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)


Since GC_PROFILING is now at a higher scope, please remove it from here and update the corresponding #endif.

I would like it to be a cue for whoever is going to separate GC_PROFILING and FEATURE_EVENT_TRACE.

NOTE: This needs to work as we see before my change for FEATURE_EVENT_TRACE, but currently it does not and that it the reason for UNIXTODO, I could have defined it out for linux only but GC_PROFILING is more generic

My suggestion was to remove the GC_PROFILING define from within the ProfScanRootsHelper function now that it is defined at an outer scope.

On Windows, everything gets compiled since both defines (gc profiling and event tracing) are defined. On Linux, gc profiling is not defined by event tracing is - thus, whenever GC profiling is enabled, the inner event tracing would be lit up too.

ok got it done

gkhanna79 · Oct 3, 2015

gkhanna79 reviewed Oct 3, 2015

View changes

gkhanna79 · Oct 3, 2015

src/gc/gcee.cpp

@@ -543,7 +540,7 @@ void GCProfileWalkHeap()
    }
 #endif // defined (GC_PROFILING)

-#ifdef FEATURE_EVENT_TRACE
+#if  defined (GC_PROFILING)//UNIXTODO: Enable this for FEATURE_EVENT_TRACE


We should remove the TODO comment. This is eventing functionality under GC_PROFILING and expected to just lightup when GC_PROFILING is defined.

I would like it to be a cue for whoever is going to separate GC_PROFILING and FEATURE_EVENT_TRACE.

NOTE: This needs to work as we see before my change for FEATURE_EVENT_TRACE, but currently it does not and that it the reason for UNIXTODO, I could have defined it out for linux only but GC_PROFILING is more generic

Same as above.

No the TODO comment is important as it preserves the idea of "what should have happened" when next person is going to fix it

gkhanna79 · Oct 3, 2015

gkhanna79 reviewed Oct 3, 2015

View changes

gkhanna79 · Oct 3, 2015

src/inc/eventtracebase.h

@@ -70,6 +70,69 @@ enum EtwThreadFlags
    kEtwThreadFlagThreadPoolWorker =  0x00000004,
 };

+#ifndef FEATURE_REDHAWK


Why do we check for FEATURE_REDHAWK?

I just move code around to account for its usage in the same file

gkhanna79 · Oct 3, 2015

gkhanna79 reviewed Oct 3, 2015

View changes

gkhanna79 · Oct 3, 2015

src/inc/eventtracebase.h

+#ifndef FEATURE_REDHAWK
+
+#if defined(FEATURE_EVENT_TRACE) 
+#if  !defined(FEATURE_PAL)


Is the goal to only compile this for Windows or exclude it from the PAL build even on Unix platforms?

I just move code around to account for its usage in the same file

gkhanna79 · Oct 3, 2015

gkhanna79 reviewed Oct 3, 2015

View changes

gkhanna79 · Oct 3, 2015

src/inc/eventtracebase.h

@@ -90,7 +153,7 @@ struct ProfilerWalkHeapContext
 };

 class Object;
-
+#if !defined(FEATURE_PAL)


Same question as above.

Here it is to only compile for windows

brianrob · Oct 8, 2015

LGTM. Can you please fix up the CI build errors?

mmitche · Oct 9, 2015

@dotnet-bot test this please

dotnet-bot added the 2 - In Progress label Sep 21, 2015

dnfclas added the cla-already-signed label Sep 21, 2015

ramarag force-pushed the ramarag:enablelttng branch 7 times, most recently from 4fe6749 to c5b0299 Sep 21, 2015

This was referenced Sep 28, 2015

Closed

GCProfileWalkHeapWorker - Enable the function usage on Linux #1641

Closed

Refactor code writing directly to the event stream #1642

ramarag self-assigned this Oct 8, 2015

ramarag force-pushed the ramarag:enablelttng branch 5 times, most recently from ca378d1 to 7cd526d Oct 9, 2015

ramarag force-pushed the ramarag:enablelttng branch 2 times, most recently from cea1127 to c51bed9 Oct 9, 2015

ramarag force-pushed the ramarag:enablelttng branch from c51bed9 to ff328b6 Oct 12, 2015

ramarag added a commit that referenced this pull request Oct 12, 2015

Merge pull request #1598 from ramarag/enablelttng
Enablelttng

Loading status checks…

404ab59

dotnet-bot removed the 2 - In Progress label Oct 12, 2015

ramarag deleted the ramarag:enablelttng branch Jan 7, 2016

This was referenced Feb 28, 2017

Closed

dotnet cli on Yocto dotnet/cli#5120

Open

Allow overriding the __DistroRid in build.sh #9856

Open

Consider removing cross compilation setup code from compileoptions.cmake #9858



		add_library(eventprovider1
		"lttngeventprovMicrosoft_Linux_DotNETRuntime.cpp"


		#elif defined(PLATFORM_UNIX)

		#include "clrallevents.h"

dotnet/coreclr

Join GitHub today

Enablelttng #1598

Conversation

ramarag commented Sep 21, 2015

dotnet-bot added the 2 - In Progress label Sep 21, 2015

dnfclas added the cla-already-signed label Sep 21, 2015

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

janvorli reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

janvorli commented Sep 21, 2015

brianrob reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brianrob reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

brianrob reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

brianrob reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

brianrob reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brianrob reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brianrob reviewed Sep 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

ramarag force-pushed the ramarag:enablelttng branch 7 times, most recently from 4fe6749 to c5b0299 Sep 21, 2015

ramarag reviewed Sep 28, 2015 View changes

This comment has been minimized.

brianrob reviewed Sep 28, 2015 View changes

This comment has been minimized.

brianrob reviewed Sep 28, 2015 View changes

This comment has been minimized.

brianrob reviewed Sep 28, 2015 View changes

This comment has been minimized.

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

janvorli reviewed Sep 21, 2015

View changes

brianrob reviewed Sep 21, 2015

View changes

brianrob reviewed Sep 21, 2015

View changes

brianrob reviewed Sep 21, 2015

View changes

brianrob reviewed Sep 21, 2015

View changes

brianrob reviewed Sep 21, 2015

View changes

brianrob reviewed Sep 21, 2015

View changes

brianrob reviewed Sep 21, 2015

View changes

ramarag force-pushed the ramarag:enablelttng branch 7 times, most recently from `4fe6749` to `c5b0299` Sep 21, 2015

ramarag reviewed Sep 28, 2015

View changes

brianrob reviewed Sep 28, 2015

View changes

brianrob reviewed Sep 28, 2015

View changes

brianrob reviewed Sep 28, 2015

View changes

gkhanna79 reviewed Oct 3, 2015

View changes

gkhanna79 reviewed Oct 3, 2015

View changes