vsk added a comment. In D56624#1369940 <https://reviews.llvm.org/D56624#1369940>, @yln wrote:
> Note that all of this currently only matters when compiling with > `-fsanitize=unreachable`. The following discussion is within the context of > the current implementation: UBSan removes the `noreturn` so it can instrument > `unreachable` without the added instrumentation being optimized away. Maybe > we should take a step back and ask if that is the right approach at all? > > In D56624#1369795 <https://reviews.llvm.org/D56624#1369795>, @vsk wrote: > > > Because "expect_noreturn" calls are allowed to return, the compiler must > > behave as they could. In particular, this means that unpoisoning the stack > > before expect_noreturn calls (given the current semantics) is premature. > > > > Put another way, a frontend author may (understandably, but mistakenly!) > > attach expect_noreturn to calls which they expect to be cold. > > > I think about this differently. Yes, most noreturn functions are also cold, > e.g., `abort`, but not necessarily, e.g., calls to `longjmp` do not > necessarily have to be. Why would it be okay to attach expect_noreturn > instead of cold? It would be okay by definition, because it would be allowed by the proposed IR semantics. > Why do we think that this is an easy-to-make mistake? I don't think that's the right question. Rather, we should ask: why is it acceptable to define semantics in a way that makes the mistake possible? My thinking on this is: it's not acceptable, because a narrower change (say, introducing a sanitizer_noreturn attribute) would address the issue without as much potential for abuse. > Can we agree on the following? > "It is orthogonal on the language level, but seems to be redundant in terms > of the optimizer. Since LLVM IR's main purpose it support the optimizer, this > is a strong argument against the general purpose attribute." I'm making a more neutral point: that expect_noreturn conflates different concerns -- optimization and sanitizer correctness. I'm not making a claim about what the main purpose of IR is. >> That would regress ASan coverage. > > You talk specifically about cases of misuses of the attribute, right? > In the context of the current issue with UBSan the possibility for false > negative is not too much of a regression: it only occurs when UBSan is going > to diagnose an "unreachable error" anyways. > > So the main point is whether or not to use a "general purpose" attribute or a > "narrow purpose" attribute/intrinsic. My understanding is that you list the > following points as arguments against general purpose. Is my understanding > accurate? > > 1. Potential misuse can regress ASan coverage > 2. Complicates optimizer > > Narrow purpose: No potential misuses, and optimizer can simply ignore it. Yes, I think this is a fair summary, thanks :). > Initially I proposed a narrow purpose attribute, but while iterating on this > revision changed it to be general purpose. @eugenis > Now, I have a slight preference for general purpose: I don't think 1. is a > big issue (but then again, I have no experience here), Changes to the IR semantics have hard-to-predict ripple effects on many, many other projects. It pays to be conservative in this area. > and 2. it is always correct for the optimizer to continue ignoring the > attribute (current status). > > Actually, 2. also encompasses the potential upside: a more complicated > optimizer that takes advantage of the attribute to do additional > optimizations. I'm having a hard time thinking of any optimizations based on expect_noreturn which aren't already enabled by the cold attribute. What do you have in mind? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56624/new/ https://reviews.llvm.org/D56624 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
