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

Issue 116060043: code review 116060043: cmd/gc: mark auxiliary symbols as containing no pointers (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 4 months ago by dvyukov
Modified:
6 years, 4 months ago
Reviewers:
rsc
CC:
golang-codereviews, dfc, josharian, khr1, rsc, iant, khr, rlh
Visibility:
Public.

Description

cmd/gc: mark auxiliary symbols as containing no pointers They do not, but pretend that they do. The immediate need is that it breaks the new GC because these are weird symbols as if with pointers but not necessary pointer aligned.

Patch Set 1 #

Patch Set 2 : diff -r d5c083552430 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 3 : diff -r d5c083552430 https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 2

Patch Set 4 : diff -r da1002b78b19 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 5 : diff -r f09014ba559b https://dvyukov%40google.com@code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -28 lines) Patch
M src/cmd/5g/gsubr.c View 2 chunks +2 lines, -5 lines 0 comments Download
M src/cmd/6g/gsubr.c View 2 chunks +2 lines, -5 lines 0 comments Download
M src/cmd/8g/gsubr.c View 2 chunks +2 lines, -5 lines 0 comments Download
M src/cmd/gc/go.h View 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/gc/obj.c View 5 chunks +5 lines, -4 lines 0 comments Download
M src/cmd/gc/plive.c View 2 chunks +2 lines, -1 line 0 comments Download
M src/cmd/gc/reflect.c View 7 chunks +9 lines, -6 lines 0 comments Download
M src/cmd/gc/walk.c View 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 19
dvyukov
Hello golang-codereviews@googlegroups.com (cc: iant@golang.org, khr@golang.org, rlh@golang.org, rsc@golang.org), I'd like you to review this change to ...
6 years, 4 months ago (2014-07-21 09:19:22 UTC) #1
dfc
This change and the description are a little confusing. You've introduced hasptr, and if !hasptr ...
6 years, 4 months ago (2014-07-21 09:24:34 UTC) #2
dvyukov
On 2014/07/21 09:24:34, dfc wrote: > This change and the description are a little confusing. ...
6 years, 4 months ago (2014-07-21 09:45:44 UTC) #3
dfc
Thanks. Mine is not a strongly informed opinion, just noting that hasptr was always false, ...
6 years, 4 months ago (2014-07-21 09:50:36 UTC) #4
dvyukov
Another option is to remove the flag entirely and always set NOPTR flag (but I ...
6 years, 4 months ago (2014-07-21 12:08:53 UTC) #5
josharian
I also found the CL description quite confusing. Suggested tweaks: Auxiliary symbols were marked as ...
6 years, 4 months ago (2014-07-21 18:14:53 UTC) #6
rsc
This change seems unnecessary. It seems like NOPTR == RODATA. Russ
6 years, 4 months ago (2014-07-21 18:17:46 UTC) #7
rsc
Also, why is the new GC scanning the read-only data at all?
6 years, 4 months ago (2014-07-21 18:18:08 UTC) #8
dvyukov
On 2014/07/21 18:18:08, rsc wrote: > Also, why is the new GC scanning the read-only ...
6 years, 4 months ago (2014-07-21 18:28:13 UTC) #9
dvyukov
On 2014/07/21 18:28:13, dvyukov wrote: > On 2014/07/21 18:18:08, rsc wrote: > > Also, why ...
6 years, 4 months ago (2014-07-21 19:01:36 UTC) #10
khr1
We already have NOPTRDATA and NOPTRBSS symbols (defined by the linker). On Mon, Jul 21, ...
6 years, 4 months ago (2014-07-21 19:35:22 UTC) #11
dvyukov
Yes, that's exactly where I am moving these symbols to. On Mon, Jul 21, 2014 ...
6 years, 4 months ago (2014-07-21 19:37:22 UTC) #12
rsc
https://codereview.appspot.com/116060043/diff/40001/src/cmd/5g/gsubr.c File src/cmd/5g/gsubr.c (right): https://codereview.appspot.com/116060043/diff/40001/src/cmd/5g/gsubr.c#newcode234 src/cmd/5g/gsubr.c:234: if(!hasptr) now that the flags have names, just make ...
6 years, 4 months ago (2014-07-22 03:42:17 UTC) #13
dvyukov
https://codereview.appspot.com/116060043/diff/40001/src/cmd/5g/gsubr.c File src/cmd/5g/gsubr.c (right): https://codereview.appspot.com/116060043/diff/40001/src/cmd/5g/gsubr.c#newcode234 src/cmd/5g/gsubr.c:234: if(!hasptr) On 2014/07/22 03:42:17, rsc wrote: > now that ...
6 years, 4 months ago (2014-07-22 07:43:48 UTC) #14
dvyukov
On 2014/07/22 07:43:48, dvyukov wrote: > https://codereview.appspot.com/116060043/diff/40001/src/cmd/5g/gsubr.c > File src/cmd/5g/gsubr.c (right): > > https://codereview.appspot.com/116060043/diff/40001/src/cmd/5g/gsubr.c#newcode234 > ...
6 years, 4 months ago (2014-07-22 12:54:50 UTC) #15
rsc
On Tue, Jul 22, 2014 at 3:43 AM, <dvyukov@google.com> wrote: > do you mean to ...
6 years, 4 months ago (2014-07-22 12:59:54 UTC) #16
dvyukov
On 2014/07/22 12:59:54, rsc wrote: > On Tue, Jul 22, 2014 at 3:43 AM, <mailto:dvyukov@google.com> ...
6 years, 4 months ago (2014-07-22 13:24:05 UTC) #17
rsc
LGTM much nicer
6 years, 4 months ago (2014-07-23 12:40:48 UTC) #18
dvyukov
6 years, 4 months ago (2014-07-23 13:36:23 UTC) #19
*** Submitted as https://code.google.com/p/go/source/detail?r=2fa7b95cf34c ***

cmd/gc: mark auxiliary symbols as containing no pointers
They do not, but pretend that they do.
The immediate need is that it breaks the new GC because
these are weird symbols as if with pointers but not necessary
pointer aligned.

LGTM=rsc
R=golang-codereviews, dave, josharian, khr, rsc
CC=golang-codereviews, iant, khr, rlh
https://codereview.appspot.com/116060043
Sign in to reply to this message.

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