Re: [1/2 PATCH] Kprobes: watchpoint probes debug registerallocation interface

[Jim Keniston <jkenisto at us dot ibm dot com>]

On Mon, 2005-08-01 at 21:49, Prasanna S Panchamukhi wrote:
> Hi Jim,
> 
> Thanks for your comments, I have updated the patch as per your comments.
> Please see the updated patch and my comments inline below. 
...

> > > +	 */
> > > +	if (unlikely(next_dr7)) {
> > > +		if (prev->debugreg[0] != next->debugreg[0])
> > > +			set_debugreg(current->thread.debugreg[0], 0);
> > > +		if (prev->debugreg[1] != next->debugreg[1])
> > > +			set_debugreg(current->thread.debugreg[1], 1);
> > > +		if (prev->debugreg[2] != next->debugreg[2])
> > > +			set_debugreg(current->thread.debugreg[2], 2);
> > > +		if (prev->debugreg[3] != next->debugreg[3])
> > > +			set_debugreg(current->thread.debugreg[3], 3);
> > 
> > The above should be converted to a for(...) loop.
> > 
> 
> This change gives me a buil error if I change it to a for(...) loop.
>   CHK     include/linux/version.h
>   CHK     usr/initramfs_list
>   CHK     include/linux/compile.h
> {standard input}: Assembler messages:
> {standard input}:1475: Error: bad register name `%dbi'

Yeah, I missed that.  I can't think of any way to parameterize the "movl
xxx,%db0" type of instruction.

> 
> > > +
> > > +static inline unsigned long read_dr(int regnum)
> > > +{
> > > +	unsigned long val = 0;
> > > +	switch (regnum) {
> > > +	case 0:
> > > +		get_dr(0, val);
> > > +		break;
> > > +	case 1:
> > > +		get_dr(1, val);
> > > +		break;
> > > +	case 2:
> > > +		get_dr(2, val);
> > > +		break;
> > > +	case 3:
> > > +		get_dr(3, val);
> > > +		break;
> > > +	case 6:
> > > +		get_dr(6, val);
> > > +		break;
> > > +	case 7:
> > > +		get_dr(7, val);
> > > +		break;
> > > +	}
> > > +	return val;
> > > +}
> > 
> > The above is another situation where a case statement is inappropriate.
> > You could do
> > 
> > #define DR_IS_READABLE(regnum) (0xcf & (1 << (regnum))) // dr0-3, 6, 7
> > 	if (DR_IS_READABLE(regnum))
> > 		get_dr(regnum, val);
> > 
> > or at least
> > 	case 0:
> > 	case 1:
> > 	...
> > 	case 7:
> > 		get_dr(regnum, val);
> 
> If I make above changes, it gives me a build error as shown below.
> 
>   CHK     include/linux/version.h
>   CHK     usr/initramfs_list
>   CHK     include/linux/compile.h
> {standard input}: Assembler messages:
> {standard input}:1475: Error: bad register name `%dbi'
> {standard input}:1492: Error: bad register name `%dbregnum'
> {standard input}:1498: Error: bad register name `%dbregnum'

Yeah, same issue as above.

> 
> Am I missing somethig here?
> Can the above errors be avoided?

I think the answer is "no" in both cases.

Jim