Closed Bug 1235186 Opened 4 years ago Closed 4 years ago

Fix -Wformat warnings in layout/

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(3 files)

These -Wformat warnings were revealed by a local patch I have to mark MOZ_LOG and PR_LogPrint as printf-style functions (with `__attribute__ ((format (printf,...)))`) so gcc/clang can type-check the format string's arguments.

layout/base/AccessibleCaret.cpp:129:48 [-Wformat-extra-args] data argument not used by format string

layout/generic/nsContainerFrame.cpp:425:71 [-Wformat] more '%' conversions than data arguments

layout/style/FontFaceSet.cpp:798:10 [-Wformat] format specifies type 'int' but the argument has type 'size_type' (aka 'unsigned long')

layout/style/Loader.cpp:1817:12 [-Wformat] format specifies type 'unsigned int' but the argument has type 'nsICSSLoaderObserver *'

layout/style/Loader.cpp:1817:35 [-Wformat] format specifies type 'unsigned int' but the argument has type 'mozilla::css::SheetLoadData *'

layout/style/Loader.cpp:1827:12 [-Wformat] format specifies type 'unsigned int' but the argument has type 'nsICSSLoaderObserver *'

layout/style/Loader.cpp:1827:23 [-Wformat] format specifies type 'char *' but the argument has type 'mozilla::css::SheetLoadData *'
Attachment #8702031 - Flags: review?(dholbert)
Part 2: Fix -Wclass-varargs warning in layout/

layout/base/nsLayoutDebugger.cpp:189:36: warning: passing object of class type 'AnimatedGeometryRoot' through variadic constructor [-Wclass-varargs]
Attachment #8702032 - Flags: review?(dholbert)
Comment on attachment 8702032 [details] [diff] [review]
1235186_part-2_layout_Wclass-varargs.patch

This "part 2" warning looks definitely-sane -- the format-string has %p for this thing, so we should be providing a pointer, not a dereferenced pointer.

hg blame says the buggy dereferencing here was a very recent (and likely-accidental) change that got slipped in with bug 1222880, here:
 https://hg.mozilla.org/mozilla-central/rev/78381aa444d4#l4.22

mattwoodrow, would it be worth backporting this fix here to Aurora 45? (the only non-Nightly branch with this issue)  Not sure what this printing code is used for, or what happens when we pass in an actual object here for a %p format string (probably just the first few bytes of the object get printed as a pointer?)  To the extent that we care about this printing code, it seems like we might as well fix it so that it's printing something useful.  And I expect backport approval will be easy to get.
Flags: needinfo?(matt.woodrow)
Attachment #8702032 - Flags: review?(dholbert) → review+
Comment on attachment 8702031 [details] [diff] [review]
part-1_layout_Wformat.patch

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

r=me on part 1, too.
Attachment #8702031 - Flags: review?(dholbert) → review+
It might be worth spinning part 2 off into its own bug, since IMO we should consider backporting it per comment 2, and a partial-backport on this bug (part 2 but not part 1) would be kinda confusing.
https://hg.mozilla.org/mozilla-central/rev/e36c0185f1fd
https://hg.mozilla.org/mozilla-central/rev/798ec66ec31c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 8702032 [details] [diff] [review]
> 1235186_part-2_layout_Wclass-varargs.patch
> 
> This "part 2" warning looks definitely-sane -- the format-string has %p for
> this thing, so we should be providing a pointer, not a dereferenced pointer.
> 
> hg blame says the buggy dereferencing here was a very recent (and
> likely-accidental) change that got slipped in with bug 1222880, here:
>  https://hg.mozilla.org/mozilla-central/rev/78381aa444d4#l4.22
> 
> mattwoodrow, would it be worth backporting this fix here to Aurora 45? (the
> only non-Nightly branch with this issue)  Not sure what this printing code
> is used for, or what happens when we pass in an actual object here for a %p
> format string (probably just the first few bytes of the object get printed
> as a pointer?)  To the extent that we care about this printing code, it
> seems like we might as well fix it so that it's printing something useful. 
> And I expect backport approval will be easy to get.

The change was intentional, unfortunately the change didn't quite do what was intended. :)

nsDisplayItem::GetAnimatedGeometryRoot went from returning an nsIFrame* to a AnimatedGeometryRoot*. AnimatedGeometryRoot being a struct that holds a nsIFrame* and and a AnimatedGeometryRoot* pointing a the parent agr. The intention was to continue printing the nsIFrame* by using the overloaded operator nsIFrame* on struct AnimatedGeometryRoot. This worked everywhere else in the patch, but unfortunately not in printf when it is expecting a "%p" and not a nsIFrame*.

We got lucky though in that the correct thing still got printed because interpreting a struct AnimatedGeometryRoot as a pointer means we just access it's first field, which is the nsIFrame* that we wanted.

I'll file a bug and patch to fix this the proper way.
Flags: needinfo?(matt.woodrow)
Depends on: 1235678
Comment on attachment 8702031 [details] [diff] [review]
part-1_layout_Wformat.patch

>    if (LOG_ENABLED() && !mRuleFaces.IsEmpty()) {
> -    LOG(("userfonts (%p) userfont rules update (%s) rule count: %d",
> +    LOG(("userfonts (%p) userfont rules update (%s) rule count: %" PRIuSIZE,
>           mUserFontSet.get(),
>           (modified ? "modified" : "not modified"),
>           mRuleFaces.Length()));
>    }

Looks like this wasn't tested. Instead of printing a number this code now omits:

userfonts (15278290) userfont rules update (modified) rule count: %Iu

So it would appear that the code below isn't correct for the SDK version used in the Windows build:

https://dxr.mozilla.org/mozilla-central/source/mfbt/SizePrintfMacros.h#23

Chris, Jeff, suggestions?
Flags: needinfo?(cpeterson)
Flags: needinfo?(jwalden+bmo)
John, what version of VS and Windows are you running?

I didn't actually execute this LOG call on Windows. Just now, I ran a simple test program on Windows 10, compiled with VS2015. printf("%" PRIuSIZE, size) printed the expected result with PRIuSIZE #defined as either "Iu" or "zu". Could this LOG(()) call be hitting some preprocessor weirdness with string concatenation within LOG's parenthesized parameter list?

I was surprised that "%zu" worked because that contradicts what I've read online. Maybe "%zu" support is new in Windows 10's CRT? Here is a 2015 Chromium bug report about "%zu" aborting on (unspecified versions of) Windows:

https://codereview.chromium.org/849193002/

And Chromium's PRIuS macro is #defined as "Iu", like our PRIuSIZE:

https://chromium.googlesource.com/chromium/src/+/master/base/format_macros.h#92
Flags: needinfo?(cpeterson)
(In reply to Chris Peterson [:cpeterson] from comment #9)
> John, what version of VS and Windows are you running?

This isn't my own build, this is the nightly build running under Windows 8.1!! So the real question here is what is being used by the builds? One thing to note is that there are several instances of PRIuSIZE in our code so this may indeed be something to do with the LOG() macro.
What's the result of

  static_assert(mozilla::IsSame<decltype(mRuleFaces.Length()), size_t>::value,
                "???");

Code-reading says they should be the same, and if this passed, it'd confirm the format string wasn't being interpreted right by whatever does the formatting.

Frankly, I think it's misguided to try to use any of the PRI* macros with anything but the actual C/C++ *printf APIs.  If LOG doesn't turn into a call to printf under the hood, it seems very wrong to me to try to use any of the standard macros -- they're meant *only* for use with those methods, not with anything else that might use any other convention.  If printf isn't being used here, you probably should cast to unsigned long long and use whatever LOG demands as the specifier for that type.
Flags: needinfo?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> Frankly, I think it's misguided to try to use any of the PRI* macros with
> anything but the actual C/C++ *printf APIs.  If LOG doesn't turn into a call
> to printf under the hood, it seems very wrong to me to try to use any of the
> standard macros -- they're meant *only* for use with those methods, not with
> anything else that might use any other convention.  If printf isn't being
> used here, you probably should cast to unsigned long long and use whatever
> LOG demands as the specifier for that type.

OK. That makes sense. I can fix that.
As discussed with Chris, add a cast to userfont logging. In this case, the size value is the number of fonts in a document so it will rarely go over a relatively small number and it's only for tracking down font problems.
Attachment #8707679 - Flags: review?(m_kato)
Comment on attachment 8707679 [details] [diff] [review]
follow-up patch, add a cast to userfont logging

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

Both VS2013 and VS2015 supports "Iu" as size_t as long as I test.

But, LOG() will use NSPR's dosprintf.  dosprintf supports "zu" as size_t.  So we should check that PRIuSIZE doesn't use for LOG() by another bug.
Attachment #8707679 - Flags: review?(m_kato) → review+
Blocks: 1239539
You need to log in before you can comment on or make changes to this bug.