This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] "single step" atomic instruction sequences as a whole on PPC
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: luisgpm at linux dot vnet dot ibm dot com
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 8 May 2007 00:47:07 +0200 (CEST)
- Subject: Re: [RFC] "single step" atomic instruction sequences as a whole on PPC
Luis Machado wrote:
> > It's much better, thanks. Unfortunately there's still a number of
> > issues -- thanks for you patience in dealing with those.
>
> Looks like i missed some. I'm getting used to the standard, however.
> Thanks for reviewing.
Looks good now, thanks.
> > I guess that's OK with me. I'm wondering why we need the one message
> > that's still in there then -- I'd say either we warn whenever we find
> > a sequence we don't understand, or we never warn.
>
> In order to make it more transparent to the user, i removed the messages
> during the process.
Fine with me.
Just one more thing: the ChangeLog isn't quite right. Sorry for not
noticing that earlier ...
> 2007-04-13 Paul Gilliam <pgilliam@us.ibm.com>
> Luis Machado <luisgpm@br.ibm.com>
>
> * rs6000-tdep.c: Defines masks for POWER instructions that set
> and use the reservation flag (LWARX,LDARX,STWCX,STDCX).
You need to mention those flags explicitly.
> * rs6000-tdep.c (deal_with_atomic_sequence): Handles single
> stepping through an atomic sequence of instructions.
> * rs6000-tdep.c (rs6000_software_single_step): Added a function
> call to check if we are stepping through an atomic sequence of
> instructions.
> * rs6000-tdep.c (rs6000_gdbarch_init): Initializes a function to
> check for atomic instruction sequences while single stepping.
Do not describe *why* you're changing things, only *what* you're
changing. For example, in this ChangeLog
(deal_with_atomic_sequence): New function.
is sufficient. To describe *why* you need this function, and what it
does, you should add a comment to the source file -- that's where
people will look for this information.
Also, if you have multiple changes to the same file, you need to
list the file name only once.
I'd write a ChangeLog for this patch somewhat like:
* rs6000-tdep.c (LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION,
STWCX_MASK, STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK,
BC_INSTRUCTION): Define.
(deal_with_atomic_sequence): New function.
(rs6000_software_single_step): Call deal_with_atomic_sequence.
(rs6000_gdbarch_init): Install deal_with_atomic_sequence as
gdbarch_software_single_step routine.
Can you add a comment to the patch before the new function that explains
what it does and why it is needed? Thanks!
> + CORE_ADDR branch_bp; /* Breakpoint at branch instruction's destination. */
> + int insn = read_memory_integer (loc, PPC_INSN_SIZE);
> + int insn_count;
> + int index; /* Index used for the "breaks" array. */
> + int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed). */
> + const int atomic_sequence_length = 16; /* Instruction sequence length. */
> + const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode. */
> + int bc_insn_count = 0; /* Conditional branch instruction count. */
In exchange, you might want to get rid of those comments -- that a variable
called "index" is used as index doesn't need to be pointed out as comment ...
With the new comment and ChangeLog, the patch is OK.
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU Toolchain for Linux on System z and Cell BE
Ulrich.Weigand@de.ibm.com