| Message ID | 1303633774-22961-3-git-send-email-Joakim.Tjernlund@transmode.se |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Joakim Tjernlund, In message <1303633774-22961-3-git-send-email-Joakim.Tjernlund@transmode.se> you wrote: > Noticed while building all of mpc8xx Please include in the commit message what the compiler warnings were, and which compiler version was used to produce these warnings. > diff --git a/examples/standalone/timer.c b/examples/standalone/timer.c > index 834cc9a..a4d4581 100644 > --- a/examples/standalone/timer.c > +++ b/examples/standalone/timer.c > @@ -186,7 +186,7 @@ int timer (int argc, char * const argv[]) > /* clear all events */ > *hwp->terp = (CPMT_EVENT_CAP | CPMT_EVENT_REF); > > - printf (usage); > + printf("%s", usage); I dislike this change. Which warning does the old code produce for you? Best regards, Wolfgang Denk
On Sun, Apr 24, 2011 at 6:14 PM, Wolfgang Denk wrote: > Joakim Tjernlund wrote: >> --- a/examples/standalone/timer.c >> +++ b/examples/standalone/timer.c >> @@ -186,7 +186,7 @@ int timer (int argc, char * const argv[]) >> /* clear all events */ >> *hwp->terp = (CPMT_EVENT_CAP | CPMT_EVENT_REF); >> >> - printf (usage); >> + printf("%s", usage); > > I dislike this change. Which warning does the old code produce for > you? i imagine he is using one of those "security conscious" compilers that warn when you try to printf with a dynamic argument as the format. we probably want to disable this stuff for u-boot since it doesnt make much sense by adding -Wno-format-nonliteral and -Wno-format-security when the compiler supports it. as for this one particular change, it probably makes sense to change it to puts(usage) anyways since the usage string contains no format modifiers. it'll be faster this way. and the code should be written: static const char usage[] = "..."; the current usage has useless overhead. -mike
vapierfilter@gmail.com wrote on 2011/04/25 00:38:31: > > On Sun, Apr 24, 2011 at 6:14 PM, Wolfgang Denk wrote: > > Joakim Tjernlund wrote: > >> --- a/examples/standalone/timer.c > >> +++ b/examples/standalone/timer.c > >> @@ -186,7 +186,7 @@ int timer (int argc, char * const argv[]) > >> /* clear all events */ > >> *hwp->terp = (CPMT_EVENT_CAP | CPMT_EVENT_REF); > >> > >> - printf (usage); > >> + printf("%s", usage); > > > > I dislike this change. Which warning does the old code produce for > > you? > > i imagine he is using one of those "security conscious" compilers that > warn when you try to printf with a dynamic argument as the format. we Yes, if gcc 4.4.5 counst as "security conscious" :) > probably want to disable this stuff for u-boot since it doesnt make > much sense by adding -Wno-format-nonliteral and -Wno-format-security > when the compiler supports it. > > as for this one particular change, it probably makes sense to change > it to puts(usage) anyways since the usage string contains no format > modifiers. it'll be faster this way. and the code should be written: > static const char usage[] = "..."; > > the current usage has useless overhead. Yes, but puts() adds an newline so you can't just replace the above printf with puts() Jocke
On Sun, Apr 24, 2011 at 7:42 PM, Joakim Tjernlund wrote: > vapierfilter@gmail.com wrote on 2011/04/25 00:38:31: >> On Sun, Apr 24, 2011 at 6:14 PM, Wolfgang Denk wrote: >> > Joakim Tjernlund wrote: >> >> --- a/examples/standalone/timer.c >> >> +++ b/examples/standalone/timer.c >> >> @@ -186,7 +186,7 @@ int timer (int argc, char * const argv[]) >> >> /* clear all events */ >> >> *hwp->terp = (CPMT_EVENT_CAP | CPMT_EVENT_REF); >> >> >> >> - printf (usage); >> >> + printf("%s", usage); >> > >> > I dislike this change. Which warning does the old code produce for >> > you? >> >> i imagine he is using one of those "security conscious" compilers that >> warn when you try to printf with a dynamic argument as the format. we > > Yes, if gcc 4.4.5 counst as "security conscious" :) your distro probably enables some default warning flags from the vanilla one >> probably want to disable this stuff for u-boot since it doesnt make >> much sense by adding -Wno-format-nonliteral and -Wno-format-security >> when the compiler supports it. >> >> as for this one particular change, it probably makes sense to change >> it to puts(usage) anyways since the usage string contains no format >> modifiers. it'll be faster this way. and the code should be written: >> static const char usage[] = "..."; >> >> the current usage has useless overhead. > > Yes, but puts() adds an newline so you can't just replace the above printf > with puts() no, it doesnt. u-boot's put() doesnt act the same as the standard C library. however, that doesnt change my original point ... we shouldnt be "fixing" things like this that have no relevance in the u-boot world. disable the warning flags in the build system. -mike
vapierfilter@gmail.com wrote on 2011/04/25 06:13:20: > > On Sun, Apr 24, 2011 at 7:42 PM, Joakim Tjernlund wrote: > > vapierfilter@gmail.com wrote on 2011/04/25 00:38:31: > >> On Sun, Apr 24, 2011 at 6:14 PM, Wolfgang Denk wrote: > >> > Joakim Tjernlund wrote: > >> >> --- a/examples/standalone/timer.c > >> >> +++ b/examples/standalone/timer.c > >> >> @@ -186,7 +186,7 @@ int timer (int argc, char * const argv[]) > >> >> /* clear all events */ > >> >> *hwp->terp = (CPMT_EVENT_CAP | CPMT_EVENT_REF); > >> >> > >> >> - printf (usage); > >> >> + printf("%s", usage); > >> > > >> > I dislike this change. Which warning does the old code produce for > >> > you? > >> > >> i imagine he is using one of those "security conscious" compilers that > >> warn when you try to printf with a dynamic argument as the format. we > > > > Yes, if gcc 4.4.5 counst as "security conscious" :) > > your distro probably enables some default warning flags from the vanilla one > > >> probably want to disable this stuff for u-boot since it doesnt make > >> much sense by adding -Wno-format-nonliteral and -Wno-format-security > >> when the compiler supports it. > >> > >> as for this one particular change, it probably makes sense to change > >> it to puts(usage) anyways since the usage string contains no format > >> modifiers. it'll be faster this way. and the code should be written: > >> static const char usage[] = "..."; > >> > >> the current usage has useless overhead. > > > > Yes, but puts() adds an newline so you can't just replace the above printf > > with puts() > > no, it doesnt. u-boot's put() doesnt act the same as the standard C library. Ah, didn't know that. I am not sure I like that especially when there is a fputs in u-boot too. Jocke
On Mon, Apr 25, 2011 at 04:30, Joakim Tjernlund wrote: > vapierfilter@gmail.com wrote on 2011/04/25 06:13:20: >> On Sun, Apr 24, 2011 at 7:42 PM, Joakim Tjernlund wrote: >> > Yes, but puts() adds an newline so you can't just replace the above printf >> > with puts() >> >> no, it doesnt. u-boot's put() doesnt act the same as the standard C library. > > Ah, didn't know that. I am not sure I like that especially > when there is a fputs in u-boot too. cant say i'm a big fan of it either, but i think it's way too buried in the fabric of u-boot to be worth trying to change now. and it is slightly more flexible than the C library puts() in that you can puts() multiple partial strings without worrying about the newline interjection ... -mike
On Mon, 25 Apr 2011 00:13:20 -0400 Mike Frysinger <vapier@gentoo.org> wrote: > On Sun, Apr 24, 2011 at 7:42 PM, Joakim Tjernlund wrote: > > vapierfilter@gmail.com wrote on 2011/04/25 00:38:31: > >> probably want to disable this stuff for u-boot since it doesnt make > >> much sense by adding -Wno-format-nonliteral and -Wno-format-security > >> when the compiler supports it. > >> > >> as for this one particular change, it probably makes sense to change > >> it to puts(usage) anyways since the usage string contains no format > >> modifiers. it'll be faster this way. and the code should be written: > >> static const char usage[] = "..."; > >> > >> the current usage has useless overhead. > > > > Yes, but puts() adds an newline so you can't just replace the above printf > > with puts() > > no, it doesnt. u-boot's put() doesnt act the same as the standard C library. > > however, that doesnt change my original point ... we shouldnt be > "fixing" things like this that have no relevance in the u-boot world. > disable the warning flags in the build system. Why encourage bad habits? Are there any instances of this in U-Boot where conversion to puts() wouldn't be an improvement, especially given the lack of an automatic newline in U-Boot's version? If we have an instance where we really want formatting on a dynamic string, that's another matter. -Scott
On Mon, Apr 25, 2011 at 13:45, Scott Wood wrote: > On Mon, 25 Apr 2011 00:13:20 -0400 Mike Frysinger wrote: >> On Sun, Apr 24, 2011 at 7:42 PM, Joakim Tjernlund wrote: >> > vapierfilter@gmail.com wrote on 2011/04/25 00:38:31: >> >> probably want to disable this stuff for u-boot since it doesnt make >> >> much sense by adding -Wno-format-nonliteral and -Wno-format-security >> >> when the compiler supports it. >> >> >> >> as for this one particular change, it probably makes sense to change >> >> it to puts(usage) anyways since the usage string contains no format >> >> modifiers. it'll be faster this way. and the code should be written: >> >> static const char usage[] = "..."; >> >> >> >> the current usage has useless overhead. >> > >> > Yes, but puts() adds an newline so you can't just replace the above printf >> > with puts() >> >> no, it doesnt. u-boot's put() doesnt act the same as the standard C library. >> >> however, that doesnt change my original point ... we shouldnt be >> "fixing" things like this that have no relevance in the u-boot world. >> disable the warning flags in the build system. > > Why encourage bad habits? Are there any instances of this in U-Boot where > conversion to puts() wouldn't be an improvement, especially given the lack > of an automatic newline in U-Boot's version? that wasnt what i was saying. my point is simply that changing printf(foo); to printf("%s", foo); simply to satisfy a gcc warning is wrong and unnecessarily bloats the compiled code. if you want to change it from printf(foo) to puts(foo), that's fine by me (and is actually what i suggested). -mike
vapierfilter@gmail.com wrote on 2011/04/25 19:53:50: > > On Mon, Apr 25, 2011 at 13:45, Scott Wood wrote: > > On Mon, 25 Apr 2011 00:13:20 -0400 Mike Frysinger wrote: > >> On Sun, Apr 24, 2011 at 7:42 PM, Joakim Tjernlund wrote: > >> > vapierfilter@gmail.com wrote on 2011/04/25 00:38:31: > >> >> probably want to disable this stuff for u-boot since it doesnt make > >> >> much sense by adding -Wno-format-nonliteral and -Wno-format-security > >> >> when the compiler supports it. > >> >> > >> >> as for this one particular change, it probably makes sense to change > >> >> it to puts(usage) anyways since the usage string contains no format > >> >> modifiers. it'll be faster this way. and the code should be written: > >> >> static const char usage[] = "..."; > >> >> > >> >> the current usage has useless overhead. > >> > > >> > Yes, but puts() adds an newline so you can't just replace the above printf > >> > with puts() > >> > >> no, it doesnt. u-boot's put() doesnt act the same as the standard C library. > >> > >> however, that doesnt change my original point ... we shouldnt be > >> "fixing" things like this that have no relevance in the u-boot world. > >> disable the warning flags in the build system. > > > > Why encourage bad habits? Are there any instances of this in U-Boot where > > conversion to puts() wouldn't be an improvement, especially given the lack > > of an automatic newline in U-Boot's version? > > that wasnt what i was saying. my point is simply that changing > printf(foo); to printf("%s", foo); simply to satisfy a gcc warning is > wrong and unnecessarily bloats the compiled code. if you want to > change it from printf(foo) to puts(foo), that's fine by me (and is > actually what i suggested). > -mike Which is what I did in the second version I sent in a few hours ago :) Jocke
On Mon, Apr 25, 2011 at 13:58, Joakim Tjernlund wrote: > vapierfilter@gmail.com wrote on 2011/04/25 19:53:50: >> On Mon, Apr 25, 2011 at 13:45, Scott Wood wrote: >> > On Mon, 25 Apr 2011 00:13:20 -0400 Mike Frysinger wrote: >> >> On Sun, Apr 24, 2011 at 7:42 PM, Joakim Tjernlund wrote: >> >> > vapierfilter@gmail.com wrote on 2011/04/25 00:38:31: >> >> >> probably want to disable this stuff for u-boot since it doesnt make >> >> >> much sense by adding -Wno-format-nonliteral and -Wno-format-security >> >> >> when the compiler supports it. >> >> >> >> >> >> as for this one particular change, it probably makes sense to change >> >> >> it to puts(usage) anyways since the usage string contains no format >> >> >> modifiers. it'll be faster this way. and the code should be written: >> >> >> static const char usage[] = "..."; >> >> >> >> >> >> the current usage has useless overhead. >> >> > >> >> > Yes, but puts() adds an newline so you can't just replace the above printf >> >> > with puts() >> >> >> >> no, it doesnt. u-boot's put() doesnt act the same as the standard C library. >> >> >> >> however, that doesnt change my original point ... we shouldnt be >> >> "fixing" things like this that have no relevance in the u-boot world. >> >> disable the warning flags in the build system. >> > >> > Why encourage bad habits? Are there any instances of this in U-Boot where >> > conversion to puts() wouldn't be an improvement, especially given the lack >> > of an automatic newline in U-Boot's version? >> >> that wasnt what i was saying. my point is simply that changing >> printf(foo); to printf("%s", foo); simply to satisfy a gcc warning is >> wrong and unnecessarily bloats the compiled code. if you want to >> change it from printf(foo) to puts(foo), that's fine by me (and is >> actually what i suggested). > > Which is what I did in the second version I sent in a few hours ago :) yes, but this response was just for Scott -mike
On Mon, 25 Apr 2011 13:53:50 -0400 Mike Frysinger <vapier@gentoo.org> wrote: > On Mon, Apr 25, 2011 at 13:45, Scott Wood wrote: > > Why encourage bad habits? Are there any instances of this in U-Boot where > > conversion to puts() wouldn't be an improvement, especially given the lack > > of an automatic newline in U-Boot's version? > > that wasnt what i was saying. my point is simply that changing > printf(foo); to printf("%s", foo); simply to satisfy a gcc warning is > wrong and unnecessarily bloats the compiled code. My point was that the warning should stay, until such a time as it's complaining about something that we actually want to do -- I've yet to see an example cited so far that didn't have an easy non-"bloating" fix. Even in a context such as U-Boot, IMHO format-string functions shouldn't be used in such a way -- it may not be a security issue, but it's a potential readability/maintainability issue when it may not be obvious from the other context where the string is defined, that any embedded percent characters must be doubled. It's a different situation from where a string literal is defined right in the context of the format-string function. > if you want to > change it from printf(foo) to puts(foo), that's fine by me (and is > actually what i suggested). And after writing this, you sent a patch changing the warning options... -Scott
On Mon, Apr 25, 2011 at 15:45, Scott Wood wrote:
> And after writing this, you sent a patch changing the warning options...
i sent a proper patch to do a suggestion i already made here. and it
isnt really changing anything ... most toolchains already have this
behavior by default. my patch only changes it for people who are
using non-standard toolchains, and they are clearly the minority as
the majority of people writing & submitting patches arent seeing the
warnings.
-mike
Dear Scott Wood, In message <20110425144518.5a37bd4c@schlenkerla.am.freescale.net> you wrote: > > Mike Frysinger <vapier@gentoo.org> wrote: ... > > that wasnt what i was saying. my point is simply that changing > > printf(foo); to printf("%s", foo); simply to satisfy a gcc warning is > > wrong and unnecessarily bloats the compiled code. > > My point was that the warning should stay, until such a time as it's > complaining about something that we actually want to do -- I've yet to see > an example cited so far that didn't have an easy non-"bloating" fix. I disagree. "printf(foo);" may be suboptimal but there are cases where I do not want to see a warning about this. Consider for example common/main.c: 115 # ifdef CONFIG_AUTOBOOT_PROMPT 116 printf(CONFIG_AUTOBOOT_PROMPT); 117 # endif Here we provide a way for a user-defined autoboot prompt message. Some users may just want to provide a plain string - what's wrong with that? [Yes, there are other ways to implement this, but why make it more complicated than necessary?] > Even in a context such as U-Boot, IMHO format-string functions shouldn't be > used in such a way -- it may not be a security issue, but it's a potential > readability/maintainability issue when it may not be obvious from the other > context where the string is defined, that any embedded percent characters > must be doubled. It's a different situation from where a string literal is > defined right in the context of the format-string function. Again, I disagree. Best regards, Wolfgang Denk
On Mon, 25 Apr 2011 23:23:41 +0200 Wolfgang Denk <wd@denx.de> wrote: > Dear Scott Wood, > > In message <20110425144518.5a37bd4c@schlenkerla.am.freescale.net> you wrote: > > > > Mike Frysinger <vapier@gentoo.org> wrote: > ... > > > that wasnt what i was saying. my point is simply that changing > > > printf(foo); to printf("%s", foo); simply to satisfy a gcc warning is > > > wrong and unnecessarily bloats the compiled code. > > > > My point was that the warning should stay, until such a time as it's > > complaining about something that we actually want to do -- I've yet to see > > an example cited so far that didn't have an easy non-"bloating" fix. > > I disagree. "printf(foo);" may be suboptimal but there are cases > where I do not want to see a warning about this. Consider for example > common/main.c: > > 115 # ifdef CONFIG_AUTOBOOT_PROMPT > 116 printf(CONFIG_AUTOBOOT_PROMPT); > 117 # endif > > Here we provide a way for a user-defined autoboot prompt message. Some > users may just want to provide a plain string - what's wrong with > that? [Yes, there are other ways to implement this, but why make it > more complicated than necessary?] It won't warn there, because it all happens in the preprocessor. -Scott
Dear Scott Wood, In message <20110425162854.05500a06@schlenkerla.am.freescale.net> you wrote: > > > I disagree. "printf(foo);" may be suboptimal but there are cases > > where I do not want to see a warning about this. Consider for example > > common/main.c: > > > > 115 # ifdef CONFIG_AUTOBOOT_PROMPT > > 116 printf(CONFIG_AUTOBOOT_PROMPT); > > 117 # endif > > > > Here we provide a way for a user-defined autoboot prompt message. Some > > users may just want to provide a plain string - what's wrong with > > that? [Yes, there are other ways to implement this, but why make it > > more complicated than necessary?] > > It won't warn there, because it all happens in the preprocessor. Why would it not warn if the user just does #define CONFIG_AUTOBOOT_PROMPT "my prompt:" Best regards, Wolfgang Denk
On Tue, 26 Apr 2011 00:29:06 +0200 Wolfgang Denk <wd@denx.de> wrote: > Dear Scott Wood, > > In message <20110425162854.05500a06@schlenkerla.am.freescale.net> you wrote: > > > > > I disagree. "printf(foo);" may be suboptimal but there are cases > > > where I do not want to see a warning about this. Consider for example > > > common/main.c: > > > > > > 115 # ifdef CONFIG_AUTOBOOT_PROMPT > > > 116 printf(CONFIG_AUTOBOOT_PROMPT); > > > 117 # endif > > > > > > Here we provide a way for a user-defined autoboot prompt message. Some > > > users may just want to provide a plain string - what's wrong with > > > that? [Yes, there are other ways to implement this, but why make it > > > more complicated than necessary?] > > > > It won't warn there, because it all happens in the preprocessor. > > Why would it not warn if the user just does > > #define CONFIG_AUTOBOOT_PROMPT "my prompt:" Because the warning only applies to non-literals (preprocessor substitution doesn't count, as it's a literal when the preprocessor's done with it). Otherwise it would be warning on every instance of printf("foo\n"). -Scott
diff --git a/board/netta/codec.c b/board/netta/codec.c index 844aa18..c8d31d7 100644 --- a/board/netta/codec.c +++ b/board/netta/codec.c @@ -502,7 +502,7 @@ void codsp_write_sop_short(int duslic_id, int channel, unsigned char regno, unsi void codsp_write_sop_int(int duslic_id, int channel, unsigned char regno, unsigned int val) { - unsigned char cmd[5]; + unsigned char cmd[6]; cmd[0] = CODSP_WR | CODSP_ADR(channel) | CODSP_CMD_SOP; cmd[1] = regno; @@ -577,7 +577,7 @@ void codsp_write_cop_char(int duslic_id, int channel, unsigned char addr, unsign void codsp_write_cop_short(int duslic_id, int channel, unsigned char addr, unsigned short val) { - unsigned char cmd[3]; + unsigned char cmd[4]; cmd[0] = CODSP_WR | CODSP_OP | CODSP_ADR(channel) | CODSP_CMD_COP; cmd[1] = addr; @@ -668,7 +668,7 @@ void codsp_write_pop_short (int duslic_id, int channel, unsigned char regno, void codsp_write_pop_int (int duslic_id, int channel, unsigned char regno, unsigned int val) { - unsigned char cmd[5]; + unsigned char cmd[6]; cmd[0] = CODSP_WR | CODSP_ADR (channel) | CODSP_CMD_POP; cmd[1] = regno; diff --git a/board/siemens/IAD210/atm.c b/board/siemens/IAD210/atm.c index e599c10..40aad0a 100644 --- a/board/siemens/IAD210/atm.c +++ b/board/siemens/IAD210/atm.c @@ -62,7 +62,7 @@ int atmLoad() volatile iop8xx_t *iop = &immap->im_ioport; timers->cpmt_tgcr &= 0x0FFF; SYNC; /* Disable Timer 4 */ - immap->im_cpm.cp_scc[4].scc_gsmrl = 0x0; SYNC; /* Disable SCC4 */ + immap->im_cpm.cp_scc[3].scc_gsmrl = 0x0; SYNC; /* Disable SCC4 */ iop->iop_pdpar &= 0x3FFF; SYNC; /* Disable SAR and UTOPIA */ if ( atmMemInit() != OK ) return ERROR; @@ -96,7 +96,7 @@ void atmUnload() volatile iop8xx_t *iop = &immap->im_ioport; timers->cpmt_tgcr &= 0x0FFF; SYNC; /* Disable Timer 4 */ - immap->im_cpm.cp_scc[4].scc_gsmrl = 0x0; SYNC; /* Disable SCC4 */ + immap->im_cpm.cp_scc[3].scc_gsmrl = 0x0; SYNC; /* Disable SCC4 */ iop->iop_pdpar &= 0x3FFF; SYNC; /* Disable SAR and UTOPIA */ g_atm.loaded = FALSE; } diff --git a/examples/standalone/timer.c b/examples/standalone/timer.c index 834cc9a..a4d4581 100644 --- a/examples/standalone/timer.c +++ b/examples/standalone/timer.c @@ -186,7 +186,7 @@ int timer (int argc, char * const argv[]) /* clear all events */ *hwp->terp = (CPMT_EVENT_CAP | CPMT_EVENT_REF); - printf (usage); + printf("%s", usage); running = 0; while ((c = getc()) != 'q') { if (c == 'b') { @@ -255,7 +255,7 @@ int timer (int argc, char * const argv[]) } else { printf ("\nEnter: q - quit, b - start timer, e - stop timer, ? - get status\n"); } - printf (usage); + printf("%s", usage); } if (running) { printf ("Stopping timer\n");
Noticed while building all of mpc8xx Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se> --- board/netta/codec.c | 6 +++--- board/siemens/IAD210/atm.c | 4 ++-- examples/standalone/timer.c | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-)