Patchwork [U-Boot] cmd_fdt.c: Use %p when printing pointers

login
register
mail settings
Submitter Tom Rini
Date Oct. 30, 2012, 12:53 a.m.
Message ID <1351558398-6902-1-git-send-email-trini@ti.com>
Download mbox | patch
Permalink /patch/195205/
State Accepted
Delegated to: Tom Rini
Headers show

Comments

Tom Rini - Oct. 30, 2012, 12:53 a.m.
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(-)
Joe Hershberger - Oct. 30, 2012, 1:36 a.m.
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
Wolfgang Denk - Oct. 30, 2012, 9:59 a.m.
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
Joe Hershberger - Oct. 30, 2012, 5:48 p.m.
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
Tom Rini - Oct. 30, 2012, 6:10 p.m.
-----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-----
Wolfgang Denk - Oct. 30, 2012, 7:04 p.m.
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
Tom Rini - Nov. 4, 2012, 6:28 p.m.
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.

Patch

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;