Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797163006:
Android: update static initializer checks in resource_sizes.py. (Closed)
|
Created:
3 years, 8 months ago by estevenson Modified:
3 years, 8 months ago Reviewers:
agrieve CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: update static initializer checks in resource_sizes.py.
In resource_sizes.py, we check for static initializers using readelf
and using dump-static-initializers.py. The latter requires an
unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a
32 bit binary is also included but since this binary isn't located in
the normal location ($OUTDIR/lib.unstripped/), we currently just check
the 64 bit binary twice.
This also makes |_VerifyBuildIdsMatch()| fail (which was previously
added incorrectly in https://codereview.chromium.org/2706243013) since
we try to verify the build ids of two different binaries (32 and 64 bit
versions).
To simplify this, we ignore 32 bit .so files when 64 bit .so file are
present since the 32 bit .so files will be checked by other bots.
BUG=708942
Review-Url: https://codereview.chromium.org/2797163006
Cr-Commit-Position: refs/heads/master@{#463284}
Committed: https://chromium.googlesource.com/chromium/src/+/7ad22b20258b96ac8c6da01059e0eb1b321e56f1
Patch Set 1 #Patch Set 2 : Update comment #Patch Set 3 : n -> -n #
Total comments: 1
Patch Set 4 : Use agrieve@ suggestion #2 #
Total comments: 1
Patch Set 5 : Add comment about 64/32 bit si checks #Patch Set 6 : Add back GetStaticInitializers comment #Messages
Total messages: 18 (11 generated)
Description was changed from ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build id of the 32 and 64 bit binary. To simply this, we only perform the dump-static-initializers check in the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ========== to ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build id of a 32 and 64 bit binary. To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ==========
Description was changed from ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build id of a 32 and 64 bit binary. To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ========== to ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ==========
Description was changed from ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ========== to ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ==========
Description was changed from ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ========== to ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ==========
Description was changed from ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ========== to ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ==========
estevenson@chromium.org changed reviewers: + agrieve@chromium.org
ptal Andrew. Is it even worth keeping the dump-static-initializers check since we don't report this value anywhere (we print it, but who's checking this?).
On 2017/04/06 18:17:58, estevenson wrote: > ptal Andrew. > > Is it even worth keeping the dump-static-initializers check since we don't > report this value anywhere (we print it, but who's checking this?). Thought about this a bit today, and I think what we should do is write a console.py query that can list them, and then just print out "use this tool to find them".
https://codereview.chromium.org/2797163006/diff/40001/build/android/resource_... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2797163006/diff/40001/build/android/resource_... build/android/resource_sizes.py:615: check_unstripped, saw_first_so = not saw_first_so, True This will not check the SIs in the crazy linker, which we probably still want to do. One idea: change the script to: 1. Show a warning & skip any .so that doesn't match the build id 2. Fail if no build ids matched Second idea: so_files = [x for x in z.infolist() if x.filename.endswith('.so') and x.file_size > 0] has_64 = any('64' in x.filename for x in so_files) files_to_check = [x for x in so_files if not has_64 or '64' in x.filename] This one skips extraction as well, which is nice. I think it's fine to entirely skip counting SIs of the 32 bit libs since the 32 bit builder will do it anyways.
Description was changed from ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simply this, we only perform the dump-static-initializers check on the largest .so file (64 bit binary is much larger than the 32 bit binary). BUG=708942 ========== to ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simplify this, we ignore 32 bit .so files when 64 bit .so file are present since the 32 bit .so files will be checked by other bots. BUG=708942 ==========
ptal Andrew. I thought we had added the readelf counting methods for sis for Android but after looking at sizes.py a little I learned that linux does the same thing (uses count of readelf-found sis but prints dump-static-initializer sis - and the same off-by-one error seems to happen there). There was also a recent change (https://codereview.chromium.org/2765983002/diff/40001/infra/scripts/legacy/sc...) that updated the readelf method of counting sis, it didn't change anything when I added it to resource_sizes.py so I left it out. This skips counting sis for 32 bit libs when 64 bit libs are present as you suggested.
lgtm /w nit. Probably a wait-until-monday thing though. https://codereview.chromium.org/2797163006/diff/60001/build/android/resource_... File build/android/resource_sizes.py (right): https://codereview.chromium.org/2797163006/diff/60001/build/android/resource_... build/android/resource_sizes.py:600: has_64 = any('64' in f.filename for f in so_files) would be good to add in some comments saying why we check only one architecture.
The CQ bit was checked by estevenson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2797163006/#ps100001 (title: "Add back GetStaticInitializers comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491831473863400,
"parent_rev": "2cd83da8d67df6634cb770c559f7e588b5ae25b5", "commit_rev":
"7ad22b20258b96ac8c6da01059e0eb1b321e56f1"}
Message was sent while issue was closed.
Description was changed from ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simplify this, we ignore 32 bit .so files when 64 bit .so file are present since the 32 bit .so files will be checked by other bots. BUG=708942 ========== to ========== Android: update static initializer checks in resource_sizes.py. In resource_sizes.py, we check for static initializers using readelf and using dump-static-initializers.py. The latter requires an unstripped .so file. In some 64 bit APKs (i.e. MonochromePublic.apk), a 32 bit binary is also included but since this binary isn't located in the normal location ($OUTDIR/lib.unstripped/), we currently just check the 64 bit binary twice. This also makes |_VerifyBuildIdsMatch()| fail (which was previously added incorrectly in https://codereview.chromium.org/2706243013) since we try to verify the build ids of two different binaries (32 and 64 bit versions). To simplify this, we ignore 32 bit .so files when 64 bit .so file are present since the 32 bit .so files will be checked by other bots. BUG=708942 Review-Url: https://codereview.chromium.org/2797163006 Cr-Commit-Position: refs/heads/master@{#463284} Committed: https://chromium.googlesource.com/chromium/src/+/7ad22b20258b96ac8c6da01059e0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/7ad22b20258b96ac8c6da01059e0... |
