Don't build NaCl's syscall handler into a shared library

Don't build NaCl's syscall handler into a shared library

Fixes shared library Chromium build on x86-64 on Linux.

BUG=35829
TEST=build

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=39729

[Mark Seaborn, 2010-02-23 10:58:32]

This fixes the build but it may not be ideal if it statically links more than is
necessary.

However, as I understand it, the chromium executable usually links in all of
NaCl's sel_ldr, and it does not invoke sel_ldr at run time (despite the fact
that there is also a sel_ldr target in the Chromium build).  Greg, can you
comment?

Mark

[Evan Martin, 2010-02-23 11:03:17]

http://codereview.chromium.org/652109/diff/1/2
File chrome/nacl.gypi (right):

http://codereview.chromium.org/652109/diff/1/2#newcode69
chrome/nacl.gypi:69:
'../native_client/src/trusted/validator_x86/validator_x86.gyp:ncvalidate',
Are all of these dependencies marked as static libraries?

[Mark Seaborn, 2010-02-23 11:33:11]

On 2010/02/23 11:03:17, Evan Martin wrote:
> http://codereview.chromium.org/652109/diff/1/2#newcode69
> chrome/nacl.gypi:69:
> '../native_client/src/trusted/validator_x86/validator_x86.gyp:ncvalidate',
> Are all of these dependencies marked as static libraries?

I think they all are except for '../webkit/webkit.gyp:glue'.

Do all the dependencies need to be static libraries?

I imagine that the reason the build doesn't fail with this change is
 * webkit.gyp:glue is pulled into the chromium executable by some other
dependency, or
 * Gyp has some magic so that transitive dependencies on shared libraries
through static libraries can be fulfilled.  But your comment suggests that this
doesn't happen.

Mark

[Evan Martin, 2010-02-23 11:40:56]

On 2010/02/23 11:33:11, mseaborn1 wrote:
> On 2010/02/23 11:03:17, Evan Martin wrote:
> > http://codereview.chromium.org/652109/diff/1/2#newcode69
> > chrome/nacl.gypi:69:
> > '../native_client/src/trusted/validator_x86/validator_x86.gyp:ncvalidate',
> > Are all of these dependencies marked as static libraries?
> 
> I think they all are except for '../webkit/webkit.gyp:glue'.
> 
> Do all the dependencies need to be static libraries?
> 
> I imagine that the reason the build doesn't fail with this change is
>  * webkit.gyp:glue is pulled into the chromium executable by some other
> dependency, or
>  * Gyp has some magic so that transitive dependencies on shared libraries
> through static libraries can be fulfilled.  But your comment suggests that
this
> doesn't happen.

I actually don't know about the magic or lack of it.  I was just verifying that
you think this change actually makes the relevant important bits of code end up
static.  If so, LGTM

[jochen (gone - plz use gerrit), 2010-02-23 14:37:56]

I compiled stuff locally, and it worked (i.e. no link error, and I could fire up
chrome)

LGTM

On 2010/02/23 11:40:56, Evan Martin wrote:
> On 2010/02/23 11:33:11, mseaborn1 wrote:
> > On 2010/02/23 11:03:17, Evan Martin wrote:
> > > http://codereview.chromium.org/652109/diff/1/2#newcode69
> > > chrome/nacl.gypi:69:
> > > '../native_client/src/trusted/validator_x86/validator_x86.gyp:ncvalidate',
> > > Are all of these dependencies marked as static libraries?
> > 
> > I think they all are except for '../webkit/webkit.gyp:glue'.
> > 
> > Do all the dependencies need to be static libraries?
> > 
> > I imagine that the reason the build doesn't fail with this change is
> >  * webkit.gyp:glue is pulled into the chromium executable by some other
> > dependency, or
> >  * Gyp has some magic so that transitive dependencies on shared libraries
> > through static libraries can be fulfilled.  But your comment suggests that
> this
> > doesn't happen.
> 
> I actually don't know about the magic or lack of it.  I was just verifying
that
> you think this change actually makes the relevant important bits of code end
up
> static.  If so, LGTM

[Mark Seaborn, 2010-02-23 15:56:51]

On Tue, Feb 23, 2010 at 2:37 PM, <jochen@chromium.org> wrote:

> I compiled stuff locally, and it worked (i.e. no link error, and I could
> fire up
> chrome)
>
> LGTM


Can you commit this on my behalf?  I'm not a Chromium committer.


On 2010/02/23 11:40:56, Evan Martin wrote:
>
>  I actually don't know about the magic or lack of it.  I was just verifying
>> that
>>
> you think this change actually makes the relevant important bits of code
>> end up
>>
> static.  If so, LGTM
>
>
service_runtime is the relevant part, and that will end up static, so yes.

Cheers,
Mark

[gregoryd, 2010-02-23 19:07:26]

Mark,
You are correct about sel_ldr code being linked into Chrome. sel_ldr target
exists in NaCl gyp files (for testing purposes), but no Chrome target
depends on it. The situation is different for Win64 (sel_ldr is linked into
a separate 64-bit binary), but that's another story.

LGTM

On Tue, Feb 23, 2010 at 02:58, <mseaborn@chromium.org> wrote:

> Reviewers: Evan Martin, bsy, sehr, Markus (顧孟勤), jochen, gregoryd,
>
> Message:
> This fixes the build but it may not be ideal if it statically links more
> than is
> necessary.
>
> However, as I understand it, the chromium executable usually links in all
> of
> NaCl's sel_ldr, and it does not invoke sel_ldr at run time (despite the
> fact
> that there is also a sel_ldr target in the Chromium build).  Greg, can you
> comment?
>
> Mark
>
>
> Description:
> Don't build NaCl's syscall handler into a shared library
>
> Fixes shared library Chromium build on x86-64 on Linux.
>
> BUG=35829
> TEST=build
>
> Please review this at http://codereview.chromium.org/652109
>
> Affected files:
>  M chrome/nacl.gypi
>
>
> Index: chrome/nacl.gypi
> diff --git a/chrome/nacl.gypi b/chrome/nacl.gypi
> index
>
d142af7efa9640f1d4b996d0ea127e86dfbf0384..1858f970833b78b260fad47d241db674d89bc4ea
> 100644
> --- a/chrome/nacl.gypi
> +++ b/chrome/nacl.gypi
> @@ -47,7 +47,10 @@
>   'targets': [
>     {
>       'target_name': 'nacl',
> -      'type': '<(library)',
> +      # The TLS (Thread Local Storage) access used by NaCl on x86-64
> +      # on Linux/ELF can't be linked into a shared library, so we
> +      # can't use '<(library)' here.  See http://crbug.com/35829.
> +      'type': 'static_library',
>       'msvs_guid': '83E86DAF-5763-4711-AD34-5FDAE395560C',
>       'variables': {
>         'nacl_target': 1,
>
>
>