From 32476b99344fb0a3b85acbdba666e165181ec638 Mon Sep 17 00:00:00 2001 From: Arthur Eubanks Date: Wed, 19 Mar 2025 20:27:57 +0000 Subject: [PATCH] Revert " [profile] Implement a non-mmap path when reading profile files from a non-local filesystem (#131177)" This reverts commit 14c95e0c8b25f6deba47cd279c5dcdeef3870159. Test fails on mac, e.g. https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-RA/3899/testReport/junit/Profile-x86_64/Profile-x86_64/instrprof_no_mmap_during_merging_c/ --- compiler-rt/lib/profile/InstrProfilingFile.c | 32 ++--- compiler-rt/lib/profile/InstrProfilingPort.h | 2 - compiler-rt/lib/profile/InstrProfilingUtil.c | 124 ------------------ compiler-rt/lib/profile/InstrProfilingUtil.h | 19 --- .../test/profile/Posix/instrprof-fork.c | 40 +++--- .../instrprof-no-mmap-during-merging.c | 26 ---- 6 files changed, 31 insertions(+), 212 deletions(-) delete mode 100644 compiler-rt/test/profile/instrprof-no-mmap-during-merging.c diff --git a/compiler-rt/lib/profile/InstrProfilingFile.c b/compiler-rt/lib/profile/InstrProfilingFile.c index 4667c0289250..ce5aa9a8fd32 100644 --- a/compiler-rt/lib/profile/InstrProfilingFile.c +++ b/compiler-rt/lib/profile/InstrProfilingFile.c @@ -419,17 +419,17 @@ static int getProfileFileSizeForMerging(FILE *ProfileFile, * \p ProfileBuffer. Returns -1 on failure. On success, the caller is * responsible for unmapping the mmap'd buffer in \p ProfileBuffer. */ static int mmapProfileForMerging(FILE *ProfileFile, uint64_t ProfileFileSize, - ManagedMemory *ProfileBuffer) { - lprofGetFileContentBuffer(ProfileFile, ProfileFileSize, ProfileBuffer); - - if (ProfileBuffer->Status == MS_INVALID) { - PROF_ERR("Unable to merge profile data: %s\n", "reading file failed"); + char **ProfileBuffer) { + *ProfileBuffer = mmap(NULL, ProfileFileSize, PROT_READ, MAP_SHARED | MAP_FILE, + fileno(ProfileFile), 0); + if (*ProfileBuffer == MAP_FAILED) { + PROF_ERR("Unable to merge profile data, mmap failed: %s\n", + strerror(errno)); return -1; } - if (__llvm_profile_check_compatibility(ProfileBuffer->Addr, - ProfileFileSize)) { - (void)lprofReleaseBuffer(ProfileBuffer, ProfileFileSize); + if (__llvm_profile_check_compatibility(*ProfileBuffer, ProfileFileSize)) { + (void)munmap(*ProfileBuffer, ProfileFileSize); PROF_WARN("Unable to merge profile data: %s\n", "source profile file is not compatible."); return -1; @@ -444,7 +444,7 @@ static int mmapProfileForMerging(FILE *ProfileFile, uint64_t ProfileFileSize, */ static int doProfileMerging(FILE *ProfileFile, int *MergeDone) { uint64_t ProfileFileSize; - ManagedMemory ProfileBuffer; + char *ProfileBuffer; /* Get the size of the profile on disk. */ if (getProfileFileSizeForMerging(ProfileFile, &ProfileFileSize) == -1) @@ -460,9 +460,9 @@ static int doProfileMerging(FILE *ProfileFile, int *MergeDone) { return -1; /* Now start merging */ - if (__llvm_profile_merge_from_buffer(ProfileBuffer.Addr, ProfileFileSize)) { + if (__llvm_profile_merge_from_buffer(ProfileBuffer, ProfileFileSize)) { PROF_ERR("%s\n", "Invalid profile data to merge"); - (void)lprofReleaseBuffer(&ProfileBuffer, ProfileFileSize); + (void)munmap(ProfileBuffer, ProfileFileSize); return -1; } @@ -471,7 +471,7 @@ static int doProfileMerging(FILE *ProfileFile, int *MergeDone) { (void)COMPILER_RT_FTRUNCATE(ProfileFile, __llvm_profile_get_size_for_buffer()); - (void)lprofReleaseBuffer(&ProfileBuffer, ProfileFileSize); + (void)munmap(ProfileBuffer, ProfileFileSize); *MergeDone = 1; return 0; @@ -672,13 +672,13 @@ static void initializeProfileForContinuousMode(void) { } else { /* The merged profile has a non-zero length. Check that it is compatible * with the data in this process. */ - ManagedMemory ProfileBuffer; + char *ProfileBuffer; if (mmapProfileForMerging(File, ProfileFileSize, &ProfileBuffer) == -1) { lprofUnlockFileHandle(File); fclose(File); return; } - (void)lprofReleaseBuffer(&ProfileBuffer, ProfileFileSize); + (void)munmap(ProfileBuffer, ProfileFileSize); } } else { File = fopen(Filename, FileOpenMode); @@ -1257,12 +1257,12 @@ COMPILER_RT_VISIBILITY int __llvm_profile_set_file_object(FILE *File, } else { /* The merged profile has a non-zero length. Check that it is compatible * with the data in this process. */ - ManagedMemory ProfileBuffer; + char *ProfileBuffer; if (mmapProfileForMerging(File, ProfileFileSize, &ProfileBuffer) == -1) { lprofUnlockFileHandle(File); return 1; } - (void)lprofReleaseBuffer(&ProfileBuffer, ProfileFileSize); + (void)munmap(ProfileBuffer, ProfileFileSize); } mmapForContinuousMode(0, File); lprofUnlockFileHandle(File); diff --git a/compiler-rt/lib/profile/InstrProfilingPort.h b/compiler-rt/lib/profile/InstrProfilingPort.h index a77fa3073167..66d697885eae 100644 --- a/compiler-rt/lib/profile/InstrProfilingPort.h +++ b/compiler-rt/lib/profile/InstrProfilingPort.h @@ -23,7 +23,6 @@ #define COMPILER_RT_FTRUNCATE(f,l) _chsize(_fileno(f),l) #define COMPILER_RT_ALWAYS_INLINE __forceinline #define COMPILER_RT_CLEANUP(x) -#define COMPILER_RT_UNUSED #define COMPILER_RT_USED #elif __GNUC__ #ifdef _WIN32 @@ -39,7 +38,6 @@ #define COMPILER_RT_ALLOCA __builtin_alloca #define COMPILER_RT_ALWAYS_INLINE inline __attribute((always_inline)) #define COMPILER_RT_CLEANUP(x) __attribute__((cleanup(x))) -#define COMPILER_RT_UNUSED __attribute__((unused)) #define COMPILER_RT_USED __attribute__((used)) #endif diff --git a/compiler-rt/lib/profile/InstrProfilingUtil.c b/compiler-rt/lib/profile/InstrProfilingUtil.c index 0fae91cfb895..c637b9d0b893 100644 --- a/compiler-rt/lib/profile/InstrProfilingUtil.c +++ b/compiler-rt/lib/profile/InstrProfilingUtil.c @@ -21,15 +21,6 @@ #include #endif -#ifdef _AIX -#include -// depends on `uint` to be a typedef from to -// `uint_t`; however, does not always declare `uint`. We provide -// the typedef prior to including to work around this issue. -typedef uint_t uint; -#include -#endif - #ifdef COMPILER_RT_HAS_UNAME #include #endif @@ -267,121 +258,6 @@ COMPILER_RT_VISIBILITY FILE *lprofOpenFileEx(const char *ProfileName) { return f; } -#if defined(_AIX) -// Return 1 (true) if the file descriptor Fd represents a file that is on a -// local filesystem, otherwise return 0. -static int isLocalFilesystem(int Fd) { - struct statfs Vfs; - if (fstatfs(Fd, &Vfs) != 0) { - PROF_ERR("%s: fstatfs(%d) failed: %s\n", __func__, Fd, strerror(errno)); - return 0; - } - - int Ret; - size_t BufSize = 2048u; - char *Buf; - int Tries = 3; - while (Tries--) { - Buf = malloc(BufSize); - // mntctl returns -1 if `Buf` is `NULL`. - Ret = mntctl(MCTL_QUERY, BufSize, Buf); - if (Ret != 0) - break; - BufSize = *(unsigned int *)Buf; - free(Buf); - } - - if (Ret != -1) { - // Look for the correct vmount entry. - char *CurObjPtr = Buf; - while (Ret--) { - struct vmount *Vp = (struct vmount *)CurObjPtr; - _Static_assert(sizeof(Vfs.f_fsid) == sizeof(Vp->vmt_fsid), - "fsid length mismatch"); - if (memcmp(&Vfs.f_fsid, &Vp->vmt_fsid, sizeof Vfs.f_fsid) == 0) { - int Answer = (Vp->vmt_flags & MNT_REMOTE) == 0; - free(Buf); - return Answer; - } - CurObjPtr += Vp->vmt_length; - } - } - - free(Buf); - // There was an error in mntctl or vmount entry not found; "remote" is the - // conservative answer. - return 0; -} -#endif - -static int isMmapSafe(int Fd) { - if (getenv("LLVM_PROFILE_NO_MMAP")) // For testing purposes. - return 0; -#ifdef _AIX - return isLocalFilesystem(Fd); -#else - return 1; -#endif -} - -COMPILER_RT_VISIBILITY void lprofGetFileContentBuffer(FILE *F, uint64_t Length, - ManagedMemory *Buf) { - Buf->Status = MS_INVALID; - if (isMmapSafe(fileno(F))) { - Buf->Addr = - mmap(NULL, Length, PROT_READ, MAP_SHARED | MAP_FILE, fileno(F), 0); - if (Buf->Addr == MAP_FAILED) - PROF_ERR("%s: mmap failed: %s\n", __func__, strerror(errno)) - else - Buf->Status = MS_MMAP; - return; - } - - if (getenv("LLVM_PROFILE_VERBOSE")) - PROF_NOTE("%s\n", "could not use mmap; using fread instead"); - - void *Buffer = malloc(Length); - if (!Buffer) { - PROF_ERR("%s: malloc failed: %s\n", __func__, strerror(errno)); - return; - } - if (ftell(F) != 0) { - PROF_ERR("%s: expecting ftell to return zero\n", __func__); - free(Buffer); - return; - } - - // Read the entire file into memory. - size_t BytesRead = fread(Buffer, 1, Length, F); - if (BytesRead != (size_t)Length) { - PROF_ERR("%s: fread failed%s\n", __func__, - feof(F) ? ": end of file reached" : ""); - free(Buffer); - return; - } - - // Reading was successful, record the result in the Buf parameter. - Buf->Addr = Buffer; - Buf->Status = MS_MALLOC; -} - -COMPILER_RT_VISIBILITY -void lprofReleaseBuffer(ManagedMemory *Buf, size_t Length) { - switch (Buf->Status) { - case MS_MALLOC: - free(Buf->Addr); - break; - case MS_MMAP: - (void)munmap(Buf->Addr, Length); - break; - default: - PROF_ERR("%s: Buffer has invalid state: %d\n", __func__, Buf->Status); - break; - } - Buf->Addr = NULL; - Buf->Status = MS_INVALID; -} - COMPILER_RT_VISIBILITY const char *lprofGetPathPrefix(int *PrefixStrip, size_t *PrefixLen) { const char *Prefix = getenv("GCOV_PREFIX"); diff --git a/compiler-rt/lib/profile/InstrProfilingUtil.h b/compiler-rt/lib/profile/InstrProfilingUtil.h index 234cf26cc0af..227c2aa0a7ca 100644 --- a/compiler-rt/lib/profile/InstrProfilingUtil.h +++ b/compiler-rt/lib/profile/InstrProfilingUtil.h @@ -31,25 +31,6 @@ int lprofUnlockFileHandle(FILE *F); * lock for exclusive access. The caller will block * if the lock is already held by another process. */ FILE *lprofOpenFileEx(const char *Filename); - -enum MemoryStatus { - MS_INVALID, // Addr is not a valid address - MS_MMAP, // Addr was mmap'ed - MS_MALLOC // Addr was malloc'ed -}; -typedef struct { - void *Addr; - enum MemoryStatus Status; -} ManagedMemory; - -/* Read the content of a file using mmap or fread into a buffer. - * Certain files (e.g. NFS mounted) cannot be opened reliably with mmap, - * so we use fread in those cases. The corresponding lprofReleaseBuffer - * will free/munmap the buffer. - */ -void lprofGetFileContentBuffer(FILE *F, uint64_t FileSize, ManagedMemory *Buf); -void lprofReleaseBuffer(ManagedMemory *FileBuffer, size_t Length); - /* PS4 doesn't have setenv/getenv/fork. Define a shim. */ #if __ORBIS__ #include diff --git a/compiler-rt/test/profile/Posix/instrprof-fork.c b/compiler-rt/test/profile/Posix/instrprof-fork.c index a1b99b492aa4..8df0abd73c4c 100644 --- a/compiler-rt/test/profile/Posix/instrprof-fork.c +++ b/compiler-rt/test/profile/Posix/instrprof-fork.c @@ -3,15 +3,11 @@ // RUN: %clang_pgogen=%t.profdir -o %t -O2 %s // RUN: %run %t // RUN: llvm-profdata show --all-functions --counts %t.profdir/default_*.profraw | FileCheck %s -// RUN: rm -fr %t.profdir -// RUN: env LLVM_PROFILE_NO_MMAP=1 %run %t -// RUN: llvm-profdata show --all-functions --counts %t.profdir/default_*.profraw | FileCheck %s - // // CHECK: func1: -// CHECK: Block counts: [21] +// CHECK: Block counts: [4] // CHECK: func2: -// CHECK: Block counts: [10] +// CHECK: Block counts: [1] #include #include @@ -20,23 +16,17 @@ __attribute__((noinline)) void func1() {} __attribute__((noinline)) void func2() {} int main(void) { - // child | parent - // func1 func2 | func1 func2 - func1(); // +10 | +1 (*) - int i = 10; // | - while (i-- > 0) { // | - pid_t pid = fork(); // | - if (pid == -1) // | - return 1; // | - if (pid == 0) { // | - func2(); // +10 | - func1(); // +10 | - return 0; // | - } // | - } // ------------+------------ - int status; // 20 10 | 1 0 - i = 10; // (*) the child inherits counter values prior to fork - while (i-- > 0) // from the parent in non-continuous mode. - wait(&status); - return 0; + // child | parent + int status; // func1 func2 | func1 func2 + func1(); // +1 | +1 (*) + pid_t pid = fork(); // | + if (pid == -1) // | + return 1; // | + if (pid == 0) // | + func2(); // +1 | + func1(); // +1 | +1 + if (pid) // ------------+------------ + wait(&status); // 2 1 | 2 0 + return 0; // (*) the child inherits counter values prior to fork + // from the parent in non-continuous mode. } diff --git a/compiler-rt/test/profile/instrprof-no-mmap-during-merging.c b/compiler-rt/test/profile/instrprof-no-mmap-during-merging.c deleted file mode 100644 index c3f0c786e9fa..000000000000 --- a/compiler-rt/test/profile/instrprof-no-mmap-during-merging.c +++ /dev/null @@ -1,26 +0,0 @@ -// RUN: mkdir -p %t.d && cd %t.d -// RUN: rm -f *.profraw -// RUN: %clang_pgogen %s -o a.out - -// Need to run a.out twice. On the second time, a merge will occur, which will -// trigger an mmap. -// RUN: ./a.out -// RUN: llvm-profdata show default_*.profraw --all-functions --counts --memop-sizes 2>&1 | FileCheck %s -check-prefix=PROFDATA -// RUN: env LLVM_PROFILE_NO_MMAP=1 LLVM_PROFILE_VERBOSE=1 ./a.out 2>&1 | FileCheck %s -// RUN: llvm-profdata show default_*.profraw --all-functions --counts --memop-sizes 2>&1 | FileCheck %s -check-prefix=PROFDATA2 - -// CHECK: could not use mmap; using fread instead -// PROFDATA: Block counts: [1] -// PROFDATA: [ 0, 0, 1 ] -// PROFDATA: Maximum function count: 1 -// PROFDATA2: Block counts: [2] -// PROFDATA2: [ 0, 0, 2 ] -// PROFDATA2: Maximum function count: 2 - -#include -int ar[8]; -int main() { - memcpy(ar, ar + 2, ar[0]); - memcpy(ar, ar + 2, ar[2]); - return ar[2]; -}