purego: fix three small fields losing the last field (#211)

There was a bug, that caused the last small field, that directly followed a set of small fields that combined into one full
machine word, in a struct to not be added to the list of arguments. It was caused by incorrectly marking that the field
that is being processed right now is flushed if processing of previous small field resulted in val getting filled enough to
be flushed.

In order to avoid similar errors and to have flushing logic at one place I've moved that out into an anonymous function. It
should impact performance somewhat, I guess I could just inline it. Would like to hear your opinion on that!

Closes #210

commit: 29473c68def94dd8b10d3088261885993a9cd8ba
author: Matvei Vdovitsyn · 2y ago · 2024-02-19 14:45 UTC
committer: GitHub · 2y ago · 2024-02-19 14:45 UTC
parent: c284568
signature: GPG UnverifiedB5690EEEBB952194 The key is unknown

3 files · +47 -26

M struct_amd64.go +29 -26


@@ -76,6 +76,17 @@ func tryPlaceRegister(v reflect.Value, addFloat func(uintptr), addInt func(uintp
76	76		var shift byte // # of bits to shift
77	77		var flushed bool
78	78		class := _NO_CLASS
	79	+	flush := func() {
	80	+	flushed = true
	81	+	if class == _SSE {
	82	+	addFloat(uintptr(val))
	83	+	} else {
	84	+	addInt(uintptr(val))
	85	+	}
	86	+	val = 0
	87	+	shift = 0
	88	+	class = _NO_CLASS
	89	+	}
79	90		var place func(v reflect.Value)
80	91		place = func(v reflect.Value) {
81	92		var numFields int

@@ -93,16 +104,6 @@ func tryPlaceRegister(v reflect.Value, addFloat func(uintptr), addInt func(uintp
93	104		} else {
94	105		f = v.Index(i)
95	106		}
96		-	if shift >= 64 {
97		-	shift = 0
98		-	flushed = true
99		-	if class == _SSE {
100		-	addFloat(uintptr(val))
101		-	} else {
102		-	addInt(uintptr(val))
103		-	}
104		-	class = _NO_CLASS
105		-	}
106	107		switch f.Kind() {
107	108		case reflect.Struct:
108	109		place(f)

@@ -128,10 +129,9 @@ func tryPlaceRegister(v reflect.Value, addFloat func(uintptr), addInt func(uintp
128	129		shift += 32
129	130		class \|= _INTEGER
130	131		case reflect.Int64:
131		-	addInt(uintptr(f.Int()))
132		-	shift = 0
133		-	class = _NO_CLASS
134		-	flushed = true
	132	+	val = uint64(f.Int())
	133	+	shift = 64
	134	+	class = _INTEGER
135	135		case reflect.Uint8:
136	136		val \|= f.Uint() << shift
137	137		shift += 8

@@ -145,10 +145,9 @@ func tryPlaceRegister(v reflect.Value, addFloat func(uintptr), addInt func(uintp
145	145		shift += 32
146	146		class \|= _INTEGER
147	147		case reflect.Uint64:
148		-	addInt(uintptr(f.Uint()))
149		-	shift = 0
150		-	class = _NO_CLASS
151		-	flushed = true
	148	+	val = f.Uint()
	149	+	shift = 64
	150	+	class = _INTEGER
152	151		case reflect.Float32:
153	152		val \|= uint64(math.Float32bits(float32(f.Float()))) << shift
154	153		shift += 32

@@ -158,24 +157,28 @@ func tryPlaceRegister(v reflect.Value, addFloat func(uintptr), addInt func(uintp
158	157		ok = false
159	158		return
160	159		}
161		-	addFloat(uintptr(math.Float64bits(f.Float())))
162		-	class = _NO_CLASS
163		-	flushed = true
	160	+	val = uint64(math.Float64bits(f.Float()))
	161	+	shift = 64
	162	+	class = _SSE
164	163		case reflect.Array:
165	164		place(f)
166	165		default:
167	166		panic("purego: unsupported kind " + f.Kind().String())
168	167		}
	168	+
	169	+	if shift == 64 {
	170	+	flush()
	171	+	} else if shift > 64 {
	172	+	// Should never happen, but may if we forget to reset shift after flush (or forget to flush),
	173	+	// better fall apart here, than corrupt arguments.
	174	+	panic("purego: tryPlaceRegisters shift > 64")
	175	+	}
169	176		}
170	177		}
171	178
172	179		place(v)
173	180		if !flushed {
174		-	if class == _SSE {
175		-	addFloat(uintptr(val))
176		-	} else {
177		-	addInt(uintptr(val))
178		-	}
	181	+	flush()
179	182		}
180	183		return ok
181	184		}

 	var shift byte // # of bits to shift
 	var flushed bool
 	class := _NO_CLASS
+	flush := func() {
+		flushed = true
+		if class == _SSE {
+			addFloat(uintptr(val))
+		} else {
+			addInt(uintptr(val))
+		}
+		val = 0
+		shift = 0
+		class = _NO_CLASS
+	}
 	var place func(v reflect.Value)
 	place = func(v reflect.Value) {
 		var numFields int
 			} else {
 				f = v.Index(i)
 			}
-			if shift >= 64 {
-				shift = 0
-				flushed = true
-				if class == _SSE {
-					addFloat(uintptr(val))
-				} else {
-					addInt(uintptr(val))
-				}
-				class = _NO_CLASS
-			}
 			switch f.Kind() {
 			case reflect.Struct:
 				place(f)
 				shift += 32
 				class |= _INTEGER
 			case reflect.Int64:
-				addInt(uintptr(f.Int()))
+				val = uint64(f.Int())
-				shift = 0
+				shift = 64
-				class = _NO_CLASS
+				class = _INTEGER
-				flushed = true
 			case reflect.Uint8:
 				val |= f.Uint() << shift
 				shift += 8
 				shift += 32
 				class |= _INTEGER
 			case reflect.Uint64:
-				addInt(uintptr(f.Uint()))
+				val = f.Uint()
-				shift = 0
+				shift = 64
-				class = _NO_CLASS
+				class = _INTEGER
-				flushed = true
 			case reflect.Float32:
 				val |= uint64(math.Float32bits(float32(f.Float()))) << shift
 				shift += 32
 					ok = false
 					return
 				}
-				addFloat(uintptr(math.Float64bits(f.Float())))
+				val = uint64(math.Float64bits(f.Float()))
-				class = _NO_CLASS
+				shift = 64
-				flushed = true
+				class = _SSE
 			case reflect.Array:
 				place(f)
 			default:
 				panic("purego: unsupported kind " + f.Kind().String())
 			}
+			if shift == 64 {
+				flush()
+			} else if shift > 64 {
+				// Should never happen, but may if we forget to reset shift after flush (or forget to flush),
+				// better fall apart here, than corrupt arguments.
+				panic("purego: tryPlaceRegisters shift > 64")
+			}
 		}
 	}
 	place(v)
 	if !flushed {
-		if class == _SSE {
+		flush()
-			addFloat(uintptr(val))
-		} else {
-			addInt(uintptr(val))
-		}
 	}
 	return ok
 }

M struct_test.go +10 -0


@@ -118,6 +118,16 @@ func TestRegisterFunc_structArgs(t *testing.T) {
118	118		t.Fatalf("FloatLessThan16Bytes returned %f wanted %f", ret, expectedFloat)
119	119		}
120	120		}
	121	+	{
	122	+	type ThreeSmallFields struct {
	123	+	x, y, z float32
	124	+	}
	125	+	var ThreeSmallFieldsFn func(ThreeSmallFields) float32
	126	+	purego.RegisterLibFunc(&ThreeSmallFieldsFn, lib, "ThreeSmallFields")
	127	+	if ret := ThreeSmallFieldsFn(ThreeSmallFields{1, 2, 7}); ret != expectedFloat {
	128	+	t.Fatalf("ThreeSmallFields returned %f wanted %f", ret, expectedFloat)
	129	+	}
	130	+	}
121	131		{
122	132		type FloatAndInt struct {
123	133		x float32


@@ -118,6 +118,16 @@ func TestRegisterFunc_structArgs(t *testing.T) {
118	t.Fatalf("FloatLessThan16Bytes returned %f wanted %f", ret, expectedFloat)	118		t.Fatalf("FloatLessThan16Bytes returned %f wanted %f", ret, expectedFloat)
119	}	119		}
120	}	120		}
		121	+	{
		122	+	type ThreeSmallFields struct {
		123	+	x, y, z float32
		124	+	}
		125	+	var ThreeSmallFieldsFn func(ThreeSmallFields) float32
		126	+	purego.RegisterLibFunc(&ThreeSmallFieldsFn, lib, "ThreeSmallFields")
		127	+	if ret := ThreeSmallFieldsFn(ThreeSmallFields{1, 2, 7}); ret != expectedFloat {
		128	+	t.Fatalf("ThreeSmallFields returned %f wanted %f", ret, expectedFloat)
		129	+	}
		130	+	}
121	{	131		{
122	type FloatAndInt struct {	132		type FloatAndInt struct {
123	x float32	133		x float32

M testdata/structtest/struct_test.c +8 -0


@@ -74,6 +74,14 @@ float FloatLessThan16Bytes(struct FloatLessThan16Bytes f) {
74	74		return f.x + f.y;
75	75		}
76	76
	77	+	struct ThreeSmallFields {
	78	+	float x, y, z;
	79	+	};
	80	+
	81	+	float ThreeSmallFields(struct ThreeSmallFields f) {
	82	+	return f.x + f.y + f.z;
	83	+	}
	84	+
77	85		struct FloatAndInt {
78	86		float x;
79	87		int y;


@@ -74,6 +74,14 @@ float FloatLessThan16Bytes(struct FloatLessThan16Bytes f) {
74	return f.x + f.y;	74		return f.x + f.y;
75	}	75		}
76		76
		77	+	struct ThreeSmallFields {
		78	+	float x, y, z;
		79	+	};
		80	+
		81	+	float ThreeSmallFields(struct ThreeSmallFields f) {
		82	+	return f.x + f.y + f.z;
		83	+	}
		84	+
77	struct FloatAndInt {	85		struct FloatAndInt {
78	float x;	86		float x;
79	int y;	87		int y;