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

x/tools/go/ssa: bad code generation for struct declaration and assignment #10127

Closed
elliott5 opened this issue Mar 10, 2015 · 5 comments
Closed

x/tools/go/ssa: bad code generation for struct declaration and assignment #10127

elliott5 opened this issue Mar 10, 2015 · 5 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@elliott5
Copy link

Issue discovered in http://golang.org/src/compress/flate/inflate.go
when testing that package in https://github.com/tardisgo/tardisgo
using go version go1.4.2 darwin/amd64

Program demonstrating error: reset.go

$ go run reset.go
good
good
good
$ ssadump -run reset.go
good
good
panic: runtime error: invalid memory address or nil pointer dereference

Offending code generated:

func (f *decompressor) ResetBad() { // abridged from the orginal version in the compress/flate package
    *f = decompressor{
        hist: f.hist,
    }
}

/* SSA:
# Name: (*main.decompressor).ResetBad
# Package: main
# Location: reset.go:60:24
# Locals:
#   0:  t0 decompressor
func (f *decompressor) ResetBad():
0:                                                                entry P:0 S:0
    t0 = local decompressor ()                                *decompressor
    t1 = *t0                                                   decompressor
    *f = t1
    t2 = &f.hist [#9]                                         **[32768]byte
    t3 = &f.hist [#9]                                         **[32768]byte
    t4 = *t3                                                   *[32768]byte
    *t2 = t4
    return
*/

The problem is that *f is overwritten and then used to fetch a field value.
Using a temporary variable in Go fixes the problem, see the ResetOK method.
@alandonovan
Copy link
Contributor

Hi Elliot, thanks for reporting this bug. I understand what's wrong. The fix is rather involved so I may not get to it today.

@elliott5
Copy link
Author

No problem Alan, thank you for looking at it so promptly.

I've programmed round that particular instance of the problem; and as the problem probably only occurs in a very unusual set of circumstances, while I wait for your fix I can look out for those circumstances if I hit an inexplicable error again.

It's a tribute to the quality your SSA code that I've got 60+ standard library packages passing their tests so far and this is the first bug I've found.

@alandonovan
Copy link
Contributor

Fix pending.
https://go-review.googlesource.com/#/c/7533/2
Let me know if patching it does not solve your problem.

@alandonovan alandonovan reopened this Mar 13, 2015
@elliott5
Copy link
Author

Thank you for fixing this so quickly Alan, the patch does solve my problem; both in the small test case and for tardisgo/haxe running the compress/flate tests.

@elliott5 elliott5 mentioned this issue Apr 1, 2015
Closed
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title go/ssa: bad code generation for struct declaration and assignment x/tools/go/ssa: bad code generation for struct declaration and assignment Apr 14, 2015
@rsc rsc modified the milestones: Unreleased, Unplanned Apr 14, 2015
@rsc rsc removed the repo-tools label Apr 14, 2015
@elliott5
Copy link
Author

This issue is fixed in the revised x/tools/go/ssa so I'm closing the issue. Thank you again for dealing with it so quickly.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
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
Unreleased
Development

No branches or pull requests
5 participants
@rsc @minux @elliott5 @alandonovan @gopherbot