-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
enable ubsan #3452
enable ubsan #3452
Conversation
|
Can we know what test is still running? And was some behaviour intended? STL/tests/tr1/tests/type_traits5/test.cpp Line 183 in 9ae1b3f
STL/tests/tr1/tests/type_traits5/test.cpp Lines 197 to 198 in 9ae1b3f
|
ubsan has some options: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks
|
These case are made undefined by CWG-1766, see [dcl.enum]/8 and [expr.static.cast]/10. STL/tests/tr1/tests/type_traits5/test.cpp Line 183 in 9ae1b3f
In this file, I believe it's better to add fixed underlying type Lines 3799 to 3805 in 9ae1b3f
IIUC the valid values of |
STL/tests/tr1/tests/cstddef/test.cpp Line 26 in 9ae1b3f
This line (and line 32) has UB (invalid pointer arithmetic). I think we should perform pointer arithmetic on Line 483 in 9ae1b3f
Likely dereferenced |
Line 272 in 9ae1b3f
Passing |
Thank you for your analysis!
but it works with |
Co-authored-by: A. Jiang <de34@live.cn>
I remembered #2121 :) |
X86 failing tests
|
STL/tests/tr1/tests/streambuf1/test.cpp Line 31 in d4f1e09
STL/tests/tr1/tests/streambuf2/test.cpp Line 32 in d4f1e09
I think this 2 lines are doing evil, as the begin pointer may be set to non-null but end pointer is definitely set to null. They should be Alignment issues: see DevCom-294259. I guess it can't be fixed without ABI breakage. |
|
line: |
I used this matrix and your test code and your branch.
command:
|
/pr review |
We talked about this at the weekly maintainer meeting and are (reluctantly) okay with enabling ubsan in the test coverage, unless and until sporadic or weird behavior appears. |
This comment was marked as resolved.
This comment was marked as resolved.
Thanks! 😻 I pushed a merge with The most significant impact is that this increases test cost by adding a new configuration to the matrices, but given the issues that this has found, it seems worth it.
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks for improving test coverage and fixing these places where the STL and its tests were being bad kitties by jumping up on the table where they're not allowed! 😼 🚫 🎉 |
It works only with
/MT