From 79e0c7b71eef1674813aac5e9a2dfe33dee99d18 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 8 Jul 2014 10:11:36 -0400 Subject: [PATCH] go.tools/go/pointer: eliminate TODOs regarding sound treatment of unsafe.Pointer. Ain't gonna happen. Also, don't emit a warning when this happens. LGTM=crawshaw R=crawshaw CC=golang-codereviews https://golang.org/cl/110030044 --- go/pointer/TODO | 9 -------- go/pointer/gen.go | 45 +++++++------------------------------ go/pointer/intrinsics.go | 4 ++-- go/pointer/testdata/conv.go | 2 -- 4 files changed, 10 insertions(+), 50 deletions(-) diff --git a/go/pointer/TODO b/go/pointer/TODO index 9be3b8ca..f95e7062 100644 --- a/go/pointer/TODO +++ b/go/pointer/TODO @@ -8,15 +8,6 @@ CONSTRAINT GENERATION: - a couple of operators are missing - reflect.Values may contain lvalues (CanAddr) - implement native intrinsics. These vary by platform. -- unsafe.Pointer conversions. Three options: - 1) unsoundly (but type-safely) treat p=unsafe.Pointer(x) conversions as - allocations, losing aliases. This is what's currently implemented. - 2) unsoundly (but type-safely) treat p=unsafe.Pointer(x) and T(p) - conversions as interface boxing and unboxing operations. - This may preserve some aliasing relations at little cost. - 3) soundly track physical field offsets. (Summarise dannyb's email here.) - A downside is that we can't keep the identity field of struct - allocations that identifies the object. - add to pts(a.panic) a label representing all runtime panics, e.g. runtime.{TypeAssertionError,errorString,errorCString}. diff --git a/go/pointer/gen.go b/go/pointer/gen.go index e2b7da9e..12bbfb1f 100644 --- a/go/pointer/gen.go +++ b/go/pointer/gen.go @@ -435,29 +435,15 @@ func (a *analysis) genConv(conv *ssa.Convert, cgn *cgnode) { case *types.Pointer: // *T -> unsafe.Pointer? if tDst.Underlying() == tUnsafePtr { - // ignore for now - // a.copy(res, a.valueNode(conv.X), 1) - return + return // we don't model unsafe aliasing (unsound) } case *types.Basic: - switch utDst := tDst.Underlying().(type) { + switch tDst.Underlying().(type) { case *types.Pointer: - // unsafe.Pointer -> *T? (currently unsound) + // Treat unsafe.Pointer->*T conversions like + // new(T) and create an unaliased object. if utSrc == tUnsafePtr { - // For now, suppress unsafe.Pointer conversion - // warnings on "syscall" package. - // TODO(adonovan): audit for soundness. - if conv.Parent().Pkg.Object.Path() != "syscall" { - a.warnf(conv.Pos(), - "unsound: %s contains an unsafe.Pointer conversion (to %s)", - conv.Parent(), tDst) - } - - // For now, we treat unsafe.Pointer->*T - // conversion like new(T) and create an - // unaliased object. In future we may handle - // unsafe conversions soundly; see TODO file. obj := a.addNodes(mustDeref(tDst), "unsafe.Pointer conversion") a.endObject(obj, cgn, conv) a.addressOf(tDst, res, obj) @@ -474,25 +460,10 @@ func (a *analysis) genConv(conv *ssa.Convert, cgn *cgnode) { } case *types.Basic: - // TODO(adonovan): - // unsafe.Pointer -> uintptr? - // uintptr -> unsafe.Pointer - // - // The language doesn't adequately specify the - // behaviour of these operations, but almost - // all uses of these conversions (even in the - // spec) seem to imply a non-moving garbage - // collection strategy, or implicit "pinning" - // semantics for unsafe.Pointer conversions. - - // TODO(adonovan): we need more work before we can handle - // cryptopointers well. - if utSrc == tUnsafePtr || utDst == tUnsafePtr { - // Ignore for now. See TODO file for ideas. - return - } - - return // ignore all other basic type conversions + // All basic-to-basic type conversions are no-ops. + // This includes uintptr<->unsafe.Pointer conversions, + // which we (unsoundly) ignore. + return } } diff --git a/go/pointer/intrinsics.go b/go/pointer/intrinsics.go index 61e2f7ee..5a11ea11 100644 --- a/go/pointer/intrinsics.go +++ b/go/pointer/intrinsics.go @@ -152,12 +152,12 @@ func init() { "sync/atomic.CompareAndSwapUintptr": ext۰NoEffect, "sync/atomic.LoadInt32": ext۰NoEffect, "sync/atomic.LoadInt64": ext۰NoEffect, - "sync/atomic.LoadPointer": ext۰NoEffect, // ignore unsafe.Pointer for now + "sync/atomic.LoadPointer": ext۰NoEffect, // ignore unsafe.Pointers "sync/atomic.LoadUint32": ext۰NoEffect, "sync/atomic.LoadUint64": ext۰NoEffect, "sync/atomic.LoadUintptr": ext۰NoEffect, "sync/atomic.StoreInt32": ext۰NoEffect, - "sync/atomic.StorePointer": ext۰NoEffect, // ignore unsafe.Pointer for now + "sync/atomic.StorePointer": ext۰NoEffect, // ignore unsafe.Pointers "sync/atomic.StoreUint32": ext۰NoEffect, "sync/atomic.StoreUintptr": ext۰NoEffect, "syscall.Close": ext۰NoEffect, diff --git a/go/pointer/testdata/conv.go b/go/pointer/testdata/conv.go index 89623173..692f0ceb 100644 --- a/go/pointer/testdata/conv.go +++ b/go/pointer/testdata/conv.go @@ -40,11 +40,9 @@ func conv3() { print(*y) // @pointsto main.a } -// @warning "main.conv4 contains an unsafe.Pointer conversion" func conv4() { // Handling of unsafe.Pointer conversion is unsound: // we lose the alias to main.a and get something like new(int) instead. - // We require users to provide aliasing summaries. p := (*int)(unsafe.Pointer(&a)) // @line c2p print(p) // @pointsto convert@c2p:13 }