The default bug view has changed. See this FAQ.

Building SpiderMonkey with clang warns up the wazoo

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #571413 - Flags: review?(luke)
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.
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

6 years ago
Gross!  offsetof(T, f.s) is really useful!
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

5 years ago
Cool, keep us updated!

Updated

5 years ago
Attachment #571413 - Flags: review?(luke)
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.
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...
Attachment #571413 - Attachment is obsolete: true
Attachment #573141 - Flags: review?(ted.mielczarek)
Attachment #573141 - Flags: review?(ted.mielczarek) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9d5d2c4d49
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/8b9d5d2c4d49
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.