pcsx2 icon indicating copy to clipboard operation
pcsx2 copied to clipboard

[BUG]: linux: set _FILE_OFFSET_BITS=64 and use non-lfs64 calls

Open nekopsykose opened this issue 1 year ago • 1 comments

Describe the Bug

(i don't know which template to write generic build issues in)

currently in pcsx2/Linux/LnxFlatFileReader.cpp, there is:

uint FlatFileReader::GetBlockCount(void) const
{
#if defined(__HAIKU__) || defined(__APPLE__) || defined(__FreeBSD__)
	struct stat sysStatData;
	if (fstat(m_fd, &sysStatData) < 0)
		return 0;
#else
	struct stat64 sysStatData;
	if (fstat64(m_fd, &sysStatData) < 0)
		return 0;
#endif

	return (int)(sysStatData.st_size / m_blocksize);
}

on linux, this is using the 'lfs64 compat function defines', generally exposed by -D_LARGEFILE_SOURCE.

however, this in itself is mostly legacy. the modern way to do this is to have only:

struct stat sysStatData;
if (fstat( ...

and pass -D_FILE_OFFSET_BITS=64. this makes the default functions 64-bit types and utilises a 64-bit off_t, etc, on both 32-bit and 64-bit architectures. small meaningless reference.

utilising the -D_FILE_OFFSET_BITS=64 would fix compiling against musl master, which no longer exposes the *64 apis by default with -D_GNU_SOURCE, intentionally: https://github.com/bminor/musl/commit/25e6fee27f4a293728dd15b659170e7b9c7db9bc

(aside: musl does not have 32-bit interfaces for this to begin with, off_t is always 64 regardless, these are just compat shims, but this doesn't matter here)

it would also be portable to all other linux libcs- it's required to respect -D_FILE_OFFSET_BITS=64 to expose the 64-bit-accepting interfaces for these non-64-prefixed functions.

tl;dr:

  • change stat64/fstat64 to just stat/fstat
  • define -D_FILE_OFFSET_BITS=64 when building on linux

i would submit an mr but don't exactly know where you would want to add this in cmake.

Reproduction Steps

build pcsx2 against musl-libc master without a deprecated -D_LARGEFILE64_SOURCE macro, and observe:

/home/demon/src/aports/testing/pcsx2/src/pcsx2-1.7.4231/pcsx2/Linux/LnxFlatFileReader.cpp:108:16: error: variable has incomplete type 'struct stat64'
        struct stat64 sysStatData;
                      ^

(type doesn't exist)

Expected Behavior

No response

PCSX2 Revision

v1.7.4231

Operating System

Linux (64bit) - Specify distro below

If Linux - Specify Distro

alpine

nekopsykose avatar Mar 14 '23 17:03 nekopsykose

ah, also goes for common/Linux/LnxHostSys.cpp for ftruncate64/off64_t, and common/FileSystem.cpp

a patch for the code specifically that works is:

diff --git a/common/FileSystem.cpp b/common/FileSystem.cpp
index a700e68..c8327b9 100644
--- a/common/FileSystem.cpp
+++ b/common/FileSystem.cpp
@@ -1488,17 +1488,10 @@ static u32 RecursiveFindFiles(const char* OriginPath, const char* ParentPath, co
 		FILESYSTEM_FIND_DATA outData;
 		outData.Attributes = 0;
 
-#if defined(__HAIKU__) || defined(__APPLE__) || defined(__FreeBSD__)
 		struct stat sDir;
 		if (stat(full_path.c_str(), &sDir) < 0)
 			continue;
 
-#else
-		struct stat64 sDir;
-		if (stat64(full_path.c_str(), &sDir) < 0)
-			continue;
-#endif
-
 		if (S_ISDIR(sDir.st_mode))
 		{
 			if (Flags & FILESYSTEM_FIND_RECURSIVE)
@@ -1601,13 +1594,8 @@ bool FileSystem::StatFile(const char* path, FILESYSTEM_STAT_DATA* sd)
 		return false;
 
 		// stat file
-#if defined(__HAIKU__) || defined(__APPLE__) || defined(__FreeBSD__)
 	struct stat sysStatData;
 	if (stat(path, &sysStatData) < 0)
-#else
-	struct stat64 sysStatData;
-	if (stat64(path, &sysStatData) < 0)
-#endif
 		return false;
 
 	// parse attributes
@@ -1634,13 +1622,8 @@ bool FileSystem::StatFile(std::FILE* fp, FILESYSTEM_STAT_DATA* sd)
 		return false;
 
 		// stat file
-#if defined(__HAIKU__) || defined(__APPLE__) || defined(__FreeBSD__)
 	struct stat sysStatData;
 	if (fstat(fd, &sysStatData) < 0)
-#else
-	struct stat64 sysStatData;
-	if (fstat64(fd, &sysStatData) < 0)
-#endif
 		return false;
 
 	// parse attributes
@@ -1667,13 +1650,8 @@ bool FileSystem::FileExists(const char* path)
 		return false;
 
 		// stat file
-#if defined(__HAIKU__) || defined(__APPLE__) || defined(__FreeBSD__)
 	struct stat sysStatData;
 	if (stat(path, &sysStatData) < 0)
-#else
-	struct stat64 sysStatData;
-	if (stat64(path, &sysStatData) < 0)
-#endif
 		return false;
 
 	if (S_ISDIR(sysStatData.st_mode))
@@ -1689,13 +1667,8 @@ bool FileSystem::DirectoryExists(const char* path)
 		return false;
 
 		// stat file
-#if defined(__HAIKU__) || defined(__APPLE__) || defined(__FreeBSD__)
 	struct stat sysStatData;
 	if (stat(path, &sysStatData) < 0)
-#else
-	struct stat64 sysStatData;
-	if (stat64(path, &sysStatData) < 0)
-#endif
 		return false;
 
 	if (S_ISDIR(sysStatData.st_mode))
diff --git a/common/Linux/LnxHostSys.cpp b/common/Linux/LnxHostSys.cpp
index 5b63cda..2ed3d81 100644
--- a/common/Linux/LnxHostSys.cpp
+++ b/common/Linux/LnxHostSys.cpp
@@ -243,12 +243,7 @@ void* HostSys::CreateSharedMemory(const char* name, size_t size)
 	// we're not going to be opening this mapping in other processes, so remove the file
 	shm_unlink(name);
 
-	// ensure it's the correct size
-#if !defined(__APPLE__) && !defined(__FreeBSD__)
-	if (ftruncate64(fd, static_cast<off64_t>(size)) < 0)
-#else
 	if (ftruncate(fd, static_cast<off_t>(size)) < 0)
-#endif
 	{
 		std::fprintf(stderr, "ftruncate64(%zu) failed: %d\n", size, errno);
 		return nullptr;
diff --git a/pcsx2/Linux/LnxFlatFileReader.cpp b/pcsx2/Linux/LnxFlatFileReader.cpp
index 2ecc9f2..dd1f045 100644
--- a/pcsx2/Linux/LnxFlatFileReader.cpp
+++ b/pcsx2/Linux/LnxFlatFileReader.cpp
@@ -100,15 +100,9 @@ void FlatFileReader::Close(void)
 
 uint FlatFileReader::GetBlockCount(void) const
 {
-#if defined(__HAIKU__) || defined(__APPLE__) || defined(__FreeBSD__)
 	struct stat sysStatData;
 	if (fstat(m_fd, &sysStatData) < 0)
 		return 0;
-#else
-	struct stat64 sysStatData;
-	if (fstat64(m_fd, &sysStatData) < 0)
-		return 0;
-#endif
 
 	return (int)(sysStatData.st_size / m_blocksize);
 }

i don't where you would want to add the define though.

nekopsykose avatar Mar 14 '23 18:03 nekopsykose

Are you sure _FILE_OFFSET_BITS needs to be defined? As far as I can tell, off_t is 64-bit already (we no longer have 32-bit builds).

stenzek avatar Sep 04 '23 12:09 stenzek

hmm, on 64-bit only builds it might not need to be defined, yeah. off_t and the like default to 64.

nekopsykose avatar Sep 04 '23 12:09 nekopsykose

Windows defines _off_t as 32bit, even in 64bit

typedef long _off_t; // file offset value sizeof results in 4

refractionpcsx2 avatar Sep 04 '23 12:09 refractionpcsx2

Gotta love 32-bit vs 64-bit long.

I know I mentioned it on Discord, but documenting here for completeness's sake: Windows explicitly uses the 64-bit variants, so this is Linux specific.

stenzek avatar Sep 04 '23 12:09 stenzek

thanks!

nekopsykose avatar Sep 05 '23 03:09 nekopsykose

No problem. Sorry for not seeing this earlier.

Obviously we can't provide official support for such environments, but we're not using anything specifically from glibc, so in theory it should work fine. Feel free to let us know if there's any other build issues.

stenzek avatar Sep 05 '23 06:09 stenzek