-
Notifications
You must be signed in to change notification settings - Fork 105
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
add address and undefined sanitizers to github action #417
Conversation
It just proved its value, a null pointer reference was detected
WIP |
nix/overlays.nix
Outdated
inherit version src meta doCheck doTarget dontStrip; | ||
pname = "hobbes-clang-" + (toString llvmVersion) + (if sans then "-ASanAndUBSan" else ""); | ||
|
||
cmakeBuildType=(if sans then "Debug" else "Release"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a debug flag to this nix expression, you should use that instead
Run sanitizers with clang only (not gcc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add new SAN specific package targets
- use Clang (not GCC) only for the SAN packages
nix/overlays.nix
Outdated
value = recurseIntoAttrs ({ | ||
hobbes = dbg (callPackage withCLANG { inherit llvmVersion; }); | ||
}); | ||
value = recurseIntoAttrs (builtins.listToAttrs (builtins.map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add new separate SAN specific packages withSans
instead. Do not add params to the existing ones
There are few changed in the last two commits
Now CI failed on an function pointer conversion which I've seen at least a month ago, then I've not been able to reproduce it since then... WIP |
@@ -262,6 +262,9 @@ template <typename T> | |||
} | |||
template <typename TIter> | |||
inline void writes(imagefile* f, TIter begin, TIter end) { | |||
if (begin == end) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!
@@ -89,5 +115,11 @@ in { | |||
value = recurseIntoAttrs ({ | |||
hobbes = dbg (callPackage withCLANG { inherit llvmVersion; }); | |||
}); | |||
}) llvmVersions)) // recurseIntoAttrs (builtins.listToAttrs (builtins.map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is great, @mo-xiaoming
… causes too many failures in current code base
d589c1e
to
705402f
Compare
Besides being ugly, there are few things to note
-Og
to debug flags, otherwise, getwarning _FORTIFY_SOURCE requires compiling with optimization (-O)
. This is a long standing nix/gcc bug GCC has unwanted flags NixOS/nixpkgs#18995Default
added to path, otherwise, nix generates something likehobbesPackages/gcc-8/llvm-9//hobbes
. Don't know how to get ride of//
, so aDefault
addedRelease
build with sanitizer and debug symbols takes forever to build. So there is only one active(gcc8, llvm11?) version is checked by sanitizers