8324833: Signed integer overflows in ABS #17617

shipilev · 2024-01-29T15:59:49Z

See the details in the bug. I think current ABS implementation is beyond repair, and we should just switch to uabs.

Additional testing:

Linux x86_64 fastdebug, all with -ftrapv (now fully passes!)
Linux x86_64 fastdebug, all

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-8324833: Signed integer overflows in ABS (Bug - P4)

Reviewers

Andrew Haley (@theRealAph - Reviewer) ⚠️ Review applies to 074679a6
Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 00d4d84b

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17617

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-29T16:00:40Z

👋 Welcome back shade! 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-01-29T16:01:31Z

@shipilev The following labels will be automatically applied to this pull request:

graal
hotspot

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

mlbridge · 2024-01-31T08:26:09Z

Webrevs

theRealAph

Looks good. All of these are improvements, I think.

openjdk · 2024-01-31T11:38:39Z

@shipilev This change is no longer ready for integration - check the PR body for details.

vnkozlov

Looks good. I submitted testing.

vnkozlov

My testing passed.

shipilev · 2024-02-01T08:25:28Z

Thanks! Any other reviews? I am planning to integrate this today.

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

src/hotspot/share/opto/ifnode.cpp

dean-long · 2024-02-01T20:31:39Z

This seems to introduce a lot of potential warnings if -Wsigned-conversion is ever enabled. Use of checked_cast() could solve this, but I only see it used in one place.
@coleenp, @kimbarrett, do we still care about keeping the code -Wsigned-conversion safe?

shipilev · 2024-02-02T06:20:03Z

Yes, I think we care. I would try a build with Wsigned-conversion and look where we should sprinkle more checked_cast-s.

dean-long · 2024-02-02T21:25:08Z

Yes, I think we care. I would try a build with Wsigned-conversion and look where we should sprinkle more checked_cast-s.

Let me try your latest with -Wsigned-conversion turned on. If I remember correctly, checked_cast doesn't always work right when the "signed-ness" changes (signed --> unsigned or unsigned --> signed).

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

dean-long · 2024-02-03T00:04:27Z

I think it's confusing and error-prone to use uabs() for signed values. Using checked_cast silences warnings but doesn't handle the undefined behavior caused by values such as min_jint and min_jlong. I suggest fixing ABS so that it first calls uabs and then safely converts to signed, checking for negative values caused by MIN INT inputs. However, that will cause some new asserts where we are relying on undefine behavior, like in exact_limit() described in JBS, so it seems like that caller needs to be fixed.
Fixing all the -Wsigned-conversion warnings in these files, or even in the changed functions, seems out of scope for this fix.

dean-long · 2024-02-03T00:12:02Z

Another alternative, leave ABS as is, but have it check for invalid inputs, and again fix the callers that are passing values like MIN_VALUE.

theRealAph · 2024-02-03T18:40:00Z

Yes, I think we care. I would try a build with Wsigned-conversion and look where we should sprinkle more checked_cast-s.

Let me try your latest with -Wsigned-conversion turned on. If I remember correctly, checked_cast doesn't always work right when the "signed-ness" changes (signed --> unsigned or unsigned --> signed).

It should work. The checked_cast from signed --> unsigned can never overflow and is always well-defined, but the overflow behaviour in the other direction is implementation defined. Having said that, it's defined the same way on every compiler we use AFAIK

theRealAph · 2024-02-03T19:14:40Z

I think it's confusing and error-prone to use uabs() for signed values. Using checked_cast silences warnings but doesn't handle the undefined behavior caused by values such as min_jint and min_jlong.

uabs itself has no UB. I don't think checked_cast has either: at worst it's implementation defined.

I suggest fixing ABS so that it first calls uabs and then safely converts to signed, checking for negative values caused by MIN INT inputs.

That's a good idea too, but it doesn't solve the problems uabs was designed to solve.

dean-long · 2024-02-05T20:37:48Z

The undefined behavior I was talking about is what abs(INT_MIN) returns on two's complement, and the problem with checked_cast is that checked_cast<int,uint>(0x80000000u) won't complain, because the bits match, but having the sign change to negative may not be what the caller wants, especially when using ABS.

theRealAph · 2024-02-06T09:07:44Z

The undefined behavior I was talking about is what abs(INT_MIN) returns on two's complement

I don't believe ABS is undefined if we go from signed to unsigned, via uabs, and back to signed. The conversion from signed->unsigned is well defined, as is the negation of the unsigned value, and the conversion from unsigned back to signed is at worst implementation defined. ABS is undefined when there is an arithmetic overflow, and in this case there is none.

dean-long · 2024-02-07T08:54:44Z

I didn't mean to say ABS() can't be implemented in C++ without UB. But if it returns a negative number, then that seems surprising. For reference, the Linux man page for the abs(3) library call says "Trying to take the absolute value of the most negative integer is not defined."

shipilev · 2024-02-22T08:56:23Z

I think it's confusing and error-prone to use uabs() for signed values. Using checked_cast silences warnings but doesn't handle the undefined behavior caused by values such as min_jint and min_jlong.

Getting back to this...

AFAICS, uabs is well-defined: all possible values of jlong passed there, including min_jlong would be mapped to a correct unsigned value. We "only" need to make sure that the math is then done over unsigned values. I agree that immediately casting to jlong after uabs is dodgy when we can reasonably get the unsigned julong value from uabs that would not fit in jlong, making it negative. I think I fixed a single place where it can possibly happen.

I am re-running tests with -ftrapv now.

dean-long · 2024-02-23T03:25:21Z

src/hotspot/share/opto/loopnode.cpp

  // overflow: max_jint - stride_con max. -1 so there's no need for a
  // loop limit check if the exit test is <= or >=.
-  int iters_limit = max_jint - ABS(stride_con) - 1;
+  int iters_limit = max_jint - uabs(stride_con) - 1;


It appears iters_limit can become -1 or -2 here, depending on the value of stride_con. See below for the problem.

dean-long · 2024-02-23T03:35:31Z

src/hotspot/share/opto/loopnode.cpp

 #endif
  // At least 2 iterations so counted loop construction doesn't fail
-  if (iters_limit/ABS(stride_con) < 2) {
+  if (iters_limit/uabs(stride_con) < 2) {


Previously negative values of iters_limit would cause us to return false here. Now the comparison is unsigned, so we can get a different result:
stride_con is max_jint
iters_limit is -1
(unsigned) -1 / max_jint is 2
so we no longer return false here.
Using uabs with signed values is error-prone.

dean-long · 2024-02-23T03:47:16Z

src/hotspot/share/opto/loopnode.cpp

        if (loop->is_range_check_if(if_proj, this, T_LONG, phi, range, offset, scale) &&
            loop->is_invariant(range) && loop->is_invariant(offset) &&
-            original_iters_limit / ABS(scale * stride_con) >= min_iters) {
-          reduced_iters_limit = MIN2(reduced_iters_limit, original_iters_limit/ABS(scale));


Mixing signed and unsigned can cause compiler warnings and a change in semantics. And again, checked_cast is not better than a raw cast here. I think it would be better to make min_iters, reduced_iters_limit, original_iters_limit all unsigned.

openjdk · 2024-03-18T09:51:43Z

@shipilev this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8324833-uabs
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

dean-long

Unfortunately, I'm now convinced this is the wrong approach and introduces bugs. I would rather see illegal inputs caught (JDK-8328934) and prevented on a case-by-case basis (see JDK-8329163 for example).

shipilev · 2024-03-28T08:12:40Z

All right, I am abandoning this PR.

Update

5d548e3

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jan 29, 2024

shipilev added 6 commits January 30, 2024 09:59

Rewrite JVMCI usage to avoid the messy overload

7d0ea58

Merge branch 'master' into JDK-8324833-uabs

f110e39

Cleaner version

91d5302

Work

e9333b6

Another attempt at JVMCI fix

d7b951c

Also handle the -stride_con with proper uabs

074679a

shipilev marked this pull request as ready for review January 31, 2024 08:22

openjdk bot added the rfr Pull request is ready for review label Jan 31, 2024

Unnecessary comment

00d4d84

theRealAph approved these changes Jan 31, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 31, 2024

vnkozlov reviewed Jan 31, 2024

View reviewed changes

vnkozlov approved these changes Feb 1, 2024

View reviewed changes

dean-long reviewed Feb 1, 2024

View reviewed changes

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp Show resolved Hide resolved

dean-long reviewed Feb 1, 2024

View reviewed changes

src/hotspot/share/opto/ifnode.cpp Show resolved Hide resolved

Touchups

96a1987

dean-long reviewed Feb 2, 2024

View reviewed changes

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp Outdated Show resolved Hide resolved

shipilev added 3 commits February 21, 2024 17:16

Merge branch 'master' into JDK-8324833-uabs

18dd071

Review comments

272fb16

Fix a place where we cast to jlong after uabs

b942242

dean-long reviewed Feb 23, 2024

View reviewed changes

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 18, 2024

dean-long suggested changes Mar 28, 2024

View reviewed changes

shipilev closed this Mar 28, 2024

shipilev deleted the JDK-8324833-uabs branch January 8, 2025 12:09

8324833: Signed integer overflows in ABS #17617

8324833: Signed integer overflows in ABS #17617

Uh oh!

Conversation

shipilev commented Jan 29, 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 Jan 29, 2024

Uh oh!

openjdk bot commented Jan 29, 2024

Uh oh!

mlbridge bot commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

theRealAph left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev commented Feb 1, 2024

Uh oh!

Uh oh!

Uh oh!

dean-long commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shipilev commented Feb 2, 2024

Uh oh!

dean-long commented Feb 2, 2024

Uh oh!

Uh oh!

dean-long commented Feb 3, 2024

Uh oh!

dean-long commented Feb 3, 2024

Uh oh!

theRealAph commented Feb 3, 2024

Uh oh!

theRealAph commented Feb 3, 2024

Uh oh!

dean-long commented Feb 5, 2024

Uh oh!

theRealAph commented Feb 6, 2024

Uh oh!

dean-long commented Feb 7, 2024

Uh oh!

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

Uh oh!

Uh oh!

dean-long Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

dean-long Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

dean-long Feb 23, 2024

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 18, 2024

Uh oh!

dean-long left a comment

Choose a reason for hiding this comment

Uh oh!

shipilev commented Mar 28, 2024

Uh oh!

Reviewers

Assignees

Labels

shipilev commented Jan 29, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 31, 2024 •

edited

Loading

openjdk bot commented Jan 31, 2024 •

edited

Loading

dean-long commented Feb 1, 2024 •

edited

Loading

shipilev commented Feb 22, 2024 •

edited

Loading