Treat signed int bit-fields as signed

Prior to this PR, `int`, `signed int`, and `unsigned int`
bitfields are all treated as `unsigned int`.

With this PR, `signed int` will be treated as `signed int`,
and the others remain unsigned.

Since `Type` does not distinguish between `int` and `signed int`,
add an extra `int* SignenessSpecified` param to `ParseTypeSpec`
so we can tell these apart for bit-fields and treat plain `int : N`
as `unsigned int : N` since it is more efficient to zero-extend
than sign-extend.

Fixes #1095

src/cc65/codegen.c

@@ -4455,7 +4455,7 @@ void g_testbitfield (unsigned Flags, unsigned BitOffs, unsigned BitWidth)
 
 
 
-void g_extractbitfield (unsigned Flags, unsigned FullWidthFlags,
+void g_extractbitfield (unsigned Flags, unsigned FullWidthFlags, int IsSigned,
                         unsigned BitOffs, unsigned BitWidth)
 /* Extract bits from bit-field in ax. */
 {
@@ -4465,18 +4465,79 @@ void g_extractbitfield (unsigned Flags, unsigned FullWidthFlags,
     g_asr (Flags | CF_CONST, BitOffs);
 
     /* Since we have now shifted down, we could do char ops when the width fits in a char, but we
-    ** also need to clear the high byte since we've been using CF_FORCECHAR up to now.
+    ** also need to clear (or set) the high byte since we've been using CF_FORCECHAR up to now.
     */
+    unsigned Mask = (1U << BitWidth) - 1;
 
-    /* And by the width if the field doesn't end on a char or int boundary.  If it does end on
+    /* To zero-extend, we will and by the width if the field doesn't end on a char or
-    ** a boundary, then zeros have already been shifted in, but we need to clear the high byte
+    ** int boundary.  If it does end on a boundary, then zeros will have already been shifted in,
-    ** for char.  g_and emits no code if the mask is all ones.
+    ** but we need to clear the high byte for char.  g_and emits no code if the mask is all ones.
+    ** This is here so the signed and unsigned branches can use it.
     */
+    unsigned ZeroExtendMask = 0;  /* Zero if we don't need to zero-extend. */
     if (EndBit == CHAR_BITS) {
         /* We need to clear the high byte, since CF_FORCECHAR was set. */
-        g_and (FullWidthFlags | CF_CONST, 0xFF);
+        ZeroExtendMask = 0xFF;
     } else if (EndBit != INT_BITS) {
-        g_and (FullWidthFlags | CF_CONST, (0x0001U << BitWidth) - 1U);
+        ZeroExtendMask = (1U << BitWidth) - 1;
+    }
+
+    /* Handle signed bit-fields. */
+    if (IsSigned) {
+        /* Push A, since the sign bit test will destroy it. */
+        AddCodeLine ("pha");
+
+        /* Check sign bit */
+        unsigned SignBitPos = BitWidth - 1U;
+        unsigned SignBitByte = SignBitPos / CHAR_BITS;
+        unsigned SignBitPosInByte = SignBitPos % CHAR_BITS;
+        unsigned SignBitMask = 1U << SignBitPosInByte;
+
+        /* Move the correct byte to A.  This can only be X for now,
+        ** but more cases will be needed to support long.
+        */
+        switch (SignBitByte) {
+          case 0:
+            break;
+          case 1:
+            AddCodeLine ("txa");
+            break;
+          default:
+            FAIL ("Invalid Byte for sign bit");
+        }
+
+        /* Test the sign bit */
+        AddCodeLine ("and #$%02X", SignBitMask);
+        unsigned ZeroExtendLabel = GetLocalLabel ();
+        AddCodeLine ("beq %s", LocalLabelName (ZeroExtendLabel));
+
+        /* Pop A back and sign extend if required; operating on the full result need
+        ** to sign-extend into high byte, too.
+        */
+        AddCodeLine ("pla");
+        g_or (FullWidthFlags | CF_CONST, ~Mask);
+
+        /* Apparently, there is no unconditional branch BRA, so use JMP. */
+        unsigned DoneLabel = GetLocalLabel ();
+        AddCodeLine ("jmp %s", LocalLabelName (DoneLabel));
+
+        /* Pop A back, then zero-extend; we need to duplicate the PLA rather than move it before
+        ** the branch to share with the other label because PLA sets the condition codes.
+        */
+        g_defcodelabel (ZeroExtendLabel);
+        AddCodeLine ("pla");
+
+        /* Zero the upper bits, the same as the unsigned path. */
+        if (ZeroExtendMask != 0) {
+            g_and (FullWidthFlags | CF_CONST, ZeroExtendMask);
+        }
+
+        g_defcodelabel (DoneLabel);
+    } else {
+        /* Unsigned bit-field, only needs zero-extension. */
+        if (ZeroExtendMask != 0) {
+            g_and (FullWidthFlags | CF_CONST, ZeroExtendMask);
+        }
     }
 }
 

src/cc65/codegen.h

@@ -478,7 +478,7 @@ void g_initstatic (unsigned InitLabel, unsigned VarLabel, unsigned Size);
 void g_testbitfield (unsigned Flags, unsigned BitOffs, unsigned BitWidth);
 /* Test bit-field in ax. */
 
-void g_extractbitfield (unsigned Flags, unsigned FullWidthFlags,
+void g_extractbitfield (unsigned Flags, unsigned FullWidthFlags, int IsSigned,
                         unsigned BitOffs, unsigned BitWidth);
 /* Extract bits from bit-field in ax. */
 

src/cc65/declare.c

@@ -41,6 +41,7 @@
 /* common */
 #include "addrsize.h"
 #include "mmodel.h"
+#include "shift.h"
 #include "xmalloc.h"
 
 /* cc65 */
@@ -87,7 +88,8 @@ struct StructInitData {
 
 
 
-static void ParseTypeSpec (DeclSpec* D, long Default, TypeCode Qualifiers);
+static void ParseTypeSpec (DeclSpec* D, long Default, TypeCode Qualifiers,
+                           int* SignednessSpecified);
 /* Parse a type specifier */
 
 static unsigned ParseInitInternal (Type* T, int* Braces, int AllowFlexibleMembers);
@@ -252,12 +254,15 @@ static void OptionalInt (void)
 
 
 
-static void OptionalSigned (void)
+static void OptionalSigned (int* SignednessSpecified)
 /* Eat an optional "signed" token */
 {
     if (CurTok.Tok == TOK_SIGNED) {
         /* Skip it */
         NextToken ();
+        if (SignednessSpecified != NULL) {
+            *SignednessSpecified = 1;
+        }
     }
 }
 
@@ -728,6 +733,9 @@ static int ParseFieldWidth (Declaration* Decl)
         return -1;
     }
 
+    /* TODO: This can be relaxed to be any integral type, but
+    ** ParseStructInit currently only supports up to int.
+    */
     if (SizeOf (Decl->Type) != SizeOf (type_uint)) {
         /* Only int sized types may be used for bit-fields for now */
         Error ("cc65 currently only supports unsigned int bit-fields");
@@ -774,7 +782,8 @@ static unsigned PadWithBitField (unsigned StructSize, unsigned BitOffs)
     /* Add an anonymous bit-field that aligns to the next
     ** byte.
     */
-    AddBitField (Ident, StructSize, BitOffs, PaddingBits);
+    AddBitField (Ident, type_uchar, StructSize, BitOffs, PaddingBits,
+                 /*SignednessSpecified=*/1);
 
     return PaddingBits;
 }
@@ -866,8 +875,9 @@ static SymEntry* ParseUnionDecl (const char* Name)
 
         /* Get the type of the entry */
         DeclSpec Spec;
+        int SignednessSpecified = 0;
         InitDeclSpec (&Spec);
-        ParseTypeSpec (&Spec, -1, T_QUAL_NONE);
+        ParseTypeSpec (&Spec, -1, T_QUAL_NONE, &SignednessSpecified);
 
         /* Read fields with this type */
         while (1) {
@@ -909,7 +919,11 @@ static SymEntry* ParseUnionDecl (const char* Name)
 
             /* Add a field entry to the table. */
             if (FieldWidth > 0) {
-                AddBitField (Decl.Ident, 0, 0, FieldWidth);
+                /* For a union, allocate space for the type specified by the
+                ** bit-field.
+                */
+                AddBitField (Decl.Ident, Decl.Type, 0, 0, FieldWidth,
+                             SignednessSpecified);
             } else {
                 if (IsAnonName (Decl.Ident)) {
                     Entry = AddLocalSym (Decl.Ident, Decl.Type, SC_STRUCTFIELD, 0);
@@ -997,8 +1011,9 @@ static SymEntry* ParseStructDecl (const char* Name)
             continue;
         }
 
+        int SignednessSpecified = 0;
         InitDeclSpec (&Spec);
-        ParseTypeSpec (&Spec, -1, T_QUAL_NONE);
+        ParseTypeSpec (&Spec, -1, T_QUAL_NONE, &SignednessSpecified);
 
         /* Read fields with this type */
         while (1) {
@@ -1020,12 +1035,13 @@ static SymEntry* ParseStructDecl (const char* Name)
             FieldWidth = ParseFieldWidth (&Decl);
 
             /* If this is not a bit field, or the bit field is too large for
-            ** the remainder of the current member, or we have a bit field
+            ** the remainder of the allocated unit, or we have a bit field
             ** with width zero, align the struct to the next member by adding
             ** a member with an anonymous name.
             */
             if (BitOffs > 0) {
-                if (FieldWidth <= 0 || (BitOffs + FieldWidth) > INT_BITS) {
+                if (FieldWidth <= 0 ||
+                    (BitOffs + FieldWidth) > CHAR_BITS * SizeOf (Decl.Type)) {
                     /* Add an anonymous bit-field that aligns to the next
                     ** byte.
                     */
@@ -1087,9 +1103,10 @@ static SymEntry* ParseStructDecl (const char* Name)
                 ** bit-field as a char type in expressions.
                 */
                 CHECK (BitOffs < CHAR_BITS);
-                AddBitField (Decl.Ident, StructSize, BitOffs, FieldWidth);
+                AddBitField (Decl.Ident, Decl.Type, StructSize, BitOffs,
+                             FieldWidth, SignednessSpecified);
                 BitOffs += FieldWidth;
-                CHECK (BitOffs <= INT_BITS);
+                CHECK (BitOffs <= CHAR_BITS * SizeOf (Decl.Type));
                 /* Add any full bytes to the struct size. */
                 StructSize += BitOffs / CHAR_BITS;
                 BitOffs %= CHAR_BITS;
@@ -1145,12 +1162,20 @@ NextMember: if (CurTok.Tok != TOK_COMMA) {
 
 
 
-static void ParseTypeSpec (DeclSpec* D, long Default, TypeCode Qualifiers)
+static void ParseTypeSpec (DeclSpec* D, long Default, TypeCode Qualifiers,
-/* Parse a type specifier */
+                           int* SignednessSpecified)
+/* Parse a type specifier.  Store whether one of "signed" or "unsigned" was
+** specified, so bit-fields of unspecified signedness can be treated as
+** unsigned; without special handling, it would be treated as signed.
+*/
 {
     ident       Ident;
     SymEntry*   Entry;
 
+    if (SignednessSpecified != NULL) {
+        *SignednessSpecified = 0;
+    }
+
     /* Assume we have an explicit type */
     D->Flags &= ~DS_DEF_TYPE;
 
@@ -1176,12 +1201,15 @@ static void ParseTypeSpec (DeclSpec* D, long Default, TypeCode Qualifiers)
         case TOK_LONG:
             NextToken ();
             if (CurTok.Tok == TOK_UNSIGNED) {
+                if (SignednessSpecified != NULL) {
+                    *SignednessSpecified = 1;
+                }
                 NextToken ();
                 OptionalInt ();
                 D->Type[0].C = T_ULONG;
                 D->Type[1].C = T_END;
             } else {
-                OptionalSigned ();
+                OptionalSigned (SignednessSpecified);
                 OptionalInt ();
                 D->Type[0].C = T_LONG;
                 D->Type[1].C = T_END;
@@ -1191,12 +1219,15 @@ static void ParseTypeSpec (DeclSpec* D, long Default, TypeCode Qualifiers)
         case TOK_SHORT:
             NextToken ();
             if (CurTok.Tok == TOK_UNSIGNED) {
+                if (SignednessSpecified != NULL) {
+                    *SignednessSpecified = 1;
+                }
                 NextToken ();
                 OptionalInt ();
                 D->Type[0].C = T_USHORT;
                 D->Type[1].C = T_END;
             } else {
-                OptionalSigned ();
+                OptionalSigned (SignednessSpecified);
                 OptionalInt ();
                 D->Type[0].C = T_SHORT;
                 D->Type[1].C = T_END;
@@ -1210,6 +1241,9 @@ static void ParseTypeSpec (DeclSpec* D, long Default, TypeCode Qualifiers)
             break;
 
        case TOK_SIGNED:
+            if (SignednessSpecified != NULL) {
+                *SignednessSpecified = 1;
+            }
             NextToken ();
             switch (CurTok.Tok) {
 
@@ -1245,6 +1279,9 @@ static void ParseTypeSpec (DeclSpec* D, long Default, TypeCode Qualifiers)
             break;
 
         case TOK_UNSIGNED:
+            if (SignednessSpecified != NULL) {
+                *SignednessSpecified = 1;
+            }
             NextToken ();
             switch (CurTok.Tok) {
 
@@ -1835,7 +1872,7 @@ Type* ParseType (Type* T)
 
     /* Get a type without a default */
     InitDeclSpec (&Spec);
-    ParseTypeSpec (&Spec, -1, T_QUAL_NONE);
+    ParseTypeSpec (&Spec, -1, T_QUAL_NONE, NULL);
 
     /* Parse additional declarators */
     ParseDecl (&Spec, &Decl, DM_NO_IDENT);
@@ -1967,7 +2004,7 @@ void ParseDeclSpec (DeclSpec* D, unsigned DefStorage, long DefType)
     ParseStorageClass (D, DefStorage);
 
     /* Parse the type specifiers passing any initial type qualifiers */
-    ParseTypeSpec (D, DefType, Qualifiers);
+    ParseTypeSpec (D, DefType, Qualifiers, NULL);
 }
 
 
@@ -2362,6 +2399,7 @@ static unsigned ParseStructInit (Type* T, int* Braces, int AllowFlexibleMembers)
             ** into 3 bytes.
             */
             SI.ValBits += Entry->V.B.BitWidth;
+            /* TODO: Generalize this so any type can be used. */
             CHECK (SI.ValBits <= CHAR_BITS + INT_BITS - 2);
             while (SI.ValBits >= CHAR_BITS) {
                 OutputBitFieldData (&SI);
@@ -2393,16 +2431,34 @@ static unsigned ParseStructInit (Type* T, int* Braces, int AllowFlexibleMembers)
                    SI.Offs         * CHAR_BITS + SI.ValBits);
 
             /* Read the data, check for a constant integer, do a range check */
-            ParseScalarInitInternal (type_uint, &ED);
+            ParseScalarInitInternal (Entry->Type, &ED);
             if (!ED_IsConstAbsInt (&ED)) {
                 Error ("Constant initializer expected");
                 ED_MakeConstAbsInt (&ED, 1);
             }
-            if (ED.IVal > (long) Mask) {
+
-                Warning ("Truncating value in bit-field initializer");
+            /* Truncate the initializer value to the width of the bit-field and check if we lost
-                ED.IVal &= (long) Mask;
+            ** any useful bits.
+            */
+            Val = (unsigned) ED.IVal & Mask;
+            if (IsSignUnsigned (Entry->Type)) {
+                if (ED.IVal < 0 || (unsigned long) ED.IVal != Val) {
+                    Warning ("Implicit truncation from '%s' to '%s : %u' in bit-field initializer"
+                             " changes value from %ld to %u",
+                             GetFullTypeName (ED.Type), GetFullTypeName (Entry->Type),
+                             Entry->V.B.BitWidth, ED.IVal, Val);
+                }
+            } else {
+                /* Sign extend back to full width of host long. */
+                unsigned ShiftBits = sizeof (long) * CHAR_BIT - Entry->V.B.BitWidth;
+                long RestoredVal = asr_l(asl_l (Val, ShiftBits), ShiftBits);
+                if (ED.IVal != RestoredVal) {
+                    Warning ("Implicit truncation from '%s' to '%s : %u' in bit-field initializer "
+                             "changes value from %ld to %d",
+                             GetFullTypeName (ED.Type), GetFullTypeName (Entry->Type),
+                             Entry->V.B.BitWidth, ED.IVal, Val);
+                }
             }
-            Val = (unsigned) ED.IVal;
 
             /* Add the value to the currently stored bit-field value */
             Shift = (Entry->V.B.Offs - SI.Offs) * CHAR_BITS + Entry->V.B.BitOffs;
@@ -2417,6 +2473,7 @@ static unsigned ParseStructInit (Type* T, int* Braces, int AllowFlexibleMembers)
             ** aligned, so will have padding before it.
             */
             CHECK (SI.ValBits <= CHAR_BIT * sizeof(SI.BitVal));
+            /* TODO: Generalize this so any type can be used. */
             CHECK (SI.ValBits <= CHAR_BITS + INT_BITS - 2);
             while (SI.ValBits >= CHAR_BITS) {
                 OutputBitFieldData (&SI);
@@ -2425,7 +2482,8 @@ static unsigned ParseStructInit (Type* T, int* Braces, int AllowFlexibleMembers)
         } else {
 
             /* Standard member. We should never have stuff from a
-            ** bit-field left
+            ** bit-field left because an anonymous member was added
+            ** for padding by ParseStructDecl.
             */
             CHECK (SI.ValBits == 0);
 

src/cc65/loadexpr.c

@@ -123,7 +123,9 @@ void LoadExpr (unsigned Flags, struct ExprDesc* Expr)
             ** supported.
             */
             Flags |= (EndBit <= CHAR_BITS) ? CF_CHAR : CF_INT;
-            Flags |= CF_UNSIGNED;
+            if (IsSignUnsigned (Expr->Type)) {
+                Flags |= CF_UNSIGNED;
+            }
 
             /* Flags we need operate on the whole bit-field, without CF_FORCECHAR.  */
             BitFieldFullWidthFlags = Flags;
@@ -133,7 +135,11 @@ void LoadExpr (unsigned Flags, struct ExprDesc* Expr)
             ** type is not CF_CHAR.
             */
             if (AdjustBitField) {
-                Flags |= CF_FORCECHAR;
+                /* If adjusting, then we're sign extending manually, so do everything unsigned
+                ** to make shifts faster.
+                */
+                Flags |= CF_UNSIGNED | CF_FORCECHAR;
+                BitFieldFullWidthFlags |= CF_UNSIGNED;
             }
         } else if ((Flags & CF_TYPEMASK) == 0) {
             Flags |= TypeOf (Expr->Type);
@@ -231,7 +237,8 @@ void LoadExpr (unsigned Flags, struct ExprDesc* Expr)
             if (ED_NeedsTest (Expr)) {
                 g_testbitfield (Flags, Expr->BitOffs, Expr->BitWidth);
             } else {
-                g_extractbitfield (Flags, BitFieldFullWidthFlags, Expr->BitOffs, Expr->BitWidth);
+                g_extractbitfield (Flags, BitFieldFullWidthFlags, IsSignSigned (Expr->Type),
+                                   Expr->BitOffs, Expr->BitWidth);
             }
         }
 

src/cc65/symtab.c

@@ -819,7 +819,8 @@ SymEntry* AddStructSym (const char* Name, unsigned Flags, unsigned Size, SymTabl
 
 
 
-SymEntry* AddBitField (const char* Name, unsigned Offs, unsigned BitOffs, unsigned BitWidth)
+SymEntry* AddBitField (const char* Name, const Type* T, unsigned Offs,
+                       unsigned BitOffs, unsigned BitWidth, int SignednessSpecified)
 /* Add a bit field to the local symbol table and return the symbol entry */
 {
     /* Do we have an entry with this name already? */
@@ -834,12 +835,21 @@ SymEntry* AddBitField (const char* Name, unsigned Offs, unsigned BitOffs, unsign
         /* Create a new entry */
         Entry = NewSymEntry (Name, SC_BITFIELD);
 
-        /* Set the symbol attributes. Bit-fields are always of type unsigned */
+        /* Set the symbol attributes. Bit-fields are always integral types. */
-        Entry->Type         = type_uint;
+        Entry->Type         = TypeDup (T);
         Entry->V.B.Offs     = Offs;
         Entry->V.B.BitOffs  = BitOffs;
         Entry->V.B.BitWidth = BitWidth;
 
+        if (!SignednessSpecified) {
+            /* int is treated as signed int everywhere except bit-fields; switch it to unsigned,
+            ** since this is allowed for bit-fields and avoids sign-extension, so is much faster.
+            */
+            CHECK ((Entry->Type->C & T_MASK_SIGN) == T_SIGN_SIGNED);
+            Entry->Type->C &= ~T_MASK_SIGN;
+            Entry->Type->C |= T_SIGN_UNSIGNED;
+        }
+
         /* Add the entry to the symbol table */
         AddSymEntry (SymTab, Entry);
 

src/cc65/symtab.h

@@ -155,7 +155,8 @@ SymEntry* AddEnumSym (const char* Name, unsigned Flags, const Type* Type, SymTab
 SymEntry* AddStructSym (const char* Name, unsigned Flags, unsigned Size, SymTable* Tab);
 /* Add a struct/union entry and return it */
 
-SymEntry* AddBitField (const char* Name, unsigned Offs, unsigned BitOffs, unsigned BitWidth);
+SymEntry* AddBitField (const char* Name, const Type* Type, unsigned Offs,
+                       unsigned BitOffs, unsigned BitWidth, int SignednessSpecified);
 /* Add a bit field to the local symbol table and return the symbol entry */
 
 SymEntry* AddConstSym (const char* Name, const Type* T, unsigned Flags, long Val);

test/val/bug1095.c

@@ -31,7 +31,10 @@ static struct signed_ints {
     signed int b : 3;
     signed int c : 3;
     signed int d : 10;
-} si = {-4, -1, 3, -500};
+    signed int : 0;
+    signed int e : 8;
+    signed int f : 16;
+} si = {-4, -1, 3, -500, -100, -5000};
 
 static void test_signed_bitfield(void)
 {
@@ -53,7 +56,7 @@ static void test_signed_bitfield(void)
         failures++;
     }
 
-    if (si.b <= 0) {
+    if (si.c <= 0) {
         printf("Got si.c = %d, expected positive.\n", si.c);
         failures++;
     }
@@ -71,10 +74,30 @@ static void test_signed_bitfield(void)
         failures++;
     }
 
+    if (si.e >= 0) {
+        printf("Got si.e = %d, expected negative.\n", si.e);
+        failures++;
+    }
+    if (si.e != -100) {
+        printf("Got si.e = %d, expected -100.\n", si.e);
+        failures++;
+    }
+
+    if (si.f >= 0) {
+        printf("Got si.f = %d, expected negative.\n", si.f);
+        failures++;
+    }
+    if (si.f != -5000) {
+        printf("Got si.f = %d, expected -5000.\n", si.f);
+        failures++;
+    }
+
     si.a = -3;
     si.b = 1;
     si.c = -2;
     si.d = 500;
+    si.e = 100;
+    si.f = 5000;
 
     if (si.a >= 0) {
         printf("Got si.a = %d, expected negative.\n", si.a);
@@ -94,7 +117,7 @@ static void test_signed_bitfield(void)
         failures++;
     }
 
-    if (si.b >= 0) {
+    if (si.c >= 0) {
         printf("Got si.c = %d, expected negative.\n", si.c);
         failures++;
     }
@@ -111,6 +134,24 @@ static void test_signed_bitfield(void)
         printf("Got si.d = %d, expected 500.\n", si.d);
         failures++;
     }
+
+    if (si.e <= 0) {
+        printf("Got si.e = %d, expected positive.\n", si.e);
+        failures++;
+    }
+    if (si.e != 100) {
+        printf("Got si.e = %d, expected 100.\n", si.e);
+        failures++;
+    }
+
+    if (si.f <= 0) {
+        printf("Got si.f = %d, expected positive.\n", si.f);
+        failures++;
+    }
+    if (si.f != 5000) {
+        printf("Got si.f = %d, expected 5000.\n", si.f);
+        failures++;
+    }
 }
 
 int main(void)