This is a follow-up to bug 965022. See comments 31-33, and patches 4.5, 5, and 6, on that bug.

I found a number of other places that could use this treatment. (And see also bug 553032, which is specifically about js/). I've got about 16 patches including the ones referenced by this bug. There's actually one more to be done -- PR_LogPrint, PR_snprintf, etc, can be converted. However, this yields a large number of errors, so it may take a while. It's also worth noting that not everything can be marked up that we would like to mark up. JS extends printf with "%hs" (at least); and nsTextFormatter.h takes a char16_t* as a format argument, which gcc doesn't recognize. I'll upload and test my patches sometime soon.

How do you propose to solve the problem of the nspr printf functions treating %l differently from standard printf (and thus putting the attribute on them, which expects the standard printf interface, triggering bogus errors)?

(In reply to Botond Ballo [:botond] from comment #2) > How do you propose to solve the problem of the nspr printf functions > treating %l differently from standard printf (and thus putting the attribute > on them, which expects the standard printf interface, triggering bogus > errors)? I have a variant of your patch from bug 965022 in my series; and I'll need to fix all those PR_LogPrint, etc, errors mentioned in comment #1. But I'm curious why you ask -- is there a problem you foresee? It seems to me that non-standard treatment for %l is actively confusing, so it's best to change nspr and update the tree to follow. Maybe this is overly ambitious and/or misguided somehow. I'd like to know. This does find real bugs among all the logging fixes and minor %whatever nits. E.g., in your nsPrintfCString patch, there is a fix for a real bug in APZCCallbackHandler.cpp.

(In reply to Tom Tromey :tromey from comment #3) > I have a variant of your patch from bug 965022 in my series; and > I'll need to fix all those PR_LogPrint, etc, errors mentioned in comment #1. > But I'm curious why you ask -- is there a problem you foresee? I got the impression, from e.g. [1], that making a backwards-incompatible change to nspr is difficult. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1035925#c5

(In reply to Botond Ballo [:botond] from comment #4) > (In reply to Tom Tromey :tromey from comment #3) > > I have a variant of your patch from bug 965022 in my series; and > > I'll need to fix all those PR_LogPrint, etc, errors mentioned in comment #1. > > But I'm curious why you ask -- is there a problem you foresee? > > I got the impression, from e.g. [1], that making a backwards-incompatible > change to nspr is difficult. > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1035925#c5 Wow, I had no idea. That seems very unfortunate to me, both for nspr and for firefox. If nspr is truly frozen like that then the only options available I see for this bug are: * Migrate all PR_* printf-like uses to use something more standard; or * Write our own compiler plugin to check format strings. The latter is maybe more practical. It would let us solve the %hs issue for JS as well. A couple of the patches I have are salvageable, as they are wrappers for vfprintf or the like. Most are not though :(

Hah, and I just saw that JS_smprintf and friends are a copy-and-pasted version of prprf.c, only with different changes and extensions.

I started working on this a bit again. Bug 553032 made the JS printf standard-like, so my new plan is to move this to mozglue, rename the functions, then go through the tree and update calls to PR_mumble to call the new functions, fixing up the warnings along the way. I'm happy to push my current series to github if anybody cares to see it. I think it's going to take a while to finish. FWIW it turns out that there are a number of misuses of *printf APIs, though so far I think I've only seen them in logging code.

One fun thing I learned is that an enum class enumerator doesn't (AFAICT) implicitly convert to its base type when passed through varargs. So currently the series has many changes like: - LOG(("OBJLC [%p]: OpenChannel returned failure (%u)", this, rv)); + LOG(("OBJLC [%p]: OpenChannel returned failure (%" PRIu32 ")", + this, static_cast<uint32_t>(rv))); Gross! But perhaps required.

This code is a bit funny: https://dxr.mozilla.org/mozilla-central/source/js/ipc/JavaScriptLogging.h#75

Might as well admit it.

I'm probably not going to fix up the weirdness in JavaScriptLogging. I also wasn't planning to convert the various PR_fprintf calls. The current series is already quite large... I can file follow-up bugs if that's desirable.

I'm going to attach my current series for safekeeping and for anyone who is curious. It builds locally (Linux); I'm also going to push it through try to see what else complains. I'm planning to hold off requesting review until that is clean. It will need at least a rebase regardless; and probably sheriff support to land. I tried to make each patch as small as possible for simpler review. Some of them are quite large anyway; for example changing the logging API resulted in a lot of fallout. Each point in the series builds locally -- that is, a patch may depend on older patches, but not on newer ones.

Well that was hilarious.

webrtc has some -Wformat issues but IIUC we import this from upstream. So I'm just going to disable the warning there.

This spot: https://dxr.mozilla.org/mozilla-central/rev/863c2b61bd27bb6099104933134d3be7c052551a/xpcom/io/nsPipe3.cpp#888 ... really wants %td, but our printf-like doesn't implement this, and I think neither does MSVC. So I'm going to cast that to int64_t.

Only ~90 patches to get it to pass try! But I will rebase them into sanity.

libcubeb needed -Wno-format; see https://github.com/kinetiknz/cubeb/issues/203 for follow-up. I'll file a follow-up bug here to re-enable -Wformat for libcubeb, after the next import is done.

I'm going to attach the patches for review now. There is still some lingering problem on Windows; I haven't investigated that yet. As mentioned previously, this is going to require a rebase anyhow, and also some sheriff support while landing. I think it's ready for review, though, as the overall shape has been stable while I've been fixing the various minor problems.

Comment on attachment 8821232 [details] Bug 1060419 - convert one nsPrintfCString to MOZ_LOG, https://reviewboard.mozilla.org/r/100552/#review101328

Comment on attachment 8819582 [details] Bug 1060419 - make MediaEngineWebRTC.h use SprintfLiteral, https://reviewboard.mozilla.org/r/99288/#review101354

Comment on attachment 8819585 [details] Bug 1060419 - make XPC_Log_print use VsprintfLiteral, https://reviewboard.mozilla.org/r/99294/#review101358 I think that converting `%x` to `%p` will be relatively non-controversial. ::: js/xpconnect/src/XPCLog.cpp:60 (Diff revision 2) > { > va_list ap; > char line[LINE_LEN]; > > va_start(ap, fmt); > - PR_vsnprintf(line, sizeof(line)-1, fmt, ap); > + SprintfLiteral(line, fmt, ap); Surely this should be VsprintfLiteral? ::: js/xpconnect/src/XPCWrappedJS.cpp:700 (Diff revision 2) > > NS_IMETHODIMP > nsXPCWrappedJS::DebugDump(int16_t depth) > { > #ifdef DEBUG > - XPC_LOG_ALWAYS(("nsXPCWrappedJS @ %x with mRefCnt = %d", this, mRefCnt.get())); > + XPC_LOG_ALWAYS(("nsXPCWrappedJS @ %p with mRefCnt = %" PRIuPTR, this, mRefCnt.get())); This doesn't seem right: `mRefCnt` ought to be an integer-valued thing, not a pointer-sized integer value. I think `%u` is the right format code here? ::: js/xpconnect/src/XPCWrappedJSClass.cpp:1416 (Diff revision 2) > NS_IMETHODIMP > nsXPCWrappedJSClass::DebugDump(int16_t depth) > { > #ifdef DEBUG > depth-- ; > - XPC_LOG_ALWAYS(("nsXPCWrappedJSClass @ %x with mRefCnt = %d", this, mRefCnt.get())); > + XPC_LOG_ALWAYS(("nsXPCWrappedJSClass @ %p with mRefCnt = %" PRIuPTR, this, mRefCnt.get())); Same issue with the refcount here. ::: js/xpconnect/src/XPCWrappedNative.cpp:2151 (Diff revision 2) > > NS_IMETHODIMP XPCWrappedNative::DebugDump(int16_t depth) > { > #ifdef DEBUG > depth-- ; > - XPC_LOG_ALWAYS(("XPCWrappedNative @ %x with mRefCnt = %d", this, mRefCnt.get())); > + XPC_LOG_ALWAYS(("XPCWrappedNative @ %p with mRefCnt = %" PRIuPTR, this, mRefCnt.get())); Same comment here on the refcount. ::: js/xpconnect/src/nsXPConnect.cpp:857 (Diff revision 2) > NS_IMETHODIMP > nsXPConnect::DebugDump(int16_t depth) > { > #ifdef DEBUG > depth-- ; > - XPC_LOG_ALWAYS(("nsXPConnect @ %x with mRefCnt = %d", this, mRefCnt.get())); > + XPC_LOG_ALWAYS(("nsXPConnect @ %p with mRefCnt = %" PRIuPTR, this, mRefCnt.get())); Here with the refcount as well.

Comment on attachment 8819595 [details] Bug 1060419 - make WebGLContextUtils.cpp use VsprintfLiteral, https://reviewboard.mozilla.org/r/99314/#review101360 ::: dom/canvas/WebGLContext.cpp:763 (Diff revision 2) > " entry: "; > reason.info.Append(reason.key); > > out_failReasons->push_back(reason); > > - GenerateWarning(reason.info.BeginReading()); > + GenerateWarning("%s", reason.info.BeginReading()); The only good thing about this is that I think `reason.info` might have been entirely under our control before... ::: dom/canvas/WebGLContextUtils.cpp:79 (Diff revision 2) > return; > > mAlreadyGeneratedWarnings++; > > char buf[1024]; > - PR_vsnprintf(buf, 1024, fmt, ap); > + SprintfLiteral(buf, fmt, ap); Surely this should be VsprintfLiteral?

Comment on attachment 8819598 [details] Bug 1060419 - make nsFrame.cpp use VsprintfLiteral, https://reviewboard.mozilla.org/r/99320/#review101362

Comment on attachment 8819585 [details] Bug 1060419 - make XPC_Log_print use VsprintfLiteral, https://reviewboard.mozilla.org/r/99294/#review101358 > Surely this should be VsprintfLiteral? Yes, sorry about that. Fixed locally. > This doesn't seem right: `mRefCnt` ought to be an integer-valued thing, not a pointer-sized integer value. I think `%u` is the right format code here? I think mRefCnt is declared here: https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#296 It is an nsrefcnt, which is defined as a typedef of MozRefCountType: https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h#252 That in turn is a uintptr_t: https://dxr.mozilla.org/mozilla-central/source/mfbt/RefCountType.h#22 At least, this was my logic when I made this change. I'm not always confident that I'm finding the correct definitions in the tree.

Comment on attachment 8819595 [details] Bug 1060419 - make WebGLContextUtils.cpp use VsprintfLiteral, https://reviewboard.mozilla.org/r/99314/#review101360 > The only good thing about this is that I think `reason.info` might have been entirely under our control before... There are handful of changes along these lines; I figured they are obviously safe (even more obviously so than before) and not very expensive (considering that the others I recall are in logging anyhow). > Surely this should be VsprintfLiteral? Yes, sorry about this. Fixed locally.

Comment on attachment 8819580 [details] Bug 1060419 - remove unneeded includes of prprf.h, https://reviewboard.mozilla.org/r/99284/#review101374 ::: netwerk/test/TestFileInput2.cpp:92 (Diff revision 2) > double mean = mTotalTime / mCount; > double variance = fabs(mSquares / mCount - mean * mean); > double stddev = sqrt(variance); > uint32_t imean = (uint32_t)mean; > uint32_t istddev = (uint32_t)stddev; > - return PR_smprintf("%d +/- %d ms", > + return mozilla::Smprintf("%d +/- %d ms", Oops, this stuff wasn't meant to be in this patch. I will put it into another patch in the next revision.

Comment on attachment 8819580 [details] Bug 1060419 - remove unneeded includes of prprf.h, https://reviewboard.mozilla.org/r/99284/#review101374 > Oops, this stuff wasn't meant to be in this patch. I will put it into another patch in the next revision. I did this.

Comment on attachment 8819579 [details] Bug 1060419 - remove unused members from SprintfState, https://reviewboard.mozilla.org/r/99282/#review102648

Comment on attachment 8819577 [details] Bug 1060419 - remove SprintfState typedef; https://reviewboard.mozilla.org/r/99278/#review102652 I think this is OK. Some notes below. ::: js/src/jscntxt.cpp:336 (Diff revision 2) > JSErrorReport report; > > if (checkReportFlags(cx, &flags)) > return true; > > - UniqueChars message(JS_vsmprintf(format, ap)); > + UniqueChars message(mozilla::Vsprintf(format, ap)); I wonder if we shouldn't change the documentation on `Smprintf` and friends, since this sort of change is kind of abusing the knowledge that `mozilla::SmprintfFree` is really just `free` under the hood. ::: js/src/jscntxt.cpp:403 (Diff revision 2) > if (JSREPORT_IS_WARNING(report->flags) && !reportWarnings) > return false; > > char* prefix = nullptr; > if (report->filename) > - prefix = JS_smprintf("%s:", report->filename); > + prefix = mozilla::Smprintf("%s:", report->filename); All these calls to `Smprintf` result in a memory leak, right? I don't see any calls to `JS_smprintf_free` in this function, or any changes from that to `mozilla::SmprintfFree`. Possibly worth auditing other callsites.. Pre-existing issue, so not critical to fix here, I suppose. ::: js/src/jsprf.h:66 (Diff revision 2) > MOZ_FORMAT_PRINTF(2, 3); > > /* > ** va_list forms of the above. > */ > -extern JS_PUBLIC_API(char*) JS_vsmprintf(const char* fmt, va_list ap); > +extern MFBT_API char* Vsprintf(const char* fmt, va_list ap); Shouldn't this be `Vsmprintf`, for continuity with `Smprintf`, above, and for consistency with the previous `JS_vsmprintf`?

Comment on attachment 8819578 [details] Bug 1060419 - move Smprintf et al to mozglue, https://reviewboard.mozilla.org/r/99280/#review102660 I've tried to limit myself to things that matter because of the move, and not things that should be cleaned up in this grotty code. Feel free to disagree. ::: mozglue/misc/Printf.h:7 (Diff revision 2) > + * vim: set ts=8 sts=4 et sw=4 tw=99: > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_Printf_h Can you put a single-line comment before this include guard so the file displays nicely in DXR etc.? ::: mozglue/misc/Printf.h:11 (Diff revision 2) > + > +#ifndef mozilla_Printf_h > +#define mozilla_Printf_h > + > +/* > +** API for PR printf like routines. Supports the following formats I don't know that it's necessary to do here, but it'd be nice to document *why* this header and associated baggage exist. Possible reasons that come to mind: - Bugs in existing `printf` implementations. - Incomplete `printf` implementations. - The ability to control where the output goes (malloc'ing returned strings and such), though this might be less important now that we could write a portable `asprintf`? - Guarantees on behavior (e.g. no allocation under the hood). It's possible, I suppose, that the need for this is just a relic of earlier times, and we could do everything with standard code and some wrappers? You don't have to add that documentation for this bug; a follow-up would be fine. ::: mozglue/misc/Printf.h:26 (Diff revision 2) > +** %f - float > +** %g - float Would be good to document that these are formatted with the platform's native `printf`, and so may vary from platform to platform. ::: mozglue/misc/Printf.h:59 (Diff revision 2) > +** the malloc'd buffer. sprintf will append data to the end of last, > +** growing it as necessary using realloc. If last is nullptr, SprintfAppend > +** will allocate the initial string. The return value is the new value of > +** last for subsequent calls, or nullptr if there is a malloc failure. > +*/ > +extern MFBT_API char* SprintfAppend(char* last, const char* fmt, ...) Feels like once we have `PrintfTarget` from other patches, the `*Append` variants can go away as a followup bug, perhaps? ::: mozglue/misc/Printf.h:65 (Diff revision 2) > + MOZ_FORMAT_PRINTF(2, 3); > + > +/* > +** va_list forms of the above. > +*/ > +extern MFBT_API char* Vsprintf(const char* fmt, va_list ap); Comments on calling this `Vsmprintf` from a previous patch apply here too. ::: mozglue/misc/Printf.cpp:280 (Diff revision 2) > + SprintfLiteral(fout, fin, d); > + > + return (*ss->stuff)(ss, fout, strlen(fout)); Can you file a followup so we can use the return value of `SprintfLiteral` here rather than calling `strlen`? ::: mozglue/misc/Printf.cpp:357 (Diff revision 2) > + if (!nas.growByUninitialized(number)) > + return false; WDYT about putting some sort of hard, smallish cap on `number`, so that we just return false here if `number` is too big because somebody passed in a screwy format string? I guess the allocation would fail here for really large numbers, but for medium-size numbers that we don't really want to bother testing, a cap would give us some peace of mind. I think C has a recommended limit of 20 numbered arguments, which seems reasonable, and is implemented elsewhere (e.g. `nsTextFormatter`). ::: mozglue/misc/Printf.cpp:865 (Diff revision 2) > + /* Copy data */ > + while (len) { > + --len; > + *ss->cur++ = *sp++; > + } I guess they didn't have `memcpy` in the good ol' days.

Comment on attachment 8819601 [details] Bug 1060419 - add PrintfTarget class to jsprf.h, https://reviewboard.mozilla.org/r/99326/#review102680 I think this is OK. I would have liked being able to do this without virtual functions, but everything else is less convenient and less C++-y. We'll live, I guess. ::: mozglue/misc/Printf.h:81 (Diff revision 3) > + bool MFBT_API print(const char* format, ...) MOZ_FORMAT_PRINTF(2, 3); > + > + /* The Vprintf-like interface. */ > + bool MFBT_API vprint(const char* format, va_list); > + > + bool emit(const char* sp, size_t len) { Ugh, there's no good way to make this "private, but reasonably callable from the guts of the printf code" is there? I guess you'd have to make all the little helper functions, `cvt_f` and whatnot, private methods of `PrintfTarget`...actually, that's not a bad idea, given that this is basically what you have to do with C-like syntax already. WDYT about changing that, so we don't have to expose this function?

Comment on attachment 8819602 [details] Bug 1060419 - make nsDebugImpl.cpp use PrintfTarget, https://reviewboard.mozilla.org/r/99328/#review102682 ::: xpcom/base/nsDebugImpl.cpp:346 (Diff revision 3) > - PRINT_TO_NONPID_BUFFER("line %d", aLine); > + nonPIDBuf.print("line %d", aLine); > } > -#undef PRINT_TO_NONPID_BUFFER > > // Print "[PID]" or "[Desc PID]" at the beginning of the message. > -#define PRINT_TO_BUFFER(...) PR_sxprintf(StuffFixedBuffer, &buf, __VA_ARGS__) > + buf.print("["); The lone use of `PR_sxprintf` in the tree. Good riddance.

Comment on attachment 8819577 [details] Bug 1060419 - remove SprintfState typedef; https://reviewboard.mozilla.org/r/99278/#review102652 > I wonder if we shouldn't change the documentation on `Smprintf` and friends, since this sort of change is kind of abusing the knowledge that `mozilla::SmprintfFree` is really just `free` under the hood. Yeah. This is why I was talking about removing JS_smprintf_free and replacing it with plain free -- the tree already isn't consistent about which variant of free it calls. > All these calls to `Smprintf` result in a memory leak, right? I don't see any calls to `JS_smprintf_free` in this function, or any changes from that to `mozilla::SmprintfFree`. Possibly worth auditing other callsites.. > > Pre-existing issue, so not critical to fix here, I suppose. This code uses JS_free rather than JS_smprintf_free. E.g., https://dxr.mozilla.org/mozilla-central/rev/f13abb8ba9f366c9f32a3146245adf642528becd/js/src/jscntxt.cpp#465

Comment on attachment 8819578 [details] Bug 1060419 - move Smprintf et al to mozglue, https://reviewboard.mozilla.org/r/99280/#review102660 > Can you file a followup so we can use the return value of `SprintfLiteral` here rather than calling `strlen`? This is small enough that I've just tacked on an additional patch to the series.

Comment on attachment 8819578 [details] Bug 1060419 - move Smprintf et al to mozglue, https://reviewboard.mozilla.org/r/99280/#review102660 > Feels like once we have `PrintfTarget` from other patches, the `*Append` variants can go away as a followup bug, perhaps? There are ~30 calls to SprintfAppend, so I think keeping it around as a convenience function seems reasonable. VsprintfAppend only has 2 callers, so I suppose it could go away.

Comment on attachment 8819601 [details] Bug 1060419 - add PrintfTarget class to jsprf.h, https://reviewboard.mozilla.org/r/99326/#review102680 > Ugh, there's no good way to make this "private, but reasonably callable from the guts of the printf code" is there? I guess you'd have to make all the little helper functions, `cvt_f` and whatnot, private methods of `PrintfTarget`...actually, that's not a bad idea, given that this is basically what you have to do with C-like syntax already. WDYT about changing that, so we don't have to expose this function? Good idea, I will do that.

Comment on attachment 8819601 [details] Bug 1060419 - add PrintfTarget class to jsprf.h, https://reviewboard.mozilla.org/r/99326/#review102680 > Good idea, I will do that. Ok, fixed locally.

Comment on attachment 8819585 [details] Bug 1060419 - make XPC_Log_print use VsprintfLiteral, https://reviewboard.mozilla.org/r/99294/#review101358 > I think mRefCnt is declared here: > > https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#296 > > It is an nsrefcnt, which is defined as a typedef of MozRefCountType: > > https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h#252 > > That in turn is a uintptr_t: > > https://dxr.mozilla.org/mozilla-central/source/mfbt/RefCountType.h#22 > > At least, this was my logic when I made this change. I'm not always confident that I'm finding the correct definitions in the tree. We discussed this and agreed that this part is correct as-is.

Comment on attachment 8819580 [details] Bug 1060419 - remove unneeded includes of prprf.h, https://reviewboard.mozilla.org/r/99284/#review103062 Gonna assume this compiles appropriately.

Comment on attachment 8819581 [details] Bug 1060419 - make nsChromeRegistry.cpp use Printf.h, https://reviewboard.mozilla.org/r/99286/#review103064 It's really too bad that we can't printf to 16-bit characters straightaway here. `nsTextFormatter` can, though...maybe after this lands this custom `printf` code and `nsTextFormatter` could be merged somehow.

Comment on attachment 8819583 [details] Bug 1060419 - make nsPluginsDirWin.cpp use Printf.h, https://reviewboard.mozilla.org/r/99290/#review103068 Using `%ld` for the 16-bit value returned from `HIWORD` is kind of dodgy, but thanks to C, I guess this works!

Comment on attachment 8819584 [details] Bug 1060419 - make ipc/chromium use Printf.h, https://reviewboard.mozilla.org/r/99292/#review103070

Comment on attachment 8819586 [details] Bug 1060419 - make nsCookieService.cpp use Printf.h, https://reviewboard.mozilla.org/r/99296/#review103074

Comment on attachment 8819587 [details] Bug 1060419 - make nsHttpHandler.cpp use Printf.h, https://reviewboard.mozilla.org/r/99298/#review103078 ::: netwerk/protocol/http/nsHttpHandler.cpp:869 (Diff revision 3) > #endif > - char *buf = PR_smprintf(format, > + char *buf = mozilla::Smprintf(format, > - info.dwMajorVersion, > + info.dwMajorVersion, > - info.dwMinorVersion); > + info.dwMinorVersion); > if (buf) { > mOscpu = buf; Ah, C++, making code look like a memory hazard.

Comment on attachment 8819588 [details] Bug 1060419 - make mozStorageConnection.cpp and mozStorageStatement.cpp use Printf.h, https://reviewboard.mozilla.org/r/99300/#review103080

Comment on attachment 8819589 [details] Bug 1060419 - make nsExceptionHandler.cpp use Printf.h, https://reviewboard.mozilla.org/r/99302/#review103082 ::: toolkit/crashreporter/nsExceptionHandler.cpp:3505 (Diff revision 3) > #if defined(XP_WIN) > - PR_Free(childCrashNotifyPipe); > + mozilla::SmprintfFree(childCrashNotifyPipe); Hm, I guess this should be `#if defined(XP_MACOSX)` too, since we allocate `childCrashNotifyPipe` on OS X too? File a followup for that?

Comment on attachment 8819590 [details] Bug 1060419 - make nsProfileLock.cpp use Printf.h, https://reviewboard.mozilla.org/r/99304/#review103084

Comment on attachment 8819592 [details] Bug 1060419 - make ManifestParser use Printf.h, https://reviewboard.mozilla.org/r/99308/#review103094

Comment on attachment 8819593 [details] Bug 1060419 - make TestPipes.cpp use Printf.h, https://reviewboard.mozilla.org/r/99310/#review103096

Comment on attachment 8819594 [details] Bug 1060419 - make nsEnvironment.cpp use Printf.h, https://reviewboard.mozilla.org/r/99312/#review103098

Comment on attachment 8819596 [details] Bug 1060419 - make nsAppRunner.cpp use Printf.h, https://reviewboard.mozilla.org/r/99316/#review103100

Comment on attachment 8819597 [details] Bug 1060419 - make nsWebBrowserPersist.cpp use Printf.h, https://reviewboard.mozilla.org/r/99318/#review103102

Comment on attachment 8819599 [details] Bug 1060419 - make xpcom/glue/nsDebug.h use Printf.h, https://reviewboard.mozilla.org/r/99322/#review103104

Comment on attachment 8819600 [details] Bug 1060419 - make SharedMemoryBasic_mach.mm use Printf.h, https://reviewboard.mozilla.org/r/99324/#review103106

Comment on attachment 8819604 [details] Bug 1060419 - fix comment in IntegerPrintfMacros.h to reflect Printf.h change, https://reviewboard.mozilla.org/r/99332/#review103108

Comment on attachment 8821615 [details] Bug 1060419 - make netwerk/test/TestFileInput2.cpp use Printf.h; https://reviewboard.mozilla.org/r/100848/#review103110

Comment on attachment 8819577 [details] Bug 1060419 - remove SprintfState typedef; https://reviewboard.mozilla.org/r/99278/#review102652 > Shouldn't this be `Vsmprintf`, for continuity with `Smprintf`, above, and for consistency with the previous `JS_vsmprintf`? Good idea. I've changed this locally.

Comment on attachment 8819578 [details] Bug 1060419 - move Smprintf et al to mozglue, https://reviewboard.mozilla.org/r/99280/#review102660 > I don't know that it's necessary to do here, but it'd be nice to document *why* this header and associated baggage exist. Possible reasons that come to mind: > > - Bugs in existing `printf` implementations. > - Incomplete `printf` implementations. > - The ability to control where the output goes (malloc'ing returned strings and such), though this might be less important now that we could write a portable `asprintf`? > - Guarantees on behavior (e.g. no allocation under the hood). > > It's possible, I suppose, that the need for this is just a relic of earlier times, and we could do everything with standard code and some wrappers? > > You don't have to add that documentation for this bug; a follow-up would be fine. I'm adding another patch for this comment and also the one before the include guard.

Comment on attachment 8819578 [details] Bug 1060419 - move Smprintf et al to mozglue, https://reviewboard.mozilla.org/r/99280/#review102660 > WDYT about putting some sort of hard, smallish cap on `number`, so that we just return false here if `number` is too big because somebody passed in a screwy format string? I guess the allocation would fail here for really large numbers, but for medium-size numbers that we don't really want to bother testing, a cap would give us some peace of mind. > > I think C has a recommended limit of 20 numbered arguments, which seems reasonable, and is implemented elsewhere (e.g. `nsTextFormatter`). Sure, I used MOZ_RELEASE_ASSERT, hopefully that's correct.

Comment on attachment 8819577 [details] Bug 1060419 - remove SprintfState typedef; https://reviewboard.mozilla.org/r/99278/#review102652 > Yeah. This is why I was talking about removing JS_smprintf_free and replacing it with plain free -- the tree already isn't consistent about which variant of free it calls. I was somewhat mistaken here. It's true that the tree is not consistent about using JS_smprintf_free. However, UniqueChars does use js_free: https://dxr.mozilla.org/mozilla-central/rev/a14094edbad78fc1d16e8d4c57902537cf286fd1/js/public/Utility.h#496 ... so while some code is violating the abstraction here, it isn't actually incorrect. The various uses of values coming from JS_smprintf spread out enough that I am thinking it would be simplest to just continue using js_realloc and js_free in the new Printf.cpp. I'm not fond of this, but I'm not completely confident that I could correctly write a patch to change all the users of these values. I'd appreciate your input on this as well.

Comment on attachment 8819605 [details] Bug 1060419 - convert mozglue/linker to use the mfbt-provided printf format defines, https://reviewboard.mozilla.org/r/99334/#review103482

(In reply to Tom Tromey :tromey from comment #153) > The various uses of values coming from JS_smprintf spread out enough that I > am thinking it would be simplest to just continue using js_realloc and > js_free in the new Printf.cpp. I'm not fond of this, but I'm not completely > confident that I could correctly write a patch to change all the users of > these values. Won't this cause a circular dependency? JS depends on mozglue, which in turns depends on JS to provide js_free. (In reply to Tom Tromey :tromey from comment #152) > > I think C has a recommended limit of 20 numbered arguments, which seems reasonable, and is implemented elsewhere (e.g. `nsTextFormatter`). > > Sure, I used MOZ_RELEASE_ASSERT, hopefully that's correct. That's perfect, thank you.

(In reply to Nathan Froyd [:froydnj] from comment #155) > Won't this cause a circular dependency? JS depends on mozglue, which in > turns depends on JS to provide js_free. Yeah :( Another idea would be to give the various printf functions an allocator policy template parameter.

(In reply to Tom Tromey :tromey from comment #156) > Another idea would be to give the various printf functions an allocator > policy > template parameter. My current idea is to do this and also move the PrintfTarget patch much earlier in the series.

(In reply to Tom Tromey :tromey from comment #158) > (In reply to Tom Tromey :tromey from comment #156) > > > Another idea would be to give the various printf functions an allocator > > policy > > template parameter. > > My current idea is to do this and also move the PrintfTarget patch much > earlier > in the series. Does that mean a lot of the printf implementation is going to move into a header file? That does not seem so great. I do think there's going to be some amount of templating involved here or eventually (e.g. output character type, details about how you want floats formatted if we ever want to unify with nsTextFormatter, etc.), I'm just trying to get a sense of how everything would fit together.

(In reply to Nathan Froyd [:froydnj] from comment #159) > Does that mean a lot of the printf implementation is going to move into a > header file? No, I think just SprintfState will move to a header and become a template class. I'll clean it up a bit to be more normal (constructor etc). In the end the constructor, SprintfState::append, and the (much shorter) implementation of Smprintf et all will move into the header. The bulk of the printf functionality will remain in Printf.cpp. If you like I can whip up a patch showing what this looks like.

Comment on attachment 8819606 [details] Bug 1060419 - make security/certverifier/ExtendedValidation.cpp use Sprintf.h, https://reviewboard.mozilla.org/r/99336/#review104942

Comment on attachment 8819603 [details] Bug 1060419 - make AppendPrintf and nsPrintfCString use Printf.h, https://reviewboard.mozilla.org/r/99330/#review104944 This looks good, thank you. ::: dom/base/DOMException.cpp:556 (Diff revision 3) > > static const char defaultMsg[] = "<no message>"; > static const char defaultLocation[] = "<unknown>"; > static const char defaultName[] = "<unknown>"; > static const char format[] = > - "[Exception... \"%s\" code: \"%d\" nsresult: \"0x%x (%s)\" location: \"%s\"]"; > + "[Exception... \"%s\" code: \"%d\" nsresult: \"0x%" PRIu32 " (%s)\" location: \"%s\"]"; This looks like it should be `PRIx32` for consistency with the previous code. ::: xpcom/string/string-template-def-char.h:15 (Diff revision 3) > #define nsTAString_IncompatibleCharT nsAString > #define nsTString_CharT nsCString > #define nsTFixedString_CharT nsFixedCString > #define nsTAutoString_CharT nsAutoCString > #define nsTSubstring_CharT nsACString > +#define PrintfAppend_CharT PrintfAppend_nsACString You have modified the string code. Level up!

Comment on attachment 8819577 [details] Bug 1060419 - remove SprintfState typedef; https://reviewboard.mozilla.org/r/99278/#review102652 > I was somewhat mistaken here. It's true that the tree is not consistent about using JS_smprintf_free. However, UniqueChars does use js_free: > > https://dxr.mozilla.org/mozilla-central/rev/a14094edbad78fc1d16e8d4c57902537cf286fd1/js/public/Utility.h#496 > > ... so while some code is violating the abstraction here, it isn't actually incorrect. > > The various uses of values coming from JS_smprintf spread out enough that I am thinking it would be simplest to just continue using js_realloc and js_free in the new Printf.cpp. I'm not fond of this, but I'm not completely confident that I could correctly write a patch to change all the users of these values. > > I'd appreciate your input on this as well. The new series templatizes the functions as discussed. It's also much simpler since I've left the JS_* variants as wrapper functions. > This code uses JS_free rather than JS_smprintf_free. E.g., https://dxr.mozilla.org/mozilla-central/rev/f13abb8ba9f366c9f32a3146245adf642528becd/js/src/jscntxt.cpp#465 Dropping this one since I think the code is ok.

Comment on attachment 8819603 [details] Bug 1060419 - make AppendPrintf and nsPrintfCString use Printf.h, https://reviewboard.mozilla.org/r/99330/#review104944 > This looks like it should be `PRIx32` for consistency with the previous code. Thanks, I've fixed this locally.

I've rearranged the patch series to deal with the free/js_free problem. The new series differs from the old: * I kept jsprf.h and the various JS_*printf functions. This greatly simplifies a couple of patches, since there's no mass renaming and no need to change all the #includes. This is done so that the JS implementation can continue using js_malloc and js_free. * I've added an allocator policy to Smprintf et al. I stopped short of making these deal exclusively with unique pointers. I've made some other smaller changes, like marking a couple of "append" methods "override". I'll request re-review on the patches that have changed substantially. I'm still tweaking the series to build properly :) Once that is done the next steps are a clean try run and then, ugh, rebasing and fixing up the fallout.

Today I discovered another fork of the NSPR printf code: https://dxr.mozilla.org/mozilla-central/rev/de67fccc4c64a49f261aea29141357b94c7b3b9c/xpcom/glue/nsTextFormatter.cpp#10 ... hilariously discovered because making SprintfState more visible caused a name clash. This one actually has users of its char16_t interface, so it's harder to fix. I will most likely just rename the offending struct and file a followup bug for this.

Err, I should say, it *only* has a char16_t interface. GCC at least won't do any checking here.