[Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Stefan Weil]

This helps detecting wrong format strings.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

This patch is not intended to be applied before fixing some potential errors.

Addings the GNU gcc format attribute results in lots of compiler errors like these ones:

  CXX   disas/libvixl/a64/disasm-a64.o
disas/libvixl/a64/disasm-a64.cc: In member function ‘int vixl::Disassembler::SubstituteImmediateField(vixl::Instruction*, const char*)’:
disas/libvixl/a64/disasm-a64.cc:1372:66: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
disas/libvixl/a64/disasm-a64.cc:1421:52: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
disas/libvixl/a64/disasm-a64.cc:1442:48: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
disas/libvixl/a64/disasm-a64.cc:1449:42: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]

I don't know the reason, because all locations seem to have arguments
which are function calls, and the called function returns Instr which
is uint32_t, not int64_t.

Stefan

 disas/libvixl/a64/disasm-a64.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/disas/libvixl/a64/disasm-a64.h b/disas/libvixl/a64/disasm-a64.h
index 3a56e15..30df187 100644
--- a/disas/libvixl/a64/disasm-a64.h
+++ b/disas/libvixl/a64/disasm-a64.h
@@ -27,6 +27,7 @@
 #ifndef VIXL_A64_DISASM_A64_H
 #define VIXL_A64_DISASM_A64_H
 
+#include "qemu/compiler.h"      // GCC_FMT_ATTR
 #include "globals.h"
 #include "utils.h"
 #include "instructions-a64.h"
@@ -85,7 +86,7 @@ class Disassembler: public DecoderVisitor {
   bool IsMovzMovnImm(unsigned reg_size, uint64_t value);
 
   void ResetOutput();
-  void AppendToOutput(const char* string, ...);
+  void GCC_FMT_ATTR(2, 3) AppendToOutput(const char* string, ...);
 
   char* buffer_;
   uint32_t buffer_pos_;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Peter Maydell]

On 17 June 2014 22:07, Stefan Weil <sw@weilnetz.de> wrote:
> This helps detecting wrong format strings.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> This patch is not intended to be applied before fixing some potential errors.
>
> Addings the GNU gcc format attribute results in lots of compiler errors like these ones:
>
>   CXX   disas/libvixl/a64/disasm-a64.o
> disas/libvixl/a64/disasm-a64.cc: In member function ‘int vixl::Disassembler::SubstituteImmediateField(vixl::Instruction*, const char*)’:
> disas/libvixl/a64/disasm-a64.cc:1372:66: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
> disas/libvixl/a64/disasm-a64.cc:1421:52: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
> disas/libvixl/a64/disasm-a64.cc:1442:48: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
> disas/libvixl/a64/disasm-a64.cc:1449:42: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
>
> I don't know the reason, because all locations seem to have arguments
> which are function calls, and the called function returns Instr which
> is uint32_t, not int64_t.

As usual, I'm sceptical about carrying local libvixl patches
unless they're really necessary.

Which platform are you building on, and what does it
define uint32_t and int64_t as? I agree that it looks to me
like the compiler's wrong here, but maybe there's an integer
promotion rule for varargs I'm unaware of that means the
uint32_t gets promoted to int64_t ?

If the format strings really are wrong we can feed that back
to upstream libvixl and get them fixed there.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Stefan Weil]

Am 18.06.2014 00:09, schrieb Peter Maydell:
> On 17 June 2014 22:07, Stefan Weil <sw@weilnetz.de> wrote:
>> This helps detecting wrong format strings.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> This patch is not intended to be applied before fixing some potential errors.
>>
>> Addings the GNU gcc format attribute results in lots of compiler errors like these ones:
>>
>>   CXX   disas/libvixl/a64/disasm-a64.o
>> disas/libvixl/a64/disasm-a64.cc: In member function ‘int vixl::Disassembler::SubstituteImmediateField(vixl::Instruction*, const char*)’:
>> disas/libvixl/a64/disasm-a64.cc:1372:66: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
>> disas/libvixl/a64/disasm-a64.cc:1421:52: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
>> disas/libvixl/a64/disasm-a64.cc:1442:48: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
>> disas/libvixl/a64/disasm-a64.cc:1449:42: error: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘int64_t {aka long int}’ [-Werror=format]
>>
>> I don't know the reason, because all locations seem to have arguments
>> which are function calls, and the called function returns Instr which
>> is uint32_t, not int64_t.
> 
> As usual, I'm sceptical about carrying local libvixl patches
> unless they're really necessary.
> 
> Which platform are you building on, and what does it
> define uint32_t and int64_t as? I agree that it looks to me
> like the compiler's wrong here, but maybe there's an integer
> promotion rule for varargs I'm unaware of that means the
> uint32_t gets promoted to int64_t ?
> 
> If the format strings really are wrong we can feed that back
> to upstream libvixl and get them fixed there.
> 
> thanks
> -- PMM
> 

The platform is Debian wheezy (64 bit) with gcc-4.7.2, but I'm afraid
any of my builds shows these errors.

Stefan

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Richard Henderson]

> I don't know the reason, because all locations seem to have arguments
> which are function calls, and the called function returns Instr which
> is uint32_t, not int64_t.
...
> +  void GCC_FMT_ATTR(2, 3) AppendToOutput(const char* string, ...);

It "helps" because 2,3 is wrong.  Correct would be 1,2 for this function.


r~

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Stefan Weil]

Am 18.06.2014 06:28, schrieb Richard Henderson:
>> I don't know the reason, because all locations seem to have arguments
>> which are function calls, and the called function returns Instr which
>> is uint32_t, not int64_t.
> ...
>> +  void GCC_FMT_ATTR(2, 3) AppendToOutput(const char* string, ...);
> 
> It "helps" because 2,3 is wrong.  Correct would be 1,2 for this function.
> 
> 
> r~
> 

No. This is a class member function, so there is an invisible fist
"this" argument which counts for the gcc format attribute.

gcc would complain if the numbering were wrong.

Stefan

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Stefan Weil]

Am 18.06.2014 06:31, schrieb Stefan Weil:
> Am 18.06.2014 06:28, schrieb Richard Henderson:
>>> I don't know the reason, because all locations seem to have arguments
>>> which are function calls, and the called function returns Instr which
>>> is uint32_t, not int64_t.
>> ...
>>> +  void GCC_FMT_ATTR(2, 3) AppendToOutput(const char* string, ...);
>>
>> It "helps" because 2,3 is wrong.  Correct would be 1,2 for this function.
>>
>>
>> r~
>>
> 
> No. This is a class member function, so there is an invisible fist

s/fist/first/

> "this" argument which counts for the gcc format attribute.
> 
> gcc would complain if the numbering were wrong.
> 
> Stefan


A 32 bit build on Ubuntu gcc-4.6.3-1ubuntu5 just finished and shows the
same error messages, so really all of my builds show them (32 and 64
bit, Linux native and cross for Windows).

Peter, I know that libvixl is external code, but posted this patch
because I need help: I simply don't know why the compiler complains and
whether these errors are really errors. It's easy to "fix" them by using
PRId64, but would that be correct?

Variable arguments usually are not converted to 64 bit values: if they
are smaller than int, they are expanded to int, and larger values are
passed as they are. But here obviously the compiler expands uint32_t to
int64_t. Why?

Stefan

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Peter Maydell]

On 18 June 2014 05:45, Stefan Weil <sw@weilnetz.de> wrote:
> A 32 bit build on Ubuntu gcc-4.6.3-1ubuntu5 just finished and shows the
> same error messages, so really all of my builds show them (32 and 64
> bit, Linux native and cross for Windows).
>
> Peter, I know that libvixl is external code, but posted this patch
> because I need help: I simply don't know why the compiler complains and
> whether these errors are really errors. It's easy to "fix" them by using
> PRId64, but would that be correct?
>
> Variable arguments usually are not converted to 64 bit values: if they
> are smaller than int, they are expanded to int, and larger values are
> passed as they are. But here obviously the compiler expands uint32_t to
> int64_t. Why?

Unfortunately I don't know the answer... I was hoping RTH did.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Paolo Bonzini]

Il 18/06/2014 06:45, Stefan Weil ha scritto:
> A 32 bit build on Ubuntu gcc-4.6.3-1ubuntu5 just finished and shows the
> same error messages, so really all of my builds show them (32 and 64
> bit, Linux native and cross for Windows).
>
> Peter, I know that libvixl is external code, but posted this patch
> because I need help: I simply don't know why the compiler complains and
> whether these errors are really errors. It's easy to "fix" them by using
> PRId64, but would that be correct?
>
> Variable arguments usually are not converted to 64 bit values: if they
> are smaller than int, they are expanded to int, and larger values are
> passed as they are. But here obviously the compiler expands uint32_t to
> int64_t. Why?

You can try synthesizing a minimal test case (either manually, or using 
the "delta" tool).  Then compile it with -fdump-tree-all and see what it 
shows.

Paolo

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Richard Henderson]

On 06/17/2014 09:45 PM, Stefan Weil wrote:
> Variable arguments usually are not converted to 64 bit values: if they
> are smaller than int, they are expanded to int, and larger values are
> passed as they are. But here obviously the compiler expands uint32_t to
> int64_t. Why?

They really really shouldn't be.

It might be worth trying something more recent than 4.6.3, and if it persists
file a bug.


r~

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Stefan Weil]

Am 18.06.2014 17:19, schrieb Richard Henderson:
>
> On 06/17/2014 09:45 PM, Stefan Weil wrote:
>>
>> Variable arguments usually are not converted to 64 bit values: if
>> they are smaller than int, they are expanded to int, and larger
>> values are passed as they are. But here obviously the compiler
>> expands uint32_t to int64_t. Why?
>
> They really really shouldn't be. It might be worth trying something
> more recent than 4.6.3, and if it persists file a bug. r~

I found the source of the problem.

The compiler is correct. Eight format strings in
disas/libvixl/a64/disasm-a64.cc are wrong.

The functions which are called don't come from
disas/libvixl/a64/assembler-a64.h as I expected.
They are generated by code in disas/libvixl/a64/instructions-a64.h:

  #define DEFINE_GETTER(Name, HighBit, LowBit, Func)             \
  inline int64_t Name() const { return Func(HighBit, LowBit); }
  INSTRUCTION_FIELDS_LIST(DEFINE_GETTER)
  #undef DEFINE_GETTER

So each of those functions really returns an int64_t which of course
should not use a "%d" format string.

Stefan

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Peter Maydell]

On 18 June 2014 18:27, Stefan Weil <sw@weilnetz.de> wrote:
> Am 18.06.2014 17:19, schrieb Richard Henderson:
>>
>> On 06/17/2014 09:45 PM, Stefan Weil wrote:
>>>
>>> Variable arguments usually are not converted to 64 bit values: if
>>> they are smaller than int, they are expanded to int, and larger
>>> values are passed as they are. But here obviously the compiler
>>> expands uint32_t to int64_t. Why?
>>
>> They really really shouldn't be. It might be worth trying something
>> more recent than 4.6.3, and if it persists file a bug. r~
>
> I found the source of the problem.
>
> The compiler is correct. Eight format strings in
> disas/libvixl/a64/disasm-a64.cc are wrong.
>
> The functions which are called don't come from
> disas/libvixl/a64/assembler-a64.h as I expected.
> They are generated by code in disas/libvixl/a64/instructions-a64.h:
>
>   #define DEFINE_GETTER(Name, HighBit, LowBit, Func)             \
>   inline int64_t Name() const { return Func(HighBit, LowBit); }
>   INSTRUCTION_FIELDS_LIST(DEFINE_GETTER)
>   #undef DEFINE_GETTER
>
> So each of those functions really returns an int64_t which of course
> should not use a "%d" format string.

Thanks. I'll forward the bug report upstream...

-- PMM

Re: [Qemu-devel] [PATCH] libvixl: Add gcc format attribute

[Peter Maydell]

On 18 June 2014 18:27, Stefan Weil <sw@weilnetz.de> wrote:
> So each of those functions really returns an int64_t which of course
> should not use a "%d" format string.

Forgot to mention, I'm happy to take a patch which fixes these
locally pending our next libvixl update.

thanks
-- PMM