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

[Profile][Windows] Fix flakyness when checking existence of binary id #84196

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

ZequanWu
Copy link
Contributor

@ZequanWu ZequanWu commented Mar 6, 2024

There is a small chance that binary id starting with 0 (1/256). It is not sufficient to just check the first byte.

@ZequanWu ZequanWu requested a review from zmodem March 6, 2024 16:55
@llvmbot llvmbot added compiler-rt PGO Profile Guided Optimizations labels Mar 6, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 6, 2024

@llvm/pr-subscribers-pgo

Author: Zequan Wu (ZequanWu)

Changes

There is a small chance that binary id starting with 0 (1/256). It is not sufficient to just check the first byte.


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

1 Files Affected:

  • (modified) compiler-rt/lib/profile/InstrProfilingPlatformWindows.c (+3-2)
diff --git a/compiler-rt/lib/profile/InstrProfilingPlatformWindows.c b/compiler-rt/lib/profile/InstrProfilingPlatformWindows.c
index b9642ca7f6810f..f2bb02efa50078 100644
--- a/compiler-rt/lib/profile/InstrProfilingPlatformWindows.c
+++ b/compiler-rt/lib/profile/InstrProfilingPlatformWindows.c
@@ -96,9 +96,10 @@ ValueProfNode *EndVNode = &VNodesEnd;
 /* lld-link provides __buildid symbol which ponits to the 16 bytes build id when
  * using /build-id flag. https://lld.llvm.org/windows_support.html#lld-flags */
 #define BUILD_ID_LEN 16
-COMPILER_RT_WEAK uint8_t __buildid[BUILD_ID_LEN];
+COMPILER_RT_WEAK uint8_t __buildid[BUILD_ID_LEN] = {0};
 COMPILER_RT_VISIBILITY int __llvm_write_binary_ids(ProfDataWriter *Writer) {
-  if (*__buildid) {
+  uint8_t zeros[BUILD_ID_LEN] = {0};
+  if (memcmp(__buildid, zeros, BUILD_ID_LEN) != 0) {
     if (Writer &&
         lprofWriteOneBinaryId(Writer, BUILD_ID_LEN, __buildid, 0) == -1)
       return -1;

Copy link
Collaborator

@zmodem zmodem left a comment

Choose a reason for hiding this comment

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

lgtm with nit

Can a build-id be all-zeros? If yes, maybe as a followup we should ensure that it can't.

compiler-rt/lib/profile/InstrProfilingPlatformWindows.c Outdated Show resolved Hide resolved
@ZequanWu
Copy link
Contributor Author

ZequanWu commented Mar 6, 2024

lgtm with nit

Can a build-id be all-zeros? If yes, maybe as a followup we should ensure that it can't.

build-id provided by lld-link is generated by the hash of the object file or pdb: https://github.com/llvm/llvm-project/blob/main/lld/COFF/Writer.cpp#L2171, which is almost impossible to be all 0s.

@ZequanWu ZequanWu merged commit c371ee9 into llvm:main Mar 6, 2024
3 of 4 checks passed
@zmodem
Copy link
Collaborator

zmodem commented Mar 6, 2024

which is almost impossible to be all 0s.

It may be unlikely, but it sounds like it's definitely possible.
If all-zero build-ids are not supported, maybe that code should do something like if (hash == 0) ++hash;

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.

None yet

3 participants