Re: [PATCH 0/1] (Was: Linux 3.11-rc4)

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Aug 08, 2013 at 05:41:07PM +0200, Oleg Nesterov wrote:
> On 08/07, Linus Torvalds wrote:
> >
> > Now, I do agree that the debug registers are *much* less likely to
> > have those kinds of really subtle issues, so maybe relaxing some of
> > the tests might be reasonable. I'd be a bit nervous about it, but if
> > it's *only* the length/alignment, and Intel people can be convinced
> > that it doesn't result in any nasty undefined behavior (as long as the
> > address is in user space), maybe we could make that change just to
> > make it easier for Wine.
> 
> Oh, I do not know. And again, this way a user can't notice the problem
> if the arguments are wrong.
> 
> But personally I think it would be nice to cleanup the perf interface,
> although probably it is too later.
> 
> On x86 execute breakpoints are only a single byte, which has to be
> the first byte of the instruction. IOW the hardware requires len = 1
> in dr7 or it doesn't work (iirc).
> 
> But for some reason perf requires bp_len = sizeof(long), not 1. And
> note that it sets info->len = X86_BREAKPOINT_LEN_X. The comment says:
> 
> 	x86 inst breakpoints need to have a specific undefined len
> 
> but despite its "special" name LEN_X is simply LEN_1, and other code
> relies on this fact.
> 
> Now, ptrace correctly requires DR_LEN_1. So arch_bp_generic_fields()
> translates this into "gen_len = sizeof(long)" for validation.
> 
> arch_build_bp_info() thinks that X86_BREAKPOINT_EXECUTE should have
> ->bp_len == sizeof(long), so we translate it back into LEN_1 internally.

I did this interface and I'm sorry about it.

This bp_len == sizeof(long) requirement comes from a very buggy conception
I had at the time I wrote that. I thought it would be pretty intuitive to
assume that instruction breakpoints should be the size of the instruction
itself as a generic interface for all archs. But at least x86 instructions
size aren't static. That sizeof(long) assumption just popped up from nowhere
at 5 am two years ago I guess :-(

And worse: I realized that mistake later but never moved it in the top of the
TODO-list pile because I had the feeling that nobody was using the perf breakpoint
interface anyway.

I'm all for fixing this. May be we can start by backporting a patch that
ignores the value of gen_len for instruction breakpoints in x86?

I don't know how other archs use it. I need to check. But this bp_len
should rather be used for range breakpoints on archs that support it. I
hope we can still reuse it if the damage of my initial misconception
isn't too widely expanded.

What do you think?

> 
> This looks confusing, imho. And imho X86_BREAKPOINT_LEN_X should die.

Yep.

Thanks.