| Message ID | 1403039230-3427-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Superseded |
| Headers | show |
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
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
> 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~
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
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
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
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
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~
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
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
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
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_;
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(-)