There are printf-style functions/methods in various places, most notably in the nsString family. And PR_*printf* that are used a lot in loggers. But they currently do not use available compiler sanity checks, so it may not be obvious in some use cases that the wrong parameters are passed in (e.g. '%d' is given a float, etc.) On gcc and clang, there is: __attribute__ ((format (printf, ...))) https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Function-Attributes.html On msvc: _Printf_format_string_ from sal.h (No obvious MSDN doc)

Note: NSPR printf functions may use non-standard format specifiers, so the solution may not be obvious there. (Thank you Nathan for reminding me)

(In reply to Gerald Squelart [:gerald] from comment #1) > Note: NSPR printf functions may use non-standard format specifiers, so the > solution may not be obvious there. > (Thank you Nathan for reminding me) this has been tried before, and failed because of that. I believe nspr may also you standard specifiers to mean something different than what the standard says they mean :(

And I see that nsTSubstring::AppendPrintf uses PR_vsxprintf. Gah, it's everywhere! prprf.h explains it all. It should be part of the new-hire firehose, and referred to from MDN's internal-string & coding-style pages. My initial attempt (not knowing The Truth yet) pointed at a few mismatches according to gcc, e.g. %lu for a uint32_t, but I see now that it's correct for NSPR. What's worrying is that there were *only* a few warnings (about 40 for AppendPrintf and nsPrintCString), meaning some developers (e.g., me) have probably used standard format specifiers instead of NSPR ones. Is this set in stone, i.e., NSPR cannot be changed without hurting ourselves and/or others? Alternatively, it'd be interesting to see if gcc/clang could be tailored to accept NSPR's style; or I'm thinking some templating magic might help with checks...

(In reply to Gerald Squelart [:gerald] from comment #3) > And I see that nsTSubstring::AppendPrintf uses PR_vsxprintf. Gah, it's > everywhere! heh, yeah > prprf.h explains it all. It should be part of the new-hire firehose, and > referred to from MDN's internal-string & coding-style pages. nspr is tricky to deal with? yes probably ;) > My initial attempt (not knowing The Truth yet) pointed at a few mismatches > according to gcc, e.g. %lu for a uint32_t, but I see now that it's correct > for NSPR. > > What's worrying is that there were *only* a few warnings (about 40 for > AppendPrintf and nsPrintCString), meaning some developers (e.g., me) have > probably used standard format specifiers instead of NSPR ones. well, it often kind of mostly works though of course mixing up floating point vs integer won't > Is this set in stone, i.e., NSPR cannot be changed without hurting ourselves > and/or others? Well, changing nspr is certainly painful for you, and somewhat problematic other people do use it. Ted owns it now I think and maybe he's more open to breaking backwards compat. > Alternatively, it'd be interesting to see if gcc/clang could be tailored to > accept NSPR's style; or I'm thinking some templating magic might help with > checks... I'm not sure compilers support that today though adding support for custom formats has been discussed I think. I'd probably advise just moving away from nspr formatting to using a standard formatter, that's probably a bit of work but killing off nspr dependencies is always kind of nice.

(In reply to Trevor Saunders (:tbsaunde) from comment #4) > (In reply to Gerald Squelart [:gerald] from comment #3) > > Is this set in stone, i.e., NSPR cannot be changed without hurting ourselves > > and/or others? > > Well, changing nspr is certainly painful for you, and somewhat problematic > other people do use it. Ted owns it now I think and maybe he's more open to > breaking backwards compat. I think breaking backwards compat in what sort of format strings NSPR accepted would not be considered reasonable. > I'd probably advise just moving away from nspr formatting to using a > standard formatter, that's probably a bit of work but killing off nspr > dependencies is always kind of nice. I think this is much more plausible: import NSPR's formatter (which I believe we've already basically done for nsTextFormatter), clean it up to accept the standard format strings, and then use that everywhere.

(In reply to Gerald Squelart [:gerald] from comment #3) > My initial attempt (not knowing The Truth yet) pointed at a few mismatches > according to gcc, e.g. %lu for a uint32_t, but I see now that it's correct > for NSPR. I looked into this at one point. It's a mess :( * As you note, things like %l differ * In NSPR I believe %s is well-defined when the string is NULL, but this isn't valid for ordinary printf. * SpiderMonkey has its own printf-like which is a multi-step ewww: * In-tree fork of the NSPR code * Defines %hs to print UCS-2 strings (which, amazingly, can't be done with the standard printf, as they added char16_t but no facility to print it) * ... but fails to actually print such strings properly: https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/js/src/jsprf.cpp#101 Moving to a standard formatter would be very good but the above need to be addressed. Moving an in-tree formatter to use standard formats would also be good, but the char16_t problem would still need some solution. > I'm not sure compilers support that today though adding support for custom formats has been discussed I think. I have an unfinished GCC patch to let one write a GCC plugin to add new formats. You can also do something ridiculous like https://github.com/tromey/typesafe-printf

(In reply to Trevor Saunders (:tbsaunde) from comment #2) > (In reply to Gerald Squelart [:gerald] from comment #1) > > Note: NSPR printf functions may use non-standard format specifiers, so the > > solution may not be obvious there. > > (Thank you Nathan for reminding me) > > this has been tried before, and failed because of that. The previous attempt (or one of them) was bug 1060419. Feel free to dupe or forward-dupe.

Thank you everybody for your responses so far, very informative. Given the fairly small amount of warnings when adding __attribute__, it might actually be feasible to switch at least a significant number of uses of NSPR printfs to another (compiler-checkable) API -- with special care for %s accepting null. (In reply to Tom Tromey :tromey from comment #6) > You can also do something ridiculous like https://github.com/tromey/typesafe-printf Something like Tom's "ridiculous" solution is something I was considering, this would be a portable way to do checks ourselves without compiler help. (In reply to Botond Ballo [:botond] from comment #7) > The previous attempt (or one of them) was bug 1060419. Feel free to dupe or > forward-dupe. I should have know that such an "interesting" project had been attempted before! I'd like to keep this open on its own a little while longer. More comments&ideas welcome! Maybe the description could be changed to "Check NSPR printf uses" or "Replace NSPR printfs uses with standard ones" so it's different enough that it's not exactly a dupe. And if someone else is eager and sees a good solution, please go ahead and take this bug as I'll probably be slow to act on it.

In bug 553032, I changed the js printf-like to follow the standard a bit more closely, and enabled compiler printf checking. Maybe this code could be moved to mfbt or mozglue and then used elsewhere.