6 ms·

Go CheckLocks Analyzer

94 points by lima 5y ago

nasretdinov 5y ago
I like it. Allows to check some potential race conditions statically and not rely on "go test -race" or running "-race" binaries in prod for a small percent of the traffic (does anyone even do that really?)
maerF0x0 5y ago
i wonder if it might be helpful to go a step further and define an alias type that has the tag applied and accesses examined? // +checkatomic type atomicint int64 type mystruct struct { count atomicint }
- numbsafari 5y ago
  What’s nice about the current approach is that it is explicitly stated at the point of the variable declaration, and not hidden away in some far off place. Also, it doesn’t require you to import a package, it’s completely orthogonal to the consumer of the code. They can opt in if they want, others aren’t forced to import anything.
sour-taste 5y ago
Clang supports similar annotations for C++. That's being developed by Google too: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
travisd 5y ago
I like Go, and it’s nice to see more safety being added, but it definitely feels bolted on in a less than ideal way. Soon enough we’ll have a similar tool that asserts that nilability is checked on certain fields, whereas we could have just had a language with null safety to begin with. I understand there are trade offs (for example non-nillability is add odds with useful zero defaults in some way), but I wonder what was missed that should have been built into the language and type system directly.
- jolux 5y ago
  Tools like this are often a sign that the language was under-designed.
  - spenczar5 5y ago
    Yes, but it is also nice that Go is simple enough (simplistic enough, if you want) that these tools can be added on top, without requiring deep integration.
    - Groxx 5y ago
      I don't know of any language where building a static analyzer requires "deep integration". The closest I can think of is Java, where you can optionally do something during compilation with e.g. an annotation processor, which has a number of rather significant reliability and performance benefits. But that's an optional feature provided by an absurdly mature set of tools, definitely not a requirement - there are tons of java analysis tools that don't plug into javac. If anything, this one serves as a bad example of Go's simplicity being a benefit. It's using the SSA analysis pass to do much of what a compiler does, because the compiler doesn't give you enough data to do it otherwise. It's an extremely complex workaround with only one implementation which is also built by the language developers, though it does plug into a decent framework (x/tools/go/analysis).
      - amscanne 5y ago
        There’s a totally valid complaint that the ssa analysis pass and compiler’s ssa representation are different and should be merged (I have the same one, and I’m pretty sure the Go team knows that), but what’s the problem with exposing this to user analyzer tools? The language is simple enough to allow you to write analyzers in the first place. I’m totally fine with people disliking the tool (it’s purpose built for one codebase!), but I sincerely fail to see how it serves as a bad example of Go’s simplicity.
        Groxx 5y ago
JulienSchmidt 5y ago
At least for atomic access, I still believe that using wrapper types that prevent non-atomic access are the better idea: e.g. https://github.com/julienschmidt/atom https://github.com/julienschmidt/atom (or uber/atomic). That already ensures safe access at dev / compile-time. One explicitly states on declaration that the variable is accessed atomically by using the wrapper type. A linter could then simply check that sync/atomic isn't used directly anywhere.
- amscanne 5y ago
  Note that this also exists in the gVisor code base, the atomics support here is mostly for mixed-mode operation (read atomic, write locked) which is also common. It’s pretty new, so I still need to annotate a lot of the code.
amscanne 5y ago
I’m one of the authors of this analyzer, and it’s surprising to see it here. To clear one misconception: some of the comments seem to be taking this as some kind of official Go tool, or using it a data point regarding the language itself. I don’t think this is justified; the analyzer is only used within the gVisor codebase, and is not an outgrowth of Go itself. On the points raised: I’m not a languages or compiler person; I think it’s pretty neat that I can write a decently powerful static analyzer using the same standardized packages used by vet and other tools. To me, this is the strength of Go: the core is simple. I agree with the idea that it would be cool if Go had some more advanced features in this area, but I also understand why these changes are made slowly, to ensure there’s experience and evidence. In the meantime, we’re free to write tooling that meets our specific needs (and lets one scratch a fun itch). One last thing: the README is incomplete and out of date (globals are supported, for example), but it was never really for public consumption.
- atombender 5y ago
  Maybe this tool could be extracted into its own repo and made ready for public consumption, if it's generally useful?

94 points by lima 5y ago

I like it. Allows to check some potential race conditions statically and not rely on "go test -race" or running "-race" binaries in prod for a small percent of the traffic (does anyone even do that really?)

maerF0x0 5y ago

i wonder if it might be helpful to go a step further and define an alias type that has the tag applied and accesses examined? // +checkatomic type atomicint int64 type mystruct struct { count atomicint }

numbsafari 5y ago
What’s nice about the current approach is that it is explicitly stated at the point of the variable declaration, and not hidden away in some far off place. Also, it doesn’t require you to import a package, it’s completely orthogonal to the consumer of the code. They can opt in if they want, others aren’t forced to import anything.

sour-taste 5y ago

Clang supports similar annotations for C++. That's being developed by Google too: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

travisd 5y ago

I like Go, and it’s nice to see more safety being added, but it definitely feels bolted on in a less than ideal way. Soon enough we’ll have a similar tool that asserts that nilability is checked on certain fields, whereas we could have just had a language with null safety to begin with. I understand there are trade offs (for example non-nillability is add odds with useful zero defaults in some way), but I wonder what was missed that should have been built into the language and type system directly.

jolux 5y ago
Tools like this are often a sign that the language was under-designed.
- spenczar5 5y ago
  Yes, but it is also nice that Go is simple enough (simplistic enough, if you want) that these tools can be added on top, without requiring deep integration.
  - Groxx 5y ago
    I don't know of any language where building a static analyzer requires "deep integration". The closest I can think of is Java, where you can optionally do something during compilation with e.g. an annotation processor, which has a number of rather significant reliability and performance benefits. But that's an optional feature provided by an absurdly mature set of tools, definitely not a requirement - there are tons of java analysis tools that don't plug into javac. If anything, this one serves as a bad example of Go's simplicity being a benefit. It's using the SSA analysis pass to do much of what a compiler does, because the compiler doesn't give you enough data to do it otherwise. It's an extremely complex workaround with only one implementation which is also built by the language developers, though it does plug into a decent framework (x/tools/go/analysis).
    - amscanne 5y ago
      There’s a totally valid complaint that the ssa analysis pass and compiler’s ssa representation are different and should be merged (I have the same one, and I’m pretty sure the Go team knows that), but what’s the problem with exposing this to user analyzer tools? The language is simple enough to allow you to write analyzers in the first place. I’m totally fine with people disliking the tool (it’s purpose built for one codebase!), but I sincerely fail to see how it serves as a bad example of Go’s simplicity.
      - Groxx 5y ago

JulienSchmidt 5y ago

At least for atomic access, I still believe that using wrapper types that prevent non-atomic access are the better idea: e.g. https://github.com/julienschmidt/atom https://github.com/julienschmidt/atom (or uber/atomic). That already ensures safe access at dev / compile-time. One explicitly states on declaration that the variable is accessed atomically by using the wrapper type. A linter could then simply check that sync/atomic isn't used directly anywhere.

amscanne 5y ago
Note that this also exists in the gVisor code base, the atomics support here is mostly for mixed-mode operation (read atomic, write locked) which is also common. It’s pretty new, so I still need to annotate a lot of the code.

amscanne 5y ago

I’m one of the authors of this analyzer, and it’s surprising to see it here. To clear one misconception: some of the comments seem to be taking this as some kind of official Go tool, or using it a data point regarding the language itself. I don’t think this is justified; the analyzer is only used within the gVisor codebase, and is not an outgrowth of Go itself. On the points raised: I’m not a languages or compiler person; I think it’s pretty neat that I can write a decently powerful static analyzer using the same standardized packages used by vet and other tools. To me, this is the strength of Go: the core is simple. I agree with the idea that it would be cool if Go had some more advanced features in this area, but I also understand why these changes are made slowly, to ensure there’s experience and evidence. In the meantime, we’re free to write tooling that meets our specific needs (and lets one scratch a fun itch). One last thing: the README is incomplete and out of date (globals are supported, for example), but it was never really for public consumption.

atombender 5y ago
Maybe this tool could be extracted into its own repo and made ready for public consumption, if it's generally useful?