code review 7307127: runtime: Support Solaris AMD64 in lfstack. (issue 7307127)

[sn...@epipe.com]

Reviewers: gofrontend-dev_googlegroups.com,

Message:
Hello gofront...@googlegroups.com (cc:
gofront...@googlegroups.com),

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


Description:
runtime: Support Solaris AMD64 in lfstack.
The address space layout is similar on SPARC64 and AMD64 when
running Solaris. Thus check for "__sun__" instead of "__sparc__".

Please review this at https://codereview.appspot.com/7307127/

Affected files:
M libgo/runtime/lfstack.c


Index: libgo/runtime/lfstack.c
===================================================================
--- a/libgo/runtime/lfstack.c
+++ b/libgo/runtime/lfstack.c
@@ -17,9 +17,10 @@
#define PTR_MASK ((1ull<<PTR_BITS)-1)
#define CNT_MASK (0ull-1)

-#if __SIZEOF_POINTER__ == 8 && defined(__sparc__)
-// SPARC64 uses all 64 bits of virtual addresses. Use low-order three
-// bits as ABA counter.
+#if __SIZEOF_POINTER__ == 8 && defined(__sun__)
+// Solaris on SPARC64 and AMD64 uses all 64 bits of virtual addresses.
+// Use low-order three bits as ABA counter.
+//
http://docs.oracle.com/cd/E19120-01/open.solaris/816-5138/6mba6ua5p/index.html
#undef PTR_BITS
#undef CNT_MASK
#undef PTR_MASK

[ia...@golang.org]

https://codereview.appspot.com/7307127/diff/5001/libgo/runtime/lfstack.c
File libgo/runtime/lfstack.c (right):

https://codereview.appspot.com/7307127/diff/5001/libgo/runtime/lfstack.c#newcode20
libgo/runtime/lfstack.c:20: #if __SIZEOF_POINTER__ == 8 &&
defined(__sun__)
Don't we want to this for all SPARC systems, even those that are not
Solaris? E.g., SPARC GNU/Linux?

https://codereview.appspot.com/7307127/

[sn...@epipe.com]

https://codereview.appspot.com/7307127/diff/5001/libgo/runtime/lfstack.c
File libgo/runtime/lfstack.c (right):

https://codereview.appspot.com/7307127/diff/5001/libgo/runtime/lfstack.c#newcode20
libgo/runtime/lfstack.c:20: #if __SIZEOF_POINTER__ == 8 &&
defined(__sun__)

On 2013/02/20 03:48:04, iant wrote:
> Don't we want to this for all SPARC systems, even those that are not
Solaris?
> E.g., SPARC GNU/Linux?

Good question but unfortunately I do not know. I only know that we need
this on Solaris AMD64 architecture. I do not have access to any SPARC
machine and my searches for "netbsd sparc virtual memory layout" and
likewise for Linux turned up nothing.

I was thinking that the virtual memory layout depends more on the OS
than the CPU architecture but of course it can depend on both. I was
assuming that the previous commit that added this #ifdef was made with
only Solaris in mind. I might be mistaken.

Would it be better to change it to the following? Then it does not
change any existing behavior on any other platforms than Solaris AMD64.

#if __SIZEOF_POINTER__ == 8 && (defined(__sparc__) || (defined(__sun__)
&& defined(__amd64__)))

https://codereview.appspot.com/7307127/

[Ian Lance Taylor]

On Tue, Feb 19, 2013 at 8:27 PM, <sn...@epipe.com> wrote:
>
> Would it be better to change it to the following? Then it does not
> change any existing behavior on any other platforms than Solaris AMD64.
>
> #if __SIZEOF_POINTER__ == 8 && (defined(__sparc__) || (defined(__sun__)
> && defined(__amd64__)))

I think that is more likely to be correct.

Ian

[sn...@epipe.com]

Hello gofront...@googlegroups.com, ia...@golang.org (cc:
gofront...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/7307127/

[ia...@golang.org]

LGTM

Thanks.


https://codereview.appspot.com/7307127/

[ia...@golang.org]

*** Submitted as
https://code.google.com/p/gofrontend/source/detail?r=3b1c3cceaf02 ***

runtime: Support Solaris AMD64 in lfstack.
The address space layout is similar on SPARC64 and AMD64 when
running Solaris.

R=gofrontend-dev, iant
CC=gofrontend-dev
https://codereview.appspot.com/7307127

Committer: Ian Lance Taylor <ia...@golang.org>


https://codereview.appspot.com/7307127/