965022 - Add __attribute__((format)) to printf like apis

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This makes for useful warnings.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: patch

Comment 2

[Mats Palmgren (inactive)]

>+      MOZ_FORMAT_PRINTF(2, 3)
>       void AppendPrintf( const char* format, ... );

Shouldn't that be "1, 2"?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mats Palmgren (:mats) from comment #2)
> >+      MOZ_FORMAT_PRINTF(2, 3)
> >       void AppendPrintf( const char* format, ... );
> 
> Shouldn't that be "1, 2"?

parameter 1 is the implicit 'this'

Comment 4

[(no longer active)]

Comment on attachment 8366955 [details] [diff] [review]
patch

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

I don't know anything about this attribute.  Making this Nathan's problem.  :-)

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8366955 [details] [diff] [review]
patch

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

r=me with the comments below addressed.  We get a gold star if adding these doesn't trigger additional warnings...

::: xpcom/glue/nsCRTGlue.cpp
@@ +308,5 @@
>    vfprintf(stderr, fmt, args);
>  }
>  #endif
>  
> +MOZ_FORMAT_PRINTF(1, 2)

This should go on the declaration of printf_stderr in nsDebug.h.  At that location, it should go after the argument list, as is typical for function attributes in the codebase.

@@ +318,5 @@
>    vprintf_stderr(fmt, args);
>    va_end(args);
>  }
>  
> +MOZ_FORMAT_PRINTF(2, 3)

Same comments as above, for fprintf_stderr.

::: xpcom/string/public/nsTSubstring.h
@@ +460,1 @@
>        void AppendPrintf( const char* format, ... );

Move this to after the argument list.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: format

Comment 7

[Botond Ballo [:botond]]

Going to take this because it would be extremely helpful for bug 1057642.

Comment 8

[Botond Ballo [:botond]]

Attachment: Part 1 - Introduce MOZ_FORMAT_PRINTF

This bit is straight out of Jeff's patch, which was r+'d by :froydnj, so carrying r+ for it.

Comment 9

[Botond Ballo [:botond]]

Attachment: Part 2 - Avoid pintf'ing a TimeStamp in PrintUniformityInfo

Comment 10

[Botond Ballo [:botond]]

Attachment: Part 3 - Add MOZ_FORMAT_PRINTF to [f]printf_stderr and fix fallout

Doing a build-only Try push to flush out any errors on other platforms: https://tbpl.mozilla.org/?tree=Try&rev=3a62ed8f607c

Comment 11

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8478528 [details] [diff] [review]
Part 2 - Avoid pintf'ing a TimeStamp in PrintUniformityInfo

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

LGTM. Can we print the TimeDuration::ToMilliseconds instead of MicroSeconds? Thanks!

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 2 - Avoid pintf'ing a TimeStamp in PrintUniformityInfo

(In reply to Mason Chang [:mchang] from comment #11)
> LGTM. Can we print the TimeDuration::ToMilliseconds instead of MicroSeconds?
> Thanks!

Done. Updated, carrying r+.

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 3 - Introduce PRISize

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 4 - Add MOZ_FORMAT_PRINTF to [f]printf_stderr and fix fallout

Updated to fix some missing cases caught by Try, and to use PRISize.

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 5 - Add MOZ_FORMAT_PRINTF to AppendPrintf and fix fallout

Comment 16

[Botond Ballo [:botond]]

Another buildonly Try push: https://tbpl.mozilla.org/?tree=Try&rev=67cb664acdef

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8478660 [details] [diff] [review]
Part 3 - Introduce PRISize

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

PR

::: mfbt/IntegerPrintfMacros.h
@@ +62,5 @@
> +/**
> + * For printing size_t.
> + */
> +#define PRISize PRIuPTR
> +

PRIuSIZE is probably a more consistent name and is used by other projects.

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #16)
> Another buildonly Try push:
> https://tbpl.mozilla.org/?tree=Try&rev=67cb664acdef

Gah, OS X build error from the AppendPrintf patch.

Here's a buildonly Try push for just the [f]printf_stderr patch, so that I can at least land that while I play whac-a-mole with the AppendPrintf OS X errors on Try (not having a local OS X build): https://tbpl.mozilla.org/?tree=Try&rev=79a16a82455a

Comment 19

[Botond Ballo [:botond]]

Attachment: Part 3 - Introduce PRIuSIZE

(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)
> > +/**
> > + * For printing size_t.
> > + */
> > +#define PRISize PRIuPTR
> > +
> 
> PRIuSIZE is probably a more consistent name and is used by other projects.

Good point. Updated.

Comment 20

[Botond Ballo [:botond]]

Attachment: Part 4 - Add MOZ_FORMAT_PRINTF to [f]printf_stderr and fix fallout

Updated to change PRISize to PRIuSIZE.

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8478709 [details] [diff] [review]
Part 4 - Add MOZ_FORMAT_PRINTF to [f]printf_stderr and fix fallout

This one's clean.

Comment 22

[Botond Ballo [:botond]]

OS X whac-a-mole, attempt 2: https://tbpl.mozilla.org/?tree=Try&rev=38a816b10ea2

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Botond Ballo [:botond] from comment #13)
> Part 3 - Introduce PRISize

I'm +0 on this; the state of portably printing size_t values is sad-making, and we should try to make it pleasant, but IntegerPrintfMacros.h was intended as a polyfill for <inttypes.h>, so adding things to it that aren't standardized would be frowned upon.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8478664 [details] [diff] [review]
Part 5 - Add MOZ_FORMAT_PRINTF to AppendPrintf and fix fallout

Clean as well.

Now on to nsPrintfCString.

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8478708 [details] [diff] [review]
Part 3 - Introduce PRIuSIZE

This should be fine it might be worth still asking Nathan for a post commit review to make sure there are no objections.

Comment 26

[Botond Ballo [:botond]]

Attachment: Part 6 - Add MOZ_FORMAT_PRINTF to nsPrintfCString and fix fallout

A bit more fallout here, including lots of ugly PRIu64 stuff...

Comment 27

[Botond Ballo [:botond]]

Built-only Try push that includes the nsPrintCString change: https://tbpl.mozilla.org/?tree=Try&rev=9fde4221f009

Comment 28

[Botond Ballo [:botond]]

Landed Parts 1-5:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f076faf3c282
https://hg.mozilla.org/integration/mozilla-inbound/rev/f747fd154739
https://hg.mozilla.org/integration/mozilla-inbound/rev/a34ae046c947
https://hg.mozilla.org/integration/mozilla-inbound/rev/24251d4da019
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f68752ffe1e

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux64 mochitest-2 and mochitest-bc failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/894b7372561d

https://tbpl.mozilla.org/php/getParsedLog.php?id=46790067&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=46790834&tree=Mozilla-Inbound

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I want to say this broke lots of Windows tests as well, but they weren't run for several pushes including this one's. The tests turned green sometime after this was backed out: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3a545eb9828b&jobname=Windows

Comment 31

[Botond Ballo [:botond]]

Attachment: Part 4.5 - Make PR_vsxprintf's handling of %l consistent with standard printf

I looked into the WebGL test failures. The cause is that AppendPrintf uses a custom-written printf implementation (PR_vsxprintf from NSPR), which treats %l differently than the standard printf functions (it treats %l as always 32-bit, whereas standard printf function treat %l as whatever size 'long' is on that platform).

This patch modifies PR_vsxprintf to treat %l the same standard print functions do.

Try push for parts 1-5, including tests this time: https://tbpl.mozilla.org/?tree=Try&rev=a9bb070916b9

Comment 32

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #27)
> Built-only Try push that includes the nsPrintCString change:
> https://tbpl.mozilla.org/?tree=Try&rev=9fde4221f009

Fixed OS X failures in nsPrintfCString patch. Trying again: https://tbpl.mozilla.org/?tree=Try&rev=f7b5a1d73e10

Comment 33

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #31)
> Created attachment 8480210 [details] [diff] [review]
> Part 4.5 - Make PR_vsxprintf's handling of %l consistent with standard printf
> 
> I looked into the WebGL test failures. The cause is that AppendPrintf uses a
> custom-written printf implementation (PR_vsxprintf from NSPR), which treats
> %l differently than the standard printf functions (it treats %l as always
> 32-bit, whereas standard printf function treat %l as whatever size 'long' is
> on that platform).
> 
> This patch modifies PR_vsxprintf to treat %l the same standard print
> functions do.
> 
> Try push for parts 1-5, including tests this time:
> https://tbpl.mozilla.org/?tree=Try&rev=a9bb070916b9

The test results indicate that my modification to PR_vsxprintf isn't correct.

Unfortunately I don't have more time to spend on this right now, so I will try landing parts 1-4 instead and leave the AppendPrintf/nsPrintfCString part for later.

Try push for parts 1-4: https://tbpl.mozilla.org/?tree=Try&rev=b2677c7b2dca

Comment 34

[Botond Ballo [:botond]]

Landed parts 1-4:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7beceadb18b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe235a6881ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/a54e395d3945
https://hg.mozilla.org/integration/mozilla-inbound/rev/d60a7e8582b1

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7beceadb18b4
https://hg.mozilla.org/mozilla-central/rev/fe235a6881ca
https://hg.mozilla.org/mozilla-central/rev/a54e395d3945
https://hg.mozilla.org/mozilla-central/rev/d60a7e8582b1

Comment 36

[Botond Ballo [:botond]]

Filed bug 1060419 as a follow-up for the AppendPrintf/nsPrintfCString work.

Comment 37

[Botond Ballo [:botond]]

Sorry, meant to close this when I filed the follow-up.