Withdrawn: 8332632: Redundant assert "compiler should always document failure: %s" with possible UB

[duke]

On Fri, 24 May 2024 14:27:13 GMT, Evgeny Astigeevich <eastigeevich at openjdk.org> wrote:

> [JDK-8303951](https://bugs.openjdk.org/browse/JDK-8303951) added the following code: https://github.com/openjdk/jdk/pull/13038/files#diff-2e74481e557cbe87170a56a6e592eea33bb59019926e1c32bebcfaf5b571bb53R2280
> 
> 
>      if (!ci_env.failing() && !task->is_success()) {
> + assert(ci_env.failure_reason() != nullptr, "expect failure reason");
> + assert(false, "compiler should always document failure: %s", ci_env.failure_reason());
> 
> 
> The second assert is redundant because `ci_env.failure_reason() != nullptr` is always `false`. It also has possible UB.
> 
> A compiler sees if-statement checking `!ci_env.failing() ` which, if it is true, implies `ci_env.failure_reason()` is `nullptr`.
> 
> Based on this information the compiler can optimize `assert(ci_env.failure_reason() != nullptr, "expect failure reason"); ` to
> `assert(false, "expect failure reason"); `.
> The compiler can optimize `assert(false, "compiler should always document failure: %s", ci_env.failure_reason()); ` to `assert(false, "compiler should always document failure: %s", nullptr); `.
> 
> So the original code would be like the following:
> 
> 
> if (!ci_env.failing() && !task->is_success()) {
>   assert(false, "expect failure reason");
>   assert(false, "compiler should always document failure: %s", nullptr);
> }
> 
> 
> We have an expression where a format string is used. Format strings usually have undefined behavior if `nullptr` is passed for the character string format specifier. See `std::printf` for example.
> 
> Even the second assert is never executed, it makes the IF-block to have UB. The C++ standard says: correct C++ programs are free of undefined behavior. See https://en.cppreference.com/w/cpp/language/ub and https://en.cppreference.com/w/cpp/language/as_if
> 
> Choosing between readability and correctness, I choose correctness.
> 
> I think the one assert `assert(ci_env.failure_reason() != nullptr, "compiler should always document failure"); ` meets being self-documented and correct.

This pull request has been closed without being integrated.

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

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