Skip to content

Commit

Permalink
metrics/generic: fix uint64 alignment (#1007)
Browse files Browse the repository at this point in the history
* fix: uint64 alignment in metrics/generic

* test: adds a weak test on atomic alignment.
  • Loading branch information
ldez committed Sep 15, 2020
1 parent 4133f7f commit 439c4d2
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 2 deletions.
4 changes: 2 additions & 2 deletions metrics/generic/generic.go
Expand Up @@ -18,9 +18,9 @@ import (

// Counter is an in-memory implementation of a Counter.
type Counter struct {
bits uint64 // bits has to be the first word in order to be 64-aligned on 32-bit
Name string
lvs lv.LabelValues
bits uint64
}

// NewCounter returns a new, usable Counter.
Expand Down Expand Up @@ -81,9 +81,9 @@ func (c *Counter) LabelValues() []string {

// Gauge is an in-memory implementation of a Gauge.
type Gauge struct {
bits uint64 // bits has to be the first word in order to be 64-aligned on 32-bit
Name string
lvs lv.LabelValues
bits uint64
}

// NewGauge returns a new, usable Gauge.
Expand Down
75 changes: 75 additions & 0 deletions metrics/generic/generic_test.go
Expand Up @@ -5,6 +5,12 @@ package generic_test
// generic to use its Histogram in the Quantiles helper function.

import (
"go/ast"
"go/importer"
"go/parser"
"go/token"
"go/types"
"io/ioutil"
"math"
"math/rand"
"sync"
Expand Down Expand Up @@ -107,3 +113,72 @@ func TestSimpleHistogram(t *testing.T) {
t.Errorf("want %f, have %f", want, have)
}
}

// Naive atomic alignment test.
// The problem is related to the use of `atomic.*` and not directly to a structure.
// But currently works for Counter and Gauge.
// To have a more solid test, this test should be removed and the other tests should be run on a 32-bit arch.
func TestAtomicAlignment(t *testing.T) {
content, err := ioutil.ReadFile("./generic.go")
if err != nil {
t.Fatal(err)
}

fset := token.NewFileSet()

file, err := parser.ParseFile(fset, "generic.go", content, parser.ParseComments)
if err != nil {
t.Fatal(err)
}

conf := types.Config{Importer: importer.ForCompiler(fset, "source", nil)}

pkg, err := conf.Check(".", fset, []*ast.File{file}, nil)
if err != nil {
t.Fatal(err)
}

// uses ARM as reference for 32-bit arch
sizes := types.SizesFor("gc", "arm")

names := []string{"Counter", "Gauge"}

for _, name := range names {
t.Run(name, func(t *testing.T) {
checkAtomicAlignment(t, sizes, pkg.Scope().Lookup(name), pkg)
})
}
}

func checkAtomicAlignment(t *testing.T, sizes types.Sizes, obj types.Object, pkg *types.Package) {
t.Helper()

st := obj.Type().Underlying().(*types.Struct)

posToCheck := make(map[int]types.Type)

var vars []*types.Var
for i := 0; i < st.NumFields(); i++ {
field := st.Field(i)

if v, ok := field.Type().(*types.Basic); ok {
switch v.Kind() {
case types.Uint64, types.Float64, types.Int64:
posToCheck[i] = v
}
}

vars = append(vars, types.NewVar(field.Pos(), pkg, field.Name(), field.Type()))
}

offsets := sizes.Offsetsof(vars)
for i, offset := range offsets {
if _, ok := posToCheck[i]; !ok {
continue
}

if offset%8 != 0 {
t.Errorf("misalignment detected in %s for the type %s, offset %d", obj.Name(), posToCheck[i], offset)
}
}
}

0 comments on commit 439c4d2

Please sign in to comment.