Re: [PATCH] Adding systemtap probe points in pthread library (slightly revised again)

[Roland McGrath <roland at redhat dot com>]

Unrelated changes, even trivial ones like comment typo fixes, should be in
a separate patch/commit.  I committed those typo fixes myself separately,
and rebased the roland/systemtap branch after that.

Please include the ChangeLog header line in your post.  nptl/ChangeLog is
separate, so log entries go in that file and those don't get the nptl/
prefix on the file names in the log.  It is correct to keep lines under 80
characters, but when you wrap them you need to indent the wrapped lines.

It appears there are some long lines in DESIGN-systemtap-probes.txt and
there are two problems there.  First is that you should keep the lines in
the file itself under 80 characters.  Much worse is that you allowed your
patch to be word-wrapped, so it is no longer a valid patch at all.

Don't use man-page reference style.  The name of the function is
just pthread_create, not pthread_create(3) or pthread_create().

To make the file more readable, write the probe descriptions something like:

pthread_create - probe for pthread_create
	       arg1 = start_routine
	       arg2 = arguments

In that case, "arguments" is a confusing and wrong thing to write, when
what it is in fact is the "argument to start_routine".

Cases like this:

+#if (defined USE_STAP_PROBE) && (defined IS_IN_libpthread)

have gratuitous parentheses that are of no use at all.
Remove them.  In some quarters, the thing to write is "defined (FOO)".
But in glibc sources, we just write "defined FOO".

There is no need to test USE_STAP_PROBE at all.  
That's the whole point of the LIBC_PROBE* macros.
They expand to nothing if USE_STAP_PROBE is disabled.
Just use them directly.  Use plain #ifdef IS_IN_libpthread where appropriate.

It's not at all clear to me that conditionalizing probes on
IS_IN_libpthread is actually appropriate at all.  Please explain your
rationale.


Thanks,
Roland