Re: [PATCH] PowerPC - Add a faster way to read the Time Base register

[Roland McGrath <roland at hack dot frob dot com>]

> 	* sysdeps/powerpc/Makefile (tests): Add test-gettimebase.
> 	(sysdep_headers): Include sys/platform/ppc.h.
> @@ -26,3 +26,7 @@ gen-as-const-headers += rtld-global-offsets.sym
>  # get offset to __locale_struct.__ctype_tolower
>  gen-as-const-headers += locale-defines.sym
>  endif
> +
> +sysdep_headers += sys/platform/ppc.h
> +
> +tests += test-gettimebase

These need to be inside some ifeq ($(subdir),...) conditional.
Otherwise you're adding the same things to every subdir, so the header
will be installed n times and the same test built and run n times.

Usually we use the misc subdir for these sorts of things.

> 	* manual/Makefile (appendices): Include platform.texi.
> 	* manual/contrib.texi: Likewise.

contrib.texi does not include platform.texi, so this is wrong.
It should say:

	* manual/contrib.texi (Contributors): Update @node pointers.
	* manual/maint.texi (Maintenance): Likewise.

However, Texinfo @node has not actually required the manual pointers
for a long time.  So we can just change these to "@node Name" and
leave it at that.

> 	* manual/maint.texi: Document how to include platform-specific
> 	headers.

This needs to cite the specific changes, i.e.:

	* manual/maint.texi (Platform): New node.

> 	* manual/platform.texi: New file. Document the new features.

Two spaces between sentences.

> +@menu
> +* Adding Platform-specific Features::    Adding platform-specific features.
> +@end menu
> +
> +@node Adding Platform-specific Features
> +@appendixsubsec Platform-specific types, macros and functions

That's pretty long for a node name.  Remember, a node name is
something people might type (with completion) using the g command in
info mode or the standalone info reader.  So make it more concise.
Something like "Platform-specific" or "Platform" is better.  Since
you've already used "Platform" elsewhere as a node name, here you
can use "Adding Platform-specific".

The lhs of the menu item doesn't have to match the node name.  The
purpose of the lhs is to be convenient to type (with completion) using
the m command.  It only has to be unique within that menu.  So here
you can just use "Platform::".

> +The current guidelines are:
> +@itemize @bullet
> +@item
> +If the header provides features that only make sense on a particular machine
> +architecture and have nothing to do with an operating system, then the
> +features should be provided as GCC builtins.

You state this as a guideline but then this very change violates the
guideline, as __ppc_get_timebase has nothing to do with an operating
system.  So what's the rationale for this being implemented in libc
rather than as a GCC built-in?  I'm not especially arguing that it
shouldn't be added to libc.  But the documentation claiming to provide
general guidelines should not advise something contrary to what you're
actually doing.  So if there is good reason to have this in libc
rather than only in a GCC built-in, then the guidelines should cover
the situation clearly.

> +@item
> +If the header provides features that are specific to an operating system
> +but used by @theglibc{}, then @theglibc{} should provide them.
> +
> +@end itemize

You don't need a blank line before @end here.

> +@itemize @bullet
> +@item
> +Nonstandard low-level headers that define macros and static inline
> +functions go in @file{sys/platform/}.

Just say "inline", not "static inline".  Also, I think it's better to
phrase this as a file name rather than a directory name.  Try:

A nonstandard, low-level header file that defines macros and inline
functions should be called @file{sys/platform/@var{name}.h}.

> +@item
> +Each is uniquely named per platform to avoid users thinking they have
> +anything in common e.g.,  @file{sys/platform/@var{arch}.h} or
> +@file{sys/platform/ppc.h}, but not @file{sys/platform.h}.

Be less terse: say "Each header file".  Throughout, always say "header
file", not just "header".  (Some of the latter-day parts of the manual
have been sloppy about this, but the intro.texi sections are
consistent and define the term as @dfn{header files}.)

You don't need two spaces after "e.g.,", but you do need proper
punctuation before "e.g."  It's better to use more, shorter sentences
and be a bit more verbose:

Each header file's name should include the platform name, to avoid
users thinking there is anything in common between different the
header files for different platforms.  For example, a
@file{sys/platform/@var{arch}.h} name such as
@file{sys/platform/ppc.h} is better than @file{sys/platform.h}.

> +@item
> +The platform header provided by @theglibc{} should coordinate with GCC such
> +that compiler built-in versions of the functions and macros are
> +preferred if available.  This means that user programs will only ever need to
> +include @file{sys/platform/@var{arch}.h}, keeping the same names of types,
> +macros, and functions for convenience and portability.

Say, "A platform-specific header file..."

Earlier you used "builtin" but here you used "built-in".  Be
consistent.  The phraseology used in the GCC manual is "built-in
function", so use that (and never "builtin" or "built-in" alone as a
noun).

> +@item
> +Each included symbol must have the prefix @file{__@var{arch}_}, e.g.,
> +@file{__ppc_get_timebase}.

@code, not @file.  Whenever you can use plain English phraseology like
"such as" rather than the more academic "e.g.", do that.

> +The easiest way to ship a header is to add it to the @code{sysdep_headers}
> +variable. For example, the combination of Linux-specific headers on
> +PowerPC could be provided like this:

Two spaces between sentences.  s/header/& file/ again.  I don't think
it's useful to use the term "ship".  Say "provide" or another similar
verb that is about making a thing exist, rather than a term that in
plain English means to transport but here is being used in product-speak.

> +@smallexample
> +sysdep_header += sys/platform/ppc.h
> +@end smallexample

sysdep_headers

> +Then ensure that you have added a @file{sys/platform/ppc.h}
> +header underneath your target platform sysdeps directory, e.g.,
> +@file{sysdeps/powerpc/sys/platform/ppc.h}.

Say the simpler "in" rather than "underneath".  It's not "your target
platform directory", it's "the machine-specific directory".  The term
"target" as you're using it does not make sense anywhere in the libc manual.

> +@Theglibc{} can provide platform-specific functionality.
> +@ref{Adding Platform-specific Features} describes the process to include
> +these facilities in @theglibc{} and whether they should be included in
> +GCC as well.

We have never used the term "platform" like this before in libc
documentation AFAIK.  When we mean machine-specific we say that.
When we mean operating system-specific, we say that.

> +@node PowerPC Architecture Facilities
> +@appendixsec Summary of Facilities Specific to the PowerPC Architecture

It's not a summary.  It's the only description there is.
I don't think saying "Architecture" adds anything here.
And remember, short node names.
I'd use:

@node PowerPC
@appendixsec PowerPC-specific Facilities

> +All the facilities specific to the PowerPC Architecture that aren't OS
> +specific are provided by the header @file{sys/platform/ppc.h}.

Facilities specific to PowerPC that are not specific to a particular
operating system are declared in @file{sys/platform/ppc.h}.

Don't say "all the facilities", because probably we'll one day add
something else that will be in its own header file.

> +@deftp {Data Type} {__ppc_timebase}
> +This is an unsigned long long type that represents the value read from the
> +timebase register.
> +@end deftp

If it were appropriate to use the C type name like that, you would
write @code{unsigned long long int}.  (Use @code for
programming-language syntax; never omit implicit "int".)  But it's not
appropriate to say what the underlying type is--if that were a matter
to be documented, there wouldn't be a typedef at all.  

So say "an unsigned integer type".  If the intent of the API is that
it is known to be exactly 64 bits, then we don't need this typedef at
all and instead should just use uint64_t.

Why doesn't this typedef name end in _t?

"timebase" is not an English word.  The other documentation I found
on the web says "time base register".  So use that.

But this explanation is almost entirely useless.  It doesn't say what
a time base register is, or what the meaning of the values of this
type are.  If they don't have an exact well-defined meaning, that is
OK.  But say whatever it does mean, and define the term (using @dfn).
e.g.

@deftp {Data Type} __ppc_timebase
This is an unsigned integer type that represents a value read from the
PowerPC time base register.  A @dfn{time base} is ...

> +@deftypefun {__ppc_timebase} __ppc_get_timebase (void)
> +Read the timebase register without the need of a system call.

"system call" is an operating system implementation detail.
Don't say what the function doesn't do.  Say what it does and
what matters about that.

Read the current value of the time base register.
@code{__ppc_timebase} uses the processor's time base facility directly
without requiring assistance from the operating system, so it is very
efficient.

> +++ b/sysdeps/powerpc/sys/platform/ppc.h
> @@ -0,0 +1,53 @@
> +/* Copyright (C) 2012 Free Software Foundation, Inc.

Always make the first line of a new file a descriptive comment.
Put the copyright notice on the second line.

> +/* Read the Time Base Register
> +   The Time Base Register is a 64-bit register that stores a monotonically
> +   incremented value updated at a system-dependent frequency that may be
> +   different from the processor frequency.
> +   More information in Power ISA 2.06b - Book II - Section 5.2   */

It's great to have that much explanation in the comment.  But
something like that sentence is exactly what ought to be in the manual
defining the term.

> +  __ppc_timebase __tb;
> +  __asm__ volatile (

Is volatile really required?  The asm has an output, so it does not on
its face seem to require volatile.  If there is a specific intent
behind adding volatile, such as to ensure that the time sample is
taken exactly where the call appears and never moved down to where the
value is used later in the compiled code, then explain that in a comment.

> +		    "mfspr %0, 268\n"
> +		    : "=r" (__tb)
> +		    : );

You never need a final colon with nothing after it in an asm.

You don't need a \n at the end of an asm, only between lines in a
multiline asm.  If you're trying to make the assembly code look nice
for -S readers, use \n\t between lines.

> +  return __tb;
> +#else  /* not __powerpc64__ */
> +  register unsigned long __tbu, __tbl, __tmp; \

register is meaningless, just drop it.
Always use "unsigned long int", not "unsigned long".

> +  __asm__ volatile (
> +		    "0:\n"
> +		    "mftbu %0\n"
> +		    "mftbl %1\n"
> +		    "mftbu %2\n"
> +		    "cmpw %0, %2\n"
> +		    "bne- 0b\n"
> +		    : "=r" (__tbu), "=r" (__tbl), "=r" (__tmp)
> +		    : );

Superfluous colon again.

> +  return (( (__ppc_timebase) __tbu << 32) | __tbl);

No need for the extra space before the cast.


Thanks,
Roland