compiler: make sure zero-sized array types are as small as possible

[skabbes]

As part of an #2667 - an effort to use protobufs with tinygo (particularly with WASM):

Fixes #2806

Context
The protobuf lib at google.golang.org/protobuf/runtime/protoimpl includes some code includes some code that acts as a "non-comparable" static check. I have simplified the code to its minimal repro:

package main

import (
	"unsafe"
)

// DoNotCompare can be embedded in a struct to prevent comparability.
type DoNotCompare [0]func()

type MessageState struct {
	DoNotCompare
	data *int32
}

// Static check that MessageState does not exceed the size of a pointer.
const _ = unsafe.Sizeof(unsafe.Pointer(nil)) - unsafe.Sizeof((MessageState{}))

func main() {
	println("protoc minimal example")
}

Problem

alignof([0]func()) was 2 words large, should have only been 1.

[skabbes]

Needs #2853 to be merged to pass windows build.

@deadprogram would you be the right person to review this? I think it is pretty straightforward.

[deadprogram]

@skabbes can you please rebase against dev now to see if CI passing? Thanks!

[skabbes]

@deadprogram all green!

[deadprogram]

@skabbes it would have been better to rebase without that last merge commit. I usually use the command line for this e.g.

git checkout dev
git pull --rebase origin dev
git checkout my-feature-branch
git rebase dev
git push myfork my-feature-branch -f

[skabbes]

Ya, some projects prefer merge - some rebase. I'll squash them and force-push if that's how you prefer in tinygo.

[deadprogram]

I see we need to clarify that on https://tinygo.org/docs/guides/contributing/

[skabbes]

Happy to do it however the team finds best for them - I probably could have figured that out by looking at a few other PRs. But appreciate you letting me know. I'll rebase off dev from now on - and already did it for this branch.

If you want me to squash these too, just say the word!

[skabbes]

I see we need to clarify that on https://tinygo.org/docs/guides/contributing/

Added a PR to clarify:
tinygo-org/tinygo-site#267

[deadprogram]

I think you should perhaps remove commit d0e10e8 now since it was basically replaced by 8a4eecf

[skabbes]

I think you should perhaps remove commit d0e10e8 now since it was basically replaced by 8a4eecf

OK, so I assume that you do no rewrite the commits and squash them when PRs are merged (as many other project do). You would like me to have a perfectly clean commit history, merged as-is into dev.

I'll just squash them both together in that case, since there is no point in having so many commits in dev. But there is value in having them here in this PR, so everyone review can see my thought process as I went through the changes.

[deadprogram]

@skabbes you totally get what we're trying to do, and thank you for the contribution and also for being a cool collaborator!

[skabbes]

@deadprogram thanks! do you mind reviewing the code?

[deadprogram]

Thanks for making the requested changes @skabbes now merging.

[dgryski]

Seems reasonable. I know in the past there have been issues with trailing 0-byte elements in structs. Is there any danger of that happening again here?

[skabbes]

I unfortunately do not have the context to answer that, but it as least seems that the previous intent of the code was that 0-sized structs have alignof = 1. I don't see a reason why 0-sized array types should be any different.

tinygo/compiler/sizes.go

Lines 55 to 59 in 4c7449e

	a := s.Sizeof(T) // may be 0
	// spec: "For a variable x of any type: unsafe.Alignof(x) is at least 1."
	if a < 1 {
	return 1
	}

[aykevl]

After some testing, I found that this PR isn't entirely correct either. Proof:

package main

import "unsafe"

func main() {
        v := struct {
                a byte
                b [0]func()
                c byte
        }{}
        println("unsafe.Alignof: ", int(unsafe.Alignof([0]func(){})))
        println("unsafe.Offsetof:", int(unsafe.Offsetof(v.c)))
        println("actual offset:  ", int(uintptr(unsafe.Pointer(&v.c))-uintptr(unsafe.Pointer(&v))))
}

The unsafe.Offsetof and actual offset values must match, but they do not on 32-bit architectures (on the latest dev branch with this PR included). For example, for wasm:

unsafe.Alignof:  1
unsafe.Offsetof: 1
actual offset:   4

I think this is the proper fix:

diff --git a/compiler/sizes.go b/compiler/sizes.go
index 57108323..36b8d1ab 100644
--- a/compiler/sizes.go
+++ b/compiler/sizes.go
@@ -51,6 +51,8 @@ func (s *stdSizes) Alignof(T types.Type) int64 {
                if t.Info()&types.IsString != 0 {
                        return s.PtrSize
                }
+       case *types.Signature:
+               return s.PtrSize
        }
        a := s.Sizeof(T) // may be 0
        // spec: "For a variable x of any type: unsafe.Alignof(x) is at least 1."

This is a bit different from upstream Go because func values in TinyGo consist of two pointers instead of one.

[aykevl]

Also, to add: TinyGo does not necessarily match Go when it comes to size and alignment. Always check these things by checking against Clang or by comparing pointer values/indices.
The unsafe package can't be used either because it is computed using the values from compiler/sizes.go, and must adhere to what LLVM will make of it (not the other way around).

[deadprogram]

Seems I merged this a little too quickly, then. @skabbes are you able to submit another PR with this change?

[skabbes]

@aykevl thank you - your comment really clarified a mental model for me. The unsafe package simply reports what they layout was - it does not determine it. compiler/sizes.go kind sort of "configures" LLVM, but at the end of the day whatever memory layout clang generates is the source of truth.

@deadprogram I'll give this another shot - add some tests that check the invariants Ayke laid out above.

[aykevl]

@aykevl thank you - your comment really clarified a mental model for me. The unsafe package simply reports what they layout was - it does not determine it. compiler/sizes.go kind sort of "configures" LLVM, but at the end of the day whatever memory layout clang generates is the source of truth.

Yes, basically. Except this bit:

compiler/sizes.go kind sort of "configures" LLVM,

It doesn't. LLVM does its own thing. The only thing compiler/sizes.go is used for, is unsafe.Sizeof/unsafe.Alignof/unsafe.Offsetof. Of these, unsafe.Sizeof and unsafe.Alignof are used by the reflect package for things like Type.Size(), Type.Align(), and Value.Field(i). So essentially it has to match whatever LLVM will do to avoid tricky bugs.

[skabbes]

Perfect, that plus a little extra reading of compiler.go - I think I got it.

I put your suggested changes up for PR #2862, along with to some tests for offsetof alignof and the actual offset.