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

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

[Tom Rini]

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

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

[Joe Hershberger]

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

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

[Wolfgang Denk]

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!_

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

[Joe Hershberger]

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

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

[Tom Rini]

-----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-----

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

[Wolfgang Denk]

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

[U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null

[Wolfgang Denk]

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

[U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null

[Joe Hershberger]

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>

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

[Tom Rini]

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>

[U-Boot] [PATCH] lib/vsprintf.c: don't special-case pointers to address null

[Tom Rini]

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>