1088790 - dosprint() doesn't support %zu and other size formats

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P1)

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

This makes it hard, e.g., to PRLog size_t types across 32- and 64-bit platforms.

  PR_LOG( ..., ("foo=%zu", (size_t)foo));

...shows up in the output (on b2g) as:

  PRLog: foo=%zu

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add support for (s)size_t types (e.g. "%zu") to dosprintf(), v1

I've tested this with --enable-32bit and -64bit on my PC (Ubuntu 14.04) and the test program passes. Is there an NSPR-version of the try-server I can bounce this off of?

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 8511317 [details] [diff] [review]
Add support for (s)size_t types (e.g. "%zu") to dosprintf(), v1

Review of attachment 8511317 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the review delay. Overall this looks good, I just have a few comments.

Also, no, sadly we do not have a NSPR try and we don't run NSPR tests as part of Firefox builds, so... You could probably write a hacky build system patch to forcibly run the NSPR tests in make check, I'm not sure we'd want to land that but it might serve to get you enough test coverage to be comfortable landing this.

::: pr/src/io/prprf.c
@@ +379,5 @@
>  }
>  
>  /*
>  ** BuildArgArray stands for Numbered Argument list Sprintf
>  ** for example,  

Can you nick the trailing whitespace while you're here?

@@ +523,5 @@
> +	    if (sizeof(size_t) == sizeof(PRInt32)) {
> +	        nas[ cn ].type = TYPE_INT32;
> +	    } else if (sizeof(size_t) == sizeof(PRInt64)) {
> +	        nas[ cn ].type = TYPE_INT64;
> +	    }

I realize other possible sizes are not likely to occur on any architecture that exists, but maybe an else { type = TYPE_UNKNOWN } would be a sane thing to add here?

@@ +547,5 @@
>  
>  	case 'p':
>  	    /* XXX should use cpp */
>  	    if (sizeof(void *) == sizeof(PRInt32)) {
> +	        nas[ cn ].type = TYPE_UINT32;

This looks like a whitespace-only change?

@@ +820,5 @@
>  		type = TYPE_INT64;
>  		c = *fmt++;
>  	    }
> +	} else if (c == 'z') {
> +	    if (sizeof(void *) == sizeof(PRInt32)) {

Why is this sizeof(void*) but you're using sizeof(size_t) in the other function?

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add support for (s)size_t types (e.g. "%zu") to dosprintf(), v2

Incorporate review feedback.

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add support for (s)size_t types (e.g. "%zu") to dosprintf(), v2.1

Fix a typo in a comment.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8528567 [details] [diff] [review]
Add support for (s)size_t types (e.g. "%zu") to dosprintf(), v2.1

Review of attachment 8528567 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I'll land this for you in a bit.

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

Hi Ted, just wondering what the time-frame is for landing this.

Comment 7

[Tom Tromey :tromey]

On Windows, PRI*SIZE are defined with "I" rather than "z".
From SizePrintfMacros.h:

#if defined(XP_WIN)
#  define PRIoSIZE  "Io"
[...]

I think it would be nice to recognize 'I' alongside 'z' so that these macros can be used.

Comment 8

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/projects/nspr/rev/81eee9de0f07

Comment 9

[(not currently active) Ted Mielczarek]

Sorry, I totally forgot about this. Thanks for the reminder. I've landed your patch in the NSPR repo and tagged that revision as NSPR_4_10_9_BETA1. If you'd like to land this in m-c, file a bug for updating NSPR, then in your local clone run "client.py update_nspr NSPR_4_10_9_BETA1" to pull in the changes.

Comment 10

[Wan-Teh Chang]

Attachment: Conditionally compile the ssize_t test cases for Unix only

Windows doesn't seem to have the signed ssize_t type, so the
prfz.c test doesn't compile on Windows.

The easiest fix seems to be assume that ssize_t is available
on Unix/POSIX platforms only and conditionally compile those
test cases only if the macro XP_UNIX is defined.

Comment 11

[Wan-Teh Chang]

Comment on attachment 8625358 [details] [diff] [review]
Conditionally compile the ssize_t test cases for Unix only

https://hg.mozilla.org/projects/nspr/rev/9cd44a67ff5b

Comment 12

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8625358 [details] [diff] [review]
Conditionally compile the ssize_t test cases for Unix only

Looks like this has already landed, so removing r?.