Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

go/types: calling Sizeof(*Struct) twice with different sizes yields incorrect results #16316

Closed
josharian opened this issue Jul 11, 2016 · 5 comments
Closed

go/types: calling Sizeof(*Struct) twice with different sizes yields incorrect results #16316

josharian opened this issue Jul 11, 2016 · 5 comments
Labels
FrozenDueToAge
Milestone
Go1.8

Comments

@josharian
Copy link
Contributor

josharian commented Jul 11, 2016 •
edited
Loading

Typecheck a package containing:

type S struct {
    i int
    b bool
    s string
    n int
}

Given t (of type *types.Struct) that contains the type S, run:

    sizes := types.StdSizes{WordSize: 4, MaxAlign: 4}
    fmt.Println(sizes.Sizeof(t))
    sizes = types.StdSizes{WordSize: 8, MaxAlign: 8}
    fmt.Println(sizes.Sizeof(t))

Want: 20, then 40. Got: 20, then 20. It appears that the problem is in go/types/sizes.go:

func setOffsets(s *Struct, sizes Sizes) bool {
    var calculated bool
    s.offsetsOnce.Do(func() {
        calculated = true
        s.offsets = sizes.Offsetsof(s.fields)
    })
    return calculated
}

Once the offsets for a struct have been calculated once, they are cached forever, even if the sizes parameter changes.

I encountered this while working on improvements to vet. I'm happy to fix, but I'd like input on what direction the fix should take. It's not obvious to me. The only decent options I see are:

(1) removing the cached offsets entirely
(2) keeping a map from StdSizes to cached offsets and type assert to go from Sizes to StdSizes (because not all Sizes implementations can serve as a map key)

and neither of those is particularly appealing.

My temporary workaround to eliminate the call to s.offsetsOnce.Do, to force recalculation each time.

cc @griesemer @mdempsky
@josharian josharian self-assigned this Jul 11, 2016
@josharian
Copy link
Contributor Author

Another, intermediate option: Store the Sizes param that was used to calculate the cached offsets. If it is unchanged (normal case), used the cache. Otherwise recalculate and replace the old cache. Simple, and should maintain performance for normal uses. It loses the concurrency safety of once.Do; does that matter?

@josharian josharian changed the title go/types: *Struct offsets cache not keyed by Sizes go/types: calling Sizeof(*Struct) twice with different sizes yields incorrect results Jul 11, 2016
josharian added a commit to josharian/go that referenced this issue Jul 13, 2016
@josharian
cmd/vet: improve asmdecl parameter handling
21f6968 
DO NOT SUBMIT

[includes temporary workaround for issue golang#16316]

The asmdecl check had hand-rolled code that
calculated the size and offset of parameters
based only on the AST.
It included a list of known named types.

This CL changes asmdecl to use go/types instead.
This allows us to easily handle named types.
It also adds support for structs, arrays,
and complex parameters.

It improves the default names given to unnamed
parameters. Previously, all anonymous arguments were
called "unnamed", and the first anonymous return
argument was called "ret".
Anonymous arguments are now called arg, arg1, arg2,
etc., depending on the index in the argument list.
Return arguments are ret, ret1, ret2.

This CL also fixes a bug in the printing of
composite data type sizes.

Updates golang#11041

Change-Id: I1085116a26fe6199480b680eff659eb9ab31769b
josharian added a commit to josharian/go that referenced this issue Jul 14, 2016
@josharian
cmd/vet: improve asmdecl parameter handling
1e4966e 
DO NOT SUBMIT

[includes temporary workaround for issue golang#16316]

The asmdecl check had hand-rolled code that
calculated the size and offset of parameters
based only on the AST.
It included a list of known named types.

This CL changes asmdecl to use go/types instead.
This allows us to easily handle named types.
It also adds support for structs, arrays,
and complex parameters.

It improves the default names given to unnamed
parameters. Previously, all anonymous arguments were
called "unnamed", and the first anonymous return
argument was called "ret".
Anonymous arguments are now called arg, arg1, arg2,
etc., depending on the index in the argument list.
Return arguments are ret, ret1, ret2.

This CL also fixes a bug in the printing of
composite data type sizes.

Updates golang#11041

Change-Id: I1085116a26fe6199480b680eff659eb9ab31769b
josharian added a commit to josharian/go that referenced this issue Jul 14, 2016
@josharian
cmd/vet: improve asmdecl parameter handling
b06e3fe 
DO NOT SUBMIT

[includes temporary workaround for issue golang#16316]

The asmdecl check had hand-rolled code that
calculated the size and offset of parameters
based only on the AST.
It included a list of known named types.

This CL changes asmdecl to use go/types instead.
This allows us to easily handle named types.
It also adds support for structs, arrays,
and complex parameters.

It improves the default names given to unnamed
parameters. Previously, all anonymous arguments were
called "unnamed", and the first anonymous return
argument was called "ret".
Anonymous arguments are now called arg, arg1, arg2,
etc., depending on the index in the argument list.
Return arguments are ret, ret1, ret2.

This CL also fixes a bug in the printing of
composite data type sizes.

Updates golang#11041

Change-Id: I1085116a26fe6199480b680eff659eb9ab31769b
@griesemer griesemer added this to the Go1.8 milestone Jul 15, 2016
@griesemer griesemer self-assigned this Jul 15, 2016
@griesemer
Copy link
Contributor

I suspect recomputing the size each time is ok. I doubt this is a speed-critical portion of code.

josharian added a commit to josharian/go that referenced this issue Jul 21, 2016
@josharian
cmd/vet: improve asmdecl parameter handling
068b692 
DO NOT SUBMIT

[includes temporary workaround for issue golang#16316]

The asmdecl check had hand-rolled code that
calculated the size and offset of parameters
based only on the AST.
It included a list of known named types.

This CL changes asmdecl to use go/types instead.
This allows us to easily handle named types.
It also adds support for structs, arrays,
and complex parameters.

It improves the default names given to unnamed
parameters. Previously, all anonymous arguments were
called "unnamed", and the first anonymous return
argument was called "ret".
Anonymous arguments are now called arg, arg1, arg2,
etc., depending on the index in the argument list.
Return arguments are ret, ret1, ret2.

This CL also fixes a bug in the printing of
composite data type sizes.

Updates golang#11041

Change-Id: I1085116a26fe6199480b680eff659eb9ab31769b
@josharian
Copy link
Contributor Author

I have a CL ready for 1.8 that removes the cache. I'm not sure how best to confirm that the performance impact is ok, though. Is there a standard corpus to measure on?

@griesemer
Copy link
Contributor

@josharian I tend to run the type checker on the std lib and look at the times. But I doubt that will show you much. Calls to Sizeof are exceedingly rare.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/26995 mentions this issue.

@golang golang locked and limited conversation to collaborators Aug 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
3 participants
@josharian @griesemer @gopherbot