Error in accel/tcg?

Error in accel/tcg?

[Kenneth Adam Miller]

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

Hello,

I get the following error:

<long cmd here> -c ../accel/tcg/cputlb.c
../qemu/accel/tcg/cputlb.c: In function 'tlb_flush_page_by_mmuidx':
../qemu/accel/tcg/cputlb.c:602:23: error: comparison is always true due to
limited range of data type [-Werror=type-limits]
    } else if (idxmap < TARGET_PAGE_SIZE) {

I don't know why that suddenly shows up. The variable idxmap is a uint16_t
and I haven't changed that at all. Also, the TARGET_PAGE_SIZE is indicated
set by cscope/global tags in a specific header, and there's no reason to
believe that the value it takes happens to be larger than a uint16_t, so I
suppose that the static tags are lacking where the compiler evaluation
would indicate correctly.

In other parts of the code, I think somehow the meson build system is
triggering errors for warnings, so things like uninitialized variables that
have their address passed so that a called function can edit them are
making. But I didn't specifically turn on any of these warnings to error
settings. So my other thought is, perhaps because a version of gcc has some
implicit initialization for variables declared without initialization. I
checked that and resolved those. But I've been stumped for a while on the
idxmap problem.

[-- Attachment #2: Type: text/html, Size: 1348 bytes --]

Re: Error in accel/tcg?

[Peter Maydell]

On Mon, 19 Jul 2021 at 23:20, Kenneth Adam Miller
<kennethadammiller@gmail.com> wrote:
>
> Hello,
>
> I get the following error:
>
> <long cmd here> -c ../accel/tcg/cputlb.c
> ../qemu/accel/tcg/cputlb.c: In function 'tlb_flush_page_by_mmuidx':
> ../qemu/accel/tcg/cputlb.c:602:23: error: comparison is always true due to limited range of data type [-Werror=type-limits]
>     } else if (idxmap < TARGET_PAGE_SIZE) {
>
> I don't know why that suddenly shows up.

So, which target are you building for, which host, and which
compiler version? (TARGET_PAGE_SIZE gets a value that depends
on the TARGET_PAGE_BITS setting for the target.)

> In other parts of the code, I think somehow the meson build system
>is triggering errors for warnings

The QEMU build system defaults to warnings-are-errors when
building from git. You can turn this off by passing
configure '--disable-werror'. (Note that that's a bad idea if
you're working on the QEMU source code, because you want to
be able to see and fix the warnings in the code you're working on.)

thanks
-- PMM

Re: Error in accel/tcg?

[Peter Maydell]

On Tue, 20 Jul 2021 at 10:06, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 19 Jul 2021 at 23:20, Kenneth Adam Miller
> <kennethadammiller@gmail.com> wrote:
> >
> > Hello,
> >
> > I get the following error:
> >
> > <long cmd here> -c ../accel/tcg/cputlb.c
> > ../qemu/accel/tcg/cputlb.c: In function 'tlb_flush_page_by_mmuidx':
> > ../qemu/accel/tcg/cputlb.c:602:23: error: comparison is always true due to limited range of data type [-Werror=type-limits]
> >     } else if (idxmap < TARGET_PAGE_SIZE) {
> >
> > I don't know why that suddenly shows up.
>
> So, which target are you building for, which host, and which
> compiler version? (TARGET_PAGE_SIZE gets a value that depends
> on the TARGET_PAGE_BITS setting for the target.)

You'll get this warning, incidentally, if you have a
target which sets TARGET_PAGE_BITS to 16 or more.
Currently the only target which does that is hexagon, and
that is linux-user only, so it doesn't run into this (yet).

The warning is harmless (apart from preventing compilation with
-Werror), but there's no in-theory reason why softmmu shouldn't
work with 64K pages, so we should figure out a way to rephrase
the cputlb.c code to suppress it.

-- PMM

Re: Error in accel/tcg?

[Kenneth Adam Miller]

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

I saw that TARGET_PAGE_SIZE gets a value that depends on TARGET_PAGE_BITS
using tags and grep, but I did not see either of them take a value that is
beyond the idxmap size.

On Tue, Jul 20, 2021 at 5:07 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 19 Jul 2021 at 23:20, Kenneth Adam Miller
> <kennethadammiller@gmail.com> wrote:
> >
> > Hello,
> >
> > I get the following error:
> >
> > <long cmd here> -c ../accel/tcg/cputlb.c
> > ../qemu/accel/tcg/cputlb.c: In function 'tlb_flush_page_by_mmuidx':
> > ../qemu/accel/tcg/cputlb.c:602:23: error: comparison is always true due
> to limited range of data type [-Werror=type-limits]
> >     } else if (idxmap < TARGET_PAGE_SIZE) {
> >
> > I don't know why that suddenly shows up.
>
> So, which target are you building for, which host, and which
> compiler version? (TARGET_PAGE_SIZE gets a value that depends
> on the TARGET_PAGE_BITS setting for the target.)
>
> > In other parts of the code, I think somehow the meson build system
> >is triggering errors for warnings
>
> The QEMU build system defaults to warnings-are-errors when
> building from git. You can turn this off by passing
> configure '--disable-werror'. (Note that that's a bad idea if
> you're working on the QEMU source code, because you want to
> be able to see and fix the warnings in the code you're working on.)
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 1895 bytes --]

Re: Error in accel/tcg?

[Peter Maydell]

On Tue, 20 Jul 2021 at 16:06, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 20 Jul 2021 at 10:06, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 19 Jul 2021 at 23:20, Kenneth Adam Miller
> > <kennethadammiller@gmail.com> wrote:
> > >
> > > Hello,
> > >
> > > I get the following error:
> > >
> > > <long cmd here> -c ../accel/tcg/cputlb.c
> > > ../qemu/accel/tcg/cputlb.c: In function 'tlb_flush_page_by_mmuidx':
> > > ../qemu/accel/tcg/cputlb.c:602:23: error: comparison is always true due to limited range of data type [-Werror=type-limits]
> > >     } else if (idxmap < TARGET_PAGE_SIZE) {
> > >
> > > I don't know why that suddenly shows up.

> You'll get this warning, incidentally, if you have a
> target which sets TARGET_PAGE_BITS to 16 or more.
> Currently the only target which does that is hexagon, and
> that is linux-user only, so it doesn't run into this (yet).
>
> The warning is harmless (apart from preventing compilation with
> -Werror), but there's no in-theory reason why softmmu shouldn't
> work with 64K pages, so we should figure out a way to rephrase
> the cputlb.c code to suppress it.

Assuming you do have something with TARGET_PAGE_BITS 16, if
you rewrite the conditions to
 "if ((uint32_t)idxmap < TARGET_PAGE_SIZE)" does that make
the compiler happier ?

-- PMM

Re: Error in accel/tcg?

[Kenneth Adam Miller]

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

See of course I thought about editing code outside of my target too, but
thought against it. And doing it that way may make the compiler happier,
but then would it be inviting a runtime error?

On Tue, Jul 20, 2021 at 11:08 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 20 Jul 2021 at 16:06, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >
> > On Tue, 20 Jul 2021 at 10:06, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> > >
> > > On Mon, 19 Jul 2021 at 23:20, Kenneth Adam Miller
> > > <kennethadammiller@gmail.com> wrote:
> > > >
> > > > Hello,
> > > >
> > > > I get the following error:
> > > >
> > > > <long cmd here> -c ../accel/tcg/cputlb.c
> > > > ../qemu/accel/tcg/cputlb.c: In function 'tlb_flush_page_by_mmuidx':
> > > > ../qemu/accel/tcg/cputlb.c:602:23: error: comparison is always true
> due to limited range of data type [-Werror=type-limits]
> > > >     } else if (idxmap < TARGET_PAGE_SIZE) {
> > > >
> > > > I don't know why that suddenly shows up.
>
> > You'll get this warning, incidentally, if you have a
> > target which sets TARGET_PAGE_BITS to 16 or more.
> > Currently the only target which does that is hexagon, and
> > that is linux-user only, so it doesn't run into this (yet).
> >
> > The warning is harmless (apart from preventing compilation with
> > -Werror), but there's no in-theory reason why softmmu shouldn't
> > work with 64K pages, so we should figure out a way to rephrase
> > the cputlb.c code to suppress it.
>
> Assuming you do have something with TARGET_PAGE_BITS 16, if
> you rewrite the conditions to
>  "if ((uint32_t)idxmap < TARGET_PAGE_SIZE)" does that make
> the compiler happier ?
>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 2464 bytes --]

Re: Error in accel/tcg?

[Peter Maydell]

On Tue, 20 Jul 2021 at 16:13, Kenneth Adam Miller
<kennethadammiller@gmail.com> wrote:
> See of course I thought about editing code outside of my target too, but thought against it.

Yeah, it's a bit of a hard one to judge. Mostly new target code shouldn't
need to result in changes to core code, but sometimes if the target is
doing something nobody else has needed before then the best approach
is to update the core code to provide the necessary facilities for it.

> And doing it that way may make the compiler happier, but then would it be inviting a runtime error

In this case what the code is testing is "can I fit a value into the low bits
of a page-aligned address?": if it can, there's a faster codepath it uses,
and it has a fallback to a slower approach otherwise. If TARGET_PAGE_BITS
is 16 or more, then it is always true that the value will fit into the
low bits, and we can always use the fastpath. The compiler is correct that
the comparison is always true, and that is OK here.

thanks
-- PMM