RFR: 8302262: Remove -XX:SuppressErrorAt develop option

[Thomas Stuefe]

On Sat, 11 Feb 2023 09:42:17 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> > While I have used it rarely, I found it useful the few times I did.
> > Its useful in situations where I analyzed a problem, ran into an assert, but obtaining a new build was difficult or just time-consuming - slow hardware, or a customer's machine. E.g., I used this switch when porting to AIX since rebuilding the VM took about an hour and some asserts could be waived.
> > Quoting from[ JDK-8302189](https://bugs.openjdk.org/browse/JDK-8302189):
> > > "This also addresses the problems encountered in [JDK-8294031](https://bugs.openjdk.org/browse/JDK-8294031). Because the failure reporting function used by assert is not "noreturn", the compiler is analyzing the continuation from a failed assert as potentially having the values that would result in an assertion failure (in this case, a variable having a null value). Doing so, it discovers a use of that variable in a context where a null value is UB, and so issues a warning."
> > 
> > 
> > I don't understand why this is not a problem for release builds. Relying on assert in this case is not sufficient anyway, you'd need to handle the nullptr in release builds too.
> 
> In the example from JDK-8294031 the value shouldn’t be null. But the existence of the not-null assertion indicates that it was null on the failed assertion branch of the conditional. That branch doesn’t terminate in a noreturn/unreachable black hole. The compiler can separately treat the post-assertion rejoin point as two different continuations, one of which definitely has a null value (because of the lack of the black hole). And that leads to discovering later code that is unhappy with that null value, and warning about it. That kind of splitting and value propagation is a normal thing that can happen for all kinds of value, not just checks for non-null. For example, an index in-bounds assertion could run into exactly the same issue.
> 
> In a release build there isn't a null check, so the compiler has no information either way, and _assumes_ the code doesn't invoke UB, and doesn't check for it either.

Interesting. Thanks for explaining this. So the fact that we set the assert triggers the compiler check.

> 
> > > "This matters even for production, since some of the operations (such as `guarantee` and `ShouldNotReachHere`) are present in product builds, not just debug builds. "
> > 
> > 
> > I can see that. But `guarantee` is rare, so maybe a compromise would be to make `guarantee` `noreturn` but leave `assert` as it is?
> 
> Also `ShouldNotReachHere`, `fatal`, &etc. which are pretty common.

In the light of your explanation, I think loosing the switch could be an acceptable price. I see the C-asserts in glibc also use noreturn attributes. Less code is also nice.

-------------

PR: https://git.openjdk.org/jdk/pull/12521