vptr check makes -fsanitize=undefined very inconsistent.
So this is a proposal to move vptr from default undefined check into
an optional check which need to be explicitly requested with a flag.
Here is PR for that [ubsan] Remove -fsanitizer=vptr from -fsanitizer=undefined by vitalybuka · Pull Request #121115 · llvm/llvm-project · GitHub
Any concerns?
Any comments?
It isn’t clear to me the motivation here. Your RFC needs a ton more background/information before we can judge what is going on here.
I’m not sure anyone can comment otherwise.
This is more or less to get visibility for PR.
Looks like I can’t edit the post any more, will reply here.
-fsanitize=undefined is inconsistent with vptr:
Trap
-fsanitizer=undefined -fno-sanitize-trap=undefined includes vptr-fsanitizer=undefined -fsanitize-trap=undefined silently does not include vptrRTTI
-fsanitizer=undefined -frtti: vptr is ON-fsanitizer=undefined -fno-rtti:: silently does not include vptrRequires linking standard C++ library
No other checks included into -fsanitize=undefined has such requirements.
Proposed solution is just to remove vptr check from undefined group. User will have explicitly requests vptr, and deal with issues above. We already have ubsan checks like this.
Will this removal cause a silent behavioral change for existing users? Basically, is there a way we can make sure users are not caught by surprise with the change?
I don’t know have any ideas other than this RFC, and ReleaseNotes.
Warning in every invocation of -fanitize=undefined, which changes behavior, seems excessive.
Given that this check pulls sanitizer runtime, and does not support -trap mode, I hope no one is using vptr for any production hardening. So it should be less important than removing e.g. =signed-integer-overflow, or =bounds checks.
I think we can probably live with it via release notes.
CC @zygoloid as UBSan owner, in case he has a different opinion.
Yes, I think this is reasonable to announce via release notes only.
FWIW, vptr is also unlike other undefined checks in that it’s probabilistic (and might be nondeterministic under ASLR as a result) – it checks whether the hash of the check is in a table of known-good hashes and skips the expensive check if so.
This is not a good change in behavior - I actually thought this was a regression and spent a few hours bisecting.
We’ve already had one person make claims on a WG21 (proposals TBF) mailing list about some UB behavior in their code not being UB.
It is perfectly reasonable for the default mode to include a vptr check and to the standard library because that is the default expectation for any C++ being compiled by user.
Basically, these changes regress the behavior and testing of ubsan for the default case, in order to make the non-default case easier. That’s clearly the wrong way around.
I’m not certain I agree, especially considering Richard’s observation:
Nondeterminism in your UB check is something that doesn’t seem like it belongs in the default set of behaviors.
This doesn’t seem like a great argument – -fsanitize=undefined can’t exhaustively detect all undefined behavior. If someone things that -fsanitize=undefined can detect any and all UB and use that as an argument that behavior is defined, either they are being unreasonable or we have a documentation problem.
You know, I find this compelling, but I don’t think you put it very well, so I’ll try to help restate it. 
The problem @vitalybuka is trying to solve is simplifying the relationship between all these interrelated compiler flags (-nostdlib, -fno-rtti, -fsanitize=trap, -fsanitize=undefined, defaults). -fsanitize=undefined is what I might call an “intent-based compiler flag”, it’s sort of like “do what I mean, turn on all the stuff, I don’t really know what I want, but if you’ve got more, I want it”.
If we take that as the goal of this flag, then perhaps it really does make sense for the Clang driver to bear the cost of disabling the vptr sanitizer when RTTI is disabled, so that everyday users with more banal use cases get more cheap type checking out of the box without having to read the manual at all.
If this is what we go with, we should document it.