Get a clean -fsanitize=integer run of make check#3568
Get a clean -fsanitize=integer run of make check#3568roystgnr merged 15 commits intolibMesh:develfrom
-fsanitize=integer run of make check#3568Conversation
|
|
||
| Node* head = &global_head; | ||
| for(size_t b=sequence.size(); b-->0; ) | ||
| for(ptrdiff_t b=sequence.size(); b-->0; ) |
There was a problem hiding this comment.
How was this working before with an unsigned type and this cutesy b-->0 BS? When b==0 the test fails and then you post-decrement so the number wraps to max size_t, but we never see it because the loop is done?
There was a problem hiding this comment.
Precisely. It's apparently a popular idiom, too; this is what -fsanitize=integer was complaining about in a couple places in libstdc++ basic_string code.
| (name.rfind(".nem") == name.size() - 4) || | ||
| (name.rfind(".n") == name.size() - 2) || |
There was a problem hiding this comment.
I'd have guessed the sanitizer wouldn't like subtracting from an unsigned type that could possibly be 0? Oh, but std::string::rfind() returns std::string::npos which is effectively max size_t so adding to that would also be bad... not sure which is best actually.
There was a problem hiding this comment.
Yeah; the saving grace here is that npos actually gets returned pretty frequently in practice, whereas at least in our unit tests and examples we never try to open a filename that's only 3 characters long. UBSan adds a bunch of runtime checking; if it tried to also do compile-time checking of ranges then I can't imagine how many thousand false positives we'd have hit...
| _dof_constraints.lower_bound(this->first_dof()), | ||
| upper = | ||
| _dof_constraints.upper_bound(this->end_dof()-1); | ||
| _dof_constraints.lower_bound(this->end_dof()); |
There was a problem hiding this comment.
This looks like an actual bugfix... was that suggested by the sanitizer?
There was a problem hiding this comment.
It wasn't suggested by the sanitizer; that only reports filename and line number and what type of behavior was detected, and leaves it up to the developer to figure out how to fix it. Maybe in LLVM 30.
But ... yeah, this looks like a real bug fix. I recently spent a ton of time fixing a bug that was triggered by an unexpected corner case with element deletion, just like this one could easily have been, and as annoying as the sanitizer is I have to say this fix alone made me feel like the annoyance was worth it so far.
jwpeterson
left a comment
jwpeterson
left a comment
There was a problem hiding this comment.
Looks good to me, but sounds like it was pretty annoying to use the sanitizer.
I guess you'll have to version the sanitizer attributes as well. |
|
Job Coverage on 4b99bb8 wanted to post the following: Coverage
Warnings
This comment will be updated on new commits. |
||||||||||||||||||||||||||
|
Ah, damn. I should just add that file to the ignorelist. |
This pulls in all the sanitize fixes we got there
npos is huge; if we try to add to it then we overflow. This could actually have caused false positives with super tiny filenames.
This is an int as a member variable; let's keep it an int through more of the codepath, and make the conversions explicit casts. This keeps clang fsanitize=integer happy with RB.
This fixes an fsanitize=integer complaint
Unsigned integers, so this was allowed and comes out with a correct answer, but it freaked out fsanitize and kinda freaks me out too.
I'm not sure what trying to instantiate a -1 (or 2^N-1) dimensional quadrature rule does, but that can't be something we want to support.
This just handles clang++ -fsanitize=integer for now, but we could expand it.
This should have worked correctly, but it makes fsanitize=integer scream, and it definitely is creepy when you think about it.
roystgnr
force-pushed
the
sanitize_integer
branch
from
f4e6a4e to
4b99bb8
Compare
3 participants
Pinging @NamjaeChoi
I just ran in serial and on 4 procs in parallel, but this is runtime behavior checking and it's quite possible that corner cases exist at other processors counts, especially with AMR.
A clean run here requires
-fsanitize-ignorelist=yoursrcdir/fsanitize_ignorelist.txt. In the most extreme example, there are a few libstdc++ functions that aren't-fsanitize=integerclean and are never going to be. The GCC team seems to believe that unsigned integer overflow and underflow are not undefined behavior (definitely correct), are necessary for optimal code when doing certain kinds of bit fiddling and looping (probably at least partially correct), and that there's no point in looking for them elsewhere (incorrect, but that's what we have the ignore list for). In addition to upstream stuff like libstdc++ and Eigen, I've got ignore entries to cover the heavy bit-fiddling in our fparser fork and in some hashing headers.It would be interesting to add a recipe for this to the devel->master merge, but a quick glance through the commits here should show how annoying it is to fix the lines that fsanitize=integer screams about. 90% are obvious false positives, and perhaps 90% of the rest are subtle false positives. Oh, and runs with UBSan active are pretty slow, though that's probably my fault for combining it with dbg in my own testing. Maybe we should add a recipe but it only runs opt mode and we don't make it required to pass?
Longer-term: I love the goal here, but I'm not sure how feasible it's going to be to extend it to MOOSE. AFAIK nobody's ever even tried to get a
-Wconversion -Werrorbuild there, and I'm betting the libMesh history of sporadically doing things like that has made this cleanup much easier than it could have been.