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

[Scott Lystig Fritchie]

Björn Gustavsson <bgustavsson@REDACTED> wrote:

bg> Doing a default build should work without problems. I did a
bg> "./otp_build setup -a" on my Mac (Mac OS 10.7.2) and got:

Yup, sorry about that.  It's been too long since I last did a build
completely without "--enable-dtrace", and it's ususally only to build
the VM.  That error was in the "dtrace" Erlang app, which (oddly enough)
isn't built when running make inside the erts/emulator dir.

bg> The erts/.gitignore file is included in the commit, but it only adds
bg> a blank line.

Yup, thanks.

bg> Regarding coding style, our main rule is to imitate the style of the
bg> surrounding code in the same file. In this case, in most C source
bg> files, we put the beginning curly bracket on a new line like this:

bg> int foo(void) { ..  }

Yup, also fixed.

bg> (You've put the curly bracket at the end of function declaration
bg> line.)

bg> Also, the most common comment style for "block" comments is:

bg> /* * Here is a comment.  */

bg> (Your added comments seem to have different styles.)

Ditto.

bg> My final comment is that your changes are included in a single
bg> commit, which make it harder to review and will it make it harder to
bg> understand in the future. I would suggest breaking it up into
bg> several commits (at least three): [...]

Well, I've also fixed one NULL pointer dereferencing problem (when a
probe is enabled) and I've got at least one more to fix.  So there's
going to be a bit of churn between now and then.

I'd also really like to try to make a 'ustack()' helper.  That may be
possible, using the GDB macro package erts/etc/unix/etp-commands could
make that wish come true.  But it would be another (small) source of
churn in the coming few days.

Overnight last night, I ran several instances of the full OTP test
suite.  (See also my recent query over on the erlang-bugs list about the
stability of tests for R15A.)  If I can't get stable test suite results,
then ... hrmm.

OK, so ... I'm resigned to more churn happening in the next few days.
However, it'd be great to get the DTrace stuff into "pu" sooner rather
than later.  Is it reasonable to assume that the next step is mine?
Specifically, I'll create a new review branch that addresses all of
Bjorn's and Mikael's comments?  I'll call that review branch
"dtrace-review3".  And I'll work as hard as I can to find & fix bugs
that pop up when the probes are enabled?

Furthermore, when I make bugfixes (and any additional code style edits,
adding 'ustack()' helper, etc), they'll be commits on the
"dtrace-review3" branch.  The understanding is that those later commits
would be squashed prior to merging with "master"?

-Scott