Handle bit-field test after shift/mask

Previously, bit-field tests were incorrectly combined with load in
`if (x.bitfield)`.  Delay the test until after the shift/mask
is done.  Still combine tests with load if no shift/mask is required.

Fixes #1139

src/cc65/loadexpr.c

@@ -33,6 +33,8 @@
 
 
 
+#include <limits.h>
+
 /* cc65 */
 #include "codegen.h"
 #include "error.h"
@@ -113,13 +115,23 @@ void LoadExpr (unsigned Flags, struct ExprDesc* Expr)
         ** field is completely contained in the lower byte, we will throw away
         ** the high byte anyway and may therefore load just the low byte.
         */
+        unsigned EndBit = 0;  /* End bit for bit-fields, or zero if non-bit-field. */
+        int AdjustBitField = 0;
         if (ED_IsBitField (Expr)) {
-            Flags |= (Expr->BitOffs + Expr->BitWidth <= CHAR_BITS) ? CF_CHAR : CF_INT;
+            EndBit = Expr->BitOffs + Expr->BitWidth;
+            AdjustBitField = Expr->BitOffs != 0 || (EndBit != CHAR_BITS && EndBit != INT_BITS);
+
+            Flags |= (EndBit <= CHAR_BITS) ? CF_CHAR : CF_INT;
             Flags |= CF_UNSIGNED;
         } else if ((Flags & CF_TYPEMASK) == 0) {
             Flags |= TypeOf (Expr->Type);
         }
-        if (ED_NeedsTest (Expr)) {
+
+        /* Setting CF_TEST will cause the load to perform optimizations and not actually load all
+        ** bits of the bit-field, instead just computing the condition codes.  Therefore, if
+        ** adjustment is required, we do not set CF_TEST here, but handle it below.
+        */
+        if (ED_NeedsTest (Expr) && !AdjustBitField) {
             Flags |= CF_TEST;
         }
 
@@ -188,18 +200,41 @@ void LoadExpr (unsigned Flags, struct ExprDesc* Expr)
                 Internal ("Invalid location in LoadExpr: 0x%04X", ED_GetLoc (Expr));
         }
 
-        /* Handle bit fields. The actual type may have been casted or
+        /* Handle bit fields if necessary. The actual type may have been casted or converted,
-        ** converted, so be sure to always use unsigned ints for the
+        ** so be sure to always use unsigned ints for the operations.
-        ** operations.
         */
-        if (ED_IsBitField (Expr)) {
+        if (AdjustBitField) {
             /* If the field was loaded as a char, force the shift/mask ops to be char ops.
             ** If it is a char, the load has already put 0 in the upper byte, so that can remain.
             ** CF_FORCECHAR does nothing if the type is not CF_CHAR.
             */
             unsigned F = Flags | CF_FORCECHAR | CF_CONST;
 
-            /* Shift right by the bit offset */
+            /* We always need to do something with the low byte, so there is no opportunity
+            ** for optimization by skipping it.
+            */
+            CHECK (Expr->BitOffs < CHAR_BITS);
+
+            if (ED_NeedsTest (Expr)) {
+                /* If we need to do a test, then we avoid shifting (ASR only shifts one bit
+                ** at a time, so is slow) and just AND with the appropriate mask, then test
+                ** the result of that.
+                */
+
+                /* Avoid overly large shift. */
+                if (EndBit == sizeof (unsigned long) * CHAR_BIT) {
+                    g_and (F, (~0UL << Expr->BitOffs));
+                } else {
+                    g_and (F, ((1UL << EndBit) - 1) & (~0UL << Expr->BitOffs));
+                }
+
+                /* TODO: When long bit-fields are supported, an optimization to test only 3 bytes
+                ** when EndBit <= 24 is possible.
+                */
+                g_test (F);
+                ED_TestDone (Expr);
+            } else {
+                /* Shift right by the bit offset; no code is emitted if BitOffs is zero */
                 g_asr (F, Expr->BitOffs);
 
                 /* Since we have now shifted down, we can do char ops as long as the width fits in
@@ -209,15 +244,15 @@ void LoadExpr (unsigned Flags, struct ExprDesc* Expr)
                     F |= CF_CHAR;
                 }
 
-            /* And by the width if the field doesn't end on a char or int boundary.  If it does
+                /* And by the width if the field doesn't end on a char or int boundary.
-            ** end on a boundary, then zeros have already been shifted in.  g_and emits no code
+                ** If it does end on a boundary, then zeros have already been shifted in.
-            ** if the mask is all ones.
+                ** g_and emits no code if the mask is all ones.
                 */
-            if (Expr->BitOffs + Expr->BitWidth != CHAR_BITS &&
+                if (EndBit != CHAR_BITS && EndBit != INT_BITS) {
-                Expr->BitOffs + Expr->BitWidth != INT_BITS) {
                     g_and (F, (0x0001U << Expr->BitWidth) - 1U);
                 }
             }
+        }
 
         /* Expression was tested */
         ED_TestDone (Expr);

test/todo/bug1139.c

@@ -1,52 +0,0 @@
-/*
-  Copyright 2020 The cc65 Authors
-
-  This software is provided 'as-is', without any express or implied
-  warranty. In no event will the authors be held liable for any damages
-  arising from the use of this software.
-
-  Permission is granted to anyone to use this software for any purpose,
-  including commercial applications, and to alter it and redistribute it
-  freely, subject to the following restrictions:
-
-  1. The origin of this software must not be misrepresented; you must not
-     claim that you wrote the original software. If you use this software
-     in a product, an acknowledgment in the product documentation would be
-     appreciated but is not required.
-  2. Altered source versions must be plainly marked as such, and must not be
-     misrepresented as being the original software.
-  3. This notice may not be removed or altered from any source distribution.
-*/
-
-/*
-  Tests bit-field in if condition; see https://github.com/cc65/cc65/issues/1139
-*/
-
-#include <stdio.h>
-
-static unsigned char failures = 0;
-
-static struct overlap {
-    unsigned int x : 10;
-    unsigned int y : 10;
-} o = {11, 22};
-
-/* Test using bit-fields in if conditions. */
-static void test_if(void)
-{
-    o.x = 0;
-    o.y = 44;
-    if (o.x) {
-        printf("Bad, o.x is false\n");
-        failures++;
-    } else {
-        printf("Good\n");
-    }
-}
-
-int main(void)
-{
-    test_if();
-    printf("failures: %u\n", failures);
-    return failures;
-}

test/val/bug1139.c

@@ -0,0 +1,353 @@
+/*
+  Copyright 2020 The cc65 Authors
+
+  This software is provided 'as-is', without any express or implied
+  warranty. In no event will the authors be held liable for any damages
+  arising from the use of this software.
+
+  Permission is granted to anyone to use this software for any purpose,
+  including commercial applications, and to alter it and redistribute it
+  freely, subject to the following restrictions:
+
+  1. The origin of this software must not be misrepresented; you must not
+     claim that you wrote the original software. If you use this software
+     in a product, an acknowledgment in the product documentation would be
+     appreciated but is not required.
+  2. Altered source versions must be plainly marked as such, and must not be
+     misrepresented as being the original software.
+  3. This notice may not be removed or altered from any source distribution.
+*/
+
+/*
+  Tests bit-field in if condition; see https://github.com/cc65/cc65/issues/1139
+*/
+
+#include <stdio.h>
+
+static unsigned char failures = 0;
+
+static struct four_bits {
+    unsigned int x : 4;
+} fb = {1};
+
+static struct overlap {
+    unsigned int x : 10;
+    unsigned int y : 10;
+} o = {11, 22};
+
+static struct full_width {
+    unsigned int x : 8;
+    unsigned int y : 16;
+} fw = {255, 17};
+
+static struct aligned_end {
+    unsigned int : 2;
+    unsigned int x : 6;
+    unsigned int : 3;
+    unsigned int y : 13;
+    /* z crosses a byte boundary, but fits in a byte when shifted. */
+    unsigned int : 6;
+    unsigned int z : 7;
+} ae = {63, 17, 100};
+
+/* Test using bit-fields in if conditions. */
+static void test_if(void)
+{
+    /* Original test case for the bug. */
+    o.x = 0;
+    o.y = 44;
+    if (o.x) {
+        printf("Bad, o.x is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    /* Additionally, test most fields from bitfield.c to try to cover all start/end situations. */
+    /* four_bits */
+    fb.x = 1;
+    if (fb.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, fb.x is true (1)\n");
+        failures++;
+    }
+
+    fb.x = 0;
+    if (fb.x) {
+        printf("Bad, fb.x is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    /* overlap */
+    o.x = 123;
+    if (o.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, o.x is true (123)\n");
+        failures++;
+    }
+
+    o.x = 0;
+    if (o.x) {
+        printf("Bad, o.x is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    o.y = 321;
+    if (o.y) {
+        printf("Good\n");
+    } else {
+        printf("Bad, o.y is true (321)\n");
+        failures++;
+    }
+
+    o.y = 0;
+    if (o.y) {
+        printf("Bad, o.y is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    /* full_width */
+    fw.x = 117;
+    if (fw.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, fw.x is true (117)\n");
+        failures++;
+    }
+
+    fw.x = 0;
+    if (fw.x) {
+        printf("Bad, fw.x is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    fw.y = 32123;
+    if (fw.y) {
+        printf("Good\n");
+    } else {
+        printf("Bad, fw.y is true (32123)\n");
+        failures++;
+    }
+
+    fw.y = 0;
+    if (fw.y) {
+        printf("Bad, fw.y is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    /* aligned_end */
+    ae.x = 2;
+    if (ae.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, ae.x is true (2)\n");
+        failures++;
+    }
+
+    ae.x = 0;
+    if (ae.x) {
+        printf("Bad, ae.x is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    ae.y = 2222;
+    if (ae.y) {
+        printf("Good\n");
+    } else {
+        printf("Bad, ae.y is true (2222)\n");
+        failures++;
+    }
+
+    ae.y = 0;
+    if (ae.y) {
+        printf("Bad, ae.y is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    ae.z = 111;
+    if (ae.z) {
+        printf("Good\n");
+    } else {
+        printf("Bad, ae.z is true (111)\n");
+        failures++;
+    }
+
+    ae.z = 0;
+    if (ae.z) {
+        printf("Bad, ae.z is false\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+}
+
+/* Test using bit-fields in inverted if conditions. */
+static void test_if_not(void)
+{
+    /* Original test case for the bug, inverted. */
+    o.x = 0;
+    o.y = 44;
+    if (!o.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, o.x is false\n");
+        failures++;
+    }
+
+    /* Additionally, test most fields from bitfield.c to try to cover all start/end situations. */
+    /* four_bits */
+    fb.x = 1;
+    if (!fb.x) {
+        printf("Bad, fb.x is true (1)\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    fb.x = 0;
+    if (!fb.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, fb.x is false\n");
+        failures++;
+    }
+
+    /* overlap */
+    o.x = 123;
+    if (!o.x) {
+        printf("Bad, o.x is true (123)\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    o.x = 0;
+    if (!o.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, o.x is false\n");
+        failures++;
+    }
+
+    o.y = 321;
+    if (!o.y) {
+        printf("Bad, o.y is true (321)\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    o.y = 0;
+    if (!o.y) {
+        printf("Good\n");
+    } else {
+        printf("Bad, o.y is false\n");
+        failures++;
+    }
+
+    /* full_width */
+    fw.x = 117;
+    if (!fw.x) {
+        printf("Bad, fw.x is true (117)\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    fw.x = 0;
+    if (!fw.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, fw.x is false\n");
+        failures++;
+    }
+
+    fw.y = 32123;
+    if (!fw.y) {
+        printf("Bad, fw.y is true (32123)\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    fw.y = 0;
+    if (!fw.y) {
+        printf("Good\n");
+    } else {
+        printf("Bad, fw.y is false\n");
+        failures++;
+    }
+
+    /* aligned_end */
+    ae.x = 2;
+    if (!ae.x) {
+        printf("Bad, ae.x is true (2)\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    ae.x = 0;
+    if (!ae.x) {
+        printf("Good\n");
+    } else {
+        printf("Bad, ae.x is false\n");
+        failures++;
+    }
+
+    ae.y = 2222;
+    if (!ae.y) {
+        printf("Bad, ae.y is true (2222)\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    ae.y = 0;
+    if (!ae.y) {
+        printf("Good\n");
+    } else {
+        printf("Bad, ae.y is false\n");
+        failures++;
+    }
+
+    ae.z = 111;
+    if (!ae.z) {
+        printf("Bad, ae.z is true (111)\n");
+        failures++;
+    } else {
+        printf("Good\n");
+    }
+
+    ae.z = 0;
+    if (!ae.z) {
+        printf("Good\n");
+    } else {
+        printf("Bad, ae.z is false\n");
+        failures++;
+    }
+}
+
+int main(void)
+{
+    test_if();
+    test_if_not();
+    printf("failures: %u\n", failures);
+    return failures;
+}