Skip to content

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Jun 16, 2021

When mapping the file, we need to pass the proper dwFileOffsetHigh
instead of 0.


I'm not sure about adding a regression test. That would be terribly slow, but given that I missed this issue when fixing bug 79423 it might make sense to have one.

When mapping the file, we need to pass the proper `dwFileOffsetHigh`
instead of `0`.
@cmb69 cmb69 added the Bug label Jun 16, 2021
@nikic
Copy link
Member

nikic commented Jun 16, 2021

I'm not sure about adding a regression test. That would be terribly slow, but given that I missed this issue when fixing bug 79423 it might make sense to have one.

How slow are we talking here? On Linux the test case takes 2s, is it much slower on Windows?

@cmb69
Copy link
Member Author

cmb69 commented Jun 16, 2021

How slow are we talking here? On Linux the test case takes 2s, is it much slower on Windows?

Takes ~50sec for me. Lets see on CI. (see also https://bugs.php.net/bug.php?id=80695)

@cmb69
Copy link
Member Author

cmb69 commented Jun 16, 2021

Well, seems perf depends: Linux (16s) vs Windows (9s). Anyway, the test makes sense.

@divinity76
Copy link
Contributor

divinity76 commented Jun 16, 2021

FWIW we didn't add a test for https://bugs.php.net/bug.php?id=80523 either
(that bug required ~80GB RAM and ~4GB disk space to test!)

Co-authored-by: Nikita Popov <nikita.ppv@gmail.com>
@nikic
Copy link
Member

nikic commented Jun 17, 2021

Test fails on 32-bit Linux:

Warning: fopen(/home/vsts/work/1/s/ext/standard/tests/file/bug81145_src.bin): failed to open stream: Value too large for defined data type

We can't copy files > 4GB there, anyway.
@cmb69
Copy link
Member Author

cmb69 commented Jun 17, 2021

Ah, thanks! Need to skip there.

@cmb69
Copy link
Member Author

cmb69 commented Jun 17, 2021

@divinity76, PHP currently only supports some 32bit and 64bit architectures. To support other architectures, would require many more changes, so adapting this test as well shouldn't be hard (there are already ~200 other tests with basically the same SKIPIF clause).

@cmb69 cmb69 closed this in 2555efa Jun 17, 2021
@cmb69 cmb69 deleted the cmb/79423 branch June 17, 2021 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants