Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1264783002:
Remove failure for bad mmap address hints. (Closed)
|
Created:
5 years, 4 months ago by Sean Klein Modified:
5 years, 4 months ago Reviewers:
Mark Seaborn CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/src/native_client.git@master Target Ref:
refs/heads/master Project:
nacl Visibility:
Public. |
DescriptionRemove failure for bad mmap address hints.
BUG=https://code.google.com/p/nativeclient/issues/detail?id=4100
Committed: https://chromium.googlesource.com/native_client/src/native_client/+/5a8384a542cf08e57425a063be58d17b682f1923
Patch Set 1 : Demonstrate that test for behavior DOES fail. #Patch Set 2 : Added fix for failing test #
Total comments: 8
Patch Set 3 : Responding to review #
Total comments: 2
Patch Set 4 : Dodging copyright presubmit warning #
Total comments: 3
Patch Set 5 : #Messages
Total messages: 24 (10 generated)
smklein@chromium.org changed reviewers: + mseaborn@chromium.org
Fixing a small mmap bug, PTAL
https://chromiumcodereview.appspot.com/1264783002/diff/20001/src/trusted/serv... File src/trusted/service_runtime/sys_memory.c (right): https://chromiumcodereview.appspot.com/1264783002/diff/20001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:332: if (NACL_ABI_MAP_FIXED & flags) { Nit: use "(NACL_ABI_MAP_FIXED & flags) != 0" NaCl style is not to implicitly convert to boolean. https://chromiumcodereview.appspot.com/1264783002/diff/20001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:334: map_result = -NACL_ABI_EINVAL; Is there a test case covering this code path? https://chromiumcodereview.appspot.com/1264783002/diff/20001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:337: /* Ignore the hint, as we do not need it. */ So we use the hint if it's aligned but ignore it otherwise? Why not just round the hint down to page size alignment when MAP_FIXED isn't passed? What does Linux do? https://chromiumcodereview.appspot.com/1264783002/diff/20001/tests/syscalls/m... File tests/syscalls/mem_test.cc (right): https://chromiumcodereview.appspot.com/1264783002/diff/20001/tests/syscalls/m... tests/syscalls/mem_test.cc:73: Nit: be consistent about whether there's 1 or 2 empty lines between top-level definitions
https://chromiumcodereview.appspot.com/1264783002/diff/20001/src/trusted/serv... File src/trusted/service_runtime/sys_memory.c (right): https://chromiumcodereview.appspot.com/1264783002/diff/20001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:332: if (NACL_ABI_MAP_FIXED & flags) { On 2015/07/30 15:20:14, Mark Seaborn wrote: > Nit: use "(NACL_ABI_MAP_FIXED & flags) != 0" > > NaCl style is not to implicitly convert to boolean. Done. https://chromiumcodereview.appspot.com/1264783002/diff/20001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:334: map_result = -NACL_ABI_EINVAL; On 2015/07/30 15:20:14, Mark Seaborn wrote: > Is there a test case covering this code path? There is now (added to mem_test.cc). https://chromiumcodereview.appspot.com/1264783002/diff/20001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:337: /* Ignore the hint, as we do not need it. */ On 2015/07/30 15:20:14, Mark Seaborn wrote: > So we use the hint if it's aligned but ignore it otherwise? > > Why not just round the hint down to page size alignment when MAP_FIXED isn't > passed? What does Linux do? Linux rounds the address to a minimum value, and then forces alignment -- I will match the alignment behavior here. https://chromiumcodereview.appspot.com/1264783002/diff/20001/tests/syscalls/m... File tests/syscalls/mem_test.cc (right): https://chromiumcodereview.appspot.com/1264783002/diff/20001/tests/syscalls/m... tests/syscalls/mem_test.cc:73: On 2015/07/30 15:20:14, Mark Seaborn wrote: > Nit: be consistent about whether there's 1 or 2 empty lines between top-level > definitions Done.
LGTM with a nit... https://chromiumcodereview.appspot.com/1264783002/diff/40001/src/trusted/serv... File src/trusted/service_runtime/sys_memory.c (right): https://chromiumcodereview.appspot.com/1264783002/diff/40001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:338: usraddr = NaClRoundAllocPage(usraddr); Do you want to be rounding up or down? This rounds up. NaClTruncAllocPage() rounds down. Rounding down seems more natural for an mmap address.
On 2015/07/30 16:46:32, Mark Seaborn wrote: > LGTM with a nit... > > https://chromiumcodereview.appspot.com/1264783002/diff/40001/src/trusted/serv... > File src/trusted/service_runtime/sys_memory.c (right): > > https://chromiumcodereview.appspot.com/1264783002/diff/40001/src/trusted/serv... > src/trusted/service_runtime/sys_memory.c:338: usraddr = > NaClRoundAllocPage(usraddr); > Do you want to be rounding up or down? This rounds up. NaClTruncAllocPage() > rounds down. Rounding down seems more natural for an mmap address. I used this as a reference: http://lxr.free-electrons.com/source/mm/mmap.c#L1920 Linux uses: PAGE_ALIGN --> _ALIGN --> _ALIGN_UP. I wanted to match this behavior, hence why I rounded up.
The CQ bit was checked by smklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264783002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264783002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: nacl-presubmit on tryserver.nacl (JOB_FAILED, http://build.chromium.org/p/tryserver.nacl/builders/nacl-presubmit/builds/267)
The CQ bit was checked by smklein@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mseaborn@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1264783002/#ps60001 (title: "Dodging copyright presubmit warning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264783002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264783002/60001
https://chromiumcodereview.appspot.com/1264783002/diff/40001/src/trusted/serv... File src/trusted/service_runtime/sys_memory.c (right): https://chromiumcodereview.appspot.com/1264783002/diff/40001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:338: usraddr = NaClRoundAllocPage(usraddr); On 2015/07/30 16:46:32, Mark Seaborn wrote: > Do you want to be rounding up or down? This rounds up. NaClTruncAllocPage() > rounds down. Rounding down seems more natural for an mmap address. OK, here's an interesting corner case if you round up: What happens if you pass 0xffffffff? On a 64-bit system, that gets rounded to 0x100000000 -- a value that we can't pass directly as the 1st arg (since the arg is 32-bit). How does 0x100000000 get handled? https://chromiumcodereview.appspot.com/1264783002/diff/60001/tests/syscalls/m... File tests/syscalls/mem_test.cc (right): https://chromiumcodereview.appspot.com/1264783002/diff/60001/tests/syscalls/m... tests/syscalls/mem_test.cc:2: * Copyright (c) 2015 The Native Client Authors. All rights reserved. FWIW, current policy is to leave the copyright years alone. See past chromium-dev posts.
The CQ bit was unchecked by smklein@chromium.org
https://chromiumcodereview.appspot.com/1264783002/diff/60001/src/trusted/serv... File src/trusted/service_runtime/sys_memory.c (right): https://chromiumcodereview.appspot.com/1264783002/diff/60001/src/trusted/serv... src/trusted/service_runtime/sys_memory.c:338: usraddr = NaClRoundAllocPage(usraddr); Regarding your comment of 0xffffffff as a usraddr -- I agree with you. Rounding up to 0x100000000, even if appropriate on a typical 64-bit Linux system, is not the desired behavior for NaCl. I'll change the alignment to "NaClTruncAllocPage". https://chromiumcodereview.appspot.com/1264783002/diff/60001/tests/syscalls/m... File tests/syscalls/mem_test.cc (right): https://chromiumcodereview.appspot.com/1264783002/diff/60001/tests/syscalls/m... tests/syscalls/mem_test.cc:2: * Copyright (c) 2015 The Native Client Authors. All rights reserved. On 2015/07/30 21:02:24, Mark Seaborn wrote: > FWIW, current policy is to leave the copyright years alone. See past > chromium-dev posts. Whoops. Changing it back to 2010.
The CQ bit was checked by smklein@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mseaborn@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1264783002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264783002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264783002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: nacl-arm_perf_panda on tryserver.nacl (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.nacl/builders/nacl-arm_perf_panda/build...)
The CQ bit was checked by smklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1264783002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1264783002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/native_client/src/native_client/+/5a8384a54... |
