New issue
Advanced search Search tips

Issue 628052 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

Moving Mac build to GN adds static initializer[s] / causes sizes issues

Project Member Reported by dbeam@chromium.org, Jul 14 2016

Issue description

After flipping the rest of the Mac bots to GN[1] (and subsequent compile fixes[2]), the sizes step on Mac clobber builder continues to run red with additional static initializers[2].

dpranke@ believe the issue is here[3], in third_party/angle, which causes a static initialize for the GN build (but not GYP).

The Mac bots have been out of commission a lot today between compile failures and sizes issues, but are still (at this time) running GN.  dpranke@ mentioned that we may need to roll third_party/angle to pick up a fix.  The latency involved with this may encourage flipping Mac builder back to GYP.

I know very little about all this stuff; just happened to be sheriff on this fun day!

[1] https://codereview.chromium.org/2143863002
[2] https://build.chromium.org/p/chromium/builders/Mac/builds/17638
[3] https://cs.chromium.org/chromium/src/third_party/angle/src/common/mathutil.cpp?q=mathutil.cpp&sq=package:chromium&dr=C&l=34
 
Cc: cwallez@chromium.org
Fix out for review:

https://chromium-review.googlesource.com/#/c/360228/1

This will require an angle roll, so I'll revert the GN change for now.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/944255248a81d03034cfaf5d12255557d64a6dea

commit 944255248a81d03034cfaf5d12255557d64a6dea
Author: dpranke <dpranke@chromium.org>
Date: Thu Jul 14 01:18:26 2016

Flip the /p/chromium Mac builder (and trybot) back to GYP.

Until we get the fix for the `sizes` regression rolled in, we
should flip this builder back to GYP to keep it green.

TBR=rsesek@chromium.org, thakis@chromium.org, danbeam@chromium.org
NOTRY=true
BUG= 628052 ,  618468 

Review-Url: https://codereview.chromium.org/2152633002
Cr-Commit-Position: refs/heads/master@{#405382}

[modify] https://crrev.com/944255248a81d03034cfaf5d12255557d64a6dea/tools/mb/mb_config.pyl

Comment 3 by thakis@chromium.org, Jul 14 2016

In addition to the initializer, the symbolization of the initializers is also broken:

/System/Library/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python /b/build/slave/Mac/build/src/infra/scripts/legacy/scripts/slave/chromium/sizes.py --json /tmp/tmpevJu0b
error: unable to open executable '/b/build/slave/Mac/build/src/out/Release/Chromium Framework.framework.dSYM/Contents/Resources/DWARF/Chromium Framework'

I guess the dsym is generated in the wrong place.

Comment 4 by rsesek@chromium.org, Jul 14 2016

Just to document how I found that it was mathutil.cpp in angle:

% otool -l out/bot/Chromium\ Framework.framework/Chromium\ Framework | grep -C5 mod
    nreloc 0
     flags 0x00000007
 reserved1 3318 (index into indirect symbol table)
 reserved2 0
Section
  sectname __mod_init_func
   segname __DATA
      addr 0x0000000006cb37b0
      size 0x0000000000000008
    offset 113981360
     align 2^3 (8)

% xxd -g64 -s 0x0000000006cb37b0 -l 0x0000000000000008 out/bot/Chromium\ Framework.framework/Chromium\ Framework
6cb37b0: 90888c0300000000                  ........

% dwarfdump --lookup 0x00000000038c8890  ./out/bot/Chromium\ Framework.dSYM/Contents/Resources/DWARF/Chromium\ Framework

0x22137948: Compile Unit: length = 0x00003f24  version = 0x0002  abbr_offset = 0x00000000  addr_size = 0x08  (next CU at 0x2213b870)

0x22137953: TAG_compile_unit [612] *
             AT_producer( "clang version 3.9.0 (trunk 274369)" )
             AT_language( DW_LANG_C_plus_plus )
             AT_name( "../../third_party/angle/src/common/mathutil.cpp" )
             AT_stmt_list( 0x03faca2b )
             AT_comp_dir( "/Volumes/Build/src/out/bot" )
             AT_APPLE_optimized( 0x01 )
             AT_low_pc( 0x00000000038c8890 )
             AT_ranges( 0x0226fcb0
                [0x00000000038c8890 - 0x00000000038c88a0)
                 End )

0x2213b838:     TAG_subprogram [809] *
                 AT_low_pc( 0x00000000038c8890 )
                 AT_high_pc( 0x00000000038c88a0 )
                 AT_frame_base( rbp )
                 AT_MIPS_linkage_name( "_GLOBAL__sub_I_mathutil.cpp" )
                 AT_artificial( 0x01 )
                 AT_APPLE_optimized( 0x01 )

0x22137953:         TAG_compile_unit [612] *
                     AT_producer( "clang version 3.9.0 (trunk 274369)" )
                     AT_language( DW_LANG_C_plus_plus )
                     AT_name( "../../third_party/angle/src/common/mathutil.cpp" )
                     AT_stmt_list( 0x03faca2b )
                     AT_comp_dir( "/Volumes/Build/src/out/bot" )
                     AT_APPLE_optimized( 0x01 )
                     AT_low_pc( 0x00000000038c8890 )
                     AT_ranges( 0x0226fcb0
                        [0x00000000038c8890 - 0x00000000038c88a0)
                         End )
Line table dir : '../../third_party/angle/src/common'
Line table file: 'mathutil.cpp' line 0, column 0 with start address 0x00000000038c8890

Looking up address: 0x00000000038c8890 in .debug_frame... not found.

Comment 5 by thakis@chromium.org, Jul 14 2016

The bot runs tools/mac/dump-static-initializers.py, which also prints where the initializer is from iirc

Comment 6 by rsesek@chromium.org, Jul 14 2016

> I guess the dsym is generated in the wrong place.

The "dsyms" on the chromium/Mac bot are fake dsyms. We decided that these are bad and don't produce them in GN. This poses an interesting problem for dump-static-initializers.py because the build products are stripped.

GYP config: test_isolation_mode=noop component=static_library use_goma=1 mac_strip_release=1 

GN config: 
enable_stripping = true
is_component_build = false
is_debug = false
use_goma = true


The difference is in GYP, mac_strip_release=1 and mac_want_real_dsyms=default. This causes mac_real_dsym=0, which then generates the fake dSYM by copying the unstripped executable into a dSYM bundle and then stripping the build output. In GN, the output is just stripped.

I'm not sure how we want to handle the dump-static-initializers.py now. We can not strip the build products (much larger build sizes), generate dsyms (takes a long time), or rely on the unstripped shared library in the out/gn/obj directory.

Comment 7 by thakis@chromium.org, Jul 14 2016

Maybe revisit the decision to not produce fake dsyms :-P

Comment 8 by rsesek@chromium.org, Jul 14 2016

Cc: mark@chromium.org
Mark was pretty explicit about not wanting to produce them. I also don't think they're a good idea.

Comment 9 by thakis@chromium.org, Jul 14 2016

If you come up with something else that allows the same functionality, that's fine with me too of course.

(As a general point, I think it's a good idea to not change too many things at once. gyp->gn is already a big change, so I'd try to keep that as small in scope as possible, and then do other changes like eg getting rid of fake dsyms and whatnot later)
Having proper symbols for the continuous builds would probably be helpful, and we do provide them for Windows. The problem is that dsymutil is rather slow, but given that this bot is always a clobber bot, it is already pretty slow. Comparing the build times on my machine:

enable_dsyms = true
ninja -C out/bot -j250 all  8711.03s user 6259.28s system 462% cpu 53:59.09 total

enable_dsyms = false
ninja -C out/bot -j250 all  1407.27s user 1266.77s system 112% cpu 39:42.65 total


If we don't think it's advisable to turn on real dSYMs, then I can think of two alternatives to the fake dSYMs:

- Save a copy of the binaries in an "unstripped" location (or with an ".unstripped" suffix). This is all that fake dSYMs do, but without putting it in a bogus wrapper.
- Run nm on the product before stripping and write it to a file. That has enough information to print the name of the static initializer, which could be used by the sizes step.
Either sounds fine; the .unstripped variant sounds a bit more flexible.
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/angle/angle/+/313d944745601ba9b371866760f0496670c1cc43

commit 313d944745601ba9b371866760f0496670c1cc43
Author: Dirk Pranke <dpranke@chromium.org>
Date: Thu Jul 14 01:01:55 2016

Stop linking a static_initializer into the Mac chromium build.

The :angle-image_util target was a source_set rather than a
static_library(), causing us to link in loadimage.cpp, which
pulled in mathutils.cpp, which contains a static initializer.
Switching :angle_image_util to a static library (which is what GYP
does) solved the problem.

R=rsesek@chromium.org, cwallez@chromium.org
BUG= 628052 

Change-Id: I63387b3fc9d799c92f7c1b49a1c7c7435e70a951
Reviewed-on: https://chromium-review.googlesource.com/360228
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

[modify] https://crrev.com/313d944745601ba9b371866760f0496670c1cc43/BUILD.gn

Components: -Infra Infra>CQ
Triaging ouf ot -Infra; +Infra>CQ, just because I can't find any better specific component to assign this to.

Comment 14 by mark@chromium.org, Jul 15 2016

I think we can always save .unstripped when we strip. It doesn’t have to be tied to whether we’re making a .dSYM or not.
Owner: rsesek@chromium.org
Status: Assigned (was: Started)
Bouncing to rsesek@ for the symbols issue; as far as I know, the static initializer issue has been worked around (or fixed, depending on how you look at it) by switching to use a static_library for that target.
Components: -Infra>CQ Build
Status: Started (was: Assigned)
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0db21f9f2bc7bee9c40c64c5ad192297d32a567c

commit 0db21f9f2bc7bee9c40c64c5ad192297d32a567c
Author: jmadill <jmadill@chromium.org>
Date: Mon Jul 18 18:45:41 2016

Roll ANGLE 4c32feb..5bc93c4

https://chromium.googlesource.com/angle/angle.git/+log/4c32feb..5bc93c4

BUG= 621031 , 628052 , 612066 
TBR=cwallez@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2158003002
Cr-Commit-Position: refs/heads/master@{#406049}

[modify] https://crrev.com/0db21f9f2bc7bee9c40c64c5ad192297d32a567c/DEPS

Project Member

Comment 18 by bugdroid1@chromium.org, Jul 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7fa0d497278e20eb8f38e02269f0e88d8cffa9c2

commit 7fa0d497278e20eb8f38e02269f0e88d8cffa9c2
Author: rsesek <rsesek@chromium.org>
Date: Mon Jul 18 21:54:39 2016

[Mac/GN] Add a new linker_driver.py option to save unstripped linker output.

This declares a new GN arg save_unstripped_output that can be enabled if
enable_stripping=true. If true, then an unstripped copy of the linker output
will be saved in the output directory with an ".unstripped" suffix.

BUG= 628052 
R=mark@chromium.org

Review-Url: https://codereview.chromium.org/2157573002
Cr-Commit-Position: refs/heads/master@{#406110}

[modify] https://crrev.com/7fa0d497278e20eb8f38e02269f0e88d8cffa9c2/build/config/mac/BUILD.gn
[modify] https://crrev.com/7fa0d497278e20eb8f38e02269f0e88d8cffa9c2/build/config/mac/symbols.gni
[modify] https://crrev.com/7fa0d497278e20eb8f38e02269f0e88d8cffa9c2/build/toolchain/mac/BUILD.gn
[modify] https://crrev.com/7fa0d497278e20eb8f38e02269f0e88d8cffa9c2/build/toolchain/mac/linker_driver.py

Comment 19 Deleted

Comment 20 Deleted

Project Member

Comment 21 by bugdroid1@chromium.org, Jul 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03a19d56c6b11996704a6871d52d05bc6f252328

commit 03a19d56c6b11996704a6871d52d05bc6f252328
Author: Robert Sesek <rsesek@chromium.org>
Date: Tue Jul 19 18:24:51 2016

Add tools/mac/show_mod_init_func.py to symbolize static initializers without a dSYM.

BUG= 628052 
R=mark@chromium.org

Review URL: https://codereview.chromium.org/2161093002 .

Cr-Commit-Position: refs/heads/master@{#406325}

[modify] https://crrev.com/03a19d56c6b11996704a6871d52d05bc6f252328/tools/mac/dump-static-initializers.py
[add] https://crrev.com/03a19d56c6b11996704a6871d52d05bc6f252328/tools/mac/show_mod_init_func.py

Cc: -cwallez@chromium.org
Project Member

Comment 23 by bugdroid1@chromium.org, Jul 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b53b935fb9febdbc52456d8643b02a39c83b737

commit 1b53b935fb9febdbc52456d8643b02a39c83b737
Author: dpranke <dpranke@chromium.org>
Date: Tue Jul 19 19:16:40 2016

Downgrade Mac GN arg save_unstripped_output to just a variable.

It's not clear that we really need the GN arg `save_unstripped_output`
to be a user-configurable thing, so this change removes it from the
declare_arg() block in the build file and instead becomes a shorthand
for (enable_stripping && !enable_dsyms).

R=rsesek@chromium.org
BUG= 623685 ,  628052 

Review-Url: https://codereview.chromium.org/2163533002
Cr-Commit-Position: refs/heads/master@{#406338}

[modify] https://crrev.com/1b53b935fb9febdbc52456d8643b02a39c83b737/build/config/mac/symbols.gni

Project Member

Comment 24 by bugdroid1@chromium.org, Jul 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ca703764dc9a08fa26537ca96ca5cc0edb01df4

commit 7ca703764dc9a08fa26537ca96ca5cc0edb01df4
Author: rsesek <rsesek@chromium.org>
Date: Fri Jul 22 00:03:55 2016

[Mac/GN] Update sizes.py to use unstripped build output for reporting static initializers.

This uses the new tool in //tools/mac/show_mod_init_func.py to display the
function names.

BUG= 628052 
R=mark@chromium.org,dpranke@chromium.org

Review-Url: https://codereview.chromium.org/2163373008
Cr-Commit-Position: refs/heads/master@{#406999}

[modify] https://crrev.com/7ca703764dc9a08fa26537ca96ca5cc0edb01df4/infra/scripts/legacy/scripts/slave/chromium/sizes.py

Status: Fixed (was: Started)
Thank you for making the sizes step work in gn builds!

Sign in to add a comment