[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:36:24 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/5] [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/5] 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/5] 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/5] 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;

>From 0b5f88733e12e630de63ccfe06f596b4f7b4bd04 Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Mon, 11 Mar 2024 11:35:51 -0500
Subject: [PATCH 5/5] Use {} in initializer

---
 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 7db3aef4606612..175868e699f18a 100644
--- a/flang/include/flang/Evaluate/integer.h
+++ b/flang/include/flang/Evaluate/integer.h
@@ -479,7 +479,7 @@ class Integer {
     if constexpr (bits < maxBits) {
       // Avoid left shifts of negative signed values (that's an undefined
       // behavior in C++).
-      auto u = std::make_unsigned_t<SINT>(ToUInt());
+      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);



More information about the flang-commits mailing list