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.

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

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

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

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Jul 11, 2016

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

@josharian josharian commented Jul 11, 2016

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?

Loading

@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
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
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
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

@griesemer griesemer commented Jul 15, 2016

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

Loading

josharian added a commit to josharian/go that referenced this issue Jul 21, 2016
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

@josharian josharian commented Aug 7, 2016

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?

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Aug 11, 2016

@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.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2016

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

Loading

@gopherbot gopherbot closed this in df9eeb1 Aug 16, 2016
@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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants