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

Issue 93640044: code review 93640044: runtime: fix empty heap dump bug on windows. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 10 months ago by minux1
Modified:
9 years, 10 months ago
Reviewers:
gobot, minux, rsc, khr1, khr
CC:
brainman, khr, bradfitz, rsc, golang-codereviews
Visibility:
Public.

Description

runtime: fix empty heap dump bug on windows. Fixes issue 8119.

Patch Set 1 #

Patch Set 2 : diff -r 5bf1a8b3aeea https://code.google.com/p/go #

Patch Set 3 : diff -r 5bf1a8b3aeea https://code.google.com/p/go #

Total comments: 6

Patch Set 4 : diff -r 5bf1a8b3aeea https://code.google.com/p/go #

Total comments: 3

Patch Set 5 : diff -r 78cda70d487e https://code.google.com/p/go #

Patch Set 6 : diff -r 78cda70d487e https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -6 lines) Patch
A src/pkg/runtime/debug/heapdump_test.go View 1 2 3 4 1 chunk +29 lines, -0 lines 0 comments Download
M src/pkg/runtime/os_plan9.c View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M src/pkg/runtime/os_solaris.c View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/runtime/os_windows.c View 1 2 3 2 chunks +4 lines, -2 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 15
minux1
Hello alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com, khr@golang.org), I'd like you to review this change to https://code.google.com/p/go
9 years, 10 months ago (2014-05-30 00:28:08 UTC) #1
minux1
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/os_windows.c File src/pkg/runtime/os_windows.c (right): https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/os_windows.c#newcode184 src/pkg/runtime/os_windows.c:184: handle = (void*)fd; note: on amd64, HANDLE is 64-bit, ...
9 years, 10 months ago (2014-05-30 00:47:51 UTC) #2
khr
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/debug/heapdump_test.go File src/pkg/runtime/debug/heapdump_test.go (right): https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/debug/heapdump_test.go#newcode25 src/pkg/runtime/debug/heapdump_test.go:25: const minSize = 10240 Where does 10240 come from? ...
9 years, 10 months ago (2014-05-30 01:57:38 UTC) #3
minux1
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/debug/heapdump_test.go File src/pkg/runtime/debug/heapdump_test.go (right): https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/debug/heapdump_test.go#newcode25 src/pkg/runtime/debug/heapdump_test.go:25: const minSize = 10240 On 2014/05/30 01:57:38, khr wrote: ...
9 years, 10 months ago (2014-05-30 02:24:35 UTC) #4
khr
https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/os_windows.c File src/pkg/runtime/os_windows.c (right): https://codereview.appspot.com/93640044/diff/40001/src/pkg/runtime/os_windows.c#newcode184 src/pkg/runtime/os_windows.c:184: handle = (void*)fd; On 2014/05/30 02:24:34, minux wrote: > ...
9 years, 10 months ago (2014-05-30 03:18:51 UTC) #5
minux1
On 2014/05/30 03:18:51, khr wrote: > Then how is this going to work? WriteHeapDump just ...
9 years, 10 months ago (2014-05-30 03:44:26 UTC) #6
khr
On 2014/05/30 03:44:26, minux wrote: > On 2014/05/30 03:18:51, khr wrote: > > Then how ...
9 years, 10 months ago (2014-05-30 04:02:49 UTC) #7
bradfitz
https://codereview.appspot.com/93640044/diff/60001/src/pkg/runtime/debug/heapdump_test.go File src/pkg/runtime/debug/heapdump_test.go (right): https://codereview.appspot.com/93640044/diff/60001/src/pkg/runtime/debug/heapdump_test.go#newcode21 src/pkg/runtime/debug/heapdump_test.go:21: stat, err := f.Stat() stat is generally spelled "fi" ...
9 years, 10 months ago (2014-05-30 04:16:30 UTC) #8
rsc
LGTM https://codereview.appspot.com/93640044/diff/60001/src/pkg/runtime/os_solaris.c File src/pkg/runtime/os_solaris.c (right): https://codereview.appspot.com/93640044/diff/60001/src/pkg/runtime/os_solaris.c#newcode575 src/pkg/runtime/os_solaris.c:575: return runtime·sysvicall6(libc·write, 3, (uintptr)fd, (uintptr)buf, (uintptr)nbyte); On 2014/05/30 ...
9 years, 10 months ago (2014-05-30 17:27:25 UTC) #9
minux1
*** Submitted as https://code.google.com/p/go/source/detail?r=7816cf77a4e5 *** runtime: fix empty heap dump bug on windows. Fixes issue ...
9 years, 10 months ago (2014-05-31 08:09:52 UTC) #10
gobot
This CL appears to have broken the nacl-amd64p32 builder. See http://build.golang.org/log/ef37f913fa65e7e7e7f9b11375363f07f7faccb4
9 years, 10 months ago (2014-05-31 08:30:07 UTC) #11
minux
On Sat, May 31, 2014 at 1:30 AM, <gobot@golang.org> wrote: > This CL appears to ...
9 years, 10 months ago (2014-05-31 09:13:00 UTC) #12
khr1
I don't think WriteHeapDump itself is broken under NaCl, it's more os.File's fault. Just skipping ...
9 years, 10 months ago (2014-05-31 17:59:01 UTC) #13
rsc
skip the test on nacl; there's no need to write anything in the docs.
9 years, 10 months ago (2014-06-01 02:19:14 UTC) #14
minux
9 years, 10 months ago (2014-06-01 03:25:05 UTC) #15
On Sat, May 31, 2014 at 10:59 AM, Keith Randall <khr@google.com> wrote:

> I don't think WriteHeapDump itself is broken under NaCl, it's more
> os.File's fault.  Just skipping the test should be fine.
>
you're right. i should say that it's a platform limitation rather than heap
dump is broken.


On Sat, May 31, 2014 at 7:19 PM, Russ Cox <rsc@golang.org> wrote:

> skip the test on nacl; there's no need to write anything in the docs.
>
skipped in CL 101980048, already submitted.
Sign in to reply to this message.

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