Radovan =?utf-8?q?Božić?= <[email protected]>, Radovan =?utf-8?q?Božić?= <[email protected]>, Radovan =?utf-8?q?Božić?= <[email protected]> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/[email protected]>
philnik777 wrote: > > > I think this is already the precedent? [...] > > > > > > I don't think so. AFAICT the currently added attributes simply add > > semantics guaranteed by some standard/are fundamental parts of the contract > > of a function (e.g. `noreturn`), allow removing dead code or some amount of > > constant folding (e.g. `pure`), or enable diagnostics (e.g. `format`). None > > of these allow exploitation of UB the library could define I think. > > I consider whether a pointer is allowed to be null or not to be a fundamental > part of the contract of the function; you don't? Not necessarily. The standard doesn't make any guarantees, but libraries are allowed to. That's the part the below example doesn't match. `__builtin_unreachable()` isn't part of any standard, so the compiler defines the contract from scratch. If you take a function that's actually defined in a standard, e.g. `exit`, that is guaranteed by the standard to not return. We're adding semantic information guaranteed by the standard - we're not assuming what an implementation actually does when it's called out of contract. > Noreturn is a good example -- if a noreturn function actually returns, > [that's UB](https://eel.is/c++draft/dcl.attr.noreturn#2) and we do "exploit" > it: https://godbolt.org/z/5YcMnbf11 > > I think builtins should model as much semantic information as we would like > to glean, both for diagnostics and for optimizations. But if there are good > reasons to ignore some of that information, then I think it's reasonable for > us to do so. I don't think there are good reasons to ignore that information > in this specific case, but we've ignored it in the past. > > > > Do you have some usage pattern you're worried about? If we have evidence > > > this will be disruptive, that would be compelling to know about. > > > > > > I don't. I mostly didn't see any discussion of whether it's a good idea to > > potentially introduce UB here, which worried me somewhat, especially since > > I think this patch does introduce a novel idea. > > This does not _introduce_ UB here. It's already UB and we have evidence that > standard library implementations treat it as behaviorally undefined: > https://godbolt.org/z/bqn16fWTs and optimize on it, even without the standard > library marking anything explicitly: https://godbolt.org/z/3zY6MzcY9 So I > don't think what this patch is doing is actually novel. Yes, I agree now that it doesn't introduce UB in this case. What I meant is that that wasn't clear to me given the lack of discussion about it on the PR, and IMO `nonnull` is an attribute that should always require good evidence that there is no library defining the behaviour of a nullptr. Now that we have that evidence I don't think this particular patch is a bad idea. > That said, I don't think we have a good plan for hardened libraries. We have > no notion of conditionally applying these markings in tablegen, and perhaps > we need to think of something for that. e.g., if the user passes `-fhardened` > to the compiler (or whatever we end up exposing), I can imagine we'd want to > switch from `__attribute__((nonnull))` to `_Nonnull` semantics in that case. > But I'm also not certain we need such a mechanism today for this PR, either. > > CC @rjmccall @rnk @efriedma-quic @jyknight for some additional opinions, in > case my stance is off-base. I agree. https://github.com/llvm/llvm-project/pull/160988 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
