linux: fix library=shared_library issues

linux: fix library=shared_library issues

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

[piman, 2009-10-30 04:51:59]

With these changes, I can compile with library=shared_library on x64 if I
disable NaCl (that needs a fix in the assembly code)

[Craig, 2009-10-30 05:53:46]

On 2009/10/30 04:51:59, piman wrote:
> With these changes, I can compile with library=shared_library on x64 if I
> disable NaCl (that needs a fix in the assembly code)

Ah, super, thank you. I'm unfamiliar with the -fPIC situation but the rest LGTM
- it mirrors  changes I've been making locally - I should have checked my email
first!

I needed cstring in asynctcpsocket.cc too btw. and still ran into some errors
but I'm doing a clean build now to see if that helps.

[Craig, 2009-10-30 06:01:31]

On 2009/10/30 05:53:46, Craig wrote:
> I needed cstring in asynctcpsocket.cc too btw. and still ran into some errors
> but I'm doing a clean build now to see if that helps.

Clean build has fixed it for me. Yay!!!! I'm also doing
LD_LIBRARY_PATH=out/Release/lib.target/
which should perhaps go into chrome-wrapper .. I'll post something this weekend
for that and for the cstring include.

[Michael Moss, 2009-10-30 13:23:25]

Markus has a change which might allow using '-fPIC' globally,
including NaCl, but it looks like it's still waiting on review.

http://codereview.chromium.org/332015

Michael

On Thu, Oct 29, 2009 at 11:01 PM,  <craig.schlenter@chromium.org> wrote:
> On 2009/10/30 05:53:46, Craig wrote:
>>
>> I needed cstring in asynctcpsocket.cc too btw. and still ran into some
>> errors
>> but I'm doing a clean build now to see if that helps.
>
> Clean build has fixed it for me. Yay!!!! I'm also doing
> LD_LIBRARY_PATH=out/Release/lib.target/
> which should perhaps go into chrome-wrapper .. I'll post something this
> weekend
> for that and for the cstring include.
>
> http://codereview.chromium.org/339079
>

[Markus Gutschke (顧孟勤), 2009-10-30 16:35:35]

In fact, even on 32bit it might make sense to build with -fPIC, as that
should result in a lot fewer pages that cannot be shared between processes.

I don't really believe in the utility of shared builds, but if distributions
ask for it, we might as well give them something that actually shares data.

Unfortunately, I believe that V8 is known to take a performance hit when
compiling as shared 32bit code -- and that's the one library most likely to
be shared with other projects.

Markus

On Oct 29, 2009 9:52 PM, <piman@chromium.org> wrote:


Reviewers: Craig, Michael Moss, Evan Martin,

Message:
With these changes, I can compile with library=shared_library on x64 if I
disable NaCl (that needs a fix in the assembly code)

Description:
linux: fix library=shared_library issues

Please review this at http://codereview.chromium.org/339079

Affected files:
 M build/common.gypi
 M chrome/chrome.gyp
 M third_party/libjingle/libjingle.gyp
 M third_party/protobuf2/protobuf.gyp


Index: build/common.gypi
diff --git a/build/common.gypi b/build/common.gypi
index
c571277ff9687fac378394874e6a475b72c6d63c..1572adddf99cb47ac829fc007c7f909656694107
100644
--- a/build/common.gypi
+++ b/build/common.gypi
@@ -672,6 +672,12 @@
            # When building with shared libraries, remove the
visiblity-hiding
            # flag.
            'cflags!': [ '-fvisibility=hidden' ],
+            'conditions': [
+              ['target_arch=="x64"', {
+                # Shared libraries need -fPIC on x86-64
+                'cflags': ['-fPIC']
+              }]
+            ],
          }],
        ],
      },
Index: chrome/chrome.gyp
diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp
index
accb720785b4425952bdbb2f65d94ff9581d1276..11e2bd50058d49a49116b096cd0d2cb77bef85f3
100755
--- a/chrome/chrome.gyp
+++ b/chrome/chrome.gyp
@@ -5236,6 +5236,9 @@
            'POSIX',
            'OS_LINUX',
          ],
+          'sources!': [
+
 'browser/sync/notifier/base/network_status_detector_task_mt.cc',
+          ],
          'dependencies': [
            '../build/linux/system.gyp:gtk'
          ],
Index: third_party/libjingle/libjingle.gyp
diff --git a/third_party/libjingle/libjingle.gyp
b/third_party/libjingle/libjingle.gyp
index
b3605683e4fba2ac1ac36b0b3dbbf053566f7e5a..b600e75d3d77a637d964b1ceadb1743f55243177
100644
--- a/third_party/libjingle/libjingle.gyp
+++ b/third_party/libjingle/libjingle.gyp
@@ -67,6 +67,7 @@
        'files/talk/base/asyncpacketsocket.cc',
        'files/talk/base/asyncpacketsocket.h',
        'files/talk/base/asynctcpsocket.h',
+        'files/talk/base/asynctcpsocket.cc',
        'files/talk/base/asyncudpsocket.cc',
        'files/talk/base/asyncudpsocket.h',
        'files/talk/base/autodetectproxy.cc',
Index: third_party/protobuf2/protobuf.gyp
diff --git a/third_party/protobuf2/protobuf.gyp
b/third_party/protobuf2/protobuf.gyp
index
7b6d0db55476cea06dee8619f882f239e192244c..d96d131d2253e3c49d7c95f8f6273353430cd55f
100644
--- a/third_party/protobuf2/protobuf.gyp
+++ b/third_party/protobuf2/protobuf.gyp
@@ -133,11 +133,12 @@
        'src/src/google/protobuf/text_format.cc',
        'src/src/google/protobuf/unknown_field_set.cc',
        'src/src/google/protobuf/wire_format.cc',
-        'src/src/google/protobuf/io/gzip_stream.cc',
+        # This file pulls in zlib, but it's not actually used by protoc, so
+        # instead of compiling zlib for the host, let's just exclude this.
+        # 'src/src/google/protobuf/io/gzip_stream.cc',
        'src/src/google/protobuf/io/printer.cc',
        'src/src/google/protobuf/io/tokenizer.cc',
        'src/src/google/protobuf/io/zero_copy_stream_impl.cc',
-        'src/src/google/protobuf/io/zero_copy_stream_impl_lite.cc',
        'src/src/google/protobuf/compiler/importer.cc',
        'src/src/google/protobuf/compiler/parser.cc',
      ],

[piman, 2009-10-30 19:54:01]

On 2009/10/30 16:35:35, Markus Gutschke (顧孟勤) wrote:
> In fact, even on 32bit it might make sense to build with -fPIC, as that
> should result in a lot fewer pages that cannot be shared between processes.
> 
> I don't really believe in the utility of shared builds, but if distributions
> ask for it, we might as well give them something that actually shares data.
> 
> Unfortunately, I believe that V8 is known to take a performance hit when
> compiling as shared 32bit code -- and that's the one library most likely to
> be shared with other projects.
> 
> Markus

The main goal is faster link times for development. The linker for ARM takes 15
minutes to link static in release (2 hours in debug !).

Otherwise, LGT you guys ?

Antoine

[Evan Martin, 2009-10-30 19:59:27]

LGTM

http://codereview.chromium.org/339079/diff/2001/3002
File chrome/chrome.gyp (right):

http://codereview.chromium.org/339079/diff/2001/3002#newcode5240
Line 5240: 'browser/sync/notifier/base/network_status_detector_task_mt.cc',
Will this break ChromeOS?

[piman, 2009-10-30 20:09:27]

http://codereview.chromium.org/339079/diff/2001/3002
File chrome/chrome.gyp (right):

http://codereview.chromium.org/339079/diff/2001/3002#newcode5240
Line 5240: 'browser/sync/notifier/base/network_status_detector_task_mt.cc',
On 2009/10/30 19:59:27, Evan Martin wrote:
> Will this break ChromeOS?

I don't think so, why ? That file conflicts with
network_status_detector_task_linux.cc, so it should be one or the other anyway.

[Craig Schlenter, 2009-10-30 20:45:10]

I'm not near a pc so this is from memory and maybe I'm confusing this  
with something else but I think the Linux file was just a stub. The mt  
file had actual code but it had a dependency on some win32 stuff for  
which no existing Linux equivalent existed in the code. My initial  
assumption was that the stub was thus the correct choice but it's  
possible that the win32 dependency is as a result of some unnecessary  
part of mt i.e. perhaps the mt file is needed given that if has other  
code in it? Is this stuff supposed to work btw.? - I thought it was  
still being ported.

--Craig

On 30 Oct 2009, at 22:09, piman@chromium.org wrote:

>
> http://codereview.chromium.org/339079/diff/2001/3002
> File chrome/chrome.gyp (right):
>
> http://codereview.chromium.org/339079/diff/2001/3002#newcode5240
> Line 5240:
> 'browser/sync/notifier/base/network_status_detector_task_mt.cc',
> On 2009/10/30 19:59:27, Evan Martin wrote:
>> Will this break ChromeOS?
>
> I don't think so, why ? That file conflicts with
> network_status_detector_task_linux.cc, so it should be one or the  
> other
> anyway.
>
> http://codereview.chromium.org/339079

[piman, 2009-10-30 21:01:05]

On 2009/10/30 20:45:10, Craig Schlenter wrote:
> I'm not near a pc so this is from memory and maybe I'm confusing this  
> with something else but I think the Linux file was just a stub. The mt  
> file had actual code but it had a dependency on some win32 stuff for  
> which no existing Linux equivalent existed in the code. My initial  
> assumption was that the stub was thus the correct choice but it's  
> possible that the win32 dependency is as a result of some unnecessary  
> part of mt i.e. perhaps the mt file is needed given that if has other  
> code in it? Is this stuff supposed to work btw.? - I thought it was  
> still being ported.
> 
> --Craig

Well I should have looked into the files... The _linux file is indeed a stub,
but it turns out that the _mt file is calling things that don't exist
(AsyncNetworkAlive::Create, there's a header file but no .cc).
So it builds with the _linux file, but not with the _mt file.

Nick, you introduced these files, what's your take ?

[ncarter (slow), 2009-10-31 01:04:56]

If you look at sync/notifier/communicator/login.cc, there's the expectation
that there's not yet a network status detector implementation:

    67:   network_status = *NetworkStatusDetectorTask*::Create(parent_);
    68:   if (network_status) {
    69:     // On linux we don't have an implementation of
*NetworkStatusDetectorTask*.    70:     network_status->Start();


So, currently, it is correct to suppress compilation of
network_status_detector_task_mt on linux where ::Create is stubbed out in
the _linux.cc variant. An ancestor of this code used _mt on Linux, I think
that's where the confusion came from.


In other words: LGTM.


 - nick

*
*

On Fri, Oct 30, 2009 at 2:01 PM, <piman@chromium.org> wrote:

> On 2009/10/30 20:45:10, Craig Schlenter wrote:
>
>> I'm not near a pc so this is from memory and maybe I'm confusing this
>> with something else but I think the Linux file was just a stub. The mt
>> file had actual code but it had a dependency on some win32 stuff for
>> which no existing Linux equivalent existed in the code. My initial
>> assumption was that the stub was thus the correct choice but it's
>> possible that the win32 dependency is as a result of some unnecessary
>> part of mt i.e. perhaps the mt file is needed given that if has other
>> code in it? Is this stuff supposed to work btw.? - I thought it was
>> still being ported.
>>
>
>  --Craig
>>
>
> Well I should have looked into the files... The _linux file is indeed a
> stub,
> but it turns out that the _mt file is calling things that don't exist
> (AsyncNetworkAlive::Create, there's a header file but no .cc).
> So it builds with the _linux file, but not with the _mt file.
>
> Nick, you introduced these files, what's your take ?
>
>
> http://codereview.chromium.org/339079
>

[piman, 2009-10-31 01:19:38]

On Fri, Oct 30, 2009 at 6:04 PM, Nick Carter <nick@chromium.org> wrote:

> If you look at sync/notifier/communicator/login.cc, there's the expectation
> that there's not yet a network status detector implementation:
>
>     67:   network_status = *NetworkStatusDetectorTask*::Create(parent_);
>     68:   if (network_status) {
>     69:     // On linux we don't have an implementation of
*NetworkStatusDetectorTask*.    70:     network_status->Start();
>
>
> So, currently, it is correct to suppress compilation of
> network_status_detector_task_mt on linux where ::Create is stubbed out in
> the _linux.cc variant. An ancestor of this code used _mt on Linux, I think
> that's where the confusion came from.
>
>
> In other words: LGTM.
>
>
>  - nick
>
>
Cool, thanks for the explanation !

Antoine

> *
> *
>
> On Fri, Oct 30, 2009 at 2:01 PM, <piman@chromium.org> wrote:
>
>> On 2009/10/30 20:45:10, Craig Schlenter wrote:
>>
>>> I'm not near a pc so this is from memory and maybe I'm confusing this
>>> with something else but I think the Linux file was just a stub. The mt
>>> file had actual code but it had a dependency on some win32 stuff for
>>> which no existing Linux equivalent existed in the code. My initial
>>> assumption was that the stub was thus the correct choice but it's
>>> possible that the win32 dependency is as a result of some unnecessary
>>> part of mt i.e. perhaps the mt file is needed given that if has other
>>> code in it? Is this stuff supposed to work btw.? - I thought it was
>>> still being ported.
>>>
>>
>>  --Craig
>>>
>>
>> Well I should have looked into the files... The _linux file is indeed a
>> stub,
>> but it turns out that the _mt file is calling things that don't exist
>> (AsyncNetworkAlive::Create, there's a header file but no .cc).
>> So it builds with the _linux file, but not with the _mt file.
>>
>> Nick, you introduced these files, what's your take ?
>>
>>
>> http://codereview.chromium.org/339079
>>
>
>

[Michael Moss, 2009-11-02 18:34:04]

Any update on this?

[piman, 2009-11-02 18:53:53]

On Mon, Nov 2, 2009 at 10:34 AM, <mmoss@chromium.org> wrote:

> Any update on this?


I'll check it in today.


>
>
> http://codereview.chromium.org/339079
>

[piman, 2009-11-02 20:46:45]

On Mon, Nov 2, 2009 at 11:53 AM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Mon, Nov 2, 2009 at 10:34 AM, <mmoss@chromium.org> wrote:
>
>> Any update on this?
>
>
> I'll check it in today.
>

It's in, hoping it sticks.


>
>
>>
>>
>> http://codereview.chromium.org/339079
>>
>
>