Reviewers: danno, Sven Panne,
Message:
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#newcode154
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-aix.cc
File src/base/platform/platform-aix.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-aix.cc#newcode46
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-aix.cc#newcode164
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/read.htm
will fix
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-posix.cc
File src/base/platform/platform-posix.cc (right):
https://codereview.chromium.org/866843003/diff/1/src/base/platform/platform-posix.cc#newcode178
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.cc#newcode5
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.cc#newcode6
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#newcode251
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.
Description:
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
[email protected], [email protected]
BUG=
Please review this at https://codereview.chromium.org/866843003/
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Affected files (+294, -112 lines):
M build/standalone.gypi
M build/toolchain.gypi
M include/v8config.h
M src/assembler.cc
M src/base/atomicops.h
M src/base/cpu.cc
M src/base/logging.h
M src/base/macros.h
A + src/base/platform/platform-aix.cc
M src/base/platform/platform-posix.cc
M src/base/sys-info.cc
M src/code-stubs.h
M src/codegen.cc
M src/d8.cc
M src/d8.gyp
M src/debug.cc
M src/disassembler.cc
M src/globals.h
M src/sampler.cc
M src/serialize.cc
M src/utils.cc
M test/cctest/test-sampler-api.cc
M test/mjsunit/d8-os.js
M test/mjsunit/mjsunit.status
M tools/gyp/v8.gyp
M tools/run-deopt-fuzzer.py
M tools/run-tests.py
M tools/testrunner/local/statusfile.py
M tools/testrunner/local/utils.py
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
