| Message ID | 20160616185639.5312-1-bobby.prani@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 06/16/2016 11:56 AM, Pranith Kumar wrote: > Using gcc 6.1 for alpha-linux-user target we see the following build > error: > > /mnt/devops/code/qemu/target-alpha/translate.c: In function ‘in_superpage’: > /mnt/devops/code/qemu/target-alpha/translate.c:454:52: error: self-comparison always evaluates to true [-Werror=tautological-compare] > && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); > > Fix it by replacing (addr >> 63) by '1' which is what it evaluates to > since addr is negative. > > Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> > --- > target-alpha/translate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target-alpha/translate.c b/target-alpha/translate.c > index f9b2426..31da6ea 100644 > --- a/target-alpha/translate.c > +++ b/target-alpha/translate.c > @@ -451,7 +451,7 @@ static bool in_superpage(DisasContext *ctx, int64_t addr) > return ((ctx->tb->flags & TB_FLAGS_USER_MODE) == 0 > && addr < 0 > && ((addr >> 41) & 3) == 2 > - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); > + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); > } This fix is incorrect. (1) addr is not always negative. (2) in_superpage is only relevant for alpha-softmmu, where TARGET_VIRT_ADDR_SPACE_BITS is not 63. r~
On Thu, Jun 16, 2016 at 3:07 PM, Richard Henderson <rth@twiddle.net> wrote: > On 06/16/2016 11:56 AM, Pranith Kumar wrote: >> Using gcc 6.1 for alpha-linux-user target we see the following build >> error: >> >> /mnt/devops/code/qemu/target-alpha/translate.c: In function ‘in_superpage’: >> /mnt/devops/code/qemu/target-alpha/translate.c:454:52: error: self-comparison always evaluates to true [-Werror=tautological-compare] >> && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >> >> Fix it by replacing (addr >> 63) by '1' which is what it evaluates to >> since addr is negative. >> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> --- >> target-alpha/translate.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target-alpha/translate.c b/target-alpha/translate.c >> index f9b2426..31da6ea 100644 >> --- a/target-alpha/translate.c >> +++ b/target-alpha/translate.c >> @@ -451,7 +451,7 @@ static bool in_superpage(DisasContext *ctx, int64_t addr) >> return ((ctx->tb->flags & TB_FLAGS_USER_MODE) == 0 >> && addr < 0 >> && ((addr >> 41) & 3) == 2 >> - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >> + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); >> } > > This fix is incorrect. > > (1) addr is not always negative. > (2) in_superpage is only relevant for alpha-softmmu, where > TARGET_VIRT_ADDR_SPACE_BITS is not 63. If you see line 2 of the condition you check for (addr < 0). Only if this evaluates to true do you check the right shift value of addr. Reg. (2), I think that is what gcc is erroring out for. TARGET_VIRT_ADDR_SPACE_BITS is 63 for linux-user and we are comparing (addr >> 63) with itself. GCC rightly says that it is a tautological compare and errors out. May be we can have a better work around. Thanks!
Le 16/06/2016 à 21:15, Pranith Kumar a écrit : > On Thu, Jun 16, 2016 at 3:07 PM, Richard Henderson <rth@twiddle.net> wrote: >> On 06/16/2016 11:56 AM, Pranith Kumar wrote: >>> Using gcc 6.1 for alpha-linux-user target we see the following build >>> error: >>> >>> /mnt/devops/code/qemu/target-alpha/translate.c: In function ‘in_superpage’: >>> /mnt/devops/code/qemu/target-alpha/translate.c:454:52: error: self-comparison always evaluates to true [-Werror=tautological-compare] >>> && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >>> >>> Fix it by replacing (addr >> 63) by '1' which is what it evaluates to >>> since addr is negative. >>> >>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >>> --- >>> target-alpha/translate.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/target-alpha/translate.c b/target-alpha/translate.c >>> index f9b2426..31da6ea 100644 >>> --- a/target-alpha/translate.c >>> +++ b/target-alpha/translate.c >>> @@ -451,7 +451,7 @@ static bool in_superpage(DisasContext *ctx, int64_t addr) >>> return ((ctx->tb->flags & TB_FLAGS_USER_MODE) == 0 >>> && addr < 0 >>> && ((addr >> 41) & 3) == 2 >>> - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >>> + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); >>> } >> >> This fix is incorrect. >> >> (1) addr is not always negative. >> (2) in_superpage is only relevant for alpha-softmmu, where >> TARGET_VIRT_ADDR_SPACE_BITS is not 63. > > If you see line 2 of the condition you check for (addr < 0). Only if > this evaluates to true do you check the right shift value of addr. > > Reg. (2), I think that is what gcc is erroring out for. > TARGET_VIRT_ADDR_SPACE_BITS is 63 for linux-user and we are comparing > (addr >> 63) with itself. GCC rightly says that it is a tautological > compare and errors out. May be we can have a better work around. Perhaps something like: #ifndef CONFIG_USER_ONLY static bool in_superpage(DisasContext *ctx, int64_t addr) { ... } #endif ... #ifdef CONFIG_USER_ONLY pc_mask = ~TARGET_PAGE_MASK; #else if (in_superpage(&ctx, pc_start)) { pc_mask = (1ULL << 41) - 1; } else { pc_mask = ~TARGET_PAGE_MASK; } #endif Laurent
On Thu, Jun 16, 2016 at 8:43 PM, Laurent Vivier <laurent@vivier.eu> wrote: > > > Le 16/06/2016 à 21:15, Pranith Kumar a écrit : >> On Thu, Jun 16, 2016 at 3:07 PM, Richard Henderson <rth@twiddle.net> wrote: >>> On 06/16/2016 11:56 AM, Pranith Kumar wrote: >>>> Using gcc 6.1 for alpha-linux-user target we see the following build >>>> error: >>>> >>>> /mnt/devops/code/qemu/target-alpha/translate.c: In function ‘in_superpage’: >>>> /mnt/devops/code/qemu/target-alpha/translate.c:454:52: error: self-comparison always evaluates to true [-Werror=tautological-compare] >>>> && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >>>> >>>> Fix it by replacing (addr >> 63) by '1' which is what it evaluates to >>>> since addr is negative. >>>> >>>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >>>> --- >>>> target-alpha/translate.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/target-alpha/translate.c b/target-alpha/translate.c >>>> index f9b2426..31da6ea 100644 >>>> --- a/target-alpha/translate.c >>>> +++ b/target-alpha/translate.c >>>> @@ -451,7 +451,7 @@ static bool in_superpage(DisasContext *ctx, int64_t addr) >>>> return ((ctx->tb->flags & TB_FLAGS_USER_MODE) == 0 >>>> && addr < 0 >>>> && ((addr >> 41) & 3) == 2 >>>> - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >>>> + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); >>>> } >>> >>> This fix is incorrect. >>> >>> (1) addr is not always negative. >>> (2) in_superpage is only relevant for alpha-softmmu, where >>> TARGET_VIRT_ADDR_SPACE_BITS is not 63. >> >> If you see line 2 of the condition you check for (addr < 0). Only if >> this evaluates to true do you check the right shift value of addr. >> >> Reg. (2), I think that is what gcc is erroring out for. >> TARGET_VIRT_ADDR_SPACE_BITS is 63 for linux-user and we are comparing >> (addr >> 63) with itself. GCC rightly says that it is a tautological >> compare and errors out. May be we can have a better work around. > > Perhaps something like: > > #ifndef CONFIG_USER_ONLY > static bool in_superpage(DisasContext *ctx, int64_t addr) > { > ... > } > #endif > ... > > #ifdef CONFIG_USER_ONLY > pc_mask = ~TARGET_PAGE_MASK; > #else > if (in_superpage(&ctx, pc_start)) { > pc_mask = (1ULL << 41) - 1; > } else { > pc_mask = ~TARGET_PAGE_MASK; > } > #endif > OK, I will use this. Thanks!
On 16/06/2016 21:07, Richard Henderson wrote: >> && ((addr >> 41) & 3) == 2 >> - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >> + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); What you want here is + addr >> TARGET_VIRT_ADDR_SPACE_BITS == -1 since that's what addr >> 63 is. With this change the patch should be fine. Alternatively, combining this condition with addr < 0 should be (uint64_t)addr >= (-1ull << TARGET_VIRT_ADDR_SPACE_BITS) I think (I might have an off by one here, sorry). If you go this way, it's of course nicer to change the argument type to uint64_t. Paolo
On Fri, Jun 17, 2016 at 2:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 16/06/2016 21:07, Richard Henderson wrote: >>> && ((addr >> 41) & 3) == 2 >>> - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >>> + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); > > What you want here is > > + addr >> TARGET_VIRT_ADDR_SPACE_BITS == -1 > > since that's what addr >> 63 is. With this change the patch should be fine. Isn't (addr >> 63) supposed to be 1? How can it be -1?
On 06/17/2016 11:07 AM, Pranith Kumar wrote: > On Fri, Jun 17, 2016 at 2:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 16/06/2016 21:07, Richard Henderson wrote: >>>> && ((addr >> 41) & 3) == 2 >>>> - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >>>> + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); >> >> What you want here is >> >> + addr >> TARGET_VIRT_ADDR_SPACE_BITS == -1 >> >> since that's what addr >> 63 is. With this change the patch should be fine. > > Isn't (addr >> 63) supposed to be 1? How can it be -1? Signed right shift. I'll prepare a patch for all of this. r~
On Fri, Jun 17, 2016 at 2:09 PM, Richard Henderson <rth@twiddle.net> wrote: > On 06/17/2016 11:07 AM, Pranith Kumar wrote: >> On Fri, Jun 17, 2016 at 2:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> >>> On 16/06/2016 21:07, Richard Henderson wrote: >>>>> && ((addr >> 41) & 3) == 2 >>>>> - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); >>>>> + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); >>> >>> What you want here is >>> >>> + addr >> TARGET_VIRT_ADDR_SPACE_BITS == -1 >>> >>> since that's what addr >> 63 is. With this change the patch should be fine. >> >> Isn't (addr >> 63) supposed to be 1? How can it be -1? > > Signed right shift. Ah, got it. :) > > I'll prepare a patch for all of this. Thanks!
diff --git a/target-alpha/translate.c b/target-alpha/translate.c index f9b2426..31da6ea 100644 --- a/target-alpha/translate.c +++ b/target-alpha/translate.c @@ -451,7 +451,7 @@ static bool in_superpage(DisasContext *ctx, int64_t addr) return ((ctx->tb->flags & TB_FLAGS_USER_MODE) == 0 && addr < 0 && ((addr >> 41) & 3) == 2 - && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); + && addr >> TARGET_VIRT_ADDR_SPACE_BITS == 1); } static bool use_goto_tb(DisasContext *ctx, uint64_t dest)
Using gcc 6.1 for alpha-linux-user target we see the following build error: /mnt/devops/code/qemu/target-alpha/translate.c: In function ‘in_superpage’: /mnt/devops/code/qemu/target-alpha/translate.c:454:52: error: self-comparison always evaluates to true [-Werror=tautological-compare] && addr >> TARGET_VIRT_ADDR_SPACE_BITS == addr >> 63); Fix it by replacing (addr >> 63) by '1' which is what it evaluates to since addr is negative. Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> --- target-alpha/translate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)