Contribution of PowerPC port (continuation of 422063005) - AIX Common1

Contribution of PowerPC port (continuation of 422063005) - AIX Common1

Contribution of PowerPC port (continuation of 422063005 and 817143002). This patch covers
the key changes needed to the common files needed to support AIX. Subsequent
patches will cover:
- changes to update the ppc directories so they are current with the changes
in the rest of the project.
- remaining AIX changes not resolved by 4.8 compiler
- individual optimizations for PPC

This is based off of the GitHub repository
https://github.com/andrewlow/v8ppc

R=danno@chromium.org, svenpanne@chromium.org
BUG=

Committed: https://crrev.com/f1ba8d8f86ad1a5c8b4777f969506c88272549c3
Cr-Commit-Position: refs/heads/master@{#26343}

[Sven Panne, 2015-01-27 11:47:06]

[ Sorry for the delay, this CL slipped between the cracks somehow... ]

First round of comments. Furthermore, you need to rebase, the CL doesn't merge
cleanly.

https://codereview.chromium.org/866843003/diff/1/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/866843003/diff/1/build/standalone.gypi#newcod...
build/standalone.gypi:154: 'visibility%': '',
What's the reason for this special case?

https://codereview.chromium.org/866843003/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/assembler.cc#newcode1461
src/assembler.cc:1461: #elif V8_OS_AIX
Can we merge this with the case above? If I see this correctly, only the
copysign is new, and this should be OK above, too (I hope :-).

https://codereview.chromium.org/866843003/diff/1/src/base/atomicops.h
File src/base/atomicops.h (right):

https://codereview.chromium.org/866843003/diff/1/src/base/atomicops.h#newcode60
src/base/atomicops.h:60: typedef Atomic32 AtomicWord;
Hmmm, why do we need this?

https://codereview.chromium.org/866843003/diff/1/src/base/logging.h
File src/base/logging.h (right):

https://codereview.chromium.org/866843003/diff/1/src/base/logging.h#newcode80
src/base/logging.h:80: #if V8_OS_AIX && V8_HOST_ARCH_32_BIT
Can we turn this into something less fragile (e.g. compile-time tests regarding
int sizes)? Why do we need this at all? Is intptr_t different from int *and*
int64_t? If yes, what is it?

As a side note, we should really get rid of this CheckEqualsHelper nonsense, we
tripped over this several times already.

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
File src/base/platform/platform-aix.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
src/base/platform/platform-aix.cc:46: // converted to double precision numbers.
Huh? Where do we convert addresses to doubles?

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
src/base/platform/platform-aix.cc:164: int rc = read(fd, addr_buffer + 2, 8);
Does "read" really return an int on AIX? I can't find any online manual pages
for the AIX system calls (or the POSIX emulation).

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-p...
File src/base/platform/platform-posix.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-p...
src/base/platform/platform-posix.cc:178: //   b) avoid losing precision if
address is stored as a double.
Same question again: Where do we store addresses as doubles?

https://codereview.chromium.org/866843003/diff/1/src/codegen.cc
File src/codegen.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/codegen.cc#newcode57
src/codegen.cc:57: double result;
Don't use a declaration without initialization here and below, we're in C++
land.

https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.cc
File src/compiler/graph-builder.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.c...
src/compiler/graph-builder.cc:5: #if defined(_AIX)
Use V8_OS_AIX

https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.c...
src/compiler/graph-builder.cc:6: #include <alloca.h>
Why do we need this? I don't see any alloca call here.

https://codereview.chromium.org/866843003/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/d8.cc#newcode66
src/d8.cc:66: #define malloc __linux_malloc
Why do we need this? A comment would be nice.

https://codereview.chromium.org/866843003/diff/1/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/disassembler.cc#newcode132
src/disassembler.cc:132: #else
Could you restructure these #if/#elif/#else/#endifs a bit, even if a few lines
might be duplicated? It's not readable as it is, having unbalanced things in
preprocessor directives is in general not a good idea.

https://codereview.chromium.org/866843003/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/866843003/diff/1/src/globals.h#newcode42
src/globals.h:42: #undef V8_INFINITY
No #undef, please, move this up to the place where V8_INFINITY is #defined.

https://codereview.chromium.org/866843003/diff/1/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/sampler.cc#newcode16
src/sampler.cc:16: #if !(V8_OS_QNX || V8_OS_AIX) && !V8_OS_NACL
De Morgan's law, please. :-)

https://codereview.chromium.org/866843003/diff/1/src/sampler.cc#newcode315
src/sampler.cc:315: // Ensure delivery of any pending SIGPROF before removing
the handler
Hmmm, why is this AIX-specific? Why can't there be yet another SIGPROF after the
handler and coming back to the yield?

In a nutshell: This looks wrong, like 99.9% of all yield uses I've seen so far.
:-) If it's really OK, we need an extensive comment here.

https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:251: 'asm/embenchen/*': [SKIP],
Do really all 9 embenchen tests fail on big-endian?

[michael_dawson, 2015-01-29 00:08:29]

Have replied to each of the individual comments and uploaded patch that
addresses comments where appropriate

https://codereview.chromium.org/866843003/diff/1/build/standalone.gypi
File build/standalone.gypi (right):

https://codereview.chromium.org/866843003/diff/1/build/standalone.gypi#newcod...
build/standalone.gypi:154: 'visibility%': '',
On 2015/01/27 11:47:05, Sven Panne wrote:
> What's the reason for this special case?

It seems to have been added when we added the -gxcoff flag for AIX, but I don't
see why it is necessary.  I removed and re-ran tests on AIX and it did not
change the results so I'll remove.

https://codereview.chromium.org/866843003/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/assembler.cc#newcode1461
src/assembler.cc:1461: #elif V8_OS_AIX
On 2015/01/27 11:47:05, Sven Panne wrote:
> Can we merge this with the case above? If I see this correctly, only the
> copysign is new, and this should be OK above, too (I hope :-).

It looks like there are a few other differences in that in the AIX case it
always returns while in the MinGW case it may not.  I created a merged version
which I think preserves the behaviour of both.  I validated the AIX case but we
compile with MSVC so I don't have a MinGW environment to validate that case.

https://codereview.chromium.org/866843003/diff/1/src/base/atomicops.h
File src/base/atomicops.h (right):

https://codereview.chromium.org/866843003/diff/1/src/base/atomicops.h#newcode60
src/base/atomicops.h:60: typedef Atomic32 AtomicWord;
On 2015/01/27 11:47:05, Sven Panne wrote:
> Hmmm, why do we need this?

It looks like this was to deal with the case mentioned in
src/base/atomicops_internals_atomicword_compat.h. The description in that file
is:

// AtomicWord is a synonym for intptr_t, and Atomic32 is a synonym for int32,
// which in turn means int. On some LP32 platforms, intptr_t is an int, but
// on others, it's a long. When AtomicWord and Atomic32 are based on different
// fundamental types, their pointers are incompatible.
//
// This file defines function overloads to allow both AtomicWord and Atomic32
// data to be used with this interface.


Given that there is an existing solution in
src/base/atomicops_internals_atomicword_compat.h I'll enable that for AIX and
remove the specialization of the typedef for AIX.

https://codereview.chromium.org/866843003/diff/1/src/base/logging.h
File src/base/logging.h (right):

https://codereview.chromium.org/866843003/diff/1/src/base/logging.h#newcode80
src/base/logging.h:80: #if V8_OS_AIX && V8_HOST_ARCH_32_BIT
On 2015/01/27 11:47:05, Sven Panne wrote:
> Can we turn this into something less fragile (e.g. compile-time tests
regarding
> int sizes)? Why do we need this at all? Is intptr_t different from int *and*
> int64_t? If yes, what is it?
> 
> As a side note, we should really get rid of this CheckEqualsHelper nonsense,
we
> tripped over this several times already.

On AIX 32 bit:
 intptr_t - defined as long
 int64_t is not the same as long

On AIX 64 bit:
 intptr_t - defined as long
 int64_t - defined as long

I read this description:
  int64_t is guaranteed by the C99 standard to be exactly 64 
  long which is at least 32

So in the case of AIX 32 we are missing the case for intptr_t which is not an
int or an int64_t

I'm probably missing the point but, I'm not sure how a compile-time check would
work as we the type used by the caller needs to match the type in the function
definition and I assume we don't want to change the callers.  

I tried defining a new helper with type long, which seemed to work for AIX 32
(since intptr_t was a long), but on 64 bit the compile failed because the helper
with using the int64_t conflicted.

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
File src/base/platform/platform-aix.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
src/base/platform/platform-aix.cc:46: // converted to double precision numbers.
On 2015/01/27 11:47:05, Sven Panne wrote:
> Huh? Where do we convert addresses to doubles?

The conversion to doubles happens in the test
cctest/test-log/EquivalenceOfLoggingAndTraversal.  It writes out a set of
addresses to a file and then later reads them in using javascript where they are
stored as doubles. The large address space on PPC64 means that there can be
addresses that get rounded due because they are stored as doubles which messes
up the table built by the test causing the test to fail/timeout.  This was an
attempt to use addresses that would be low enough to avoid this problem.  Having
discussed this with the team we'll remove as the right answer is more likely to
fix the test or exclude the test than to try and force the addresses we use.

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
src/base/platform/platform-aix.cc:164: int rc = read(fd, addr_buffer + 2, 8);
On 2015/01/27 11:47:05, Sven Panne wrote:
> Does "read" really return an int on AIX? I can't find any online manual pages
> for the AIX system calls (or the POSIX emulation).

You're right it does not return an int as per
http://www-01.ibm.com/support/knowledgecenter/ssw_aix_61/com.ibm.aix.basetrf2...
will fix

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-p...
File src/base/platform/platform-posix.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-p...
src/base/platform/platform-posix.cc:178: //   b) avoid losing precision if
address is stored as a double.
On 2015/01/27 11:47:05, Sven Panne wrote:
> Same question again: Where do we store addresses as doubles?

See explanation on similar comment on src/base/platform/platform-aix.cc.  In
this case we still want to limit the address range for a) so I'll just remove
comment b)

https://codereview.chromium.org/866843003/diff/1/src/codegen.cc
File src/codegen.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/codegen.cc#newcode57
src/codegen.cc:57: double result;
On 2015/01/27 11:47:05, Sven Panne wrote:
> Don't use a declaration without initialization here and below, we're in C++
> land.

Will do

https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.cc
File src/compiler/graph-builder.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.c...
src/compiler/graph-builder.cc:5: #if defined(_AIX)
On 2015/01/27 11:47:06, Sven Panne wrote:
> Use V8_OS_AIX

Will do

https://codereview.chromium.org/866843003/diff/1/src/compiler/graph-builder.c...
src/compiler/graph-builder.cc:6: #include <alloca.h>
On 2015/01/27 11:47:06, Sven Panne wrote:
> Why do we need this? I don't see any alloca call here.

Looks like it was added in one merge, but failed to get removed in a later one. 
Will remove.

https://codereview.chromium.org/866843003/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/d8.cc#newcode66
src/d8.cc:66: #define malloc __linux_malloc
On 2015/01/27 11:47:06, Sven Panne wrote:
> Why do we need this? A comment would be nice.

This is the comment I'll add explaining:
// On AIX if you call malloc with a size of 0 malloc fails.  On linux it
// allocates a minimum amount.  This seems to be used for things like
// finding out where the heap is located.  Using __linux_malloc makes
// the behaviour when running on AIX compatible with what is seen on linux

https://codereview.chromium.org/866843003/diff/1/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/disassembler.cc#newcode132
src/disassembler.cc:132: #else
On 2015/01/27 11:47:06, Sven Panne wrote:
> Could you restructure these #if/#elif/#else/#endifs a bit, even if a few lines
> might be duplicated? It's not readable as it is, having unbalanced things in
> preprocessor directives is in general not a good idea.

Will do

https://codereview.chromium.org/866843003/diff/1/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/866843003/diff/1/src/globals.h#newcode42
src/globals.h:42: #undef V8_INFINITY
On 2015/01/27 11:47:06, Sven Panne wrote:
> No #undef, please, move this up to the place where V8_INFINITY is #defined.

Will do

https://codereview.chromium.org/866843003/diff/1/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/sampler.cc#newcode16
src/sampler.cc:16: #if !(V8_OS_QNX || V8_OS_AIX) && !V8_OS_NACL
On 2015/01/27 11:47:06, Sven Panne wrote:
> De Morgan's law, please. :-)

Will do

https://codereview.chromium.org/866843003/diff/1/src/sampler.cc#newcode315
src/sampler.cc:315: // Ensure delivery of any pending SIGPROF before removing
the handler
On 2015/01/27 11:47:06, Sven Panne wrote:
> Hmmm, why is this AIX-specific? Why can't there be yet another SIGPROF after
the
> handler and coming back to the yield?
> 
> In a nutshell: This looks wrong, like 99.9% of all yield uses I've seen so
far.
> :-) If it's really OK, we need an extensive comment here.


We went to validate our explanation of why it was needed, but we can no longer
recreate the issue with the current level so we will remove

https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:251: 'asm/embenchen/*': [SKIP],
On 2015/01/27 11:47:06, Sven Panne wrote:
> Do really all 9 embenchen tests fail on big-endian?

All 9 tests specify that they assume LE
$ grep -i endian test/mjsunit/asm/embenchen/*
test/mjsunit/asm/embenchen/box2d.js:// Endianness check (note: assumes compiler
arch was little-endian)
test/mjsunit/asm/embenchen/box2d.js:assert(HEAPU8[0] === 255 && HEAPU8[3] === 0,
'Typed arrays 2 must be run on a little-endian system');                        
                                                                                
                       
test/mjsunit/asm/embenchen/copy.js:// Endianness check (note: assumes compiler
arch was little-endian)
test/mjsunit/asm/embenchen/copy.js:assert(HEAPU8[0] === 255 && HEAPU8[3] === 0,
'Typed arrays 2 must be run on a little-endian system');                        
                                                                                
                        
test/mjsunit/asm/embenchen/corrections.js:// Endianness check (note: assumes
compiler arch was little-endian)
test/mjsunit/asm/embenchen/corrections.js:assert(HEAPU8[0] === 255 && HEAPU8[3]
=== 0, 'Typed arrays 2 must be run on a little-endian system');                 
                                                                                
                        
test/mjsunit/asm/embenchen/fannkuch.js:// Endianness check (note: assumes
compiler arch was little-endian)
test/mjsunit/asm/embenchen/fannkuch.js:assert(HEAPU8[0] === 255 && HEAPU8[3] ===
0, 'Typed arrays 2 must be run on a little-endian system');                     
                                                                                
                       
test/mjsunit/asm/embenchen/fasta.js:// Endianness check (note: assumes compiler
arch was little-endian)
test/mjsunit/asm/embenchen/fasta.js:assert(HEAPU8[0] === 255 && HEAPU8[3] === 0,
'Typed arrays 2 must be run on a little-endian system');                        
                                                                                
                       
test/mjsunit/asm/embenchen/lua_binarytrees.js:// Endianness check (note: assumes
compiler arch was little-endian)
test/mjsunit/asm/embenchen/lua_binarytrees.js:assert(HEAPU8[0] === 255 &&
HEAPU8[3] === 0, 'Typed arrays 2 must be run on a little-endian system');       
                                                                                
                              
test/mjsunit/asm/embenchen/memops.js:// Endianness check (note: assumes compiler
arch was little-endian)
test/mjsunit/asm/embenchen/memops.js:assert(HEAPU8[0] === 255 && HEAPU8[3] ===
0, 'Typed arrays 2 must be run on a little-endian system');                     
                                                                                
                         
test/mjsunit/asm/embenchen/primes.js:// Endianness check (note: assumes compiler
arch was little-endian)
test/mjsunit/asm/embenchen/primes.js:assert(HEAPU8[0] === 255 && HEAPU8[3] ===
0, 'Typed arrays 2 must be run on a little-endian system');                     
                                                                                
                         
test/mjsunit/asm/embenchen/zlib.js:// Endianness check (note: assumes compiler
arch was little-endian)
test/mjsunit/asm/embenchen/zlib.js:assert(HEAPU8[0] === 255 && HEAPU8[3] === 0,
'Typed arrays 2 must be run on a little-endian system');             


If I remove the exclude the test report 16 failures, I think two failures for
each failing test.

[Sven Panne, 2015-01-29 09:54:33]

Almost there... (Don't forget to rebase before upload, we're changing quite
fast.)

https://codereview.chromium.org/866843003/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/assembler.cc#newcode1461
src/assembler.cc:1461: #elif V8_OS_AIX
We don't test MINGW builds, either, it was just part of an external contribution
IIRC. Just use your AIX code for the MINGW stuff above, too, and let's see if
somebody complains. >:-) Having weird conditional untested code around is
definitely worse.

https://codereview.chromium.org/866843003/diff/1/src/base/logging.h
File src/base/logging.h (right):

https://codereview.chromium.org/866843003/diff/1/src/base/logging.h#newcode80
src/base/logging.h:80: #if V8_OS_AIX && V8_HOST_ARCH_32_BIT
The whole Check(Non)EqualsHelper approach in v8 is fundamentally broken: The
expected value and the actual value have to be of the same type, and this gives
endless headaches with C++'s integral promotion rules when you mix different
sizes/signedness. We should really just use Chrome's approach where this is
handled via templates with 2 type arguments, there is no other sane way.
Benedikt just told me that he wants to give it try, but this shouldn't block
this CL: I propose to leave your code here as it is in the CL, we can clean this
up later when we have a sane framework.

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
File src/base/platform/platform-aix.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
src/base/platform/platform-aix.cc:46: // converted to double precision numbers.
On 2015/01/29 00:08:29, michael_dawson wrote:
> [...] Having discussed this with the team we'll remove as the right answer is
more likely to
> fix the test or exclude the test than to try and force the addresses we use.  

Exactly, this seems to be the right approach. Let's fix the root cause and don't
silence the symptoms in an ugly way. :-)

You can just blacklist the test on AIX for now and open a v8 issue for it. Then
I can try to find somebody who understands the test and can fix it, I don't even
have the slightest clue what EquivalenceOfLoggingAndTraversal is trying to test
and what it is doing in detail.

https://codereview.chromium.org/866843003/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/d8.cc#newcode66
src/d8.cc:66: #define malloc __linux_malloc
Hmmm, if I see this correctly, malloc(0) only happens via the
MockArrayBufferAllocator. I would prefer to leave out this #ifdef and change
MockArrayBufferAllocator to allocate a single byte instead. This should
hopefully work, if not, we learn something. :-)

As a side note, this would probably be more correct, anyway: POSIX explicitly
states that malloc(0) has implementation-defined behavior (either returning NULL
or a unique pointer).

https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:251: 'asm/embenchen/*': [SKIP],
*sigh* OK, just leave the SKIP here, Wintel legacy strikes again...

[michael_dawson, 2015-01-29 18:20:22]

On 2015/01/29 09:54:33, Sven Panne wrote:
> Almost there... (Don't forget to rebase before upload, we're changing quite
> fast.)
> 
> https://codereview.chromium.org/866843003/diff/1/src/assembler.cc
> File src/assembler.cc (right):
> 
> https://codereview.chromium.org/866843003/diff/1/src/assembler.cc#newcode1461
> src/assembler.cc:1461: #elif V8_OS_AIX
> We don't test MINGW builds, either, it was just part of an external
contribution
> IIRC. Just use your AIX code for the MINGW stuff above, too, and let's see if
> somebody complains. >:-) Having weird conditional untested code around is
> definitely worse.
> 
> https://codereview.chromium.org/866843003/diff/1/src/base/logging.h
> File src/base/logging.h (right):
> 
> https://codereview.chromium.org/866843003/diff/1/src/base/logging.h#newcode80
> src/base/logging.h:80: #if V8_OS_AIX && V8_HOST_ARCH_32_BIT
> The whole Check(Non)EqualsHelper approach in v8 is fundamentally broken: The
> expected value and the actual value have to be of the same type, and this
gives
> endless headaches with C++'s integral promotion rules when you mix different
> sizes/signedness. We should really just use Chrome's approach where this is
> handled via templates with 2 type arguments, there is no other sane way.
> Benedikt just told me that he wants to give it try, but this shouldn't block
> this CL: I propose to leave your code here as it is in the CL, we can clean
this
> up later when we have a sane framework.
> 
>
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
> File src/base/platform/platform-aix.cc (right):
> 
>
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
> src/base/platform/platform-aix.cc:46: // converted to double precision
numbers.
> On 2015/01/29 00:08:29, michael_dawson wrote:
> > [...] Having discussed this with the team we'll remove as the right answer
is
> more likely to
> > fix the test or exclude the test than to try and force the addresses we use.
 
> 
> Exactly, this seems to be the right approach. Let's fix the root cause and
don't
> silence the symptoms in an ugly way. :-)
> 
> You can just blacklist the test on AIX for now and open a v8 issue for it.
Then
> I can try to find somebody who understands the test and can fix it, I don't
even
> have the slightest clue what EquivalenceOfLoggingAndTraversal is trying to
test
> and what it is doing in detail.
> 
> https://codereview.chromium.org/866843003/diff/1/src/d8.cc
> File src/d8.cc (right):
> 
> https://codereview.chromium.org/866843003/diff/1/src/d8.cc#newcode66
> src/d8.cc:66: #define malloc __linux_malloc
> Hmmm, if I see this correctly, malloc(0) only happens via the
> MockArrayBufferAllocator. I would prefer to leave out this #ifdef and change
> MockArrayBufferAllocator to allocate a single byte instead. This should
> hopefully work, if not, we learn something. :-)
> 
> As a side note, this would probably be more correct, anyway: POSIX explicitly
> states that malloc(0) has implementation-defined behavior (either returning
NULL
> or a unique pointer).
> 
> https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status
> File test/mjsunit/mjsunit.status (right):
> 
>
https://codereview.chromium.org/866843003/diff/1/test/mjsunit/mjsunit.status#...
> test/mjsunit/mjsunit.status:251: 'asm/embenchen/*': [SKIP],
> *sigh* OK, just leave the SKIP here, Wintel legacy strikes again...

In terms of re-basing, I've been addressing the comments, re-basing, running
make quickcheck and then uploading.  On my current machine quickcheck is a 5-6
hour run so the re-base will always be a number of hours old.

[michael_dawson, 2015-01-29 18:21:59]

In terms of re-basing, I've been addressing the comments, re-basing, running
make quickcheck and then uploading.  On my current machine quickcheck is a 5-6
hour run so the re-base will always be a number of hours old. I'm just running
that now on the patch to address the latest comments

https://codereview.chromium.org/866843003/diff/1/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/assembler.cc#newcode1461
src/assembler.cc:1461: #elif V8_OS_AIX
On 2015/01/29 09:54:33, Sven Panne wrote:
> We don't test MINGW builds, either, it was just part of an external
contribution
> IIRC. Just use your AIX code for the MINGW stuff above, too, and let's see if
> somebody complains. >:-) Having weird conditional untested code around is
> definitely worse.

ok will do

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
File src/base/platform/platform-aix.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-a...
src/base/platform/platform-aix.cc:46: // converted to double precision numbers.
On 2015/01/29 09:54:33, Sven Panne wrote:
> On 2015/01/29 00:08:29, michael_dawson wrote:
> > [...] Having discussed this with the team we'll remove as the right answer
is
> more likely to
> > fix the test or exclude the test than to try and force the addresses we use.
 
> 
> Exactly, this seems to be the right approach. Let's fix the root cause and
don't
> silence the symptoms in an ugly way. :-)
> 
> You can just blacklist the test on AIX for now and open a v8 issue for it.
Then
> I can try to find somebody who understands the test and can fix it, I don't
even
> have the slightest clue what EquivalenceOfLoggingAndTraversal is trying to
test
> and what it is doing in detail.

I found an existing
issue(2857-https://code.google.com/p/v8/issues/detail?can=1&start=0&num=100&q=EquivalenceOfLoggingAndTraversal&colspec=ID%20Type%20Status%20Priority%20Owner%20Summary%20HW%20OS%20Area%20Stars&groupby=&sort=&id=2857)
and added a comment to that issue.  I will exclude the test for PPC 64
referencing that issue

https://codereview.chromium.org/866843003/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/866843003/diff/1/src/d8.cc#newcode66
src/d8.cc:66: #define malloc __linux_malloc
On 2015/01/29 09:54:33, Sven Panne wrote:
> Hmmm, if I see this correctly, malloc(0) only happens via the
> MockArrayBufferAllocator. I would prefer to leave out this #ifdef and change
> MockArrayBufferAllocator to allocate a single byte instead. This should
> hopefully work, if not, we learn something. :-)
> 
> As a side note, this would probably be more correct, anyway: POSIX explicitly
> states that malloc(0) has implementation-defined behavior (either returning
NULL
> or a unique pointer).

Will do

[michael_dawson, 2015-01-29 18:21:59]

In terms of re-basing, I've been addressing the comments, re-basing, running
make quickcheck and then uploading.  On my current machine quickcheck is a 5-6
hour run so the re-base will always be a number of hours old. I'm just running
that now on the patch to address the latest comments

[michael_dawson, 2015-01-29 22:13:02]

On 2015/01/29 18:21:59, michael_dawson wrote:
> In terms of re-basing, I've been addressing the comments, re-basing, running
> make quickcheck and then uploading.  On my current machine quickcheck is a 5-6
> hour run so the re-base will always be a number of hours old. I'm just running
> that now on the patch to address the latest comments

Patch uploaded

[Sven Panne, 2015-01-30 07:31:48]

The CQ bit was checked by svenpanne@chromium.org

[Sven Panne, 2015-01-30 07:31:49]

lgtm

[commit-bot: I haz the power, 2015-01-30 07:31:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/866843003/40001

[commit-bot: I haz the power, 2015-01-30 08:02:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-30 08:02:20]

Patchset 3 (id:??) landed as
https://crrev.com/f1ba8d8f86ad1a5c8b4777f969506c88272549c3
Cr-Commit-Position: refs/heads/master@{#26343}

[Yang, 2015-02-06 07:08:14]

yangguo@chromium.org changed reviewers:
+ yangguo@chromium.org

[Yang, 2015-02-06 07:08:15]

https://codereview.chromium.org/866843003/diff/40001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/866843003/diff/40001/src/serialize.cc#newcode912
src/serialize.cc:912: if (rmode == RelocInfo::INTERNAL_REFERENCE) {
I'm quite puzzled by this. It is true that V8 currently does not use
INTERNAL_REFERENCE. This however may change soon if we decide to implement jump
tables. I'm currently looking at ways to implement serialization in a
generalized way.

I think what you are assuming here is that every INTERNAL_REFERENCE in PPC code
is a function descriptor (if I understood correctly, a pointer to self), so you
simply go through all internal references and replace them by the updated self
pointer.

This would not work for arbitrary internal references, and I think this should
be solved differently.

Furthermore, adding this piece of platform-dependent code here is bad style.

[michael_dawson, 2015-02-06 23:06:35]

https://codereview.chromium.org/866843003/diff/40001/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/866843003/diff/40001/src/serialize.cc#newcode912
src/serialize.cc:912: if (rmode == RelocInfo::INTERNAL_REFERENCE) {
On 2015/02/06 07:08:14, Yang wrote:
> I'm quite puzzled by this. It is true that V8 currently does not use
> INTERNAL_REFERENCE. This however may change soon if we decide to implement
jump
> tables. I'm currently looking at ways to implement serialization in a
> generalized way.
> 
> I think what you are assuming here is that every INTERNAL_REFERENCE in PPC
code
> is a function descriptor (if I understood correctly, a pointer to self), so
you
> simply go through all internal references and replace them by the updated self
> pointer.
> 
> This would not work for arbitrary internal references, and I think this should
> be solved differently.
> 
> Furthermore, adding this piece of platform-dependent code here is bad style.

We are not surprised that there are some comments on this change.  If there is a
general
solution for INTERNAL_REFERENCE type (which it sounds like you are working on)
we expect 
that our code would be subsumed by the general case and our platform specific
code would be removed.  
If there are not already plans to have a general solution we could work to
generalize what we
have or move it to the PPC specific directories but we'd need some additional 
guidance/suggestions before starting on that.