Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TestEncodeAnonymousStruct behaves invalid on ARM (Raspberry Pi) #314

Closed
s00500 opened this issue Aug 27, 2021 · 7 comments
Closed

TestEncodeAnonymousStruct behaves invalid on ARM (Raspberry Pi) #314

s00500 opened this issue Aug 27, 2021 · 7 comments

Comments

@s00500
Copy link

s00500 commented Aug 27, 2021

I have extended the testcase "TestEncodeAnonymousStruct" like this:

func TestEncodeAnonymousStruct(t *testing.T) {
	type Inner struct {
		N int
		B int
		C string
	}
	type Outer0 struct{ Inner }
	type Outer1 struct {
		Inner `toml:"inner"`
	}

	v0 := Outer0{Inner{3, 4, "buu"}}
	expected := "N = 3\nB = 4\nC = \"buu\"\n"
	encodeExpected(t, "embedded anonymous untagged struct", v0, expected, nil)

	v1 := Outer1{Inner{3, 4, "buu"}}
	expected = "[inner]\n  N = 3\n  B = 4\n  C = \"buu\"\n"
	encodeExpected(t, "embedded anonymous tagged struct", v1, expected, nil)
}

This runs fine on my intel macbook

Then I crosscompile for ARM like so:

GOOS=linux GOARCH=arm GOARM=7 go test -c

And execute the binary on a raspberrypi.

Here is the result:

# ./toml.test
./toml.test: line 1: syntax error: unexpected word (expecting ")")
# ./toml.test
--- FAIL: TestEncodeAnonymousStruct (0.00s)
    --- FAIL: TestEncodeAnonymousStruct/embedded_anonymous_untagged_struct (0.00s)
        encode_test.go:426:
            have: C = "buu"
            C = "buu"
            C = "buu"
            want: N = 3
            B = 4
            C = "buu"
2021/08/27 13:10:10 Could not parse '3735928559' as integer: strconv.Atoi: parsing "3735928559": value out of range
#

Does anyone have an idea what could be the issue here ? I realized that it will always take the last field of the embedded struct and adds it for as many times as the struct has fields...

@arp242
Copy link
Collaborator

arp242 commented Aug 28, 2021

A simpler way to reproduce, in master:

$ GOOS=linux GOARCH=arm GOARM=7 go test -c
$ qemu-arm ./toml.test -test.v -test.run TestToml/encode
=== RUN   TestToml
=== RUN   TestToml/encode
2021/08/28 21:57:13 Could not parse '3735928559' as integer: strconv.Atoi: parsing "3735928559": value out of range

The problem is the integer size used in the test, which assumes 64 bits.

At the very least this test should fail in a more obvious way than with log.Fatal as used now; this comes from Remove() in internal/tag/rm.go. Not sure if we really need to fix the test as such, but can add it so SkipTests in toml_test.go if the architecture is 32 bits maybe.

arp242 added a commit that referenced this issue Aug 29, 2021
Don't use log.Fatal in internal/tag/rm.go

Ref: #314
arp242 added a commit that referenced this issue Aug 29, 2021
Use 64bit int instead of strconv.Atoi, which uses the int type, the size
of which is platform-dependent.

Ref: #314
@arp242
Copy link
Collaborator

arp242 commented Aug 29, 2021

Okay, I fixed the "3735928559: value out of range"-error, and the master branch runs cleanly on 32bit arm.

Your new test still fails, but if I replace the int with int64 it works, so this also seems related to some assumptions somewhere that an int is 64 bits. Not entirely sure where though 🤔

Simpler test case:

func TestXXX(t *testing.T) {
	type Inner struct {
		A, B int
	}
	type Outer struct{ Inner }

	encodeExpected(t, "embedded anonymous untagged struct",
		Outer{Inner{1, 2}},
		"A = 1\nB = 2\n",
		nil)
}

Outputs:

[~c/toml](master)% go test
PASS
ok      github.com/BurntSushi/toml      0.064s
[~c/toml](master)% GOARCH=arm GOARM=7 go test -c && qemu-arm ./toml.test -test.failfast
--- FAIL: TestXXX (0.00s)
	--- FAIL: TestXXX/embedded_anonymous_untagged_struct (0.00s)
		encode_test.go:422:
			have:
			B = 2
			B = 2
			want:
			A = 1
			B = 2
FAIL

@arp242
Copy link
Collaborator

arp242 commented Aug 29, 2021

I think it's the FormatInt over here: https://github.com/BurntSushi/toml/blob/master/encode.go#L218-L221

I'll have to check later; many nonletter keys on my laptop are broken and it's all a pain right now; replacement keyboard is taking forever to arrive :-/ But you can check that if you don't want to wait and I think it should be the fix.

@s00500
Copy link
Author

s00500 commented Aug 29, 2021

Awesome debugging work :-) Thanks so much, I am currently not in a hurry with the issue, so I think it can wait for the keyboard

@arp242
Copy link
Collaborator

arp242 commented Sep 11, 2021

A minimal example extracted from the code to reproduce the issue:

package main

import (
	"fmt"
	"reflect"
)

func main() {
	type Inner struct {
		A, B, C string
	}
	type Outer struct{ Inner }

	eStruct("", reflect.ValueOf(Outer{Inner{"a", "b", "c"}}))
}

func eStruct(key string, rv reflect.Value) {
	var (
		rt        = rv.Type()
		fields    [][]int
		addFields func(rt reflect.Type, rv reflect.Value, start []int)
	)
	addFields = func(rt reflect.Type, rv reflect.Value, start []int) {
		fmt.Printf("start: %#v (%[1]p);\tlen %d; cap %d\n", start, len(start), len(start))
		for i := 0; i < rt.NumField(); i++ {
			f := rt.Field(i)

			if f.Anonymous {
				addFields(f.Type, rv.Field(i), append(start, f.Index...))
				//addFields(f.Type, rv.Field(i), []int{0}) // Works
				continue
			}

			//fmt.Printf("append(%#v, append(%#v, %#v...)) = %#v\n",
			//	fields, start, f.Index,
			//	append(fields, append(start, f.Index...)))

			// Works
			// xx := make([]int, len(start))
			// copy(xx, start)
			// fields = append(fields, append(xx, f.Index...))

			//fields = append(fields, append([]int{0}, f.Index...)) // works

			fields = append(fields, append(start, f.Index...))
		}
	}
	addFields(rt, rv, nil)

	for i, f := range fields {
		fmt.Printf("%d: %v\t%s = %s\n", i, f, rt.FieldByIndex(f).Name, rv.FieldByIndex(f))
	}
}

The problem is that this behaves unexpected on 32bit systems:

64bit:
start: []int(nil) (0x0);        len 0; cap 0
start: []int{0} (0xc0000141b0); len 1; cap 1
0: [0 0]        A = a
1: [0 1]        B = b
2: [0 2]        C = c

32bit:
start: []int(nil) (0x0);        len 0; cap 0
start: []int{0} (0x98101a0);    len 1; cap 1
0: [0 2]        C = c
1: [0 2]        C = c
2: [0 2]        C = c

Note how it has [0 2] for all the values, rather than the expected ones; this is for both 386 and ARM.

It does work if I pass a static value or copy start as in the commented-out values, so it seems related to how append() modifies the slice in-place. But the values are always the same, and I'm not sure why it works correct on 64bit and not 32bit or why it breaks in the first place 🤔

I guess I know how to fix it, but I'd like to understand why this is happening.

@arp242
Copy link
Collaborator

arp242 commented Sep 11, 2021

Simpler example:

package main

import "fmt"

func main() {
	var (
		fields    [][]int
		addFields func(start []int)
	)
	addFields = func(start []int) {
		for i := 0; i < 3; i++ {
			if start == nil { // start == nil to not run it in the recursive calls.
				addFields(append(start, i))
				return
			}
			fields = append(fields, append(start, i))
		}
	}
	addFields(nil)

	fmt.Printf("%#v\n", fields)
}

[/tmp/tgo_20210912_6849a0wA]% go run ./main2.go;  GOARCH=386 go build main2.go && ./main2

[][]int{[]int{0, 0}, []int{0, 1}, []int{0, 2}}
[][]int{[]int{0, 2}, []int{0, 2}, []int{0, 2}}

@arp242 arp242 closed this as completed in 1891d01 Sep 23, 2021
@s00500
Copy link
Author

s00500 commented Sep 23, 2021

Awesome!, thanks for fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants