Add unsafe.Alignof() tests for code that depends on 64-bit atomics

[jmacd]

This includes several metric aggregators and the Meter implementation. Adding simple tests that for alignment will avoid confusing test errors when changes lead to improperly aligned struct fields.

This was noted in #335. See https://go101.org/article/memory-layout.html.

[jmacd]

Assign to @MrAlias.

[MrAlias]

Looking into building some tests for structures with fields that support 64-bit atomic operations and ensuring they are arrange for 64-bit alignment looks to be a bit more complicated than just checking the unsafe.Alignof() for the struct and field and might ultimately just mask an underlying memory address bug.

The unsafe.Alignof() function returns the required alignment of a type, not the actual alignment of an instance. Rather, to understand how a field of a struct will actually be aligned the relevant value to look at is the field's offset. This offset needs to be 64-bit aligned to support 64-bit atomic operations.

To find this offset the unsafe.Offsetof could be used, but that requires prior knowledge of the struct's fields. This means tests would act more as change indicators rather than useful tests. Instead we could try using the reflect package to dynamically determine fields and ensure alignment that way. E.g.

for i := 0; i < reflect.ValueOf(Aggregator{}).NumField(); i++ {
	f := reflect.TypeOf(a).Field(i)

	// TODO: check if field is one that needs to support 64-bit atomic operations.

	if f.Offset % 8 == 0 {
		// fail
	}
	...
}

This seems viable, but it still requires prior knowledge (i.e. a static list) of what types need to support 64-bit atomic operation. Which means it would still act on some level as a change indicator test and possibly have gaps when new structures are created that are passed to 64-bit atomic functions. I guess we could try parsing the AST for the project code to try and make this determination more dynamic, but that sounded a bit extreme.

Alternatively, I was wondering if refactoring the core.Number type might be a better solution. Reading through the linked article I was wondering if we considered a similar approach for core.Number, e.g.

type Number struct {
	x [15]byte // instead of "x uint64"
}

func (n *Number) Value() uint64 {
	return atomic.LoadUint64(n.xAddr())
}

// xAddr returns the 8-byte aligned pointer of n.
func (n *Number) xAddr() *uint64 {
	return (*uint64)(unsafe.Pointer(uintptr(unsafe.Pointer(&n.x)) + 8 - uintptr(unsafe.Pointer(&n.x))%8))
}

// AsInt64Ptr assumes that the number contains an int64 and returns a
// pointer to it.
func (n *Number) AsInt64Ptr() *int64 {
	return rawPtrToInt64Ptr(n.xAddr())
}

// AddInt64Atomic assumes that the number contains an int64 and adds
// the passed int64 to it atomically.
func (n *Number) AddInt64Atomic(i int64) {
	atomic.AddInt64(n.AsInt64Ptr(), i)
}
...

Obviously, there would be downside to this (i.e. potential waste of 7 bytes for Numbers that are not padded, the inability to directly convert to Number type, ...). However, it removes the existing memory address issue. This seems like a strong reason to consider this as it this memory address issue is one that can easily be reintroduced and has catastrophic downsides.

@jmacd I'm interested to hear what you think the best path forward would be here.

[jmacd]

I definitely like the first half -- the proposal to use the reflect package in a test. It was discussed in the SIG today, with a suggestion to use a new atomic struct tag, maybe. (I wonder if there's a convention out there already.)

Another idea is to auto-detect for a list of known types in the test -- core.Number, unsafe.Pointer, and more as needed.

[MrAlias]

I spent some time investigating how we could auto-detect structs that need alignment and I have come to the conclusion that it is not going to be easily achievable.

To dynamically generate a list of all structs in a package (from which you can filter for alignment based on a field tag), you need to parse the syntax tree for the package and all its sub-packages. Then, at runtime, pass values of these structs to the the reflect package's introspection functions. Given Go is not an interpreted language, this would mean building our own interpreter. This seems to be possible using something like Eval, but it also seems excessive.

Without dynamically determining structs it puts us back to just having static lists of structs to check. This type of test doesn't sound like a real improvement, but rather a maintenance overhead.

Going to think a bit more about ways to solve this.

@jmacd I'm interested to hear what your thoughts are on this.

[jmacd]

One idea, although some might call it overkill, is to use https://godoc.org/golang.org/x/tools/go/loader to load a program that depends on all OTel packages. Then it's relatively simple to iterate over all types and identify the structs. We can't use the reflect package in this case, we'll be handling a parsed program object--it's not looking very easy, but it's do-able.

I'm not sure it's worth the effort -- depends on how much of an experience you want :)

[bogdandrutu]

From the description:

Deprecated: This is an older API and does not have support for modules. Use golang.org/x/tools/go/packages instead.

[MrAlias]

@jmacd thanks for the link, I wasn't aware of that package. Taking a look at it and the replacement @bogdandrutu pointed out (https://godoc.org/golang.org/x/tools/go/packages) I think this replaces the part of the code that does discovery. Seems like it could be a good maintained replacement for the PoC I built to test the idea locally.

As for introspecting the structs once they are identified, I was thinking a bit more about that last night and think it is worth trying to get a working example using Eval to use the reflect package and find offsets.

I'll give that a go (hopefully today or early next week). It's definitely been an interesting experience so far, might as well see it through 😄 .

[MrAlias]

Looking further into using Eval to reflect an arbitrary struct's field offset does not look possible. Eval will only return a value if the type is a constant value. This will never be the case, the offset is a variable value.

At this point I don't think it is going to be viable to dynamically check declared structs. Thinking through some ideas of how to go forward I came up with the following:

1. Manually identify and document all of the structures that have fields with types that are receivers for 64-bit atomic operations.
2. Build static tests for the manually identified structures in (1). This would be a "change indicator" set of tests and fail to test any new structure that needs alignment guarantees.
3. Refactor the types that have 64-bit atomic operations performed on them to implicitly align their address (like described above).
4. Investigate alternative synchronization methods and replace the atomic 64-bit operations.

My guess is (1) is viable, but the rest seem like bad ideas (2) or challenging/controversial (3)(4). I'll plan to sync about this next week.

[MrAlias]

From the last SIG meeting (2020-01-02), the consensus was to go with (1) and (2) from the last comment.

The thinking being that comments and test would help capture at least part of the knowledge gained from this investigation. If these comments or tests become more of a problem than a help the problem can be readdressed in the future.