Create a build_config header file.

Create a build_config header file.

This introduces a build_config.h which provides some preprocessor defines
provided by build systems. Defines from the build system still take precedence
as a means for rolling this out slowly.

BUG=https://code.google.com/p/chromium/issues/detail?id=440012

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=916e3c83dcf343d00aff40621afcc58860419648

[teravest, 2014-12-10 16:59:29]

teravest@chromium.org changed reviewers:
+ mseaborn@chromium.org, ncbray@chromium.org

[Mark Seaborn, 2014-12-11 16:11:41]

> This change currently only changes the behavior for scons linux builds.

Good idea.

> I want to make sure that the #include order caused by this change is
acceptable
> before purusing this further.

I'm not sure what you mean by the #include order, or what might be wrong with
it.

The devil is in the detail of whether this builds everywhere OK, so please run
trybots.

https://codereview.chromium.org/788193003/diff/1/SConstruct
File SConstruct (right):

https://codereview.chromium.org/788193003/diff/1/SConstruct#newcode2146
SConstruct:2146: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
Remove these?

https://codereview.chromium.org/788193003/diff/1/SConstruct#newcode3479
SConstruct:3479: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
And these?

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h
File src/include/nacl_defines.h (right):

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:6: 
Could we call this "build_config.h"?  Firstly, to match Chromium.  Secondly,
because "nacl_defines.h" is a bit too generic.

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:10: #if !defined(NACL_WINDOWS) &&
!defined(NACL_LINUX) && !defined(NACL_OSX)
Can you add a TODO to remove this when the Gyp build stops defining these?

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:12: #if defined(_WIN32)
Can you indent like this?
#if defined(_WIN32)
# define NACL_WINDOWS 1
#else
...

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:40: #if defined(NACL_BUILD_ARCH) &&
!defined(NACL_BUILD_SUBARCH)
Similar TODO for NACL_BUILD_(SUB)ARCH

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:59: /* TODO(teravest): Handle __aarch64__? */
This TODO is not really needed since we don't support AArch64.

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:65: #if defined((__MIPSEL__)
Stray extra "(" here

[teravest, 2014-12-11 21:23:40]

Patchset #2 (id:20001) has been deleted

[teravest, 2014-12-11 21:23:47]

Patchset #2 (id:40001) has been deleted

[teravest, 2014-12-11 21:23:54]

Patchset #2 (id:60001) has been deleted

[teravest, 2014-12-11 21:24:04]

Patchset #2 (id:60001) has been deleted

[teravest, 2014-12-12 17:54:19]

Patchset #4 (id:120001) has been deleted

[teravest, 2014-12-12 18:40:21]

Patchset #5 (id:160001) has been deleted

[teravest, 2014-12-15 16:16:21]

Patchset #3 (id:100001) has been deleted

[teravest, 2014-12-15 16:16:29]

Patchset #3 (id:140001) has been deleted

[teravest, 2014-12-15 16:16:37]

Patchset #3 (id:180001) has been deleted

[teravest, 2014-12-15 16:16:46]

Patchset #4 (id:220001) has been deleted

[teravest, 2014-12-15 16:16:52]

Patchset #5 (id:260001) has been deleted

[teravest, 2014-12-15 16:16:57]

Patchset #4 (id:240001) has been deleted

[teravest, 2014-12-15 16:17:05]

Patchset #3 (id:200001) has been deleted

[teravest, 2014-12-15 16:17:14]

Patchset #2 (id:80001) has been deleted

[teravest, 2014-12-15 16:28:05]

Patchset #2 (id:280001) has been deleted

[teravest, 2014-12-15 17:47:07]

Patchset #3 (id:320001) has been deleted

[teravest, 2014-12-15 20:08:36]

My concern about include ordering is that this makes some include sections a
little ugly, like this:
#include "native_client/src/include/build_config.h"
#if defined(NACL_WINDOWS)
#include <windows.h>
#endif

...but I hope that nobody minds.

https://codereview.chromium.org/788193003/diff/1/SConstruct
File SConstruct (right):

https://codereview.chromium.org/788193003/diff/1/SConstruct#newcode2146
SConstruct:2146: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
On 2014/12/11 16:11:40, Mark Seaborn wrote:
> Remove these?

Done.

https://codereview.chromium.org/788193003/diff/1/SConstruct#newcode3479
SConstruct:3479: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
On 2014/12/11 16:11:40, Mark Seaborn wrote:
> And these?

Done.

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h
File src/include/nacl_defines.h (right):

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:6: 
On 2014/12/11 16:11:40, Mark Seaborn wrote:
> Could we call this "build_config.h"?  Firstly, to match Chromium.  Secondly,
> because "nacl_defines.h" is a bit too generic.

Done.

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:10: #if !defined(NACL_WINDOWS) &&
!defined(NACL_LINUX) && !defined(NACL_OSX)
On 2014/12/11 16:11:41, Mark Seaborn wrote:
> Can you add a TODO to remove this when the Gyp build stops defining these?

Done.

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:12: #if defined(_WIN32)
On 2014/12/11 16:11:40, Mark Seaborn wrote:
> Can you indent like this?
> #if defined(_WIN32)
> # define NACL_WINDOWS 1
> #else
> ...

Done.

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:40: #if defined(NACL_BUILD_ARCH) &&
!defined(NACL_BUILD_SUBARCH)
On 2014/12/11 16:11:40, Mark Seaborn wrote:
> Similar TODO for NACL_BUILD_(SUB)ARCH

Done.

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:59: /* TODO(teravest): Handle __aarch64__? */
On 2014/12/11 16:11:41, Mark Seaborn wrote:
> This TODO is not really needed since we don't support AArch64.

Removed.

https://codereview.chromium.org/788193003/diff/1/src/include/nacl_defines.h#n...
src/include/nacl_defines.h:65: #if defined((__MIPSEL__)
On 2014/12/11 16:11:40, Mark Seaborn wrote:
> Stray extra "(" here

Done.

[Mark Seaborn, 2014-12-16 18:54:14]

On 15 December 2014 at 12:08, <teravest@chromium.org> wrote:

> My concern about include ordering is that this makes some include sections
> a
> little ugly, like this:
> #include "native_client/src/include/build_config.h"
> #if defined(NACL_WINDOWS)
> #include <windows.h>
> #endif
>
> ...but I hope that nobody minds.


That pattern is OK with me.

There is a risk with this change, though.  What happens when we have code
like this:

#if NACL_LINUX
  SafetyCriticalStep();
#elif NACL_WINDOWS
  OtherSafetyCriticalStep();
#endif

What happens if this file forgets to #include build_config.h?

On Unix, we should catch that, because we compile with -Wundef, so "#if
NACL_LINUX" would give a warning/error when NACL_LINUX is not #defined.

So far we haven't enabled any equivalent of -Wundef for MSVC on Windows.
Is there an equivalent that we can enable?  This suggests that C4668 is an
equivalent:
http://stackoverflow.com/questions/26787161/what-is-msvc-equivalent-to-gccs-w...
, http://msdn.microsoft.com/en-us/library/4dt9kyhy.aspx

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[teravest, 2014-12-17 17:09:50]

Oh, gross. :(
I tried enabling C4668, but I'm unsurprisingly hitting some include warnings I
don't know what to do with.

Here are the two examples that show up a lot in my logs:
FAILED: ninja -t msvc -e environment.x64 --
"D:\src\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64\cl.exe" /nologo
/showIncludes /FC @obj\native_client\src\shared\gio\gio64.gprintf.obj.rsp /c
..\..\native_client\src\shared\gio\gprintf.c
/Foobj\native_client\src\shared\gio\gio64.gprintf.obj
/Fdobj\native_client\src\shared\gio\gio64.c.pdb

d:\src\depot_tools\win_toolchain\vs2013_files\win8sdk\include\um\processthreadsapi.h(1170)
: error C4668: '_WIN32_WINNT_WINTHRESHOLD' is not defined as a preprocessor
macro, replacing with '0' for '#if/#elif'
d:\src\depot_tools\win_toolchain\vs2013_files\win8sdk\include\um\winbase.h(8618)
: error C4668: 'NTDDI_WIN7SP1' is not defined as a preprocessor macro, replacing
with '0' for '#if/#elif'

I suppose I could just define those to 0 in the NaCl gyp files (that's the
behavior that we have today, after all), but that seems pretty ugly. Any
thoughts?

On 2014/12/16 18:54:14, Mark Seaborn wrote:
> On 15 December 2014 at 12:08, <mailto:teravest@chromium.org> wrote:
> 
> > My concern about include ordering is that this makes some include sections
> > a
> > little ugly, like this:
> > #include "native_client/src/include/build_config.h"
> > #if defined(NACL_WINDOWS)
> > #include <windows.h>
> > #endif
> >
> > ...but I hope that nobody minds.
> 
> 
> That pattern is OK with me.
> 
> There is a risk with this change, though.  What happens when we have code
> like this:
> 
> #if NACL_LINUX
>   SafetyCriticalStep();
> #elif NACL_WINDOWS
>   OtherSafetyCriticalStep();
> #endif
> 
> What happens if this file forgets to #include build_config.h?
> 
> On Unix, we should catch that, because we compile with -Wundef, so "#if
> NACL_LINUX" would give a warning/error when NACL_LINUX is not #defined.
> 
> So far we haven't enabled any equivalent of -Wundef for MSVC on Windows.
> Is there an equivalent that we can enable?  This suggests that C4668 is an
> equivalent:
>
http://stackoverflow.com/questions/26787161/what-is-msvc-equivalent-to-gccs-w...
> , http://msdn.microsoft.com/en-us/library/4dt9kyhy.aspx
> 
> Cheers,
> Mark
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2014-12-17 18:59:06]

On 17 December 2014 at 09:09, <teravest@chromium.org> wrote:

> Oh, gross. :(
> I tried enabling C4668, but I'm unsurprisingly hitting some include
> warnings I
> don't know what to do with.
>

Yeah, this seems to be a general problem with MSVC.

GCC has the ability to turn off warnings for system headers, but MSVC
doesn't.

Searching for "msvc warnings in system headers" turns up some
stackoverflow.com threads such as:
http://stackoverflow.com/questions/2541984/how-to-suppress-warnings-in-extern...
http://stackoverflow.com/questions/4001736/whats-up-with-the-thousands-of-war...
http://stackoverflow.com/questions/15175178/how-to-suppress-warnings-from-int...

The conclusion seems to be that you have to do:

#pragma warning(push, 0)
//Some includes with unfixable warnings
#pragma warning(pop)

I would suggest asking on chromium-dev first.  I expect others have run
into this problem when trying to enable new warnings in the Windows Chrome
build.

If no-one has any better suggestions, we could use the #pragma.  Maybe we
should centralise NaCl's "#include <windows.h>" so that we don't have to
copy the #pragma around.

Another possibility would be to inject nacl_defines.h into all NaCl .c/.cc
files using "-include .../nacl_defines.h", to reduce the risk of forgetting
to #include it.  This also assumes that there is an MSVC equivalent of
"-include".



> Here are the two examples that show up a lot in my logs:
> FAILED: ninja -t msvc -e environment.x64 --
> "D:\src\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64\cl.exe"
> /nologo
> /showIncludes /FC @obj\native_client\src\shared\gio\gio64.gprintf.obj.rsp
> /c
> ..\..\native_client\src\shared\gio\gprintf.c
> /Foobj\native_client\src\shared\gio\gio64.gprintf.obj
> /Fdobj\native_client\src\shared\gio\gio64.c.pdb
>
> d:\src\depot_tools\win_toolchain\vs2013_files\win8sdk\include\um\
> processthreadsapi.h(1170)
> : error C4668: '_WIN32_WINNT_WINTHRESHOLD' is not defined as a preprocessor
> macro, replacing with '0' for '#if/#elif'
> d:\src\depot_tools\win_toolchain\vs2013_files\
> win8sdk\include\um\winbase.h(8618)
> : error C4668: 'NTDDI_WIN7SP1' is not defined as a preprocessor macro,
> replacing
> with '0' for '#if/#elif'
>
> I suppose I could just define those to 0 in the NaCl gyp files (that's the
> behavior that we have today, after all), but that seems pretty ugly. Any
> thoughts?


That seems like it would be too brittle.  New #ifs might occur in new
versions of the Windows headers.  Also, it would break cases where headers
do "#ifdef FOO" and expect FOO to be undefined.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[teravest, 2014-12-18 16:49:32]

On 2014/12/17 18:59:06, Mark Seaborn wrote:
> On 17 December 2014 at 09:09, <mailto:teravest@chromium.org> wrote:
> 
> > Oh, gross. :(
> > I tried enabling C4668, but I'm unsurprisingly hitting some include
> > warnings I
> > don't know what to do with.
> >
> 
> Yeah, this seems to be a general problem with MSVC.
> 
> GCC has the ability to turn off warnings for system headers, but MSVC
> doesn't.
> 
> Searching for "msvc warnings in system headers" turns up some
> http://stackoverflow.com threads such as:
>
http://stackoverflow.com/questions/2541984/how-to-suppress-warnings-in-extern...
>
http://stackoverflow.com/questions/4001736/whats-up-with-the-thousands-of-war...
>
http://stackoverflow.com/questions/15175178/how-to-suppress-warnings-from-int...
> 
> The conclusion seems to be that you have to do:
> 
> #pragma warning(push, 0)
> //Some includes with unfixable warnings
> #pragma warning(pop)
> 
> I would suggest asking on chromium-dev first.  I expect others have run
> into this problem when trying to enable new warnings in the Windows Chrome
> build.
> 
> If no-one has any better suggestions, we could use the #pragma.  Maybe we
> should centralise NaCl's "#include <windows.h>" so that we don't have to
> copy the #pragma around.

It looks like we're stuck with pragmas for now. I'm surprised at the places that
<windows.h> is included (like nacl_compiler_annotations.h) but it might not be
too bad to add them around.

> 
> Another possibility would be to inject nacl_defines.h into all NaCl .c/.cc
> files using "-include .../nacl_defines.h", to reduce the risk of forgetting
> to #include it.  This also assumes that there is an MSVC equivalent of
> "-include".

This feature appears to be the /FI flag:
  http://msdn.microsoft.com/en-us/library/8c5ztk84.aspx

That seems pretty appealing. I'd prefer to inject it if you're okay with that.

> 
> 
> 
> > Here are the two examples that show up a lot in my logs:
> > FAILED: ninja -t msvc -e environment.x64 --
> > "D:\src\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64\cl.exe"
> > /nologo
> > /showIncludes /FC @obj\native_client\src\shared\gio\gio64.gprintf.obj.rsp
> > /c
> > ..\..\native_client\src\shared\gio\gprintf.c
> > /Foobj\native_client\src\shared\gio\gio64.gprintf.obj
> > /Fdobj\native_client\src\shared\gio\gio64.c.pdb
> >
> > d:\src\depot_tools\win_toolchain\vs2013_files\win8sdk\include\um\
> > processthreadsapi.h(1170)
> > : error C4668: '_WIN32_WINNT_WINTHRESHOLD' is not defined as a preprocessor
> > macro, replacing with '0' for '#if/#elif'
> > d:\src\depot_tools\win_toolchain\vs2013_files\
> > win8sdk\include\um\winbase.h(8618)
> > : error C4668: 'NTDDI_WIN7SP1' is not defined as a preprocessor macro,
> > replacing
> > with '0' for '#if/#elif'
> >
> > I suppose I could just define those to 0 in the NaCl gyp files (that's the
> > behavior that we have today, after all), but that seems pretty ugly. Any
> > thoughts?
> 
> 
> That seems like it would be too brittle.  New #ifs might occur in new
> versions of the Windows headers.  Also, it would break cases where headers
> do "#ifdef FOO" and expect FOO to be undefined.
> 
> Cheers,
> Mark
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2014-12-18 17:46:35]

On 18 December 2014 at 08:49, <teravest@chromium.org> wrote:

> On 2014/12/17 18:59:06, Mark Seaborn wrote:
>
>> Another possibility would be to inject nacl_defines.h into all NaCl .c/.cc
>>
> files using "-include .../nacl_defines.h", to reduce the risk of forgetting
>> to #include it.  This also assumes that there is an MSVC equivalent of
>> "-include".
>>
>
> This feature appears to be the /FI flag:
>   http://msdn.microsoft.com/en-us/library/8c5ztk84.aspx
>
> That seems pretty appealing. I'd prefer to inject it if you're okay with
> that.


Yes, that's fine with me.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[teravest, 2014-12-19 17:35:07]

I've modified this change to use build_config.h for scons Windows builds as
well.
It now uses /FI to force the include of build_config.h as part of Windows
builds.

I didn't force header inclusion on Linux because I'd prefer that build_config.h
be explicitly included generally; the forced inclusion only happens on Windows
because we don't report errors for undefined preprocessor macros.

On Windows, I confirmed that NACL_BUILD_SUBARCH is set correctly as follows:
  scons platform=x86-64 => NACL_BUILD_SUBARCH=64
  scons platform=x86-32 => NACL_BUILD_SUBARCH=32

[Mark Seaborn, 2014-12-19 17:46:19]

https://codereview.chromium.org/788193003/diff/360001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/788193003/diff/360001/SConstruct#newcode2460
SConstruct:2460: '$SOURCE_ROOT/native_client/src/include/build_config.h']
Can you put '/FI' on the same line as this, to make it clear they go together?

Also, how about commenting why this is injected?

Can you add the "-include" for:

 * Linux/Mac
 * Gyp builds (via build/common.gypi)

I'd like to make sure this works in all these cases.

https://codereview.chromium.org/788193003/diff/360001/src/include/checked_cast.h
File src/include/checked_cast.h (right):

https://codereview.chromium.org/788193003/diff/360001/src/include/checked_cas...
src/include/checked_cast.h:17: #include
"native_client/src/include/build_config.h"
So it shouldn't be necessary to add this to all these .h/.c files any more,
should it?  (If you do this for the Linux/Mac builds too, I suppose.)

[teravest, 2014-12-19 18:39:39]

Patchset #5 (id:200002) has been deleted

[teravest, 2014-12-19 18:40:01]

Patchset #5 (id:390001) has been deleted

[teravest, 2014-12-19 18:50:38]

Patchset #5 (id:410001) has been deleted

[teravest, 2014-12-19 22:02:24]

Patchset #10 (id:530001) has been deleted

[teravest, 2014-12-23 18:31:00]

Patchset #10 (id:550001) has been deleted

[teravest, 2014-12-23 18:31:06]

Patchset #10 (id:560001) has been deleted

[teravest, 2014-12-23 18:31:19]

Patchset #10 (id:580001) has been deleted

[teravest, 2014-12-29 17:25:03]

On 2014/12/19 17:46:19, Mark Seaborn wrote:
> https://codereview.chromium.org/788193003/diff/360001/SConstruct
> File SConstruct (right):
> 
> https://codereview.chromium.org/788193003/diff/360001/SConstruct#newcode2460
> SConstruct:2460: '$SOURCE_ROOT/native_client/src/include/build_config.h']
> Can you put '/FI' on the same line as this, to make it clear they go together?
> 
> Also, how about commenting why this is injected?
> 
> Can you add the "-include" for:
> 
>  * Linux/Mac
>  * Gyp builds (via build/common.gypi)
> 
> I'd like to make sure this works in all these cases.
> 
>
https://codereview.chromium.org/788193003/diff/360001/src/include/checked_cast.h
> File src/include/checked_cast.h (right):
> 
>
https://codereview.chromium.org/788193003/diff/360001/src/include/checked_cas...
> src/include/checked_cast.h:17: #include
> "native_client/src/include/build_config.h"
> So it shouldn't be necessary to add this to all these .h/.c files any more,
> should it?  (If you do this for the Linux/Mac builds too, I suppose.)

[teravest, 2014-12-29 17:25:55]

On 2014/12/29 17:25:03, teravest wrote:
> On 2014/12/19 17:46:19, Mark Seaborn wrote:
> > https://codereview.chromium.org/788193003/diff/360001/SConstruct
> > File SConstruct (right):
> > 
> > https://codereview.chromium.org/788193003/diff/360001/SConstruct#newcode2460
> > SConstruct:2460: '$SOURCE_ROOT/native_client/src/include/build_config.h']
> > Can you put '/FI' on the same line as this, to make it clear they go
together?
> > 
> > Also, how about commenting why this is injected?
> > 
> > Can you add the "-include" for:
> > 
> >  * Linux/Mac
> >  * Gyp builds (via build/common.gypi)
> > 
> > I'd like to make sure this works in all these cases.

Done. I've added linux/mac SCons support and modified this for gyp builds as
well.

> > 
> >
>
https://codereview.chromium.org/788193003/diff/360001/src/include/checked_cast.h
> > File src/include/checked_cast.h (right):
> > 
> >
>
https://codereview.chromium.org/788193003/diff/360001/src/include/checked_cas...
> > src/include/checked_cast.h:17: #include
> > "native_client/src/include/build_config.h"
> > So it shouldn't be necessary to add this to all these .h/.c files any more,
> > should it?  (If you do this for the Linux/Mac builds too, I suppose.)

Correct. I've removed the changes to add the header explicitly to many files.

[Mark Seaborn, 2014-12-29 17:51:04]

Don't forget to update the commit message to refer to build_config.h.

https://codereview.chromium.org/788193003/diff/660001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/788193003/diff/660001/SConstruct#newcode2460
SConstruct:2460: # build_config.h is injected as a header in all sources to
Nit: indent by 1 space less

https://codereview.chromium.org/788193003/diff/660001/SConstruct#newcode3112
SConstruct:3112: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
Can you inject build_config.h for untrusted code too?

https://codereview.chromium.org/788193003/diff/660001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/788193003/diff/660001/build/common.gypi#newco...
build/common.gypi:535: '-include', 'native_client/src/include/build_config.h'
Nit: indent by 2 spaces rather than 4

https://codereview.chromium.org/788193003/diff/660001/src/include/build_config.h
File src/include/build_config.h (right):

https://codereview.chromium.org/788193003/diff/660001/src/include/build_confi...
src/include/build_config.h:12: /* TODO(teravest): Remove this guard when builds
stop defining these. */
To be more specific, "when the Chromium build stops defining these via
components/nacl/nacl_defines.gypi", perhaps?

I suppose this guard would still be necessary for assembly files, but not C/C++
files, when building Chromium?

https://codereview.chromium.org/788193003/diff/660001/src/include/build_confi...
src/include/build_config.h:35: #if !defined(NACL_ANDROID)
Why is this checked separately from NACL_WINDOWS/LINUX/OSX above?

https://codereview.chromium.org/788193003/diff/660001/src/include/build_confi...
src/include/build_config.h:44: #if defined(NACL_BUILD_ARCH) &&
(NACL_ARCH(NACL_BUILD_ARCH) != NACL_pnacl) && !defined(NACL_BUILD_SUBARCH)
Line is >80 chars; please wrap

https://codereview.chromium.org/788193003/diff/660001/src/include/build_confi...
src/include/build_config.h:75: /* TODO(teravest): Require NACL_BUILD_ARCH and
NACL_BUILD_SUBARCH to be defined
Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
lines

[teravest, 2014-12-29 18:07:59]

Patchset #14 (id:680001) has been deleted

[teravest, 2014-12-29 22:29:29]

https://codereview.chromium.org/788193003/diff/660001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/788193003/diff/660001/SConstruct#newcode2460
SConstruct:2460: # build_config.h is injected as a header in all sources to
On 2014/12/29 17:51:03, Mark Seaborn wrote:
> Nit: indent by 1 space less

Done.

https://codereview.chromium.org/788193003/diff/660001/SConstruct#newcode3112
SConstruct:3112: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
On 2014/12/29 17:51:03, Mark Seaborn wrote:
> Can you inject build_config.h for untrusted code too?

I'm getting some strange errors when doing so:
/usr/local/google/nacl_repo/native_client/src/include/build_config.h:10:49:
error: native_client/src/include/nacl_base.h: No such file or directory
/usr/local/google/nacl_repo/native_client/src/include/build_config.h:42:34:
error: "NACL_ARCH" is not defined
/usr/local/google/nacl_repo/native_client/src/include/build_config.h:42:43:
error: missing binary operator before token "("

I'm not sure why it can't find nacl_base.h in the untrusted build. There's
probably something tricky that has to happen with the includes, but I'd prefer
not to mess with the untrusted build too much.

https://codereview.chromium.org/788193003/diff/660001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/788193003/diff/660001/build/common.gypi#newco...
build/common.gypi:535: '-include', 'native_client/src/include/build_config.h'
On 2014/12/29 17:51:04, Mark Seaborn wrote:
> Nit: indent by 2 spaces rather than 4

Done.

https://codereview.chromium.org/788193003/diff/660001/src/include/build_config.h
File src/include/build_config.h (right):

https://codereview.chromium.org/788193003/diff/660001/src/include/build_confi...
src/include/build_config.h:12: /* TODO(teravest): Remove this guard when builds
stop defining these. */
On 2014/12/29 17:51:04, Mark Seaborn wrote:
> To be more specific, "when the Chromium build stops defining these via
> components/nacl/nacl_defines.gypi", perhaps?
> 
> I suppose this guard would still be necessary for assembly files, but not
C/C++
> files, when building Chromium?

I've removed this TODO since we might need this guard for assembly files.

https://codereview.chromium.org/788193003/diff/660001/src/include/build_confi...
src/include/build_config.h:35: #if !defined(NACL_ANDROID)
On 2014/12/29 17:51:04, Mark Seaborn wrote:
> Why is this checked separately from NACL_WINDOWS/LINUX/OSX above?

I've lumped this in with the rest.

https://codereview.chromium.org/788193003/diff/660001/src/include/build_confi...
src/include/build_config.h:44: #if defined(NACL_BUILD_ARCH) &&
(NACL_ARCH(NACL_BUILD_ARCH) != NACL_pnacl) && !defined(NACL_BUILD_SUBARCH)
On 2014/12/29 17:51:04, Mark Seaborn wrote:
> Line is >80 chars; please wrap

Done.

https://codereview.chromium.org/788193003/diff/660001/src/include/build_confi...
src/include/build_config.h:75: /* TODO(teravest): Require NACL_BUILD_ARCH and
NACL_BUILD_SUBARCH to be defined
On 2014/12/29 17:51:04, Mark Seaborn wrote:
> Nit: Use the NaCl style for multiline comments, with "/*" and "*/" on separate
> lines

Done.

[Mark Seaborn, 2015-01-02 04:18:31]

https://codereview.chromium.org/788193003/diff/660001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/788193003/diff/660001/SConstruct#newcode3112
SConstruct:3112: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
On 2014/12/29 22:29:28, teravest wrote:
> On 2014/12/29 17:51:03, Mark Seaborn wrote:
> > Can you inject build_config.h for untrusted code too?
> 
> I'm getting some strange errors when doing so:
> /usr/local/google/nacl_repo/native_client/src/include/build_config.h:10:49:
> error: native_client/src/include/nacl_base.h: No such file or directory
> /usr/local/google/nacl_repo/native_client/src/include/build_config.h:42:34:
> error: "NACL_ARCH" is not defined
> /usr/local/google/nacl_repo/native_client/src/include/build_config.h:42:43:
> error: missing binary operator before token "("
> 
> I'm not sure why it can't find nacl_base.h in the untrusted build. There's
> probably something tricky that has to happen with the includes, but I'd prefer
> not to mess with the untrusted build too much.

That is from the Scons build, right?

Does it fail like that on the trybots too, or did you only try it locally?  Can
you share the patch you tried so that I can try to debug it?

If you run Scons with --verbose, what is the command line that fails?  (I'm
wondering what "-I" arguments are passed.)

While it looks like you don't need to change the untrusted build for now (since
build_nexe.py sets NACL_BUILD_ARCH OK), I don't like mysteries like this getting
in the way of making things consistent. :-)

[teravest, 2015-01-05 16:50:20]

On 2015/01/02 04:18:31, Mark Seaborn wrote:
> https://codereview.chromium.org/788193003/diff/660001/SConstruct
> File SConstruct (right):
> 
> https://codereview.chromium.org/788193003/diff/660001/SConstruct#newcode3112
> SConstruct:3112: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
> On 2014/12/29 22:29:28, teravest wrote:
> > On 2014/12/29 17:51:03, Mark Seaborn wrote:
> > > Can you inject build_config.h for untrusted code too?
> > 
> > I'm getting some strange errors when doing so:
> > /usr/local/google/nacl_repo/native_client/src/include/build_config.h:10:49:
> > error: native_client/src/include/nacl_base.h: No such file or directory
> > /usr/local/google/nacl_repo/native_client/src/include/build_config.h:42:34:
> > error: "NACL_ARCH" is not defined
> > /usr/local/google/nacl_repo/native_client/src/include/build_config.h:42:43:
> > error: missing binary operator before token "("
> > 
> > I'm not sure why it can't find nacl_base.h in the untrusted build. There's
> > probably something tricky that has to happen with the includes, but I'd
prefer
> > not to mess with the untrusted build too much.
> 
> That is from the Scons build, right?

That's correct.

> 
> Does it fail like that on the trybots too, or did you only try it locally? 
Can
> you share the patch you tried so that I can try to debug it?

This fails in the same way locally and on trybots. I've uploaded this as patch
set #16.

> 
> If you run Scons with --verbose, what is the command line that fails?  (I'm
> wondering what "-I" arguments are passed.)

Here's the relevant command line:
/usr/local/google/nacl_repo/native_client/toolchain/linux_x86/nacl_x86_newlib_raw/bin/x86_64-nacl-gcc
-m32 -std=gnu99 -Wstrict-prototypes -O2 -g -fomit-frame-pointer
-fasynchronous-unwind-tables -Wall -Wundef -fdiagnostics-show-option -pedantic
-Werror -Wno-variadic-macros -include
/usr/local/google/nacl_repo/native_client/src/include/build_config.h
-D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D_GNU_SOURCE=1
-D_DEFAULT_SOURCE=1 -D_BSD_SOURCE=1 -D_POSIX_C_SOURCE=199506 -D_XOPEN_SOURCE=600
-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DDYNAMIC_ANNOTATIONS_PREFIX=NACL_
-DNACL_WINDOWS=0 -DNACL_OSX=0 -DNACL_LINUX=0 -DNACL_ANDROID=0
-DNACL_BUILD_ARCH=x86 -DNACL_BUILD_SUBARCH=32 -Iscons-out/nacl-x86-32/include -c
-o scons-out/nacl-x86-32/obj/tests/includability/use_irt.o
tests/includability/use_irt.c

> 
> While it looks like you don't need to change the untrusted build for now
(since
> build_nexe.py sets NACL_BUILD_ARCH OK), I don't like mysteries like this
getting
> in the way of making things consistent. :-)


I have a local hack that gets around this build problem where I set
NACL_BUILD_SUBARCH=0 when NACL_BUILD_ARCH=pnacl in build/build_nexe.py. This
allows the use of NACL_ARCH() and the inclusion of nacl_base.h to be removed in
src/include/build_config.h. Not sure if that's too gross, though.

[Mark Seaborn, 2015-01-05 18:20:20]

On 5 January 2015 at 08:50, <teravest@chromium.org> wrote:

> On 2015/01/02 04:18:31, Mark Seaborn wrote:
>
>> If you run Scons with --verbose, what is the command line that fails?
>> (I'm
>>
> wondering what "-I" arguments are passed.)
>>
>
> Here's the relevant command line:
> /usr/local/google/nacl_repo/native_client/toolchain/linux_
> x86/nacl_x86_newlib_raw/bin/x86_64-nacl-gcc
> -m32 -std=gnu99 -Wstrict-prototypes -O2 -g -fomit-frame-pointer
> -fasynchronous-unwind-tables -Wall -Wundef -fdiagnostics-show-option
> -pedantic
> -Werror -Wno-variadic-macros -include
> /usr/local/google/nacl_repo/native_client/src/include/build_config.h
> -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D_GNU_SOURCE=1
> -D_DEFAULT_SOURCE=1 -D_BSD_SOURCE=1 -D_POSIX_C_SOURCE=199506
> -D_XOPEN_SOURCE=600
> -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DDYNAMIC_ANNOTATIONS_PREFIX=NACL_
> -DNACL_WINDOWS=0 -DNACL_OSX=0 -DNACL_LINUX=0 -DNACL_ANDROID=0
> -DNACL_BUILD_ARCH=x86 -DNACL_BUILD_SUBARCH=32 -Iscons-out/nacl-x86-32/include
> -c
> -o scons-out/nacl-x86-32/obj/tests/includability/use_irt.o
> tests/includability/use_irt.c


I see the problem.  The trybot logs indicate that only one file,
tests/includability/use_irt.c, fails to compile, and that is compiled with:

# Check that headers are #includable without native_client/ being in
# the #include search path.
env.FilterOut(CPPPATH='${SOURCE_ROOT}')

You'd need to add another FilterOut() there to remove the "-include" option.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[teravest, 2015-01-05 21:36:19]

Patchset #18 (id:780001) has been deleted

[teravest, 2015-01-05 22:21:55]

On 2015/01/05 18:20:20, Mark Seaborn wrote:
> On 5 January 2015 at 08:50, <mailto:teravest@chromium.org> wrote:
> 
> > On 2015/01/02 04:18:31, Mark Seaborn wrote:
> >
> >> If you run Scons with --verbose, what is the command line that fails?
> >> (I'm
> >>
> > wondering what "-I" arguments are passed.)
> >>
> >
> > Here's the relevant command line:
> > /usr/local/google/nacl_repo/native_client/toolchain/linux_
> > x86/nacl_x86_newlib_raw/bin/x86_64-nacl-gcc
> > -m32 -std=gnu99 -Wstrict-prototypes -O2 -g -fomit-frame-pointer
> > -fasynchronous-unwind-tables -Wall -Wundef -fdiagnostics-show-option
> > -pedantic
> > -Werror -Wno-variadic-macros -include
> > /usr/local/google/nacl_repo/native_client/src/include/build_config.h
> > -D__STDC_LIMIT_MACROS=1 -D__STDC_FORMAT_MACROS=1 -D_GNU_SOURCE=1
> > -D_DEFAULT_SOURCE=1 -D_BSD_SOURCE=1 -D_POSIX_C_SOURCE=199506
> > -D_XOPEN_SOURCE=600
> > -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DDYNAMIC_ANNOTATIONS_PREFIX=NACL_
> > -DNACL_WINDOWS=0 -DNACL_OSX=0 -DNACL_LINUX=0 -DNACL_ANDROID=0
> > -DNACL_BUILD_ARCH=x86 -DNACL_BUILD_SUBARCH=32
-Iscons-out/nacl-x86-32/include
> > -c
> > -o scons-out/nacl-x86-32/obj/tests/includability/use_irt.o
> > tests/includability/use_irt.c
> 
> 
> I see the problem.  The trybot logs indicate that only one file,
> tests/includability/use_irt.c, fails to compile, and that is compiled with:
> 
> # Check that headers are #includable without native_client/ being in
> # the #include search path.
> env.FilterOut(CPPPATH='${SOURCE_ROOT}')
> 
> You'd need to add another FilterOut() there to remove the "-include" option.

thanks, done!

I removed the OS defines from nacl_env, but I'm not sure how to safely remove
NACL_BUILD_ARCH. Does pnacl-clang provide any preprocessor defines (say,
__pnacl__) to detect if the target "architecture" is pnacl? If I remove the
define for NACL_BUILD_ARCH, I get failures on some bots.

> 
> Cheers,
> Mark
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:native-client-reviews+unsubscribe@googlegroups.com.
> To post to this group, send email to
mailto:native-client-reviews@googlegroups.com.
> Visit this group at http://groups.google.com/group/native-client-reviews.
> For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2015-01-06 00:59:19]

On 5 January 2015 at 14:21, <teravest@chromium.org> wrote:

> I removed the OS defines from nacl_env, but I'm not sure how to safely
> remove
> NACL_BUILD_ARCH. Does pnacl-clang provide any preprocessor defines (say,
> __pnacl__) to detect if the target "architecture" is pnacl? If I remove the
> define for NACL_BUILD_ARCH, I get failures on some bots.


Yes, you can check for "defined(__pnacl__)" and set NACL_BUILD_ARCH=pnacl.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[teravest, 2015-01-06 16:47:55]

For now, I think I have to set NACL_BUILD_ARCH / NACL_BUILD_SUBARCH in
SConstruct for the case of nacl_env. I tried to detect "__pnacl__" from the
compiler, and set NACL_BUILD_ARCH to "pnacl" in that case, but I believe that's
incorrect. Doing so causes tests/mmap_prot_exec/mmap_prot_exec.c to error out
at line 96.

Reading back through SConstruct, it looks like NACL_BUILD_ARCH should be set to
the running architecture of the machine, as set at pynacl/platform.py:117. This
can't give pnacl, but only x86/arm/mips as a result. However, I don't think I 
can determine the correct value from inside build_config.h; from a pnacl build,
there's nothing to show what the underlying architecture of the build machine 
is.

Is it a bug that tests/mmap_prot_exec/mmap_prot_exec.c is being built in the 
nacl_env with pnacl case? Or should that test support the pnacl NACL_BUILD_ARCH
somehow?

[Mark Seaborn, 2015-01-06 17:15:37]

On 6 January 2015 at 08:47, <teravest@chromium.org> wrote:

> For now, I think I have to set NACL_BUILD_ARCH / NACL_BUILD_SUBARCH in
> SConstruct for the case of nacl_env. I tried to detect "__pnacl__" from the
> compiler, and set NACL_BUILD_ARCH to "pnacl" in that case, but I believe
> that's
> incorrect. Doing so causes tests/mmap_prot_exec/mmap_prot_exec.c to error
> out
> at line 96.


You can deal with that test by adding this to
tests/mmap_prot_exec/nacl.scons:

if env.Bit('bitcode'):
  env.AddBiasForPNaCl()

That would be similar to tests/data_not_executable/, which happens to test
for __i386__ etc. rather than NACL_BUILD_(SUB)ARCH.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[teravest, 2015-01-06 18:07:29]

On 2015/01/06 17:15:37, Mark Seaborn wrote:
> On 6 January 2015 at 08:47, <mailto:teravest@chromium.org> wrote:
> 
> > For now, I think I have to set NACL_BUILD_ARCH / NACL_BUILD_SUBARCH in
> > SConstruct for the case of nacl_env. I tried to detect "__pnacl__" from the
> > compiler, and set NACL_BUILD_ARCH to "pnacl" in that case, but I believe
> > that's
> > incorrect. Doing so causes tests/mmap_prot_exec/mmap_prot_exec.c to error
> > out
> > at line 96.
> 
> 
> You can deal with that test by adding this to
> tests/mmap_prot_exec/nacl.scons:
> 
> if env.Bit('bitcode'):
>   env.AddBiasForPNaCl()
> 
> That would be similar to tests/data_not_executable/, which happens to test
> for __i386__ etc. rather than NACL_BUILD_(SUB)ARCH.

Thanks, that was enough to resolve the issue for that test. I'm apparently
running into other build errors in the nacl_env case:
 
http://build.chromium.org/p/tryserver.nacl/builders/nacl-precise_64-newlib-x8...

WARNING: Linking two modules of different target triples: ' is
'i686-unknown-nacl' whereas 'ld-temp.o' is 'le32-unknown-nacl'

Presumably setting NACL_BUILD_ARCH for __pnacl__ builds is still wrong and there
are tools that depend on the NACL_BUILD_ARCH agreeing with the architecture of
the building system. Is it actually correct to set it to pnacl in this case? It
doesn't seem too bad to set NACL_BUILD_ARCH / NACL_BUILD_SUBARCH in SConstruct
for nacl_env if build_config.h doesn't have sufficient information to set it
properly.

[Mark Seaborn, 2015-01-06 18:31:54]

On 6 January 2015 at 10:07, <teravest@chromium.org> wrote:

> On 2015/01/06 17:15:37, Mark Seaborn wrote:
>
>  On 6 January 2015 at 08:47, <mailto:teravest@chromium.org> wrote:
>>
>
>  > For now, I think I have to set NACL_BUILD_ARCH / NACL_BUILD_SUBARCH in
>> > SConstruct for the case of nacl_env. I tried to detect "__pnacl__" from
>> the
>> > compiler, and set NACL_BUILD_ARCH to "pnacl" in that case, but I believe
>> > that's
>> > incorrect. Doing so causes tests/mmap_prot_exec/mmap_prot_exec.c to
>> error
>> > out
>> > at line 96.
>>
>
>
>  You can deal with that test by adding this to
>> tests/mmap_prot_exec/nacl.scons:
>>
>
>  if env.Bit('bitcode'):
>>    env.AddBiasForPNaCl()
>>
>
>  That would be similar to tests/data_not_executable/, which happens to test
>> for __i386__ etc. rather than NACL_BUILD_(SUB)ARCH.
>>
>
> Thanks, that was enough to resolve the issue for that test. I'm apparently
> running into other build errors in the nacl_env case:
>
> http://build.chromium.org/p/tryserver.nacl/builders/nacl-
> precise_64-newlib-x86_32-pnacl/builds/3239/steps/
> nonsfi_tests%20x86-32/logs/stdio
>
> WARNING: Linking two modules of different target triples: ' is
> 'i686-unknown-nacl' whereas 'ld-temp.o' is 'le32-unknown-nacl'


That's just a warning.  The failure is actually this:

src/nonsfi/loader/elf_loader.c:56:26: error: use of undeclared identifier
'NACL_ELF_E_MACHINE'
  if (ehdr->e_machine != NACL_ELF_E_MACHINE) {

OK, let's keep SConstruct defining NACL_BUILD_(SUB)ARCH in nacl_env for
now.  I agree that trying to remove this runs into a few problems.  We can
try to change that incrementally later.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[teravest, 2015-01-06 21:10:39]

On 2015/01/06 18:31:54, Mark Seaborn wrote:
> On 6 January 2015 at 10:07, <mailto:teravest@chromium.org> wrote:
> 
> > On 2015/01/06 17:15:37, Mark Seaborn wrote:
> >
> >  On 6 January 2015 at 08:47, <mailto:teravest@chromium.org> wrote:
> >>
> >
> >  > For now, I think I have to set NACL_BUILD_ARCH / NACL_BUILD_SUBARCH in
> >> > SConstruct for the case of nacl_env. I tried to detect "__pnacl__" from
> >> the
> >> > compiler, and set NACL_BUILD_ARCH to "pnacl" in that case, but I believe
> >> > that's
> >> > incorrect. Doing so causes tests/mmap_prot_exec/mmap_prot_exec.c to
> >> error
> >> > out
> >> > at line 96.
> >>
> >
> >
> >  You can deal with that test by adding this to
> >> tests/mmap_prot_exec/nacl.scons:
> >>
> >
> >  if env.Bit('bitcode'):
> >>    env.AddBiasForPNaCl()
> >>
> >
> >  That would be similar to tests/data_not_executable/, which happens to test
> >> for __i386__ etc. rather than NACL_BUILD_(SUB)ARCH.
> >>
> >
> > Thanks, that was enough to resolve the issue for that test. I'm apparently
> > running into other build errors in the nacl_env case:
> >
> > http://build.chromium.org/p/tryserver.nacl/builders/nacl-
> > precise_64-newlib-x86_32-pnacl/builds/3239/steps/
> > nonsfi_tests%20x86-32/logs/stdio
> >
> > WARNING: Linking two modules of different target triples: ' is
> > 'i686-unknown-nacl' whereas 'ld-temp.o' is 'le32-unknown-nacl'
> 
> 
> That's just a warning.  The failure is actually this:
> 
> src/nonsfi/loader/elf_loader.c:56:26: error: use of undeclared identifier
> 'NACL_ELF_E_MACHINE'
>   if (ehdr->e_machine != NACL_ELF_E_MACHINE) {
> 
> OK, let's keep SConstruct defining NACL_BUILD_(SUB)ARCH in nacl_env for
> now.  I agree that trying to remove this runs into a few problems.  We can
> try to change that incrementally later.

Thanks, Mark. I've added a patchset (#24) where SConstruct defines
NACL_BUILD_(SUB)ARCH in nacl_env.

[Mark Seaborn, 2015-01-07 20:52:39]

LGTM.  Please check that this works OK in Chromium before committing.

https://codereview.chromium.org/788193003/diff/910001/SConstruct
File SConstruct (left):

https://codereview.chromium.org/788193003/diff/910001/SConstruct#oldcode2634
SConstruct:2634: env.Prepend(CPPDEFINES=[['NACL_ANDROID', '1']])
Don't you need to "#define ANDROID" as a replacement, as you do in the Gyp file?

https://codereview.chromium.org/788193003/diff/910001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/788193003/diff/910001/SConstruct#newcode3108
SConstruct:3108: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
Nit: remove space before ']'

https://codereview.chromium.org/788193003/diff/910001/SConstruct#newcode3112
SConstruct:3112: '-include',
'$SOURCE_ROOT/native_client/src/include/build_config.h' ],
Nit: remove space before ']' and indent by 4 spaces rather than 2

https://codereview.chromium.org/788193003/diff/910001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/788193003/diff/910001/build/common.gypi#newco...
build/common.gypi:311: 'defines!': ['ANDROID'],
This "defines!" entry is not needed, I think.

https://codereview.chromium.org/788193003/diff/910001/build/common.gypi#newco...
build/common.gypi:315: 'defines!': ['ANDROID'],
Ditto

https://codereview.chromium.org/788193003/diff/910001/src/include/build_config.h
File src/include/build_config.h (right):

https://codereview.chromium.org/788193003/diff/910001/src/include/build_confi...
src/include/build_config.h:43: !defined(NACL_BUILD_SUBARCH)
Indent to line up with "("

https://codereview.chromium.org/788193003/diff/910001/tests/includability/nac...
File tests/includability/nacl.scons (right):

https://codereview.chromium.org/788193003/diff/910001/tests/includability/nac...
tests/includability/nacl.scons:15: '-include',
'$SOURCE_ROOT/native_client/src/include/build_config.h'])
Nit: Either add an empty line after this or remove the one before

[Mark Seaborn, 2015-01-07 23:22:34]

https://codereview.chromium.org/788193003/diff/910001/src/include/build_config.h
File src/include/build_config.h (right):

https://codereview.chromium.org/788193003/diff/910001/src/include/build_confi...
src/include/build_config.h:43: !defined(NACL_BUILD_SUBARCH)
On 2015/01/07 20:52:39, Mark Seaborn wrote:
> Indent to line up with "("

Oops, sorry, my mistake.  I missed the closing ")" on the line before.

[teravest, 2015-01-08 17:17:13]

I patched this into a local chromium build and browser_tests built successfully.

https://codereview.chromium.org/788193003/diff/910001/SConstruct
File SConstruct (left):

https://codereview.chromium.org/788193003/diff/910001/SConstruct#oldcode2634
SConstruct:2634: env.Prepend(CPPDEFINES=[['NACL_ANDROID', '1']])
On 2015/01/07 20:52:38, Mark Seaborn wrote:
> Don't you need to "#define ANDROID" as a replacement, as you do in the Gyp
file?

ANDROID is already defined, see "-DANDROID" in CCLFAGS at line 2719.

https://codereview.chromium.org/788193003/diff/910001/SConstruct
File SConstruct (right):

https://codereview.chromium.org/788193003/diff/910001/SConstruct#newcode3108
SConstruct:3108: ['NACL_BUILD_ARCH', '${BUILD_ARCHITECTURE}' ],
On 2015/01/07 20:52:38, Mark Seaborn wrote:
> Nit: remove space before ']'

Done.

https://codereview.chromium.org/788193003/diff/910001/SConstruct#newcode3112
SConstruct:3112: '-include',
'$SOURCE_ROOT/native_client/src/include/build_config.h' ],
On 2015/01/07 20:52:38, Mark Seaborn wrote:
> Nit: remove space before ']' and indent by 4 spaces rather than 2

Done.

https://codereview.chromium.org/788193003/diff/910001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/788193003/diff/910001/build/common.gypi#newco...
build/common.gypi:311: 'defines!': ['ANDROID'],
On 2015/01/07 20:52:38, Mark Seaborn wrote:
> This "defines!" entry is not needed, I think.

Removed.

https://codereview.chromium.org/788193003/diff/910001/build/common.gypi#newco...
build/common.gypi:315: 'defines!': ['ANDROID'],
On 2015/01/07 20:52:38, Mark Seaborn wrote:
> Ditto

Removed.

https://codereview.chromium.org/788193003/diff/910001/src/include/build_config.h
File src/include/build_config.h (right):

https://codereview.chromium.org/788193003/diff/910001/src/include/build_confi...
src/include/build_config.h:43: !defined(NACL_BUILD_SUBARCH)
On 2015/01/07 23:22:33, Mark Seaborn wrote:
> On 2015/01/07 20:52:39, Mark Seaborn wrote:
> > Indent to line up with "("
> 
> Oops, sorry, my mistake.  I missed the closing ")" on the line before.

Removed the unnecessary parens around:
  NACL_ARCH(NACL_BUILD_ARCH) != NACL_pnacl

https://codereview.chromium.org/788193003/diff/910001/tests/includability/nac...
File tests/includability/nacl.scons (right):

https://codereview.chromium.org/788193003/diff/910001/tests/includability/nac...
tests/includability/nacl.scons:15: '-include',
'$SOURCE_ROOT/native_client/src/include/build_config.h'])
On 2015/01/07 20:52:39, Mark Seaborn wrote:
> Nit: Either add an empty line after this or remove the one before

Added an empty line after.

[teravest, 2015-01-08 21:30:33]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2015-01-08 21:31:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/788193003/930001

[commit-bot: I haz the power, 2015-01-08 23:17:37]

Committed patchset #25 (id:930001) as
http://src.chromium.org/viewvc/native_client?view=rev&revision=916e3c83dcf343...