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

["Ulrich Weigand" <uweigand@de.ibm.com>]

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