[PATCH] kernel: Fix compiler format string warnings when some debugging options are enabled
Signed-off-by: Glenn Washburn development@efficientek.com --- This compile-time errors from GCC 10.1.0 when the config options CONFIG_DEBUG_BOOT, CONFIG_DEBUG_DICTIONARY, and CONFIG_DEBUG_INTERPRETER are set to true in the ppc64_config.xml. I suspect these are issues for 32-bit PPC as well, but haven't checked.
Glenn
--- kernel/bootstrap.c | 8 ++++---- kernel/dict.c | 4 ++-- kernel/forth.c | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/kernel/bootstrap.c b/kernel/bootstrap.c index b7658ab..6834597 100644 --- a/kernel/bootstrap.c +++ b/kernel/bootstrap.c @@ -133,9 +133,9 @@ static void relocation_table(unsigned char * dict_one, unsigned char *dict_two, }
#ifdef CONFIG_DEBUG_DICTIONARY - printk("dict1 %lx dict2 %lx dict %lx\n",dict_one, dict_two, dict); + printk("dict1 %p dict2 %p dict %p\n",dict_one, dict_two, dict); for (i=0; i< relocation_length ; i++) - printk("reloc %d %lx\n",i+1, reloc_table[i]); + printk("reloc %d "FMT_ucellx"\n",i+1, reloc_table[i]); #endif relocation_address=reloc_table; } @@ -819,7 +819,7 @@ static int build_dictionary(void) dicthead = 0;
#ifdef CONFIG_DEBUG_DICTIONARY - printk("building dictionary, %d primitives.\nbuilt words:", + printk("building dictionary, %lu primitives.\nbuilt words:", sizeof(wordnames) / sizeof(void *)); #endif
@@ -1248,7 +1248,7 @@ int main(int argc, char *argv[]) printf("dict1: %p\n",bootstrapdict[0]); printf("dict2: %p\n",bootstrapdict[1]); printf("trampoline: %p\n",trampoline); - printf("size=%d, trampoline_size=%d\n",MEMORY_SIZE + (2 * + printf("size=%ld, trampoline_size=%ld\n",MEMORY_SIZE + (2 * DICTIONARY_SIZE) + TRAMPOLINE_SIZE, TRAMPOLINE_SIZE); #endif diff --git a/kernel/dict.c b/kernel/dict.c index 0986cb1..e449c5b 100644 --- a/kernel/dict.c +++ b/kernel/dict.c @@ -293,8 +293,8 @@ ucell load_dictionary(const char *data, ucell len) reloc_table=(ucell *)(data+dicthead);
#ifdef CONFIG_DEBUG_DICTIONARY - printk("\nmoving dictionary (%x bytes) to %x\n", - (ucell)dicthead, (ucell)dict); + printk("\nmoving dictionary (%x bytes) to "FMT_ucellx"\n", + (ucell)dicthead, pointer2cell(dict)); printk("\ndynamic relocation..."); #endif
diff --git a/kernel/forth.c b/kernel/forth.c index 61dd70d..ca3fda3 100644 --- a/kernel/forth.c +++ b/kernel/forth.c @@ -111,7 +111,7 @@ static void docol(void) PUSHR(PC); PC = read_ucell(cell2pointer(PC));
- dbg_interp_printk("docol: %s\n", cell2pointer( lfa2nfa(PC - sizeof(cell)) )); + dbg_interp_printk("docol: %s\n", (char *)cell2pointer( lfa2nfa(PC - sizeof(cell)) )); }
static void semis(void) @@ -719,7 +719,7 @@ static void docol_dbg(void) debug_xt_item = debug_xt_item->next; }
- dbg_interp_printk("docol_dbg: %s\n", cell2pointer(lfa2nfa(PC - sizeof(cell)))); + dbg_interp_printk("docol_dbg: %s\n", (char *)cell2pointer(lfa2nfa(PC - sizeof(cell)))); }
static void semis_dbg(void)
Hi!
On Tue, Jan 25, 2022 at 04:27:50PM -0600, Glenn Washburn wrote:
- printk("dict1 %lx dict2 %lx dict %lx\n",dict_one, dict_two, dict);
- printk("dict1 %p dict2 %p dict %p\n",dict_one, dict_two, dict);
This is not identical output (it prefixes 0x to non-0). The better solution (it is needed in many other cases!) is to cast the argument:
printk("dict1 %lx dict2 %lx dict %lx\n", (long)dict_one, (long)dict_two, (long)dict);
printk("reloc %d %lx\n",i+1, reloc_table[i]);
printk("reloc %d "FMT_ucellx"\n",i+1, reloc_table[i]);
Same here.
- printk("building dictionary, %d primitives.\nbuilt words:",
- printk("building dictionary, %lu primitives.\nbuilt words:", sizeof(wordnames) / sizeof(void *));
This is incorrect: sizeof produces a size_t, not a long. So:
printk("building dictionary, %lu primitives.\nbuilt words:", (long)(sizeof wordnames / sizeof wordnames[0]));
- printf("size=%d, trampoline_size=%d\n",MEMORY_SIZE + (2 *
- printf("size=%ld, trampoline_size=%ld\n",MEMORY_SIZE + (2 * DICTIONARY_SIZE) + TRAMPOLINE_SIZE, TRAMPOLINE_SIZE);
This needs a cast as well.
Etc.
All stdarg functions always need explicit casts (if the argument type isn't correct already). It is a good idea to always do it if the arg isn't obviously a compatible type (or a signed or unsigned variant of it). That way you don't have to think so much, and any reader of your code doesn't either! :-)
Segher
On Tue, 25 Jan 2022 17:03:03 -0600 Segher Boessenkool segher@kernel.crashing.org wrote:
Hi!
Hi Segher,
Thanks for the feedback. I forgot about this, and came back wondering why this patch wasn't applied.
On Tue, Jan 25, 2022 at 04:27:50PM -0600, Glenn Washburn wrote:
- printk("dict1 %lx dict2 %lx dict %lx\n",dict_one, dict_two, dict);
- printk("dict1 %p dict2 %p dict %p\n",dict_one, dict_two, dict);
This is not identical output (it prefixes 0x to non-0). The better solution (it is needed in many other cases!) is to cast the argument:
printk("dict1 %lx dict2 %lx dict %lx\n", (long)dict_one, (long)dict_two, (long)dict);
Yes, not identical, but does it matter here? This is a debug print statement, not something that a program should be relying on the output format. These are pointers, so the %p format seems, if anything, more correct.
printk("reloc %d %lx\n",i+1, reloc_table[i]);
printk("reloc %d "FMT_ucellx"\n",i+1, reloc_table[i]);Same here.
I'm not seeing why this is the same as above. The FMT_ucellx macro's purpose is to spit out the correct format code for printing a ucell type in the hex display variant. So if typeof(reloc_table[i]) is a ucell, then it should be used.
- printk("building dictionary, %d primitives.\nbuilt words:",
- printk("building dictionary, %lu primitives.\nbuilt words:", sizeof(wordnames) / sizeof(void *));
This is incorrect: sizeof produces a size_t, not a long. So:
printk("building dictionary, %lu primitives.\nbuilt words:", (long)(sizeof wordnames / sizeof wordnames[0]));
Should be cast to (unsigned long), right? Of course, if the sizeof is returning something large enough to make a difference, then there are probably other issues, but still for correctness sake.
- printf("size=%d, trampoline_size=%d\n",MEMORY_SIZE + (2 *
- printf("size=%ld, trampoline_size=%ld\n",MEMORY_SIZE + (2 * DICTIONARY_SIZE) + TRAMPOLINE_SIZE, TRAMPOLINE_SIZE);
This needs a cast as well.
Etc.
All stdarg functions always need explicit casts (if the argument type isn't correct already). It is a good idea to always do it if the arg isn't obviously a compatible type (or a signed or unsigned variant of it). That way you don't have to think so much, and any reader of your code doesn't either! :-)
Point taken. I like to avoid casting by using the appropriate format code, but I don't think that invalidates the soundness of the above.
Glenn
Segher
Hi!
On Fri, Jul 28, 2023 at 01:28:20PM -0500, Glenn Washburn wrote:
On Tue, 25 Jan 2022 17:03:03 -0600 Segher Boessenkool segher@kernel.crashing.org wrote:
On Tue, Jan 25, 2022 at 04:27:50PM -0600, Glenn Washburn wrote:
- printk("dict1 %lx dict2 %lx dict %lx\n",dict_one, dict_two, dict);
- printk("dict1 %p dict2 %p dict %p\n",dict_one, dict_two, dict);
This is not identical output (it prefixes 0x to non-0). The better solution (it is needed in many other cases!) is to cast the argument:
printk("dict1 %lx dict2 %lx dict %lx\n", (long)dict_one, (long)dict_two, (long)dict);
Yes, not identical, but does it matter here? This is a debug print statement, not something that a program should be relying on the output format. These are pointers, so the %p format seems, if anything, more correct.
It may not matter, sure. But your patch message should then say it changed this output. Silent changes are bad.
printk("reloc %d %lx\n",i+1, reloc_table[i]);
printk("reloc %d "FMT_ucellx"\n",i+1, reloc_table[i]);Same here.
I'm not seeing why this is the same as above. The FMT_ucellx macro's purpose is to spit out the correct format code for printing a ucell type in the hex display variant. So if typeof(reloc_table[i]) is a ucell, then it should be used.
It is so much easier to read (as well as write!) to just do a cast where you pass the arg :-)
- printk("building dictionary, %d primitives.\nbuilt words:",
- printk("building dictionary, %lu primitives.\nbuilt words:", sizeof(wordnames) / sizeof(void *));
This is incorrect: sizeof produces a size_t, not a long. So:
printk("building dictionary, %lu primitives.\nbuilt words:", (long)(sizeof wordnames / sizeof wordnames[0]));
Should be cast to (unsigned long), right?
Either is correct here, as required by the C standard.
Of course, if the sizeof is returning something large enough to make a difference, then there are probably other issues, but still for correctness sake.
No, "long" and "unsigned long" can be used interchangeably in varargs. You are correct that it may be easier to understand the longer form :-)
All stdarg functions always need explicit casts (if the argument type isn't correct already). It is a good idea to always do it if the arg isn't obviously a compatible type (or a signed or unsigned variant of it). That way you don't have to think so much, and any reader of your code doesn't either! :-)
Point taken. I like to avoid casting by using the appropriate format code, but I don't think that invalidates the soundness of the above.
Yeah, if and when the actual type passed is clear, just write that in the conversion specifier. My point is that very often it is way easier and way less error-prone to just do a cast!
Segher
On Fri, 28 Jul 2023 15:08:59 -0500 Segher Boessenkool segher@kernel.crashing.org wrote:
Hi!
On Fri, Jul 28, 2023 at 01:28:20PM -0500, Glenn Washburn wrote:
On Tue, 25 Jan 2022 17:03:03 -0600 Segher Boessenkool segher@kernel.crashing.org wrote:
On Tue, Jan 25, 2022 at 04:27:50PM -0600, Glenn Washburn wrote:
- printk("dict1 %lx dict2 %lx dict %lx\n",dict_one, dict_two, dict);
- printk("dict1 %p dict2 %p dict %p\n",dict_one, dict_two, dict);
This is not identical output (it prefixes 0x to non-0). The better solution (it is needed in many other cases!) is to cast the argument:
printk("dict1 %lx dict2 %lx dict %lx\n", (long)dict_one, (long)dict_two, (long)dict);
Yes, not identical, but does it matter here? This is a debug print statement, not something that a program should be relying on the output format. These are pointers, so the %p format seems, if anything, more correct.
It may not matter, sure. But your patch message should then say it changed this output. Silent changes are bad.
printk("reloc %d %lx\n",i+1, reloc_table[i]);
printk("reloc %d "FMT_ucellx"\n",i+1, reloc_table[i]);Same here.
I'm not seeing why this is the same as above. The FMT_ucellx macro's purpose is to spit out the correct format code for printing a ucell type in the hex display variant. So if typeof(reloc_table[i]) is a ucell, then it should be used.
It is so much easier to read (as well as write!) to just do a cast where you pass the arg :-)
I'm I wrong in understanding that you're against the concept of FMT_ucellx? It seems like there's no need for it if you're casting. One of the purposes, is to abstract away type size differences on different architectures. But from your point of view, wouldn't you just cast to the largest size and use the format code for that size?
Glenn
- printk("building dictionary, %d primitives.\nbuilt words:",
- printk("building dictionary, %lu primitives.\nbuilt words:", sizeof(wordnames) / sizeof(void *));
This is incorrect: sizeof produces a size_t, not a long. So:
printk("building dictionary, %lu primitives.\nbuilt words:", (long)(sizeof wordnames / sizeof wordnames[0]));
Should be cast to (unsigned long), right?
Either is correct here, as required by the C standard.
Of course, if the sizeof is returning something large enough to make a difference, then there are probably other issues, but still for correctness sake.
No, "long" and "unsigned long" can be used interchangeably in varargs. You are correct that it may be easier to understand the longer form :-)
All stdarg functions always need explicit casts (if the argument type isn't correct already). It is a good idea to always do it if the arg isn't obviously a compatible type (or a signed or unsigned variant of it). That way you don't have to think so much, and any reader of your code doesn't either! :-)
Point taken. I like to avoid casting by using the appropriate format code, but I don't think that invalidates the soundness of the above.
Yeah, if and when the actual type passed is clear, just write that in the conversion specifier. My point is that very often it is way easier and way less error-prone to just do a cast!
Segher
-
Glenn Washburn -
Segher Boessenkool