Re: [Qemu-devel] [Qemu-ppc] [PATCH for-1.1 3/3] tcg/ppc: Fix CONFIG_TCG_PASS_AREG0 mode

[Alexander Graf]

On 08.05.2012, at 20:20, Alexander Graf wrote:

> 
> On 08.05.2012, at 19:43, Alexander Graf wrote:
> 
>> 
>> On 08.05.2012, at 19:39, Alexander Graf wrote:
>> 
>>> 
>>> On 07.05.2012, at 01:46, Andreas Färber wrote:
>>> 
>>>> Adjust the tcg_out_qemu_{ld,st}() slow paths to pass AREG0 in r3.
>>>> Automate the register numbering to avoid double-coding the two modes,
>>>> and introduce TCG_TARGET_CALL_ALIGN_I64_ARG() macro to align for SVR4
>>>> but not for Darwin ABI.
>>>> 
>>>> Based on patch by malc.
>>> 
>>> AREG0-free PPC works for me with this patch on a ppc32 host.
>>> 
>>> Tested-by: Alexander Graf <address@hidden>
>> 
>> I take that one back - it breaks once things get more complex. Debugging ...
> 
> I have no idea how this code could have ever worked. We are getting unknown 
> register numbers as input variables. Then mr them into our C ABI parameter 
> registers (r3+). Then we call the C helper to do the load/store for us.
> 
> Now, what if one of those input parameters is within r3-r7 (which is the 
> highest register passed into the C ld function)? We'd happily do something 
> like
> 
>  mr r3, r5
>  mr r4, r3
>  mr r5, ...
> 
> at which point we have long overwritten the actual value of r3!

This actually makes me wonder. How do we guarantee that all the volatile 
registers at the state of a ld/st are declared as clobbered?

[a few minutes later]

Aha! Through constraints. Then the below patch (which also fixes the issue for 
me) is a lot better of course:


diff --git a/tcg/ppc/tcg-target.c b/tcg/ppc/tcg-target.c
index ace5548..5750a2b 100644
--- a/tcg/ppc/tcg-target.c
+++ b/tcg/ppc/tcg-target.c
@@ -258,6 +258,9 @@ static int target_parse_constraint(TCGArgConstraint *ct, 
const char **pct_str)
         tcg_regset_set32(ct->u.regs, 0, 0xffffffff);
         tcg_regset_reset_reg(ct->u.regs, TCG_REG_R3);
         tcg_regset_reset_reg(ct->u.regs, TCG_REG_R4);
+        tcg_regset_reset_reg(ct->u.regs, TCG_REG_R5);
+        tcg_regset_reset_reg(ct->u.regs, TCG_REG_R6);
+        tcg_regset_reset_reg(ct->u.regs, TCG_REG_R7);
         break;
     case 'K':                   /* qemu_st[8..32] constraint */
         ct->ct |= TCG_CT_REG;
@@ -267,6 +270,7 @@ static int target_parse_constraint(TCGArgConstraint *ct, 
const char **pct_str)
         tcg_regset_reset_reg(ct->u.regs, TCG_REG_R5);
 #if TARGET_LONG_BITS == 64
         tcg_regset_reset_reg(ct->u.regs, TCG_REG_R6);
+        tcg_regset_reset_reg(ct->u.regs, TCG_REG_R7);
 #endif
         break;
     case 'M':                   /* qemu_st64 constraint */