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 <[email protected]>
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 <[email protected]>
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 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_ppc_gcc.h
File src/base/atomicops_internals_ppc_gcc.h (right):
https://codereview.chromium.org/422063005/diff/1/src/base/atomicops_internals_ppc_gcc.h#newcode1
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#newcode575
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;
https://codereview.chromium.org/422063005/
--
--
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.
