[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    RFR(XXS): 7133111: libsaproc debug print should be printed as unsigned long to fit large numbers on
From:       serguei.spitsyn () oracle ! com (serguei ! spitsyn at oracle ! com)
Date:       2012-04-23 17:08:39
Message-ID: 4F958C97.7040803 () oracle ! com
[Download RAW message or body]

Looks good.

Thanks,
Serguei

On 4/23/12 2:05 AM, Staffan Larsen wrote:
> Can I get a Review for this tiny change, please.
> 
> Thanks,
> /Staffan
> 
> On 18 apr 2012, at 13:54, Rickard B?ckman wrote:
> 
> > Staffan,
> > 
> > I think the change looks good.
> > 
> > /R
> > 
> > On Apr 17, 2012, at 11:38 AM, Staffan Larsen wrote:
> > 
> > > David,
> > > 
> > > You are right, thanks for catching this. The correct format specifier is %zu \
> > > for a size_t. I've created a new bug 7162063 for this change since I had \
> > > already submitted the incorrect change. 
> > > Please review the diff below.
> > > 
> > > Thanks,
> > > /Staffan
> > > 
> > > 
> > > --- a/agent/src/os/linux/ps_core.c
> > > +++ b/agent/src/os/linux/ps_core.c
> > > @@ -440,7 +440,7 @@
> > > int j = 0;
> > > print_debug("---- sorted virtual address map ----\n");
> > > for (j = 0; j<  ph->core->num_maps; j++) {
> > > -        print_debug("base = 0x%lx\tsize = %zd\n", \
> > > ph->core->map_array[j]->vaddr, +        print_debug("base = 0x%lx\tsize = \
> > > %zu\n", ph->core->map_array[j]->vaddr, ph->core->map_array[j]->memsz);
> > > }
> > > }
> > > 
> > > On 16 apr 2012, at 07:08, David Holmes wrote:
> > > 
> > > > On 5/04/2012 10:25 PM, Staffan Larsen wrote:
> > > > > Please review the following one-character fix to a printf format string. A \
> > > > > 'z' is added to the printout of a size_t field.
> > > > Sorry I'm late to the party and this code already shipped. The z length \
> > > > modifier is not linux specific but was added as part of the C99 standard. Is \
> > > > it also a gcc extension enabled by default (I don't think we run in C99 mode \
> > > > by default) ? 
> > > > But z simply changes the following d/i/o/u/x/X to indicate it refers to a \
> > > > size_t - which is somewhat confusing as size_t is unsigned, so does %zd print \
> > > > a signed or unsigned representation? If signed then the bug still exists for \
> > > > really large numbers. 
> > > > Note the same bug exists in the BSD version of the code.
> > > > 
> > > > I agree with Dan that the reference to "unsigned long" in the synopsis is \
> > > > very confusing - please change the synopsis to reflect the actual problem \
> > > > e.g: "debug print should format size_t correctly". 
> > > > Thanks,
> > > > David
> > > > 
> > > > > Thanks,
> > > > > /Staffan
> > > > > 
> > > > > 
> > > > > diff --git a/agent/src/os/linux/ps_core.c b/agent/src/os/linux/ps_core.c
> > > > > --- a/agent/src/os/linux/ps_core.c
> > > > > +++ b/agent/src/os/linux/ps_core.c
> > > > > @@ -440,7 +440,7 @@
> > > > > int j = 0;
> > > > > print_debug("---- sorted virtual address map ----\n");
> > > > > for (j = 0; j<   ph->core->num_maps; j++) {
> > > > > -        print_debug("base = 0x%lx\tsize = %d\n", \
> > > > > ph->core->map_array[j]->vaddr, +        print_debug("base = 0x%lx\tsize = \
> > > > > %zd\n", ph->core->map_array[j]->vaddr, ph->core->map_array[j]->memsz);
> > > > > }
> > > > > }

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20120423/17f7ce47/attachment.html \



[prev in list] [next in list] [prev in thread] [next in thread]