go: go/types, x/tools/go/types, x/tools/analysis: encoding used by objectpath is inconsistent for use by compiler & tools/analysis

This issue is derived from dominikh/go-tools#924.

What version of Go are you using (`go version`)?

1.16rc1

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (`go env`)?

Linux, ARM64.

What did you do?

When using analyzers implemented using the analysis package, I hit upon a case where facts being exported and imported were being matched up with the wrong methods. (This issue was a method-specific one, and the proposed fix is also method specific.)

For example, instrumenting the facts package in Encode and Decode for serialization and deserialization of facts:

2021/02/09 12:37:21 export: exported func (*pkg/sentry/vfs/vfs.DynamicBytesFileDescriptionImpl).StateFields() []string as DynamicBytesFileDescriptionImpl.M11

2021/02/09 12:18:54 import: imported DynamicBytesFileDescriptionImpl.M11 as func (*pkg/sentry/vfs/vfs.DynamicBytesFileDescriptionImpl).Read(ctx pkg/context/context.Context, dst pkg/usermem/usermem.IOSequence, opts google3/third_party/gvir/pkg/sentry/vfs/vfs.ReadOptions) (int64, error)

It turns out that the objectpath package is producing the name above, and this is what is used for keying the type in the exported fact data. During analysis, the type information for imported packages is sourced from the compiled artifact via gcexportdata, but the current package types are synthesized directly from the source files, and facts are derived from those types. However, this means that there is the possibility of facts being constructed using a different method ordering that what might appear in the compiler artifact, and therefore fact serialization may not key types correctly (and whoever is importing this fact is in for a bad time).

Since this logic is effectively built in to the compiler (which is often a binary package), this has the potential to cause issues for any analysis packages that may link against a different go/types package or perturb the ordering for NamedType.methods.

Possible Fix

While I realize that this API is not stable, the issue is effectively mitigated by making the Method ordering stable for NamedTypes by performing a sort on the first call to Method (and ensuring that no calls to AddMethod happen after that point). This should eliminate the sensitivity issue (but won’t fix breakages in the case of genuine binary incompatibility, which is fine).

A draft of this fix is posted here: https://go-review.googlesource.com/c/go/+/290750

(But of course, I could be way off in my diagnose, or there could be a better solution.)

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 18 (14 by maintainers)

Commits related to this issue

Disable SA1019 (deprecation check) On Go tip (pre-1.18), http://golang.org/issue/44195 is making SA1019 mistake uses of reflect.Value.Len for reflect.Value.InterfaceData, which is deprecated. It is t... — committed to google/gvisor by prattmic 3 years ago
Disable SA1019 (deprecation check) On Go tip (pre-1.18), http://golang.org/issue/44195 is making SA1019 mistake uses of reflect.Value.Len for reflect.Value.InterfaceData, which is deprecated. It is t... — committed to google/gvisor by prattmic 3 years ago
Disable SA1019 (deprecation check) On Go tip (pre-1.18), http://golang.org/issue/44195 is making SA1019 mistake uses of reflect.Value.Len for reflect.Value.InterfaceData, which is deprecated. It is t... — committed to google/gvisor by prattmic 3 years ago
cmd/compile: only sort methods/interfaces during export for -d=unifiedquirks These sorts are only important for 'toolstash -cmp' testing of unified IR against -G=0 mode, but they were added before I ... — committed to golang/go by mdempsky 3 years ago

Most upvoted comments

The objectpath package itself leaves it ambiguous whether its encoding can change.

FWIW, the current encoding depends on implementation details of the export data format that I at least consider to be unstable. E.g., I’ve already landed CLs on the dev.typeparams branch that sort methods before exporting them, so that’s going to change objectpath’s method numbering anyway. (And the x/tools exporter already sorts them too, but using a different sort order.)

So to the extent that objectpath needs to be stable, it needs to be decoupled from the export data ordering of method’s anyway.

If it’s important to maintain historical sort order, it might be able to sort methods based on Pos. But I think sorting on Id is simpler / more robust.

mdempsky on Jul 15, 2021

Here is a description of the reproducers for this issue that I am aware of:

A Go project is built using blaze (Google’s internal version of bazel).
The target contains a file foo/foo.go and a bazel rule to generate a file based on foo.go
The generated file is appended to the GoFile list for the target, bazel-generated/foo/foo_generated.go.
The target is a sent to a native blaze rule to compile it.
GoFile list is sorted by that rule before sending it to the compiler. “bazel-generated/foo/foo_generated.go” < “foo/foo.go” so these now swap positions.
The compiler writes out the gcexportdata in this order.
A separate source analyzer based on (*types.Config).Check loads the files in the package using the original GoFile order.
Both foo/foo.go and bazel-generated/foo/foo.go contain methods on an exported type T.
The objectpath for the methods on T now disagree on what the i’th method of T is between gcexportdata and the types coming from (*types.Config).Check.

We can make method ids in objectpath agnostic to the GoFile order of these two paths by sorting them.

If “foo_generated.go” and “foo.go” contained init() functions, we would be changing the expected order these are executed in. This can potentially be resolved separately.

timothy-king on Jul 14, 2021