diff mbox

util/cacheinfo: Fix warning generated by clang

Message ID 20170630153946.11997-1-bobby.prani@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pranith Kumar June 30, 2017, 3:39 p.m. UTC
Clang generates the following warning on aarch64 host:

  CC      util/cacheinfo.o
/home/pranith/qemu/util/cacheinfo.c:121:48: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
        asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
                                               ^
/home/pranith/qemu/util/cacheinfo.c:121:28: note: use constraint modifier "w"
        asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
                           ^~
                           %w0

Constraint modifier 'w' is not (yet?) accepted by gcc. Fix this by increasing the ctr size.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 util/cacheinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Emilio Cota June 30, 2017, 9:15 p.m. UTC | #1
On Fri, Jun 30, 2017 at 11:39:46 -0400, Pranith Kumar wrote:
> Clang generates the following warning on aarch64 host:
> 
>   CC      util/cacheinfo.o
> /home/pranith/qemu/util/cacheinfo.c:121:48: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
>         asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
>                                                ^
> /home/pranith/qemu/util/cacheinfo.c:121:28: note: use constraint modifier "w"
>         asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
>                            ^~
>                            %w0
> 
> Constraint modifier 'w' is not (yet?) accepted by gcc. Fix this by increasing the ctr size.
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>

I can reproduce with clang 3.9.1.

Tested-by: Emilio G. Cota <cota@braap.org>
Reviewed-by: Emilio G. Cota <cota@braap.org>

Thanks,

		Emilio
Richard Henderson July 1, 2017, 10:20 p.m. UTC | #2
On 06/30/2017 08:39 AM, Pranith Kumar wrote:
> Clang generates the following warning on aarch64 host:
> 
>    CC      util/cacheinfo.o
> /home/pranith/qemu/util/cacheinfo.c:121:48: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths]
>          asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
>                                                 ^
> /home/pranith/qemu/util/cacheinfo.c:121:28: note: use constraint modifier "w"
>          asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
>                             ^~
>                             %w0

That is an absolutely stupid warning.  There's long precedent for the compiler 
choosing the prefix for you based on the type of the argument.

> 
> Constraint modifier 'w' is not (yet?) accepted by gcc. Fix this by increasing the ctr size.

Certainly it is -- since the beginning of time.


r~
Peter Maydell July 1, 2017, 10:30 p.m. UTC | #3
On 1 July 2017 at 23:20, Richard Henderson <rth@twiddle.net> wrote:
> On 06/30/2017 08:39 AM, Pranith Kumar wrote:
>>
>> Clang generates the following warning on aarch64 host:
>>
>>    CC      util/cacheinfo.o
>> /home/pranith/qemu/util/cacheinfo.c:121:48: warning: value size does not
>> match register size specified by the constraint and modifier
>> [-Wasm-operand-widths]
>>          asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
>>                                                 ^
>> /home/pranith/qemu/util/cacheinfo.c:121:28: note: use constraint modifier
>> "w"
>>          asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
>>                             ^~
>>                             %w0
>
>
> That is an absolutely stupid warning.  There's long precedent for the
> compiler choosing the prefix for you based on the type of the argument.

Isn't that the problem? The type of the argument says "32 bits"
but the instruction here really wants 64 bits (MRS takes Xn, not Wn).

thanks
-- PMM
Richard Henderson July 1, 2017, 10:35 p.m. UTC | #4
On 07/01/2017 03:30 PM, Peter Maydell wrote:
> On 1 July 2017 at 23:20, Richard Henderson <rth@twiddle.net> wrote:
>> On 06/30/2017 08:39 AM, Pranith Kumar wrote:
>>>
>>> Clang generates the following warning on aarch64 host:
>>>
>>>     CC      util/cacheinfo.o
>>> /home/pranith/qemu/util/cacheinfo.c:121:48: warning: value size does not
>>> match register size specified by the constraint and modifier
>>> [-Wasm-operand-widths]
>>>           asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
>>>                                                  ^
>>> /home/pranith/qemu/util/cacheinfo.c:121:28: note: use constraint modifier
>>> "w"
>>>           asm volatile("mrs\t%0, ctr_el0" : "=r"(ctr));
>>>                              ^~
>>>                              %w0
>>
>>
>> That is an absolutely stupid warning.  There's long precedent for the
>> compiler choosing the prefix for you based on the type of the argument.
> 
> Isn't that the problem? The type of the argument says "32 bits"
> but the instruction here really wants 64 bits (MRS takes Xn, not Wn).

The warning is telling me to use %w to force Wn.  So if the assembler really 
doesn't like Wn, the warning is a bit more than confusing.

Perhaps it ought to be telling me to use %x to force Xn in spite of the type?


r~
Peter Maydell July 1, 2017, 10:44 p.m. UTC | #5
On 1 July 2017 at 23:35, Richard Henderson <rth@twiddle.net> wrote:
> On 07/01/2017 03:30 PM, Peter Maydell wrote:
>>
>> On 1 July 2017 at 23:20, Richard Henderson <rth@twiddle.net> wrote:
>>> That is an absolutely stupid warning.  There's long precedent for the
>>> compiler choosing the prefix for you based on the type of the argument.
>>
>>
>> Isn't that the problem? The type of the argument says "32 bits"
>> but the instruction here really wants 64 bits (MRS takes Xn, not Wn).
>
>
> The warning is telling me to use %w to force Wn.  So if the assembler really
> doesn't like Wn, the warning is a bit more than confusing.

Wouldn't be the first time a compiler has produced a confusing warning :-)

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63359 includes some
previous gcc-vs-clang-dev discussion on the topic of the warning.
It looks like the clang dev rationale is that having %0 always
generate a 64-bit register access even when passed a 32-bit value
is confusing (eg people expect "str %0, [addr]" : ... : "r" (var_32bits)"
to do a 32 bit store, not a 64 bit store), so better to warn and
nudge the code author into being explicit about the size they wanted.

> Perhaps it ought to be telling me to use %x to force Xn in spite of the
> type?

You always get Xn anyway, regardless of the type.

For us, I think the right thing to do is make 'ctr' be a uint64_t,
because we're reading a 64 bit sysreg and silently truncating it
as a side effect of the asm constraints is a bit obscure.

thanks
-- PMM
Richard Henderson July 3, 2017, 9:16 p.m. UTC | #6
On 07/01/2017 03:44 PM, Peter Maydell wrote:
> On 1 July 2017 at 23:35, Richard Henderson <rth@twiddle.net> wrote:
>> Perhaps it ought to be telling me to use %x to force Xn in spite of the
>> type?
> 
> You always get Xn anyway, regardless of the type.
> 
> For us, I think the right thing to do is make 'ctr' be a uint64_t,
> because we're reading a 64 bit sysreg and silently truncating it
> as a side effect of the asm constraints is a bit obscure.

Fair enough.  Applied as-is to tcg-next.


r~
diff mbox

Patch

diff --git a/util/cacheinfo.c b/util/cacheinfo.c
index f987522df4..6253049533 100644
--- a/util/cacheinfo.c
+++ b/util/cacheinfo.c
@@ -112,7 +112,7 @@  static void sys_cache_info(int *isize, int *dsize)
 static void arch_cache_info(int *isize, int *dsize)
 {
     if (*isize == 0 || *dsize == 0) {
-        unsigned ctr;
+        unsigned long ctr;
 
         /* The real cache geometry is in CCSIDR_EL1/CLIDR_EL1/CSSELR_EL1,
            but (at least under Linux) these are marked protected by the