Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2732293002:
build: Prepare for building clang without ENABLE_LINKER_BUILD_ID=ON (Closed)
Descriptionbuild: Prepare for building clang without ENABLE_LINKER_BUILD_ID=ON
BUG=622775
Review-Url: https://codereview.chromium.org/2732293002
Cr-Commit-Position: refs/heads/master@{#455154}
Committed: https://chromium.googlesource.com/chromium/src/+/4e5c4a1545902ab817b8d758c8b3ec4380552b5d
Patch Set 1 #Patch Set 2 : explicit sha1, remove android #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 25 (9 generated)
The CQ bit was checked by thakis@chromium.org to run a CQ dry run
thakis@chromium.org changed reviewers: + hans@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thakis@chromium.org changed reviewers: + torne@chromium.org
+torne since i happened to see https://codereview.chromium.org/1801073002. Torne, should we remove the android-specific version of this after this change here? Also, I thought gcc always implicitly requested --build-id, why was your change even necessary?
Do you know if any crash handling tests will catch if we fail to include the build it?
On 2017/03/07 17:16:24, Nico wrote: > +torne since i happened to see https://codereview.chromium.org/1801073002. > Torne, should we remove the android-specific version of this after this change > here? Seems like that should be fine, but I'd want to check that we definitely end up with the right flags because breaking this is easy to go unnoticed :/ > Also, I thought gcc always implicitly requested --build-id, why was your change > even necessary? It definitely wasn't enabled by default at the point I made that other CL, with the NDK toolchain.
On 2017/03/07 17:18:29, hans wrote: > Do you know if any crash handling tests will catch if we fail to include the > build it? I don't think so; breakpad will just generate an ID from the file contents (in a way that has lots of collisions..)
lgtm It would be really nice if something checked for this though.
In patch set 2, I removed the setting from build/config/android/BUILD.gn. This means android will only get this if is_official_build is set, matching non-android (non-mac non-ios) posix. I also explicitly set it to sha1 -- the GNU docs say that that's the default if nothing's explicitly selected, but lld apparently defaults to something else for no real reason, so I figure it's better to be explici.
lgtm - we shouldn't really need this in non-official builds.
The CQ bit was checked by thakis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hans@chromium.org Link to the patchset: https://codereview.chromium.org/2732293002/#ps20001 (title: "explicit sha1, remove android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mcgrathr@chromium.org changed reviewers: + mcgrathr@chromium.org
https://codereview.chromium.org/2732293002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2732293002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:287: ldflags += [ "-Wl,--build-id=sha1" ] Gold supports --build-id=tree, which is much faster.
https://codereview.chromium.org/2732293002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2732293002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:287: ldflags += [ "-Wl,--build-id=sha1" ] On 2017/03/07 18:16:13, Roland McGrath wrote: > Gold supports --build-id=tree, which is much faster. lld supports that too, but apparently just maps it to sha1 :-D Does tree produce the same number? Do you know if crash relies on getting sha1s? I'm trying to not change behavior for official binaries for now, and just make non-official links faster for now.
On 2017/03/07 18:37:46, Nico wrote: > https://codereview.chromium.org/2732293002/diff/20001/build/config/compiler/B... > File build/config/compiler/BUILD.gn (right): > > https://codereview.chromium.org/2732293002/diff/20001/build/config/compiler/B... > build/config/compiler/BUILD.gn:287: ldflags += [ "-Wl,--build-id=sha1" ] > On 2017/03/07 18:16:13, Roland McGrath wrote: > > Gold supports --build-id=tree, which is much faster. > > lld supports that too, but apparently just maps it to sha1 :-D > > Does tree produce the same number? Do you know if crash relies on getting sha1s? > > I'm trying to not change behavior for official binaries for now, and just make > non-official links faster for now. It doesn't produce the same number, nor does it need to. It does produce a unique-enough value that is reproducible in an otherwise identical re-build, which are the only requirements for a build ID.
On 2017/03/07 18:39:44, Roland McGrath wrote: > On 2017/03/07 18:37:46, Nico wrote: > > > https://codereview.chromium.org/2732293002/diff/20001/build/config/compiler/B... > > File build/config/compiler/BUILD.gn (right): > > > > > https://codereview.chromium.org/2732293002/diff/20001/build/config/compiler/B... > > build/config/compiler/BUILD.gn:287: ldflags += [ "-Wl,--build-id=sha1" ] > > On 2017/03/07 18:16:13, Roland McGrath wrote: > > > Gold supports --build-id=tree, which is much faster. > > > > lld supports that too, but apparently just maps it to sha1 :-D > > > > Does tree produce the same number? Do you know if crash relies on getting > sha1s? > > > > I'm trying to not change behavior for official binaries for now, and just make > > non-official links faster for now. > > It doesn't produce the same number, nor does it need to. It does produce a > unique-enough value that is reproducible in an otherwise identical re-build, > which are the only requirements for a build ID. Cool, I'll check with the crash folks that they only use the build ID as build ID and then change this in a follow-up. Thanks!
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488907830797560,
"parent_rev": "9c3f741022b223c56abced353ad007e8c44c5630", "commit_rev":
"4e5c4a1545902ab817b8d758c8b3ec4380552b5d"}
Message was sent while issue was closed.
Description was changed from ========== build: Prepare for building clang without ENABLE_LINKER_BUILD_ID=ON BUG=622775 ========== to ========== build: Prepare for building clang without ENABLE_LINKER_BUILD_ID=ON BUG=622775 Review-Url: https://codereview.chromium.org/2732293002 Cr-Commit-Position: refs/heads/master@{#455154} Committed: https://chromium.googlesource.com/chromium/src/+/4e5c4a1545902ab817b8d758c8b3... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/4e5c4a1545902ab817b8d758c8b3...
Message was sent while issue was closed.
Yeah, I'm pretty sure it just needs to be a unique ID, though I'm not sure if there are any constraints on the length? The problem we had before on Android was that when there's no build ID note in the binary at all breakpad generates one from a trivial checksum of a few pages of .text and because we sort our binaries with a not-always-fresh linker orderfile, this was colliding a *lot* due to the functions sorted to the beginning not having their contents change in every build. So, any reasonably good hash of the entire binary should be fine.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2732023004/ by thakis@chromium.org. The reason for reverting is: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrom... pnacl-ld: Unrecognized option: --build-id=sha1 gn args: goma_dir = "/b/c/goma_client" is_chrome_branded = true is_debug = false is_official_build = true target_os = "chromeos" use_goma = true. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
