Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1826043002:
S390: Fixed s390 simulation check for underflow in subtraction. (Closed)
|
Created:
4 years, 9 months ago by bcleung Modified:
4 years, 8 months ago Reviewers:
jochen (gone - plz use gerrit), Jakob Kummerow, JoranSiu, danno, mtbrandyberry, john.yan, michael_dawson CC:
v8-reviews_googlegroups.com Base URL:
https://github.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionS390: Fixed s390 simulation check for underflow in subtraction.
Clang optimizes away CheckOverflowForIntSub at any opt
level (includes -O1, -O2, -O3) into a false statement,
resulting in incorrect values being returned. As the C++
standard considers overflows to be undefined behaviour,
this is technically correct as compilers can assume that
overflows never occur, but problematic in our case (where
overflows do occur, and a specific result is expected).
This change replaces the original check with a call to a
function that is optimized in a manner that returns correct output.
R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu@ca.ibm.com,jkummerow@chromium.org,jochen@chromium.org,danno@chromium.org
BUG=
Committed: https://crrev.com/0d24a0fcfe8c7e1f546b5747679dcee51d0f071e
Cr-Commit-Position: refs/heads/master@{#35082}
Patch Set 1 #Patch Set 2 : Added explicit typing for signed int over/underflow checking #Patch Set 3 : Added a TODO re: indirect static casting #
Messages
Total messages: 25 (14 generated)
Description was changed from ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang incorrectly optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3), resulting in incorrect values being returned. This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... , BUG= ========== to ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang incorrectly optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3), resulting in incorrect values being returned. This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ==========
bryleun@ca.ibm.com changed reviewers: + danno@chromium.org, jkummerow@chromium.org, jochen@chromium.org
can you please file a bug against clang for this issue?
On 2016/03/24 14:46:24, jochen - slow wrote: > can you please file a bug against clang for this issue? Technically, clang is doing the right thing. The C++ standard states that overflows result in undefined behaviour, and therefore compilers are allowed to assume that overflow never happens. The overflow check is optimized away into a function that always returns false for this reason, which is correct per the C++ standard, but problematic in our case. For this reason, it's not a bug in clang. The change here replaces the existing overflow check with one that isn't optimized away to a FALSE statement.
Description was changed from ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang incorrectly optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3), resulting in incorrect values being returned. This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ========== to ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ==========
On 2016/03/24 15:04:51, bryleun wrote: > On 2016/03/24 14:46:24, jochen - slow wrote: > > can you please file a bug against clang for this issue? > > Technically, clang is doing the right thing. The C++ standard states that > overflows result in undefined behaviour, and therefore compilers are allowed to > assume that overflow never happens. The overflow check is optimized away into a > function that always returns false for this reason, which is correct per the C++ > standard, but problematic in our case. For this reason, it's not a bug in clang. > > The change here replaces the existing overflow check with one that isn't > optimized away to a FALSE statement. Updated description to clarify.
Description was changed from ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ========== to ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ==========
Patch set 3 lgtm
On 2016/03/28 13:55:00, bcleung wrote: Another approach here would be to use the `-fwrapv` compiler flag: https://gcc.gnu.org/onlinedocs/gcc-4.0.2/gcc/Code-Gen-Options.html Both clang and gcc accept this flag.
The CQ bit was checked by bryleun@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826043002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826043002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by bryleun@ca.ibm.com
The CQ bit was checked by bryleun@ca.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1826043002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826043002/40001
Message was sent while issue was closed.
Description was changed from ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ========== to ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ========== to ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= Committed: https://crrev.com/0d24a0fcfe8c7e1f546b5747679dcee51d0f071e Cr-Commit-Position: refs/heads/master@{#35082} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0d24a0fcfe8c7e1f546b5747679dcee51d0f071e Cr-Commit-Position: refs/heads/master@{#35082}
Message was sent while issue was closed.
Description was changed from ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= Committed: https://crrev.com/0d24a0fcfe8c7e1f546b5747679dcee51d0f071e Cr-Commit-Position: refs/heads/master@{#35082} ========== to ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu..., BUG= Committed: https://crrev.com/0d24a0fcfe8c7e1f546b5747679dcee51d0f071e Cr-Commit-Position: refs/heads/master@{#35082} ==========
Message was sent while issue was closed.
bryleun@ca.ibm.com changed reviewers: - danno@chromium.org, jkummerow@chromium.org, jochen@chromium.org
Message was sent while issue was closed.
Description was changed from ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu..., BUG= Committed: https://crrev.com/0d24a0fcfe8c7e1f546b5747679dcee51d0f071e Cr-Commit-Position: refs/heads/master@{#35082} ========== to ========== S390: Fixed s390 simulation check for underflow in subtraction. Clang optimizes away CheckOverflowForIntSub at any opt level (includes -O1, -O2, -O3) into a false statement, resulting in incorrect values being returned. As the C++ standard considers overflows to be undefined behaviour, this is technically correct as compilers can assume that overflows never occur, but problematic in our case (where overflows do occur, and a specific result is expected). This change replaces the original check with a call to a function that is optimized in a manner that returns correct output. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= Committed: https://crrev.com/0d24a0fcfe8c7e1f546b5747679dcee51d0f071e Cr-Commit-Position: refs/heads/master@{#35082} ==========
Message was sent while issue was closed.
bryleun@ca.ibm.com changed reviewers: + danno@chromium.org, jkummerow@chromium.org, jochen@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
