Re: [Qemu-devel] [PATCH] target-*: Get rid of "PC advancement" trick

[Richard Henderson <rth@twiddle.net>]

On 12/12/2015 12:02 PM, Sergey Fedorov wrote:
> On 12/12/15 02:39, Richard Henderson wrote:
>> On 12/10/2015 10:47 AM, Sergey Fedorov wrote:
>>> The "PC advancement" trick was used just after recognizing that a
>>> breakpoint exception was going to be generated. This trick has had two
>>> points:
>>>   1. Guarantee that tb->size isn't zero: there are many places where it's
>>>      expected to be non-zero. In fact, that is even stated in the comment
>>>      for this field.
>>>   2. Try to satisfy disassembler's check for instruction length. To this
>>>      end, PC advancement was done for estimated instruction length, but
>>>      actually, didn't work properly in variable-instruction-length cases.
>>>
>>> Substitute this trick with checking for TB size at the end of
>>> translation. If we get an empty TB then just set tb->size to 1 and skip
>>> disassembling. Setting tb->size to 1 is enough to get correct behaviour,
>>> whereas an empty TB doesn't obviously need to be disassembled.
>>
>> This doesn't help when the TB already has instructions, the TB would
>> ordinarily cross a page boundary, and the breakpoint is at the page boundary.
>
> I see your point. But I am wondering why most architectures stop translating on
> a page boundary whereas i386 and m86k don't. There are some comments which say
> that's to ensure instruction fetch aborts occur at the right place. Isn't it
> necessary for all architectures?

In order to support targets with variable-sized instructions, like i386, where 
a single instruction may straddle the page boundary, generic code can handle a 
TB taking code from two (and only two) pages.

It's true that some of the way we treat i386 TBs plays a bit fast and loose, in 
that it's possible to design a code sequence that will report a page access 
error at the beginning of the TB, rather than another runtime exception which 
might have been visible if we'd executed the TB.

I believe that someone fixed that for arm thumb, but no one has bothered about 
this for i386, m68k, or s390.  Because it just doesn't happen in practice; one 
has to go out of one's way to design the errant code sequence.


> At least for those architectures which do stop translating on a page boundary,
> I think this patch is applicable. Certainly, it would be better to have a
> single solution for all architectures.

For that, I think it might be interesting to arrange for non-empty TBs to exit 
prior to recognizing a breakpoint.  So that a breakpoint TB is always just the 
one operation.  Except for the fact that "generate an exception" has 
traditionally been a target-specific helper, we could almost make the entire 
breakpoint generation be done in common code.

I'd think something like a generic "must we end the TB now" predicate would be 
the proper hook.  It would contain all of the usual stuff: tcg_op_buf_full and 
checks for singlestep, but then add "is there a breakpoint at the next pc".


r~