| Message ID | 1351558398-6902-1-git-send-email-trini@ti.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
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
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
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
-----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-----
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
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.
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;
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(-)