Enable tcmalloc VDSO support only on x86 to reduce static initializers

Enable tcmalloc VDSO support only on x86 to reduce static initializers

Background context
------------------
crrev.com/1466173002 switched the GN tcmalloc target from
source_set -> static_library. There are good reasons for keeping
tcmalloc a source_set (see "Note on static libraries" in [1]). However,
in the current state source_set was exposing extra static initializers
in the GN build which, are not present in the gyp build due to the
linker gc sections.

Resolution of this CL
---------------------
The fact that vdso_support.cc is GC-ed by the linker is the symptom
that such code is unreachable. A search in the codebase shows that the
only client is stacktrace_x86-inl.h, which depends on VDSO only when
defined(__linux__) && defined(__i386__)
This CL is therefore matching this condition in vdso_support.h and
conditioning the #define HAVE_VDSO_SUPPORT with the same conditions.

Final result after this CL
--------------------------
On GYP, the chrome binary is bit-identical before and after this CL:
$ md5sum out_gyp/Release/chrome{,.tot}
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome.tot

This CL does not regress GN static initializers (w.r.t. gyp) either:

$ tools/linux/dump-static-initializers.py out_gn/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

$ tools/linux/dump-static-initializers.py out_gyp/Release/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

[1] https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookbook.md

BUG=559766, 564618
Committed: https://crrev.com/a51d32cb536525bc8f6ca372da94bc719eed55f7
Cr-Commit-Position: refs/heads/master@{#368940}

[Primiano Tucci (use gerrit), 2016-01-12 13:34:45]

Description was changed from

==========
Remove tcmalloc VDSO support: unused, causes static initializers in GN

crrev.com/1466173002 switched the GN tcmalloc target from
source_set -> static_library. There are good reasons for keeping
tcmalloc a source_set (see "Note on static libraries" in [1]). However,
in the current state source_set was exposing extra static initializers
in the GN build which, are not present in the gyp build due to the
linker gc sections.

The fact that vdso_support.cc is GC-ed by the linker is the symptom
that such code is effectively dead. At this point the right thing to do
seems to disable vdso_support.cc (one line fix) and switch back tcmalloc
to be a source_set.

On GYP, the chrome binary is bit-identical before and after this CL:
$ md5sum out_gyp/Release/chrome{,.tot}
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome.tot

This CL does not regress GN static initializers (w.r.t. gyp) either:

$ tools/linux/dump-static-initializers.py out_gn/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

$ tools/linux/dump-static-initializers.py out_gyp/Release/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

[1]
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...

BUG=559766,564618
==========

to

==========
Enable tcmalloc VDSO support only on x86 to reduce static initializers

Background context
------------------
crrev.com/1466173002 switched the GN tcmalloc target from
source_set -> static_library. There are good reasons for keeping
tcmalloc a source_set (see "Note on static libraries" in [1]). However,
in the current state source_set was exposing extra static initializers
in the GN build which, are not present in the gyp build due to the
linker gc sections.

Resolution of this CL
---------------------
The fact that vdso_support.cc is GC-ed by the linker is the symptom
that such code is unreachable. A search in the codebase shows that the
only client is stacktrace_x86-inl.h, which depends on VDSO only when
defined(__linux__) && defined(__i386__)
This CL is therefore matching this condition in vdso_support.h and
conditioning the #define HAVE_VDSO_SUPPORT with the same conditions.

Final result after this CL
--------------------------
On GYP, the chrome binary is bit-identical before and after this CL:
$ md5sum out_gyp/Release/chrome{,.tot}
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome.tot

This CL does not regress GN static initializers (w.r.t. gyp) either:

$ tools/linux/dump-static-initializers.py out_gn/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

$ tools/linux/dump-static-initializers.py out_gyp/Release/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

[1]
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...

BUG=559766,564618
==========

[Primiano Tucci (use gerrit), 2016-01-12 13:37:32]

primiano@chromium.org changed reviewers:
+ dpranke@chromium.org, wfh@chromium.org

[Primiano Tucci (use gerrit), 2016-01-12 13:37:33]

+dpranke/wfh:  It's just a 4 line change in the .h and I think this is the right
resolution for the VDSO static initializers issue.

If you are wondering, my personal interest in making tcmalloc a source_set is
that it helps me in the allocator cleanup work. As a bonus, Dirk gets rid of a
TODO ;-)

[Will Harris, 2016-01-12 13:44:14]

On 2016/01/12 13:37:33, Primiano Tucci wrote:
> +dpranke/wfh:  It's just a 4 line change in the .h and I think this is the
right
> resolution for the VDSO static initializers issue.
> 
> If you are wondering, my personal interest in making tcmalloc a source_set is
> that it helps me in the allocator cleanup work. As a bonus, Dirk gets rid of a
> TODO ;-)

lgtm

[Primiano Tucci (use gerrit), 2016-01-12 13:55:54]

primiano@chromium.org changed reviewers:
+ thakis@chromium.org

[Primiano Tucci (use gerrit), 2016-01-12 13:55:55]

+thakis for a 1 line change in	base/allocator/BUILD.gn

[Nico, 2016-01-12 15:25:29]

Actually, now that I look at the other files: Do we need the vdso stuff on
32-bit linux at all? That seems like a very ancient thing -- last time I looked
at this I almost convinced myself that we should be able to drop this
everywhere, and that was a while ago...

[Primiano Tucci (use gerrit), 2016-01-12 15:51:54]

happy to undef HAVE_VDSO_SUPPORT completely if wfh agreed.
I was trying to make the change that preseves the current status of thing.
Having said that, I'm pretty confident that nobody will care if we break vdso on
linux 32-bit.
In practice the only thing that would break is the tcmalloc heap profiler
failing to unwind frames on linux x86 that happen to start in a vdso. Not sure
if there is anything relevant other than gettimeofday() there.

[Will Harris, 2016-01-12 15:54:59]

wfh@chromium.org changed reviewers:
+ jln@chromium.org, rickyz@chromium.org

[Will Harris, 2016-01-12 15:55:23]

On 2016/01/12 15:51:54, Primiano Tucci wrote:
> happy to undef HAVE_VDSO_SUPPORT completely if wfh agreed.
> I was trying to make the change that preseves the current status of thing.
> Having said that, I'm pretty confident that nobody will care if we break vdso
on
> linux 32-bit.
> In practice the only thing that would break is the tcmalloc heap profiler
> failing to unwind frames on linux x86 that happen to start in a vdso. Not sure
> if there is anything relevant other than gettimeofday() there.

I was happy that this CL made no changes to behavior, but I'd rather rickyz or
jln look over this CL if we intend to totally remove vDSO support since this
might have implications on the syscalls we make. so +those people.

[Primiano Tucci (use gerrit), 2016-01-12 16:14:05]

On 2016/01/12 15:55:23, Will Harris (in DC) wrote:
> I was happy that this CL made no changes to behavior, but I'd rather rickyz or
> jln look over this CL if we intend to totally remove vDSO support since this
> might have implications on the syscalls we make. so +those people.

Can we maybe get this first, so we get the rest unlocked, and I can file a bug,
assign to myself, and move the conversation there about nuking VDSO entirely?

[Will Harris, 2016-01-12 16:20:29]

On 2016/01/12 16:14:05, Primiano Tucci wrote:
> On 2016/01/12 15:55:23, Will Harris (in DC) wrote:
> > I was happy that this CL made no changes to behavior, but I'd rather rickyz
or
> > jln look over this CL if we intend to totally remove vDSO support since this
> > might have implications on the syscalls we make. so +those people.
> 
> Can we maybe get this first, so we get the rest unlocked, and I can file a
bug,
> assign to myself, and move the conversation there about nuking VDSO entirely?

yes I'm happy to defer any decision on vDSO to a follow-up CL. I'm happy this CL
doesn't change behavior, so a second lgtm from me.

[Nico, 2016-01-12 16:20:57]

ok

On Tue, Jan 12, 2016 at 11:20 AM, <wfh@chromium.org> wrote:

> On 2016/01/12 16:14:05, Primiano Tucci wrote:
>
>> On 2016/01/12 15:55:23, Will Harris (in DC) wrote:
>> > I was happy that this CL made no changes to behavior, but I'd rather
>> rickyz
>>
> or
>
>> > jln look over this CL if we intend to totally remove vDSO support since
>> this
>> > might have implications on the syscalls we make. so +those people.
>>
>
> Can we maybe get this first, so we get the rest unlocked, and I can file a
>>
> bug,
>
>> assign to myself, and move the conversation there about nuking VDSO
>> entirely?
>>
>
> yes I'm happy to defer any decision on vDSO to a follow-up CL. I'm happy
> this CL
> doesn't change behavior, so a second lgtm from me.
>
> https://codereview.chromium.org/1578163002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Primiano Tucci (use gerrit), 2016-01-12 17:00:03]

Filed crbug.com/576786 to track the vDSO full cleanup.

[brettw, 2016-01-12 18:55:14]

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

[brettw, 2016-01-12 18:55:14]

lgtm

[Primiano Tucci (use gerrit), 2016-01-12 19:07:30]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2016-01-12 19:09:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1578163002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1578163002/40001

[commit-bot: I haz the power, 2016-01-12 19:16:58]

Description was changed from

==========
Enable tcmalloc VDSO support only on x86 to reduce static initializers

Background context
------------------
crrev.com/1466173002 switched the GN tcmalloc target from
source_set -> static_library. There are good reasons for keeping
tcmalloc a source_set (see "Note on static libraries" in [1]). However,
in the current state source_set was exposing extra static initializers
in the GN build which, are not present in the gyp build due to the
linker gc sections.

Resolution of this CL
---------------------
The fact that vdso_support.cc is GC-ed by the linker is the symptom
that such code is unreachable. A search in the codebase shows that the
only client is stacktrace_x86-inl.h, which depends on VDSO only when
defined(__linux__) && defined(__i386__)
This CL is therefore matching this condition in vdso_support.h and
conditioning the #define HAVE_VDSO_SUPPORT with the same conditions.

Final result after this CL
--------------------------
On GYP, the chrome binary is bit-identical before and after this CL:
$ md5sum out_gyp/Release/chrome{,.tot}
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome.tot

This CL does not regress GN static initializers (w.r.t. gyp) either:

$ tools/linux/dump-static-initializers.py out_gn/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

$ tools/linux/dump-static-initializers.py out_gyp/Release/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

[1]
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...

BUG=559766,564618
==========

to

==========
Enable tcmalloc VDSO support only on x86 to reduce static initializers

Background context
------------------
crrev.com/1466173002 switched the GN tcmalloc target from
source_set -> static_library. There are good reasons for keeping
tcmalloc a source_set (see "Note on static libraries" in [1]). However,
in the current state source_set was exposing extra static initializers
in the GN build which, are not present in the gyp build due to the
linker gc sections.

Resolution of this CL
---------------------
The fact that vdso_support.cc is GC-ed by the linker is the symptom
that such code is unreachable. A search in the codebase shows that the
only client is stacktrace_x86-inl.h, which depends on VDSO only when
defined(__linux__) && defined(__i386__)
This CL is therefore matching this condition in vdso_support.h and
conditioning the #define HAVE_VDSO_SUPPORT with the same conditions.

Final result after this CL
--------------------------
On GYP, the chrome binary is bit-identical before and after this CL:
$ md5sum out_gyp/Release/chrome{,.tot}
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome.tot

This CL does not regress GN static initializers (w.r.t. gyp) either:

$ tools/linux/dump-static-initializers.py out_gn/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

$ tools/linux/dump-static-initializers.py out_gyp/Release/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

[1]
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...

BUG=559766,564618
==========

[commit-bot: I haz the power, 2016-01-12 19:16:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-12 19:18:25]

Description was changed from

==========
Enable tcmalloc VDSO support only on x86 to reduce static initializers

Background context
------------------
crrev.com/1466173002 switched the GN tcmalloc target from
source_set -> static_library. There are good reasons for keeping
tcmalloc a source_set (see "Note on static libraries" in [1]). However,
in the current state source_set was exposing extra static initializers
in the GN build which, are not present in the gyp build due to the
linker gc sections.

Resolution of this CL
---------------------
The fact that vdso_support.cc is GC-ed by the linker is the symptom
that such code is unreachable. A search in the codebase shows that the
only client is stacktrace_x86-inl.h, which depends on VDSO only when
defined(__linux__) && defined(__i386__)
This CL is therefore matching this condition in vdso_support.h and
conditioning the #define HAVE_VDSO_SUPPORT with the same conditions.

Final result after this CL
--------------------------
On GYP, the chrome binary is bit-identical before and after this CL:
$ md5sum out_gyp/Release/chrome{,.tot}
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome.tot

This CL does not regress GN static initializers (w.r.t. gyp) either:

$ tools/linux/dump-static-initializers.py out_gn/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

$ tools/linux/dump-static-initializers.py out_gyp/Release/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

[1]
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...

BUG=559766,564618
==========

to

==========
Enable tcmalloc VDSO support only on x86 to reduce static initializers

Background context
------------------
crrev.com/1466173002 switched the GN tcmalloc target from
source_set -> static_library. There are good reasons for keeping
tcmalloc a source_set (see "Note on static libraries" in [1]). However,
in the current state source_set was exposing extra static initializers
in the GN build which, are not present in the gyp build due to the
linker gc sections.

Resolution of this CL
---------------------
The fact that vdso_support.cc is GC-ed by the linker is the symptom
that such code is unreachable. A search in the codebase shows that the
only client is stacktrace_x86-inl.h, which depends on VDSO only when
defined(__linux__) && defined(__i386__)
This CL is therefore matching this condition in vdso_support.h and
conditioning the #define HAVE_VDSO_SUPPORT with the same conditions.

Final result after this CL
--------------------------
On GYP, the chrome binary is bit-identical before and after this CL:
$ md5sum out_gyp/Release/chrome{,.tot}
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome
0e9205350b9604eb7fc16988e39a7a86  out_gyp/Release/chrome.tot

This CL does not regress GN static initializers (w.r.t. gyp) either:

$ tools/linux/dump-static-initializers.py out_gn/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

$ tools/linux/dump-static-initializers.py out_gyp/Release/chrome
... (verbose output omitted) ...
Found 38 static initializers in 7 files.

[1]
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/cookboo...

BUG=559766,564618

Committed: https://crrev.com/a51d32cb536525bc8f6ca372da94bc719eed55f7
Cr-Commit-Position: refs/heads/master@{#368940}
==========

[commit-bot: I haz the power, 2016-01-12 19:18:26]

Patchset 3 (id:??) landed as
https://crrev.com/a51d32cb536525bc8f6ca372da94bc719eed55f7
Cr-Commit-Position: refs/heads/master@{#368940}

[Dirk Pranke, 2016-01-26 02:04:51]

lgtm