| Message ID | 1443558863-26132-3-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/30/2015 06:34 AM, Eduardo Habkost wrote: > Left shift of negative values is undefined behavior. Detected by clang: > qemu/target-i386/translate.c:2423:26: runtime error: > left shift of negative value -8 > > This changes the code to reverse the sign after the left shift. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/translate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-i386/translate.c b/target-i386/translate.c > index 8b35de1..cc59b7e 100644 > --- a/target-i386/translate.c > +++ b/target-i386/translate.c > @@ -2420,7 +2420,7 @@ static void gen_pusha(DisasContext *s) > { > int i; > gen_op_movl_A0_reg(R_ESP); > - gen_op_addl_A0_im(-8 << s->dflag); > + gen_op_addl_A0_im(-(8 << s->dflag)); Better as -8U << s->d_flag? > if (!s->ss32) > tcg_gen_ext16u_tl(cpu_A0, cpu_A0); > tcg_gen_mov_tl(cpu_T[1], cpu_A0); >
On Thu, Oct 01, 2015 at 11:35:52AM +1000, Richard Henderson wrote: > On 09/30/2015 06:34 AM, Eduardo Habkost wrote: > >Left shift of negative values is undefined behavior. Detected by clang: > > qemu/target-i386/translate.c:2423:26: runtime error: > > left shift of negative value -8 > > > >This changes the code to reverse the sign after the left shift. > > > >Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >--- > > target-i386/translate.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >diff --git a/target-i386/translate.c b/target-i386/translate.c > >index 8b35de1..cc59b7e 100644 > >--- a/target-i386/translate.c > >+++ b/target-i386/translate.c > >@@ -2420,7 +2420,7 @@ static void gen_pusha(DisasContext *s) > > { > > int i; > > gen_op_movl_A0_reg(R_ESP); > >- gen_op_addl_A0_im(-8 << s->dflag); > >+ gen_op_addl_A0_im(-(8 << s->dflag)); > > Better as -8U << s->d_flag? That's even more confusing to me. I wouldn't want to require other people to read the C specification to find out how many type conversions are happening in that statement. (Because I will have to do that, to find out what's the type of "-8U"). I would prefer an expression that doesn't involve any type conversion. But you are more familiar with that code, so it's up to you.
On Thu, Oct 01, 2015 at 02:06:36PM -0300, Eduardo Habkost wrote: > On Thu, Oct 01, 2015 at 11:35:52AM +1000, Richard Henderson wrote: > > On 09/30/2015 06:34 AM, Eduardo Habkost wrote: > > >Left shift of negative values is undefined behavior. Detected by clang: > > > qemu/target-i386/translate.c:2423:26: runtime error: > > > left shift of negative value -8 > > > > > >This changes the code to reverse the sign after the left shift. > > > > > >Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > >--- > > > target-i386/translate.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > >diff --git a/target-i386/translate.c b/target-i386/translate.c > > >index 8b35de1..cc59b7e 100644 > > >--- a/target-i386/translate.c > > >+++ b/target-i386/translate.c > > >@@ -2420,7 +2420,7 @@ static void gen_pusha(DisasContext *s) > > > { > > > int i; > > > gen_op_movl_A0_reg(R_ESP); > > >- gen_op_addl_A0_im(-8 << s->dflag); > > >+ gen_op_addl_A0_im(-(8 << s->dflag)); > > > > Better as -8U << s->d_flag? > > That's even more confusing to me. I wouldn't want to require other > people to read the C specification to find out how many type conversions > are happening in that statement. (Because I will have to do that, to > find out what's the type of "-8U"). > > I would prefer an expression that doesn't involve any type conversion. > But you are more familiar with that code, so it's up to you. Ping? I would really like to fix this warning to be able to enable check-mode by default in QEMU 2.5. Are there objections to get this patch included?
On 10/23/2015 05:07 AM, Eduardo Habkost wrote: > On Thu, Oct 01, 2015 at 02:06:36PM -0300, Eduardo Habkost wrote: >> On Thu, Oct 01, 2015 at 11:35:52AM +1000, Richard Henderson wrote: >>> On 09/30/2015 06:34 AM, Eduardo Habkost wrote: >>>> Left shift of negative values is undefined behavior. Detected by clang: >>>> qemu/target-i386/translate.c:2423:26: runtime error: >>>> left shift of negative value -8 >>>> >>>> This changes the code to reverse the sign after the left shift. >>>> >>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>>> --- >>>> target-i386/translate.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/target-i386/translate.c b/target-i386/translate.c >>>> index 8b35de1..cc59b7e 100644 >>>> --- a/target-i386/translate.c >>>> +++ b/target-i386/translate.c >>>> @@ -2420,7 +2420,7 @@ static void gen_pusha(DisasContext *s) >>>> { >>>> int i; >>>> gen_op_movl_A0_reg(R_ESP); >>>> - gen_op_addl_A0_im(-8 << s->dflag); >>>> + gen_op_addl_A0_im(-(8 << s->dflag)); >>> >>> Better as -8U << s->d_flag? >> >> That's even more confusing to me. I wouldn't want to require other >> people to read the C specification to find out how many type conversions >> are happening in that statement. (Because I will have to do that, to >> find out what's the type of "-8U"). >> >> I would prefer an expression that doesn't involve any type conversion. >> But you are more familiar with that code, so it's up to you. > > Ping? I would really like to fix this warning to be able to enable > check-mode by default in QEMU 2.5. Are there objections to get this > patch included? > None. r~
diff --git a/target-i386/translate.c b/target-i386/translate.c index 8b35de1..cc59b7e 100644 --- a/target-i386/translate.c +++ b/target-i386/translate.c @@ -2420,7 +2420,7 @@ static void gen_pusha(DisasContext *s) { int i; gen_op_movl_A0_reg(R_ESP); - gen_op_addl_A0_im(-8 << s->dflag); + gen_op_addl_A0_im(-(8 << s->dflag)); if (!s->ss32) tcg_gen_ext16u_tl(cpu_A0, cpu_A0); tcg_gen_mov_tl(cpu_T[1], cpu_A0);
Left shift of negative values is undefined behavior. Detected by clang: qemu/target-i386/translate.c:2423:26: runtime error: left shift of negative value -8 This changes the code to reverse the sign after the left shift. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/translate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)