Add comments and test for 64-bit field alignment (#418)

* Add comments on needed filed alignment

Add comment about alignment requirements to all struct fields who's
values are passed to 64-bit atomic operations.

Update any struct's field ordering if one or more of those fields has
alignment requirements to support 64-bit atomic operations.

* Add 64-bit alignment tests

Most `struct` that have field alignment requirements are now statically
validated prior to testing. The only `struct`s not validated that have
these requirements are ones defined in tests themselves where multiple
`TestMain` functions would be needed to test them. Given the fields are
already identified with comments specifying the alignment requirements
and they are in the test themselves, this seems like an OK omission.

Co-authored-by: Liz Fong-Jones <elizabeth@ctyalcove.org>

api/core/number_test.go

@@ -67,12 +67,14 @@ func TestNumber(t *testing.T) {
 	cmpsForPos := [3]int{1, 1, 0}
 
 	type testcase struct {
-		n    Number
+		// n needs to be aligned for 64-bit atomic operations.
+		n Number
+		// nums needs to be aligned for 64-bit atomic operations.
+		nums [3]Number
 		kind NumberKind
 		pos  bool
 		zero bool
 		neg  bool
-		nums [3]Number
 		cmps [3]int
 	}
 	testcases := []testcase{

api/metric/alignment_test.go

@@ -0,0 +1,24 @@
+package metric
+
+import (
+	"os"
+	"testing"
+	"unsafe"
+
+	ottest "go.opentelemetry.io/otel/internal/testing"
+)
+
+// Ensure struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "Measurement.number",
+			Offset: unsafe.Offsetof(Measurement{}.number),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}

api/metric/api.go

@@ -85,8 +85,9 @@ type MeasureOptionApplier interface {
 // values. Instances of this type should be created by instruments
 // (e.g., Int64Counter.Measurement()).
 type Measurement struct {
-	instrument InstrumentImpl
+	// number needs to be aligned for 64-bit atomic operations.
 	number     core.Number
+	instrument InstrumentImpl
 }
 
 // Instrument returns the instrument that created this measurement.

internal/metric/mock.go

@@ -41,9 +41,10 @@ type (
 	}
 
 	Batch struct {
+		// Measurement needs to be aligned for 64-bit atomic operations.
+		Measurements []Measurement
 		Ctx          context.Context
 		LabelSet     *LabelSet
-		Measurements []Measurement
 	}
 
 	MeterProvider struct {
@@ -58,8 +59,9 @@ type (
 	Kind int8
 
 	Measurement struct {
-		Instrument *Instrument
+		// Number needs to be aligned for 64-bit atomic operations.
 		Number     core.Number
+		Instrument *Instrument
 	}
 )
 

internal/metric/mock_test.go

@@ -0,0 +1,28 @@
+package metric
+
+import (
+	"os"
+	"testing"
+	"unsafe"
+
+	ottest "go.opentelemetry.io/otel/internal/testing"
+)
+
+// Ensure struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "Batch.Measurments",
+			Offset: unsafe.Offsetof(Batch{}.Measurements),
+		},
+		{
+			Name:   "Measurement.Number",
+			Offset: unsafe.Offsetof(Measurement{}.Number),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}

internal/testing/alignment.go

@@ -0,0 +1,57 @@
+package testing
+
+/*
+This file contains common utilities and objects to validate memory alignment
+of Go types. The primary use of this functionality is intended to ensure
+`struct` fields that need to be 64-bit aligned so they can be passed as
+arguments to 64-bit atomic operations.
+
+The common workflow is to define a slice of `FieldOffset` and pass them to the
+`Aligned8Byte` function from within a `TestMain` function from a package's
+tests. It is important to make this call from the `TestMain` function prior
+to running the rest of the test suit as it can provide useful diagnostics
+about field alignment instead of ambiguous nil pointer dereference and runtime
+panic.
+
+For more information:
+https://github.com/open-telemetry/opentelemetry-go/issues/341
+*/
+
+import (
+	"fmt"
+	"io"
+)
+
+// FieldOffset is a preprocessor representation of a struct field alignment.
+type FieldOffset struct {
+	// Name of the field.
+	Name string
+
+	// Offset of the field in bytes.
+	//
+	// To compute this at compile time use unsafe.Offsetof.
+	Offset uintptr
+}
+
+// Aligned8Byte returns if all fields are aligned modulo 8-bytes.
+//
+// Error messaging is printed to out for any fileds determined misaligned.
+func Aligned8Byte(fields []FieldOffset, out io.Writer) bool {
+	misaligned := make([]FieldOffset, 0)
+	for _, f := range fields {
+		if f.Offset%8 != 0 {
+			misaligned = append(misaligned, f)
+		}
+	}
+
+	if len(misaligned) == 0 {
+		return true
+	}
+
+	fmt.Fprintln(out, "struct fields not aligned for 64-bit atomic operations:")
+	for _, f := range misaligned {
+		fmt.Fprintf(out, "  %s: %d-byte offset\n", f.Name, f.Offset)
+	}
+
+	return false
+}

internal/trace/mock_tracer.go

@@ -28,12 +28,14 @@ import (
 // It only supports ChildOf option. SpanId is atomically increased every time a
 // new span is created.
 type MockTracer struct {
-	// Sampled specifies if the new span should be sampled or not.
-	Sampled bool
-
 	// StartSpanID is used to initialize spanId. It is incremented by one
 	// every time a new span is created.
+	//
+	// StartSpanID has to be aligned for 64-bit atomic operations.
 	StartSpanID *uint64
+
+	// Sampled specifies if the new span should be sampled or not.
+	Sampled bool
 }
 
 var _ apitrace.Tracer = (*MockTracer)(nil)

internal/trace/mock_tracer_test.go

@@ -0,0 +1,24 @@
+package trace
+
+import (
+	"os"
+	"testing"
+	"unsafe"
+
+	ottest "go.opentelemetry.io/otel/internal/testing"
+)
+
+// Ensure struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "MockTracer.StartSpanID",
+			Offset: unsafe.Offsetof(MockTracer{}.StartSpanID),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}

sdk/metric/aggregator/array/array.go

@@ -28,10 +28,11 @@ import (
 
 type (
 	Aggregator struct {
+		// ckptSum needs to be aligned for 64-bit atomic operations.
+		ckptSum    core.Number
 		lock       sync.Mutex
 		current    points
 		checkpoint points
-		ckptSum    core.Number
 	}
 
 	points []core.Number

sdk/metric/aggregator/array/array_test.go

@@ -18,16 +18,34 @@ import (
 	"context"
 	"fmt"
 	"math"
+	"os"
 	"testing"
+	"unsafe"
 
 	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/otel/api/core"
+	ottest "go.opentelemetry.io/otel/internal/testing"
 	export "go.opentelemetry.io/otel/sdk/export/metric"
 	"go.opentelemetry.io/otel/sdk/export/metric/aggregator"
 	"go.opentelemetry.io/otel/sdk/metric/aggregator/test"
 )
 
+// Ensure struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "Aggregator.ckptSum",
+			Offset: unsafe.Offsetof(Aggregator{}.ckptSum),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}
+
 type updateTest struct {
 	count    int
 	absolute bool

sdk/metric/aggregator/counter/counter.go

@@ -25,9 +25,11 @@ import (
 // Aggregator aggregates counter events.
 type Aggregator struct {
 	// current holds current increments to this counter record
+	// current needs to be aligned for 64-bit atomic operations.
 	current core.Number
 
 	// checkpoint is a temporary used during Checkpoint()
+	// checkpoint needs to be aligned for 64-bit atomic operations.
 	checkpoint core.Number
 }
 

sdk/metric/aggregator/counter/counter_test.go

@@ -16,17 +16,39 @@ package counter
 
 import (
 	"context"
+	"os"
 	"testing"
+	"unsafe"
 
 	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/otel/api/core"
+	ottest "go.opentelemetry.io/otel/internal/testing"
 	export "go.opentelemetry.io/otel/sdk/export/metric"
 	"go.opentelemetry.io/otel/sdk/metric/aggregator/test"
 )
 
 const count = 100
 
+// Ensure struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "Aggregator.current",
+			Offset: unsafe.Offsetof(Aggregator{}.current),
+		},
+		{
+			Name:   "Aggregator.checkpoint",
+			Offset: unsafe.Offsetof(Aggregator{}.checkpoint),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}
+
 func TestCounterMonotonic(t *testing.T) {
 	ctx := context.Background()
 

sdk/metric/aggregator/gauge/gauge.go

@@ -45,6 +45,8 @@ type (
 	// a sequence number to determine the winner of a race.
 	gaugeData struct {
 		// value is the int64- or float64-encoded Set() data
+		//
+		// value needs to be aligned for 64-bit atomic operations.
 		value core.Number
 
 		// timestamp indicates when this record was submitted.

sdk/metric/aggregator/gauge/gauge_test.go

@@ -17,11 +17,14 @@ package gauge
 import (
 	"context"
 	"math/rand"
+	"os"
 	"testing"
+	"unsafe"
 
 	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/otel/api/core"
+	ottest "go.opentelemetry.io/otel/internal/testing"
 	export "go.opentelemetry.io/otel/sdk/export/metric"
 	"go.opentelemetry.io/otel/sdk/export/metric/aggregator"
 	"go.opentelemetry.io/otel/sdk/metric/aggregator/test"
@@ -31,6 +34,21 @@ const count = 100
 
 var _ export.Aggregator = &Aggregator{}
 
+// Ensure struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "gaugeData.value",
+			Offset: unsafe.Offsetof(gaugeData{}.value),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}
+
 func TestGaugeNonMonotonic(t *testing.T) {
 	ctx := context.Background()
 

sdk/metric/aggregator/minmaxsumcount/mmsc.go

@@ -26,12 +26,15 @@ type (
 	// Aggregator aggregates measure events, keeping only the max,
 	// sum, and count.
 	Aggregator struct {
-		current    state
+		// current has to be aligned for 64-bit atomic operations.
+		current state
+		// checkpoint has to be aligned for 64-bit atomic operations.
 		checkpoint state
 		kind       core.NumberKind
 	}
 
 	state struct {
+		// all fields have to be aligned for 64-bit atomic operations.
 		count core.Number
 		sum   core.Number
 		min   core.Number

sdk/metric/aggregator/minmaxsumcount/mmsc_test.go

@@ -18,11 +18,14 @@ import (
 	"context"
 	"math"
 	"math/rand"
+	"os"
 	"testing"
+	"unsafe"
 
 	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/otel/api/core"
+	ottest "go.opentelemetry.io/otel/internal/testing"
 	export "go.opentelemetry.io/otel/sdk/export/metric"
 	"go.opentelemetry.io/otel/sdk/export/metric/aggregator"
 	"go.opentelemetry.io/otel/sdk/metric/aggregator/test"
@@ -59,6 +62,41 @@ var (
 	}
 )
 
+// Ensure struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "Aggregator.current",
+			Offset: unsafe.Offsetof(Aggregator{}.current),
+		},
+		{
+			Name:   "Aggregator.checkpoint",
+			Offset: unsafe.Offsetof(Aggregator{}.checkpoint),
+		},
+		{
+			Name:   "state.count",
+			Offset: unsafe.Offsetof(state{}.count),
+		},
+		{
+			Name:   "state.sum",
+			Offset: unsafe.Offsetof(state{}.sum),
+		},
+		{
+			Name:   "state.min",
+			Offset: unsafe.Offsetof(state{}.min),
+		},
+		{
+			Name:   "state.max",
+			Offset: unsafe.Offsetof(state{}.max),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}
+
 func TestMinMaxSumCountAbsolute(t *testing.T) {
 	test.RunProfiles(t, func(t *testing.T, profile test.Profile) {
 		minMaxSumCount(t, profile, positiveOnly)

sdk/metric/aggregator/test/test.go

@@ -17,10 +17,13 @@ package test
 import (
 	"context"
 	"math/rand"
+	"os"
 	"sort"
 	"testing"
+	"unsafe"
 
 	"go.opentelemetry.io/otel/api/core"
+	ottest "go.opentelemetry.io/otel/internal/testing"
 	export "go.opentelemetry.io/otel/sdk/export/metric"
 	"go.opentelemetry.io/otel/sdk/export/metric/aggregator"
 )
@@ -63,9 +66,25 @@ func RunProfiles(t *testing.T, f func(*testing.T, Profile)) {
 	}
 }
 
+// Ensure local struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "Numbers.numbers",
+			Offset: unsafe.Offsetof(Numbers{}.numbers),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}
+
 type Numbers struct {
-	kind    core.NumberKind
+	// numbers has to be aligned for 64-bit atomic operations.
 	numbers []core.Number
+	kind    core.NumberKind
 }
 
 func NewNumbers(kind core.NumberKind) Numbers {

sdk/metric/alignment_test.go

@@ -0,0 +1,36 @@
+package metric
+
+import (
+	"os"
+	"testing"
+	"unsafe"
+
+	ottest "go.opentelemetry.io/otel/internal/testing"
+)
+
+// Ensure struct alignment prior to running tests.
+func TestMain(m *testing.M) {
+	fields := []ottest.FieldOffset{
+		{
+			Name:   "record.refcount",
+			Offset: unsafe.Offsetof(record{}.refcount),
+		},
+		{
+			Name:   "record.collectedEpoch",
+			Offset: unsafe.Offsetof(record{}.collectedEpoch),
+		},
+		{
+			Name:   "record.modifiedEpoch",
+			Offset: unsafe.Offsetof(record{}.modifiedEpoch),
+		},
+		{
+			Name:   "record.reclaim",
+			Offset: unsafe.Offsetof(record{}.reclaim),
+		},
+	}
+	if !ottest.Aligned8Byte(fields, os.Stderr) {
+		os.Exit(1)
+	}
+
+	os.Exit(m.Run())
+}

sdk/metric/monotone_test.go

@@ -31,11 +31,12 @@ import (
 )
 
 type monotoneBatcher struct {
-	t *testing.T
+	// currentValue needs to be aligned for 64-bit atomic operations.
-
-	collections  int
 	currentValue *core.Number
+	collections  int
 	currentTime  *time.Time
+
+	t *testing.T
 }
 
 func (*monotoneBatcher) AggregatorFor(*export.Descriptor) export.Aggregator {

sdk/metric/sdk.go

@@ -97,31 +97,39 @@ type (
 	// `record` in existence at a time, although at most one can
 	// be referenced from the `SDK.current` map.
 	record struct {
-		// labels is the LabelSet passed by the user.
-		labels *labels
-
-		// descriptor describes the metric instrument.
-		descriptor *export.Descriptor
-
 		// refcount counts the number of active handles on
 		// referring to this record.  active handles prevent
 		// removing the record from the current map.
+		//
+		// refcount has to be aligned for 64-bit atomic operations.
 		refcount int64
 
 		// collectedEpoch is the epoch number for which this
 		// record has been exported.  This is modified by the
 		// `Collect()` method.
+		//
+		// collectedEpoch has to be aligned for 64-bit atomic operations.
 		collectedEpoch int64
 
 		// modifiedEpoch is the latest epoch number for which
 		// this record was updated.  Generally, if
 		// modifiedEpoch is less than collectedEpoch, this
 		// record is due for reclaimation.
+		//
+		// modifiedEpoch has to be aligned for 64-bit atomic operations.
 		modifiedEpoch int64
 
 		// reclaim is an atomic to control the start of reclaiming.
+		//
+		// reclaim has to be aligned for 64-bit atomic operations.
 		reclaim int64
 
+		// labels is the LabelSet passed by the user.
+		labels *labels
+
+		// descriptor describes the metric instrument.
+		descriptor *export.Descriptor
+
 		// recorder implements the actual RecordOne() API,
 		// depending on the type of aggregation.  If nil, the
 		// metric was disabled by the exporter.

sdk/metric/stress_test.go

@@ -51,6 +51,7 @@ const (
 
 type (
 	testFixture struct {
+		// stop has to be aligned for 64-bit atomic operations.
 		stop     int64
 		expected sync.Map
 		received sync.Map // Note: doesn't require synchronization
@@ -93,6 +94,7 @@ type (
 	// where a race condition causes duplicate records.  We always
 	// take the later timestamp.
 	gaugeState struct {
+		// raw has to be aligned for 64-bit atomic operations.
 		raw core.Number
 		ts  time.Time
 	}