Bug 33056 – Implement NO_RETURN for MSVC

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

Implement the NO_RETURN macro also for MSVC.

From http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Function-Attributes.html:
It does not make sense for a noreturn function to have a return type other than void. 

I found two files who use the NO_RETURN macro in a wrong way (FastMalloc.cpp and jsc.cpp).

[Patrick R. Gansterer]

Created attachment 45669 [details]
Implement NO_RETURN for MSVC

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45669 [details] without any errors.

[Patrick R. Gansterer]

Created attachment 45670 [details]
Fix use of NO_RETURN in FastMalloc.cpp

[WebKit Review Bot]

Attachment 45670 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:1380:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1

[Mark Rowe (bdash)]

Comment on attachment 45670 [details]
Fix use of NO_RETURN in FastMalloc.cpp

Given that the function is not intended to return I’m not sure why we’d want to remove the annotation that indicates that.  Anyway, this will break the Mac build (and possibly others):
JavaScriptCore/wtf/FastMalloc.cpp:1431: warning: function might be possible candidate for attribute ‘noreturn’

[Patrick R. Gansterer]

(In reply to comment #5)
> (From update of attachment 45670 [details])
> Given that the function is not intended to return I’m not sure why we’d want to
> remove the annotation that indicates that.  Anyway, this will break the Mac
> build (and possibly others):
> JavaScriptCore/wtf/FastMalloc.cpp:1431: warning: function might be possible
> candidate for attribute ‘noreturn’
When you implement NO_RETURN for MSVC then it will throw an warning, because noreturn functions must have a void return type. A solution might be that we disable "warning C4646: function declared with __declspec(noreturn) has non-void return type", but then is the problem with the compiler for WinCE. It seams that it ignores the noreturn and throws always an error!

The other possible solution is, that we don't implement NO_RETURN for MSVC and copy the hack from FastMalloc.cpp to jsc.cpp (see bug 32939). I think that this will win, because GCC is the better/prefered compiler. ;-)

[Patrick R. Gansterer]

(In reply to comment #6)
> The other possible solution is, that we don't implement NO_RETURN for MSVC and
> copy the hack from FastMalloc.cpp to jsc.cpp (see bug 32939). I think that this
> will win, because GCC is the better/prefered compiler. ;-)
But then the Win32 compiler will throw "warning C4702: unreachable code".

So mit current solution is #if COMPILER(MSVC) && PLATFORM(WINCE) :-(

[Eric Seidel (no email)]

Some one who knows how MSVC works would have to review the first patch.

[Adam Roben (:aroben)]

Comment on attachment 45669 [details]
Implement NO_RETURN for MSVC

r=me

[WebKit Commit Bot]

Comment on attachment 45669 [details]
Implement NO_RETURN for MSVC

Clearing flags on attachment: 45669

Committed r52741: <http://trac.webkit.org/changeset/52741>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

So it does not break the build to define NO_RETURN but leave FastMalloc.cpp alone? If it does then these should not have been independently reviewed patches!

[Adam Roben (:aroben)]

(In reply to comment #12)
> So it does not break the build to define NO_RETURN but leave FastMalloc.cpp
> alone? If it does then these should not have been independently reviewed
> patches!

It does break it. I'm rolling out r52741.

[Adam Roben (:aroben)]

Committed r52744: <http://trac.webkit.org/changeset/52744>

[Adam Roben (:aroben)]

Re-opening the bug since the patch was rolled out.

[Adam Roben (:aroben)]

Comment on attachment 45669 [details]
Implement NO_RETURN for MSVC

r- since this can't land until FastMalloc.cpp is changed.

[Eric Seidel (no email)]

Sorry.  The commit-queue only knows how to test on Mac Leopard (WebKit's seemingly primary platform) for now.

[Patrick R. Gansterer]

Sorry for the buildbreak! That wasn't my intention. Sorry!

Here is a small sum up:
If you implement the __declspec(noreturn) MSVC will complain on functions with returntyp other than void (e.g. functionQuit). If you remove the NO_RETURN macro from such functions the GCC will throw "warning: function might be possible candidate for attribute ‘noreturn’". If you use exit() not at the end of a function MSVC on Win32 will throw "warning C4702: unreachable code", but on WinCE it complains about a missing return statement if exit() is the last statement.
It's always fun to compile with MSVC, because it has different behaviour on every platform!

Does it make sense to implement the NO_RETURN macro for MSVC?

In the moment only functionQuit in jsc.cpp (bug 32939), is a blocker to compile jsc directly from trunk.

[Darin Adler]

You can come up with whatever solution you like, as long as you don’t lower the warning settings or break the build.

We could have multiple NO_RETURN macros to work around the differences in various versions of MSVC. We could have ifdefs at various call sites. I don’t see this as an urgent issue, but it’s fine to try to get it working.

[Patrick R. Gansterer]

Created attachment 52875 [details]
New patch

Maybe someone has a better name than NO_VALUE_RETURN ;-)

[WebKit Review Bot]

Attachment 52875 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:1434:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 52875 [details]
New patch

I think NO_RETURN_WITH_VALUE would be better if only because it doesn't make it seem like the word "no" goes with the word "value". Could you do it with that different name?

[Patrick R. Gansterer]

Created attachment 52885 [details]
The patch (now with NO_RETURN_WITH_VALUE)

(In reply to comment #22)
> (From update of attachment 52875 [details])
> I think NO_RETURN_WITH_VALUE would be better if only because it doesn't make it
> seem like the word "no" goes with the word "value". Could you do it with that
> different name?
Done!

[WebKit Review Bot]

Attachment 52885 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/FastMalloc.cpp:1434:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 52885 [details]
The patch (now with NO_RETURN_WITH_VALUE)

Clearing flags on attachment: 52885

Committed r57318: <http://trac.webkit.org/changeset/57318>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.