Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2430473002:
Implement File::Map on Windows. (Closed)
|
Created:
4 years, 2 months ago by rmacnak Modified:
4 years, 2 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImplement File::Map on Windows.
R=zra@google.com
Committed: https://github.com/dart-lang/sdk/commit/c9c33d9db3c84646232ff23e51a4069dd5e92044
Patch Set 1 #
Total comments: 4
Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 10
Patch Set 6 : . #Messages
Total messages: 17 (3 generated)
rmacnak@google.com changed reviewers: + johnmccutchan@google.com
./tools/build.py runtime ./tools/test.py -cdart2appjit -rdart_app --use-blobs language/first
some comments https://codereview.chromium.org/2430473002/diff/1/runtime/bin/file_win.cc File runtime/bin/file_win.cc (right): https://codereview.chromium.org/2430473002/diff/1/runtime/bin/file_win.cc#new... runtime/bin/file_win.cc:90: HANDLE mapping = CreateFileMapping(handle_->fd(), NULL, flags, 0, 0, NULL); error checking on mapping? https://codereview.chromium.org/2430473002/diff/1/runtime/bin/file_win.cc#new... runtime/bin/file_win.cc:91: return MapViewOfFile(mapping, check the result of MapViewOfFile and report: OS::PrintErr("MapViewOfFile failed: error=%d", GetLastError());
https://codereview.chromium.org/2430473002/diff/1/runtime/bin/file_win.cc File runtime/bin/file_win.cc (right): https://codereview.chromium.org/2430473002/diff/1/runtime/bin/file_win.cc#new... runtime/bin/file_win.cc:90: HANDLE mapping = CreateFileMapping(handle_->fd(), NULL, flags, 0, 0, NULL); On 2016/10/17 19:28:57, Cutch wrote: > error checking on mapping? Done. https://codereview.chromium.org/2430473002/diff/1/runtime/bin/file_win.cc#new... runtime/bin/file_win.cc:91: return MapViewOfFile(mapping, On 2016/10/17 19:28:57, Cutch wrote: > check the result of MapViewOfFile and report: > > OS::PrintErr("MapViewOfFile failed: error=%d", GetLastError()); Done.
rmacnak@google.com changed reviewers: + zra@google.com
On 2016/10/18 17:45:04, rmacnak wrote:
Fixes for compile errors:
void* File::Map(File::MapType type, int64_t position, int64_t length) {
ASSERT(handle_->fd() >= 0);
int flags;
switch (type) {
case File::kReadOnly:
flags = PAGE_READONLY;
break;
case File::kReadExecute:
flags = PAGE_EXECUTE_READ;
break;
default:
return NULL;
}
HANDLE mapping = CreateFileMapping(
reinterpret_cast<HANDLE>(handle_->fd()), NULL, flags, 0, 0, NULL);
if (mapping == NULL) {
return NULL;
}
void* addr = MapViewOfFile(mapping,
FILE_MAP_READ,
Utils::High32Bits(position),
Utils::Low32Bits(position),
length);
if (addr == NULL) {
Log::PrintErr("MapViewOfFile failed %d", GetLastError());
}
return addr;
}
On 2016/10/18 18:03:11, zra wrote:
> On 2016/10/18 17:45:04, rmacnak wrote:
>
> Fixes for compile errors:
>
> void* File::Map(File::MapType type, int64_t position, int64_t length) {
> ASSERT(handle_->fd() >= 0);
> int flags;
> switch (type) {
> case File::kReadOnly:
> flags = PAGE_READONLY;
> break;
> case File::kReadExecute:
> flags = PAGE_EXECUTE_READ;
> break;
> default:
> return NULL;
> }
> HANDLE mapping = CreateFileMapping(
> reinterpret_cast<HANDLE>(handle_->fd()), NULL, flags, 0, 0, NULL);
> if (mapping == NULL) {
> return NULL;
> }
> void* addr = MapViewOfFile(mapping,
> FILE_MAP_READ,
> Utils::High32Bits(position),
> Utils::Low32Bits(position),
> length);
> if (addr == NULL) {
> Log::PrintErr("MapViewOfFile failed %d", GetLastError());
> }
> return addr;
> }
This is failing because `position` is not aligned correctly. In particular
`position` is 4096, but the allocation granularity is 65536. See:
http://stackoverflow.com/questions/9889557/mapping-large-files-using-mapviewo...
On 2016/10/18 18:23:27, zra wrote:
> On 2016/10/18 18:03:11, zra wrote:
> > On 2016/10/18 17:45:04, rmacnak wrote:
> >
> > Fixes for compile errors:
> >
> > void* File::Map(File::MapType type, int64_t position, int64_t length) {
> > ASSERT(handle_->fd() >= 0);
> > int flags;
> > switch (type) {
> > case File::kReadOnly:
> > flags = PAGE_READONLY;
> > break;
> > case File::kReadExecute:
> > flags = PAGE_EXECUTE_READ;
> > break;
> > default:
> > return NULL;
> > }
> > HANDLE mapping = CreateFileMapping(
> > reinterpret_cast<HANDLE>(handle_->fd()), NULL, flags, 0, 0, NULL);
> > if (mapping == NULL) {
> > return NULL;
> > }
> > void* addr = MapViewOfFile(mapping,
> > FILE_MAP_READ,
> > Utils::High32Bits(position),
> > Utils::Low32Bits(position),
> > length);
> > if (addr == NULL) {
> > Log::PrintErr("MapViewOfFile failed %d", GetLastError());
> > }
> > return addr;
> > }
>
> This is failing because `position` is not aligned correctly. In particular
> `position` is 4096, but the allocation granularity is 65536. See:
>
>
http://stackoverflow.com/questions/9889557/mapping-large-files-using-mapviewo...
Done, plus fixed bad use of ErrorExit.
Fixed dwDesiredAccess for an executable mapping.
Falling back to VirtualAlloc + copying from the file.
Builds and tests pass https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... File runtime/bin/file_win.cc (right): https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:95: Log::PrintErr("VirtualAlloc failed %d", GetLastError()); \n https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:101: Log::PrintErr("ReadFully failed %d", GetLastError()); \n https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:108: Log::PrintErr("VirtualProtect failed %d", GetLastError()); \n
https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... File runtime/bin/file_win.cc (right): https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:100: if (!ReadFully(addr, length)) { VirtualFree(addr, 0, MEM_RELEASE) https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:107: if (!result) { VirtualFree(addr, 0, MEM_RELEASE)
Thanks! https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... File runtime/bin/file_win.cc (right): https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:95: Log::PrintErr("VirtualAlloc failed %d", GetLastError()); On 2016/10/19 22:39:45, zra wrote: > \n Done. https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:100: if (!ReadFully(addr, length)) { On 2016/10/19 22:43:06, zra wrote: > VirtualFree(addr, 0, MEM_RELEASE) Done. https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:101: Log::PrintErr("ReadFully failed %d", GetLastError()); On 2016/10/19 22:39:46, zra wrote: > \n Done. https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:107: if (!result) { On 2016/10/19 22:43:05, zra wrote: > VirtualFree(addr, 0, MEM_RELEASE) Done. https://chromiumcodereview.appspot.com/2430473002/diff/80001/runtime/bin/file... runtime/bin/file_win.cc:108: Log::PrintErr("VirtualProtect failed %d", GetLastError()); On 2016/10/19 22:39:45, zra wrote: > \n Done.
lgtm
Description was changed from ========== Implement File::Map on Windows. ========== to ========== Implement File::Map on Windows. R=zra@google.com Committed: https://github.com/dart-lang/sdk/commit/c9c33d9db3c84646232ff23e51a4069dd5e92044 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as c9c33d9db3c84646232ff23e51a4069dd5e92044 (presubmit successful). |
