[flang-commits] [flang] [flang] Avoid left shifts of negative signed values (PR #84786)
Krzysztof Parzyszek via flang-commits
flang-commits at lists.llvm.org
Mon Mar 11 09:33:06 PDT 2024
https://github.com/kparzysz updated https://github.com/llvm/llvm-project/pull/84786
>From 566a9a5b09d66ee531b326ac1f4eae8ed5e241ee Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Mon, 11 Mar 2024 10:22:16 -0500
Subject: [PATCH 1/4] [flang] Avoid left shifts of negative signed values
Shifting left a signed, negative value is an undefined behavior in C++.
This was detected by the undefined behavior sanitizer.
---
flang/include/flang/Evaluate/integer.h | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/flang/include/flang/Evaluate/integer.h b/flang/include/flang/Evaluate/integer.h
index 977d35c7eecf48..1e5f68104f72bf 100644
--- a/flang/include/flang/Evaluate/integer.h
+++ b/flang/include/flang/Evaluate/integer.h
@@ -150,7 +150,10 @@ class Integer {
}
}
} else {
- INT signExtension{-(n < 0)};
+ // Avoid left shifts of negative signed values (that's an undefined
+ // behavior in C++).
+ auto signExtension = std::make_unsigned_t<INT>(n < 0);
+ signExtension = ~signExtension + 1;
static_assert(nBits >= partBits);
if constexpr (nBits > partBits) {
signExtension <<= nBits - partBits;
@@ -474,7 +477,12 @@ class Integer {
SINT n = ToUInt<UINT>();
constexpr std::size_t maxBits{CHAR_BIT * sizeof n};
if constexpr (bits < maxBits) {
- n |= -(n >> (bits - 1)) << bits;
+ // Avoid left shifts of negative signed values (that's an undefined
+ // behavior in C++).
+ auto u = std::make_unsigned_t<SINT>(ToUInt());
+ u = (u >> (bits - 1)) << (bits - 1); // Get the sign bit only.
+ u = ~u + 1; // Negate top bits if not 0.
+ n |= static_cast<SINT>(u);
}
return n;
}
>From 2c854eacf8db5b2ddcd12b5944d2d1659b45dd99 Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Mon, 11 Mar 2024 11:30:12 -0500
Subject: [PATCH 2/4] Fix clang-format complaint
---
flang/include/flang/Evaluate/integer.h | 36 ++++++++++++++------------
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/flang/include/flang/Evaluate/integer.h b/flang/include/flang/Evaluate/integer.h
index 1e5f68104f72bf..88b91a76268160 100644
--- a/flang/include/flang/Evaluate/integer.h
+++ b/flang/include/flang/Evaluate/integer.h
@@ -46,9 +46,9 @@ namespace Fortran::evaluate::value {
// named accordingly in ALL CAPS so that they can be referenced easily in
// the language standard.
template <int BITS, bool IS_LITTLE_ENDIAN = isHostLittleEndian,
- int PARTBITS = BITS <= 32 ? BITS : 32,
- typename PART = HostUnsignedInt<PARTBITS>,
- typename BIGPART = HostUnsignedInt<PARTBITS * 2>>
+ int PARTBITS = BITS <= 32 ? BITS : 32,
+ typename PART = HostUnsignedInt<PARTBITS>,
+ typename BIGPART = HostUnsignedInt<PARTBITS * 2>>
class Integer {
public:
static constexpr int bits{BITS};
@@ -68,8 +68,8 @@ class Integer {
static constexpr int extraPartBits{maxPartBits - partBits};
static constexpr int parts{(bits + partBits - 1) / partBits};
static_assert(parts >= 1);
- static constexpr int extraTopPartBits{
- extraPartBits + (parts * partBits) - bits};
+ static constexpr int extraTopPartBits{extraPartBits + (parts * partBits) -
+ bits};
static constexpr int topPartBits{maxPartBits - extraTopPartBits};
static_assert(topPartBits > 0 && topPartBits <= partBits);
static_assert((parts - 1) * partBits + topPartBits == bits);
@@ -228,8 +228,8 @@ class Integer {
return result;
}
- static constexpr ValueWithOverflow Read(
- const char *&pp, std::uint64_t base = 10, bool isSigned = false) {
+ static constexpr ValueWithOverflow
+ Read(const char *&pp, std::uint64_t base = 10, bool isSigned = false) {
Integer result;
bool overflow{false};
const char *p{pp};
@@ -358,7 +358,8 @@ class Integer {
static constexpr int DIGITS{bits - 1}; // don't count the sign bit
static constexpr Integer HUGE() { return MASKR(bits - 1); }
static constexpr Integer Least() { return MASKL(1); }
- static constexpr int RANGE{// in the sense of SELECTED_INT_KIND
+ static constexpr int RANGE{
+ // in the sense of SELECTED_INT_KIND
// This magic value is LOG10(2.)*1E12.
static_cast<int>(((bits - 1) * 301029995664) / 1000000000000)};
@@ -462,7 +463,8 @@ class Integer {
return CompareUnsigned(y);
}
- template <typename UINT = std::uint64_t> constexpr UINT ToUInt() const {
+ template <typename UINT = std::uint64_t>
+ constexpr UINT ToUInt() const {
UINT n{LEPart(0)};
std::size_t filled{partBits};
constexpr std::size_t maxBits{CHAR_BIT * sizeof n};
@@ -481,7 +483,7 @@ class Integer {
// behavior in C++).
auto u = std::make_unsigned_t<SINT>(ToUInt());
u = (u >> (bits - 1)) << (bits - 1); // Get the sign bit only.
- u = ~u + 1; // Negate top bits if not 0.
+ u = ~u + 1; // Negate top bits if not 0.
n |= static_cast<SINT>(u);
}
return n;
@@ -555,8 +557,8 @@ class Integer {
}
} else {
for (; j > shiftParts; --j) {
- result.SetLEPart(j,
- ((LEPart(j - shiftParts) << bitShift) |
+ result.SetLEPart(
+ j, ((LEPart(j - shiftParts) << bitShift) |
(LEPart(j - shiftParts - 1) >> (partBits - bitShift))));
}
if (j == shiftParts) {
@@ -657,9 +659,9 @@ class Integer {
}
} else {
for (; j + shiftParts + 1 < parts; ++j) {
- result.SetLEPart(j,
- (LEPart(j + shiftParts) >> bitShift) |
- (LEPart(j + shiftParts + 1) << (partBits - bitShift)));
+ result.SetLEPart(
+ j, (LEPart(j + shiftParts) >> bitShift) |
+ (LEPart(j + shiftParts + 1) << (partBits - bitShift)));
}
if (j + shiftParts + 1 == parts) {
result.LEPart(j++) = LEPart(parts - 1) >> bitShift;
@@ -756,8 +758,8 @@ class Integer {
}
// Unsigned addition with carry.
- constexpr ValueWithCarry AddUnsigned(
- const Integer &y, bool carryIn = false) const {
+ constexpr ValueWithCarry AddUnsigned(const Integer &y,
+ bool carryIn = false) const {
Integer sum{nullptr};
BigPart carry{carryIn};
for (int j{0}; j + 1 < parts; ++j) {
>From a28fc2410548549a8ae4570e89f7857683a8d6c7 Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Mon, 11 Mar 2024 11:31:37 -0500
Subject: [PATCH 3/4] Revert "Fix clang-format complaint"
This reverts commit 2c854eacf8db5b2ddcd12b5944d2d1659b45dd99.
---
flang/include/flang/Evaluate/integer.h | 36 ++++++++++++--------------
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/flang/include/flang/Evaluate/integer.h b/flang/include/flang/Evaluate/integer.h
index 88b91a76268160..1e5f68104f72bf 100644
--- a/flang/include/flang/Evaluate/integer.h
+++ b/flang/include/flang/Evaluate/integer.h
@@ -46,9 +46,9 @@ namespace Fortran::evaluate::value {
// named accordingly in ALL CAPS so that they can be referenced easily in
// the language standard.
template <int BITS, bool IS_LITTLE_ENDIAN = isHostLittleEndian,
- int PARTBITS = BITS <= 32 ? BITS : 32,
- typename PART = HostUnsignedInt<PARTBITS>,
- typename BIGPART = HostUnsignedInt<PARTBITS * 2>>
+ int PARTBITS = BITS <= 32 ? BITS : 32,
+ typename PART = HostUnsignedInt<PARTBITS>,
+ typename BIGPART = HostUnsignedInt<PARTBITS * 2>>
class Integer {
public:
static constexpr int bits{BITS};
@@ -68,8 +68,8 @@ class Integer {
static constexpr int extraPartBits{maxPartBits - partBits};
static constexpr int parts{(bits + partBits - 1) / partBits};
static_assert(parts >= 1);
- static constexpr int extraTopPartBits{extraPartBits + (parts * partBits) -
- bits};
+ static constexpr int extraTopPartBits{
+ extraPartBits + (parts * partBits) - bits};
static constexpr int topPartBits{maxPartBits - extraTopPartBits};
static_assert(topPartBits > 0 && topPartBits <= partBits);
static_assert((parts - 1) * partBits + topPartBits == bits);
@@ -228,8 +228,8 @@ class Integer {
return result;
}
- static constexpr ValueWithOverflow
- Read(const char *&pp, std::uint64_t base = 10, bool isSigned = false) {
+ static constexpr ValueWithOverflow Read(
+ const char *&pp, std::uint64_t base = 10, bool isSigned = false) {
Integer result;
bool overflow{false};
const char *p{pp};
@@ -358,8 +358,7 @@ class Integer {
static constexpr int DIGITS{bits - 1}; // don't count the sign bit
static constexpr Integer HUGE() { return MASKR(bits - 1); }
static constexpr Integer Least() { return MASKL(1); }
- static constexpr int RANGE{
- // in the sense of SELECTED_INT_KIND
+ static constexpr int RANGE{// in the sense of SELECTED_INT_KIND
// This magic value is LOG10(2.)*1E12.
static_cast<int>(((bits - 1) * 301029995664) / 1000000000000)};
@@ -463,8 +462,7 @@ class Integer {
return CompareUnsigned(y);
}
- template <typename UINT = std::uint64_t>
- constexpr UINT ToUInt() const {
+ template <typename UINT = std::uint64_t> constexpr UINT ToUInt() const {
UINT n{LEPart(0)};
std::size_t filled{partBits};
constexpr std::size_t maxBits{CHAR_BIT * sizeof n};
@@ -483,7 +481,7 @@ class Integer {
// behavior in C++).
auto u = std::make_unsigned_t<SINT>(ToUInt());
u = (u >> (bits - 1)) << (bits - 1); // Get the sign bit only.
- u = ~u + 1; // Negate top bits if not 0.
+ u = ~u + 1; // Negate top bits if not 0.
n |= static_cast<SINT>(u);
}
return n;
@@ -557,8 +555,8 @@ class Integer {
}
} else {
for (; j > shiftParts; --j) {
- result.SetLEPart(
- j, ((LEPart(j - shiftParts) << bitShift) |
+ result.SetLEPart(j,
+ ((LEPart(j - shiftParts) << bitShift) |
(LEPart(j - shiftParts - 1) >> (partBits - bitShift))));
}
if (j == shiftParts) {
@@ -659,9 +657,9 @@ class Integer {
}
} else {
for (; j + shiftParts + 1 < parts; ++j) {
- result.SetLEPart(
- j, (LEPart(j + shiftParts) >> bitShift) |
- (LEPart(j + shiftParts + 1) << (partBits - bitShift)));
+ result.SetLEPart(j,
+ (LEPart(j + shiftParts) >> bitShift) |
+ (LEPart(j + shiftParts + 1) << (partBits - bitShift)));
}
if (j + shiftParts + 1 == parts) {
result.LEPart(j++) = LEPart(parts - 1) >> bitShift;
@@ -758,8 +756,8 @@ class Integer {
}
// Unsigned addition with carry.
- constexpr ValueWithCarry AddUnsigned(const Integer &y,
- bool carryIn = false) const {
+ constexpr ValueWithCarry AddUnsigned(
+ const Integer &y, bool carryIn = false) const {
Integer sum{nullptr};
BigPart carry{carryIn};
for (int j{0}; j + 1 < parts; ++j) {
>From 211a2d7510294264a6dc19ed94d9bf0c1c06b198 Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Mon, 11 Mar 2024 11:32:34 -0500
Subject: [PATCH 4/4] Fix the comment placement, not everything...
---
flang/include/flang/Evaluate/integer.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/flang/include/flang/Evaluate/integer.h b/flang/include/flang/Evaluate/integer.h
index 1e5f68104f72bf..7db3aef4606612 100644
--- a/flang/include/flang/Evaluate/integer.h
+++ b/flang/include/flang/Evaluate/integer.h
@@ -481,7 +481,7 @@ class Integer {
// behavior in C++).
auto u = std::make_unsigned_t<SINT>(ToUInt());
u = (u >> (bits - 1)) << (bits - 1); // Get the sign bit only.
- u = ~u + 1; // Negate top bits if not 0.
+ u = ~u + 1; // Negate top bits if not 0.
n |= static_cast<SINT>(u);
}
return n;
More information about the flang-commits
mailing list