MSVC build warning for skia, when building gfx/2d/: "SkTypes.h(439) : warning C4003: not enough actual parameters for macro 'free'" (similar for GrGlyph.h), due to mozalloc_macro_wrappers.h

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 2 bugs)

Trunk
mozilla28
x86_64
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments)

Just tried to build gfx/2d with FAIL_ON_WARNINGS, but I hit these build warnings on windows: 
{
DrawTargetSkia.cpp
e:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\dist\include\skia\GrGlyph.h(42) : error C2220: warning treated as error - no 'object' file generated
e:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\dist\include\skia\GrGlyph.h(42) : warning C4003: not enough actual parameters for macro 'free'
2
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=28861021&tree=Try

{
PathD2D.cpp
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTypes.h(439) : error C2220: warning treated as error - no 'object' file generated
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTypes.h(439) : warning C4003: not enough actual parameters for macro 'free'
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTypes.h(505) : warning C4003: not enough actual parameters for macro 'free'
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTemplates.h(96) : warning C4003: not enough actual parameters for macro 'free'
e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTemplates.h(122) : warning C4003: not enough actual parameters for macro 'free'
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=28861029&tree=Try#error0
(It looks like it's confusing the "free()" function defined in these methods with a macro to implement the normal malloc/free functionality.  There's probably a "free_XYZ(void* buf)" function in play, which some header has a "#defines free" pointing to, which messes up any other usage of "free" as a method name.)
(sorry, meant to say 'looks like it's confusing the "free()" method defined in these classes')
like maybe here:
> 18 #define free moz_free
http://mxr.mozilla.org/mozilla-central/source/gfx/graphite2/src/MozGrMalloc.h?rev=7c3cd4824f94#18
or something similar.
OK, the macro definition that's causing trouble is here:
> 19 #define free(_) moz_free(_)
http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc_macro_wrappers.h#12

(We end up including that via BaseRect.h's #include of nsDebug.h, which #includes nscore.h, which #includes mozalloc_macro_wrappers.h )
Summary: MSVC build warning for skia, when building gfx/2d/: "SkTypes.h(439) : warning C4003: not enough actual parameters for macro 'free'" (similar for GrGlyph.h) → MSVC build warning for skia, when building gfx/2d/: "SkTypes.h(439) : warning C4003: not enough actual parameters for macro 'free'" (similar for GrGlyph.h), due to mozalloc_macro_wrappers.h
So, I see two possible solutions:

 (1) Avoid exposing the "#define free" macro to SkTypes.h and GrGlyph.h (e.g. by #including them before anything else, or by adding an #undef)

...or...

 (2) Rename the "free()" methods on SkTypes.h and GrGlyph.h to something else (e.g. freePtr() and freeGlyph()), and rename all of their callers in Skia code.

I lean towards option 1, I think, because it seems bogus that we're randomly evaluating Skia headers with a different "free" definition, depending on whether nsDebug.h (and friends) are included, and in what order.
Actually, probably the best thing to do would be to remove "#include nsDebug.h" from BaseRect.h.  I think that might be what's causing this.

(That include was added in bug 914919, so that we could downgrade some MOZ_ASSERT assertions to be non-fatal NS_ASSERTION (the only nonfatal assertion we have at the moment). Ms2ger then pointed out in bug 914919 comment 15 that we technically shouldn't have NS_* junk in gfx/2d, and roc points out in the next comment that this is (currently) the only way to get nonfatal assertions, but we should use MOZ_NONFATAL_ASSERT if/when it becomes available.)
Blocks: 914919
(Even with BaseRect fixed, we hit another #include-nsDebug-before-skia issue in convolver.cpp and image_operations.cpp, both of which include nsAlgorithm.h, which unnecessarily includes nsDebug.h. I filed bug 925066 on fixing that.)
I've normally been #undefing free in this situation elsewhere, and it's been making me sadface :(

Any better solution would be most welcome!
yeah... "#undef free" scares me, because it seems like that could make us use unmatched malloc/free implementations and potentially do something bad, in the unlikely event that we actually bother to call malloc/free.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> (Even with BaseRect fixed, we hit another #include-nsDebug-before-skia issue
> in convolver.cpp and image_operations.cpp

Ah, so these *also* get the "free" macro definition via their includes of basictypes.h, because that header ends with:
> 252 #include "nscore.h"             // pick up mozalloc operator new() etc.
http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/basictypes.h
Depends on: 925140
OK, I've got a 3-patch patch stack (with trivial patches, just reordering #includes) to fix comment 7.
So as noted in comment 10, we're getting the "#define free" via basictypes.h.

If we include SkTypes before that, we're fine. This makes that change for convolver.h, which fixes the convolver.cpp issues.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #816274 - Flags: review?(gwright)
This makes image_operations.cpp include its own header first (which gets it a SkTypes.h include), before it includes basictypes.h and gets the problematic #define.

(This makes us also align with our general strategy of making .cpp files include their own .h file first.)
Attachment #816275 - Flags: review?(gwright)
image_operations.cpp also hits this issue for "SkTemplates.h" which gets pulled in via skia/SkBitmap.h.

So, this patch makes us include SkBitmap a bit earlier (before basictypes.h), which keeps it from getting thrown by the #define free.
Attachment #816276 - Flags: review?(gwright)
Depends on: 926275
Actually, bug 926275 might be a better way to address this.
Comment on attachment 816274 [details] [diff] [review]
part 1: make convolver.h include SkTypes before it sees the #define free

Canceling review requests, since IIUC bug 926275 is really the right fix for this.
Attachment #816274 - Flags: review?(gwright)
Attachment #816275 - Flags: review?(gwright)
Attachment #816276 - Flags: review?(gwright)
This was FIXED by bug 926275.
Assignee: dholbert → nobody
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee: nobody → dholbert
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.