Add support for 64-bit Windows build: base and chrome/app

Add support for 64-bit Windows build: base and chrome/app

This is required to support Native Client on 64-bit Windows.
Native Client will use a small 64-bit executable to load NaCl modules on 64-bit Windows. This 64-bit executable will use Chrome code, but some functionality has to be removed to minimize dependencies. This functionality may be enabled later if we decide that it is necessary.

TEST=None
BUG=28176
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33823

[Peter Kasting, 2009-11-18 05:37:54]

A lot of the omitted things seem very random when you're looking at them.  It's
not obvious why some particular thing should be off for NACL_WIN64.  Plus, my
impression was that we eventually wanted to be able to build Chrome for 64-bit
Windows anyway.  Can some of these things not be #if'd out?  Can we check in
64-bit fixes where possible?

[gregoryd, 2009-11-18 18:24:47]

We decided that building Chrome for 64-bit Windows is not our goal at this
stage. However, the changes I am working on will make it easier when we
decide to do it (several CLs with fixes for 64-bit build have already been
submitted)
The code that is being disabled in this CL will require more time to port,
so since it's not relevant for the goal of supporting NaCl, we decided to
postpone it. Another reason to actually disable code that is not required is
to minimize dependencies and make the 64-bit binaries smaller.


On Tue, Nov 17, 2009 at 21:37, <pkasting@chromium.org> wrote:

> A lot of the omitted things seem very random when you're looking at them.
>  It's
> not obvious why some particular thing should be off for NACL_WIN64.  Plus,
> my
> impression was that we eventually wanted to be able to build Chrome for
> 64-bit
> Windows anyway.  Can some of these things not be #if'd out?  Can we check
> in
> 64-bit fixes where possible?
>
>
> http://codereview.chromium.org/371073
>

[jam, 2009-11-18 19:12:23]

I share Peter's sentiment.  It's really unclear when going through code in base
or common why they have seemingly random parts ifdef'd out if NACL_WIN64 is
defined.

http://codereview.chromium.org/371073/diff/12001/13007
File base/message_loop.cc (right):

http://codereview.chromium.org/371073/diff/12001/13007#newcode531
Line 531: #ifndef NACL_WIN64
it's really unclear to me (or anyone reading this code) why these ifdefs are
here?

http://codereview.chromium.org/371073/diff/12001/13002
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/371073/diff/12001/13002#newcode287
Line 287: #ifndef NACL_WIN64
ditto

[jam, 2009-11-18 19:13:14]

On 2009/11/18 19:12:23, John Abd-El-Malek wrote:
> I share Peter's sentiment.  It's really unclear when going through code in
base
> or common why they have seemingly random parts ifdef'd out if NACL_WIN64 is
> defined.

To expand on this a little: perhaps the ifdef shouldn't be NACL, but should be
just a generic 64 bit related ifdef

[gregoryd, 2009-11-18 19:20:42]

I used the generic _WIN64 at first, but then decided to switch to
NACL_WIN64. The reason is we don't want to disable this code for 64-bit
Windows forever (or for building any other 64-bit target using this code),
we want to disable this code only when we are building the NaCl loader. Even
if the problems related to 64-bits are fixed later, we might want to keep
the NaCl loader executable small and avoid adding dependencies. It will
change when we get a separate 64-bit build for Chrome on Windows, but we may
go back to the current design for built-in NaCl then.
Will adding comments explaining what NACL_WIN64 is about help?


On Wed, Nov 18, 2009 at 11:12, <jam@chromium.org> wrote:

> I share Peter's sentiment.  It's really unclear when going through code in
> base
> or common why they have seemingly random parts ifdef'd out if NACL_WIN64 is
> defined.
>
>
> http://codereview.chromium.org/371073/diff/12001/13007
> File base/message_loop.cc (right):
>
> http://codereview.chromium.org/371073/diff/12001/13007#newcode531
> Line 531: #ifndef NACL_WIN64
> it's really unclear to me (or anyone reading this code) why these ifdefs
> are here?
>
> http://codereview.chromium.org/371073/diff/12001/13002
> File chrome/app/chrome_dll_main.cc (right):
>
> http://codereview.chromium.org/371073/diff/12001/13002#newcode287
> Line 287: #ifndef NACL_WIN64
> ditto
>
>
> http://codereview.chromium.org/371073
>

[jam, 2009-11-19 20:44:15]

I think it'll still be confusing to developers reading the code, but I'm not
sure what's a better alternative.  How will developers know if they break
the 64 bit NaCl build, is there a buildbot already?

CCing Darin who might have stronger opinions on the 64 bit macro stuff.

On Wed, Nov 18, 2009 at 11:20 AM, Gregory Dardyk <gregoryd@google.com>wrote:

> I used the generic _WIN64 at first, but then decided to switch to
> NACL_WIN64. The reason is we don't want to disable this code for 64-bit
> Windows forever (or for building any other 64-bit target using this code),
> we want to disable this code only when we are building the NaCl loader. Even
> if the problems related to 64-bits are fixed later, we might want to keep
> the NaCl loader executable small and avoid adding dependencies. It will
> change when we get a separate 64-bit build for Chrome on Windows, but we may
> go back to the current design for built-in NaCl then.
> Will adding comments explaining what NACL_WIN64 is about help?
>
>
> On Wed, Nov 18, 2009 at 11:12, <jam@chromium.org> wrote:
>
>> I share Peter's sentiment.  It's really unclear when going through code in
>> base
>> or common why they have seemingly random parts ifdef'd out if NACL_WIN64
>> is
>> defined.
>>
>>
>> http://codereview.chromium.org/371073/diff/12001/13007
>> File base/message_loop.cc (right):
>>
>> http://codereview.chromium.org/371073/diff/12001/13007#newcode531
>> Line 531: #ifndef NACL_WIN64
>> it's really unclear to me (or anyone reading this code) why these ifdefs
>> are here?
>>
>> http://codereview.chromium.org/371073/diff/12001/13002
>> File chrome/app/chrome_dll_main.cc (right):
>>
>> http://codereview.chromium.org/371073/diff/12001/13002#newcode287
>> Line 287: #ifndef NACL_WIN64
>> ditto
>>
>>
>> http://codereview.chromium.org/371073
>>
>
>

[gregoryd, 2009-11-19 20:55:14]

It will use the same Windows buildbots as we use now. Actually, building
Chrome on the developers local machines will also reveal the errors. We will
have a separate set of targets that will always be built for 64-bit Windows,
and all the binaries required for Chrome on both 32-bit and 64-bit Windows
will be part of single configuration of a single solution in Visual Studio.
This is possible thanks to a change Brad Nelson made in gyp that allows us
to specify for each target what platform it should be built for. A CL adding
the 64-bit targets is pending - waiting for this one to go in.

On Thu, Nov 19, 2009 at 12:44, John Abd-El-Malek <jam@chromium.org> wrote:

> I think it'll still be confusing to developers reading the code, but I'm
> not sure what's a better alternative.  How will developers know if they
> break the 64 bit NaCl build, is there a buildbot already?
>
> CCing Darin who might have stronger opinions on the 64 bit macro stuff.
>
>
> On Wed, Nov 18, 2009 at 11:20 AM, Gregory Dardyk <gregoryd@google.com>wrote:
>
>> I used the generic _WIN64 at first, but then decided to switch to
>> NACL_WIN64. The reason is we don't want to disable this code for 64-bit
>> Windows forever (or for building any other 64-bit target using this code),
>> we want to disable this code only when we are building the NaCl loader. Even
>> if the problems related to 64-bits are fixed later, we might want to keep
>> the NaCl loader executable small and avoid adding dependencies. It will
>> change when we get a separate 64-bit build for Chrome on Windows, but we may
>> go back to the current design for built-in NaCl then.
>> Will adding comments explaining what NACL_WIN64 is about help?
>>
>>
>> On Wed, Nov 18, 2009 at 11:12, <jam@chromium.org> wrote:
>>
>>> I share Peter's sentiment.  It's really unclear when going through code
>>> in base
>>> or common why they have seemingly random parts ifdef'd out if NACL_WIN64
>>> is
>>> defined.
>>>
>>>
>>> http://codereview.chromium.org/371073/diff/12001/13007
>>> File base/message_loop.cc (right):
>>>
>>> http://codereview.chromium.org/371073/diff/12001/13007#newcode531
>>> Line 531: #ifndef NACL_WIN64
>>> it's really unclear to me (or anyone reading this code) why these ifdefs
>>> are here?
>>>
>>> http://codereview.chromium.org/371073/diff/12001/13002
>>> File chrome/app/chrome_dll_main.cc (right):
>>>
>>> http://codereview.chromium.org/371073/diff/12001/13002#newcode287
>>> Line 287: #ifndef NACL_WIN64
>>> ditto
>>>
>>>
>>> http://codereview.chromium.org/371073
>>>
>>
>>
>

[darin (slow to review), 2009-11-19 23:14:04]

I agree with John and Peter.  The use of the NACL_WIN64 macro does not seem very
maintainable.  This will impair our ability to refactor or move code around. 
There has to be a better way.

-Darin


On 2009/11/19 20:44:15, John Abd-El-Malek wrote:
> I think it'll still be confusing to developers reading the code, but I'm not
> sure what's a better alternative.  How will developers know if they break
> the 64 bit NaCl build, is there a buildbot already?
> 
> CCing Darin who might have stronger opinions on the 64 bit macro stuff.
> 
> On Wed, Nov 18, 2009 at 11:20 AM, Gregory Dardyk <gregoryd@google.com>wrote:
> 
> > I used the generic _WIN64 at first, but then decided to switch to
> > NACL_WIN64. The reason is we don't want to disable this code for 64-bit
> > Windows forever (or for building any other 64-bit target using this code),
> > we want to disable this code only when we are building the NaCl loader. Even
> > if the problems related to 64-bits are fixed later, we might want to keep
> > the NaCl loader executable small and avoid adding dependencies. It will
> > change when we get a separate 64-bit build for Chrome on Windows, but we may
> > go back to the current design for built-in NaCl then.
> > Will adding comments explaining what NACL_WIN64 is about help?
> >
> >
> > On Wed, Nov 18, 2009 at 11:12, <mailto:jam@chromium.org> wrote:
> >
> >> I share Peter's sentiment.  It's really unclear when going through code in
> >> base
> >> or common why they have seemingly random parts ifdef'd out if NACL_WIN64
> >> is
> >> defined.
> >>
> >>
> >> http://codereview.chromium.org/371073/diff/12001/13007
> >> File base/message_loop.cc (right):
> >>
> >> http://codereview.chromium.org/371073/diff/12001/13007#newcode531
> >> Line 531: #ifndef NACL_WIN64
> >> it's really unclear to me (or anyone reading this code) why these ifdefs
> >> are here?
> >>
> >> http://codereview.chromium.org/371073/diff/12001/13002
> >> File chrome/app/chrome_dll_main.cc (right):
> >>
> >> http://codereview.chromium.org/371073/diff/12001/13002#newcode287
> >> Line 287: #ifndef NACL_WIN64
> >> ditto
> >>
> >>
> >> http://codereview.chromium.org/371073
> >>
> >
> >
>

[brettw, 2009-11-23 17:12:57]

http://codereview.chromium.org/371073/diff/25004/26004
File base/file_util.h (right):

http://codereview.chromium.org/371073/diff/25004/26004#newcode526
base/file_util.h:526: DWORD length_;
I would prefer to keep this as a size_t or convert to an int64 rather than have
platform-specific sizes. Having a platform-specific size type here means that
calling code needs to be platform-specific (say it wants to put it in a
variable). Its better to have the OS-specific code that sets it encapsulate the
platform differences.

[jam, 2009-11-23 17:47:53]

Gregory: I think Brett or Darin are better reviewers for this, they have more
useful info regarding what to call the ifdef etc

[gregoryd, 2009-11-24 17:50:05]

http://codereview.chromium.org/371073/diff/25004/26004
File base/file_util.h (right):

http://codereview.chromium.org/371073/diff/25004/26004#newcode526
base/file_util.h:526: DWORD length_;
On 2009/11/23 17:12:58, brettw wrote:
> I would prefer to keep this as a size_t or convert to an int64 rather than
have
> platform-specific sizes. Having a platform-specific size type here means that
> calling code needs to be platform-specific (say it wants to put it in a
> variable). Its better to have the OS-specific code that sets it encapsulate
the
> platform differences.

Done.

[gregoryd, 2009-12-02 22:37:29]

Switched to another solution - this is what's left from this CL.

[jam, 2009-12-04 06:41:35]

lgtm