Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1309)

Issue 3993041: code review 3993041: runtime: fix arm reflect.call boundary case (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
14 years, 5 months ago by rsc
Modified:
14 years, 5 months ago
Reviewers:
CC:
r, r2, golang-dev
Visibility:
Public.

Description

runtime: fix arm reflect.call boundary case The fault was lucky: when it wasn't faulting it was silently copying a word from some other block and later putting that same word back. If some other goroutine had changed that word of memory in the interim, too bad. The ARM code was inconsistent about whether the "argument frame" included the saved LR. Including it made some things more regular but mostly just caused confusion in the places where the regularity broke. Now the rule reflects reality: argp is always a pointer to arguments, never a saved link register. Renamed struct fields to make meaning clearer. Running ARM in QEMU, package time's gotest: * before: 27/58 failed * after: 0/50

Patch Set 1 #

Patch Set 2 : code review 3993041: runtime: fix arm reflect.call boundary case #

Patch Set 3 : code review 3993041: runtime: fix arm reflect.call boundary case #

Patch Set 4 : code review 3993041: runtime: fix arm reflect.call boundary case #

Patch Set 5 : code review 3993041: runtime: fix arm reflect.call boundary case #

Unified diffs Side-by-side diffs Delta from patch set Stats (+129 lines, -115 lines) Patch
M src/cmd/5g/ggen.c View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/5l/noop.c View 1 2 2 chunks +4 lines, -5 lines 0 comments Download
M src/pkg/runtime/386/asm.s View 1 2 3 chunks +6 lines, -6 lines 0 comments Download
M src/pkg/runtime/amd64/asm.s View 1 2 3 6 chunks +9 lines, -9 lines 0 comments Download
M src/pkg/runtime/arm/asm.s View 1 2 3 3 chunks +10 lines, -11 lines 0 comments Download
M src/pkg/runtime/proc.c View 1 2 3 19 chunks +67 lines, -48 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 chunks +6 lines, -9 lines 0 comments Download
M src/pkg/runtime/runtime_defs.go View 1 2 1 chunk +26 lines, -26 lines 0 comments Download

Messages

Total messages: 7
rsc
Hello r (cc: golang-dev@googlegroups.com), I'd like you to review this change.
14 years, 5 months ago (2011-01-13 19:33:22 UTC) #1
r
LGTM although the big comment is mostly describing code that doesn't exist. can you make ...
14 years, 5 months ago (2011-01-13 19:45:46 UTC) #2
rsc
only the 3rd paragraph is referring to removed code. i can delete it
14 years, 5 months ago (2011-01-13 19:51:21 UTC) #3
r2
On Jan 13, 2011, at 11:51 AM, Russ Cox wrote: > only the 3rd paragraph ...
14 years, 5 months ago (2011-01-13 19:56:50 UTC) #4
rsc
Hello r, r2 (cc: golang-dev@googlegroups.com), Please take another look.
14 years, 5 months ago (2011-01-13 22:50:32 UTC) #5
r
LGTM
14 years, 5 months ago (2011-01-13 22:57:51 UTC) #6
rsc
14 years, 5 months ago (2011-01-14 19:05:23 UTC) #7
*** Submitted as http://code.google.com/p/go/source/detail?r=2f6f904b13b1 ***

runtime: fix arm reflect.call boundary case

The fault was lucky: when it wasn't faulting it was silently
copying a word from some other block and later putting
that same word back.  If some other goroutine had changed
that word of memory in the interim, too bad.

The ARM code was inconsistent about whether the
"argument frame" included the saved LR.  Including it made
some things more regular but mostly just caused confusion
in the places where the regularity broke.  Now the rule
reflects reality: argp is always a pointer to arguments,
never a saved link register.

Renamed struct fields to make meaning clearer.

Running ARM in QEMU, package time's gotest:
  * before: 27/58 failed
  * after: 0/50

R=r, r2
CC=golang-dev
http://codereview.appspot.com/3993041
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b