Re: [rfc] Replace macros by gdbarch functions in gdbint manual

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri, 22 Jun 2007 09:08:07 +0200
> From: Markus Deuling <deuling@de.ibm.com>
> CC: gdb-patches@sourceware.org, uweigand@de.ibm.com
> 
> thank you for your review. I reworked the patch and think I addressed all of your points.

Thanks.  Unfortunately, this is not yet done, see below.  I apologize
if I failed to make some of these comments the first time you posted
your original patch: it's not easy to see minor problems in such a
large patch.

> The section "Converting an existing Target Architecture to Multi-arch" seems to be obsolete?

I don't know enough about this.  Daniel, could you comment, please?

> -address of the instruction.  ADDR_BITS_REMOVE should filter out these
> +address of the instruction.  gdbarch_addr_bits_remove should filter out these
>  bits with an expression such as @code{((addr) & ~3)}.

gdbarch_addr_bits_remove should be in @code, as it is a C symbol.
(Yes, I know: the original text had that mistake as well.)

> -@item CANNOT_FETCH_REGISTER (@var{regno})
> -@findex CANNOT_FETCH_REGISTER
> +@item int gdbarch_cannot_fetch_register (@var{gdbarch}, @var{regum})
> +@findex gdbarch_cannot_fetch_register
>  A C expression that should be nonzero if @var{regno} cannot be fetched
>  from an inferior process.  This is only relevant if
>  @code{FETCH_INFERIOR_REGISTERS} is not defined.

gdbarch_cannot_fetch_register is a function now, so it sounds like
saying it's ``a C expression'' would be a mistake.  A macro can be an
expression, but a function cannot.

There are other similar wording problems; search for "C expression".

> +stack top) stack address @var{rhs}. Let the function return  @code{lhs < rhs}
                                     ^^^
Two spaces after a period that ends a sentence, please (here and
elsewhere). 

> +@item int gdbarch_in_solib_return_trampoline (@var{gdbarch}, @var{pc}, @var{name})
> +@findex gdbarch_in_solib_return_trampoline
> +Declare this function to evaluate to nonzero if the program is stopped in the
>  trampoline that returns from a shared library.

You repeatedly use "declare a function" where the previous text said
"define a macro".  I don't think that the use of ``declare'' here is
correct: a declaration of a function is its prototype; you really want
to say "define a function", which in C parlance means write the
function's implementation.

> +Return the name of register @var{regnr} as a string.  May return @code{NULL}
> +to indicate that register @var{regnr} is not valid.

A minor stylistic point: "indicate that @var{regnr} is not a valid
register" sounds better.

> -@item SKIP_PERMANENT_BREAKPOINT
> -@findex SKIP_PERMANENT_BREAKPOINT
> +@item void gdbarch_skip_permanent_breakpoint (@var{gdbarch}, @var{regcache})
> +@findex gdbarch_skip_permanent_breakpoint
>  Advance the inferior's PC past a permanent breakpoint.  @value{GDBN} normally
>  steps over a breakpoint by removing it, stepping one instruction, and
>  re-inserting the breakpoint.  However, permanent breakpoints are
>  hardwired into the inferior, and can't be removed, so this strategy
> -doesn't work.  Calling @code{SKIP_PERMANENT_BREAKPOINT} adjusts the processor's
> -state so that execution will resume just after the breakpoint.  This
> -macro does the right thing even when the breakpoint is in the delay slot
> +doesn't work.  Calling @code{gdbarch_skip_permanent_breakpoint} adjusts the
> +processor's state so that execution will resume just after the breakpoint.  
> +This macro does the right thing even when the breakpoint is in the delay slot
   ^^^^^^^^^^
But this isn't a macro anymore, is it?

> +@findex gdbarch_skip_trampoline_code
>  If the target machine has trampoline code that sits between callers and
> -the functions being called, then define this macro to return a new PC
> +the functions being called, then set this function to return a new PC
>  that is at the start of the real function.

"set this function" again.