Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2094273003:
Disable --as-needed for libpthread to work around linker bug. (Closed)
DescriptionDisable --as-needed for libpthread to work around linker bug.
If a DSO is linked against with --as-needed and the program contains no
strong references to any symbols in the DSO, the documented behaviour
is that the linker should not create a DT_NEEDED entry for that
DSO. However, if the program contains weak references to symbols in
the DSO, this results in an incorrect resolution for that symbol. In
Chromium this happens in particular for the symbol __pthread_key_create
(as a result of code inlined from a pthread header), which causes
the program to segfault. For more details, see the associated bug
and the related LLVM bug: http://llvm.org/pr28335
Work around this by linking against libpthread with --no-as-needed.
Because the problem only affects glibc, only do this when not
targeting NaCl or Android, neither of which normally uses glibc
as the C runtime library (NaCl can use glibc, but we haven't
observed this problem there).
BUG=623236
R=thakis@chromium.org,engedy@chromium.org
Committed: https://crrev.com/7ebfaa0c5067c77db1b4891ac6571f8d1b97fc4d
Cr-Commit-Position: refs/heads/master@{#402650}
Patch Set 1 #Patch Set 2 : Disable for nacl #Patch Set 3 : Disable for android #
Total comments: 2
Messages
Total messages: 23 (8 generated)
LGTM, thank you for preparing the workaround on such short notice.
Do all our binaries currently link against pthread? If not, after this change they will. Could that cause issues (say, for the sandbox wrapper, or something like that?)
On 2016/06/28 13:29:01, Nico wrote: > Do all our binaries currently link against pthread? If not, after this change > they will. Could that cause issues (say, for the sandbox wrapper, or something > like that?) Not as far as I'm aware. I also ran a quick local smoke test (started chrome, navigated to a page, closed it) with a sandbox wrapper linked against libpthread and didn't see any problems.
The CQ bit was checked by pcc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2094273003/#ps20001 (title: "Disable for nacl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thakis@: PTAL.
Looking through the output of `for f in out/gn/*; do if test -x $f -a ! -d $f ; then if ! (ldd $f | grep -q pthread); then echo $f; fi; fi; done`, this should be mostly ok. For nacl_helper_nonsfi, this worries me a bit, maybe you could check with a nacl person if they think this is fine. Other than that, lgtm.
On 2016/06/28 21:45:01, Nico wrote: > Looking through the output of `for f in out/gn/*; do if test -x $f -a ! -d $f ; > then if ! (ldd $f | grep -q pthread); then echo $f; fi; fi; done`, this should > be mostly ok. For nacl_helper_nonsfi, this worries me a bit, maybe you could > check with a nacl person if they think this is fine. Other than that, lgtm. It turned out that adding the -lpthread flag broke the link for nacl_helper_nonsfi, so I added an is_nacl guard to BUILD.gn (see diff between patch sets 1 and 2). No such change was required on the gyp side because of nacl's custom build system. I will explicitly call this out in the CL description.
https://codereview.chromium.org/2094273003/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2094273003/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1283: if (!is_nacl && !is_android) { what about chromeos? should this only happen if is_clang, and/or if lto is on?
Description was changed from ========== Disable --as-needed for libpthread to work around linker bug. If a DSO is linked against with --as-needed and the program contains no strong references to any symbols in the DSO, the documented behaviour is that the linker should not create a DT_NEEDED entry for that DSO. However, if the program contains weak references to symbols in the DSO, this results in an incorrect resolution for that symbol. In Chromium this happens in particular for the symbol __pthread_key_create (as a result of code inlined from a pthread header), which causes the program to segfault. For more details, see the associated bug and the related LLVM bug: http://llvm.org/pr28335 Work around this by linking against libpthread with --no-as-needed. BUG=623236 R=thakis@chromium.org,engedy@chromium.org ========== to ========== Disable --as-needed for libpthread to work around linker bug. If a DSO is linked against with --as-needed and the program contains no strong references to any symbols in the DSO, the documented behaviour is that the linker should not create a DT_NEEDED entry for that DSO. However, if the program contains weak references to symbols in the DSO, this results in an incorrect resolution for that symbol. In Chromium this happens in particular for the symbol __pthread_key_create (as a result of code inlined from a pthread header), which causes the program to segfault. For more details, see the associated bug and the related LLVM bug: http://llvm.org/pr28335 Work around this by linking against libpthread with --no-as-needed. Because the problem only affects glibc, only do this when not targeting NaCl or Android, neither of which normally uses glibc as the C runtime library (NaCl can use glibc, but we haven't observed this problem there). BUG=623236 R=thakis@chromium.org,engedy@chromium.org ==========
https://codereview.chromium.org/2094273003/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2094273003/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1283: if (!is_nacl && !is_android) { On 2016/06/28 21:56:23, Nico wrote: > what about chromeos? should this only happen if is_clang, and/or if lto is on? This should probably be testing for whether the standard library is glibc, but I'm not sure that we have a good way to test that. It's only by chance that we saw this issue with Clang+LTO; the same issue could happen with GCC depending on how code is inlined, or with any non-PIC object file (although I don't think our build system can build any of those). Regarding ChromeOS, according to [1] it uses glibc so I don't think we need to do anything about it. [1] http://unix.stackexchange.com/questions/269487/is-chromium-os-a-gnu-linux-distro
The CQ bit was checked by pcc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2094273003/#ps40001 (title: "Disable for android")
still lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Disable --as-needed for libpthread to work around linker bug. If a DSO is linked against with --as-needed and the program contains no strong references to any symbols in the DSO, the documented behaviour is that the linker should not create a DT_NEEDED entry for that DSO. However, if the program contains weak references to symbols in the DSO, this results in an incorrect resolution for that symbol. In Chromium this happens in particular for the symbol __pthread_key_create (as a result of code inlined from a pthread header), which causes the program to segfault. For more details, see the associated bug and the related LLVM bug: http://llvm.org/pr28335 Work around this by linking against libpthread with --no-as-needed. Because the problem only affects glibc, only do this when not targeting NaCl or Android, neither of which normally uses glibc as the C runtime library (NaCl can use glibc, but we haven't observed this problem there). BUG=623236 R=thakis@chromium.org,engedy@chromium.org ========== to ========== Disable --as-needed for libpthread to work around linker bug. If a DSO is linked against with --as-needed and the program contains no strong references to any symbols in the DSO, the documented behaviour is that the linker should not create a DT_NEEDED entry for that DSO. However, if the program contains weak references to symbols in the DSO, this results in an incorrect resolution for that symbol. In Chromium this happens in particular for the symbol __pthread_key_create (as a result of code inlined from a pthread header), which causes the program to segfault. For more details, see the associated bug and the related LLVM bug: http://llvm.org/pr28335 Work around this by linking against libpthread with --no-as-needed. Because the problem only affects glibc, only do this when not targeting NaCl or Android, neither of which normally uses glibc as the C runtime library (NaCl can use glibc, but we haven't observed this problem there). BUG=623236 R=thakis@chromium.org,engedy@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Disable --as-needed for libpthread to work around linker bug. If a DSO is linked against with --as-needed and the program contains no strong references to any symbols in the DSO, the documented behaviour is that the linker should not create a DT_NEEDED entry for that DSO. However, if the program contains weak references to symbols in the DSO, this results in an incorrect resolution for that symbol. In Chromium this happens in particular for the symbol __pthread_key_create (as a result of code inlined from a pthread header), which causes the program to segfault. For more details, see the associated bug and the related LLVM bug: http://llvm.org/pr28335 Work around this by linking against libpthread with --no-as-needed. Because the problem only affects glibc, only do this when not targeting NaCl or Android, neither of which normally uses glibc as the C runtime library (NaCl can use glibc, but we haven't observed this problem there). BUG=623236 R=thakis@chromium.org,engedy@chromium.org ========== to ========== Disable --as-needed for libpthread to work around linker bug. If a DSO is linked against with --as-needed and the program contains no strong references to any symbols in the DSO, the documented behaviour is that the linker should not create a DT_NEEDED entry for that DSO. However, if the program contains weak references to symbols in the DSO, this results in an incorrect resolution for that symbol. In Chromium this happens in particular for the symbol __pthread_key_create (as a result of code inlined from a pthread header), which causes the program to segfault. For more details, see the associated bug and the related LLVM bug: http://llvm.org/pr28335 Work around this by linking against libpthread with --no-as-needed. Because the problem only affects glibc, only do this when not targeting NaCl or Android, neither of which normally uses glibc as the C runtime library (NaCl can use glibc, but we haven't observed this problem there). BUG=623236 R=thakis@chromium.org,engedy@chromium.org Committed: https://crrev.com/7ebfaa0c5067c77db1b4891ac6571f8d1b97fc4d Cr-Commit-Position: refs/heads/master@{#402650} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7ebfaa0c5067c77db1b4891ac6571f8d1b97fc4d Cr-Commit-Position: refs/heads/master@{#402650} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
