code review 112870046: time: change nsec back to int32

time: change nsec back to int32

The garbage collector and stack scans are good enough now.

Fixes issue 7446.

[rsc, 2014-07-09 20:36:31]

Hello r (cc: golang-codereviews@googlegroups.com, mtj@google.com),

I'd like you to review this change to
https://code.google.com/p/go/

[r, 2014-07-09 21:04:39]

LGTM

[dvyukov, 2014-07-10 07:43:53]

Are you sure it can't happen now?
The fake "pointer" value can become e.g. "bool block" argument of
runtime.chansend or an unitialized slot in some other C function.

[rsc, 2014-07-10 16:42:40]

On Thu, Jul 10, 2014 at 3:43 AM, <dvyukov@google.com> wrote:

> Are you sure it can't happen now?
> The fake "pointer" value can become e.g. "bool block" argument of
> runtime.chansend or an unitialized slot in some other C function.
>

I don't know what you mean.

I am asserting in this CL that there is no way for a time.Time's nsec field
to be interpreted as a pointer anymore. I'm not talking about chansend or
other things like that.

Russ

[dvyukov, 2014-07-11 08:13:01]

On Thu, Jul 10, 2014 at 8:42 PM, Russ Cox <rsc@golang.org> wrote:
> On Thu, Jul 10, 2014 at 3:43 AM, <dvyukov@google.com> wrote:
>>
>> Are you sure it can't happen now?
>> The fake "pointer" value can become e.g. "bool block" argument of
>> runtime.chansend or an unitialized slot in some other C function.
>
>
> I don't know what you mean.
>
> I am asserting in this CL that there is no way for a time.Time's nsec field
> to be interpreted as a pointer anymore. I'm not talking about chansend or
> other things like that.


The original problem was that "nsec int32" + "padding int32" were
generating random pointer-like values on stack. We still have places
where stack is scanned conservatively -- e.g. chansend. So these fake
random pointers still can be scanned as pointers by GC.

[rsc, 2014-07-12 03:13:43]

I don't understand what you mean when you say that chansend is scanned
conservatively. The problem in issue 5749 was argument frames. Argument
frames have correct types now, even in the C code, and we are careful to
zero output variables at the beginning of the function.

Russ

[dvyukov, 2014-07-14 10:54:01]

On 2014/07/12 03:13:43, rsc wrote:
> I don't understand what you mean when you say that chansend is scanned
> conservatively. The problem in issue 5749 was argument frames. Argument
> frames have correct types now, even in the C code, and we are careful to
> zero output variables at the beginning of the function.


Do you want to say that scan scanning is always 100% precise? And if we spray
stack with random pointers before goroutine starts, it will not have any
negative impact?

[rsc, 2014-07-14 13:55:21]

On Mon, Jul 14, 2014 at 6:54 AM, <dvyukov@google.com> wrote:

> Do you want to say that scan scanning is always 100% precise? And if we
> spray stack with random pointers before goroutine starts, it will not
> have any negative impact?
>

For 1.4 there are not supposed to be any C stack frames left being scanned
for garbage collection.
And Go stack frames are 100% precise.
What is the problem?

Russ

[dvyukov, 2014-07-14 14:11:28]

On 2014/07/14 13:55:21, rsc wrote:
> On Mon, Jul 14, 2014 at 6:54 AM, <mailto:dvyukov@google.com> wrote:
> 
> > Do you want to say that scan scanning is always 100% precise? And if we
> > spray stack with random pointers before goroutine starts, it will not
> > have any negative impact?
> >
> 
> For 1.4 there are not supposed to be any C stack frames left being scanned
> for garbage collection.
> And Go stack frames are 100% precise.
> What is the problem?


I was confused by the word "now":
> The garbage collector and stack scans are good enough *now*.

I can admit that it all will be resolved by the end of 1.4 cycle.

[mdempsky, 2014-07-14 20:39:11]

https://codereview.appspot.com/112870046/diff/20001/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/112870046/diff/20001/src/pkg/time/time.go#newc...
src/pkg/time/time.go:607: nsec := int32(t.nsec) + int32(d%1e9)
There are a handful of these explicit int32 conversions that could be removed. 
Also in Time.Sub and div.

[rsc, 2014-07-16 23:26:49]

*** Submitted as https://code.google.com/p/go/source/detail?r=ab07d66858b6 ***

time: change nsec back to int32

The garbage collector and stack scans are good enough now.

Fixes issue 7446.

LGTM=r
R=r, dvyukov
CC=golang-codereviews, mdempsky, mtj
https://codereview.appspot.com/112870046

[gobot, 2014-07-16 23:49:30]

This CL appears to have broken the android-arm-crawshaw builder.
See http://build.golang.org/log/52493464084267d6567caf1886f9fd50aa22614f

[dfc, 2014-07-16 23:52:07]

Probably unrelated

go_android_exec: adb push
/tmp/go-build766485303/net/http/cgi/_test/cgi.test
/data/local/tmp/goroot/cgi.test-17117-tmp
294 KB/s (4237116 bytes in 14.055s)
go_android_exec: adb shell cp
'/data/local/tmp/goroot/cgi.test-17117-tmp'
'/data/local/tmp/goroot/cgi.test-17117'
go_android_exec: adb shell rm '/data/local/tmp/goroot/cgi.test-17117-tmp'
go_android_exec: adb shell export TMPDIR="/data/local/tmp"; export
GOROOT="/data/local/tmp/goroot"; cd "$GOROOT/src/pkg/net/http/cgi";
'/data/local/tmp/goroot/cgi.test-17117' -test.short=true
-test.timeout=360s; echo -n exitcode=$?
2014/07/16 23:30:48 cgi: copy error: past write limit
Segmentation fault
exitcode=139go_android_exec: adb shell rm
'/data/local/tmp/goroot/cgi.test-17117'
FAIL	net/http/cgi	15.152s



On Thu, Jul 17, 2014 at 9:49 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the android-arm-crawshaw builder.
> See http://build.golang.org/log/52493464084267d6567caf1886f9fd50aa22614f
>
>
> https://codereview.appspot.com/112870046/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>