Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1934853002:
👕 Fix Android arm64 executables not linking in component builds (Closed)
DescriptionFix Android arm64 executables not linking in component builds
BUG=none
Committed: https://crrev.com/e9c92506df89b97f9cd0e7f027d178784224e7f0
Cr-Commit-Position: refs/heads/master@{#391119}
Patch Set 1 #Patch Set 2 : Also add sysroot #
Total comments: 3
Messages
Total messages: 25 (9 generated)
Description was changed from ========== Fix Android arm64 executables not linking in component builds BUG=none ========== to ========== Fix Android arm64 executables not linking in component builds BUG=none ==========
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org
agrieve@chromium.org changed reviewers: + dpranke@google.com
On 2016/05/02 16:47:57, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:dpranke@google.com Dirk - wondering if you know enough about sysroots to know whether this is the best fix. Verified that I can build all targets with arm64 with it, but gold doesn't seem to require it.
On 2016/05/02 16:49:16, agrieve wrote: > On 2016/05/02 16:47:57, agrieve wrote: > > mailto:agrieve@chromium.org changed reviewers: > > + mailto:dpranke@google.com > > Dirk - wondering if you know enough about sysroots to know whether this is the > best fix. Verified that I can build all targets with arm64 with it, but gold > doesn't seem to require it. Tried building one of the failing targets in GYP to compare flags, but it didn't even compile in GYP (probably due to bots not building "all" for GYP)
https://codereview.chromium.org/1934853002/diff/20001/build/config/android/BU... File build/config/android/BUILD.gn (right): https://codereview.chromium.org/1934853002/diff/20001/build/config/android/BU... build/config/android/BUILD.gn:185: if (!use_gold) { Why is this only in executable_config? Do we need it for building shared libraries, too? Why doesn't this belong up in the compiler config?
https://codereview.chromium.org/1934853002/diff/20001/build/config/android/BU... File build/config/android/BUILD.gn (right): https://codereview.chromium.org/1934853002/diff/20001/build/config/android/BU... build/config/android/BUILD.gn:185: if (!use_gold) { On 2016/05/02 17:18:05, jbudorick wrote: > Why is this only in executable_config? Do we need it for building shared > libraries, too? Why doesn't this belong up in the compiler config? For the most part, I don't know, but it doesn't seem to be needed there.
https://codereview.chromium.org/1934853002/diff/20001/build/config/android/BU... File build/config/android/BUILD.gn (right): https://codereview.chromium.org/1934853002/diff/20001/build/config/android/BU... build/config/android/BUILD.gn:185: if (!use_gold) { On 2016/05/02 17:20:33, agrieve wrote: > On 2016/05/02 17:18:05, jbudorick wrote: > > Why is this only in executable_config? Do we need it for building shared > > libraries, too? Why doesn't this belong up in the compiler config? > > For the most part, I don't know, but it doesn't seem to be needed there. I'm reading this as "the build completed locally this way." Is that correct?
On 2016/05/02 17:40:33, jbudorick wrote: > https://codereview.chromium.org/1934853002/diff/20001/build/config/android/BU... > File build/config/android/BUILD.gn (right): > > https://codereview.chromium.org/1934853002/diff/20001/build/config/android/BU... > build/config/android/BUILD.gn:185: if (!use_gold) { > On 2016/05/02 17:20:33, agrieve wrote: > > On 2016/05/02 17:18:05, jbudorick wrote: > > > Why is this only in executable_config? Do we need it for building shared > > > libraries, too? Why doesn't this belong up in the compiler config? > > > > For the most part, I don't know, but it doesn't seem to be needed there. > > I'm reading this as "the build completed locally this way." Is that correct? correct.
I suppose I'm ok with trying the narrowly-scoped solution to start. lgtm
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934853002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934853002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934853002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934853002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix Android arm64 executables not linking in component builds BUG=none ========== to ========== Fix Android arm64 executables not linking in component builds BUG=none ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix Android arm64 executables not linking in component builds BUG=none ========== to ========== Fix Android arm64 executables not linking in component builds BUG=none Committed: https://crrev.com/e9c92506df89b97f9cd0e7f027d178784224e7f0 Cr-Commit-Position: refs/heads/master@{#391119} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e9c92506df89b97f9cd0e7f027d178784224e7f0 Cr-Commit-Position: refs/heads/master@{#391119}
Message was sent while issue was closed.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Message was sent while issue was closed.
+mcgrathr ... On 2016/05/02 16:47:57, agrieve wrote: > Dirk - wondering if you know enough about sysroots to know > whether this is the best fix. Verified that I can build all targets > with arm64 with it, but gold doesn't seem to require it. Without seeing more of the context (like the actual linker errors, and knowing how the shared libraries end up getting packaged into the apk), it's hard for me to say. What little I know/understand about -rpath-link makes me suspect that this is covering up an underlying problem. However, I also vaguely remember mcgrathr@ telling me that -rpath-link wasn't needed any more for other parts of the chromium build, and I don't know if that was because gold didn't need it but ld did, or if it was only older versions of ld that needed it, or if that was because we had fixed the directory layout to not have problems, or something else. Maybe Roland can say more? That said, use_gold is also a command-line settable arg (set in //build/config/compiler/BUILD.gn), so setting it as a local variable the way you're doing here means that you'll override the args.gn entry, which is perhaps intentional or correct, but also confusing. I also don't particularly like having some gold-specific settings in one file and some in another. Parts of the //build/config/compiler file make comments about being "linux specific", but other parts are more generic, and it would probably be better to have all of the gold-specific logic in one place.
Message was sent while issue was closed.
-rpath-link was never about finding the libraries you link to via -lfoobar. That's what -L does. The only purpose of -rpath-link is when searching for a library you didn't link to, but that one you did link to refers to via DT_NEEDED. So if you link with -lfoobar and libfoobar.so has a DT_NEEDED for libother.so, then the -rpath-link search path is where to look for libother.so. However, nowadays the linker never looks for indirect libraries like that. Gold has never done so (it just ignores -rpath-link). Older vintages of GNU (BFD) ld did, but the default changed in 2.22 (Ubuntu 14.04 has 2.24) and so for years now you get that behavior only with --copy-dt-needed-entries (previously called --add-needed, name changed to avoid confusion with --as-needed). The upshot is that in linkers used today, you can't resolve a reference to some symbol defined in libother.so at link time without specifying -lother explicitly, so the linker never has a reason to search for libother.so, so -rpath-link never has any purpose. I'm pretty confident that you could remove all -rpath-link uses throughout the build today and not have anything change.
Message was sent while issue was closed.
On 2016/05/03 17:21:45, Roland McGrath wrote: > -rpath-link was never about finding the libraries you link to via -lfoobar. > That's what -L does. > > The only purpose of -rpath-link is when searching for a library you didn't link > to, but that one you did link to refers to via DT_NEEDED. So if you link with > -lfoobar and libfoobar.so has a DT_NEEDED for libother.so, then the -rpath-link > search path is where to look for libother.so. However, nowadays the linker > never looks for indirect libraries like that. Gold has never done so (it just > ignores -rpath-link). Older vintages of GNU (BFD) ld did, but the default > changed in 2.22 (Ubuntu 14.04 has 2.24) and so for years now you get that > behavior only with --copy-dt-needed-entries (previously called --add-needed, > name changed to avoid confusion with --as-needed). The upshot is that in > linkers used today, you can't resolve a reference to some symbol defined in > libother.so at link time without specifying -lother explicitly, so the linker > never has a reason to search for libother.so, so -rpath-link never has any > purpose. > > I'm pretty confident that you could remove all -rpath-link uses throughout the > build today and not have anything change. Thanks for the explanation! Certainly clears up a bit of what I'm seeing. If there's a better way to fix this, then I think it's certainly worth figuring out. Before this change (without -rpath-link), the compile for arm64 fails. It's using a version of ld that comes in the android sdk, so it might be that it's quite old. It even fails to locate .so files within $sysroot/usr/lib, which is a bit depressing :P.
Message was sent while issue was closed.
On 2016/05/03 02:56:29, Dirk Pranke wrote: > +mcgrathr ... > > On 2016/05/02 16:47:57, agrieve wrote: > > Dirk - wondering if you know enough about sysroots to know > > whether this is the best fix. Verified that I can build all targets > > with arm64 with it, but gold doesn't seem to require it. > > Without seeing more of the context (like the actual linker errors, and knowing > how the shared libraries end up getting packaged into the apk), it's hard for me > to say. > > What little I know/understand about -rpath-link makes me suspect that this is > covering up an underlying problem. > > However, I also vaguely remember mcgrathr@ telling me that -rpath-link wasn't > needed any more for other parts of the chromium build, and I don't know if that > was because gold didn't need it but ld did, or if it was only older versions of > ld that needed it, or if that was because we had fixed the directory layout to > not have problems, or something else. > > Maybe Roland can say more? > > That said, use_gold is also a command-line settable arg (set in > //build/config/compiler/BUILD.gn), so setting it as a local variable the way > you're doing here means that you'll override the args.gn entry, which is perhaps > intentional or correct, but also confusing. > > I also don't particularly like having some gold-specific settings in one file > and some in another. Parts of the //build/config/compiler file make comments > about being "linux specific", but other parts are more generic, and it would > probably be better to have all of the gold-specific logic in one place. Going to wait a bit more to see if there's a better solution, but will do a follow-up to fix the use_gold collision and look at moving where the code is. |
