Giacomo Travaglini has submitted this change. ( https://gem5-review.googlesource.com/c/public/gem5/+/26990 )

Change subject: arch-arm: Fix aapcs32/aapcs64 compilation issues
......................................................................

arch-arm: Fix aapcs32/aapcs64 compilation issues

Some compilers won't build ARM due to how guest ABI
has been implemented.

The error is: "left shift count >= width of type"
[-Werror=shift-count-overflow]

The error is triggered when there is a left shift > the variable size
(in bits); this leads to undefined behaviour.

This is a compile time vs run time problem; the code is technically
fine, but the compiler is not able to understand this.

For example in aapcs64:

struct Argument<Aapcs64, Integer, typename std::enable_if<
 std::is_integral<Integer>::value>::type> : public Aapcs64ArgumentBase
{
    [...]
    if (sizeof(Integer) == 16 && state.ngrn + 1 <= state.MAX_GRN) {
        Integer low = tc->readIntReg(state.ngrn++);
        Integer high = tc->readIntReg(state.ngrn++);
        high = high << 64;
        return high | low;
    }
}

Even if the sizeof operator will be evaluated at compile time,
the block will be executed at runtime: the block will still be part of
the code if Integer = uint32_t.
The compiler will then throw an error because we are left shifting an
uint32_t by 64 bits.

Error arising on:
Compiler: gcc/5.4.0
Distro: Ubuntu 16.04 LTS

Change-Id: Iaafe030b7262c5fb162afe7118ae592a1a759a58
Signed-off-by: Giacomo Travaglini <giacomo.travagl...@arm.com>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/26990
Reviewed-by: Gabe Black <gabebl...@google.com>
Tested-by: kokoro <noreply+kok...@google.com>
---
M src/arch/arm/aapcs32.hh
M src/arch/arm/aapcs64.hh
2 files changed, 91 insertions(+), 27 deletions(-)

Approvals:
  Gabe Black: Looks good to me, approved
  Giacomo Travaglini: Looks good to me, approved
  kokoro: Regressions pass



diff --git a/src/arch/arm/aapcs32.hh b/src/arch/arm/aapcs32.hh
index bfeb553..fd63483 100644
--- a/src/arch/arm/aapcs32.hh
+++ b/src/arch/arm/aapcs32.hh
@@ -131,39 +131,78 @@

 template <typename Integer>
 struct Result<Aapcs32, Integer, typename std::enable_if<
-    std::is_integral<Integer>::value>::type>
+ std::is_integral<Integer>::value && (sizeof(Integer) < sizeof(uint32_t))
+    >::type>
 {
     static void
     store(ThreadContext *tc, const Integer &i)
     {
-        if (sizeof(Integer) < sizeof(uint32_t)) {
-            uint32_t val = std::is_signed<Integer>::value ?
-                    sext<sizeof(Integer) * 8>(i) : i;
-            tc->setIntReg(ArmISA::INTREG_R0, val);
-        } else if (sizeof(Integer) == sizeof(uint32_t) ||
-                   std::is_same<Integer, Addr>::value) {
+        uint32_t val = std::is_signed<Integer>::value ?
+                sext<sizeof(Integer) * 8>(i) : i;
+        tc->setIntReg(ArmISA::INTREG_R0, val);
+    }
+};
+
+template <typename Integer>
+struct Result<Aapcs32, Integer, typename std::enable_if<
+ std::is_integral<Integer>::value && (sizeof(Integer) == sizeof(uint32_t))
+    >::type>
+{
+    static void
+    store(ThreadContext *tc, const Integer &i)
+    {
+        tc->setIntReg(ArmISA::INTREG_R0, (uint32_t)i);
+    }
+};
+
+template <typename Integer>
+struct Result<Aapcs32, Integer, typename std::enable_if<
+ std::is_integral<Integer>::value && (sizeof(Integer) == sizeof(uint64_t))
+    >::type>
+{
+    static void
+    store(ThreadContext *tc, const Integer &i)
+    {
+        if (std::is_same<Integer, Addr>::value) {
             tc->setIntReg(ArmISA::INTREG_R0, (uint32_t)i);
-        } else if (sizeof(Integer) == sizeof(uint64_t)) {
-            if (ArmISA::byteOrder(tc) == LittleEndianByteOrder) {
-                tc->setIntReg(ArmISA::INTREG_R0, (uint32_t)(i >> 0));
-                tc->setIntReg(ArmISA::INTREG_R1, (uint32_t)(i >> 32));
-            } else {
-                tc->setIntReg(ArmISA::INTREG_R0, (uint32_t)(i >> 32));
-                tc->setIntReg(ArmISA::INTREG_R1, (uint32_t)(i >> 0));
-            }
+        } else if (ArmISA::byteOrder(tc) == LittleEndianByteOrder) {
+            tc->setIntReg(ArmISA::INTREG_R0, (uint32_t)(i >> 0));
+            tc->setIntReg(ArmISA::INTREG_R1, (uint32_t)(i >> 32));
+        } else {
+            tc->setIntReg(ArmISA::INTREG_R0, (uint32_t)(i >> 32));
+            tc->setIntReg(ArmISA::INTREG_R1, (uint32_t)(i >> 0));
         }
     }
 };

 template <typename Integer>
 struct Argument<Aapcs32, Integer, typename std::enable_if<
-    std::is_integral<Integer>::value>::type> : public Aapcs32ArgumentBase
+ std::is_integral<Integer>::value && (sizeof(Integer) <= sizeof(uint32_t))
+    >::type> : public Aapcs32ArgumentBase
 {
     static Integer
     get(ThreadContext *tc, Aapcs32::State &state)
     {
-        if ((sizeof(Integer) <= sizeof(uint32_t) ||
-                std::is_same<Integer, Addr>::value) &&
+        if (state.ncrn <= state.MAX_CRN) {
+            return tc->readIntReg(state.ncrn++);
+        }
+
+        // Max out the ncrn since we effectively exhausted it.
+        state.ncrn = state.MAX_CRN + 1;
+
+        return loadFromStack<Integer>(tc, state);
+    }
+};
+
+template <typename Integer>
+struct Argument<Aapcs32, Integer, typename std::enable_if<
+ std::is_integral<Integer>::value && (sizeof(Integer) > sizeof(uint32_t))
+    >::type> : public Aapcs32ArgumentBase
+{
+    static Integer
+    get(ThreadContext *tc, Aapcs32::State &state)
+    {
+        if (std::is_same<Integer, Addr>::value &&
                 state.ncrn <= state.MAX_CRN) {
             return tc->readIntReg(state.ncrn++);
         }
diff --git a/src/arch/arm/aapcs64.hh b/src/arch/arm/aapcs64.hh
index 9d7623a..203846d 100644
--- a/src/arch/arm/aapcs64.hh
+++ b/src/arch/arm/aapcs64.hh
@@ -229,14 +229,30 @@
 // This will pick up Addr as well, which should be used for guest pointers.
 template <typename Integer>
 struct Argument<Aapcs64, Integer, typename std::enable_if<
-    std::is_integral<Integer>::value>::type> : public Aapcs64ArgumentBase
+    std::is_integral<Integer>::value && (sizeof(Integer) <= 8)
+    >::type> : public Aapcs64ArgumentBase
 {
     static Integer
     get(ThreadContext *tc, Aapcs64::State &state)
     {
-        if (sizeof(Integer) <= 8 && state.ngrn <= state.MAX_GRN)
+        if (state.ngrn <= state.MAX_GRN)
             return tc->readIntReg(state.ngrn++);

+        // Max out ngrn since we've effectively saturated it.
+        state.ngrn = state.MAX_GRN + 1;
+
+        return loadFromStack<Integer>(tc, state);
+    }
+};
+
+template <typename Integer>
+struct Argument<Aapcs64, Integer, typename std::enable_if<
+    std::is_integral<Integer>::value && (sizeof(Integer) > 8)
+    >::type> : public Aapcs64ArgumentBase
+{
+    static Integer
+    get(ThreadContext *tc, Aapcs64::State &state)
+    {
         if (alignof(Integer) == 16 && (state.ngrn % 2))
             state.ngrn++;

@@ -256,17 +272,26 @@

 template <typename Integer>
 struct Result<Aapcs64, Integer, typename std::enable_if<
-    std::is_integral<Integer>::value>::type>
+    std::is_integral<Integer>::value && (sizeof(Integer) <= 8)
+    >::type>
 {
     static void
     store(ThreadContext *tc, const Integer &i)
     {
-        if (sizeof(Integer) <= 8) {
-            tc->setIntReg(0, i);
-        } else {
-            tc->setIntReg(0, (uint64_t)i);
-            tc->setIntReg(1, (uint64_t)(i >> 64));
-        }
+        tc->setIntReg(0, i);
+    }
+};
+
+template <typename Integer>
+struct Result<Aapcs64, Integer, typename std::enable_if<
+    std::is_integral<Integer>::value && (sizeof(Integer) > 8)
+    >::type>
+{
+    static void
+    store(ThreadContext *tc, const Integer &i)
+    {
+        tc->setIntReg(0, (uint64_t)i);
+        tc->setIntReg(1, (uint64_t)(i >> 64));
     }
 };


--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/26990
To unsubscribe, or for help writing mail filters, visit https://gem5-review.googlesource.com/settings

Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: Iaafe030b7262c5fb162afe7118ae592a1a759a58
Gerrit-Change-Number: 26990
Gerrit-PatchSet: 3
Gerrit-Owner: Giacomo Travaglini <giacomo.travagl...@arm.com>
Gerrit-Reviewer: Bobby R. Bruce <bbr...@ucdavis.edu>
Gerrit-Reviewer: Gabe Black <gabebl...@google.com>
Gerrit-Reviewer: Giacomo Travaglini <giacomo.travagl...@arm.com>
Gerrit-Reviewer: Jason Lowe-Power <power...@gmail.com>
Gerrit-Reviewer: kokoro <noreply+kok...@google.com>
Gerrit-CC: Ciro Santilli <ciro.santi...@arm.com>
Gerrit-MessageType: merged
_______________________________________________
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Reply via email to