Skip to content

[compiler-rt][profile] Use flock shim on Windows even if detection fails #112695

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

Merged

Conversation

kateinoigakukun
Copy link
Member

This is a follow-up fix for d4efc3e, which introduced CMake-time feature test for flock. The feature test always fails on Windows, but we still need to use the flock shim provided by WindowsMMap.h regardless of the feature test result.

…fails

This is a follow-up fix for d4efc3e, which
introduced CMake-time feature test for `flock`. The feature test always
fails on Windows, but we still need to use the `flock` shim provided by
WindowsMMap.h regardless of the feature test result.
@kateinoigakukun
Copy link
Member Author

@glandium Would you mind testing this patch with your PGO setup again? 🙏

@mstorsjo
Copy link
Member

I think this change looks reasonable - the referenced commit d4efc3e is so large that it's hard to pick out all the various details that happen in there though. But it looks correct to me.

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we land this to unbreak coverage builds?

@glandium
Copy link
Contributor

@glandium Would you mind testing this patch with your PGO setup again? 🙏

I can confirm it works.

@kateinoigakukun kateinoigakukun marked this pull request as ready for review October 21, 2024 22:09
@kateinoigakukun kateinoigakukun merged commit 7a90ff7 into llvm:main Oct 21, 2024
9 checks passed
@llvmbot llvmbot added compiler-rt PGO Profile Guided Optimizations labels Oct 21, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 21, 2024

@llvm/pr-subscribers-pgo

Author: Yuta Saito (kateinoigakukun)

Changes

This is a follow-up fix for d4efc3e, which introduced CMake-time feature test for flock. The feature test always fails on Windows, but we still need to use the flock shim provided by WindowsMMap.h regardless of the feature test result.


Full diff: https://github.com/llvm/llvm-project/pull/112695.diff

1 Files Affected:

  • (modified) compiler-rt/lib/profile/InstrProfilingUtil.c (+4-2)
diff --git a/compiler-rt/lib/profile/InstrProfilingUtil.c b/compiler-rt/lib/profile/InstrProfilingUtil.c
index 95ec4080ba2504..c637b9d0b893cd 100644
--- a/compiler-rt/lib/profile/InstrProfilingUtil.c
+++ b/compiler-rt/lib/profile/InstrProfilingUtil.c
@@ -152,7 +152,8 @@ COMPILER_RT_VISIBILITY int lprofLockFd(int fd) {
     }
   }
   return 0;
-#elif defined(COMPILER_RT_HAS_FLOCK)
+#elif defined(COMPILER_RT_HAS_FLOCK) || defined(_WIN32)
+  // Windows doesn't have flock but WindowsMMap.h provides a shim
   flock(fd, LOCK_EX);
   return 0;
 #else
@@ -179,7 +180,8 @@ COMPILER_RT_VISIBILITY int lprofUnlockFd(int fd) {
     }
   }
   return 0;
-#elif defined(COMPILER_RT_HAS_FLOCK)
+#elif defined(COMPILER_RT_HAS_FLOCK) || defined(_WIN32)
+  // Windows doesn't have flock but WindowsMMap.h provides a shim
   flock(fd, LOCK_UN);
   return 0;
 #else

@kateinoigakukun
Copy link
Member Author

@glandium Thank you for checking 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler-rt PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants