Add support for emitting ETW events to LTTng #4314

MSLaguana · 2017-11-22T18:26:03Z

We define a number of ETW events on windows that we can emit for diagnostics purposes. With these changes, we support generating LTTng bindings for these same events so the same diagnostics approach can be used on linux and osx.

The script that generates the LTTng bindings from the ETW manifest is based on a similar script used by the dotnet team (see https://github.com/dotnet/coreclr/blob/master/src/scripts/genXplatLttng.py), but I have attempted to simplify it and adapt it for our purposes.

obastemur · 2017-11-27T15:47:43Z

lib/CMakeLists.txt

 )

+if(USE_LTTNG)


lttng is not for osx ++ we better share target_link_libs below.. So, move this under else() for linux below and define ${LTTNG....} and use it under the same target_link_libraries

Having multiple target_link_libraries is fine and additive so that shouldn't cause issues. Since lttng is opt-in, I was thinking that on mac people would just not specify the --lttng flag (or if they did and it broke, they would see that lttng wasn't supported on their system). Do you feel strongly about this?

Could be nice to combine them in the same place in order to prevent future what is happening here confusions.

obastemur · 2017-11-27T15:48:15Z

lib/Common/Core/EtwTraceCore.cpp

@@ -65,3 +66,4 @@ void EtwTraceCore::UnRegister()
 }

 #endif
+#endif


nit: could be nice to add // ENABLE_JS_LTTNG at the end of #endif

obastemur · 2017-11-27T15:48:58Z

lib/Common/Memory/Recycler.cpp

@@ -5968,7 +5968,7 @@ Recycler::ThreadProc()
    }
 #endif

-#ifdef ENABLE_JS_ETW
+#if defined(ENABLE_JS_ETW) && ! defined(ENABLE_JS_LTTNG)


Because LTTng doesn't have any concept of EventActivityIdControl

Perhaps call that out as the reason why, here?

obastemur · 2017-11-27T15:49:07Z

lib/Common/Memory/Recycler.cpp

@@ -6536,7 +6536,7 @@ RecyclerParallelThread::StaticThreadProc(LPVOID lpParameter)
            dllHandle = NULL;
        }
 #endif
-#ifdef ENABLE_JS_ETW
+#if defined(ENABLE_JS_ETW) && ! defined(ENABLE_JS_LTTNG)


obastemur · 2017-11-27T15:49:18Z

lib/Jsrt/JsrtHelper.cpp

@@ -143,7 +143,7 @@ void JsrtCallbackState::ObjectBeforeCallectCallbackWrapper(JsObjectBeforeCollect
            ConfigParser::ParseOnModuleLoad(parser, mod);
        }

-    #ifdef ENABLE_JS_ETW
+    #if defined(ENABLE_JS_ETW) && !defined(ENABLE_JS_LTTNG)


LTTng doesn't have the same register/unregister hooks that ETW has, so this is irrelevant for LTTng, and I thought it was simpler to just remove/avoid the whole registration concept.

We might implement dummy functions instead?

Why -> Adding an additional platform comes with an additional set of dependencies / call requirements. It could be nice to have a common interface (even if it's a dummy function)

obastemur · 2017-11-27T15:50:07Z

lib/Runtime/Base/EtwTrace.cpp

@@ -47,14 +48,17 @@ void EtwCallbackApi::OnSessionChange(ULONG controlCode, PVOID callbackContext)
        }
    }
 }
+#endif


nit: could be nice to add // ENABLE_JS_LTTNG at the end of #endif

obastemur · 2017-11-27T15:51:39Z

build.sh

@@ -52,6 +52,7 @@ PRINT_USAGE() {
    echo "     --libs-only       Do not build CH and GCStress"
    echo "     --lto             Enables LLVM Full LTO"
    echo "     --lto-thin        Enables LLVM Thin LTO - xcode 8+ or clang 3.9+"
+    echo "     --lttng           Enables LTTng support for ETW events"


show error / warning if it's used on OSX? or add a note here?

obastemur · 2017-11-27T15:55:04Z

build.sh

+if [[ $HAS_LTTNG == 1 ]]; then
+    python tools/lttng.py --man `pwd`/manifests/Microsoft-Scripting-Chakra-Instrumentation.man --intermediate `pwd`/out/intermediate
+    mkdir -p `pwd`/out/lttng
+    diff -q `pwd`/out/intermediate/lttng/jscriptEtw.h `pwd`/out/lttng/jscriptEtw.h || cp `pwd`/out/intermediate/lttng/* `pwd`/out/lttng/


Why don't you create jscriptEtw.h already and put it under PlatformAgnostic/Linux ? What's the gain of doing this here dynamically?

++ This approach doesn't comply with build.sh --target-path folder selection.

jscriptEtw.h is one of the generated files, made from the ETW manifest. We could check it in to source control, but then it would get out of date when changes are made to the ETW manifest and that would cause issues. The diff check here is to try to improve incremental builds; if jscriptEtw.h is the same then the manifest hasn't changed, and so (barring changes to the generation script) all the generated files should be the same, so no need to dirty them which would cause most files to be recompiled

Regarding --target-path I wasn't aware of that; how would you recommend addressing that? It looks like currently the target path is contained completely within build.sh, and so I wouldn't be able to have the CMakeLists.txt reference the generated files without passing in their specific location.

MSLaguana · 2017-11-27T17:17:52Z

I've also found an issue with our static builds; I suspect my cmake changes aren't quite right, since when ch dynamically links against libChakraCore.so I can get LTTng support, but when I do static builds and link against libChakraCoreStatic.a it's missing all the custom events. Trying to work out why still.

By placing structs at the end, it is simpler to generate the LTTng bindings There was also a redundant ETW event parameter which was confusing the generator.

obastemur · 2017-11-27T18:40:25Z

@dotnet-bot test OSX static_osx_osx_debug please

obastemur · 2017-11-27T18:41:58Z

CCLock assert issue showsup on slow osx debug runs (similarly static build on exit). This is due to same xplat cleanup issue. Prioritizing it.

obastemur · 2017-11-27T18:56:22Z

tools/lttng.py

+    cmakeLines = []
+    cmakeLines.append("project(Chakra.LTTng)")
+    cmakeLines.append("")
+    cmakeLines.append("add_compile_options(-fPIC)")


https://gcc.gnu.org/onlinedocs/gcc-4.8.0/gcc/Code-Gen-Options.html#Code-Gen-Options

fPIC vs fPIE?

++ could be nice to have these files under (i.e. pal/)lttng ? (instead of creating them dynamically)

The LTTng docs all refer to -fPIC rather than -fPIE.

I could pre-generate these files, but then they would get out of sync with the ETW manifest. These are supposed to be equivalent to the auto-generated-as-part-of-build etw headers e.g. VcBuild\obj\x64_debug\CoreManifests\Microsoft-Scripting-Chakra-InstrumentationEvents.h.

The LTTng docs all refer to -fPIC rather than -fPIE.

fPIC -> Shared Library
fPIE -> Executable.

(referring to ch-static builds)

You don't need to pre-generate to fix this.

cmakeLines.append("if(SHARED_LIBRARY)"); cmakeLines.append( "add_compile_options(-fPIC)"); cmakeLines.append("endif()");

Root ChakraCore CmakeList embeds this file. So SHARED_LIBRARY will be set when necessary.

obastemur · 2017-11-27T18:57:38Z

I've also found an issue with our static builds; I suspect my cmake changes aren't quite right, since when ch dynamically links against libChakraCore.so

Please try -> https://github.com/Microsoft/ChakraCore/pull/4314/files#r153289187

MSLaguana · 2017-11-27T22:20:35Z

I believe I've sorted out the dynamic vs static issue; now instead of always #define TRACEPOINT_PROBE_DYNAMIC_LINKAGE as the .net team do, I define it conditionally only if we will be dynamically linking (based on whether CHAKRA_STATIC_LIBRARY is defined).

obastemur · 2017-11-28T07:40:27Z

I believe I've sorted out the dynamic vs static issue

If things work, we can consider this as an initial step and incrementally make some changes. i.e. I'm feeling strong about removing that py file and having the cpp/h/cmake files under a folder. I also see we still have -fPIC is set regardless, not sure how that works for static builds though?

MSLaguana · 2017-11-28T18:50:22Z

I noticed that some events weren't firing, turns out that __int64 was sometimes #defined to be unsigned long and sometimes not, and lttng was comparing the signature string of the tracepoints and deciding that they were different, and thus not enabling the tracepoints. By #undefining __int64 in that header it seems to fix the issue.

obastemur · 2017-11-28T19:42:04Z

undefining __int64 shouldn't be an issue (for xplat lttng) until we have ARM/x86 around. so please add a macro check to alert us if build arch is not x64.

MSLaguana · 2017-11-28T21:23:59Z

I've added a preprocessor check to make sure we are building 64bit.

MSLaguana · 2017-11-29T00:51:36Z

I've now also added proper support for --target-path by exposing that path to cmake and using directories relative to it.

MSLaguana · 2017-11-29T00:59:26Z

Also just added support for the custom ETW events exposed to JS by moving the (fairly trivial) implementation out of PlatformAgnostic and into its one callsite. @digitalinfinity

obastemur · 2017-11-29T07:49:58Z

LGTM (#4314 (comment) could be really nice to have. Others nit.)

p.s. CI fail doesn't look serious. (unused variable)

jackhorton · 2017-12-04T17:26:12Z

lib/Common/Core/EtwTraceCore.h

@@ -47,6 +47,10 @@
 #define JS_ETW(s) s
 #define IS_JS_ETW(s) s

+#ifdef ENABLE_JS_LTTNG
+#include "jscriptEtw.h"


this is somewhat confusing here (if lttng { include etw }) -- can the output file be jscriptLTTng.h instead? (Or, even better, JScriptLTTng.h or JScriptLttng.h

The jscriptEtw.h file is kind of the cross-over point between ETW and our LTTng support: it provides an ETW-like interface (e.g. EmitEvent* functions) that call into our LTTng functions. If you still think it is confusing, I don't mind changing the name.

jackhorton · 2017-12-04T17:27:02Z

lib/Common/Memory/Recycler.cpp

@@ -5968,7 +5968,7 @@ Recycler::ThreadProc()
    }
 #endif

-#ifdef ENABLE_JS_ETW
+#if defined(ENABLE_JS_ETW) && ! defined(ENABLE_JS_LTTNG)


Perhaps call that out as the reason why, here?

jackhorton · 2017-12-04T17:29:27Z

tools/lttng.py

+    "win:UInt16"        :"const unsigned short",
+    "win:UInt8"         :"const unsigned char",
+    "win:Int8"          :"const char",
+    "win:Pointer"       :"const size_t",


nit - I have no idea how this impacts things, but perhaps uintptr_t is a better xplat version of a Pointer type?

Good point, I think I agree.

jackhorton · 2017-12-04T17:31:08Z

tools/lttng.py

+        "win:Struct"        :"const void",
+        "win:GUID"          :"const GUID",
+        "win:AnsiString"    :"LPCSTR",
+        "win:UnicodeString" :"PCWSTR",


nit - my understanding was the (!L)P*STRs were more or less deprecated, since most of the code base uses LP data types rather than P data types (even though they map down to the same thing"

I believe that some of these mappings here, including the string types, are part of the ETW interface. E.g. if you are passing a UnicodeString to an ETW event handler, I believe it expects a PCWSTR.

Running build.sh with the --lttng flag will generate ETW-like event emitting functions that produce LTTng events. This means that you can get improved diagnostics on non-windows platforms like so: ```bash ./build.sh --lttng # + whatever other flags lttng create mySession lttng enable-event -u "JScript:*" # or more specific events specified in the ETW manifest lttng start out/Release/ch script.js lttng stop lttng view ```

Since LTTng support replaces ETW backing functions, the code supporting the JS-accessible custom ETW event works cross platform, if LTTng support is compiled in. There is no need for the additional platform specific code.

Merge pull request #4314 from MSLaguana:lttng We define a number of ETW events on windows that we can emit for diagnostics purposes. With these changes, we support generating LTTng bindings for these same events so the same diagnostics approach can be used on linux and osx. The script that generates the LTTng bindings from the ETW manifest is based on a similar script used by the dotnet team (see https://github.com/dotnet/coreclr/blob/master/src/scripts/genXplatLttng.py), but I have attempted to simplify it and adapt it for our purposes.

MSLaguana requested review from digitalinfinity, obastemur and curtisman November 22, 2017 18:26

MSLaguana force-pushed the lttng branch 2 times, most recently from a7c5485 to 28ba2f0 Compare November 22, 2017 18:27

obastemur reviewed Nov 27, 2017

View reviewed changes

Tweaking ETW manifest for easier auto-generation of LTTng bindings

98dc145

By placing structs at the end, it is simpler to generate the LTTng bindings There was also a redundant ETW event parameter which was confusing the generator.

MSLaguana force-pushed the lttng branch from 28ba2f0 to 5f7bca9 Compare November 27, 2017 17:52

obastemur reviewed Nov 27, 2017

View reviewed changes

MSLaguana force-pushed the lttng branch from 5f7bca9 to 76af657 Compare November 27, 2017 22:17

MSLaguana force-pushed the lttng branch from 76af657 to a9d359c Compare November 28, 2017 18:29

MSLaguana force-pushed the lttng branch from a9d359c to 1fa5305 Compare November 28, 2017 21:23

MSLaguana force-pushed the lttng branch from 1fa5305 to 2b5477f Compare November 29, 2017 00:51

MSLaguana force-pushed the lttng branch from 2b5477f to 305dfc4 Compare November 29, 2017 00:55

MSLaguana force-pushed the lttng branch from 344ac23 to 94833ae Compare November 29, 2017 16:40

jackhorton approved these changes Dec 4, 2017

View reviewed changes

MSLaguana added 2 commits December 6, 2017 15:25

Moving custom ETW event firing out of windows specifc code

41d2de9

Since LTTng support replaces ETW backing functions, the code supporting the JS-accessible custom ETW event works cross platform, if LTTng support is compiled in. There is no need for the additional platform specific code.

MSLaguana force-pushed the lttng branch from 94833ae to 41d2de9 Compare December 7, 2017 17:24

chakrabot merged commit 41d2de9 into chakra-core:master Dec 7, 2017

@@ @@ -65,3 +66,4 @@ void EtwTraceCore::UnRegister() @@
               }
               #endif
+              #endif

		)

		if(USE_LTTNG)

Add support for emitting ETW events to LTTng #4314

Add support for emitting ETW events to LTTng #4314

Conversation

MSLaguana commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur Nov 27, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented Nov 27, 2017

obastemur commented Nov 27, 2017

obastemur commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur commented Nov 27, 2017

MSLaguana commented Nov 27, 2017

obastemur commented Nov 28, 2017

MSLaguana commented Nov 28, 2017

obastemur commented Nov 28, 2017

MSLaguana commented Nov 28, 2017

MSLaguana commented Nov 29, 2017

MSLaguana commented Nov 29, 2017

obastemur commented Nov 29, 2017

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

obastemur Nov 27, 2017 •

edited

Loading

obastemur Nov 27, 2017 •

edited

Loading