64-bit compatibility changes for the sandbox code

64-bit compatibility changes for the sandbox code
This CL contains some basic changes that eliminate some of the warnings that appear when the sandbox code is compiled for 64-bit Windows. This is part of a larger effort to support Native Client on 64-bit Windows (that will require the sandbox to support 64-bit Windows).
TEST=will be tested when the rest of the code builds for 64-bit Windows
BUG=27218
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31625

[Nicolas Sylvain, 2009-11-09 22:23:28]

LGTM

[cpu_(ooo_6.6-7.5), 2009-11-09 22:24:37]

For the most part I don't see harm on this change, but I'll hold my lgtm until
Rick has reviewed this.

http://codereview.chromium.org/378030/diff/1/6
File sandbox/src/sidestep/mini_disassembler.h (right):

http://codereview.chromium.org/378030/diff/1/6#newcode71
Line 71: uintptr_t* instruction_bytes);
this would be the first occurrence of this type in the whole of sandbox, I am
not sure we want to go that way. I rather use on of the other types we already
use that map to 

unsigned __int64

already.

[cpu_(ooo_6.6-7.5), 2009-11-09 22:26:32]

please fill out the BUG= and TEST= fields. I would not be a bad idea to have a
bug to track all the progress and commits.

[rvargas (doing something else), 2009-11-09 22:36:11]

My comment from the previous CL still applies. If you don't want to exclude the
whole set of files via gyp, please add TODOs to track the code changed but not
tested.

[gregoryd, 2009-11-09 22:52:18]

I am not sure I understand what do you mean.
Right now I removed the dependency on the sandbox from the 64-bit version
Chrome I am building, but I plan to re-enable it in the future. I'd like to
submit this CL since I already made the changes and they will be useful when
we start working on adding 64-bit support to the sandbox some time in the
near future.
What is considered to be tested code? Does it have to be covered by unit
tests? In this case it should already be tested on 32-bit and we don't plan
to use it on 64-bit before all tests run successfully on that platform.

On Mon, Nov 9, 2009 at 14:36, <rvargas@chromium.org> wrote:

> My comment from the previous CL still applies. If you don't want to exclude
> the
> whole set of files via gyp, please add TODOs to track the code changed but
> not
> tested.
>
>
> http://codereview.chromium.org/378030
>

[gregoryd, 2009-11-10 00:00:23]

On 2009/11/09 22:26:32, cpu wrote:
> please fill out the BUG= and TEST= fields. I would not be a bad idea to have a
> bug to track all the progress and commits.

Great idea - will create the bug once I get my chromium.org account (hopefully
today).

[gregoryd, 2009-11-10 00:00:37]

http://codereview.chromium.org/378030/diff/1/6
File sandbox/src/sidestep/mini_disassembler.h (right):

http://codereview.chromium.org/378030/diff/1/6#newcode71
Line 71: uintptr_t* instruction_bytes);
On 2009/11/09 22:24:37, cpu wrote:
> this would be the first occurrence of this type in the whole of sandbox, I am
> not sure we want to go that way. I rather use on of the other types we already
> use that map to 
> 
> unsigned __int64
> 
> already.
> 

Changed the implementation to use static_cast

[rvargas (doing something else), 2009-11-10 01:12:26]

On 2009/11/09 22:52:18, gregoryd wrote:
> I am not sure I understand what do you mean.
> Right now I removed the dependency on the sandbox from the 64-bit version
> Chrome I am building, but I plan to re-enable it in the future. I'd like to
> submit this CL since I already made the changes and they will be useful when
> we start working on adding 64-bit support to the sandbox some time in the
> near future.
> What is considered to be tested code? Does it have to be covered by unit
> tests? In this case it should already be tested on 32-bit and we don't plan
> to use it on 64-bit before all tests run successfully on that platform.
> 

Well, I guess that all the changes are correct. I'm just not that comfortable
with adding code everywhere to suggest that 64 bit has been taken into account
when in fact that is not the case. It could make more difficult the process of
actually adding 64 bit support.

That said, the only place where this CL could be misleading is the disassembler.
I don't think that upgrading that module to 64 bit is within anybody's plan, so
we might as well leave it outside that build forever.

[cpu_(ooo_6.6-7.5), 2009-11-10 01:35:22]

I created the bug
http://code.google.com/p/chromium/issues/detail?id=27218

now you can use BUG=27218

[gregoryd, 2009-11-10 21:54:32]

On 2009/11/10 01:12:26, rvargas wrote:
> On 2009/11/09 22:52:18, gregoryd wrote:
> > I am not sure I understand what do you mean.
> > Right now I removed the dependency on the sandbox from the 64-bit version
> > Chrome I am building, but I plan to re-enable it in the future. I'd like to
> > submit this CL since I already made the changes and they will be useful when
> > we start working on adding 64-bit support to the sandbox some time in the
> > near future.
> > What is considered to be tested code? Does it have to be covered by unit
> > tests? In this case it should already be tested on 32-bit and we don't plan
> > to use it on 64-bit before all tests run successfully on that platform.
> > 
> 
> Well, I guess that all the changes are correct. I'm just not that comfortable
> with adding code everywhere to suggest that 64 bit has been taken into account
> when in fact that is not the case. It could make more difficult the process of
> actually adding 64 bit support.
> 
> That said, the only place where this CL could be misleading is the
disassembler.
> I don't think that upgrading that module to 64 bit is within anybody's plan,
so
> we might as well leave it outside that build forever.

Removed the changes in mini_disassembler.cpp and added an #error to warn that
this file should not be ported to Win64. Also added a similar warning in
sandbox.cc (assuming this is one of the main files and will always be present in
the build).

[gregoryd, 2009-11-10 21:54:56]

On 2009/11/10 01:12:26, rvargas wrote:
> On 2009/11/09 22:52:18, gregoryd wrote:
> > I am not sure I understand what do you mean.
> > Right now I removed the dependency on the sandbox from the 64-bit  
> version
> > Chrome I am building, but I plan to re-enable it in the future. I'd  
> like to
> > submit this CL since I already made the changes and they will be useful  
> when
> > we start working on adding 64-bit support to the sandbox some time in  
> the
> > near future.
> > What is considered to be tested code? Does it have to be covered by unit
> > tests? In this case it should already be tested on 32-bit and we don't  
> plan
> > to use it on 64-bit before all tests run successfully on that platform.
> >

> Well, I guess that all the changes are correct. I'm just not that  
> comfortable
> with adding code everywhere to suggest that 64 bit has been taken into  
> account
> when in fact that is not the case. It could make more difficult the  
> process of
> actually adding 64 bit support.

> That said, the only place where this CL could be misleading is the
disassembler.
> I don't think that upgrading that module to 64 bit is within anybody's  
> plan,
so
> we might as well leave it outside that build forever.

Removed the changes in mini_disassembler.cpp and added an #error to warn  
that
this file should not be ported to Win64. Also added a similar warning in
sandbox.cc (assuming this is one of the main files and will always be  
present in
the build).

http://codereview.chromium.org/378030

[rvargas (doing something else), 2009-11-10 22:14:05]

LGTM. Thanks.

[rvargas (doing something else), 2009-11-10 22:14:23]

LGTM. Thanks.

http://codereview.chromium.org/378030

[cpu_(ooo_6.6-7.5), 2009-11-10 23:11:04]

http://codereview.chromium.org/378030/diff/12001/12005
File sandbox/src/sandbox_nt_util.cc (right):

http://codereview.chromium.org/378030/diff/12001/12005#newcode373
Line 373: DCHECK(kuint16max > size_bytes) << "The string is too long";
My understanding is that this file needs the DCHECK_NT instead, right Rick?

[cpu_(ooo_6.6-7.5), 2009-11-10 23:11:30]

http://codereview.chromium.org/378030/diff/12001/12005
File sandbox/src/sandbox_nt_util.cc (right):

http://codereview.chromium.org/378030/diff/12001/12005#newcode373
Line 373: DCHECK(kuint16max > size_bytes) << "The string is too long";
My understanding is that this file needs the DCHECK_NT instead, right
Rick?

http://codereview.chromium.org/378030

[rvargas (doing something else), 2009-11-10 23:22:18]

http://codereview.chromium.org/378030/diff/12001/12005
File sandbox/src/sandbox_nt_util.cc (right):

http://codereview.chromium.org/378030/diff/12001/12005#newcode373
Line 373: DCHECK(kuint16max > size_bytes) << "The string is too long";
On 2009/11/10 23:11:04, cpu wrote:
> My understanding is that this file needs the DCHECK_NT instead, right Rick?

oops... yes, and this code should not include logging.h

[rvargas (doing something else), 2009-11-10 23:22:40]

http://codereview.chromium.org/378030/diff/12001/12005
File sandbox/src/sandbox_nt_util.cc (right):

http://codereview.chromium.org/378030/diff/12001/12005#newcode373
Line 373: DCHECK(kuint16max > size_bytes) << "The string is too long";
On 2009/11/10 23:11:04, cpu wrote:
> My understanding is that this file needs the DCHECK_NT instead, right
Rick?

oops... yes, and this code should not include logging.h

http://codereview.chromium.org/378030

[cpu_(ooo_6.6-7.5), 2009-11-10 23:24:49]

other than the assert issue LGTM as well.

[cpu_(ooo_6.6-7.5), 2009-11-10 23:25:29]

other than the assert issue LGTM as well.

http://codereview.chromium.org/378030