racket / ChezScheme

Chez Scheme
Apache License 2.0
110 stars 8 forks source link

Mark functions that never return as noreturn #8

Closed pmatos closed 5 years ago

pmatos commented 5 years ago

Silences a few static analysis complaints

mflatt commented 5 years ago

I think there's the usual portability issue here, and probably the usual C-preprocessor solution.

pmatos commented 5 years ago

I am still not sure I completely understand the portability issue here because attribute noreturn is supported by both Gcc, clang and msvc afaik. Do you have a compiler where this fails? If so, I would like to reproduce it.

pmatos commented 5 years ago

Also, is there a minimum C standard we have to support? Are we still only supporting C99 or C11 is the minimum at this point? Maybe CI should enforce this by using one of the std compiler flags.

mflatt commented 5 years ago

I thought we concluded in racket/racket#2472 that enabling via __GNUC__ is the right idea to avoid problems with MSVC.

But then there was racket/racket#2496 where I found that compilers can't always verify the annotation, so we made it enabled only via a command-line preprocessor flag.

So, maybe the same thing as racket/racket#2496 is right here?

pmatos commented 5 years ago

That's right, however I never really reproduced those issues locally. I want to do so because the previous patches felt a bit like hacks since it telling the compiler that a function doesn't return allows the C compiler to perform optimizations that are otherwise skipped and therefore having this even outside CI could be a benefit.

mflatt commented 5 years ago

The __attribute__((noreturn)) form is definitely not supported by MSVC:

C:\Users\Matthew Flatt>cl
Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26433 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

usage: cl [ option... ] filename... [ /link linkoption... ]

C:\Users\Matthew Flatt>type x.c
#include <stdlib.h>

int adios()  __attribute__((noreturn));

int adios()
{
  exit(0);
}

C:\Users\Matthew Flatt>cl /c x.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26433 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

x.c
x.c(3): error C2061: syntax error: identifier '__attribute__'
x.c(3): error C2059: syntax error: ';'

I'm be interested if there's really a performance benefit. It seems possible that the GC could go a little faster if the slow path for an abort somehow interferes with the normal path, but I'll be surprised.

pmatos commented 5 years ago

Hold on to this PR. I will not only try to find a better solution than we currently have but time results with and without noreturn.

pmatos commented 5 years ago

The __attribute__((noreturn)) form is definitely not supported by MSVC:

C:\Users\Matthew Flatt>cl
Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26433 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

usage: cl [ option... ] filename... [ /link linkoption... ]

C:\Users\Matthew Flatt>type x.c
#include <stdlib.h>

int adios()  __attribute__((noreturn));

int adios()
{
  exit(0);
}

C:\Users\Matthew Flatt>cl /c x.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26433 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

x.c
x.c(3): error C2061: syntax error: identifier '__attribute__'
x.c(3): error C2059: syntax error: ';'

I'm be interested if there's really a performance benefit. It seems possible that the GC could go a little faster if the slow path for an abort somehow interferes with the normal path, but I'll be surprised.

Right, I got myself a windows machine early this morning and I can confirm that you're right. I did assume, incorrectly, that MSVC would have implemented this by now. :( I will add the machine to Racket CI just so I can keep an eye on how the build goes on Windows.

pmatos commented 5 years ago

Time to close this after you committed 2e3a618. Thanks!