[Qemu-devel] [PATCH][RFC] Run time TCGv size check for debugging

[Qemu-devel] [PATCH][RFC] Run time TCGv size check for debugging

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

Hi,

When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
one and vice versa. This patch adds a run time sanity check for TCGv
sizes.

Because a 32 bit Qemu host does not really use 64 bit TCGvs, the patch
is only functional on a 64 bit host. Of course also a pure 32 bit Qemu
target is not likely to suffer from TCGv size confusion.

Some use cases are not covered yet. Comments?

[-- Attachment #2: runtime_tcgv_size_check.diff --]
[-- Type: plain/text, Size: 15798 bytes --]

Re: [Qemu-devel] [PATCH][RFC] Run time TCGv size check for debugging

[Paul Brook]

On Wednesday 29 October 2008, Blue Swirl wrote:
> Hi,
>
> When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
> the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
> one and vice versa. This patch adds a run time sanity check for TCGv
> sizes.

Would it make more sense to push these down into tcg_gen_op* ?
tcg-op.h is already fairly unwieldy.

I wonder if it's worth adding TCG_LOW to enable checking on 32-bit hosts.

For futureproofing I'd name things FOO_I32 rather than FOO_32.

Paul

Re: [Qemu-devel] [PATCH][RFC] Run time TCGv size check for debugging

[Blue Swirl]

On 10/29/08, Paul Brook <paul@codesourcery.com> wrote:
> On Wednesday 29 October 2008, Blue Swirl wrote:
>  > Hi,
>  >
>  > When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
>  > the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
>  > one and vice versa. This patch adds a run time sanity check for TCGv
>  > sizes.
>
>
> Would it make more sense to push these down into tcg_gen_op* ?

How? At that point we don't know what was the correct size.

>  tcg-op.h is already fairly unwieldy.

True, and as debugging TCGv will not be common, I'm not sure whether
the patch is worth committing.

>  I wonder if it's worth adding TCG_LOW to enable checking on 32-bit hosts.
>
>  For futureproofing I'd name things FOO_I32 rather than FOO_32.

Good point, will fix.

Re: [Qemu-devel] [PATCH][RFC] Run time TCGv size check for debugging

[Paul Brook]

On Wednesday 29 October 2008, Blue Swirl wrote:
> On 10/29/08, Paul Brook <paul@codesourcery.com> wrote:
> > On Wednesday 29 October 2008, Blue Swirl wrote:
> >  > Hi,
> >  >
> >  > When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
> >  > the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
> >  > one and vice versa. This patch adds a run time sanity check for TCGv
> >  > sizes.
> >
> > Would it make more sense to push these down into tcg_gen_op* ?
>
> How? At that point we don't know what was the correct size.

I figure there's only a handful of different cases, so it'll be cleaner to 
introduce tcg_gen_op_i32_i32 etc. which trivially reduce to tcg_gen_op2 when 
debugging is disabled.

Paul

Re: [Qemu-devel] [PATCH][RFC] Run time TCGv size check for debugging

[Blue Swirl]

On 10/29/08, Paul Brook <paul@codesourcery.com> wrote:
> On Wednesday 29 October 2008, Blue Swirl wrote:
>  > On 10/29/08, Paul Brook <paul@codesourcery.com> wrote:
>  > > On Wednesday 29 October 2008, Blue Swirl wrote:
>  > >  > Hi,
>  > >  >
>  > >  > When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
>  > >  > the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
>  > >  > one and vice versa. This patch adds a run time sanity check for TCGv
>  > >  > sizes.
>  > >
>  > > Would it make more sense to push these down into tcg_gen_op* ?
>  >
>  > How? At that point we don't know what was the correct size.
>
>
> I figure there's only a handful of different cases, so it'll be cleaner to
>  introduce tcg_gen_op_i32_i32 etc. which trivially reduce to tcg_gen_op2 when
>  debugging is disabled.

Now I see. I'll try that next.

[Qemu-devel] Re: [PATCH][RFC] Run time TCGv size check for debugging

[Fabrice Bellard]

Blue Swirl wrote:
> Hi,
> 
> When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
> the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
> one and vice versa. This patch adds a run time sanity check for TCGv
> sizes.
> 
> Because a 32 bit Qemu host does not really use 64 bit TCGvs, the patch
> is only functional on a 64 bit host. Of course also a pure 32 bit Qemu
> target is not likely to suffer from TCGv size confusion.
> 
> Some use cases are not covered yet. Comments?

Theses tests can be done at compile time by introducing the TCGv_i32 and
TCGv_i64 types. The same can be done with the helpers by using a few
macros to declare them.

A optional runtime check would be still useful as an additional pass
using the OP definitions to ensure that the TCG optimizations pass(es)
are OK. IMHO, doing it only in tcg_gen_xxx is not enough.

Fabrice.

Re: [Qemu-devel] Re: [PATCH][RFC] Run time TCGv size check for debugging

[Paul Brook]

On Wednesday 29 October 2008, Fabrice Bellard wrote:
> Blue Swirl wrote:
> > Hi,
> >
> > When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
> > the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
> > one and vice versa. This patch adds a run time sanity check for TCGv
> > sizes.
> >
> > Because a 32 bit Qemu host does not really use 64 bit TCGvs, the patch
> > is only functional on a 64 bit host. Of course also a pure 32 bit Qemu
> > target is not likely to suffer from TCGv size confusion.
> >
> > Some use cases are not covered yet. Comments?
>
> Theses tests can be done at compile time by introducing the TCGv_i32 and
> TCGv_i64 types. The same can be done with the helpers by using a few
> macros to declare them.

That would also require updating all the target code in translate.c to use 
these types.  In principle there's no reason why this couldn't be done, but 
it'd be a much more invasive change.
AFAIK there's no way of doing compile time inheritance checking in C.  

Paul

Re: [Qemu-devel] Re: [PATCH][RFC] Run time TCGv size check for debugging

[Fabrice Bellard]

Paul Brook wrote:
> On Wednesday 29 October 2008, Fabrice Bellard wrote:
>> Blue Swirl wrote:
>>> Hi,
>>>
>>> When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
>>> the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
>>> one and vice versa. This patch adds a run time sanity check for TCGv
>>> sizes.
>>>
>>> Because a 32 bit Qemu host does not really use 64 bit TCGvs, the patch
>>> is only functional on a 64 bit host. Of course also a pure 32 bit Qemu
>>> target is not likely to suffer from TCGv size confusion.
>>>
>>> Some use cases are not covered yet. Comments?
>> Theses tests can be done at compile time by introducing the TCGv_i32 and
>> TCGv_i64 types. The same can be done with the helpers by using a few
>> macros to declare them.
> 
> That would also require updating all the target code in translate.c to use 
> these types.  In principle there's no reason why this couldn't be done, but 
> it'd be a much more invasive change.

If you define TCGv as the word size of the emulated CPU, it will
eliminates most of the changes.

> AFAIK there's no way of doing compile time inheritance checking in C.

Fabrice.

Re: [Qemu-devel] Re: [PATCH][RFC] Run time TCGv size check for debugging

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]

On 10/30/08, Fabrice Bellard <fabrice@bellard.org> wrote:
> Paul Brook wrote:
>  > On Wednesday 29 October 2008, Fabrice Bellard wrote:
>  >> Blue Swirl wrote:
>  >>> Hi,
>  >>>
>  >>> When emulating a mixed 32/64 bit Qemu target CPUs it's easy to confuse
>  >>> the TCGv size, passing 32 bit TCGv to a function expecting a 64 bit
>  >>> one and vice versa. This patch adds a run time sanity check for TCGv
>  >>> sizes.
>  >>>
>  >>> Because a 32 bit Qemu host does not really use 64 bit TCGvs, the patch
>  >>> is only functional on a 64 bit host. Of course also a pure 32 bit Qemu
>  >>> target is not likely to suffer from TCGv size confusion.
>  >>>
>  >>> Some use cases are not covered yet. Comments?
>  >> Theses tests can be done at compile time by introducing the TCGv_i32 and
>  >> TCGv_i64 types. The same can be done with the helpers by using a few
>  >> macros to declare them.
>  >
>  > That would also require updating all the target code in translate.c to use
>  > these types.  In principle there's no reason why this couldn't be done, but
>  > it'd be a much more invasive change.
>
>
> If you define TCGv as the word size of the emulated CPU, it will
>  eliminates most of the changes.

This version introduces TCGv_i32 and TCGv_i64. TCGv_ptr and TCGv (TL
sized) are based on them.

For Sparc, the patch is very invasive (I just commented out the
helpers to avoid that part)  but I think i386 would need much smaller
changes.

With the patch, I found some bugs in Sparc translation. I'm not sure
what to do with helpers, there should be a way to declare the size of
the arguments somehow and then the calling should be easier than:
tcg_gen_helper_1_4_i64_tl_i32_i32_i32(helper_ld_asi, dst, addr, r_asi,
r_size, r_sign);

Otherwise, I think only some variant of the TCGV_LOW parts are worth
committing, they make the code slightly more easy to understand.

[-- Attachment #2: compile_time_tcgv_size_check.diff.bz2 --]
[-- Type: application/x-bzip2, Size: 14840 bytes --]

[-- Attachment #3: sparc_tcgv_size_fixes.diff --]
[-- Type: plain/text, Size: 6657 bytes --]

Re: [Qemu-devel] Re: [PATCH][RFC] Run time TCGv size check for debugging

[Paul Brook]

> This version introduces TCGv_i32 and TCGv_i64. TCGv_ptr and TCGv (TL
> sized) are based on them.

This is looking good to me.

> For Sparc, the patch is very invasive (I just commented out the
> helpers to avoid that part)  but I think i386 would need much smaller
> changes.
>
> With the patch, I found some bugs in Sparc translation. I'm not sure
> what to do with helpers, there should be a way to declare the size of
> the arguments somehow and then the calling should be easier than:
> tcg_gen_helper_1_4_i64_tl_i32_i32_i32(helper_ld_asi, dst, addr, r_asi,
> r_size, r_sign);

I think we can build some infrastructure to make helpers easier to handle.  
It should be relatively mechanical changes to the existing helper[s].h.  ARM 
and m68k already do this, though need a bit more work to get static 
typechecking.  All the information it there's it's just not quite in the 
right form.  I'll see if I can dig out the CPP magic required for this.

Paul

Re: [Qemu-devel] Re: [PATCH][RFC] Run time TCGv size check for debugging

[Paul Brook]

> > With the patch, I found some bugs in Sparc translation. I'm not sure
> > what to do with helpers, there should be a way to declare the size of
> > the arguments somehow and then the calling should be easier than:
> > tcg_gen_helper_1_4_i64_tl_i32_i32_i32(helper_ld_asi, dst, addr, r_asi,
> > r_size, r_sign);
>
> I think we can build some infrastructure to make helpers easier to handle.
> It should be relatively mechanical changes to the existing helper[s].h. 
> ARM and m68k already do this, though need a bit more work to get static
> typechecking.  All the information it there's it's just not quite in the
> right form.  I'll see if I can dig out the CPP magic required for this.

I've got something that works. Any chance you could resend you patch with a 
signed-off-by line so I can do the integration?

Paul

Re: [Qemu-devel] Re: [PATCH][RFC] Run time TCGv size check for debugging

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

On 11/1/08, Paul Brook <paul@codesourcery.com> wrote:
> > > With the patch, I found some bugs in Sparc translation. I'm not sure
>  > > what to do with helpers, there should be a way to declare the size of
>  > > the arguments somehow and then the calling should be easier than:
>  > > tcg_gen_helper_1_4_i64_tl_i32_i32_i32(helper_ld_asi, dst, addr, r_asi,
>  > > r_size, r_sign);
>  >
>  > I think we can build some infrastructure to make helpers easier to handle.
>  > It should be relatively mechanical changes to the existing helper[s].h.
>  > ARM and m68k already do this, though need a bit more work to get static
>  > typechecking.  All the information it there's it's just not quite in the
>  > right form.  I'll see if I can dig out the CPP magic required for this.
>
>
> I've got something that works. Any chance you could resend you patch with a
>  signed-off-by line so I can do the integration?

I don't trust these much, but anyway:

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

[-- Attachment #2: compile_time_tcgv_size_check.diff.bz2 --]
[-- Type: application/x-bzip2, Size: 14840 bytes --]