NaCl-side GN changes to enable NaCl in Linux x64 Chrome builds.

Nacl-side GN changes to enable NaCl in Linux x64 Chrome builds.

This adds the sel_ldr_launcher_base target (needed for Chrome's 
implementation of the IRT) and the serialization target, and fixes a 
couple of other minor issues.

R=ncbray@chromium.org

Committed: https://chromium.googlesource.com/native_client/src/native_client/+/8387fef24018c05a6b14d3788bce5e065681c5d5

[Dirk Pranke, 2015-02-04 23:26:28]

Patchset #1 (id:1) has been deleted

[Dirk Pranke, 2015-02-04 23:26:36]

Patchset #1 (id:20001) has been deleted

[Dirk Pranke, 2015-02-04 23:26:51]

https://codereview.chromium.org/893993005/diff/40001/build/toolchain/nacl/BUI...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/40001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:150: ld = "${python_path} ${link_irt}
--tls-edit=tls_edit --link-cmd=${cxx} --readelf-cmd=${readelf}"
This change is needed because Chrome's implementation of the IRT contains C++
code.

https://codereview.chromium.org/893993005/diff/40001/src/trusted/service_runt...
File src/trusted/service_runtime/BUILD.gn (left):

https://codereview.chromium.org/893993005/diff/40001/src/trusted/service_runt...
src/trusted/service_runtime/BUILD.gn:102: }
These files are also pulled in to the service_runtime/arch/* targets, causing
multiple-definition errors; the GYP version of this target does not include
these files.

[Roland McGrath, 2015-02-04 23:31:36]

mcgrathr@chromium.org changed reviewers:
+ mcgrathr@chromium.org

[Roland McGrath, 2015-02-04 23:31:37]

https://codereview.chromium.org/893993005/diff/40001/build/toolchain/nacl/BUI...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/40001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:150: ld = "${python_path} ${link_irt}
--tls-edit=tls_edit --link-cmd=${cxx} --readelf-cmd=${readelf}"
On 2015/02/04 23:26:51, Dirk Pranke wrote:
> This change is needed because Chrome's implementation of the IRT contains C++
> code.

Put comments in the code, not in the review.  The existing comment in the code
says the exact opposite, so change it to match.

[Dirk Pranke, 2015-02-04 23:35:36]

https://codereview.chromium.org/893993005/diff/40001/build/toolchain/nacl/BUI...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/40001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:150: ld = "${python_path} ${link_irt}
--tls-edit=tls_edit --link-cmd=${cxx} --readelf-cmd=${readelf}"
On 2015/02/04 23:31:36, Roland McGrath wrote:
> On 2015/02/04 23:26:51, Dirk Pranke wrote:
> > This change is needed because Chrome's implementation of the IRT contains
C++
> > code.
> 
> Put comments in the code, not in the review.  The existing comment in the code
> says the exact opposite, so change it to match.

Ah, good point. Will do.

[Nick Bray (chromium), 2015-02-05 22:52:55]

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:129: # Some IRT implementations (notably,
Chromium's) contain C++ code,
Roland: are we cool with irt_core also being linked this way?

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:150: # Link with cc to avoid including any C++
libs.
Same comment, same change.

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:170: # Link with cc to avoid including any C++
libs.
This code is sort of dead right now, but do the same change?

https://codereview.chromium.org/893993005/diff/60001/src/shared/serialization...
File src/shared/serialization/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/60001/src/shared/serialization...
src/shared/serialization/BUILD.gn:5:
import("//components/nacl/nacl_defines.gni")
Is there a //components directory in the NaCl repo?

https://codereview.chromium.org/893993005/diff/60001/src/shared/serialization...
src/shared/serialization/BUILD.gn:8: defines = nacl_defines
I believe the correct way to get these defines is to depend on 
"//native_client/build/config/nacl:nacl_base"
In general, every target in the NaCl repo should depend on this target. 
(Including the other ones you added.)

https://codereview.chromium.org/893993005/diff/60001/src/trusted/nonnacl_util...
File src/trusted/nonnacl_util/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/60001/src/trusted/nonnacl_util...
src/trusted/nonnacl_util/BUILD.gn:13: defines = nacl_defines
Dito.

https://codereview.chromium.org/893993005/diff/60001/src/trusted/service_runt...
File src/trusted/service_runtime/BUILD.gn (left):

https://codereview.chromium.org/893993005/diff/60001/src/trusted/service_runt...
src/trusted/service_runtime/BUILD.gn:97: if (cpu_arch == "x86") {
Curiosity: why?

[Dirk Pranke, 2015-02-05 22:55:48]

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:150: # Link with cc to avoid including any C++
libs.
On 2015/02/05 22:52:55, Nick Bray (chromium) wrote:
> Same comment, same change.

Ah, I missed the duplication. Will fix.

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:170: # Link with cc to avoid including any C++
libs.
On 2015/02/05 22:52:55, Nick Bray (chromium) wrote:
> This code is sort of dead right now, but do the same change?

Will do.

https://codereview.chromium.org/893993005/diff/60001/src/shared/serialization...
File src/shared/serialization/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/60001/src/shared/serialization...
src/shared/serialization/BUILD.gn:5:
import("//components/nacl/nacl_defines.gni")
On 2015/02/05 22:52:55, Nick Bray (chromium) wrote:
> Is there a //components directory in the NaCl repo?

Whoops. No, of course not. I meant to clean this up but forgot. Will do.

https://codereview.chromium.org/893993005/diff/60001/src/shared/serialization...
src/shared/serialization/BUILD.gn:8: defines = nacl_defines
On 2015/02/05 22:52:55, Nick Bray (chromium) wrote:
> I believe the correct way to get these defines is to depend on 
> "//native_client/build/config/nacl:nacl_base"
> In general, every target in the NaCl repo should depend on this target. 
> (Including the other ones you added.)

Will do.

https://codereview.chromium.org/893993005/diff/60001/src/trusted/nonnacl_util...
File src/trusted/nonnacl_util/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/60001/src/trusted/nonnacl_util...
src/trusted/nonnacl_util/BUILD.gn:13: defines = nacl_defines
On 2015/02/05 22:52:55, Nick Bray (chromium) wrote:
> Dito.

Will do.

https://codereview.chromium.org/893993005/diff/60001/src/trusted/service_runt...
File src/trusted/service_runtime/BUILD.gn (left):

https://codereview.chromium.org/893993005/diff/60001/src/trusted/service_runt...
src/trusted/service_runtime/BUILD.gn:97: if (cpu_arch == "x86") {
On 2015/02/05 22:52:55, Nick Bray (chromium) wrote:
> Curiosity: why?

Oh, I explained this in a comment on patchset #1: 

"These files are also pulled in to the service_runtime/arch/* targets, causing
multiple-definition errors; the GYP version of this target does not include
these files."

[Roland McGrath, 2015-02-05 22:58:02]

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/893993005/diff/60001/build/toolchain/nacl/BUI...
build/toolchain/nacl/BUILD.gn:129: # Some IRT implementations (notably,
Chromium's) contain C++ code,
On 2015/02/05 22:52:55, Nick Bray (chromium) wrote:
> Roland: are we cool with irt_core also being linked this way?

It's probably fine, but I'd like to see the comparison of the irt_core.nexe
produced one way vs the other.  If it's identical, then clearly it's fine.  If
it bloats a little, probably still fine, but I want to grok the details.

[Dirk Pranke, 2015-02-06 03:22:50]

Please take another look? I think I've addressed all of the review feedback
except for the binary size issue Roland requested (which I will try to get on
Friday).

[Nick Bray (chromium), 2015-02-07 02:18:59]

LGTM w/ no size regressions.

[Dirk Pranke, 2015-02-09 23:11:43]

On 2015/02/07 02:18:59, Nick Bray (chromium) wrote:
> LGTM w/ no size regressions.

Okay, building the irt_x64/nacl_core.exe with $(cc) and with $(cxx) appears to
have produced the exact same binary. I will land this and we can see if anything
else arises.

[Dirk Pranke, 2015-02-09 23:11:46]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-02-09 23:12:27]

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

[commit-bot: I haz the power, 2015-02-10 17:00:11]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/native_client/src/native_client/+/8387fef24...