Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
315323002:
Disable Warning 4702 for the PGO builds. (Closed)
|
Created:
6 years, 6 months ago by Sébastien Marchand Modified:
6 years, 6 months ago Reviewers:
Peter Kasting, bradn, bradnelson, scottmg, Nico, Brad Chen (chromium), Mark Seaborn, brettw CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDisable Warning 4702 for the PGO builds.
There's more optimization done on a PGO build than on a regular one, this results in some "Unreachable code" warnings that we don't see normally.
But probably anything that this would catch that wouldn't be caught in a normal build isn't going to actually be a bug, so the incremental value of C4702 for PGO builds is likely very small and blocks the setup of the PGO bots.
BUG=380175
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276737
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove the TODO. #
Total comments: 1
Patch Set 3 : Update the comment. #
Total comments: 6
Patch Set 4 : Revert the change on nacl_listener.cc #Patch Set 5 : Update the GN build file. #
Total comments: 2
Patch Set 6 : address Brett's comment. #
Messages
Total messages: 44 (0 generated)
PTAL (thakis@ and bradnelson@ for owner approval)
Please add some rationale/explanation per the bug discussion to the CL description, then lgtm. https://codereview.chromium.org/315323002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/315323002/diff/1/build/common.gypi#newcode4972 build/common.gypi:4972: # TODO(sebmarchand): Re-enable this once it doesn't trigger I think you can remove this TODO.
Thanks. Waiting for other reviews. https://codereview.chromium.org/315323002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/315323002/diff/1/build/common.gypi#newcode4972 build/common.gypi:4972: # TODO(sebmarchand): Re-enable this once it doesn't trigger On 2014/06/05 21:10:08, scottmg wrote: > I think you can remove this TODO. Done.
LGTM https://codereview.chromium.org/315323002/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/315323002/diff/20001/build/common.gypi#newcod... build/common.gypi:4971: # builds, see crbug.com/380175. Nit: Try to avoid referring to bugs in comments, instead just supply a brief explanation in-line.
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/315323002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/315323002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
+brettw@ for base/ owner's approval +bradchen@ for components/nacl/ owner's approval
+brettw@ for base/ owner's approval +bradchen@ for components/nacl/ owner's approval
https://codereview.chromium.org/315323002/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/315323002/diff/40001/build/common.gypi#newcod... build/common.gypi:4976: 4702 Could you not introduce a define here to allow the unreached markers in question to be gated out only in this context? https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:478: NaClChromeMainStartApp(nap, args); I'm a little uncomfortable with this being removed. Could we not introduce a define and do this conditionally?
https://codereview.chromium.org/315323002/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/315323002/diff/40001/build/common.gypi#newcod... build/common.gypi:4976: 4702 On 2014/06/10 16:42:47, bradn wrote: > Could you not introduce a define here to allow the unreached markers in question > to be gated out only in this context? I could do this, but maintaining a list of these would be pretty painful... The PGO build process takes hours and crash as soon as one of these warning is encountered, so we'll have to spend hours trying to find the lines that trigger this warning for no real value... See crbug.com/380175 for the discussion we had about this with pkasting@ and scottmg@ https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:478: NaClChromeMainStartApp(nap, args); On 2014/06/10 16:42:47, bradn wrote: > I'm a little uncomfortable with this being removed. > Could we not introduce a define and do this conditionally? I could do this but the NaCLChromeMainStartApp function doesn't return and this cause an 'unreachable code' warning to be triggered...
+ mseaborn https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:478: NaClChromeMainStartApp(nap, args); On 2014/06/10 17:03:33, Sébastien Marchand wrote: > On 2014/06/10 16:42:47, bradn wrote: > > I'm a little uncomfortable with this being removed. > > Could we not introduce a define and do this conditionally? > > I could do this but the NaCLChromeMainStartApp function doesn't return and this > cause an 'unreachable code' warning to be triggered... So we have this here for a very particular reason. This is very much an assert. We're setting up nacl at this point, potentially doing non-intuitive things and want to be certain that control flow does not get beyond here. As you're also disabling the unreachable code warning in this change, why is this any issue anyhow?
https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:478: NaClChromeMainStartApp(nap, args); On 2014/06/10 17:35:31, bradn wrote: > On 2014/06/10 17:03:33, Sébastien Marchand wrote: > > On 2014/06/10 16:42:47, bradn wrote: > > > I'm a little uncomfortable with this being removed. > > > Could we not introduce a define and do this conditionally? > > > > I could do this but the NaCLChromeMainStartApp function doesn't return and > this > > cause an 'unreachable code' warning to be triggered... > > So we have this here for a very particular reason. > This is very much an assert. We're setting up nacl at this point, potentially > doing non-intuitive things and want to be certain that control flow does not get > beyond here. > > As you're also disabling the unreachable code warning in this change, why is > this any issue anyhow? I've address Peter's comment her https://code.google.com/p/chromium/issues/detail?id=380175#c29 ("We should kill this particular NOTREACHED() regardless -- it's legitimately unreachable and shouldn't be in the codebase"), but if you feel that there's a strong reason for it to be there then I could restore it... It's just that it might potentially break the official build one day (as the 'unreachable code' warning isn't turned off on it, so the compiler might realize that this code can't be reached...)
On 2014/06/10 17:35:31, bradn wrote: > + mseaborn > > https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... > File components/nacl/loader/nacl_listener.cc (right): > > https://codereview.chromium.org/315323002/diff/40001/components/nacl/loader/n... > components/nacl/loader/nacl_listener.cc:478: NaClChromeMainStartApp(nap, args); > On 2014/06/10 17:03:33, Sébastien Marchand wrote: > > On 2014/06/10 16:42:47, bradn wrote: > > > I'm a little uncomfortable with this being removed. > > > Could we not introduce a define and do this conditionally? > > > > I could do this but the NaCLChromeMainStartApp function doesn't return and > this > > cause an 'unreachable code' warning to be triggered... > > So we have this here for a very particular reason. > This is very much an assert. We're setting up nacl at this point, potentially > doing non-intuitive things and want to be certain that control flow does not get > beyond here. Then you're doing it wrong. NOTREACHED() does not "make certain control flow does not get here", it says definitively that control flow never reaches there, and it's documentation for the reader, not some kind of actual functional assertion. If you need anything in the code here, it would be something based on CHECK instead, to force a crash if control flow reaches here. But honestly, I don't think that's what you really want. I think removing this entirely really is correct. Especially because we're not removing this warning, we're only disabling it for PGO builds. In theory, the compiler could warn about this in a non-PGO build, at which point we'd need to remove it anyway, and at most just leave a comment like "// Control never returns here."
On 10 June 2014 11:07, <pkasting@chromium.org> wrote: > On 2014/06/10 17:35:31, bradn wrote: > >> + mseaborn >> > > > https://codereview.chromium.org/315323002/diff/40001/ > components/nacl/loader/nacl_listener.cc > >> File components/nacl/loader/nacl_listener.cc (right): >> > > > https://codereview.chromium.org/315323002/diff/40001/ > components/nacl/loader/nacl_listener.cc#newcode478 > >> components/nacl/loader/nacl_listener.cc:478: NaClChromeMainStartApp(nap, >> > args); > >> On 2014/06/10 17:03:33, Sébastien Marchand wrote: >> >> > On 2014/06/10 16:42:47, bradn wrote: >> > > I'm a little uncomfortable with this being removed. >> > > Could we not introduce a define and do this conditionally? >> > >> > I could do this but the NaCLChromeMainStartApp function doesn't return >> and this >> > cause an 'unreachable code' warning to be triggered... >> > > So we have this here for a very particular reason. >> This is very much an assert. We're setting up nacl at this point, >> potentially >> doing non-intuitive things and want to be certain that control flow does >> not get > > beyond here. >> > > Then you're doing it wrong. NOTREACHED() does not "make certain control > flow > does not get here", it says definitively that control flow never reaches > there, > and it's documentation for the reader, not some kind of actual functional > assertion. > > If you need anything in the code here, it would be something based on CHECK > instead, to force a crash if control flow reaches here. I think a CHECK would be better than a NOTREACHED here, but it would fall afoul of unreachable-code warnings in the same way, wouldn't it? > But honestly, I don't > think that's what you really want. I think removing this entirely really > is > correct. Especially because we're not removing this warning, we're only > disabling it for PGO builds. > In theory, the compiler could warn about this in a > non-PGO build, at which point we'd need to remove it anyway, and at most > just > leave a comment like "// Control never returns here." > How could the compiler warn about that when the function that nacl_listener.cc is calling (NaClChromeMainStartApp()) lives in a separate compilation unit? Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
2014-06-10 14:16 GMT-04:00 Mark Seaborn <mseaborn@chromium.org>: > On 10 June 2014 11:07, <pkasting@chromium.org> wrote: > >> On 2014/06/10 17:35:31, bradn wrote: >> >>> + mseaborn >>> >> >> >> https://codereview.chromium.org/315323002/diff/40001/ >> components/nacl/loader/nacl_listener.cc >> >>> File components/nacl/loader/nacl_listener.cc (right): >>> >> >> >> https://codereview.chromium.org/315323002/diff/40001/ >> components/nacl/loader/nacl_listener.cc#newcode478 >> >>> components/nacl/loader/nacl_listener.cc:478: NaClChromeMainStartApp(nap, >>> >> args); >> >>> On 2014/06/10 17:03:33, Sébastien Marchand wrote: >>> >>> > On 2014/06/10 16:42:47, bradn wrote: >>> > > I'm a little uncomfortable with this being removed. >>> > > Could we not introduce a define and do this conditionally? >>> > >>> > I could do this but the NaCLChromeMainStartApp function doesn't return >>> and this >>> > cause an 'unreachable code' warning to be triggered... >>> >> >> So we have this here for a very particular reason. >>> This is very much an assert. We're setting up nacl at this point, >>> potentially >>> doing non-intuitive things and want to be certain that control flow does >>> not get >> >> beyond here. >>> >> >> Then you're doing it wrong. NOTREACHED() does not "make certain control >> flow >> does not get here", it says definitively that control flow never reaches >> there, >> and it's documentation for the reader, not some kind of actual functional >> assertion. >> >> If you need anything in the code here, it would be something based on >> CHECK >> instead, to force a crash if control flow reaches here. > > > I think a CHECK would be better than a NOTREACHED here, but it would fall > afoul of unreachable-code warnings in the same way, wouldn't it? > Yeah, we'll trigger the same 'unreachable code' warning with something based on a CHECK. What about keeping the NOTREACHED inside a #ifndef NDEBUG so it'll only be here in debug ? > > >> But honestly, I don't >> think that's what you really want. I think removing this entirely really >> is >> correct. Especially because we're not removing this warning, we're only >> disabling it for PGO builds. > > > >> In theory, the compiler could warn about this in a >> non-PGO build, at which point we'd need to remove it anyway, and at most >> just >> leave a comment like "// Control never returns here." >> > > How could the compiler warn about that when the function that > nacl_listener.cc is calling (NaClChromeMainStartApp()) lives in a separate > compilation unit? > s/compiler/linker/ , this trigger in PGO mode where the linker do some aggressive WPO (Whole Program Optimization) > > Cheers, > Mark > > -- Sébastien Marchand | Software Developer | sebmarchand@google.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/06/10 18:24:58, Sébastien Marchand wrote:
> 2014-06-10 14:16 GMT-04:00 Mark Seaborn <mailto:mseaborn@chromium.org>:
> > On 10 June 2014 11:07, <mailto:pkasting@chromium.org> wrote:
> >> If you need anything in the code here, it would be something based on
> >> CHECK
> >> instead, to force a crash if control flow reaches here.
> >
> > I think a CHECK would be better than a NOTREACHED here, but it would fall
> > afoul of unreachable-code warnings in the same way, wouldn't it?
>
> Yeah, we'll trigger the same 'unreachable code' warning with something
> based on a CHECK. What about keeping the NOTREACHED inside a #ifndef NDEBUG
> so it'll only be here in debug ?
That doesn't help. Compilers are still allowed to do link-time optimization in
Debug mode, or with PGO off. (The toolchain from my previous company did so.)
Let's step back a second. What, precisely, is going to go wrong if control
returns from this function call? Is the process going to terminate anyway? It
seems like, if something bad is going to happen, and we need to put anything in
the code, the most important part is to have a comment that explains what's
actually going to go wrong.
After that, if it's really important that NaClChromeMainStartApp() not return,
why not make use of the compiler's ability to enforce that? Both MSVC and GCC
have "noreturn" markers("__declspec(noreturn)" and "__attribute__((noreturn))",
respectively) that you can use on the declaration of this function. Then the
compiler will complain at you if the function contains a return statement.
If you make use of this attribute, I think all you need is a comment here rather
than some sort of CHECK-based code. It's a safer pattern.
Note that we don't have a NORETURN macro defined that expands to the correct
attribute -- we'd want to add one for this.
On 10 June 2014 11:24, Sébastien Marchand <sebmarchand@chromium.org> wrote: > 2014-06-10 14:16 GMT-04:00 Mark Seaborn <mseaborn@chromium.org>: > > On 10 June 2014 11:07, <pkasting@chromium.org> wrote: >> >>> On 2014/06/10 17:35:31, bradn wrote: >>> >>>> + mseaborn >>>> >>> >>> >>> https://codereview.chromium.org/315323002/diff/40001/ >>> components/nacl/loader/nacl_listener.cc >>> >>>> File components/nacl/loader/nacl_listener.cc (right): >>>> >>> >>> >>> https://codereview.chromium.org/315323002/diff/40001/ >>> components/nacl/loader/nacl_listener.cc#newcode478 >>> >>>> components/nacl/loader/nacl_listener.cc:478: >>>> NaClChromeMainStartApp(nap, >>>> >>> args); >>> >>>> On 2014/06/10 17:03:33, Sébastien Marchand wrote: >>>> >>>> > On 2014/06/10 16:42:47, bradn wrote: >>>> > > I'm a little uncomfortable with this being removed. >>>> > > Could we not introduce a define and do this conditionally? >>>> > >>>> > I could do this but the NaCLChromeMainStartApp function doesn't >>>> return and this >>>> > cause an 'unreachable code' warning to be triggered... >>>> >>> >>> So we have this here for a very particular reason. >>>> This is very much an assert. We're setting up nacl at this point, >>>> potentially >>>> doing non-intuitive things and want to be certain that control flow >>>> does not get >>> >>> beyond here. >>>> >>> >>> Then you're doing it wrong. NOTREACHED() does not "make certain control >>> flow >>> does not get here", it says definitively that control flow never reaches >>> there, >>> and it's documentation for the reader, not some kind of actual functional >>> assertion. >>> >>> If you need anything in the code here, it would be something based on >>> CHECK >>> instead, to force a crash if control flow reaches here. >> >> >> I think a CHECK would be better than a NOTREACHED here, but it would fall >> afoul of unreachable-code warnings in the same way, wouldn't it? >> > > Yeah, we'll trigger the same 'unreachable code' warning with something > based on a CHECK. What about keeping the NOTREACHED inside a #ifndef NDEBUG > so it'll only be here in debug ? > NOTREACHED is already a no-op in release builds, IIRC. > >> >>> But honestly, I don't >>> think that's what you really want. I think removing this entirely >>> really is >>> correct. Especially because we're not removing this warning, we're only >>> disabling it for PGO builds. >> >> >> >>> In theory, the compiler could warn about this in a >>> non-PGO build, at which point we'd need to remove it anyway, and at most >>> just >>> leave a comment like "// Control never returns here." >>> >> >> How could the compiler warn about that when the function that >> nacl_listener.cc is calling (NaClChromeMainStartApp()) lives in a separate >> compilation unit? >> > > s/compiler/linker/ , this trigger in PGO mode where the linker do some > aggressive WPO (Whole Program Optimization) > I'm not keen on the idea of enabling werrors (fatal warnings) about unreachable code that depend on non-trivial compiler optimisations. For example, it is a common and good pattern to write: if (CONFIG_BLAH) { ... } where CONFIG_BLAH is a compile-time constant, or perhaps a more complex function call that happens to return a value that's constant for a given build. That can be preferable to "#ifdef CONFIG_BLAH" because the body of the if() gets compile-tested in all builds. If the compiler warned about the if() body being unreachable, it would disallow a quite reasonable pattern. Anyhow, removing code that triggers warning 4702 in the same change as disabling warning 4702 seems rather contradictory (as Brad noted). If you really want to remove this NOTREACHED, can you do that in a separate change, so that our discussion about that doesn't hold up getting your current build fix committed, please? Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm not opposed to splitting the two changes, but it just means we'll be continuing this same discussion on another patch. You didn't respond to my comments about preferring the "noreturn" attribute on the function to using assertions in this way. I'm interested in your thoughts. Regarding other possible problems with "warn on unreachable code", since the warning is on by default already and hasn't required us to go around eliminating things like "if (CONFIG_BLAH)", I'd prefer to worry about those cases if and when they occur.
Thanks, I've reverted the change to nacl_listener.cc and I'll start another CL just for this (as this one is currently blocking the setup of the PGO builder). Waiting for brett's owner lgtm for the changes in base/
Can you do the same in the GN build? build/config/compiler/BUILD.gn Add "/wd4702" to the list of windows-only cflags in the "optimize_max" config
Done, PTAnL.
LGTM https://codereview.chromium.org/315323002/diff/80001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/315323002/diff/80001/build/config/compiler/BU... build/config/compiler/BUILD.gn:770: "/wd4702", # Disable warning C4702 "unreachable code". Can you copy the comment from the .gypi file to above this line? That has a lot better information.
Thanks, committing. https://codereview.chromium.org/315323002/diff/80001/build/config/compiler/BU... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/315323002/diff/80001/build/config/compiler/BU... build/config/compiler/BUILD.gn:770: "/wd4702", # Disable warning C4702 "unreachable code". On 2014/06/11 16:33:09, brettw wrote: > Can you copy the comment from the .gypi file to above this line? That has a lot > better information. Done.
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/315323002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/315323002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/315323002/100001
Message was sent while issue was closed.
Change committed as 276737 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
