Bug ID: JDK-8332632 Redundant assert "compiler should always document failure: %s" with possible UB

Type: Enhancement
Component: hotspot
Sub-Component: compiler
Affected Version: 21,22,23

Priority: P5
Status: Open
Resolution: Unresolved

Submitted: 2024-05-21
Updated: 2024-05-24

Other
tbdUnresolved

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());
```

`ciEnv` functions:
```
bool failing() const { return _failure_reason.get() != nullptr; }
const char* failure_reason() const { return _failure_reason.get(); }
```
So the code above is:
```
    if (_failure_reason.get() == nullptr && !task->is_success()) {
      assert(_failure_reason.get() != nullptr, "expect failure reason");
      assert(false, "compiler should always document failure: %s", ci_env.failure_reason());
```

The condition `_failure_reason.get() != nullptr` is always false in the assert because we can get to it only if `_failure_reason.get() == nullptr`. So it will be triggered. The second assert is redundant.

The code above should be changed to:
```
   if (!ci_env.failing() && !task->is_success()) {
      assert(ci_env.failure_reason() != nullptr, "compiler should always document failure");
      // The compiler elected, without comment, not to register a result.
```

`assert(false, "compiler should always document failure: %s", ci_env.failure_reason()); ` has UB because `ci_env.failure_reason()` is `nullptr`.

A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/19395 Date: 2024-05-24 14:27:13 +0000
24-05-2024
Hi [~epeter], > But it is maybe still better to keep it, just in case someone messes up something with the failure_reason code. Could you please explain how it would help with this? Why would having the following assert not serve this purpose: `assert(ci_env.failure_reason() != nullptr, "compiler should always document failure"); `? > I think that is not true, since the first assert checks that "ci_env.failure_reason() != nullptr". A compiler sees the following: 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 will 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.
24-05-2024
In any case, converting this to a cleanup RFE.
22-05-2024
Why is this a bug? I agree the second assert looks redundant. But it is maybe still better to keep it, just in case someone messes up something with the failure_reason code. And you say: BTW, `assert(false, "compiler should always document failure: %s", ci_env.failure_reason()); ` has UB because `ci_env.failure_reason()` is `nullptr`. I think that is not true, since the first assert checks that "ci_env.failure_reason() != nullptr". I would vote not to change the code here.
22-05-2024