Contribution of PowerPC port.

Contribution of PowerPC port.

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

Build results can be seen here
http://v8ppc.osuosl.org:8080/view/V8/

BUG=

[andrew_low, 2014-07-28 20:18:22]

I'm not entirely sure that the patch set is complete, I got this error at the
end

"Got exception while uploading -- saving description to
/home/alow/.git_cl_description_backup"

Tried simply doing a "$git cl upload" again and got the same error. 

The summary information about the patch appears to match my local changes.

[andrew_low, 2014-07-29 13:06:13]

This port support 32 and 64 bits. Big Endian (BE) and Little Endian (LE). There
is simulator support on Intel (Linux).

Proof points for "usefulness"

- A version of this code (3.14 level) is available in Ubuntu
https://launchpad.net/ubuntu/+source/libv8-3.14/3.14.5.8-5ubuntu2 supporting
PowerPC 64 (LE).

- IBM makes a developerWorks build of Node (includes V8) available here
http://www.ibm.com/developerworks/web/nodesdk/

- As pointed out initially, our master source repository is here on GitHub
https://github.com/andrewlow/v8ppc and is built regularly with results here:
http://v8ppc.osuosl.org:8080/view/V8/  [this is live dev/build results]

[danno, 2014-07-29 13:24:08]

Thanks for the patch. This clearly is a serious investment for PPC and is
definitely on the right path, we'll work with your to iron the remaining issues
and get this landed.

Two high-level observations:
- Wherever possible, please don't add "#ifdef V8_TARGET_ARCH_PPC" in shared
platform code. If there are additional abstractions that are needed, it would be
great to add those for all platforms before adding the PPC-specific bits. 
- There seem to be a couple of patches to the code that were useful in the PPC
context but not actually part of the PPC port (e.g. the new "trace_sim_stubs"
flag in flag-definitions). Can you please factor these out where possible into
separate CLs? The goal should be to land the port CL with only boilerplate
#ifdef changes in the shared code plus all the new platform-specific stuff
limited to the ppc and test directories. 

I will on vacation for ~4 weeks starting next week, so I am adding Sven Panne to
continue the review.

Also Ross should look at your constant pool changes.

https://codereview.chromium.org/422063005/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/422063005/diff/1/AUTHORS#newcode25
AUTHORS:25: Andrew Low <andrew_low@ca.ibm.com>
You and all the ibm contributors are covered under the corporate CLA and should
not be listed individually (we need to clean this up for the other corporate
entries in this file, but we shouldn't make it worse).

https://codereview.chromium.org/422063005/diff/1/AUTHORS#newcode33
AUTHORS:33: David Eelsohn <dje.gcc@gmail.com>
If David is doing this work for IBM at IBM, he is also covered under the
corporate CLA doesn't need to be listed (note that if he contributes code, he
will have to use his ibm address for that, tho). If he is contributing on his
own time, he needs to sign the ICLA.

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

https://codereview.chromium.org/422063005/diff/1/src/assembler.cc#newcode992
src/assembler.cc:992: #if V8_TARGET_ARCH_PPC64
Again, why the #ifdef? It seems like on most platforms, result_size will always
either 1, or if there are cases where there are already 2 results returned, then
BUILTIN_OBJECTPAIR_CALL is just an alias for BUILTIN_CALL on those platforms and
should be threaded through as such.

https://codereview.chromium.org/422063005/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/422063005/diff/1/src/assembler.h#newcode747
src/assembler.h:747: #if V8_TARGET_ARCH_PPC64
Why the #ifdef? This may not be implemented on all platforms, but it should be a
goal to make as few platform-specific #ifs as possible.

https://codereview.chromium.org/422063005/diff/1/src/base/atomicops_internals...
File src/base/atomicops_internals_ppc_gcc.h (right):

https://codereview.chromium.org/422063005/diff/1/src/base/atomicops_internals...
src/base/atomicops_internals_ppc_gcc.h:1: // Copyright 2010 the V8 project
authors. All rights reserved.
Please use the new compact header.

https://codereview.chromium.org/422063005/diff/1/src/base/cpu.cc
File src/base/cpu.cc (right):

https://codereview.chromium.org/422063005/diff/1/src/base/cpu.cc#newcode254
src/base/cpu.cc:254: cache_line_size_(0),
Seems like it makes sense to add a sentinel constant here, e.g.
UNKOWN_CACHE_LINE_SIZE = 0.

https://codereview.chromium.org/422063005/diff/1/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode1896
src/debug.cc:1896: if (FLAG_enable_ool_constant_pool) {
Is this a change that is unrelated to the port (seems it is), if so, can you
please factor out into a separate patch?

https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode2327
src/debug.cc:2327: Address addr = frame->pc() -
Assembler::kPatchDebugBreakSlotReturnOffset;
Could you please abstract this to Assembler? There probably should be new
method: Assembler::break_address_from_break_address which is implemented as
"frame->pc() - Assembler::kPatchDebugBreakSlotReturnOffset" on all platforms
except PPC. That would remove the #if def here.

https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode2419
src/debug.cc:2419: #if V8_TARGET_ARCH_PPC
See above

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

https://codereview.chromium.org/422063005/diff/1/src/disassembler.cc#newcode149
src/disassembler.cc:149: if (it != NULL && !it->done() && it->rinfo()->pc() ==
pc &&
Again, please try to avoid non-boilerplate platform #ifdefs in shared code
whenever possible.

It seems like it would be cleaner to make everything in this if() a call to a
help function that is present on all platforms that also returns "false" on non
PPC (and thus falls through to the decode_buffer[...] code), and actually does
this heavy lifting on PPC.

https://codereview.chromium.org/422063005/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/422063005/diff/1/src/flag-definitions.h#newco...
src/flag-definitions.h:575: DEFINE_BOOL(trace_sim_stubs, false, "Trace simulator
execution w/ stub markers")
This looks like new functionality not related to the PPC port? Please submit as
a separate patch.

https://codereview.chromium.org/422063005/diff/1/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/422063005/diff/1/src/frames.h#newcode75
src/frames.h:75: static const int kStateIntOffset = kStateOffset;
If kStateOffset isn't used for anything else but kStateIntOffset, then simply
remove it.

Perhaps it makes sense to put a constant kLowWordOfIntOffset in v8.h or
somewhere common that is keyed off of V8_TARGET_LITTLE_ENDIAN ||
!V8_HOST_ARCH_64_BIT, and then this can simply be

static const int kStateOffset = 2 * kPointerSize + kLowWordOfIntOffset;

https://codereview.chromium.org/422063005/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/422063005/diff/1/src/objects.cc#newcode787
src/objects.cc:787: #pragma GCC optimize "O0"
Please use this style of guard with the GCC detection and resetting afterwards
elsewhere where #pragmaGCC optimize is used.

Also, are you sure it's bad optimization, or not some strange hidden bug in our
code?

[rmcilroy, 2014-07-29 17:11:46]

https://codereview.chromium.org/422063005/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/422063005/diff/1/src/objects-inl.h#newcode2605
src/objects-inl.h:2605: WRITE_UINT32_FIELD(this, kSmallLayout3Offset,
small_layout_3);
Why did you have to add another layout field?  I tried really hard to avoid this
as it adds another word to every constant pool which ends up costing a lot of
memory unnecessarily.  If you need a larger number of entries you can use the
EXTENDED_SECTION which provides unlimited entries in a constant pool.

https://codereview.chromium.org/422063005/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/422063005/diff/1/src/objects.h#newcode11195
src/objects.h:11195: 
It looks like this is only here to modify VerifyMarkingVisitor - this shouldn't
be necessary - if you are getting an error that a weak object is visited by the
VerifyMarkingVisitor but not marked then this is a bug in the GC system.  There
was a bug before r22445, but these should have been fixed by
https://codereview.chromium.org/385163005/, so you may not see this issue if for
current ToT.

[andrew_low, 2014-07-30 13:27:05]

At this point I've addressed the first set of comments from danno, the latest
patch should represent the set of changes.

As suggested - some of the code has been broken out to other commits:
https://codereview.chromium.org/430523002/
https://codereview.chromium.org/427863003/

I will now work on the comments from rmcilroy

I'm continuing to have some sort of time-out failure uploading the patch set.
This appears to happen during the base file upload, suggestions on fixing this
welcomed.

Uploading base file for src/ppc/lithium-codegen-ppc.h
Uploading base file for src/ppc/ic-ppc.cc
Traceback (most recent call last):
  File "/home/alow/src/depot_tools/git_cl.py", line 2642, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/home/alow/src/depot_tools/git_cl.py", line 2628, in main
    return dispatcher.execute(OptionParser(), argv)
  File "/home/alow/src/depot_tools/subcommand.py", line 245, in execute
    return command(parser, args[1:])
  File "/home/alow/src/depot_tools/git_cl.py", line 1794, in CMDupload
    ret = RietveldUpload(options, args, cl, change)
  File "/home/alow/src/depot_tools/git_cl.py", line 1664, in RietveldUpload
    issue, patchset = upload.RealMain(upload_args)
  File "/home/alow/src/depot_tools/third_party/upload.py", line 2685, in
RealMain
    vcs.UploadBaseFiles(issue, rpc_server, patches, patchset, options, files)
  File "/home/alow/src/depot_tools/third_party/upload.py", line 1218, in
UploadBaseFiles
    print t.get(timeout=60)
  File "/usr/lib/python2.7/multiprocessing/pool.py", line 528, in get
    raise self._value
urllib2.URLError: <urlopen error [Errno 8] _ssl.c:504: EOF occurred in violation
of protocol>

https://codereview.chromium.org/422063005/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/422063005/diff/1/AUTHORS#newcode25
AUTHORS:25: Andrew Low <andrew_low@ca.ibm.com>
On 2014/07/29 13:24:08, danno wrote:
> You and all the ibm contributors are covered under the corporate CLA and
should
> not be listed individually (we need to clean this up for the other corporate
> entries in this file, but we shouldn't make it worse).

Done.

https://codereview.chromium.org/422063005/diff/1/AUTHORS#newcode33
AUTHORS:33: David Eelsohn <dje.gcc@gmail.com>
On 2014/07/29 13:24:08, danno wrote:
> If David is doing this work for IBM at IBM, he is also covered under the
> corporate CLA doesn't need to be listed (note that if he contributes code, he
> will have to use his ibm address for that, tho). If he is contributing on his
> own time, he needs to sign the ICLA.

Done.

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

https://codereview.chromium.org/422063005/diff/1/src/assembler.cc#newcode992
src/assembler.cc:992: #if V8_TARGET_ARCH_PPC64
This is related to running the simulator version of the code 'natively' on
PowerPC. This is a useful feature, but I've removed for now as it cleans up the
common code.

On 2014/07/29 13:24:07, danno wrote:
> Again, why the #ifdef? It seems like on most platforms, result_size will
always
> either 1, or if there are cases where there&nbsp;are already 2 results
returned, then
> BUILTIN_OBJECTPAIR_CALL is just an alias for BUILTIN_CALL on those platforms
and
> should be threaded through as such.

https://codereview.chromium.org/422063005/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/422063005/diff/1/src/assembler.h#newcode747
src/assembler.h:747: #if V8_TARGET_ARCH_PPC64
Also related to running simulation 'natively' on PowerPC. Removed.

On 2014/07/29 13:24:07, danno wrote:
> Why the #ifdef? This may not be implemented on all platforms, but it should be
a
> goal to make as few platform-specific #ifs as possible.

https://codereview.chromium.org/422063005/diff/1/src/base/atomicops_internals...
File src/base/atomicops_internals_ppc_gcc.h (right):

https://codereview.chromium.org/422063005/diff/1/src/base/atomicops_internals...
src/base/atomicops_internals_ppc_gcc.h:1: // Copyright 2010 the V8 project
authors. All rights reserved.
On 2014/07/29 13:24:07, danno wrote:
> Please use the new compact header.

Acknowledged.

https://codereview.chromium.org/422063005/diff/1/src/base/cpu.cc
File src/base/cpu.cc (right):

https://codereview.chromium.org/422063005/diff/1/src/base/cpu.cc#newcode254
src/base/cpu.cc:254: cache_line_size_(0),
Seems sensible, I've made what I believe are the changes you're asking for. 

On 2014/07/29 13:24:08, danno wrote:
> Seems like it makes sense to add a sentinel constant here, e.g.
> UNKOWN_CACHE_LINE_SIZE = 0.

https://codereview.chromium.org/422063005/diff/1/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode1896
src/debug.cc:1896: if (FLAG_enable_ool_constant_pool) {
Created https://codereview.chromium.org/430523002 to cover this commit

On 2014/07/29 13:24:08, danno wrote:
> Is this a change that is unrelated to the port (seems it is), if so, can you
> please factor out into a separate patch?

https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode2327
src/debug.cc:2327: Address addr = frame->pc() -
Assembler::kPatchDebugBreakSlotReturnOffset;
On 2014/07/29 13:24:08, danno wrote:
> Could you please abstract this to Assembler? There probably should be new
> method: Assembler::break_address_from_break_address which is implemented as
> "frame->pc() - Assembler::kPatchDebugBreakSlotReturnOffset" on all platforms
> except PPC. That would remove the #if def here.

Done.

https://codereview.chromium.org/422063005/diff/1/src/debug.cc#newcode2419
src/debug.cc:2419: #if V8_TARGET_ARCH_PPC
On 2014/07/29 13:24:08, danno wrote:
> See above

Done.

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

https://codereview.chromium.org/422063005/diff/1/src/disassembler.cc#newcode149
src/disassembler.cc:149: if (it != NULL && !it->done() && it->rinfo()->pc() ==
pc &&
I'm going to need a bit more help / detail here. Your comment appears to be
attached to line 149 in my modified version of the file. However, if we look
back on line 117 we can see #if !V8_TARGET_ARCH_PPC which seems like a bad idea.

I don't dis-agree with the idea of refactoring this. When developing the code,
we avoided changing the other sub-directories (implementations) and this caused
a bit more of a mess in common code.

Please be more specific in your refactoring / abstraction suggestion.

On 2014/07/29 13:24:08, danno wrote:
> Again, please try to avoid non-boilerplate platform #ifdefs in shared code
> whenever possible.
> 
> It seems like it would be cleaner to make everything in this if() a call to a
> help function that is present on all platforms that also returns "false" on
non
> PPC (and thus falls through to the decode_buffer[...] code), and actually does
> this heavy lifting on PPC.

https://codereview.chromium.org/422063005/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/422063005/diff/1/src/flag-definitions.h#newco...
src/flag-definitions.h:575: DEFINE_BOOL(trace_sim_stubs, false, "Trace simulator
execution w/ stub markers")
On 2014/07/29 13:24:08, danno wrote:
> This looks like new functionality not related to the PPC port? Please submit
as
> a separate patch.

Acknowledged.

https://codereview.chromium.org/422063005/diff/1/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/422063005/diff/1/src/frames.h#newcode75
src/frames.h:75: static const int kStateIntOffset = kStateOffset;
Both kStateOffset and kStateIntOffset are used is a handful of places. I agree
with your comment about kLowWordOfIntOffset - but would like more guidance
before I start down that path.

$ grep kStateOffset *
frames.cc:  Memory::uintptr_at(address() + StackHandlerConstants::kStateOffset)
= state;
frames.h:  static const int kStateOffset    = 2 * kPointerSize;
frames.h:  static const int kStateIntOffset = kStateOffset;
frames.h:  static const int kStateIntOffset = kStateOffset + kIntSize;

$ grep kStateIntOffset *
frames.h:  static const int kStateIntOffset = kStateOffset;
frames.h:  static const int kStateIntOffset = kStateOffset + kIntSize;
frames-inl.h:  const int offset = StackHandlerConstants::kStateIntOffset;
frames-inl.h:  const int offset = StackHandlerConstants::kStateIntOffset;

[andrew_low, 2014-07-30 20:32:27]

On 2014/07/29 17:11:46, rmcilroy wrote:
> https://codereview.chromium.org/422063005/diff/1/src/objects-inl.h
> File src/objects-inl.h (right):
> 
> https://codereview.chromium.org/422063005/diff/1/src/objects-inl.h#newcode2605
> src/objects-inl.h:2605: WRITE_UINT32_FIELD(this, kSmallLayout3Offset,
> small_layout_3);
> Why did you have to add another layout field?  I tried really hard to avoid
this
> as it adds another word to every constant pool which ends up costing a lot of
> memory unnecessarily.  If you need a larger number of entries you can use the
> EXTENDED_SECTION which provides unlimited entries in a constant pool.
 
Re-working this as you suggest is probably the right direction to go. It'll add
complexity but save object size. It may take a bit to get this sorted out.

> https://codereview.chromium.org/422063005/diff/1/src/objects.h
> File src/objects.h (right):
> 
> https://codereview.chromium.org/422063005/diff/1/src/objects.h#newcode11195
> src/objects.h:11195: 
> It looks like this is only here to modify VerifyMarkingVisitor - this
shouldn't
> be necessary - if you are getting an error that a weak object is visited by
the
> VerifyMarkingVisitor but not marked then this is a bug in the GC system. 
There
> was a bug before r22445, but these should have been fixed by
> https://codereview.chromium.org/385163005/, so you may not see this issue if
for
> current ToT.

I believe you are correct. We moved to using constant pools as there was a
fairly big performance win on PowerPC, but it seems we're early adopters of this
code.

[andrew_low, 2014-07-31 16:29:00]

Would like some advice on the best way to keep this patch current relative to
the bleeding edge.

This set of patches will no longer cleanly apply.

I'm using the git svn / git cl integration.

So I think I'm supposed to:
$ git svn fetch
$ git svn rebase master
$ git cl rebase
.. fix issues ..
$ git cl upload

Does that seem correct?

[andrew_low, 2014-08-13 20:36:51]

I haven't forgotten about this (huge) patch, just getting past the TurboFan
update etc. No action from the V8 team for now until I get an updated patch up.

[Jakob Kummerow, 2014-08-14 09:10:20]

Turbofan isn't ready for prime-time yet anyway, so you definitely don't need to
implement the entire backend right away. The changes required for being able to
compile and run tests (with TF disabled) should be fairly small; you can look at
the mips/mips64/x87 ports for examples.

Looking at your buildbots
(http://v8ppc.osuosl.org:8080/view/V8/job/Build-PowerPC-V8/lastBuild/console),
it seems they need to nuke the third_party/icu directory. In theory, "make
dependencies" should have migrated the ICU checkout, but apparently that didn't
work. Once ICU is in a sane state, "make dependencies" should proceed to check
out gtest/gmock, which are currently making the build fail.

Regarding your previous question about keeping up to date, my usual workflow is:
$ git checkout master # I use this branch to track bleeding_edge
$ git svn rebase
$ git rebase master feature_branch
$ git cl upload
But if you've found a flow that works for you, then that's obviously fine as
well.

Rebasing can get painful when you have several local commits on the branch
you're rebasing. When that happens, you can consider squashing those commits
locally -- you'll lose local fine-grained history, but you'll only have to
resolve rebasing issues once, rather than for every local commit.

Most patches are shortlived, avoiding the need for any rebasing, but this patch
here is obviously an exception :-)

[andrew_low, 2014-08-15 20:34:34]

Finally got caught up to bleeding edge again.

Code is again ready for tire-kicking.

[rmcilroy, 2014-08-18 11:11:04]

On 2014/08/15 20:34:34, andrew_low wrote:
> Finally got caught up to bleeding edge again.
> 
> Code is again ready for tire-kicking.

The architecture-independed constant pool changes look good to me, thanks!

[rmcilroy, 2014-08-18 11:14:42]

The architecture-indepent constant pool changes look good to me (with a comment
on object-printer.cc), thanks!

https://codereview.chromium.org/422063005/diff/40001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/422063005/diff/40001/src/objects-printer.cc#n...
src/objects-printer.cc:532: os << "\n  [" << i << "]: int64: " <<
get_int64_entry(i);
I would prefer that you just print out the value as both int64 and double for
all architectures - e.g.: 

 os << "\n  [" << i << "]: int64: " << get_int64_entry(i) << " (as double: " <<
get_int64_entry_as_double(i) << ")";

https://codereview.chromium.org/422063005/diff/40001/src/objects-printer.cc#n...
src/objects-printer.cc:552: os << "\n  [" << i << "]: int64: " <<
get_int64_entry(i);
ditto.

[andrew_low, 2014-08-18 14:02:09]

https://codereview.chromium.org/422063005/diff/40001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/422063005/diff/40001/src/objects-printer.cc#n...
src/objects-printer.cc:532: os << "\n  [" << i << "]: int64: " <<
get_int64_entry(i);
On 2014/08/18 11:14:42, rmcilroy wrote:
> I would prefer that you just print out the value as both int64 and double for
> all architectures - e.g.: 
> 
>  os << "\n  [" << i << "]: int64: " << get_int64_entry(i) << " (as double: "
<<
> get_int64_entry_as_double(i) << ")";

Done.

https://codereview.chromium.org/422063005/diff/40001/src/objects-printer.cc#n...
src/objects-printer.cc:552: os << "\n  [" << i << "]: int64: " <<
get_int64_entry(i);
On 2014/08/18 11:14:42, rmcilroy wrote:
> ditto.

Done.

[andrew_low, 2014-08-19 16:34:34]

Caught up to bleeding edge as of 8/19.

Code ready for additional reviews / merge at this point.

I did get caught between 
$git cl format
$git cl presubmit

Format wanted to change one of the files (removing spaces) that then were
flagged in presubmit. I picked the logical correct (with spaces) version to
push.

Please let me know what other changes are needed to get this code accepted.

[Jakob Kummerow, 2014-08-19 17:08:04]

Sorry, reviewing bandwidth is a bit of an issue right now with many people being
on vacation. We'll find someone to take a look at the current state of this
patch.

[Sven Panne, 2014-08-25 13:02:34]

On 2014/08/19 17:08:04, Jakob wrote:
> Sorry, reviewing bandwidth is a bit of an issue right now with many people
being
> on vacation. We'll find someone to take a look at the current state of this
> patch.

I'm just back from my vacation, and I can have look. The problem is that I get
quite a few "error: old chunk mismatch" when trying to view the diffs. Could you
try to delete the last patch set and re-upload it?

[andrew_low, 2014-08-26 13:07:19]

Patchset #7 (id:60001) has been deleted

[andrew_low, 2014-08-26 13:16:06]

Deleted and re-uploaded the patch set as requested.

I suspect the same problem will manifest as I continue to get an error from the
tooling when doing the upload. My guess is that the size/scope of this patch is
so large that we hit some sort of time-out.

I'm open to hacking my tooling or whatever needs to be done, but I will need
advice on fixing this error.

I do get the warning: "Patch is large, so uploading file patches separately."

The patches go up ok, but the base files never seem to complete.

Uploading base file for src/ppc/lithium-gap-resolver-ppc.cc
Uploading base file for src/debug.cc
Uploading base file for src/lithium.cc
Traceback (most recent call last):
  File "/home/alow/src/depot_tools/git_cl.py", line 2679, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/home/alow/src/depot_tools/git_cl.py", line 2665, in main
    return dispatcher.execute(OptionParser(), argv)
  File "/home/alow/src/depot_tools/subcommand.py", line 245, in execute
    return command(parser, args[1:])
  File "/home/alow/src/depot_tools/git_cl.py", line 1797, in CMDupload
    ret = RietveldUpload(options, args, cl, change)
  File "/home/alow/src/depot_tools/git_cl.py", line 1664, in RietveldUpload
    issue, patchset = upload.RealMain(upload_args)
  File "/home/alow/src/depot_tools/third_party/upload.py", line 2686, in
RealMain
    vcs.UploadBaseFiles(issue, rpc_server, patches, patchset, options, files)
  File "/home/alow/src/depot_tools/third_party/upload.py", line 1218, in
UploadBaseFiles
    print t.get(timeout=60)
  File "/usr/lib/python2.7/multiprocessing/pool.py", line 524, in get
    raise TimeoutError
multiprocessing.TimeoutError

[Sven Panne, 2014-09-01 05:59:25]

On 2014/08/26 13:16:06, andrew_low wrote:
> Deleted and re-uploaded the patch set as requested.
> 
> I suspect the same problem will manifest as I continue to get an error from
the
> tooling when doing the upload. My guess is that the size/scope of this patch
is
> so large that we hit some sort of time-out.
> 
> I'm open to hacking my tooling or whatever needs to be done, but I will need
> advice on fixing this error.
> 
> I do get the warning: "Patch is large, so uploading file patches separately."
> 
> The patches go up ok, but the base files never seem to complete.
> 
> Uploading base file for src/ppc/lithium-gap-resolver-ppc.cc
> Uploading base file for src/debug.cc
> Uploading base file for src/lithium.cc
> Traceback (most recent call last):
>   File "/home/alow/src/depot_tools/git_cl.py", line 2679, in <module>
>     sys.exit(main(sys.argv[1:]))
>   File "/home/alow/src/depot_tools/git_cl.py", line 2665, in main
>     return dispatcher.execute(OptionParser(), argv)
>   File "/home/alow/src/depot_tools/subcommand.py", line 245, in execute
>     return command(parser, args[1:])
>   File "/home/alow/src/depot_tools/git_cl.py", line 1797, in CMDupload
>     ret = RietveldUpload(options, args, cl, change)
>   File "/home/alow/src/depot_tools/git_cl.py", line 1664, in RietveldUpload
>     issue, patchset = upload.RealMain(upload_args)
>   File "/home/alow/src/depot_tools/third_party/upload.py", line 2686, in
> RealMain
>     vcs.UploadBaseFiles(issue, rpc_server, patches, patchset, options, files)
>   File "/home/alow/src/depot_tools/third_party/upload.py", line 1218, in
> UploadBaseFiles
>     print t.get(timeout=60)
>   File "/usr/lib/python2.7/multiprocessing/pool.py", line 524, in get
>     raise TimeoutError
> multiprocessing.TimeoutError

The CL still shows "Chunk mismatches", so I can't review, even downloading the
raw patch is disabled. :-P We had similar problems in the past, but I thought
that our review tool was fixed. Is there a way to split up this CL? @danno: Any
other ideas?

[Sven Panne, 2014-09-01 07:55:17]

I just started to look a little bit into the reviewable parts, see the
individual comments below. Instead of fixing these in this CL, I propose to
split up the CL into manageable parts as follows:

   * The first CL should just contain fixes for big-endian support in the
current v8, no PowerPC stuff yet. We really need to keep architecture and
endianess separate here!

   * A CL with src/ppc only, no other changes. This will hopefully make our
tooling happy and the v8 team doesn't need to review this.

   * A CL hooking in the PowerPC part.

https://codereview.chromium.org/422063005/diff/70001/Makefile
File Makefile (right):

https://codereview.chromium.org/422063005/diff/70001/Makefile#newcode204
Makefile:204: GYPFLAGS += -Dv8_host_byteorder=$(shell python -c "import sys;
print sys.byteorder")
Why???

https://codereview.chromium.org/422063005/diff/70001/src/base/platform/platfo...
File src/base/platform/platform-posix.cc (right):

https://codereview.chromium.org/422063005/diff/70001/src/base/platform/platfo...
src/base/platform/platform-posix.cc:151: #elif V8_TARGET_ARCH_PPC
No need for a separate case here, just extend the comment in the default below.

https://codereview.chromium.org/422063005/diff/70001/src/base/platform/platfo...
src/base/platform/platform-posix.cc:244: #if V8_TARGET_ARCH_PPC_BE
Please don't mix up the architecture and the endianess, we have separate
#defines for the latter: V8_TARGET_BIG_ENDIAN and V8_TARGET_LITTLE_ENDIAN, these
should be orthogonal to V8_TARGET_ARCH_PPC{,64}.

https://codereview.chromium.org/422063005/diff/70001/src/base/platform/platfo...
src/base/platform/platform-posix.cc:619: #if V8_TARGET_ARCH_PPC
Huh? How is this related to the *architecture*? Do we have a POSIX platform
without sched_yield? Sounds a bit strange...

https://codereview.chromium.org/422063005/diff/70001/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/422063005/diff/70001/src/disassembler.cc#newc...
src/disassembler.cc:110: #if !V8_TARGET_ARCH_PPC
No architecture-dependent #ifdefs in this file, please.

https://codereview.chromium.org/422063005/diff/70001/src/hydrogen-bch.cc
File src/hydrogen-bch.cc (right):

https://codereview.chromium.org/422063005/diff/70001/src/hydrogen-bch.cc#newc...
src/hydrogen-bch.cc:179: #pragma GCC diagnostic ignored "-Wuninitialized"
Remove this, GCC 4.4 is unsupported.

[andrew_low, 2014-09-15 17:53:16]

I've now (again) caught the PowerPC code base up to the very latest bleeding
edge.

As was suggested, I've broken off non-PowerPC specific changes into a patch for
big-endian support. https://codereview.chromium.org/551803004/

The PowerPC sub-directories have been split off as well into another commit.
https://codereview.chromium.org/571173003/

I will keep this one around and chop it down to only the changes required to
hook in the PowerPC implementation.

[Sven Panne, 2014-11-11 13:13:50]

On 2014/09/15 17:53:16, andrew_low wrote:
> [...] I will keep this one around and chop it down to only the changes
required to
> hook in the PowerPC implementation.

OK, then endianess-related changes and the ppc directories have been landed, so
you can remove these changes from this CL, leaving only the "hooking in" part,
which is hopefully very small.

[michael_dawson, 2014-12-19 17:24:14]

On 2014/11/11 13:13:50, Sven Panne wrote:
> On 2014/09/15 17:53:16, andrew_low wrote:
> > [...] I will keep this one around and chop it down to only the changes
> required to
> > hook in the PowerPC implementation.
> 
> OK, then endianess-related changes and the ppc directories have been landed,
so
> you can remove these changes from this CL, leaving only the "hooking in" part,
> which is hopefully very small.

Based on discussion with Daniel Clifford the next steps will be:

1) Get the changes to the common files in.  These include changes related to ppc
and others related to AIX. We will do those as two separate patches.  The first
one (1a) being the ppc specific changes and excluding those for AIX.  The second
(1b) being the patch with the changes for AIX.

- once these are in the ppc targets still won't compile because the ppc changes
will behind
- on our side separately we'll be building/testing with the combination of the
changes that have gone in and the outstanding updates for the ppc subdirectories
  
2) bulk update of the files in the ppc directories.  
        - at this point the ppc targets should compile and run 

3) individual submission of some smaller patches to add optimizations for ppc

4) ongoing regular updates to keep the ppc targets up to date.

I'm just running quickcheck on the patch for 1a. Once complete I'll submit and
specify ISSUE=422063005 which I think should make it be attached to this issue.

[michael_dawson, 2014-12-20 23:03:22]

Since I was not the owner of this issue I could not upload this patch on
this issue.  

I therefore uploaded on a new issue: https://codereview.chromium.org/817143002/

If there was a better way to do this just let me know.

[Sven Panne, 2015-01-07 07:55:07]

On 2014/12/20 23:03:22, michael_dawson wrote:
> [...] If there was a better way to do this just let me know.

I don't think so, closing this issue...