Skip to content
New issue

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

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

Already on GitHub? # to your account

testing: atomic variable offsets static analysis tool #5027

Closed
philips opened this issue Apr 10, 2016 · 9 comments
Closed

testing: atomic variable offsets static analysis tool #5027

philips opened this issue Apr 10, 2016 · 9 comments

Comments

@philips
Copy link
Contributor

philips commented Apr 10, 2016

Every few months etcd breaks on 32 bit systems due to atomic alignments, see #4734 and #4733. We need a static analysis tool to check for unaligned atomics that is part of our test runs.

cc @luxas

@philips
Copy link
Contributor Author

philips commented Apr 10, 2016

This is necessary for us to drop the unsupported flag: #4749

@xiang90
Copy link
Contributor

xiang90 commented Apr 11, 2016

@philips I agree with you. To drop the flag, we need to be confident with arm. We need to have this tool and make sure other dependencies work well with arm/32bit (for example boltdb). We need to have doc around how to build and what are the limitations.

@luxas It would be great if you can help with arm part. Thanks!

@ericchiang
Copy link
Contributor

Here's my initial plan.

  1. Scan package for uses of atomic.X(&f.bar) calls using go/types.
  2. Determine type of first argument. If it's not a struct field stop analysis.
  3. For each struct field, generate a program that checks the offset (will do something tricky source rewrites so it will work for un-exported fields).
  4. Run program. Report results.

Example generated program.

package main
// imports
func main() {
    var f foo
    if invalidOffset(unsafe.Offsetof(f.bar)) {
        fmt.Fprintln(os.Stderr, "invalid offset of field foo.bar")
        os.Exit(2)
    }
}

@luxas
Copy link
Contributor

luxas commented Apr 12, 2016

@ericchiang Sounds good. We should make sure though that we make it generic, so k8s for example may use it. There haven't been known issues with atomic alignment in k8s, but it's a huge project, so probably somewhere something is wrong.

@xiang90 @philips I may send a PR that compiles etcd for experimental arches (arm, arm64 and ppc64le) for testing. As a first step, we have to make sure etcd always compiles, so travis should catch that. Is a simple test-compilation bash script enough when added to travis.yaml or should I put this in test? (That would slow down normal tests though)
WDYT?

Also, when will a v2.3.2 be released with #5028?

@heyitsanthony
Copy link
Contributor

@ericchiang and I chatted yesterday about doing static anlaysis for this. Detecting alignment problems is a lot more difficult than his initial proposal. Some examples:

Interfaces:

func (i *consistentIndex) ConsistentIndex() uint64 {
        return atomic.LoadUint64((*uint64)(i))
}

slices:

type A struct {
x int64
y int32
}
s := make([]A, 2)
a := &s[1]
atomic.LoadInt64(&a.x)

@luxas
Copy link
Contributor

luxas commented Apr 20, 2016

Repeating my question: When will v2.3.2 be released with #5028?

It's really important to release a version with that change
@xiang90 @philips @gyuho @heyitsanthony @glevand

@gyuho
Copy link
Contributor

gyuho commented Apr 20, 2016

@luxas We plan to release v2.3.2 today. I will ask @xiang90 if we can include that.

@xiang90 xiang90 added this to the unplanned milestone Apr 29, 2016
@heyitsanthony
Copy link
Contributor

@xiang90 @philips Can we close this? I feel this is a dead end.

@xiang90
Copy link
Contributor

xiang90 commented Aug 15, 2016

@heyitsanthony I think we should just close this. People start to run tests + CI to ensure etcd is good on a platform. Here is the doc for that: https://github.com/coreos/etcd/blob/master/Documentation/op-guide/supported-platform.md

Static analysis could be beneficial, but it does seem that no one will work on that just for etcd.

@xiang90 xiang90 closed this as completed Aug 15, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Development

No branches or pull requests

6 participants