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

More warnings #39

Closed
NOhs opened this issue May 2, 2018 · 6 comments · Fixed by #99
Closed

More warnings #39

NOhs opened this issue May 2, 2018 · 6 comments · Fixed by #99
Labels
discussion Something that might end up being labled enhancement or wontfix enhancement New feature or request

Comments

@NOhs
Copy link
Collaborator

NOhs commented May 2, 2018

I think the following flags should be added to the defaults:

-fsanitize=address

see https://clang.llvm.org/docs/AddressSanitizer.html

and also

-Wshadow

seem useful flags for writing clean code. The first one can help identify illegal access mistakes and the second one can help finding shadowing bugs like in e.g. nested loops where the loop variables are shadowed in one of the inner loops.

@NOhs NOhs added enhancement New feature or request discussion Something that might end up being labled enhancement or wontfix labels May 2, 2018
@GPMueller
Copy link
Contributor

Agreed!
But isn't -Wshadow automatically added by -Wall? Or does that not actually add "all" warnings?

@GPMueller
Copy link
Contributor

Should behave the same as in gcc, so according to https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html -Wshadow is not activated by -Wall or -Wextra.

We should maybe also add -Wpedantic. People who do not want to be ISO-conformant should probably manually set all their flags.

And what about -fsanitize=leak and -fsanitize=undefined, see https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#Instrumentation-Options

@NOhs
Copy link
Collaborator Author

NOhs commented May 5, 2018

I agree to add all your suggestions.

@GPMueller
Copy link
Contributor

GPMueller commented Sep 9, 2018

Unfortunately

  • -fsanitize=leak won't work, except on OSX: clang++.EXE: error: unsupported option '-fsanitize=leak' for target 'x86_64-pc-windows-msvc'
  • it seems -fsanitize=address and -fsanitize=undefined should be also passed to the linker, but not for static libraries

See also http://clang.llvm.org/docs/AddressSanitizer.html#platforms and http://clang.llvm.org/docs/LeakSanitizer.html

@GPMueller
Copy link
Contributor

GPMueller commented Sep 17, 2019

As the documentation (address, undefined behaviour) states, there is no run-time performance impact of ubsan, but asan gives a ~2x slowdown.
Should we

  • add sanitizers to the default debug flags (also increase optimisation to Og and add -fno-optimize-sibling-calls -fno-omit-frame-pointer)?
  • create a new sanitize build type?

@GPMueller
Copy link
Contributor

I decided to add sanitizers directly to the debug build on PR #99. This can easily be changed later, if we feel that there should be a separate build type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Something that might end up being labled enhancement or wontfix enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants