code review 116060043: cmd/gc: mark auxiliary symbols as containing no pointers

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.

[dvyukov, 2014-07-21 09:19:22]

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
https://dvyukov%40google.com@code.google.com/p/go/

[dave_cheney.net, 2014-07-21 09:24:34]

This change and the description are a little confusing. You've introduced
hasptr, and if !hasptr then RODATA is set on the value, but hasptr is always
false in all the invocations, so the logic of this change is hard to follow.

Could I ask you to take another pass at it, maybe make the parameter `noptr`, so
there are less negations of false values in the normal path.

[dvyukov, 2014-07-21 09:45:44]

On 2014/07/21 09:24:34, dfc wrote:
> This change and the description are a little confusing. You've introduced
> hasptr, and if !hasptr then RODATA is set on the value, but hasptr is always
> false in all the invocations, so the logic of this change is hard to follow.
> 
> Could I ask you to take another pass at it, maybe make the parameter `noptr`,
so
> there are less negations of false values in the normal path.


It's not RODATA, it's NOPTR. If !hasptr, then I set NOPTR, looks reasonable to
me.
I've chosen hasptr instead of noptr, because we have haspointers(t) function in
the compiler and hasptr is positive (as opposed to noptr which is already
negative). Having said that, I do not have strong preference. If there is
consensus, I am ready to change it to noptr.

[dave_cheney.net, 2014-07-21 09:50:36]

Thanks. Mine is not a strongly informed opinion, just noting that hasptr
was always false, and then its negation by if(!hasptr) was the K&R
equivalent of a double negative.


On Mon, Jul 21, 2014 at 7:45 PM, <dvyukov@google.com> wrote:

> On 2014/07/21 09:24:34, dfc wrote:
>
>> This change and the description are a little confusing. You've
>>
> introduced
>
>> hasptr, and if !hasptr then RODATA is set on the value, but hasptr is
>>
> always
>
>> false in all the invocations, so the logic of this change is hard to
>>
> follow.
>
>  Could I ask you to take another pass at it, maybe make the parameter
>>
> `noptr`, so
>
>> there are less negations of false values in the normal path.
>>
>
>
> It's not RODATA, it's NOPTR. If !hasptr, then I set NOPTR, looks
> reasonable to me.
> I've chosen hasptr instead of noptr, because we have haspointers(t)
> function in the compiler and hasptr is positive (as opposed to noptr
> which is already negative). Having said that, I do not have strong
> preference. If there is consensus, I am ready to change it to noptr.
>
>
> https://codereview.appspot.com/116060043/
>

[dvyukov, 2014-07-21 12:08:53]

Another option is to remove the flag entirely and always set NOPTR
flag (but I wanted to be more explicit).
Let's wait for somebody else.


On Mon, Jul 21, 2014 at 1:50 PM, Dave Cheney <dave@cheney.net> wrote:
> Thanks. Mine is not a strongly informed opinion, just noting that hasptr was
> always false, and then its negation by if(!hasptr) was the K&R equivalent of
> a double negative.
>
>
> On Mon, Jul 21, 2014 at 7:45 PM, <dvyukov@google.com> wrote:
>>
>> On 2014/07/21 09:24:34, dfc wrote:
>>>
>>> This change and the description are a little confusing. You've
>>
>> introduced
>>>
>>> hasptr, and if !hasptr then RODATA is set on the value, but hasptr is
>>
>> always
>>>
>>> false in all the invocations, so the logic of this change is hard to
>>
>> follow.
>>
>>> Could I ask you to take another pass at it, maybe make the parameter
>>
>> `noptr`, so
>>>
>>> there are less negations of false values in the normal path.
>>
>>
>>
>> It's not RODATA, it's NOPTR. If !hasptr, then I set NOPTR, looks
>> reasonable to me.
>> I've chosen hasptr instead of noptr, because we have haspointers(t)
>> function in the compiler and hasptr is positive (as opposed to noptr
>> which is already negative). Having said that, I do not have strong
>> preference. If there is consensus, I am ready to change it to noptr.
>>
>>
>> https://codereview.appspot.com/116060043/
>
>

[josharian, 2014-07-21 18:14:53]

I also found the CL description quite confusing. Suggested tweaks:

Auxiliary symbols were marked as containing pointers, but they do not.
The immediate need is that it breaks the new GC because
these appeared as weird symbols containing pointers, yet they were not
necessarily pointer aligned.


> Another option is to remove the flag entirely and always set NOPTR flag (but I
wanted to be more explicit).

Do you foresee a concrete, near-term need for the flag? If not, I'd go for
always setting the NOPTR flag for now. Just my 2c.

[rsc, 2014-07-21 18:17:46]

This change seems unnecessary. It seems like NOPTR == RODATA.

Russ

[rsc, 2014-07-21 18:18:08]

Also, why is the new GC scanning the read-only data at all?

[dvyukov, 2014-07-21 18:28:13]

On 2014/07/21 18:18:08, rsc wrote:
> Also, why is the new GC scanning the read-only data at all?

In some cases this function emits mutable DATA that does not contain pointers
(e.g. .gobytes). But that symbols is not marked as NOPTR. In the result
*current* GC scans that.

[dvyukov, 2014-07-21 19:01:36]

On 2014/07/21 18:28:13, dvyukov wrote:
> On 2014/07/21 18:18:08, rsc wrote:
> > Also, why is the new GC scanning the read-only data at all?
> 
> In some cases this function emits mutable DATA that does not contain pointers
> (e.g. .gobytes). But that symbols is not marked as NOPTR. In the result
> *current* GC scans that.

In the new GC I need to emit another symbol that is *not* RODATA, but does not
contain pointers.

[khr1, 2014-07-21 19:35:22]

We already have NOPTRDATA and NOPTRBSS symbols (defined by the linker).



On Mon, Jul 21, 2014 at 12:01 PM, <dvyukov@google.com> wrote:

> On 2014/07/21 18:28:13, dvyukov wrote:
>
>> On 2014/07/21 18:18:08, rsc wrote:
>> > Also, why is the new GC scanning the read-only data at all?
>>
>
>  In some cases this function emits mutable DATA that does not contain
>>
> pointers
>
>> (e.g. .gobytes). But that symbols is not marked as NOPTR. In the
>>
> result
>
>> *current* GC scans that.
>>
>
> In the new GC I need to emit another symbol that is *not* RODATA, but
> does not contain pointers.
>
>
> https://codereview.appspot.com/116060043/
>

[dvyukov, 2014-07-21 19:37:22]

Yes, that's exactly where I am moving these symbols to.

On Mon, Jul 21, 2014 at 11:35 PM, Keith Randall <khr@google.com> wrote:
> We already have NOPTRDATA and NOPTRBSS symbols (defined by the linker).
>
>
>
> On Mon, Jul 21, 2014 at 12:01 PM, <dvyukov@google.com> wrote:
>>
>> On 2014/07/21 18:28:13, dvyukov wrote:
>>>
>>> On 2014/07/21 18:18:08, rsc wrote:
>>> > Also, why is the new GC scanning the read-only data at all?
>>
>>
>>> In some cases this function emits mutable DATA that does not contain
>>
>> pointers
>>>
>>> (e.g. .gobytes). But that symbols is not marked as NOPTR. In the
>>
>> result
>>>
>>> *current* GC scans that.
>>
>>
>> In the new GC I need to emit another symbol that is *not* RODATA, but
>> does not contain pointers.
>>
>>
>> https://codereview.appspot.com/116060043/
>
>

[rsc, 2014-07-22 03:42:17]

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#newcod...
src/cmd/5g/gsubr.c:234: if(!hasptr)
now that the flags have names, just make ggloblsym take the flag list.

[dvyukov, 2014-07-22 07:43:48]

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#newcod...
src/cmd/5g/gsubr.c:234: if(!hasptr)
On 2014/07/22 03:42:17, rsc wrote:
> now that the flags have names, just make ggloblsym take the flag list.

do you mean to take "int flags" and callers will pass DUPOK|NOPTR?

[dvyukov, 2014-07-22 12:54:50]

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#newcod...
> src/cmd/5g/gsubr.c:234: if(!hasptr)
> On 2014/07/22 03:42:17, rsc wrote:
> > now that the flags have names, just make ggloblsym take the flag list.
> 
> do you mean to take "int flags" and callers will pass DUPOK|NOPTR?

I can just set NOPTR for all symbols, currently that's what we want. I wanted to
be more explicit. But up to you.

[rsc, 2014-07-22 12:59:54]

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

> do you mean to take "int flags" and callers will pass DUPOK|NOPTR?
>

yes, and RODATA when appropriate.

russ

[dvyukov, 2014-07-22 13:24:05]

On 2014/07/22 12:59:54, rsc wrote:
> On Tue, Jul 22, 2014 at 3:43 AM, <mailto:dvyukov@google.com> wrote:
> 
> > do you mean to take "int flags" and callers will pass DUPOK|NOPTR?
> >
> 
> yes, and RODATA when appropriate.
> 
> russ

Done
PTAL

[rsc, 2014-07-23 12:40:48]

LGTM

much nicer

[dvyukov, 2014-07-23 13:36:23]

*** 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