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)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(1 file, 3 obsolete files)
1.31 KB,
patch
|
bobowen
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
(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
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
Isn't that Windows only?
Assignee | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8726785 -
Attachment is obsolete: true
Attachment #8726785 -
Flags: review?(bobowen.code)
Assignee | ||
Comment 13•8 years ago
|
||
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 14•8 years ago
|
||
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+
Assignee | ||
Comment 16•8 years ago
|
||
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-
Assignee | ||
Comment 17•8 years ago
|
||
(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. :))
Comment 18•8 years ago
|
||
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. :)
Comment 19•8 years ago
|
||
bugherder |
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.
Description
•