Closed Bug 1253194 Opened 8 years ago Closed 8 years ago

security/sandbox/chromium/base/third_party/icu/icu_utf.cc:162:9: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough], with warnings as errors & bug 1253170,

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 3 obsolete files)

STR:
 1. Be using Clang as your compiler on linux (I'm using clang 3.8 on Ubuntu 15.10 64-bit)
 2. Apply cpeterson's patch from bug 1253170 (currently attachment 8726095 [details] [diff] [review])
 3. Build with this in your mozconfig:
ac_add_options --enable-debug --disable-optimize
ac_add_options --enable-warnings-as-errors

ACTUAL RESULTS:
Build error like the folowing:
{
30:33.30 ../../../../mozilla/security/sandbox/chromium/base/third_party/icu/icu_utf.cc:162:9: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
30:33.30         case 2:
30:33.30         ^
30:33.30 ../../../../mozilla/security/sandbox/chromium/base/third_party/icu/icu_utf.cc:162:9: note: insert '[[clang::fallthrough]];' to silence this warning
30:33.30         case 2:
30:33.30         ^
30:33.30         [[clang::fallthrough]];
30:33.30 ../../../../mozilla/security/sandbox/chromium/base/third_party/icu/icu_utf.cc:162:9: note: insert 'break;' to avoid fall-through
30:33.30         case 2:
30:33.30         ^
30:33.31         break;
30:33.31 ../../../../mozilla/security/sandbox/chromium/base/third_party/icu/icu_utf.cc:166:9: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
30:33.31         case 1:
30:33.31         ^
30:33.31 ../../../../mozilla/security/sandbox/chromium/base/third_party/icu/icu_utf.cc:166:9: note: insert '[[clang::fallthrough]];' to silence this warning
30:33.31         case 1:
30:33.31         ^
30:33.31         [[clang::fallthrough]];
30:33.31 ../../../../mozilla/security/sandbox/chromium/base/third_party/icu/icu_utf.cc:166:9: note: insert 'break;' to avoid fall-through
30:33.31         case 1:
30:33.31         ^
30:33.31         break;
30:33.32 2 errors generated.
}

EXPECTED RESULTS:
No build error -- either because we address the build warning if we can, or disable fatal warnings (or disable this warning) for this directory, as-appropriate.
Here's a quote of the code that clang is talking about:

152         case 3:
153             trail=s[(i)++];
154             (c)=((c)<<6)|(trail&0x3f);
155             if(c<0x110) {
156                 illegal|=(trail&0xc0)^0x80;
157             } else {
158                 /* code point>0x10ffff, outside Unicode */
159                 illegal=1;
160                 break;
161             }
162         case 2:
163             trail=s[(i)++];
164             (c)=((c)<<6)|(trail&0x3f);
165             illegal|=(trail&0xc0)^0x80;
166         case 1:
167             trail=s[(i)++];
168             (c)=((c)<<6)|(trail&0x3f);
169             illegal|=(trail&0xc0)^0x80;
170             break;
171         case 0:
http://mxr.mozilla.org/mozilla-central/source/security/sandbox/chromium/base/third_party/icu/icu_utf.cc#152

* If we satisfy the "if" check in "case 3", we fall through from case 3 to case 2
* We fall through from case 2 to case 1 regardless.

These are the 2 fallthrough scenarios that clang is warning about.
Ah, there's a comment at the top of this "switch" statement that says the fallthrough is intended:
> 146         /* each branch falls through to the next one */

So we can just add MOZ_FALLTHROUGH annotations here, to tell the compiler that the fallthrough behavior is allowed/expected.  I'll post a patch to do that.
Attached patch fix v1 (obsolete) — Splinter Review
Here's the patch. I included a ton of context, so that you can see the code-comment noted above saying that each branch falls through.

(It'd be nice to get this fixed soon, since it may be the only thing keeping cpeterson's bug 1253170 from landing.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8726442 - Flags: review?(bobowen.code)
(I'm not clear on how thirdparty-flavored this code is, but I'm hoping this fix is OK since it's pretty minimal and it doesn't change behavior.)

Note that we do have several existing usages of "mozilla/Attributes.h" in other files that are nearby this one:
http://mxr.mozilla.org/mozilla-central/search?string=mozilla%2FAttributes&find=security%2Fsandbox
I think it might be easier to just suppress -Wimplicit-fallthrough warnings so we don't need to reapply the diffs when updating our snapshot of Chromium's sandbox code. We already suppress some MSVC warnings in security/sandbox/moz.build.
Comment on attachment 8726442 [details] [diff] [review]
fix v1

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

Like Chris said, I'd rather suppress for the security/sandbox/chromium code.

We're trying to keep it as close to the original as possible.

Those mozilla/Attributes.h includes should have been removed when we got rid of some MOZ_IMPLICITs (after Jed had made it so this code was ignored).
That was a review failure from me :-) ... but they'll get blown away when I can finally update (waiting on an MSVC update on build servers).

The code in security/sandbox/linux is ours.
Attachment #8726442 - Flags: review?(bobowen.code)
Having said all that ...
If you can't suppress just for the chromium code, because we don't have a separate moz.build for that.
(Filed bug 1253539 for potentially fixing that.)

Then r+ with a line added to:
security/sandbox/modifications-to-chromium-to-reapply-after-upstream-merge.txt

Thanks.
OK, makes sense -- thanks for the feedback! 

Unassigning & punting to cpeterson, because
 (0) I only posted a patch because it was easy (I'd already written that patch locally, to get past this build error w/ cpeterson's fix.)
 (1) I'm on vacation until Monday, so I won't be able to get to write/test a better fix for at least a few days.
 (2) I've got a backlog of reviews that I really should be focused on once I'm back at work.
 (3) I don't recall the best way to disable newish gcc/clang warnings from a moz.build file. (IIRC, the straightforward way ends up producing *different* warnings in older clang versions, for "unrecognized warning", which is why we do the whole MOZ_CXX_SUPPORTS_WARNING dance in configure.in; So we really have to either test for support before disabling, or figure out the exact clang versions that support this new warning and guard the disabling behind that.)

cpeterson, since you're driving this wimplicit-fallthrough project, I'm hoping you might have more cycles/motivation to push this over the finish line.  (Even if you can't directly test a fix because this directory isn't built on Mac, I expect you could test a fix in some other directory that *is* built, and then just copypaste the fix over to this directory.
Assignee: dholbert → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(cpeterson)
Oh, maybe we don't need the version/support-check after all... I do see us disabling this warning in other moz.build files with no fuss at all, e.g.
> if CONFIG['CLANG_CXX']:
>     CXXFLAGS += ['-Wno-implicit-fallthrough']
http://mxr.mozilla.org/mozilla-central/source/parser/html/moz.build#102

If that's all we need, then I'm still OK finishing this off.
Flags: needinfo?(cpeterson)
Attached patch fix v2 (obsolete) — Splinter Review
I haven't tested with this yet (waiting for a rebuild), but I'm fairly confident it'll do the trick, based on the similar patch for parser/html in http://hg.mozilla.org/mozilla-central/rev/462d202bc50d
Attachment #8726442 - Attachment is obsolete: true
Attachment #8726785 - Flags: review?(bobowen.code)
Isn't that Windows only?
Attached patch fix v3 (more targeted) (obsolete) — Splinter Review
Actually, this is more targeted, and it seems to be the strategy we've used elsewhere when we know there's only one or two files that are affected -- e.g. here:
http://mxr.mozilla.org/mozilla-central/source/gfx/2d/moz.build#98

I've verified locally that this suppresses this build warning [and keeps it from breaking the build when cpeterson's patch turns it on], using clang 3.8.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8726788 - Flags: review?(bobowen.code)
Attachment #8726785 - Attachment is obsolete: true
Attachment #8726785 - Flags: review?(bobowen.code)
Apologies for the review spam -- I'm posting one last version with an updated commit message to note that this is now a targeted fix, rather than a change for all of the security/sandbox code.
Attachment #8726788 - Attachment is obsolete: true
Attachment #8726788 - Flags: review?(bobowen.code)
Attachment #8726790 - Flags: review?(bobowen.code)
Comment on attachment 8726790 [details] [diff] [review]
fix v3a (more targeted, & now with improved commit message)

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

I'm not a peer on the linux sandboxing (or build config for that matter :-) ), but I think it'll be OK in this case.
Attachment #8726790 - Flags: review?(bobowen.code) → review+
Thanks!

I sanity-checked that this patch still works, with an old clang version (clang 3.4) -- which (to my surprise) does actually support this warning.  Maybe it's supported back a long ways, which is how we get away with not bothering to check clang versions.

Landed, per previous comment.
Flags: in-testsuite-
(In reply to Bob Owen (:bobowen) from comment #11)
> Isn't that Windows only?

(Just saw this comment -- good point! I hadn't noticed that I was inside of an CONFIG['OS_ARCH'] == 'WINNT' chunk in that patch. Anyway, fixed in the later patch-version. :))
LGTM. Thanks, Daniel! I wasn't trying to make you fix this bug when I asked to test clang on Linux, but I appreciate your help. :)
https://hg.mozilla.org/mozilla-central/rev/310ca84e24fa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.