I'm trying to build with clang 3.9 (on Linux64), and I got these build errors (from a build warning being promoted to an error due to --enable-warnings-as-errors in my mozconfig): { obj/dom/bindings/TestCodeGenBinding.cpp:6236:7: error: code will never be executed [-Werror,-Wunreachable-code] ThrowErrorMessage(cx, MSG_PERMISSION_DENIED_TO_PASS_ARG, "return value of TestObjectReturn"); ^~~~~~~~~~~~~~~~~ obj/dom/bindings/TestCodeGenBinding.cpp:6265:7: error: code will never be executed [-Werror,-Wunreachable-code] ThrowErrorMessage(cx, MSG_PERMISSION_DENIED_TO_PASS_ARG, "return value of TestNullableObjectReturn"); ^~~~~~~~~~~~~~~~~ } The generated code in question looks like this: > void > TestObjectReturn::Call(JSContext* cx, JS::Handle<JS::Value> aThisVal, JS::MutableHandle<JSObject*> aRetVal, ErrorResult& aRv) > { > [...] > if (rval.isObject()) { > if ((false) && !CallerSubsumes(rval)) { > ThrowErrorMessage(cx, MSG_PERMISSION_DENIED_TO_PASS_ARG, "return value of TestObjectReturn"); > aRv.Throw(NS_ERROR_UNEXPECTED); > return; I think the "if ((false) && ..." is the problem there. Looks like this code is generated from the "TestObjectReturn" and "TestNullableObjectReturn" in TestCodeGen.webidl: https://hg.mozilla.org/mozilla-central/annotate/ffac2798999c5b84f1b4605a1280994bb665a406/dom/bindings/test/TestCodeGen.webidl#l89 ...which was added here as part of bug 779048: https://hg.mozilla.org/mozilla-central/rev/e32756c083ef#l4.27

(bz, seems like this is in your wheelhouse, though feel free to punt if you're backlogged) Here's a link directly to the "if ((false)" check in the generated code on DXR, fwiw (takes a little while to load, as the file is huge): https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dom/bindings/TestCodeGenBinding.cpp#6235

So the codegen template here looks like this: """ if (($${passedToJSImpl}) && !CallerSubsumes($${val})) { ThrowErrorMessage(cx, MSG_PERMISSION_DENIED_TO_PASS_ARG, "${sourceDescription}"); $*{exceptionCode} } """, with a preceding comment like so: # For JS-implemented APIs, we refuse to allow passing objects that the # API consumer does not subsume. The extra parens around # ($${passedToJSImpl}) suppress unreachable code warnings when # $${passedToJSImpl} is the literal `false`. There are some cases in which that template is instantiated with a non-literal (a function argument, in fact) for the passedToJSImpl bit, and other cases in which it's a literal boolean, "true" or "false". Rejiggering codegen to change the entire condition here based on which of those three cases we're in would be possible, probably, but rather nontrivial. :( It's a bit sad that the () thing no longer works.

I guess the question is whether we can do something else to suppress the warning here. That would be ideal...

(Side note: I get a bunch of these same warnings/errors for TestJSImplGenBinding.cpp, too, but I didn't initially notice them because my build errored out before I hit them.)

Ok, I misread the code here. I see now why Boris said this would be hard to fix in the code gen. Chris, do you have any ideas how we could easily suppress this warning here, if somehow Clang decided to get rid of their parentheses workaround?

(In reply to Andrew McCreight [:mccr8] from comment #5) > Ok, I misread the code here. I see now why Boris said this would be hard to > fix in the code gen. > > Chris, do you have any ideas how we could easily suppress this warning here, > if somehow Clang decided to get rid of their parentheses workaround? Daniel, does moving the parentheses from `(false)` to the `!CallerSubsumes(rval)` expression or the entire `false && !CallerSubsumes(rval)` expression suppress the -Wunreachable-code warning? I don't have clang 3.9 or a Linux build environment, so I can't test this. If that doesn't work, you could suppress the -Wunreachable-code and -Wunreachable-code-return warnings in dom/bindings/moz.build or in the generated code with pragmas at the head of the generated source file: #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wunreachable-code" #pragma clang diagnostic ignored "-Wunreachable-code-return" #pragma clang diagnostic pop

(In reply to Chris Peterson [:cpeterson] from comment #6) > (In reply to Andrew McCreight [:mccr8] from comment #5) > > Ok, I misread the code here. I see now why Boris said this would be hard to > > fix in the code gen. > > > > Chris, do you have any ideas how we could easily suppress this warning here, > > if somehow Clang decided to get rid of their parentheses workaround? > > Daniel, does moving the parentheses from `(false)` to the > `!CallerSubsumes(rval)` expression or the entire `false && > !CallerSubsumes(rval)` expression suppress the -Wunreachable-code warning? I > don't have clang 3.9 or a Linux build environment, so I can't test this. Hit this today and I tried to move the parentheses but that didn't work.

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #7) > Hit this today and I tried to move the parentheses but that didn't work. That's too bad. I guess suppressing the warnings in dom/bindings/moz.build for this directory is the way to go. I would submit a patch, but I don't have a clang 3.9 or a Linux build environment to test. I wonder why clang made this change. Perhaps this is an unintentional clang regression and should be reported?

(In reply to Chris Peterson [:cpeterson] from comment #8) > I wonder why clang made this change. Perhaps this is an unintentional clang > regression and should be reported? Looking at the old bug where you were enabling this warning, it was printing out something about a way to suppress the warning. If in fact it isn't doing this any more (I assume dholbert would have mentioned it as a way to fix this issue), I'd assume it is an intentional change. I guess it wouldn't hurt to complain, though.

I think it's unintentional, because the trivial test case still works as expected: void test(int a) { if (false && !someTest()) { doSomething(a); return; } doSomething(a); return; } test.cpp:8:7: note: silence by adding parentheses to mark code as explicitly dead if (false && !someTest()) { ^ /* DISABLES CODE */ ( )

I asked [1] on the clang's cfe-users mailing list whether this was an intentional change. According to the reply [2], the change was not intentional, but as Kan-Ru demonstrated in comment 10, the warning suppression still works correctly in a simple test case. If someone with clang-3.9/ToT can extract a smaller test case of the unsuppressable warning from TestCodeGenBinding.cpp, I can share it on the cfe-users mailing list or file a clang bug. [1] http://lists.llvm.org/pipermail/cfe-users/2016-August/001000.html [2] http://lists.llvm.org/pipermail/cfe-users/2016-August/001003.html

The warning only seems to happen when the boolean condition involves an expression with a templated type, I think. (In this testcase, at least, a templated local variable gets copy-constructed to form an argument to a boolean helper-function.) I get the following output when compiling this program: > $ clang++ -Wunreachable-code /tmp/clang-warning.cpp > /tmp/clang-warning.cpp:23:5: warning: code will never be executed [-Wunreachable-code] > printf("Does clang warn about this code being unreachable?\n"); > ^~~~~~ > 1 warning generated.

I get that same warning (with this reduced C++ testcase) in all clang versions I've tested, though, FWIW. (I tested my recent-SVN-trunk build, clang 3.8, clang 3.7, and clang 3.6.) Not sure why this hasn't caused problems for me in this generated code until now. In any case, hopefully that testcase will help clang developers to isolate the bug!

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #13) > Not sure why this hasn't caused problems for me in this generated code until now. I think the answer to this is that something about how clang handles "JS::Rooted<JS::Value> rval" -- the variable in question in TestCodeGenBinding.cpp -- must've changed to make it behave more like how clang handles std::vector. That's why this seems to have "regressed". Really, this is an old clang bug, but the variable "JS::Rooted<JS::Value> rval" only started triggering it in clang 3.9. (whereas std::vector triggers it in old versions as well.)

I filed a clang bug: https://llvm.org/bugs/show_bug.cgi?id=28918

So are we just waiting on a clang fix?

(In reply to Andrew Overholt [:overholt] from comment #18) > So are we just waiting on a clang fix? We're waiting for a clang fix or a one-line Gecko patch to suppress this warning in dom/bindings/moz.build. I don't have a Linux build environment or clang 3.9 to test.

There are two clang issues here: (a) https://llvm.org/bugs/show_bug.cgi?id=28918 is about a generalization of this issue, which reproduces in old clang versions as well. (b) https://llvm.org/bugs/show_bug.cgi?id=28994 is specifically about the behavior-change in clang 3.9 -- I've just come up with a reduced testcase for that and posted it there. The latter bug is the more important one to get fixed, since it's about the actual behavior change that's breaking our build. But it also might just be a valid code-expansion that just happens to create a new opportunity for the earlier bug to crop up -- so I don't know if it will get its own dedicated fix, or if it'll just be duped. We'll see what happens on the clang bugs, anyway.

The second clang issue from comment 20 (the one that's an actual regression) has now been fixed. I just tested a locally-compiled clang built from revision 285863 (which was trunk, as of earlier today), and I was able to build with --enable-warnings-as-errors & without any special hackarounds for this specific bug. So, we can close out this bug! Not sure what the correct resolution is here, for a bug in an external piece of our toolchain that may-or-may-not-require-working-around-on-our-side. (INVALID is the default resolution for "this is a bug in another piece of software", but I'm not sure that makes sense when it's another piece of software that we depend on and sometimes land fixes/workarounds to play nicely with.) Anyway, I guess I'll reclassify this as "build config" and resolve as WORKSFORME.

I'm going to reopen this. It looks like Apple shipped the buggy clang in Xcode 8.3, so this is going to cause major grief for people. :(

Yeah, see bug 1349268 comment 10 - 11. :-/ This was fixed (in clang source code) before clang 3.9 was released, but it didn't get uplifted to the clang 3.9 release branch. It might get uplifted for a dot release, but we should probably work around it somehow in the meantime.

MozReview-Commit-ID: 49DfFFQGOsP

Comment on attachment 8853491 [details] [diff] [review] Work around clang bug that they didn't actually manage to ship a fix for which causes it to give spurious warnings it shouldn't be giving, which are then fatal due to -Werror Review of attachment 8853491 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the typo fixed ::: dom/bindings/Codegen.py @@ +4690,5 @@ > # For JS-implemented APIs, we refuse to allow passing objects that the > # API consumer does not subsume. The extra parens around > # ($${passedToJSImpl}) suppress unreachable code warnings when > + # $${passedToJSImpl} is the literal `false`. But Apple is shipping a > + # buggy clan in Xcode 8.3, so there even the parens are not enough. So s/clan/clang/ Might also be worth mentioning the known-bad clang version here ("clang 3.9)", if you want, so that it'll be easier in the future for us to know make a judgement call about when we can remove this hackaround. (clang 3.9 is the only affected version -- clang 3.8 was before the regression, and clang 4.0 has the fix.) @@ +4698,5 @@ > """ > + #ifdef __clang__ > + #pragma clang diagnostic push > + #pragma clang diagnostic ignored "-Wunreachable-code" > + #pragma clang diagnostic ignored "-Wunreachable-code-return" (I'm assuming these warnings are recognized in all versions of clang that we support building with. If there's some old clang version we care about that lacks support for these warnings, though, then this would trigger a new warning for "unrecognized warning group" or somesuch in that version of clang. That's OK though -- if anyone reports issues about that, we can just make this condition a bit more special with a "__has_warning" directive, as we did in bug 1339921.) @@ +5906,5 @@ > # For JS-implemented APIs, we refuse to allow passing objects that the > # API consumer does not subsume. The extra parens around > # ($${passedToJSImpl}) suppress unreachable code warnings when > + # $${passedToJSImpl} is the literal `false`. But Apple is shipping a > + # buggy clan in Xcode 8.3, so there even the parens are not enough. So As above: s/clan/clang, and consider mentioning the version number (3.9).

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cb9742f4629f Work around clang bug that they didn't actually manage to ship a fix for which causes it to give spurious warnings it shouldn't be giving, which are then fatal due to -Werror. r=dholbert