Closed Bug 699185 Opened 8 years ago Closed 8 years ago

Building SpiderMonkey with clang warns up the wazoo


(Core :: JavaScript Engine, defect, minor)

Not set





(Reporter: Waldo, Assigned: Waldo)



(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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* ;-)
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:

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.
Cool, keep us updated!
Attachment #571413 - Flags: review?(luke)
Callooh callay!  Clang now supports -Wno-extended-offsetof to disable this warning.

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.
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. 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+
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.