v1.3.2: ci run encoding tests on big-endian and fix c sign extension for big-endian signed ints

.github/workflows/ci.yml

@@ -82,21 +82,26 @@ jobs:
       with:
         platforms: s390x
 
-    - name: Run endian tests on s390x
+    - name: Run encoding tests on s390x
       run: |
         docker run --rm --platform linux/s390x \
           -v "${{ github.workspace }}:/work" \
           -w /work \
-          python:3.13-bookworm \
+          golang:1.25-bookworm \
           bash -euxo pipefail -c '
             apt-get update
-            apt-get install -y --no-install-recommends gcc make
+            apt-get install -y --no-install-recommends gcc make python3 python3-venv
+            python3 -m venv /tmp/bitproto-venv
+            . /tmp/bitproto-venv/bin/activate
             python -m pip install -r compiler/requirements.txt -r tests/requirements_tests.txt
             printf "%s\n" \
               "#!/usr/bin/env sh" \
-              "PYTHONPATH=/work/compiler exec python -m bitproto._main \"\$@\"" \
+              "PYTHONPATH=/work/compiler exec /tmp/bitproto-venv/bin/python -m bitproto._main \"\$@\"" \
               >/usr/local/bin/bitproto
             chmod +x /usr/local/bin/bitproto
+            export GOFLAGS=-buildvcs=false
             PYTHONPATH=/work/compiler:/work/lib/py python -c "import sys; assert sys.byteorder == \"big\", sys.byteorder"
-            PYTHONPATH=/work/compiler:/work/lib/py python -m pytest tests/test_encoding/test_endian.py -v
+            PYTHONPATH=/work/compiler:/work/lib/py BP_TEST_LANGS=c pytest tests/test_encoding -v -s -x
+            PYTHONPATH=/work/compiler:/work/lib/py BP_TEST_LANGS=c BP_TEST_CC_OPTIMIZATION=-O2 pytest tests/test_encoding -v -s -x
+            PYTHONPATH=/work/compiler:/work/lib/py BP_TEST_LANGS=c BP_TEST_OPTIMIZATION_ARG=-O pytest tests/test_encoding -v -s -x
           '

changes.rst

@@ -1,5 +1,14 @@
 .. currentmodule:: bitproto
 
+Version 1.3.2
+-------------
+
+.. _version-1.3.2:
+
+- Bugfix: Fix sign extension for arbitrary-width signed integers such as
+  ``int62`` in the standard-mode C runtime. This avoids undefined shifts and
+  fixes decoding these fields on big-endian hosts.
+
 Version 1.3.1
 -------------
 

compiler/bitproto/__init__.py

@@ -8,5 +8,5 @@
 
 """
 
-__version__ = "1.3.1"
+__version__ = "1.3.2"
 __description__ = "bit level data interchange format."

compiler/pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "bitproto"
-version = "1.3.1"
+version = "1.3.2"
 description = "bit level data interchange format."
 readme = "README.md"
 authors = [

lib/c/bitproto.c

@@ -13,12 +13,11 @@
 // Clang. __big_endian__ covers legacy TI ARM CGT (armcl). __BIG_ENDIAN__
 // covers some other toolchains.
 // __LITTLE_ENDIAN__ == 0 covers IAR (which always defines it).
-#if !defined(BP_BIG_ENDIAN) && (                                             \
-    (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)) || \
-    defined(__ARM_BIG_ENDIAN) ||                                             \
-    defined(__big_endian__) ||                                               \
-    defined(__BIG_ENDIAN__) ||                                               \
-    (defined(__LITTLE_ENDIAN__) && (__LITTLE_ENDIAN__ == 0)))
+#if !defined(BP_BIG_ENDIAN) &&                                                \
+    ((defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)) || \
+     defined(__ARM_BIG_ENDIAN) || defined(__big_endian__) ||                  \
+     defined(__BIG_ENDIAN__) ||                                               \
+     (defined(__LITTLE_ENDIAN__) && (__LITTLE_ENDIAN__ == 0)))
 #define BP_BIG_ENDIAN 1
 #endif
 
@@ -268,6 +267,7 @@ void BpCopyBufferBits(int n, unsigned char *dst, unsigned char *src, int di,
 
             // Number of bits to process during batch copy.
             int bits = n + si;
+            bool copied = false;
 
 #ifndef BP_BIG_ENDIAN
             // These fast paths load multiple bytes through a native-endian
@@ -279,28 +279,33 @@ void BpCopyBufferBits(int n, unsigned char *dst, unsigned char *src, int di,
                 // This way, performance faster x2 than bits copy approach.
                 ((uint32_t *)dst)[0] = ((uint32_t *)(src))[0] >> si;
                 c = 32 - si;
+                copied = true;
             } else if (bits >= 16) {
                 // Copy as an uint16 integer.
                 ((uint16_t *)dst)[0] = ((uint16_t *)(src))[0] >> si;
                 c = 16 - si;
-            } else
+                copied = true;
+            }
 #endif
-            if (bits >= 8) {
-                // Copy as an unsigned char.
-                dst[0] = (src[0] >> si) & 0xff;
-                c = 8 - si;
-            } else {
-                // When bits < 8 and di == 0
-                // Copy partial bits inside a byte.
-                // For the original statement:
-                // c = BpMinTriple(8 - di, 8 - si, n);
-                // since di is 0 and bits <8, then 8-di is 8
-                // and n <8 , the 8-di won't be the smallest, we
-                // just pick function BpMin over BpMinTriple for the little
-                // little performance improvement.
-                c = BpMin(8 - si, n);
-                // Also, when di is 0, special case of next case.
-                dst[0] |= ((src[0] >> si) & ~(0xff << c));
+
+            if (!copied) {
+                if (bits >= 8) {
+                    // Copy as an unsigned char.
+                    dst[0] = (src[0] >> si) & 0xff;
+                    c = 8 - si;
+                } else {
+                    // When bits < 8 and di == 0
+                    // Copy partial bits inside a byte.
+                    // For the original statement:
+                    // c = BpMinTriple(8 - di, 8 - si, n);
+                    // since di is 0 and bits <8, then 8-di is 8
+                    // and n <8 , the 8-di won't be the smallest, we
+                    // just pick function BpMin over BpMinTriple for the little
+                    // little performance improvement.
+                    c = BpMin(8 - si, n);
+                    // Also, when di is 0, special case of next case.
+                    dst[0] |= ((src[0] >> si) & ~(0xff << c));
+                }
             }
         } else {
             // When di != 0, we have to copy partial bits inside a
@@ -320,10 +325,10 @@ void BpCopyBufferBits(int n, unsigned char *dst, unsigned char *src, int di,
             // ~(0xff << di << c) Gives a mask to clear higher not-need bits
             // all to 0, e.g. 00001111 on di=4, c=4; Finally: dst |= src to
             // copy bits.
-            // The ch is the first byte at pointer src. If this byte is Zero,
-            // then there's no need to copy anything, just count c. The
-            // benchmark on stm32 seems performance is improved by 2us by adding
-            // an if statement that skip the Zero byte.
+            // The ch is the first byte at pointer src. If this byte is
+            // Zero, then there's no need to copy anything, just count c.
+            // The benchmark on stm32 seems performance is improved by 2us
+            // by adding an if statement that skip the Zero byte.
             unsigned char ch = src[0];
             if (ch) dst[0] |= ((ch >> si << di) & ~(0xff << di << c));
         }
@@ -337,13 +342,14 @@ void BpCopyBufferBits(int n, unsigned char *dst, unsigned char *src, int di,
 
 // BpEndecodeBaseType process given base type at given data.
 // This function guarantees to work geven a nbits > 64 is passed in (the
-// batch-array path used on little-endian only; on big-endian arrays are processed
-// element by element so nbits here is always a single base type's <= 64 bits).
+// batch-array path used on little-endian only; on big-endian arrays are
+// processed element by element so nbits here is always a single base type's
+// <= 64 bits).
 void BpEndecodeBaseType(int nbits, struct BpProcessorContext *ctx, void *data) {
 #ifdef BP_BIG_ENDIAN
     // Stage the field through a little-endian byte view so the wire stays
-    // little-endian regardless of host byte order. A single base integer is at
-    // most 64 bits, so 8 bytes of staging is always enough.
+    // little-endian regardless of host byte order. A single base integer is
+    // at most 64 bits, so 8 bytes of staging is always enough.
     int size = BpBaseTypeStorageSize(nbits);
     unsigned char le[8] = {0};
     unsigned char *p = (unsigned char *)data;
@@ -368,11 +374,11 @@ void BpEndecodeBaseType(int nbits, struct BpProcessorContext *ctx, void *data) {
 }
 
 // BpHandleIntSignAfterEndecode processes signed integer at given data after
-// this integer is endecode. The most left bit (Nth bit for int{N}) of a signed
-// integer indicates the sign. For example 00000101 is a negative integer for a
-// int3, but a positive integer for a int4. Where nbits is the number of bits
-// for this bitproto signed integer, for int{N}, its the N, the argument size is
-// the number of bytes in C language.
+// this integer is endecode. The most left bit (Nth bit for int{N}) of a
+// signed integer indicates the sign. For example 00000101 is a negative
+// integer for a int3, but a positive integer for a int4. Where nbits is the
+// number of bits for this bitproto signed integer, for int{N}, its the N,
+// the argument size is the number of bytes in C language.
 void BpHandleIntSignAfterEndecode(int size, int nbits,
                                   struct BpProcessorContext *ctx, void *data) {
     // Signed integer's sign bit processing is only about decoding.
@@ -387,34 +393,33 @@ void BpHandleIntSignAfterEndecode(int size, int nbits,
 
     switch (n) {
         // Suppose pointer data points to a int24 value V:
-        // 1. Check its sign: (V >> (24-1)) & 1
+        // 1. Check its sign: V & (1 << (24-1))
         // 2. If V is negative:
         //    Make a mask that keeps right 24bits be 0, left 8bits be 1:
-        //    mask = ~(1 << 24 - 1)
+        //    mask = ~((1 << 24) - 1)
         //    Recover the original integer is: V | mask.
         //
-        // Why not use V << 8 >> 8 solution here? This depends on arithmetic
-        // shifting. Which propagates the sign bit on the left vacant bits
-        // when doing a right shift. But C standard points that this
-        // behavior is implementation-defined.
+        // Use unsigned integers to avoid implementation-defined right
+        // shifts on negative signed values, and to avoid undefined behavior
+        // such as shifting a plain int by 32 or more bits for int62.
         case 8:  // int8_t
-            if (((*(int8_t *)data) >> (nbits - 1)) & 1) {
-                *(int8_t *)data |= ~((1 << nbits) - 1);
+            if ((*(uint8_t *)data) & ((uint8_t)1 << (nbits - 1))) {
+                *(uint8_t *)data |= (uint8_t)(~(((uint8_t)1 << nbits) - 1));
             }
             break;
         case 16:  // int16_t
-            if (((*(int16_t *)data) >> (nbits - 1)) & 1) {
-                *(int16_t *)data |= ~((1 << nbits) - 1);
+            if ((*(uint16_t *)data) & ((uint16_t)1 << (nbits - 1))) {
+                *(uint16_t *)data |= (uint16_t)(~(((uint16_t)1 << nbits) - 1));
             }
             break;
         case 32:  // int32_t
-            if (((*(int32_t *)data) >> (nbits - 1)) & 1) {
-                *(int32_t *)data |= ~((1 << nbits) - 1);
+            if ((*(uint32_t *)data) & ((uint32_t)1 << (nbits - 1))) {
+                *(uint32_t *)data |= (uint32_t)(~(((uint32_t)1 << nbits) - 1));
             }
             break;
         case 64:  // int64_t
-            if (((*(int64_t *)data) >> (nbits - 1)) & 1) {
-                *(int64_t *)data |= ~((1 << nbits) - 1);
+            if ((*(uint64_t *)data) & ((uint64_t)1 << (nbits - 1))) {
+                *(uint64_t *)data |= (uint64_t)(~(((uint64_t)1 << nbits) - 1));
             }
             break;
     }

tests/test_encoding/test_encoding.py

@@ -1,3 +1,4 @@
+import hashlib
 import json
 import os
 import subprocess
@@ -24,6 +25,7 @@ class _TestCase:
     # Whether to compare output of command run.
     compare_output: bool = True
     compare_output_as_json: bool = False
+    golden_sha256: str = ""
 
     cc_optimization_arg: str = ""
 
@@ -33,6 +35,10 @@ class _TestCase:
     def __post_init__(self) -> None:
         if not self.langs:
             self.langs = ["c", "go", "py"]
+        langs = os.environ.get("BP_TEST_LANGS", "")
+        if langs:
+            requested_langs = langs.split(",")
+            self.langs = [lang for lang in self.langs if lang in requested_langs]
         self.cc_optimization_arg = os.environ.get("BP_TEST_CC_OPTIMIZATION", "")
         self.optimization_mode_arg = os.environ.get("BP_TEST_OPTIMIZATION_ARG", "")
 
@@ -67,11 +73,21 @@ def compare_outputs(self, outputs: List[bytes]) -> None:
             else:
                 assert json.loads(out.strip()) == json.loads(outputs[0].strip())
 
+    def compare_golden_output(self, outputs: List[bytes]) -> None:
+        """Compares encoded output bytes against a golden digest."""
+        if not self.golden_sha256:
+            return
+        for out in outputs:
+            digest = hashlib.sha256(out.strip()).hexdigest()
+            assert digest == self.golden_sha256
+
     def run(self) -> None:
         """Run this case, returns the"""
 
         if self.optimization_mode_arg and not self.support_optimization_mode:
             return
+        if not self.langs:
+            return
 
         current_cwd = os.getcwd()
         outputs: List[bytes] = []
@@ -85,6 +101,7 @@ def run(self) -> None:
 
             if self.compare_output:
                 self.compare_outputs(outputs)
+            self.compare_golden_output(outputs)
 
         finally:
             self.execute_cmd_clean()
@@ -118,23 +135,36 @@ def test_encoding_nested() -> None:
 
 
 def test_encoding_arrays() -> None:
-    _TestCase("arrays").run()
+    _TestCase(
+        "arrays",
+        golden_sha256="97d5cf1251363d5de7af2c3586ae6581d12fc73e8608c87594e47c20a919aba5",
+    ).run()
 
 
 def test_encoding_scatter() -> None:
-    _TestCase("scatter").run()
+    _TestCase(
+        "scatter",
+        golden_sha256="ab428ea4324fb51fe2bf0e486b477e7f1880967520a8a07588755ed983f88496",
+    ).run()
 
 
 def test_encoding_enums() -> None:
     _TestCase("enums").run()
 
 
 def test_encoding_signed() -> None:
-    _TestCase("signed").run()
+    _TestCase(
+        "signed",
+        golden_sha256="420d13682d2bec113d32b4504f29e01023b28c6391d275afd0eb16dfd7662e6a",
+    ).run()
 
 
 def test_encoding_complexx() -> None:
-    _TestCase("complexx", support_optimization_mode=False).run()
+    _TestCase(
+        "complexx",
+        support_optimization_mode=False,
+        golden_sha256="4e43481907688740db930c1b034cd746e7a72c6865e9076cfe080d48c1c6f1c2",
+    ).run()
 
 
 def test_encoding_issue52() -> None: