Skip to content

Commit

Permalink
Revert "Move static initializer check to a testing script"
Browse files Browse the repository at this point in the history
This reverts commit 7268f2d.

Reason for revert: Breaks sizes step of Google Chrome Mac and Windows

https://ci.chromium.org/buildbot/chromium.chrome/Google%20Chrome%20Mac/39214

sizes.py: error: no such option: --failures

Original change's description:
> Move static initializer check to a testing script
> 
> R=​thakis,dpranke
> 
> Bug: 110002
> Change-Id: I8da8e50930e973bd4bfac261571a66b7bad92778
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng;luci.chromium.try:win_archive;luci.chromium.try:win_x64_archive;master.tryserver.chromium.android:android_archive_rel_ng
> Reviewed-on: https://chromium-review.googlesource.com/c/1351716
> Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612544}

TBR=thakis@chromium.org,dpranke@chromium.org,thomasanderson@chromium.org

Change-Id: I3bfdb124c0fd18ba422553be8a78bc370d6e35f2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 110002
Cq-Include-Trybots: luci.chromium.try:linux_chromium_archive_rel_ng;luci.chromium.try:mac_chromium_archive_rel_ng;luci.chromium.try:win_archive;luci.chromium.try:win_x64_archive;master.tryserver.chromium.android:android_archive_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/1356439
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612573}
  • Loading branch information
Giovanni Ortuño Urquidi authored and Commit Bot committed Nov 30, 2018
1 parent a0ddfab commit 246dedf
Show file tree
Hide file tree
Showing 11 changed files with 176 additions and 293 deletions.
139 changes: 139 additions & 0 deletions infra/scripts/legacy/scripts/slave/chromium/sizes.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,27 @@
sys.path.append(os.path.join(SRC_DIR, 'third_party', 'catapult', 'tracing'))
from tracing.value import convert_chart_json

# If something adds a static initializer, revert it, don't increase these
# numbers. We don't accept regressions in static initializers.
#
# Note: Counts for chrome and nacl_helper are one higher in branded builds
# compared to release builds. This is due to a static initializer in
# WelsThreadPool.cpp (https://crbug.com/893594).
EXPECTED_LINUX_SI_COUNTS = {
'chrome': 5,
'nacl_helper': 5,
'nacl_helper_bootstrap': 0,
}

# If something adds a static initializer, revert it, don't increase these
# numbers. We don't accept regressions in static initializers.
EXPECTED_MAC_SI_COUNT = 1 # https://crbug.com/893594


class ResultsCollector(object):
def __init__(self):
self.results = {}
self.failures = []

def add_result(self, name, identifier, value, units):
assert name not in self.results
Expand All @@ -49,6 +67,9 @@ def add_result(self, name, identifier, value, units):
# Legacy printing, previously used for parsing the text logs.
print 'RESULT %s: %s= %s %s' % (name, identifier, value, units)

def add_failure(self, failure):
self.failures.append(failure)


def get_size(filename):
return os.stat(filename)[stat.ST_SIZE]
Expand Down Expand Up @@ -83,6 +104,12 @@ def run_process(result, command):
return result, stdout


def print_si_fail_hint(path_to_tool):
"""Print a hint regarding how to handle a static initializer failure."""
print '# HINT: To get this list, run %s' % path_to_tool
print '# HINT: diff against the log from the last run to see what changed'


def main_mac(options, args, results_collector):
"""Print appropriate size information about built Mac targets.
Expand Down Expand Up @@ -146,6 +173,55 @@ def main_mac(options, args, results_collector):
du_s = re.search(r'(\d+)', stdout).group(1)
print_dict['app_bundle_size'] = (int(du_s) * 1024)

# Count the number of files with at least one static initializer.
si_count = 0
# Find the __DATA,__mod_init_func section.
result, stdout = run_process(result,
['otool', '-l', chromium_framework_executable])
section_index = stdout.find('sectname __mod_init_func')
if section_index != -1:
# If the section exists, the "size" line must follow it.
initializers_s = re.search('size 0x([0-9a-f]+)',
stdout[section_index:]).group(1)
word_size = 8 # Assume 64 bit
si_count = int(initializers_s, 16) / word_size
print_dict['initializers'] = si_count

# For Release builds only, use dump-static-initializers.py to print the
# list of static initializers.
if si_count > EXPECTED_MAC_SI_COUNT and options.target == 'Release':
result = 125
results_collector.add_failure(
'Expected 0 static initializers in %s, but found %d' %
(chromium_framework_executable, si_count))
print '\n# Static initializers in %s:' % chromium_framework_executable

# First look for a dSYM to get information about the initializers. If
# one is not present, check if there is an unstripped copy of the build
# output.
mac_tools_path = os.path.join(os.path.dirname(build_dir),
'tools', 'mac')
if os.path.exists(chromium_framework_dsym):
dump_static_initializers = os.path.join(
mac_tools_path, 'dump-static-initializers.py')
result, stdout = run_process(result, [dump_static_initializers,
chromium_framework_dsym])
print_si_fail_hint('tools/mac/dump-static-initializers.py')
print stdout
else:
show_mod_init_func = os.path.join(
mac_tools_path, 'show_mod_init_func.py')
args = [show_mod_init_func]
if os.path.exists(chromium_framework_unstripped):
args.append(chromium_framework_unstripped)
else:
print '# Warning: Falling back to potentially stripped output.'
args.append(chromium_framework_executable)
result, stdout = run_process(result, args)
print_si_fail_hint('tools/mac/show_mod_init_func.py')
print stdout


results_collector.add_result(
print_dict['app_name'], print_dict['app_name'],
print_dict['app_size'], 'bytes')
Expand Down Expand Up @@ -173,6 +249,9 @@ def main_mac(options, args, results_collector):
results_collector.add_result(
print_dict['app_bundle'], print_dict['app_bundle'],
print_dict['app_bundle_size'], 'bytes')
results_collector.add_result(
'chrome-si', 'initializers',
print_dict['initializers'], 'files')

# Found a match, don't check the other base_names.
return result
Expand Down Expand Up @@ -200,6 +279,15 @@ def check_linux_binary(target_dir, binary_name, options, results_collector):
result = 0
sizes = []

def get_elf_section_size(readelf_stdout, section_name):
# Matches: .ctors PROGBITS 000000000516add0 5169dd0 000010 00 WA 0 0 8
match = re.search(r'\.%s.*$' % re.escape(section_name),
readelf_stdout, re.MULTILINE)
if not match:
return (False, -1)
size_str = re.split(r'\W+', match.group(0))[5]
return (True, int(size_str, 16))

sizes.append((binary_name, binary_name, 'size',
get_size(binary_file), 'bytes'))

Expand All @@ -216,6 +304,51 @@ def check_linux_binary(target_dir, binary_name, options, results_collector):
(binary_name + '-bss', 'bss', '', bss, 'bytes'),
]

# Find the number of files with at least one static initializer.
# First determine if we're 32 or 64 bit
result, stdout = run_process(result, ['readelf', '-h', binary_file])
elf_class_line = re.search('Class:.*$', stdout, re.MULTILINE).group(0)
elf_class = re.split(r'\W+', elf_class_line)[1]
if elf_class == 'ELF32':
word_size = 4
else:
word_size = 8

# Then find the number of files with global static initializers.
# NOTE: this is very implementation-specific and makes assumptions
# about how compiler and linker implement global static initializers.
si_count = 0
result, stdout = run_process(result, ['readelf', '-SW', binary_file])
has_init_array, init_array_size = get_elf_section_size(stdout, 'init_array')
if has_init_array:
si_count = init_array_size / word_size
# In newer versions of gcc crtbegin.o inserts frame_dummy into .init_array
# but we don't want to count this entry, since its alwasys present and not
# related to our code.
si_count -= 1
si_count = max(si_count, 0)
sizes.append((binary_name + '-si', 'initializers', '', si_count, 'files'))

# For Release builds only, use dump-static-initializers.py to print the list
# of static initializers.
if options.target == 'Release':
if (binary_name in EXPECTED_LINUX_SI_COUNTS and
si_count > EXPECTED_LINUX_SI_COUNTS[binary_name]):
result = 125
results_collector.add_failure(
'Expected <= %d static initializers in %s, but found %d' %
(EXPECTED_LINUX_SI_COUNTS[binary_name], binary_name, si_count))
if si_count > 0:
build_dir = os.path.dirname(target_dir)
dump_static_initializers = os.path.join(os.path.dirname(build_dir),
'tools', 'linux',
'dump-static-initializers.py')
result, stdout = run_process(result, [dump_static_initializers,
'-d', binary_file])
print '\n# Static initializers in %s:' % binary_file
print_si_fail_hint('tools/linux/dump-static-initializers.py')
print stdout

# Determine if the binary has the DT_TEXTREL marker.
result, stdout = run_process(result, ['readelf', '-Wd', binary_file])
if re.search(r'\bTEXTREL\b', stdout) is None:
Expand Down Expand Up @@ -439,6 +572,8 @@ def main():
help='specify platform (%s) [default: %%default]'
% ', '.join(platforms))
option_parser.add_option('--json', help='Path to JSON output file')
option_parser.add_option('--failures',
help='Path to JSON output file for failures')
# This needs to be --output-dir (and not something like --output-directory) in
# order to work properly with the build-side runtest.py script that's
# currently used for dashboard uploading results from this script.
Expand Down Expand Up @@ -484,6 +619,10 @@ def main():
with open(histogram_path, 'w') as f:
f.write(histogram_result.stdout)

if options.failures:
with open(options.failures, 'w') as f:
json.dump(results_collector.failures, f)

return rc


Expand Down
4 changes: 0 additions & 4 deletions testing/buildbot/chromium.fyi.json
Original file line number Diff line number Diff line change
Expand Up @@ -7454,10 +7454,6 @@
}
],
"scripts": [
{
"name": "check_static_initializers",
"script": "check_static_initializers.py"
},
{
"name": "checkdeps",
"script": "checkdeps.py"
Expand Down
24 changes: 0 additions & 24 deletions testing/buildbot/chromium.gpu.json
Original file line number Diff line number Diff line change
Expand Up @@ -1542,18 +1542,6 @@
"shards": 2
}
}
],
"scripts": [
{
"name": "check_static_initializers",
"script": "check_static_initializers.py",
"swarming": {}
},
{
"name": "webkit_lint",
"script": "webkit_lint.py",
"swarming": {}
}
]
},
"Mac Retina Debug (AMD)": {
Expand Down Expand Up @@ -2229,18 +2217,6 @@
"shards": 2
}
}
],
"scripts": [
{
"name": "check_static_initializers",
"script": "check_static_initializers.py",
"swarming": {}
},
{
"name": "webkit_lint",
"script": "webkit_lint.py",
"swarming": {}
}
]
},
"Win10 Debug (NVIDIA)": {
Expand Down
8 changes: 0 additions & 8 deletions testing/buildbot/chromium.linux.json
Original file line number Diff line number Diff line change
Expand Up @@ -1648,10 +1648,6 @@
}
],
"scripts": [
{
"name": "check_static_initializers",
"script": "check_static_initializers.py"
},
{
"name": "checkdeps",
"script": "checkdeps.py"
Expand Down Expand Up @@ -4285,10 +4281,6 @@
}
],
"scripts": [
{
"name": "check_static_initializers",
"script": "check_static_initializers.py"
},
{
"name": "checkdeps",
"script": "checkdeps.py"
Expand Down
5 changes: 0 additions & 5 deletions testing/buildbot/chromium.mac.json
Original file line number Diff line number Diff line change
Expand Up @@ -4531,11 +4531,6 @@
}
],
"scripts": [
{
"name": "check_static_initializers",
"script": "check_static_initializers.py",
"swarming": {}
},
{
"name": "webkit_lint",
"script": "webkit_lint.py",
Expand Down
13 changes: 0 additions & 13 deletions testing/buildbot/gn_isolate_map.pyl
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,6 @@
# The optional "script" field is used when "type" == "script", and
# specifies the GN path to the corresponding python file, e.g.
# "//testing/scripts/foo.py".
#
# The optional "skip_usage_check" field indicates that we should skip the check
# that the target is used in at least one buildbot json file.

{
"All_syzygy": {
Expand Down Expand Up @@ -924,16 +921,6 @@
"../../tools/metrics/metrics_python_tests.py"
]
},
"nacl_helper": {
"label": "//components/nacl/loader:nacl_helper",
"type": "additional_compile_target",
"skip_usage_check": True,
},
"nacl_helper_bootstrap": {
"label": "//native_client/src/trusted/service_runtime/linux:bootstrap",
"type": "additional_compile_target",
"skip_usage_check": True,
},
"nacl_helper_nonsfi_unittests": {
"label": "//components/nacl/loader:nacl_helper_nonsfi_unittests",
"type": "raw",
Expand Down
6 changes: 2 additions & 4 deletions testing/buildbot/manage.py
Original file line number Diff line number Diff line change
Expand Up @@ -450,10 +450,8 @@ def main():
ninja_targets, ninja_targets_seen):
result = 1

skip_targets = [k for k, v in gn_isolate_map.items() if
('skip_usage_check' in v and v['skip_usage_check'])]
extra_targets = (set(ninja_targets) - set(skip_targets) -
ninja_targets_seen - SKIP_GN_ISOLATE_MAP_TARGETS)
extra_targets = (set(ninja_targets) - ninja_targets_seen -
SKIP_GN_ISOLATE_MAP_TARGETS)
if extra_targets:
if len(extra_targets) > 1:
extra_targets_str = ', '.join(extra_targets) + ' are'
Expand Down
16 changes: 2 additions & 14 deletions testing/buildbot/test_suites.pyl
Original file line number Diff line number Diff line change
Expand Up @@ -699,27 +699,15 @@
'checkperms': {
'script': 'checkperms.py',
},
'check_static_initializers': {
'script': 'check_static_initializers.py',
},
'webkit_lint': {
'script': 'webkit_lint.py',
},
},

'chromium_mac_scripts': {
'check_static_initializers': {
'script': 'check_static_initializers.py',
},
'webkit_lint': {
'script': 'webkit_lint.py',
},
}
},

'chromium_scripts': {
'webkit_lint': {
'script': 'webkit_lint.py',
},
}
},

'chromium_swarm_android_gtests': {
Expand Down
4 changes: 1 addition & 3 deletions testing/buildbot/waterfalls.pyl
Original file line number Diff line number Diff line change
Expand Up @@ -1801,7 +1801,6 @@
'test_suites': {
'gtest_tests': 'gpu_desktop_gtests',
'gpu_telemetry_tests': 'gpu_common_telemetry_tests',
'scripts': 'chromium_mac_scripts',
},
},
'Mac Retina Debug (AMD)': {
Expand All @@ -1824,7 +1823,6 @@
'test_suites': {
'gtest_tests': 'gpu_desktop_gtests',
'gpu_telemetry_tests': 'gpu_common_telemetry_tests',
'scripts': 'chromium_mac_scripts',
},
},
'Win10 Debug (NVIDIA)': {
Expand Down Expand Up @@ -3060,7 +3058,7 @@
'test_suites': {
'gtest_tests': 'chromium_mac_gtests',
'isolated_scripts': 'chromium_rel_isolated_scripts',
'scripts': 'chromium_mac_scripts',
'scripts': 'chromium_scripts',
},
},
'Mac10.13 Tests (dbg)': {
Expand Down
Loading

0 comments on commit 246dedf

Please sign in to comment.