Added compile-time define to add "initial-exec" attribute.

Added compile-time define to gate "initial-exec" attribute.

The "initial-exec" attribute is added to the ThreadCache::threadlocal_heap_
variable under normal compilation. This causes the linker to emit a
R_X86_64_TPOFF64 relocation in the _pyautolib.so shared object. This relocation
can cause python to error with:

cannot allocate memory in static TLS block

when it calls dlopen() on _pyautolib.so (when the import statement is
interpreted).

This only happens when the TLS section is large enough. Building Chrome with
-fprofile-generate to add some instrumentation-related data to the TLS is enough
to trigger this condition.

This CL encloses the function attribute in #if ! defined(PGO_GENERATE). When
Chrome is built with -fprofile-generate, we will also pass in -DPGO_GENERATE and
-ftls-model=global-dynamic so profile data can be collected without any errors.

BUG=none
TEST=Rebuilt chromeos-chrome in the chroot with -fprofile-generate
-ftls-model=global-dynamic -DPGO_GENERATE && ran pyautoperf autotests.


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

[asharif1, 2012-04-17 23:03:32]

PTAL.

[asharif1, 2012-04-17 23:04:22]

-skg
+sgk

[sgk, 2012-04-18 00:04:09]

I've been away from the project for some time now. Best that you find another
reviewer if you want another set of eyes in addition to Alexander's.

[asharif1, 2012-04-18 02:26:38]

-sgk
+gpike

[asharif1, 2012-04-18 03:02:32]

+dennisjeffrey.

Dennis, do you know if _pyautolib.so really needs to link against tcmalloc? This
whole situation can be avoided if _pyautolib.so doesn't use tcmalloc and uses
glibc malloc.

The comment in thread_cache.h says that anything using tcmalloc cannot be
dlopen()'d. I don't know how _pyautolib.so was working all this time. Perhaps it
wasn't really using any tcmalloc routines but just linking against it?

I'll do some more digging tomorrow.

[Alexander Potapenko, 2012-04-18 08:07:20]

Is PGO_GENERATE a standard define? If not, it's better to add an appropriate
line to config.h

Please also note that tcmalloc/vendor is the verbatim tcmalloc trunk. Neither
should you patch it, nor will your patches make it to Chromium, because
liballocator is built from tcmalloc/chromium.

[Alexander Potapenko, 2012-04-18 08:08:18]

And, yes, it might be better not to depend on tcmalloc in pyautolib.

[dennisjeffrey, 2012-04-18 15:39:17]

I don't know about this - I'm cc'ing Nirnimesh who is the most likely to
know.

-Dennis

On Tue, Apr 17, 2012 at 8:02 PM, <asharif@chromium.org> wrote:

> +dennisjeffrey.
>
> Dennis, do you know if _pyautolib.so really needs to link against
> tcmalloc? This
> whole situation can be avoided if _pyautolib.so doesn't use tcmalloc and
> uses
> glibc malloc.
>
> The comment in thread_cache.h says that anything using tcmalloc cannot be
> dlopen()'d. I don't know how _pyautolib.so was working all this time.
> Perhaps it
> wasn't really using any tcmalloc routines but just linking against it?
>
> I'll do some more digging tomorrow.
>
>
http://codereview.chromium.**org/10035012/<http://codereview.chromium.org/100...
>

[asharif1, 2012-04-18 18:38:41]

On 2012/04/18 08:07:20, Alexander Potapenko wrote:
> Is PGO_GENERATE a standard define? If not, it's better to add an appropriate
> line to config.h

It is not a standard define and is only a define I'll be passing down when
building with -fprofile-generate.

Which config.h?

This one ./third_party/tcmalloc/chromium/src/config.h only contains OS_*
defines.

build/build_config.h defines OS_* and ARCH related macros.

Even if I put the macro there, I'll have to edit thread_cache.cc and
thread_cache.h to use it.

What would be an appropriate line addition to config.h?

> 
> Please also note that tcmalloc/vendor is the verbatim tcmalloc trunk. Neither
> should you patch it, nor will your patches make it to Chromium, because
> liballocator is built from tcmalloc/chromium.

[Nirnimesh, 2012-04-18 18:49:58]

_pyautolib.so uses chrome's base & ipc libs. If they don't need tcmalloc, I
don't see why pyautolib should. patches welcome

[asharif1, 2012-04-18 21:25:59]

On 2012/04/18 18:49:58, Nirnimesh wrote:
> _pyautolib.so uses chrome's base & ipc libs. If they don't need tcmalloc, I
> don't see why pyautolib should. patches welcome

I looked at this for a while and this dependency is required because pyautolib
depends on test_support_base and that depends on base which depends on tcmalloc.

So something similar to this patch seems to be the only option.

[asharif1, 2012-04-18 22:55:13]

On 2012/04/18 21:25:59, asharif1 wrote:
> On 2012/04/18 18:49:58, Nirnimesh wrote:
> > _pyautolib.so uses chrome's base & ipc libs. If they don't need tcmalloc, I
> > don't see why pyautolib should. patches welcome
> 
> I looked at this for a while and this dependency is required because pyautolib
> depends on test_support_base and that depends on base which depends on
tcmalloc.
> 
> So something similar to this patch seems to be the only option.

glider, please take a look again. I'd like to get this in soon, if possible.

[Alexander Potapenko, 2012-04-19 11:49:26]

I think I'm ok with this #ifdef in the Chromium branch of tcmalloc (please add a
comment about that)

But I still can't understand why pyautolib should require tcmalloc (i.e.
liballocator)
In Chromium only executable targets should depend on liballocator, so this is
possibly not true that base depends on liballocator.
Because pyautolib is about testing the browser, not the contents of the shared
library, I think we can afford to build it without tcmalloc.

[asharif1, 2012-04-19 17:50:54]

On 2012/04/19 11:49:26, Alexander Potapenko wrote:
> I think I'm ok with this #ifdef in the Chromium branch of tcmalloc (please add
a
> comment about that)

In what file should I add a comment?

> 
> But I still can't understand why pyautolib should require tcmalloc (i.e.
> liballocator)
> In Chromium only executable targets should depend on liballocator, so this is
> possibly not true that base depends on liballocator.

I looked at base.gyp and one of the base files is icu_string_conversions.cc.
That file contains a function that calls std::string->resize(). The resize
function may call malloc I think.

> Because pyautolib is about testing the browser, not the contents of the shared
> library, I think we can afford to build it without tcmalloc.

PyAutoLib depends on base and ipc as Nirmimesh said. I'll experiment today with
removing tcmalloc from base to see whether it builds and runs fine.

[Alexander Potapenko, 2012-04-19 19:15:10]

According to
http://src.chromium.org/viewvc/chrome/trunk/src/base/base.gypi?view=markup, the
target named "base" does not depend on tcmalloc (that's "allocator"). IIRC we
have a policy that only executables may depend on liballocator.
I suppose you can easily build pyautolib with linux_use_tcmalloc=0, and it will
use the malloc() from libc (I haven't tried that, so it's better if you
double-check).
In fact I'm a bit surprised that pyautolib depends on tcmalloc by default, and
it probably needs to be fixed.

Regarding the comments (in the case pyautolib really needs tcmalloc): I think
they should be added to both files. It's a matter of merging them with the
trunk: if a conflict needs to be resolved manually, the person who's making the
merge needs to understand where did the diff come from. If there's a bug about
the problem you're trying to solve, a link should be enough, otherwise it's ok
to describe the problem in the header file and refer to that comment in the .cc.

[asharif1, 2012-04-19 19:39:17]

On 2012/04/19 19:15:10, Alexander Potapenko wrote:
> According to
> http://src.chromium.org/viewvc/chrome/trunk/src/base/base.gypi?view=markup,
the
> target named "base" does not depend on tcmalloc (that's "allocator"). IIRC we

You are right. base doesn't seem to depend on tcmalloc.

But why is it being pulled in the link line of _pyautoperf.so? I find gyp files
difficult to read. Is there some gyp file analysis tool out there that I can use
to see the full dependency chain and where I need to make the edit for
_pyautoperf.so to not depend on tcmalloc?

> have a policy that only executables may depend on liballocator.
> I suppose you can easily build pyautolib with linux_use_tcmalloc=0, and it
will
> use the malloc() from libc (I haven't tried that, so it's better if you
> double-check).
> In fact I'm a bit surprised that pyautolib depends on tcmalloc by default, and
> it probably needs to be fixed.
> 
> Regarding the comments (in the case pyautolib really needs tcmalloc): I think
> they should be added to both files. It's a matter of merging them with the
> trunk: if a conflict needs to be resolved manually, the person who's making
the
> merge needs to understand where did the diff come from. If there's a bug about
> the problem you're trying to solve, a link should be enough, otherwise it's ok
> to describe the problem in the header file and refer to that comment in the
.cc.

[Alexander Potapenko, 2012-04-19 19:59:19]

I don't know of any such tool. Maybe hacking the gyp interpreter to
print intermediate info may help. Otherwise you can try to look at the
corresponding .target.mk file.

[asharif1, 2012-04-24 00:02:16]

glider, please take a look at the issue again. We need this fix to go in before
R20 of ChromeOS. I filed a bug here for removing liballocator.a from pyautolib,
but it may be a lot of work since gyp doesn't support an easy way:

https://groups.google.com/group/gyp-developer/browse_thread/thread/45639aa805...

I have a partial CL here, but it fails on some platforms, and it's not trivial
to fix:

http://codereview.chromium.org/10159006/

I have to work on other PGO related CLs, so Nirnimesh or some other pyautolib
maintainer could work on a real fix.

[Alexander Potapenko, 2012-04-24 10:38:15]

LGTM with a nit.

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.cc (right):

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.cc:70: #if
defined(HAVE___ATTRIBUTE__) && ! defined(PGO_GENERATE)
Ditto.

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.h:268: #if
defined(HAVE___ATTRIBUTE__) && ! defined(PGO_GENERATE)
I think "!defined" is more common in Chromium.

[asharif1, 2012-04-24 17:50:12]

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.cc (right):

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.cc:70: #if
defined(HAVE___ATTRIBUTE__) && ! defined(PGO_GENERATE)
On 2012/04/24 10:38:15, Alexander Potapenko wrote:
> Ditto.

Done.

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.h:268: #if
defined(HAVE___ATTRIBUTE__) && ! defined(PGO_GENERATE)
On 2012/04/24 10:38:15, Alexander Potapenko wrote:
> I think "!defined" is more common in Chromium.

Done.

[gpike, 2012-04-24 18:38:21]

LGTM

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.h:267: // workaround is to pass
in -DPGO_GENERATE.
Please improve the comment. Under what circumstances is -DPGO_GENERATE supposed
to be used? Is Chrome still going to get the performance benefit of
initial-exec?

[asharif1, 2012-04-24 20:41:48]

On 2012/04/24 18:38:21, gpike wrote:
> LGTM
> 
>
http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
> File third_party/tcmalloc/chromium/src/thread_cache.h (right):
> 
>
http://codereview.chromium.org/10035012/diff/15001/third_party/tcmalloc/chrom...
> third_party/tcmalloc/chromium/src/thread_cache.h:267: // workaround is to pass
> in -DPGO_GENERATE.
> Please improve the comment. Under what circumstances is -DPGO_GENERATE
supposed
> to be used? Is Chrome still going to get the performance benefit of
> initial-exec?

PTAL.

[gpike, 2012-04-24 20:49:29]

LGTM

http://codereview.chromium.org/10035012/diff/31001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/10035012/diff/31001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.h:262: // See comments in the
header file about this #define. Bug here:
s/the header file/thread_cache.cc/
s/ #define//

[asharif1, 2012-04-24 21:02:33]

PTAL.

http://codereview.chromium.org/10035012/diff/31001/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.h (right):

http://codereview.chromium.org/10035012/diff/31001/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.h:262: // See comments in the
header file about this #define. Bug here:
On 2012/04/24 20:49:29, gpike wrote:
> s/the header file/thread_cache.cc/
> s/ #define//

Good catch. This is the header file. I will switch the comments in the next
patchset.

[gpike, 2012-04-24 21:05:13]

LGTM

http://codereview.chromium.org/10035012/diff/25007/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.cc (right):

http://codereview.chromium.org/10035012/diff/25007/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.cc:67: // See comments in the
thread_cache.h about this. Bug here:
s/ the//

[asharif1, 2012-04-24 21:11:43]

http://codereview.chromium.org/10035012/diff/25007/third_party/tcmalloc/chrom...
File third_party/tcmalloc/chromium/src/thread_cache.cc (right):

http://codereview.chromium.org/10035012/diff/25007/third_party/tcmalloc/chrom...
third_party/tcmalloc/chromium/src/thread_cache.cc:67: // See comments in the
thread_cache.h about this. Bug here:
On 2012/04/24 21:05:13, gpike wrote:
> s/ the//

Thanks, I should have proofread this comment. It should be fixed now.

[gpike, 2012-04-24 21:59:09]

lgtm

[commit-bot: I haz the power, 2012-04-24 22:01:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asharif@chromium.org/10035012/24004

[commit-bot: I haz the power, 2012-04-24 23:56:08]

Change committed as 133810