libMesh / TIMPI

Templated Interface to MPI
GNU Lesser General Public License v2.1
12 stars 11 forks source link

Sanitize and test vector<bool> specializations #124

Closed roystgnr closed 1 year ago

roystgnr commented 1 year ago

@NamjaeChoi - can you see if this still gives you -fsanitize=integer complaints? I'll need to do a little futzing to get a clang+MPI build working myself.

moosebuild commented 1 year ago

Job Coverage on 5523954 wanted to post the following:

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.

NamjaeChoi commented 1 year ago

I think this removes the complaint related with bit shifting.

NamjaeChoi commented 1 year ago

Only the following complaint remains related with parallel_implementation.h:

/Users/choin/projects/griffin/moose/scripts/../libmesh/installed/include/timpi/parallel_implementation.h:4055:3: runtime error: implicit conversion from type 'unsigned int' of value 4294967294 (32-bit, unsigned) to type 'int' changed the value to -2 (32-bit, signed) SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/choin/projects/griffin/moose/scripts/../libmesh/installed/include/timpi/parallel_implementation.h:4055:3 in

roystgnr commented 1 year ago

I'll see if I can reproduce that in a libMesh run; I think I've figured out the clang+MPI situation. I don't suppose you can trigger it via the core MOOSE framework at most?

roystgnr commented 1 year ago

Whoa, I seem to be able to trigger it in TIMPI alone.

roystgnr commented 1 year ago

Is there a way to get -fsanitize=integer errors to be fatal? For now I can check run_unit_tests.sh.log, but if I could actually stop the unit tests from passing then we could get regression coverage for this later. The TIMPI test suite runs really fast.

I tried adding -fno-sanitize-recover=integer too but it didn't seem to change anything.

NamjaeChoi commented 1 year ago

Not sure about making it fatal, but maybe you have to spell out each option in the integer group explicitly? The documentation says integer includes signed-integer-overflow, unsigned-integer-overflow, shift, integer-divide-by-zero, implicit-unsigned-integer-truncation, implicit-signed-integer-truncation, implicit-integer-sign-change.

roystgnr commented 1 year ago

Would you see how things work for you with these last two commits? Those seem to fix all the fsanitize issues I can trigger at the TIMPI level alone.

NamjaeChoi commented 1 year ago

I also confirmed that it removes all the MPI-related complaints. Thanks.