1690167 - Switch SprintfLiteral to using PrintfTarget instead of snprintf

Status: RESOLVED FIXED

(Core :: MFBT, task)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1690167 - Move Sprintf.h and IntegerPrintfMacros.h next to Printf.h.

Which means they move from MFBT to mozglue.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1690167 - Add support for the hh length modifier in Printf.cpp.

Test cases from TestIntegerPrintfMacros will cover this in the next commit.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1690167 - Change VsprintfLiteral/SprintfLiteral to rely on PrintfTarget.

Instead of snprintf.

Because some standalone code uses those functions directly or indirectly,
and PrintfTarget lives in mozglue, they now need to depend on mozglue
instead of mfbt. Except logalloc/replay, which cherry-picks what it
uses.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cac4879f50b4
Move Sprintf.h and IntegerPrintfMacros.h next to Printf.h. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/87b2a2a66fd9
Add support for the hh length modifier in Printf.cpp. r=nika,Gankra
https://hg.mozilla.org/integration/autoland/rev/cfffb092a39f
Change VsprintfLiteral/SprintfLiteral to rely on PrintfTarget. r=nika,Gankra,firefox-build-system-reviewers,mhentges

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27dee66eba83
Fix windows bustages. a=bustage-fix

Comment 6

[Alexandru Michis [:malexandru]]

Backed out 4 changesets (Bug 1690167) for causing cppunit failures in TestIntegerPrintfMacros.

Backout link: https://hg.mozilla.org/integration/autoland/rev/ff4b6faa5911d809f0815785c0593c44c6912ba4

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=ppunit&tochange=78fac34502ccebf64b59d85f8d0c17817958148f&fromchange=0935679e9bce0da9ae143218c233801e95c7eea5

Failure log: https://treeherder.mozilla.org/logviewer?job_id=329830435&repo=autoland&lineNumber=1500

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1690167 - Add support for the j length modifier in Printf.cpp.

Test cases from TestIntegerPrintfMacros will cover this in an upcoming commit.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1690167 - Allocate some memory before running RandomNum tests.

Because the previous commit changed how MFBT tests are linked, they now
use mozjemalloc. Mozjemalloc randomizes small allocations, which id does
by using MFBT's RandomNum. The code in RandomNum, on mac, uses a system
API that allocates memory. So mozjemalloc has some code to handle the
recursion gracefully.

When the RandomNum test runs, it essentially only runs the RNG... which
goes on to allocate memory, which then goes into the RNG. Needless to
say, that doesn't go well. In typical cases, this is not the type of
things that would happen, but it does happen for that one test.

We work around the issue by allocating memory first, which is actually
hard, because compilers like to optimize unused allocations away. So we
turn the existing code into one that uses an allocation instead of an
array on the stack.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d83474e81bb
Move Sprintf.h and IntegerPrintfMacros.h next to Printf.h. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/f85934a2b7ad
Add support for the j length modifier in Printf.cpp. r=Gankra
https://hg.mozilla.org/integration/autoland/rev/7e925e90a251
Add support for the hh length modifier in Printf.cpp. r=nika,Gankra
https://hg.mozilla.org/integration/autoland/rev/3b2bebed9128
Change VsprintfLiteral/SprintfLiteral to rely on PrintfTarget. r=nika,Gankra,firefox-build-system-reviewers,mhentges
https://hg.mozilla.org/integration/autoland/rev/d28c0f11743f
Allocate some memory before running RandomNum tests. r=Gankra

Comment 10

[Andreea Pavel [:apavel]]

Backed out for failing xpcshell at bootstrapSvc.js

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Xzo32zoFTIyj9D3sUH_V-Q.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=d28c0f11743f57dd91193e9e9d0a7a28b2d36f9b

Failure log: https://treeherder.mozilla.org/logviewer?job_id=330191582&repo=autoland&lineNumber=3373

Backout: https://hg.mozilla.org/integration/autoland/rev/c9c31702a4fd890741b1bc9ac7f8e14383d486bc

Comment 11

[Mike Hommey [:glandium]]

It's the linking of mozglue to the updater code that causes those: https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=OY4aNoGHT1C9qaCKHKcyWg.0&revision=0ec49bdcb138e609f4cee0f9e8fbfed872c4f460 (patch: https://hg.mozilla.org/try/rev/9946cb884b4156a14ee84ab58458283115f735af)
I guess the Mozilla Maintenance Service would need to have mozglue installed if we keep the dependency... OTOH, the Windows code for the updater does not use SprintfLiteral...

Comment 12

[Mike Hommey [:glandium]]

Another factor is that the updater on Linux went from 100K to 1M (mostly because of the allocator sneaking in as well), which might not be the best thing.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:glandium, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4529553d5fee
Move Sprintf.h and IntegerPrintfMacros.h next to Printf.h. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/a020e59e7740
Add support for the j length modifier in Printf.cpp. r=Gankra
https://hg.mozilla.org/integration/autoland/rev/fab4467e9520
Add support for the hh length modifier in Printf.cpp. r=nika,Gankra
https://hg.mozilla.org/integration/autoland/rev/5b3af655c87f
Change VsprintfLiteral/SprintfLiteral to rely on PrintfTarget. r=nika,Gankra,firefox-build-system-reviewers,mhentges
https://hg.mozilla.org/integration/autoland/rev/dbccd0a00984
Allocate some memory before running RandomNum tests. r=Gankra

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4529553d5fee
https://hg.mozilla.org/mozilla-central/rev/a020e59e7740
https://hg.mozilla.org/mozilla-central/rev/fab4467e9520
https://hg.mozilla.org/mozilla-central/rev/5b3af655c87f
https://hg.mozilla.org/mozilla-central/rev/dbccd0a00984

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

FYI, I believe that this has caused bug 1710757 (in baseprofiler, SprintfLiteral(buf, "%#llx", number) doesn't output "0x<number>" anymore), which made symbolication of profiles fail.
TIL: "%#..." allows an alternative output! I now see that PrintfTarget doesn't support that like vsnprintf does.

I'm fixing bug 1710757 there by just manually doing SprintfLiteral("0x%llx"..., easy enough.
And I don't see any other uses of "%#" with SprintfLiteral in our codebase, so I think it should be safe for now.

But that's something you may want to investigate (in case there are other things than "%#" that are different), or at least document?