From: Giacomo Travaglini <giacomo.travaglini@arm.com>
Date: Fri, 20 Mar 2020 17:29:52 +0000 (+0000)
Subject: arch-arm: Fix aapcs32/aapcs64 compilation issues
X-Git-Tag: v20.0.0.0~273
X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=2fbd03599d2c0a7e8ce149cba173dfdeea1d9975;p=gem5.git

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.travaglini@arm.com>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/26990
Reviewed-by: Gabe Black <gabeblack@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
---

diff --git a/src/arch/arm/aapcs32.hh b/src/arch/arm/aapcs32.hh
index bfeb55374..fd63483ef 100644
--- a/src/arch/arm/aapcs32.hh
+++ b/src/arch/arm/aapcs32.hh
@@ -131,39 +131,78 @@ struct Aapcs32ArgumentBase
 
 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 (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 ((sizeof(Integer) <= sizeof(uint32_t) ||
-                std::is_same<Integer, Addr>::value) &&
+        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 9d7623a05..203846d43 100644
--- a/src/arch/arm/aapcs64.hh
+++ b/src/arch/arm/aapcs64.hh
@@ -229,14 +229,30 @@ struct Result<Aapcs64, Float, typename std::enable_if<
 // 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 @@ struct Argument<Aapcs64, Integer, typename std::enable_if<
 
 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));
     }
 };