This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix 64-bit file creation for Bionic and Windows
ClosedPublic

Authored by rprichard on Oct 31 2022, 4:01 PM.

Details

Summary

Bionic didn't add fopen64 until API 24, but there's no meaningful
distinction between them with Bionic, so just use fopen instead.

On Windows, use _chsize_s instead of _chsize. _chsize uses a 32-bit
long size argument, while _chsize_s uses __int64 instead.

Factor out utils::{off64_t, fopen64, ftruncate64} for use within the
create_file function.

Diff Detail

Event Timeline

rprichard created this revision.Oct 31 2022, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 4:01 PM
Herald added a subscriber: danielkiss. · View Herald Transcript
rprichard requested review of this revision.Oct 31 2022, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 4:01 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Nov 1 2022, 9:09 AM
ldionne added inline comments.
libcxx/test/support/filesystem_test_helper.h
193–198

I think it would be better to instead add utils::fopen64, utils::ftruncate64 and utils::off64_t. That way, we can simplify this code and hide the complexity inside utils:: above, which was the original goal.

This revision now requires changes to proceed.Nov 1 2022, 9:09 AM
rprichard updated this revision to Diff 472804.Nov 2 2022, 4:37 PM

Factor out utils::{off64_t, fopen64, ftruncate64}.

rprichard edited the summary of this revision. (Show Details)Nov 2 2022, 4:37 PM
rprichard updated this revision to Diff 473086.Nov 3 2022, 5:04 PM
rprichard edited the summary of this revision. (Show Details)

On Windows, use int64_t/_chsize_s instead of off_t/_chsize.

I'm not sure if MVS is actually 64-bit or not...

ldionne accepted this revision.Nov 22 2022, 8:09 AM

Thank you. This makes the code simpler while supporting what you need. 🚢 it!

This revision is now accepted and ready to land.Nov 22 2022, 8:09 AM
rprichard retitled this revision from [libc++][Android] Use fopen/ftruncate64/off64_t on 32-bit Bionic to [libc++] Fix 64-bit file creation for Bionic and Windows.Nov 29 2022, 4:53 PM
rprichard edited the summary of this revision. (Show Details)

Adding mstorsjo as subscriber in case they're interested in the Windows part of this change.

This looks ok to me (as long as the configurations in CI still are passing I don't see any issue with it). I don't really see the practical rationale for changing from _chsize to _chsize_s though - if the only difference is that the latter returns the actual error code instead of -1 on error, but we still do return ::_chsize_s(fd, length) ? -1 : 0;, there's not much point in doing that instead of just return ::_chsize(fd, length); right? Or is there some other aspect that isn't spelled out here?

This looks ok to me (as long as the configurations in CI still are passing I don't see any issue with it). I don't really see the practical rationale for changing from _chsize to _chsize_s though - if the only difference is that the latter returns the actual error code instead of -1 on error, but we still do return ::_chsize_s(fd, length) ? -1 : 0;, there's not much point in doing that instead of just return ::_chsize(fd, length); right? Or is there some other aspect that isn't spelled out here?

_chsize uses a long size parameter, which is 32 bits on Windows, while _chsize_s uses __int64. Maybe I should add that to the commit comment.

rprichard edited the summary of this revision. (Show Details)Dec 1 2022, 2:18 PM
mstorsjo accepted this revision.Dec 1 2022, 2:27 PM

This looks ok to me (as long as the configurations in CI still are passing I don't see any issue with it). I don't really see the practical rationale for changing from _chsize to _chsize_s though - if the only difference is that the latter returns the actual error code instead of -1 on error, but we still do return ::_chsize_s(fd, length) ? -1 : 0;, there's not much point in doing that instead of just return ::_chsize(fd, length); right? Or is there some other aspect that isn't spelled out here?

_chsize uses a long size parameter, which is 32 bits on Windows, while _chsize_s uses __int64. Maybe I should add that to the commit comment.

Oh, I see - thanks. That sounds reasonable yes.

Thanks for updating the commit message!

The patch is ok with me as long as CI keeps working (and the only failure there seems to be a fluke).

This revision was landed with ongoing or failed builds.Dec 1 2022, 2:48 PM
This revision was automatically updated to reflect the committed changes.