Last Comment Bug 699185 - Building SpiderMonkey with clang warns up the wazoo
: Building SpiderMonkey with clang warns up the wazoo
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla11
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-02 12:29 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-11-11 02:25 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (14.19 KB, patch)
2011-11-02 12:29 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Patch to use -Wno-extended-offsetof (13.64 KB, patch)
2011-11-09 01:48 PST, Jeff Walden [:Waldo] (remove +bmo to email)
ted: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-02 12:29:59 PDT
Created attachment 571413 [details] [diff] [review]
Patch

Most or all of the warnings really come down to one problem: in |offsetof(T, f)|, |f| is technically required to be an identifier, and it can't be something like |f.s| or |arr[3]| or |f.arr[3]|.

It's easy to decompose such offsetofs into smaller ones going field by field.  Unfortunately that loses type-safety, because if someone changes the type of a field the offsetofs may not match reality.  So we'll need something to preserve type-safety.  Here's a stab at that.  Thoughts on something prettier appreciated.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-02 12:37:28 PDT
There are a few more offsetofs that are problematic in this way, but they act upon jsval_layout, and the anonymous structs/unions in play there aren't named similarly across platforms, so it's not trivial to fix that problem now.  I figure getting this done is better than delaying it all for that harder problem.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-02 12:56:07 PDT
Clang generates this warning because of the -pedantic flag, as I understand it.  There's no -W flag to disable it: see llvm/tools/clang/test/Misc/warning-flags.c, it's the ext_offsetof_extended_field_designator item.  Maybe some enterprising Clang hacker could fix this?  *wink* *wink*, *nudge* *nudge* ;-)
Comment 3 Luke Wagner [:luke] 2011-11-06 17:00:38 PST
Gross!  offsetof(T, f.s) is really useful!
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-07 13:40:01 PST
I did enough digging to cargo-cult up a patch to add -Wno-offsetof-extended-field-designator to clang:

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-November/018541.html

So hopefully the patch here won't be necessary after that lands, and after we implement the equivalent of bug 450194 when compiling with clang.
Comment 5 Luke Wagner [:luke] 2011-11-07 14:01:32 PST
Cool, keep us updated!
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-09 00:51:22 PST
Callooh callay!  Clang now supports -Wno-extended-offsetof to disable this warning.

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-November/018594.html

Some of the changes in the patch here -- moving the offsetofs into static methods on the relevant classes -- may not be a bad idea.  But we can fork those concerns off to a separate bug, leaving this one to its original purpose.

I should have a patch to disable this warning in clang builds in the morning.  There's a smidgen of tricky here because I'll have to reorder the command line flags so that -pedantic precedes -Wno-extended-offsetof, but I'm hoping doing so won't cause any problems.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-09 01:48:10 PST
Created attachment 573141 [details] [diff] [review]
Patch to use -Wno-extended-offsetof

Sigh; I am a glutton for getting stuff done when the end's just almost in sight.

If we were to arrange these checks logically, I think -Wall and -pedantic would be the first things set.  Then we'd set all the extra warnings deemed desirable.  Then we'd opt out of any warnings we didn't actually want.  That's not at all what we do now, naturally.  I moved things slightly in this direction by putting -pedantic at the start of the compiler flags strings rather than at the end, where it can't easily be overridden.

http://clang.llvm.org/docs/LanguageExtensions.html#__has_warning is a useful reference for clang's __has_warning macro.  It's a clever idea; too bad gcc doesn't have some sort of similar macro.

With this patch I can all but build SpiderMonkey warning-free.  I now get one or so from a valgrind macro (I understand a valgrind upgrade would fix this), and I've sometimes seen one or two from tracer/nanojit code that's going away Real Soon Now.  But other than that, SpiderMonkey's as warning-free with trunk clang as with gcc.  \o/

And with that, really good night.  (Or morning for some of us on the other coast with insane hours.  ;-) )  Zzz...
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-10 22:29:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9d5d2c4d49
Comment 9 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-11-11 02:25:13 PST
https://hg.mozilla.org/mozilla-central/rev/8b9d5d2c4d49

Note You need to log in before you can comment on or make changes to this bug.