Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMMON: Beginnings of 64-bit filesystem support #3133

Merged
merged 5 commits into from Jul 9, 2021

Conversation

@dreammaster
Copy link
Member

@dreammaster dreammaster commented Jul 4, 2021

This one has been pending for supporting the AGS game Strangeland, which has a 3Gb executable. It required pushing up the data types of the Stream class's seek, size, and pos methods to 64-bits offsets/sizes. This is necessary, for example, for allowing seek to still specify negative offsets for moving backwards with SEEK_CUR, yet allow for seeking to offsets greater than the 2Gb mark.

At the moment, I've only put in a special _MSC_VER check in the StdIOStream class for Visual Studio to use it's 64-bit versions of fseek/ftell.. I'm uncertain whether gcc for Windows, as well as the other backends, will need changes, or their versions will handle the int64 offsets transparently. I'll need assistance from the various port maintainers to review their backends accordingly.

For AGS at least, I've also added some code in the startup so that if the size() method returns -1 for the file, it will give an error message saying that the particular platform doesn't support the game due to it's size, rather than threw an arbitrary wobbly at some point during game loading/startup. Also, additionally, Strangeland does require some plugin work, so even with this change it won't start up yet, but I'll wait until this pull request is accepted into master before I start doing further work on it.

@ccawley2011
Copy link
Member

@ccawley2011 ccawley2011 commented Jul 4, 2021

It might be worth adding a typedef to the base Stream class, similar to the size_type typedef provided by the Common::Array class, and using that for pos, seek and size.

@sev-
Copy link
Member

@sev- sev- commented Jul 4, 2021

This is huge but long overdue.

On MacOS and in general in POSIX.1 standard, there are int fseeko(FILE *stream, off_t offset, int whence);, off_t ftello(FILE *stream) functions which work with 64-bit filesystems. Maybe @criezy has additional insight.

@criezy
Copy link
Member

@criezy criezy commented Jul 5, 2021

For Linux I had to look at this 17 years ago when we made the transition for our code at work. I checked quickly our subversion history to refresh my memory, and that involved:

  • Changing calls to fseek() and ftell() to use fseeko() and ftello() instead. The offset arguments for those functions is off_t instead of long int.
  • Compiling with -D_FILE_OFFSET_BITS=64 so that off_t is 64 bits (as it is 32 bits by default).
  • Changing the source code to use a 64 bits type instead of long int in places were we need to support 64 bits files.

On macOS we can also use fseeko() and ftello(), and the default has been 64 bits for quite a while (I checked that on 10.5 off_t is unconditionally defined to a 64 bit type). So defining _FILE_OFFSET_BITS is not needed.

On linux this may still be needed. The man page for fseeko and ftello on Debian 9 says:

On some architectures, both off_t and long are 32-bit types, but defining _FILE_OFFSET_BITS with the value 64 (before including any header files) will turn off_t into a 64-bit type.

At the time I looked at this we also needed to use -D_LARGEFILE_SOURCE on Linux so that we could use fseeko() and ftello(). But I think that define is no longer needed and they should now be available by default as they are now part of the POSIX standard.

@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jul 5, 2021

I personally don't favor using a typedef. Particularly since the original didn't use it either. As the other systems, it sounds like it'd be cleaner to merge this into master first, and then add in the appropriate conditional blocks for the different systems by those who can actually test the code on the given system before committing it. Do you all concur?

@lotharsm
Copy link
Member

@lotharsm lotharsm commented Jul 5, 2021

At the moment, I've only put in a special _MSC_VER check in the StdIOStream class for Visual Studio to use it's 64-bit versions of fseek/ftell.. I'm uncertain whether gcc for Windows, as well as the other backends, will need changes, or their versions will handle the int64 offsets transparently. I'll need assistance from the various port maintainers to review their backends accordingly.

I tried building your patch with current mingw-w64, since this is our current release toolchain. My test case is Strangeland here since it's a good way to test if the 64-bit values are used properly.

With MSVC, I get the script link error, indicating that the game files are loaded. However, the mingw-w64 build just tells me that the game data files are larger than 2GB and that my build of ScummVM won't support such large game files.

I was able to solve this by modifying the checks in stdiostream.cpp and setting them to

int64 StdioStream::pos() const {
#if defined(_MSC_VER) || defined(__MINGW32__) || defined(__MING64__)

instead.

@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jul 6, 2021

Actually, if MinGW has the i64 methods, then I can likely just change the #If check to #if defined(WIN32) like is done elsewhere. I just didn't do it initially because I presumed the i64 methods were yet another case of MS specific extensions.

@dreammaster dreammaster force-pushed the dreammaster:seek64 branch from bfc3c8e to 79ea9d7 Jul 6, 2021
@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jul 8, 2021

Unless there's anything else, I'll merge this into master tonight. Luckily, since this just affects method parameters and return values, at most, it may only cause some compilation issues for backends that I missed, and should be easily remedied.

@dreammaster dreammaster merged commit bb83fdf into scummvm:master Jul 9, 2021
1 check passed
1 check passed
@codacy-production
Codacy Static Code Analysis Codacy Static Code Analysis
Details
@digitall
Copy link
Member

@digitall digitall commented Jul 9, 2021

@dreammaster : This caused breakage of "make devtools" ... specifically create_titanic. See:

devtools/create_titanic/zlib.cpp: In member function ‘virtual bool Common::GZipReadStream::seek(int64, int)’:
devtools/create_titanic/zlib.cpp:310:60: error: no matching function for call to ‘MIN(int32, int64&)’
    offset -= read(tmpBuf, MIN((int32)sizeof(tmpBuf), offset));
                                                            ^
In file included from devtools/create_titanic/zlib.cpp:29:
./common/util.h:61:31: note: candidate: ‘template<class T> T MIN(T, T)’
 template<typename T> inline T MIN(T a, T b) { return (a < b) ? a : b; }
                               ^~~
./common/util.h:61:31: note:   template argument deduction/substitution failed:
devtools/create_titanic/zlib.cpp:310:60: note:   deduced conflicting types for parameter ‘T’ (‘int’ and ‘long int’)
    offset -= read(tmpBuf, MIN((int32)sizeof(tmpBuf), offset));
                                                            ^
make: *** [Makefile.common:161: devtools/create_titanic/zlib.o] Error 1

I can try to fix, but thought you would probably want to look at this as I am not sure if this has more impact than just needing a cast to (int32) or (int64) ...

@dreammaster
Copy link
Member Author

@dreammaster dreammaster commented Jul 10, 2021

@digitall if you could fix it, that would be helpful, thanks. Just a case to int32 again should be sufficient.

@digitall
Copy link
Member

@digitall digitall commented Jul 10, 2021

@dreammaster : cough Already did it :) 245bef2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants