[erlang-patches] DTrace patch, review draft #1

Andreas Schultz <>
Fri Nov 11 15:29:20 CET 2011


Hi Scott,

When trying to compile this with gcc-4.6 and systemtap 1.6 on Ubuntu Oneiric, I run into some problems:

* erts_this_node_sysname usage produces errors like this:
gcc  -g  -O3 -fomit-frame-pointer -I/usr/src/dtrace-review1/erts/x86_64-unknown-linux-gnu   -fno-tree-copyrename  -D_GNU_SOURCE -DERTS_SMP -DHAVE_CONFIG_H -Wall -Wstrict-prototypes -Wmissing-prototypes -Wdeclaration-after-statement -DUSE_THREADS -D_THREAD_SAFE -D_REENTRANT -DPOSIX_THREADS -D_POSIX_THREAD_SAFE_FUNCTIONS  -Ix86_64-unknown-linux-gnu/opt/smp -Ibeam -Isys/unix -Isys/common -Ix86_64-unknown-linux-gnu -Izlib  -Ipcre -Ihipe -I../include -I../include/x86_64-unknown-linux-gnu -I../include/internal -I../include/internal/x86_64-unknown-linux-gnu -c beam/dist.c -o obj/x86_64-unknown-linux-gnu/opt/smp/dist.o
beam/dist.c: In function ‘dsig_send’:
beam/dist.c:1691:1: error: invalid application of ‘sizeof’ to incomplete type ‘char[]’ 
beam/dist.c:1751:1: error: invalid application of ‘sizeof’ to incomplete type ‘char[]’ 

The problem is systemtap, it uses a lot of preprocessor magic to at the end generate a sizeof(erts_this_node_sysname). Since erts_this_node_sysname is defined as 'extern char erts_this_node_sysname[];', this will cause the error above. I have fixed that by changing the extern declaration to match the actual declaration:

diff --git a/erts/emulator/beam/erl_node_tables.h b/erts/emulator/beam/erl_node_tables.h
index 364d8eb..b884100 100644
--- a/erts/emulator/beam/erl_node_tables.h
+++ b/erts/emulator/beam/erl_node_tables.h
@@ -169,7 +169,7 @@ extern Sint erts_no_of_not_connected_dist_entries;
 
 extern DistEntry *erts_this_dist_entry;
 extern ErlNode *erts_this_node;
-extern char erts_this_node_sysname[];
+extern char erts_this_node_sysname[64+1];
 
 DistEntry *erts_channel_no_to_dist_entry(Uint);
 DistEntry *erts_sysname_to_connected_dist_entry(Eterm);

* DTRACE/STAP_PROBE11 does no exist in systemtap
gcc -g -O2 -I/usr/src/dtrace-review1/erts/x86_64-unknown-linux-gnu   -fno-tree-copyrename  -D_GNU_SOURCE  -DHAVE_CONFIG_H -Wall -Wstrict-prototypes -Wmissing-prototypes -Wdeclaration-after-statement -DUSE_THREADS -D_THREAD_SAFE -D_REENTRANT -DPOSIX_THREADS -D_POSIX_THREAD_SAFE_FUNCTIONS  -DLIBSCTP= -Ix86_64-unknown-linux-gnu/opt/plain -Ibeam -Isys/unix -Isys/common -Ix86_64-unknown-linux-gnu -Izlib  -Ipcre -Ihipe -I../include -I../include/x86_64-unknown-linux-gnu -I../include/internal -I../include/internal/x86_64-unknown-linux-gnu -Idrivers/common -Idrivers/unix -c drivers/common/efile_drv.c -o obj/x86_64-unknown-linux-gnu/opt/plain/efile_drv.o
drivers/common/efile_drv.c: In function ‘flush_write’:
drivers/common/efile_drv.c:1936:9: warning: implicit declaration of function ‘STAP_PROBE11’ [-Wimplicit-function-declaration]

SystemTap only defines wrapper macros for up to 10 arguments. I have filed a bug with systemtap for this: http://sourceware.org/bugzilla/show_bug.cgi?id=13404
In the meantime since there are only a very few places that use 11 arguments, providing the macro our self should be ok. There is a variadic version of the STAP_PROBE macro, but that actually uses the fixed size macros in the end.

diff --git a/erts/emulator/beam/dtrace-wrapper.h b/erts/emulator/beam/dtrace-wrapper.h
index 55c3e6b..49e997d 100644
--- a/erts/emulator/beam/dtrace-wrapper.h
+++ b/erts/emulator/beam/dtrace-wrapper.h
@@ -59,6 +59,16 @@ inline void dtrace_fun_decode(Process *process,
 #define DTRACE11(name, a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10) \
     erlang_##name((a0), (a1), (a2), (a3), (a4), (a5), (a6), (a7), (a8), (a9), (a10))
 
+#if defined(_SDT_PROBE) && !defined(STAP_PROBE11)
+/* work arround for missing STAP macro */
+#define STAP_PROBE11(provider,name,arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8,arg9,arg10,arg11) \
+  _SDT_PROBE(provider, name, 11, \
+             (arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8,arg9,arg10,arg11))
+#define _SDT_ASM_OPERANDS_11(arg1,arg2,arg3,arg4,arg5,arg6,arg7,arg8,arg9,arg10,arg11) \
+  _SDT_ASM_OPERANDS_10(arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9,arg10), \
+    _SDT_ARG(11, arg11)
+#endif
+
 #else   /* HAVE_DTRACE */
 
 /* Render all macros to do nothing */

Andreas
----- Original Message -----
> Hi, all.  Here's the first draft of a DTrace patch to start the
> review
> process.  Contrary to the commit message (sorry!), this patch as-is
> is
> broken for Linux+SystemTap.  (It still works for OS X Snow Leopard
> and
> Solaris 10.)  The Linux+SystemTap breakage is temporary and will be
> fixed before a final patch is submitted.
> 
> git clone git://github.com/slfritchie/otp.git dtrace-review1
> 
> https://github.com/slfritchie/otp/compare/9a064a8c4f33d091c424bd23713bf76cd0ac7826...dtrace-review1
> https://github.com/slfritchie/otp/compare/9a064a8c4f33d091c424bd23713bf76cd0ac7826...dtrace-review1.patch
> 
> I'll include the commit message below.
> 
> -Scott
> 
> --- snip --- snip --- snip --- snip --- snip --- snip ---
> 
> Add DTrace support for OS X, Solaris, and Linux (via SystemTap)
> 
> Since it's been quite a while since I've written C code, *and* I
> haven't done any significant hacking on the VM itself in years, it's
> quite likely that I haven't done things in 100% proper style.  Or
> my co-collaborators Dustin Sallings (CouchBase) or Michal Ptaszek
> (Erlang Solutions).  My intent for this patch is to start discussion
> and review of DTrace support for consideration for the R15 release.
> 
> For additional background on the motivation for this work, please
> see the slides for the presentation at the Erlang User Conference
> 2011
> in Stockholm:
>   https://www.erlang-factory.com/upload/presentations/462/euc2011-draft2.pdf
> 
> Add autoconf support: use "./configure --enable-dtrace" on all
> supported
> platforms:
>     * OS X Snow Leopard or later
>     * Solaris 10 or OpenSolaris
>     * Linux, via SystemTap's DTrace compatibility packages
> 
> See the file `erts/emulator/beam/erlang_dtrace.d` for the definition
> of all DTrace probes in the virtual machine so far.
> 
> Example D scripts can be found in `lib/dtrace/examples`.  Note that
> if
> you see the error message `{name of probe} does not match any
> probes`,
> then there is no Erlang VM process + DTrace probes running.  To fix,
> start a DTrace-enabled VM or remove `-q` from the `dtrace` command
> line.
> 
> The `lib/dtrace` directory contains a small code-only OTP application
> that contains code that allows Erlang code to trigger a DTrace probe.
> Dynamic creation & deletion of DTrace probes is not currently
> supported, so the `dtrace:p()` function is hacked to allow a variable
> number of arguments (up to four integers and up to four strings) to
> be
> used.  See the comments at the top of `lib/dtrace/src/dtrace.c` for
> more detail.
> 
> One feature that may be controversial is the notion I've introduced
> of a special process dictionary key that can be used by Erlang code
> to
> tag I/O operations for an application-specific purpose.  Right now,
> that tag's name is `dtrace_utag`.  The dictionary keys used by `sys`
> and other modules start with a dollar sign.  Perhaps there is some
> convention (but not a dollar sign?) that this tag should use?
> 
> The purpose of the process dictionary key is to allow the tag to
> be included in trace messages, e.g. for file I/O, without changing
> the
> API of the `file.erl` module's functions.  For example, here's a use
> of the tag when calling the `file:rename/2` function:
> 
>     ()1> put(dtrace_utag, "GGOOOAAALL!!!!!").
>     undefined
> 
>     ()2> dtrace:init().
>     ok
> 
>     %% Now start both the `user-probe.d` and `efile_drv.d` D scripts
>     %% found in the `lib/dtrace/examples` directory.
> 
>     ()3> dtrace:p(7, 8, 9, "one", "four").
>     true
> 
>     %% The output from the `user-probe.d` script:
>     <0.40.0> GGOOOAAALL!!!!! 7 8 9 0 'one' 'four' '' ''
> 
>     ()4> file:rename("old-name", "new-name").
>     {error,enoent}
> 
>     %% The output from the `efile_drv.d` script:
>     async I/O pool port #Port<0.59> queue len 1
>     async I/O pool port #Port<0.59> queue len 0
>     efile_drv enter tag={1,110} user tag GGOOOAAALL!!!!! | RENAME
>     (12) | args: old-name new-name , 0 0 (port #Port<0.59>)
>     async I/O worker tag={1,110} | RENAME (12) | efile_drv-int_entry
>     async I/O worker tag={1,110} | RENAME (12) | efile_drv-int_return
>     efile_drv return tag={1,110} user tag GGOOOAAALL!!!!! | RENAME
>     (12) | errno 2
> 
> I'm not exactly happy with this choice of tagging, namely using
> `put(dtrace_utag, Tag::list())`.  But this is an experiment, so
> we'll see how it goes.  I can't imagine changing the API for
> all file.erl functions in order pass the tag explicitly.
> 
> Some modules have some extensive (ab)use of the C preprocessor to
> reduce the amount of #ifdefs that clutter the code.  In several
> places,
> I have not #ifdef'ed automatic variables because of clutter.  For the
> same reason, there are a handful of cases where I added
> DTrace-related
> members to a struct definition without an #ifdef.  I feel that the
> result is easier to read than earlier drafts where I did use many
> more
> `https://github.com/slfritchie/otp/tree/dtrace-experiment+michal2` if
> you're curious.)  I expect there may be some debate about whether the
> bloat of the affected structs is worthwhile.  I erred on adding stuff
> to structs, especially in the efile_drv.c driver, not having a full
> grasp on what was thread-safe and what was not ... so I erred on the
> side of caution.
> 
> The efile_drv.c has a work-around for a crazy GCC optimization bug.
> Thank goodness for Google, I dunno how I would've found a work-around
> for this silly thing.  Many thanks to Trond Norbye for writing
> clearly
> about the problem in a membase Git repo commit message.
> 
> /*
>  * A note on probe naming: if "__" appears in a provider probe
>  * definition, then two things happen during compilation:
>  *
>  *    1. The "__" will turn into a hypen, "-", for the probe name.
>  *    2. The "__" will turn into a single underscore, "_", for the
>  *       macro names and function definitions that the compiler and
>  *       C developers will see.
>  *
>  * We'll try to use the following naming convention.  We're a bit
>  * limited because, as a USDT probe, we can only specify the 4th part
>  * of the probe name, e.g. erlang*:::mumble.  The 2nd part of the
>  * probe name is always going to be "beam" or "beam.smp", and the 3rd
>  * part of the probe name will always be the name of the function
>  * that's calling the probe.
>  *
>  * So, all probes will be have names defined in this file using the
>  * convention category__name or category__sub_category__name.  This
>  * will translate to probe names of category-name or
>  * category-sub_category-name.
>  *
>  * Each of "category", "sub_category", and "name" may have
>  underscores
>  * but may not have hyphens.
>  */
> 
> Add tentative support for sequential tracing sending, queueing, and
> receiving a message.  I don't believe I've fully covered all the
> major
> places where it would be useful to have the sequential trace token
> info
> in a probe -- guidance from the OTP team would be helpful, if there's
> time to do that kind of review.
> 
> Add global variable `erts_this_node_sysname`.
> 
> The purpose of this new global variable is to have quick
> access to the local node name without having to acquire
> locks to look at the `erts_this_node` variable safely.
> [Though, after some later view, this isn't really required??]
> _______________________________________________
> erlang-patches mailing list
> 
> http://erlang.org/mailman/listinfo/erlang-patches
> 

-- 
-- 
Dipl. Inform.
Andreas Schultz

email: 
------------------ managed broadband access ------------------

Travelping GmbH               phone:           +49-391-8190990
Roentgenstr. 13               fax:           +49-391-819099299
D-39108 Magdeburg             email:       
GERMANY                       web:   http://www.travelping.com

Company Registration: HRB21276 Handelsregistergericht Chemnitz
Geschaeftsfuehrer: Holger Winkelmann | VAT ID No.: DE236673780
--------------------------------------------------------------



More information about the erlang-patches mailing list