* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.