Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

Ulrich Weigand
2007-05-07T22:47:13Z

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