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

[Björn Gustavsson]

On 11/23/11, Scott Lystig Fritchie <fritchie@REDACTED> wrote:
> OK, I've squashed things so that it looks as if the original 4 commits
> are the only ones.

Here are my first batch of comments, found during my first quick
scan. I will probably post more comments later.

Don't  use assert.h in the emulator source code. We have
our own ASSERT() macro defined in sys.h.

The ERTS_INLINE macros are not used in a portable way.
For functions to be used just in the same source file,
use "static ERTS_INLINE". For functions to be used from
several source files, you must put the inline functions in
a header files. Here is an example from global.h how it
should be done:

ERTS_GLB_INLINE void *erts_prtsd_get(Port *p, int ix);

#if ERTS_GLB_INLINE_INCL_FUNC_DEF

ERTS_GLB_INLINE void *
erts_prtsd_get(Port *prt, int ix)
{
    return prt->psd ? prt->psd->data[ix] : NULL;
}

#endif

In addition, the header file must be included from utils.c
(to ensure that the functions will be defined even if
inlining is turned off).

Here are a few git issues, that makes it confusing to
examine the diff in gitk:

lib/Makefile is update in both commit 1/4 and 4/4.
I think that the update should be combined into one
that should go into commit 1/4.

erl_node_tables.h has an addition in commit 1/4:

+extern char erts_this_node_sysname[256]; /* must match erl_node_tables.c */

but is later changed in commit 3/4.


lib/dtrace/c_src/Makefile contains a reference to
"clearmake -C gnu". We no longer support clearmake,
so the comment should not be added to new Makefiles.

If noticed two typos in README.dtrace.md:

diff --git a/README.dtrace.md b/README.dtrace.md
index a01606f..fff8a5f 100644
--- a/README.dtrace.md
+++ b/README.dtrace.md
@@ -28,7 +28,7 @@ Supported platforms
 -------------------

 The autoconf procedure is supported, I believe, for OS X/Snow Leopard
-and OpenSolaris/64-bit.  Just add the `--enable-dtrace` option your
+and OpenSolaris/64-bit.  Just add the `--enable-dtrace` option to your
 command to run the `configure` script.

 The code has been only very lightly tested on OS X.  It ought to
@@ -38,7 +38,7 @@ The autoconf stuff is ugly right now.  It could use
some cleaning up.
 For example:

 * After editing the `erlang_dtrace.d` file, you need to re-run the
-* top-level "configure" script in order to update `erlang_dtrace.h`.
+  top-level "configure" script in order to update `erlang_dtrace.h`.
 * `make clean` will remove `erlang_dtrace.h`.  A build will fail
   unless the top-level "configure" script is re-run to re-create that
   file.


That is all for now.

/Björn

-- 
Björn Gustavsson, Erlang/OTP, Ericsson AB