Hi Eli, again thank you very much for your time. Eli Zaretskii wrote: >> Date: Fri, 22 Jun 2007 09:08:07 +0200 >> From: Markus Deuling >> 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. > attached is a reworked version of the patch hopefully with all the points you mentioned. ChangeLog: * gdb.texinfo: Replace following macros by their appropriate gdbarch routines: (TARGET_CHAR_SIGNED, CALL_DUMMY_LOCATION, CANNOT_FETCH_REGISTER) (CANNOT_STORE_REGISTER, GET_LONGJMP_TARGET, POINTER_TO_ADDRESS) (ADDRESS_TO_POINTER, INNER_THAN, FRAME_NUM_ARGS) (HAVE_NONSTEPPABLE_WATCHPOINT, TARGET_SHORT_BIT, TARGET_INT_BIT) (TARGET_LONG_BIT, TARGET_LONG_LONG_BIT, TARGET_FLOAT_BIT) (TARGET_DOUBLE_BIT, TARGET_LONG_DOUBLE_BIT, TARGET_PTR_BIT (TARGET_ADDR_BIT, SP_REGNUM, PC_REGNUM, PS_REGNUM, FP0_REGNUM) (STAB_REG_TO_REGNUM, ECOFF_REG_TO_REGNUM, DWARF_REG_TO_REGNUM) (SDB_REG_TO_REGNUM, DWARF2_REG_TO_REGNUM, BELIEVE_PCC_PROMOTION) (CONVERT_REGISTER_P, REGISTER_TO_VALUE, VALUE_TO_REGISTER) (POINTER_TO_ADDRESS, ADDRESS_TO_POINTER, EXTRACT_RETURN_VALUE) (STORE_RETURN_VALUE, SKIP_PROLOGUE, MEMORY_INSERT_BREAKPOINT) (BREAKPOINT_FROM_PC, MEMORY_REMOVE_BREAKPOINT, DECR_PC_AFTER_BREAK) (ADDR_BITS_REMOVE, TARGET_PRINT_INSN, SKIP_TRAMPOLINE_CODE) (IN_SOLIB_RETURN_TRAMPOLINE, NAME_OF_MALLOC, ADDRESS_CLASS_TYPE_FLAGS) (ADDRESS_CLASS_TYPE_FLAGS_TO_NAME, ADDRESS_CLASS_TYPE_FLAGS_P). (ADDRESS_CLASS_NAME_to_TYPE_FLAGS, ADJUST_BREAKPOINT_ADDRESS) (PRINT_FLOAT_INFO, PRINT_VECTOR_INFO, INTEGER_TO_ADDRESS) (SKIP_PERMANENT_BREAKPOINT, TARGET_VIRTUAL_FRAME_POINTER) (SOFTWARE_SINGLE_STEP_P (push_dummy_call, stabs_argument_has_addr, unwind_sp, unwind_pc) (print_registers_info, push_dummy_code, unwind_dummy_id): Rework (REGISTER_CONVERT_TO_TYPE, END_OF_TEXT_DEFAULT, GDB_MULTI_ARCH) (GDB_TARGET_IS_HPPA, DEPRECATED_GET_SAVED_REGISTER) (SYMBOLS_CAN_START_WITH_DOLLAR, DEPRECATED_INIT_EXTRA_FRAME_INFO) (DEPRECATED_INIT_FRAME_PC, DEPRECATED_SIGTRAMP_START) (IN_SOLIB_CALL_TRAMPOLINE, NO_HIF_SUPPORT, REGISTER_CONVERTIBLE) (DEPRECATED_REGISTER_RAW_SIZE, PARM_BOUNDARY, DEPRECATED_STACK_ALIGN) (PROLOGUE_FIRSTLINE_OVERLAP, DEPRECATED_POP_FRAME, STEP_SKIPS_DELAY) (TARGET_COMPLEX_BIT, TARGET_DOUBLE_COMPLEX_BIT) (OS9K_VARIABLES_INSIDE_BLOCK, KERNEL_U_ADDR, KERNEL_U_ADDR_HPUX) (REGISTER_U_ADDR, U_REGS_OFFSET, DEBUG_PTRACE): Remove description. (Converting an existing Target Architecture to Multi-arch): Remove section. (gdbarch_unwind_pc, gdbarch_unwind_sp): Renew code example. (gdbarch_addr_bits_remove): Add code example. * gdb.texinfo: Replace REGISTER_NAME by gdbarch_register_name. Is this ok to commit? -- Markus Deuling GNU Toolchain for Linux on Cell BE deuling@de.ibm.com