8338449: ubsan: division by zero in sharedRuntimeTrans.cpp #21500

MBaesken · 2024-10-14T14:08:05Z

When running with ubsan enabled binaries on Linux ppc64le, some divisions by zero are detected in the java/lang/Math jdk jtreg tests dealing with log - related calculations.

java/lang/Math/Log10Tests.java: Tests for {Math, StrictMath}.log10

/jdk/src/hotspot/share/runtime/sharedRuntimeTrans.cpp:219:27: runtime error: division by zero
#0 0x7fffa7e14abc in SharedRuntime::dlog10(double) (/build_ubsan/images/jdk/lib/server/libjvm.so+0x69f4abc)
#1 0x7fff8b8fc8e8 ()

test
java/lang/Math/LogTests.java: Tests for {Math, StrictMath}.log

/jdk/src/hotspot/share/runtime/sharedRuntimeTrans.cpp:125:27: runtime error: division by zero
#0 0x7fff887f48bc in __ieee754_log(double) (/build_ubsan/images/jdk/lib/server/libjvm.so+0x69f48bc)
#1 0x7fff6b8fc768 ()

test
java/lang/Math/PowTests.java: Tests for {Math, StrictMath}.pow

/jdk/src/hotspot/share/runtime/sharedRuntimeTrans.cpp:508:23: runtime error: division by zero
#0 0x7fff92fd61f0 in SharedRuntime::dpow(double, double) (/build_ubsan/images/jdk/lib/server/libjvm.so+0x69f61f0)
#1 0x7fff7701c8ec ()

The coding does intentional division by zero.
We should probably check for IEEE compliance (or rewrite the coding); but checking for compliance might be sufficient.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8338449: ubsan: division by zero in sharedRuntimeTrans.cpp (Bug - P4)

Reviewers

Lutz Schmidt (@RealLucy - Reviewer)
Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21500/head:pull/21500
$ git checkout pull/21500

Update a local copy of the PR:
$ git checkout pull/21500
$ git pull https://git.openjdk.org/jdk.git pull/21500/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21500

View PR using the GUI difftool:
$ git pr show -t 21500

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21500.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-14T14:08:42Z

👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-14T14:09:28Z

@MBaesken This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8338449: ubsan: division by zero in sharedRuntimeTrans.cpp

Reviewed-by: lucy, kbarrett

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 138 new commits pushed to the master branch:

d6eddcd: 8327624: Remove VM implementation that bypass verification for core reflection
28147da: 8341913: Support CDS heap dumping for Shenandoah and Epsilon
c61d2c5: 8341445: DecimalFormatSymbols setters should throw NPE
aafc8d0: 8342850: Change ProblemList to have LimitDirectMemory refer to JDK-8342849
893266c: 8342601: AArch64: Micro-optimize bit shift in copy_memory
f9852ae: 8342839: Malformed copyright in StringNameTable since JDK-8342806
af5e532: 8342806: Desugar capturing lambda in StringNameTable
01b681c: 8326949: Authorization header is removed when a proxy Authenticator is set on HttpClient
d10eecf: 8341776: Remove unused enum values from LambdaForm$Kind
3bba0f3: 8342701: [PPC64] TestOSRLotsOfLocals.java crashes
... and 128 more: https://git.openjdk.org/jdk/compare/1581508988141bfb420d97759138203f30926b35...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-14T14:10:07Z

@MBaesken The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-10-14T14:13:45Z

Webrevs

00: Full (e44ff114)

dholmes-ora · 2024-10-15T12:18:33Z

src/hotspot/share/runtime/sharedRuntimeTrans.cpp

 static double __ieee754_log(double x) {
  double hfsq,f,s,z,R,w,t1,t2,dk;
  int k,hx,i,j;
  unsigned lx;

+  static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 required");


I'm not sure what the implications of this actually are. Do we really need it? Maybe @jddarcy can comment?

I'm not sure what the implications of this actually are. Do we really need it? Maybe @jddarcy can comment?

It needs to return -Inf. ubsan seems wrong here: this IEEE-754 arithmetic is well defined, and has been for a very long while.

Ubsan is just following the C++ standard
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf
5.6 Multiplicative operators
....
'The binary / operator yields the quotient, and the binary % operator yields the remainder from the division
of the first expression by the second. If the second operand of / or % is zero the behavior is undefined.'

See also https://stackoverflow.com/questions/42926763/the-behaviour-of-floating-point-division-by-zero .

However on our set of platforms in OpenJDK we probably get away with the expected results when dividing by 0 (because they all seem to follow IEEE-754).
That's why I just added the static_assert.

To me, this feels like the C++ standard is not compatible with IEEE-754 arithmetic. Undefined behavior would give the optimizer freedom to do whatever it likes to do. That is in contrast to the well-defined IEEE-754 requirement.

ATTRIBUTE_NO_UBSAN suppresses conditions found by ubsan instrumentation. If
ubsan is enabled, the compiler's optimizer presumably doesn't take advantage
of the no-UB assumption, as that would defeat the point of ubsan. But in a
non-ubsan-enabled build, I think that attribute does nothing and the compiler
may take advantage of the no-UB assumption. So that part of the change may not
do anything useful for non-ubsan builds.

cppreference is inconsistent about this. It first states that division by zero
is UB, as per the standard. But then it says if the type is_iec599, then
division by zero returns the IEEE defined value (correctly signed infinity if
lhs is non-zero, NaN if lhs is zero) and raises the appropriate floating point
trap. I think this claim makes sense for a C compiler that supports Annex F.
But C++ isn't C, and I don't see anything comparable in a C++ standard.

I can't find any text in any version of either the C or C++ standard to
support the claimed is_iec599 behavior though.

I did find this, which has some interesting bits.
https://stackoverflow.com/questions/42926763/the-behaviour-of-floating-point-division-by-zero

For example, it is suggested that claiming is_iec599 implicity defines the
behavior for floating point division by zero (among other things). It is also
mentioned that gcc does that, but (some version of?) clang-the-compiler
doesn't implement iec599, even though the standard library one is using might
claim so. Hm...

It also mentions that the default for -fsanitize=undefined for clang includes
float-divide-by-zero, while gcc does not. Any chance the reported issue is
arising with clang as the compiler?

Never mind my question about whether this was happening with clang as the compiler rather than gcc.
Our ubsan configuration explicitly includes -fsantize=float-divide-by-zero.
https://github.com/openjdk/jdk/blame/8bcd4920f1b03d0ef8e295e53557c629f05ceaa4/make/autoconf/jdk-options.m4#L516

I'm not sure what the implications of this actually are. Do we really need it? Maybe @jddarcy can comment?

Catching up on email, whatever the appropriate C/C++ idiom is, it is appropriate for this file to assert "use IEEE 754 semantics for floating-point operations." The divide by zero behavior is well-defined by IEEE 754/IEC 559.

It's not clear there is any in-language mechanism (whether portable or compiler-specific) for reliably requesting or verifying that one will get that behavior. A static_assert of is_iec599 might be sufficient verification, but the standard is at least unclear on that. But that seems to be the best we've got.

MBaesken · 2024-10-22T13:18:26Z

java/text/Format/CompactNumberFormat/TestCompactNumber.java
or
jdk/incubator/vector/Double128VectorTests.java

trigger the issue too (when using ubsan - enabled binaries on linux ppc64le).
So any reviews ?

RealLucy

After all the discussion: LGTM.

dean-long · 2024-10-22T19:27:56Z

If this file requires certain IEEE divide-by-zero semantics, then should we also add static asserts for those? Something like:
static_assert(-1.0 / 0.0 == -INFINITY, "IEEE 754 required");
static_assert(1.0 / 0.0 == INFINITY, "IEEE 754 required");
static_assert(std::isnan(0.0 / 0.0), "IEEE 754 required");

kimbarrett · 2024-10-22T20:19:28Z

If this file requires certain IEEE divide-by-zero semantics, then should we also add static asserts for those? Something like: static_assert(-1.0 / 0.0 == -INFINITY, "IEEE 754 required"); static_assert(1.0 / 0.0 == INFINITY, "IEEE 754 required"); static_assert(std::isnan(0.0 / 0.0), "IEEE 754 required");

These expressions are UB, so not constexpr, so shouldn't be usable in a static_assert. And indeed, gcc says

static_assert_iec599.cpp:4:27: error: non-constant condition for static assertion
    4 |   static_assert(1.0 / 0.0 == INFINITY, "1/0 failed");
      |                           ^

kimbarrett

I don't see a better approach than the addition of the static_asserts. I wish
the language standards were clearer, but oh well...

dean-long · 2024-10-22T22:32:47Z

These expressions are UB, so not constexpr, so shouldn't be usable in a static_assert. And indeed, gcc says

OK, then how about regular runtime asserts? They would only be in debug builds and the compiler should be able to figure out the result at build time.

kimbarrett · 2024-10-23T01:07:45Z

These expressions are UB, so not constexpr, so shouldn't be usable in a static_assert. And indeed, gcc says

OK, then how about regular runtime asserts? They would only be in debug builds and the compiler should be able to figure out the result at build time.

UB => nasal demons, including passing the test on a platform that should fail.

MBaesken · 2024-10-23T07:07:28Z

Thanks for the reviews !

The other static_assert could be added in a separate PR if they are really desired.

/integrate

openjdk · 2024-10-23T07:08:11Z

Going to push as commit 37cfaa8.
Since your change was applied there have been 143 commits pushed to the master branch:

a1ef818: 8342825: Fix order of @param tags in module java.desktop
cdad728: 8342646: JTREG_TEST_THREAD_FACTORY in testing.md should be TEST_THREAD_FACTORY
018db8c: 8342809: C2 hits "assert(is_If()) failed: invalid node class: Con" during IGVN due to unhandled top
f1f1537: 8341453: java/awt/a11y/AccessibleJTableTest.java fails in some cases where the test tables are not visible
476d0f1: 8339309: unused-variable warnings happen in libfontmanager
d6eddcd: 8327624: Remove VM implementation that bypass verification for core reflection
28147da: 8341913: Support CDS heap dumping for Shenandoah and Epsilon
c61d2c5: 8341445: DecimalFormatSymbols setters should throw NPE
aafc8d0: 8342850: Change ProblemList to have LimitDirectMemory refer to JDK-8342849
893266c: 8342601: AArch64: Micro-optimize bit shift in copy_memory
... and 133 more: https://git.openjdk.org/jdk/compare/1581508988141bfb420d97759138203f30926b35...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-23T07:08:20Z

@MBaesken Pushed as commit 37cfaa8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8338449

e44ff11

openjdk bot changed the title ~~JDK-8338449: ubsan: division by zero in sharedRuntimeTrans.cpp~~ 8338449: ubsan: division by zero in sharedRuntimeTrans.cpp Oct 14, 2024

openjdk bot added the rfr Pull request is ready for review label Oct 14, 2024

openjdk bot added the hotspot [email protected] label Oct 14, 2024

dholmes-ora reviewed Oct 15, 2024

View reviewed changes

RealLucy approved these changes Oct 22, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 22, 2024

kimbarrett approved these changes Oct 22, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 23, 2024

openjdk bot closed this Oct 23, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 23, 2024

8338449: ubsan: division by zero in sharedRuntimeTrans.cpp #21500

8338449: ubsan: division by zero in sharedRuntimeTrans.cpp #21500

Uh oh!

Conversation

MBaesken commented Oct 14, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 14, 2024

Uh oh!

openjdk bot commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 14, 2024

Uh oh!

mlbridge bot commented Oct 14, 2024

Webrevs

Uh oh!

dholmes-ora Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

theRealAph Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

MBaesken Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

RealLucy Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

kimbarrett Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimbarrett Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

jddarcy Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

kimbarrett Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

MBaesken commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RealLucy left a comment

Choose a reason for hiding this comment

Uh oh!

dean-long commented Oct 22, 2024

Uh oh!

kimbarrett commented Oct 22, 2024

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

dean-long commented Oct 22, 2024

Uh oh!

kimbarrett commented Oct 23, 2024

Uh oh!

MBaesken commented Oct 23, 2024

Uh oh!

openjdk bot commented Oct 23, 2024

Uh oh!

openjdk bot commented Oct 23, 2024

Uh oh!

Uh oh!

MBaesken commented Oct 14, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 14, 2024 •

edited

Loading

kimbarrett Oct 22, 2024 •

edited

Loading

MBaesken commented Oct 22, 2024 •

edited

Loading