* [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers
@ 2012-10-30 0:53 Tom Rini
2012-10-30 1:36 ` Joe Hershberger
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Tom Rini @ 2012-10-30 0:53 UTC (permalink / raw)
To: u-boot
When putting pointers into a format string use %p to ensure that they
are printed correctly regardless of bitsize. This fixes warnings on
sandbox on 64bit systems.
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: Gerald Van Baren <vanbaren@cideas.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
common/cmd_fdt.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c
index a5e2cfc..f9acfc1 100644
--- a/common/cmd_fdt.c
+++ b/common/cmd_fdt.c
@@ -375,7 +375,7 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
/* Get address */
char buf[11];
- sprintf(buf, "0x%08X", (uint32_t)nodep);
+ sprintf(buf, "0x%p", nodep);
setenv(var, buf);
} else if (subcmd[0] == 's') {
/* Get size */
@@ -816,7 +816,7 @@ static void print_data(const void *data, int len)
if ((len %4) == 0) {
if (len > CONFIG_CMD_FDT_MAX_DUMP)
- printf("* 0x%08x [0x%08x]", (unsigned int)data, len);
+ printf("* 0x%p [0x%08x]", data, len);
else {
const u32 *p;
@@ -828,7 +828,7 @@ static void print_data(const void *data, int len)
}
} else { /* anything else... hexdump */
if (len > CONFIG_CMD_FDT_MAX_DUMP)
- printf("* 0x%08x [0x%08x]", (unsigned int)data, len);
+ printf("* 0x%p [0x%08x]", data, len);
else {
const u8 *s;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread* [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers 2012-10-30 0:53 [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers Tom Rini @ 2012-10-30 1:36 ` Joe Hershberger 2012-10-30 9:59 ` Wolfgang Denk 2012-11-04 18:28 ` [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers Tom Rini 2 siblings, 0 replies; 10+ messages in thread From: Joe Hershberger @ 2012-10-30 1:36 UTC (permalink / raw) To: u-boot Hi Tom, On Mon, Oct 29, 2012 at 7:53 PM, Tom Rini <trini@ti.com> wrote: > When putting pointers into a format string use %p to ensure that they > are printed correctly regardless of bitsize. This fixes warnings on > sandbox on 64bit systems. > > Cc: Joe Hershberger <joe.hershberger@ni.com> > Cc: Gerald Van Baren <vanbaren@cideas.com> > Signed-off-by: Tom Rini <trini@ti.com> > --- Acked-by: Joe Hershberger <joe.hershberger@ni.com> Thanks! -Joe ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers 2012-10-30 0:53 [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers Tom Rini 2012-10-30 1:36 ` Joe Hershberger @ 2012-10-30 9:59 ` Wolfgang Denk 2012-10-30 17:48 ` Joe Hershberger ` (2 more replies) 2012-11-04 18:28 ` [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers Tom Rini 2 siblings, 3 replies; 10+ messages in thread From: Wolfgang Denk @ 2012-10-30 9:59 UTC (permalink / raw) To: u-boot Dear Tom Rini, In message <1351558398-6902-1-git-send-email-trini@ti.com> you wrote: > When putting pointers into a format string use %p to ensure that they > are printed correctly regardless of bitsize. This fixes warnings on > sandbox on 64bit systems. > > Cc: Joe Hershberger <joe.hershberger@ni.com> > Cc: Gerald Van Baren <vanbaren@cideas.com> > Signed-off-by: Tom Rini <trini@ti.com> > --- > common/cmd_fdt.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c > index a5e2cfc..f9acfc1 100644 > --- a/common/cmd_fdt.c > +++ b/common/cmd_fdt.c > @@ -375,7 +375,7 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[]) > /* Get address */ > char buf[11]; > > - sprintf(buf, "0x%08X", (uint32_t)nodep); > + sprintf(buf, "0x%p", nodep); > setenv(var, buf); This may put bogus data ("var=0x(null)") into the environment. I see two approaches to fix this: 1) Handle this locally, say like that: char buf[11] = { '0', 0, }; if (nodep) sprintf(buf, "0x%p", nodep); setenv(var, buf); This is the solution with minimal global impact. 2) Fix the root cause: given that we have valid situations where we may want to dereference a pointer pointing to address 0x0000, I wonder if it would not actually be better (i. e. more correct) to get rid of the 495 static char *pointer(const char *fmt, char *buf, char *end, void *ptr, 496 int field_width, int precision, int flags) 497 { 498 if (!ptr) 499 return string(buf, end, "(null)", field_width, precision, 500 flags); special handling in "lib/vsprintf.c" Would anybody shed any tears if we drop this? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de You could end up being oddly sad and full of a strange, diffuse com- passion which would lead you to believe that it might be a good idea to wipe out the whole human race and start again with amoebas. - Terry Pratchett, _Guards! Guards!_ ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers 2012-10-30 9:59 ` Wolfgang Denk @ 2012-10-30 17:48 ` Joe Hershberger 2012-10-30 19:04 ` Wolfgang Denk [not found] ` <508FCBBA.1070200@cideas.com> 2012-10-30 19:19 ` [U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null Wolfgang Denk 2 siblings, 1 reply; 10+ messages in thread From: Joe Hershberger @ 2012-10-30 17:48 UTC (permalink / raw) To: u-boot Hi Wolfgang, On Tue, Oct 30, 2012 at 4:59 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Tom Rini, > > In message <1351558398-6902-1-git-send-email-trini@ti.com> you wrote: >> When putting pointers into a format string use %p to ensure that they >> are printed correctly regardless of bitsize. This fixes warnings on >> sandbox on 64bit systems. >> >> Cc: Joe Hershberger <joe.hershberger@ni.com> >> Cc: Gerald Van Baren <vanbaren@cideas.com> >> Signed-off-by: Tom Rini <trini@ti.com> >> --- >> common/cmd_fdt.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c >> index a5e2cfc..f9acfc1 100644 >> --- a/common/cmd_fdt.c >> +++ b/common/cmd_fdt.c >> @@ -375,7 +375,7 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[]) >> /* Get address */ >> char buf[11]; >> >> - sprintf(buf, "0x%08X", (uint32_t)nodep); >> + sprintf(buf, "0x%p", nodep); >> setenv(var, buf); > > This may put bogus data ("var=0x(null)") into the environment. > > I see two approaches to fix this: > > 1) Handle this locally, say like that: > > char buf[11] = { '0', 0, }; > > if (nodep) > sprintf(buf, "0x%p", nodep); > > setenv(var, buf); > > This is the solution with minimal global impact. I think this solution is not needed. In this particular case, we are always printing the pointer to a member inside the fdt, so even if the image is at 0, no pointer that we are printing will ever be at 0. Therefore this is code that will never run and can be left out. > 2) Fix the root cause: given that we have valid situations where we > may want to dereference a pointer pointing to address 0x0000, > I wonder if it would not actually be better (i. e. more correct) to > get rid of the > > 495 static char *pointer(const char *fmt, char *buf, char *end, void *ptr, > 496 int field_width, int precision, int flags) > 497 { > 498 if (!ptr) > 499 return string(buf, end, "(null)", field_width, precision, > 500 flags); > > special handling in "lib/vsprintf.c" > > Would anybody shed any tears if we drop this? Getting rid of this would be good in general IMO. I never did understand why printing "(null)" was better than "0". -Joe ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers 2012-10-30 17:48 ` Joe Hershberger @ 2012-10-30 19:04 ` Wolfgang Denk 0 siblings, 0 replies; 10+ messages in thread From: Wolfgang Denk @ 2012-10-30 19:04 UTC (permalink / raw) To: u-boot Dear Joe, In message <CANr=Z=bxwwKkpUa_UzegN=E=TuqzeNUDTtZe7fDC+iCU9B6+3g@mail.gmail.com> you wrote: > > > 1) Handle this locally, say like that: ... > I think this solution is not needed. In this particular case, we are > always printing the pointer to a member inside the fdt, so even if the > image is at 0, no pointer that we are printing will ever be at 0. > Therefore this is code that will never run and can be left out. If we would decide for this variant, such reasoning should be explained in a comment. > > Would anybody shed any tears if we drop this? > > Getting rid of this would be good in general IMO. I never did > understand why printing "(null)" was better than "0". I guess for the same reasons we are forced^W encouraged to write NULL instead of 0 . In the standard C library it certainly makes sense to note specifically if one tries to dereference a NULL pointer, because this is aways a bug. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de I have never understood the female capacity to avoid a direct answer to any question. -- Spock, "This Side of Paradise", stardate 3417.3 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <508FCBBA.1070200@cideas.com>]
* [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers [not found] ` <508FCBBA.1070200@cideas.com> @ 2012-10-30 18:10 ` Tom Rini 0 siblings, 0 replies; 10+ messages in thread From: Tom Rini @ 2012-10-30 18:10 UTC (permalink / raw) To: u-boot -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 10/30/12 05:44, Jerry Van Baren wrote: > On 10/30/2012 05:59 AM, Wolfgang Denk wrote: >> Dear Tom Rini, [snip] >> 2) Fix the root cause: given that we have valid situations where >> we may want to dereference a pointer pointing to address 0x0000, >> I wonder if it would not actually be better (i. e. more >> correct) to get rid of the >> >> 495 static char *pointer(const char *fmt, char *buf, char *end, >> void *ptr, 496 int field_width, int precision, >> int flags) 497 { 498 if (!ptr) 499 >> return string(buf, end, "(null)", field_width, precision, 500 >> flags); >> >> special handling in "lib/vsprintf.c" >> >> Would anybody shed any tears if we drop this? > > I like that a lot better, gets my vote. I would suggest replacing > the code with a comment that the (lack of) code intentionally does > not print "(null)" or someone may put it back in in the future. > :-/ Yes, a comment as a reminder is a good idea. Please make it so. And as Joe notes (he pointed this out on IRC before I posted) we'll never do this in the cmd_fdt.c code as it's always inside of a tree somewhere. Thanks! - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQIcBAEBAgAGBQJQkBguAAoJENk4IS6UOR1WiKcP/RSFDevD6jDW8cV6TH1VbE0u v+2ZMClL/DVcwTq+EasUyFStCJJgBZRjgpTva0S5/VPevC/Q7TAQSuOSQjhizTlO 6LlUNY3Qmrznd/70WO1uXGkZ9YxNqHwfbmjH3V67Ge+E6xM9QtIqsj4rI+TvEWub NiFlynwi/DQRzO1RuCzuoey+nIPO8Vmy2L8HbT09s24B85IfLGkCPpf40wsZY9p8 pPf5qTBjaKuzxRax02msVEllpCC0l1AiDzdjiOjrNDkk+ULaHehzB/8kT/q3Bg7k Q8j2msa/gDSCfqGfl4ravQ+5ekpwbW22ywhfL6DmFzw8tlPYlzLJs7x34GyFx6Me /5RSAqpiq+qw9qc5NQKPNEgd9FyPC6QH6q9DG0Ns0NoMwnWbLe590bBuDNitsmwH tfdBJspVGJnY0Lcf8nGl6xKw12Q2H4uOqofaI3MrtUIW2bUB3RHB421wZAWwuTez EcuvF1CqcBcbf17wOksftg+51N5e4ng9L35mMCAv8pLuheEvhgRKLcvZZVHhXGX6 QqG7DIBR+lVAflFQU5ctNVOESknuzPOt4UveAZnuXyKPH5KH5k+kN5HpKghtC4aF j/AWcqZaNXsFwuObgRbAk2O3eSnWzLODQL61C2u9gmEjbv9PIZ3c30UsX3g3QY8v CxnWPsbwsFDoo/OmSCvj =vTzN -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null 2012-10-30 9:59 ` Wolfgang Denk 2012-10-30 17:48 ` Joe Hershberger [not found] ` <508FCBBA.1070200@cideas.com> @ 2012-10-30 19:19 ` Wolfgang Denk 2012-10-30 19:23 ` Joe Hershberger 2 siblings, 1 reply; 10+ messages in thread From: Wolfgang Denk @ 2012-10-30 19:19 UTC (permalink / raw) To: u-boot The %p format of printf() would print a pointer to address null as "(null)". This makes sense in a real OS where a NULL pointer must never be dereferenced, but this is a bootloader, and there are cases where accessing the data at address null makes perfect sense. Remove the special case in lib/vsprintf.c using "#if 0" with a comment to make clear this was an intentional change and to stop re-adding this code. Signed-off-by: Wolfgang Denk <wd@denx.de> --- lib/vsprintf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index d762763..dd13bca 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -495,9 +495,15 @@ static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int field_width, int precision, int flags) { + /* + * Being a boot loader, we explicitly allow pointers to + * (physical) address null. + */ +#if 0 if (!ptr) return string(buf, end, "(null)", field_width, precision, flags); +#endif #ifdef CONFIG_CMD_NET switch (*fmt) { -- 1.7.11.7 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null 2012-10-30 19:19 ` [U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null Wolfgang Denk @ 2012-10-30 19:23 ` Joe Hershberger 2012-11-04 18:28 ` Tom Rini 0 siblings, 1 reply; 10+ messages in thread From: Joe Hershberger @ 2012-10-30 19:23 UTC (permalink / raw) To: u-boot Hi Wolfgang, On Tue, Oct 30, 2012 at 2:19 PM, Wolfgang Denk <wd@denx.de> wrote: > The %p format of printf() would print a pointer to address null as > "(null)". This makes sense in a real OS where a NULL pointer must > never be dereferenced, but this is a bootloader, and there are cases > where accessing the data at address null makes perfect sense. > > Remove the special case in lib/vsprintf.c using "#if 0" with a comment > to make clear this was an intentional change and to stop re-adding > this code. > > Signed-off-by: Wolfgang Denk <wd@denx.de> > --- Acked-by: Joe Hershberger <joe.hershberger@ni.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null 2012-10-30 19:23 ` Joe Hershberger @ 2012-11-04 18:28 ` Tom Rini 0 siblings, 0 replies; 10+ messages in thread From: Tom Rini @ 2012-11-04 18:28 UTC (permalink / raw) To: u-boot On Tue, Oct 30, 2012 at 02:23:17PM -0500, Joe Hershberger wrote: > Hi Wolfgang, > > On Tue, Oct 30, 2012 at 2:19 PM, Wolfgang Denk <wd@denx.de> wrote: > > The %p format of printf() would print a pointer to address null as > > "(null)". This makes sense in a real OS where a NULL pointer must > > never be dereferenced, but this is a bootloader, and there are cases > > where accessing the data at address null makes perfect sense. > > > > Remove the special case in lib/vsprintf.c using "#if 0" with a comment > > to make clear this was an intentional change and to stop re-adding > > this code. > > > > Signed-off-by: Wolfgang Denk <wd@denx.de> > > --- > > Acked-by: Joe Hershberger <joe.hershberger@ni.com> Applied to u-boot/master, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121104/2636ff12/attachment.pgp> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers 2012-10-30 0:53 [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers Tom Rini 2012-10-30 1:36 ` Joe Hershberger 2012-10-30 9:59 ` Wolfgang Denk @ 2012-11-04 18:28 ` Tom Rini 2 siblings, 0 replies; 10+ messages in thread From: Tom Rini @ 2012-11-04 18:28 UTC (permalink / raw) To: u-boot On Mon, Oct 29, 2012 at 05:53:18PM -0700, Tom Rini wrote: > When putting pointers into a format string use %p to ensure that they > are printed correctly regardless of bitsize. This fixes warnings on > sandbox on 64bit systems. > > Cc: Joe Hershberger <joe.hershberger@ni.com> > Cc: Gerald Van Baren <vanbaren@cideas.com> > Signed-off-by: Tom Rini <trini@ti.com> Applied to u-boot/master. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121104/9cce852a/attachment.pgp> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-11-04 18:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-30 0:53 [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers Tom Rini
2012-10-30 1:36 ` Joe Hershberger
2012-10-30 9:59 ` Wolfgang Denk
2012-10-30 17:48 ` Joe Hershberger
2012-10-30 19:04 ` Wolfgang Denk
[not found] ` <508FCBBA.1070200@cideas.com>
2012-10-30 18:10 ` Tom Rini
2012-10-30 19:19 ` [U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null Wolfgang Denk
2012-10-30 19:23 ` Joe Hershberger
2012-11-04 18:28 ` Tom Rini
2012-11-04 18:28 ` [U-Boot] [PATCH] cmd_fdt.c: Use %p when printing pointers Tom Rini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox