Never instrument the crazy linker.

Never instrument the crazy linker.

We are not interested in computing an order file for the crazy linker. This CL ensures that we never try to instrument it.

R=mariakhomenko@chromium.org, pliard@chromium.org, thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272967

[Philippe, 2014-05-22 10:47:18]

lgtm, thanks Benjamin!

[Maria, 2014-05-22 17:55:08]

lgtm

[qsr, 2014-05-23 07:33:37]

thakis@ -> ping for OWNERS.

[Nico, 2014-05-23 23:11:48]

Sorry, I don't know what this does. I'm guessing there's an effort to spin up
'cyg profile' support? If so, there should be a tracking bug for this. Or
cygprofile already works, but not with the crazy linker?

Please expand the CL description a bit so that I can understand why this is
done.

[qsr, 2014-05-26 10:56:22]

On 2014/05/23 23:11:48, Nico wrote:
> Sorry, I don't know what this does. I'm guessing there's an effort to spin up
> 'cyg profile' support? If so, there should be a tracking bug for this. Or
> cygprofile already works, but not with the crazy linker?
> 
> Please expand the CL description a bit so that I can understand why this is
> done.

 Hum... The issue is that the crazy linker is an independent library. When
instrumentation is enabled, the code for the crazy linker is intrumented as
everything else. We do not care about instrumentation for the linker, but we
need to provide the function that cygprofile expect. This add empty stub for
those functions for the crazy linker when we compile with instrumentation. As
the description said, all this CL is doing is allowing the crazy linker library
to compile in presence of the intrumentation flag.

[Nico, 2014-05-27 08:08:18]

On 2014/05/26 10:56:22, qsr wrote:
> On 2014/05/23 23:11:48, Nico wrote:
> > Sorry, I don't know what this does. I'm guessing there's an effort to spin
up
> > 'cyg profile' support? If so, there should be a tracking bug for this. Or
> > cygprofile already works, but not with the crazy linker?
> > 
> > Please expand the CL description a bit so that I can understand why this is
> > done.
> 
>  Hum... The issue is that the crazy linker is an independent library. When
> instrumentation is enabled, the code for the crazy linker is intrumented as
> everything else. We do not care about instrumentation for the linker, but we
> need to provide the function that cygprofile expect. This add empty stub for
> those functions for the crazy linker when we compile with instrumentation. As
> the description said, all this CL is doing is allowing the crazy linker
library
> to compile in presence of the intrumentation flag.

Can't you just build crazy linker without finstrument-functions then?

[qsr, 2014-05-27 08:13:03]

On 2014/05/27 08:08:18, Nico (traveling) wrote:
> On 2014/05/26 10:56:22, qsr wrote:
> > On 2014/05/23 23:11:48, Nico wrote:
> > > Sorry, I don't know what this does. I'm guessing there's an effort to spin
> up
> > > 'cyg profile' support? If so, there should be a tracking bug for this. Or
> > > cygprofile already works, but not with the crazy linker?
> > > 
> > > Please expand the CL description a bit so that I can understand why this
is
> > > done.
> > 
> >  Hum... The issue is that the crazy linker is an independent library. When
> > instrumentation is enabled, the code for the crazy linker is intrumented as
> > everything else. We do not care about instrumentation for the linker, but we
> > need to provide the function that cygprofile expect. This add empty stub for
> > those functions for the crazy linker when we compile with instrumentation.
As
> > the description said, all this CL is doing is allowing the crazy linker
> library
> > to compile in presence of the intrumentation flag.
> 
> Can't you just build crazy linker without finstrument-functions then?

 The crazy linker gyp is defined outside of chromium (in android sdk). I don't
think we should prevent it at this level.

[Nico, 2014-05-27 08:17:57]

On 2014/05/27 08:13:03, qsr wrote:
> On 2014/05/27 08:08:18, Nico (traveling) wrote:
> > On 2014/05/26 10:56:22, qsr wrote:
> > > On 2014/05/23 23:11:48, Nico wrote:
> > > > Sorry, I don't know what this does. I'm guessing there's an effort to
spin
> > up
> > > > 'cyg profile' support? If so, there should be a tracking bug for this.
Or
> > > > cygprofile already works, but not with the crazy linker?
> > > > 
> > > > Please expand the CL description a bit so that I can understand why this
> is
> > > > done.
> > > 
> > >  Hum... The issue is that the crazy linker is an independent library. When
> > > instrumentation is enabled, the code for the crazy linker is intrumented
as
> > > everything else. We do not care about instrumentation for the linker, but
we
> > > need to provide the function that cygprofile expect. This add empty stub
for
> > > those functions for the crazy linker when we compile with instrumentation.
> As
> > > the description said, all this CL is doing is allowing the crazy linker
> > library
> > > to compile in presence of the intrumentation flag.
> > 
> > Can't you just build crazy linker without finstrument-functions then?
> 
>  The crazy linker gyp is defined outside of chromium (in android sdk). I don't
> think we should prevent it at this level.

Are you sure you get profiling symbols for the crazy linker itself, not just for
base/android/linker/linker_jni.cc ? (As mentioned above, a bug explaining what
you want to do would help me to understand this faster; sorry about the dense
questions in the meantime.)

[qsr, 2014-05-27 09:05:36]

On 2014/05/27 08:17:57, Nico (traveling) wrote:
> On 2014/05/27 08:13:03, qsr wrote:
> > On 2014/05/27 08:08:18, Nico (traveling) wrote:
> > > On 2014/05/26 10:56:22, qsr wrote:
> > > > On 2014/05/23 23:11:48, Nico wrote:
> > > > > Sorry, I don't know what this does. I'm guessing there's an effort to
> spin
> > > up
> > > > > 'cyg profile' support? If so, there should be a tracking bug for this.
> Or
> > > > > cygprofile already works, but not with the crazy linker?
> > > > > 
> > > > > Please expand the CL description a bit so that I can understand why
this
> > is
> > > > > done.
> > > > 
> > > >  Hum... The issue is that the crazy linker is an independent library.
When
> > > > instrumentation is enabled, the code for the crazy linker is intrumented
> as
> > > > everything else. We do not care about instrumentation for the linker,
but
> we
> > > > need to provide the function that cygprofile expect. This add empty stub
> for
> > > > those functions for the crazy linker when we compile with
instrumentation.
> > As
> > > > the description said, all this CL is doing is allowing the crazy linker
> > > library
> > > > to compile in presence of the intrumentation flag.
> > > 
> > > Can't you just build crazy linker without finstrument-functions then?
> > 
> >  The crazy linker gyp is defined outside of chromium (in android sdk). I
don't
> > think we should prevent it at this level.
> 
> Are you sure you get profiling symbols for the crazy linker itself, not just
for
> base/android/linker/linker_jni.cc ? (As mentioned above, a bug explaining what
> you want to do would help me to understand this faster; sorry about the dense
> questions in the meantime.)

 Following our f2f conversation, changed common.gypi to not instrument the crazy
linker.

[Nico, 2014-05-27 09:08:00]

I like this better, thanks. Please update the CL description too :-)

https://chromiumcodereview.appspot.com/292123008/diff/40001/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/292123008/diff/40001/base/base.gyp#new...
base/base.gyp:1418: 'cflags!': [
Maybe add a comment for why this is here.

[qsr, 2014-05-27 09:10:59]

https://chromiumcodereview.appspot.com/292123008/diff/40001/base/base.gyp
File base/base.gyp (right):

https://chromiumcodereview.appspot.com/292123008/diff/40001/base/base.gyp#new...
base/base.gyp:1418: 'cflags!': [
On 2014/05/27 09:08:00, Nico (traveling) wrote:
> Maybe add a comment for why this is here.

Done.

[Nico, 2014-05-27 09:12:07]

lgtm!

https://chromiumcodereview.appspot.com/292123008/diff/60001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/292123008/diff/60001/build/common.gypi...
build/common.gypi:3909: ['_toolset=="target" and _target_name!="crazy_linker"',
{
# crazy_linker has an upstream gyp file we can't edit, and we don't want to
instrument it.

[qsr, 2014-05-27 09:22:43]

https://chromiumcodereview.appspot.com/292123008/diff/60001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/292123008/diff/60001/build/common.gypi...
build/common.gypi:3909: ['_toolset=="target" and _target_name!="crazy_linker"',
{
On 2014/05/27 09:12:07, Nico (traveling) wrote:
> # crazy_linker has an upstream gyp file we can't edit, and we don't want to
> instrument it.

Done.

[qsr, 2014-05-27 09:22:47]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-05-27 09:23:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/292123008/80001

[commit-bot: I haz the power, 2014-05-27 11:24:37]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-27 11:29:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-27 11:29:58]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[qsr, 2014-05-27 12:59:50]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-05-27 13:00:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/292123008/80001

[commit-bot: I haz the power, 2014-05-27 13:07:28]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[commit-bot: I haz the power, 2014-05-27 13:31:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-27 13:31:18]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[Andrew Hayden (chromium.org), 2014-05-27 13:54:33]

The CQ bit was checked by andrewhayden@chromium.org

[commit-bot: I haz the power, 2014-05-27 13:54:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qsr@chromium.org/292123008/80001

[Andrew Hayden (chromium.org), 2014-05-27 13:55:01]

re-upping the commit box, since this looks like a transient build bot FS error
and I'm sheriffing for Android right now (and want this fixed)

[Andrew Hayden (chromium.org), 2014-05-27 13:55:29]

+simonb fyi

[commit-bot: I haz the power, 2014-05-27 14:00:41]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[qsr, 2014-05-27 14:02:52]

Committed patchset #5 manually as r272967 (presubmit successful).