Incorrect conversion of signed integers (when converting from lower to higher bit-size).

[mewmew]

When converting signed 32-bit integers to 64-bit integers, the sign is lost, and negative values are converted to their unsigned counterpart.

E.g. converting -1 from int32 to int64 results in 4294967295, when the correct result should be -1.

The following test case demonstrates this:

diff --git a/struc_test.go b/struc_test.go
index 6352634..5e18aba 100644
--- a/struc_test.go
+++ b/struc_test.go
@@ -38,6 +38,9 @@ type Example struct {
    Float1 float32 // 41 a0 00 00
    Float2 float64 // 41 35 00 00 00 00 00 00

+   I32f2 int64 `struc:"int32"`  // ff ff ff ff
+   U32f2 int64 `struc:"uint32"` // ff ff ff ff
+
    Size int    `struc:"sizeof=Str,little"` // 0a 00 00 00
    Str  string `struc:"[]byte"`            // "ijklmnopqr"
    Strb string `struc:"[4]byte"`           // "stuv"
@@ -75,6 +78,8 @@ var reference = &Example{
    1, 2, 3, 4, 5, 6, 7, 8, 0, []byte{'a', 'b', 'c', 'd'},
    9, 10, 11, 12, 13, 14, 15, 16, true, false, [4]byte{'e', 'f', 'g', 'h'},
    20, 21,
+   -1,
+   4294967295,
    10, "ijklmnopqr", "stuv",
    4, "1234",
    4, []byte("5678"),
@@ -100,6 +105,9 @@ var referenceBytes = []byte{
    65, 160, 0, 0, // real float32(20)
    64, 53, 0, 0, 0, 0, 0, 0, // real float64(21)

+   255, 255, 255, 255, // fake int32(-1)
+   255, 255, 255, 255, // fake uint32(4294967295)
+
    10, 0, 0, 0, // little-endian int32(10) sizeof=Str
    'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', // Str
    's', 't', 'u', 'v', // fake string([4]byte)

Note, the unsigned test passes the test case successfully. However, the signed test fails.

To merge the test case, either apply this patch, or find it in the signedconv branch at https://github.com/mewpull/struc/tree/signedconv

[lunixbochs]

https://github.com/lunixbochs/struc/blob/master/field.go#L102 - try changing to int64

[lunixbochs]

Need a similar fix and corresponding test on unpack - https://github.com/lunixbochs/struc/blob/master/field.go#L225

Packing from int16 to int64 fails to sign extend, unpacking from int16 to int64 fails to sign extend?

case Int8: n = uint64(int8(buf[0]))
case Int16: n = uint64(int16(order.Uint16(buf)))
...etc

[lunixbochs]

I've been considering trying some shenanigans with field.Interface() and argjoy's type switch codec (which has code to warn on truncation) instead of using full reflection for basic types

[mewmew]

Thanks for the notes!

With https://github.com/mewpull/struc/commit/2b93aae5551fef7b1cb665966556c45141fb506f unpacking of 32-bit signed integers to 64-bit signed integer fields works.

[mewmew]

P.S. I also added some pretty printing of values for failed test cases, so that it is easier to trouble shoot issues such as this.

For instance, if we do not convert the input to a singed 32-bit before converting it to the larger 64-bit signed integer,

diff --git a/field.go b/field.go
index 2c3ba29..64b059c 100644
--- a/field.go
+++ b/field.go
@@ -227,7 +227,7 @@ func (f *Field) unpackVal(buf []byte, val reflect.Value, length int, options *Op
                case Int16:
                        n = int64(int16(order.Uint16(buf)))
                case Int32:
-                       n = int64(int32(order.Uint32(buf)))
+                       n = int64(order.Uint32(buf))
                case Int64:
                        n = int64(order.Uint64(buf))
                }

Then we get the following pretty printed test case failure.

u@x220 ~/g/s/g/l/struc> go test
[I32f2: 4294967295 != -1]
--- FAIL: TestCodec (0.00s)
    struc_test.go:145: encode/decode failed
[I32f2: 4294967295 != -1]
--- FAIL: TestDecode (0.00s)
    struc_test.go:168: decode failed
FAIL
exit status 1
FAIL    github.com/lunixbochs/struc 0.004s

Pretty printing is implemented in https://github.com/mewpull/struc/commit/178c22d98083de3daa4e9b369ff64e709989395e

[lunixbochs]

Since struc is currently a zero-dependency library, I'm kinda against adding a test-only dependency.

[mewmew]

Since struc is currently a zero-dependency library, I'm kinda against adding a test-only dependency.

That's fair. I guess we could use the fmt library to print expected and wanted values? Or just leave it as is, and instrument the test cases when trouble-shooting some code.

[lunixbochs]

Using fmt.Printf("%#v") or %+v and requiring a manual diff is okay.

[mewmew]

Using fmt.Printf("%#v") or %+v and requiring a manual diff is okay.

Done.