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

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