Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark functions that never return as noreturn #8

Closed
wants to merge 1 commit into from

Conversation

pmatos
Copy link
Collaborator

@pmatos pmatos commented Jun 10, 2019

Silences a few static analysis complaints

Silences a few static analysis complaints
@mflatt
Copy link
Member

mflatt commented Jun 10, 2019

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

@pmatos
Copy link
Collaborator Author

pmatos commented Jun 10, 2019

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
Copy link
Collaborator Author

pmatos commented Jun 10, 2019

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
Copy link
Member

mflatt commented Jun 10, 2019

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
Copy link
Collaborator Author

pmatos commented Jun 10, 2019

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
Copy link
Member

mflatt commented Jun 10, 2019

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
Copy link
Collaborator Author

pmatos commented Jun 11, 2019

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
Copy link
Collaborator Author

pmatos commented Jun 11, 2019

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.

mflatt added a commit to mflatt/ChezScheme that referenced this pull request Jun 21, 2019
@pmatos
Copy link
Collaborator Author

pmatos commented Jun 22, 2019

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

@pmatos pmatos closed this Jun 22, 2019
@pmatos pmatos deleted the patch-6 branch April 26, 2020 08:29
mflatt added a commit to mflatt/racket that referenced this pull request Jul 27, 2020
@pmatos did all the work here in racket/ChezScheme#8 and
racket#2344.

original commit: 2e3a618b0072d547b6c5abe6dd8dbac36a98c10e
mflatt added a commit that referenced this pull request Mar 24, 2021
mflatt added a commit to mflatt/ChezScheme that referenced this pull request Oct 10, 2023
mflatt added a commit to mflatt/ChezScheme that referenced this pull request Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants