[erlang-patches] DTrace patch, review draft #1
Andreas Schultz
aschultz@REDACTED
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:
>
> (bar@REDACTED)1> put(dtrace_utag, "GGOOOAAALL!!!!!").
> undefined
>
> (bar@REDACTED)2> dtrace:init().
> ok
>
> %% Now start both the `user-probe.d` and `efile_drv.d` D scripts
> %% found in the `lib/dtrace/examples` directory.
>
> (bar@REDACTED)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' '' ''
>
> (bar@REDACTED)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
> erlang-patches@REDACTED
> http://erlang.org/mailman/listinfo/erlang-patches
>
--
--
Dipl. Inform.
Andreas Schultz
email: as@REDACTED
------------------ managed broadband access ------------------
Travelping GmbH phone: +49-391-8190990
Roentgenstr. 13 fax: +49-391-819099299
D-39108 Magdeburg email: info@REDACTED
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