Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1158133003:
Fix duplicate symbol in ARM build of nacl_helper_nonsfi_unittests (Closed)
DescriptionFix duplicate symbol in ARM build of nacl_helper_nonsfi_unittests
The arm linux is currently broken due to duplicate
definition of BPF_TEST_C_socketpair.
BUG=492731
R=hidehiko@chromium.org, mseaborn@chromium.org, hidehiko
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_arm_compile
Committed: https://chromium.googlesource.com/chromium/src/+/9e646abedf24cf3f4659674c2d2b603dfaef42a7
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 3
Patch Set 5 : nits #Messages
Total messages: 17 (3 generated)
sbc@chromium.org changed reviewers: + mseaborn@chromium.org
Thank you for your fix, and sorry for inconvenience. https://codereview.chromium.org/1158133003/diff/1/components/nacl/loader/nons... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/1158133003/diff/1/components/nacl/loader/nons... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:203: #if defined(OS_NACL_NONSFI) Would you mind to change this condition to: #if defined(OS_NACL_NONSFI) || defined(__x86_64__) || defined(__arm__) BPF_DEATH_TEST_C(..., socketpair, ...) { ... } #endif #if !defined(OS_NACL_NONSFI) BPF_TEST_C(..., socketcall_allowed, ...) { ... } #endif ?
https://codereview.chromium.org/1158133003/diff/1/components/nacl/loader/nons... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/1158133003/diff/1/components/nacl/loader/nons... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:203: #if defined(OS_NACL_NONSFI) On 2015/05/27 18:40:33, hidehiko wrote: > Would you mind to change this condition to: > > #if defined(OS_NACL_NONSFI) || defined(__x86_64__) || defined(__arm__) > BPF_DEATH_TEST_C(..., socketpair, ...) { > ... > } > #endif > > #if !defined(OS_NACL_NONSFI) > BPF_TEST_C(..., socketcall_allowed, ...) { > ... > } > #endif > > ? Done.
The CQ bit was checked by sbc@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/1158133003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/05/27 19:11:29, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Any idea why we are seeing those try failures
On 2015/05/27 19:53:40, Sam Clegg wrote: > On 2015/05/27 19:11:29, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux > (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Any idea why we are seeing those try failures Maybe something to do with the fact that one version of the test uses AF_UNIX and the other uses AF_INET?
Indeed, those two test cases can't be merged. It might be worth putting them next to each other for clarity. https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (left): https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:396: #if defined(__x86_64__) || defined(__arm__) Maybe add comment to explain background: The arguments to socketpair() are passed in registers on these architectures and so can be filtered. We don't apply this check on x86-32 where the arguments are passed in memory. https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:398: socketpair, Maybe rename to socketpair_af_inet_disallowed, to avoid the clash? https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:205: socketpair, Rather than your current change... Maybe rename to socketpair_af_unix_disallowed, to avoid the clash?
https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (left): https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:396: #if defined(__x86_64__) || defined(__arm__) On 2015/05/27 21:40:35, Mark Seaborn wrote: > Maybe add comment to explain background: > > The arguments to socketpair() are passed in registers on these architectures and > so can be filtered. We don't apply this check on x86-32 where the arguments are > passed in memory. Done. https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:398: socketpair, On 2015/05/27 21:40:35, Mark Seaborn wrote: > Maybe rename to socketpair_af_inet_disallowed, to avoid the clash? Done. https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:205: socketpair, On 2015/05/27 21:40:35, Mark Seaborn wrote: > Rather than your current change... > > Maybe rename to socketpair_af_unix_disallowed, to avoid the clash? Done.
On 2015/05/27 22:30:20, Sam Clegg wrote: > https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... > File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (left): > > https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... > components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:396: #if > defined(__x86_64__) || defined(__arm__) > On 2015/05/27 21:40:35, Mark Seaborn wrote: > > Maybe add comment to explain background: > > > > The arguments to socketpair() are passed in registers on these architectures > and > > so can be filtered. We don't apply this check on x86-32 where the arguments > are > > passed in memory. > > Done. > > https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... > components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:398: socketpair, > On 2015/05/27 21:40:35, Mark Seaborn wrote: > > Maybe rename to socketpair_af_inet_disallowed, to avoid the clash? > > Done. > > https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... > File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): > > https://codereview.chromium.org/1158133003/diff/20001/components/nacl/loader/... > components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:205: socketpair, > On 2015/05/27 21:40:35, Mark Seaborn wrote: > > Rather than your current change... > > > > Maybe rename to socketpair_af_unix_disallowed, to avoid the clash? > > Done. Could one of you take a look at this? I'd like to get the the ARM build fixed ASAP (today if possible).
lgtm
LGTM with comment fix https://codereview.chromium.org/1158133003/diff/60001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc (right): https://codereview.chromium.org/1158133003/diff/60001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:232: // On arm and x86_64 the argument to socketpair are passed in registers Nit: "arguments ... are" https://codereview.chromium.org/1158133003/diff/60001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:233: // so that can be filtered by seccomp-bpf. This filter cannot be applied "...registers so that" -> "...registers, so they" https://codereview.chromium.org/1158133003/diff/60001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_sandbox_unittest.cc:234: // on x86_32 as they arguments are passed in memory. "the arguments"
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9e646abedf24cf3f4659674c2d2b603dfaef42a7 Cr-Commit-Position: refs/heads/master@{#331802}
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 9e646abedf24cf3f4659674c2d2b603dfaef42a7 (presubmit successful). |
