[Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Alexander Graf]

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

PowerPCs have static instruction lengths, so writing an "in-between" brl
detection is quite simple on this architecture. You are welcome to write
something like this for any other platform, but if a compile doesn't
trigger build errors on PowerPC, it shouldn't on other platforms either,
as PowerPC has quite advanced branch instructions.

This patch is not mandatory, makes debugging a lot easier though.

[-- Attachment #2: qemu-gcc4-ppc-warnbranch.patch --]
[-- Type: text/x-patch, Size: 839 bytes --]

Index: qemu-snapshot-2008-01-15_05/dyngen.c
===================================================================
--- qemu-snapshot-2008-01-15_05.orig/dyngen.c
+++ qemu-snapshot-2008-01-15_05/dyngen.c
@@ -1488,6 +1488,16 @@ void gen_code(const char *name, host_ulo
         if (get32((uint32_t *)p) != 0x4e800020)
             error("blr expected at the end of %s", name);
         copy_size = p - p_start;
+
+/* blr check for inline returns */
+
+        if(strstart(name, "op_", NULL) && !strstart(name, "op_exit", NULL)) {
+            for(p=p_start; p < p_end - 4; p+=4) {
+                if ((get32((uint32_t *)p) & 0xfc00fff0) == 0x4c000020) {
+                    error("Inline blr detected in %s. Please append FORCE_RET to the function.", name);
+                }
+            }
+        }
     }
 #elif defined(HOST_S390)
     {

Re: [Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Thiemo Seufer]

Alexander Graf wrote:
> PowerPCs have static instruction lengths, so writing an "in-between" brl
> detection is quite simple on this architecture. You are welcome to write
> something like this for any other platform, but if a compile doesn't
> trigger build errors on PowerPC, it shouldn't on other platforms either,
> as PowerPC has quite advanced branch instructions.
> 
> This patch is not mandatory, makes debugging a lot easier though.

> Index: qemu-snapshot-2008-01-15_05/dyngen.c
> ===================================================================
> --- qemu-snapshot-2008-01-15_05.orig/dyngen.c
> +++ qemu-snapshot-2008-01-15_05/dyngen.c
> @@ -1488,6 +1488,16 @@ void gen_code(const char *name, host_ulo
>          if (get32((uint32_t *)p) != 0x4e800020)
>              error("blr expected at the end of %s", name);
>          copy_size = p - p_start;
> +
> +/* blr check for inline returns */
> +
> +        if(strstart(name, "op_", NULL) && !strstart(name, "op_exit", NULL)) {
> +            for(p=p_start; p < p_end - 4; p+=4) {
> +                if ((get32((uint32_t *)p) & 0xfc00fff0) == 0x4c000020) {
> +                    error("Inline blr detected in %s. Please append FORCE_RET to the function.", name);
> +                }
> +            }
> +        }

Is check_ops.sh not enough for debugging micro-ops?


Thiemo

Re: [Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Alexander Graf]

Thiemo Seufer wrote:
> Alexander Graf wrote:
>   
>> PowerPCs have static instruction lengths, so writing an "in-between" brl
>> detection is quite simple on this architecture. You are welcome to write
>> something like this for any other platform, but if a compile doesn't
>> trigger build errors on PowerPC, it shouldn't on other platforms either,
>> as PowerPC has quite advanced branch instructions.
>>
>> This patch is not mandatory, makes debugging a lot easier though.
>>     
>
>   
>> Index: qemu-snapshot-2008-01-15_05/dyngen.c
>> ===================================================================
>> --- qemu-snapshot-2008-01-15_05.orig/dyngen.c
>> +++ qemu-snapshot-2008-01-15_05/dyngen.c
>> @@ -1488,6 +1488,16 @@ void gen_code(const char *name, host_ulo
>>          if (get32((uint32_t *)p) != 0x4e800020)
>>              error("blr expected at the end of %s", name);
>>          copy_size = p - p_start;
>> +
>> +/* blr check for inline returns */
>> +
>> +        if(strstart(name, "op_", NULL) && !strstart(name, "op_exit", NULL)) {
>> +            for(p=p_start; p < p_end - 4; p+=4) {
>> +                if ((get32((uint32_t *)p) & 0xfc00fff0) == 0x4c000020) {
>> +                    error("Inline blr detected in %s. Please append FORCE_RET to the function.", name);
>> +                }
>> +            }
>> +        }
>>     
>
> Is check_ops.sh not enough for debugging micro-ops?
>
>
>   
Basically it should be. PowerPC branching can be (regex) b..rl. Honestly
I did not know about this script though and as it was not in the
makefile, it did not tell me that something wrong was going on. This
check costs near no time and has to be passed in order to build
successfully.

So either check_ops should be fixed (not only brl) and put into the
Makefile.target or a check like this is good to have.

Alex

Re: [Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Thiemo Seufer]

Alexander Graf wrote:
[snip]
> > Is check_ops.sh not enough for debugging micro-ops?
> >
> Basically it should be. PowerPC branching can be (regex) b..rl. Honestly
> I did not know about this script though and as it was not in the
> makefile, it did not tell me that something wrong was going on. This
> check costs near no time and has to be passed in order to build
> successfully.
> 
> So either check_ops should be fixed (not only brl) and put into the
> Makefile.target or a check like this is good to have.

Fixing check_ops is IMO preferable, OTOH I hope the whole problem goes
away with the new code generator which is in the works.


Thiemo

Re: [Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Johannes Schindelin]

Hi,

On Thu, 17 Jan 2008, Thiemo Seufer wrote:

> Alexander Graf wrote:
> [snip]
> > > Is check_ops.sh not enough for debugging micro-ops?
> > >
> > Basically it should be. PowerPC branching can be (regex) b..rl. Honestly
> > I did not know about this script though and as it was not in the
> > makefile, it did not tell me that something wrong was going on. This
> > check costs near no time and has to be passed in order to build
> > successfully.
> > 
> > So either check_ops should be fixed (not only brl) and put into the
> > Makefile.target or a check like this is good to have.
> 
> Fixing check_ops is IMO preferable, OTOH I hope the whole problem goes
> away with the new code generator which is in the works.

The code generated by that new code generator is substantially slower than 
what we have right now, so I am not so enthusiastic.

Ciao,
Dscho

Re: [Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Thiemo Seufer]

Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 17 Jan 2008, Thiemo Seufer wrote:
> 
> > Alexander Graf wrote:
> > [snip]
> > > > Is check_ops.sh not enough for debugging micro-ops?
> > > >
> > > Basically it should be. PowerPC branching can be (regex) b..rl. Honestly
> > > I did not know about this script though and as it was not in the
> > > makefile, it did not tell me that something wrong was going on. This
> > > check costs near no time and has to be passed in order to build
> > > successfully.
> > > 
> > > So either check_ops should be fixed (not only brl) and put into the
> > > Makefile.target or a check like this is good to have.
> > 
> > Fixing check_ops is IMO preferable, OTOH I hope the whole problem goes
> > away with the new code generator which is in the works.
> 
> The code generated by that new code generator is substantially slower than 
> what we have right now, so I am not so enthusiastic.

I meant not qops but something Fabrice is working on. Which apparently has
about the same speed.


Thiemo

Re: [Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Alexander Graf]

Thiemo Seufer wrote:
> Johannes Schindelin wrote:
>   
>> Hi,
>>
>> On Thu, 17 Jan 2008, Thiemo Seufer wrote:
>>
>>     
>>> Alexander Graf wrote:
>>> [snip]
>>>       
>>>>> Is check_ops.sh not enough for debugging micro-ops?
>>>>>
>>>>>           
>>>> Basically it should be. PowerPC branching can be (regex) b..rl. Honestly
>>>> I did not know about this script though and as it was not in the
>>>> makefile, it did not tell me that something wrong was going on. This
>>>> check costs near no time and has to be passed in order to build
>>>> successfully.
>>>>
>>>> So either check_ops should be fixed (not only brl) and put into the
>>>> Makefile.target or a check like this is good to have.
>>>>         
>>> Fixing check_ops is IMO preferable, OTOH I hope the whole problem goes
>>> away with the new code generator which is in the works.
>>>       
>> The code generated by that new code generator is substantially slower than 
>> what we have right now, so I am not so enthusiastic.
>>     
>
> I meant not qops but something Fabrice is working on. Which apparently has
> about the same speed.
>
>   

Either way I wouldn't mind having a working gcc4 version for the time
being, including checks that it doesn't break too easily again.

Alex

Re: [Qemu-devel] [PATCH 2/5] Add a PowerPC branch detector

[Andreas Färber]

Am 17.01.2008 um 15:38 schrieb Thiemo Seufer:

>>> I hope the whole problem goes
>>> away with the new code generator which is in the works.
>>
>> The code generated by that new code generator is substantially  
>> slower than
>> what we have right now, so I am not so enthusiastic.
>
> I meant not qops but something Fabrice is working on. Which  
> apparently has
> about the same speed.

Are those available to look at somewhere?

Andreas