Use MapVector to fix lld thinLTO "nondeterminism" issue. #117551

llvmssh · 2024-11-25T13:12:27Z

When the ModuleSymbolTable is generated, the binary consistency problem occurs due to the
data structure for collecting asm symbols was ordered by memory pointers.
Use MapVector to fix lld thinLTO "nondeterminism" issue.

github-actions · 2024-11-25T13:12:47Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-11-25T13:13:20Z

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (llvmssh)

Changes

When the ModuleSymbolTable is generated, the binary consistency problem occurs due to the
unorder of the data structure when collecting asm symbols.
Use MapVector to fix lld thinLTO bep issue.

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

2 Files Affected:

(modified) llvm/lib/Object/ModuleSymbolTable.cpp (+1-1)
(modified) llvm/lib/Object/RecordStreamer.h (+4-3)

diff --git a/llvm/lib/Object/ModuleSymbolTable.cpp b/llvm/lib/Object/ModuleSymbolTable.cpp
index 54e654a0d121cb..dc8ba00d3c91c7 100644
--- a/llvm/lib/Object/ModuleSymbolTable.cpp
+++ b/llvm/lib/Object/ModuleSymbolTable.cpp
@@ -144,7 +144,7 @@ void ModuleSymbolTable::CollectAsmSymbols(
     Streamer.flushSymverDirectives();
 
     for (auto &KV : Streamer) {
-      StringRef Key = KV.first();
+      StringRef Key = KV.first;
       RecordStreamer::State Value = KV.second;
       // FIXME: For now we just assume that all asm symbols are executable.
       uint32_t Res = BasicSymbolRef::SF_Executable;
diff --git a/llvm/lib/Object/RecordStreamer.h b/llvm/lib/Object/RecordStreamer.h
index 70b41f270720b4..946578ef06d11e 100644
--- a/llvm/lib/Object/RecordStreamer.h
+++ b/llvm/lib/Object/RecordStreamer.h
@@ -14,6 +14,7 @@
 #include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/Support/SMLoc.h"
+#include "llvm/ADT/MapVector.h"
 #include <vector>
 
 namespace llvm {
@@ -28,11 +29,11 @@ class RecordStreamer : public MCStreamer {
 
 private:
   const Module &M;
-  StringMap<State> Symbols;
+  MapVector<StringRef, State> Symbols;
   // Map of aliases created by .symver directives, saved so we can update
   // their symbol binding after parsing complete. This maps from each
   // aliasee to its list of aliases.
-  DenseMap<const MCSymbol *, std::vector<StringRef>> SymverAliasMap;
+  MapVector<const MCSymbol *, std::vector<StringRef>> SymverAliasMap;
 
   /// Get the state recorded for the given symbol.
   State getSymbolState(const MCSymbol *Sym);
@@ -70,7 +71,7 @@ class RecordStreamer : public MCStreamer {
   void flushSymverDirectives();
 
   // Symbols iterators
-  using const_iterator = StringMap<State>::const_iterator;
+  using const_iterator = MapVector<StringRef, State>::const_iterator;
   const_iterator begin();
   const_iterator end();

jh7370 · 2024-11-25T13:51:21Z

I don't know who filed this PR, but the author is listed as "None". I think whoever did this has messed up their commit email/author details?

github-actions · 2024-11-25T13:54:54Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmssh · 2024-11-26T01:34:45Z

I don't know who filed this PR, but the author is listed as "None". I think whoever did this has messed up their commit email/author details?

Thank you for your comment. I should have modified the information.

jh7370 · 2024-11-26T08:29:24Z

Thanks for the update. Is there a GitHub issue associated with the problem you're trying to fix? If so, please include "Fixes #####" in your PR description.

Just so you know, I'm not familiar with the area, so somebody who is will need to perform the review. Take a look through past contributors to this file and try pinging them on here/adding them as reviewers.

llvmssh · 2024-11-26T08:55:10Z

Thanks for the update. Is there a GitHub issue associated with the problem you're trying to fix? If so, please include "Fixes #####" in your PR description.

Just so you know, I'm not familiar with the area, so somebody who is will need to perform the review. Take a look through past contributors to this file and try pinging them on here/adding them as reviewers.

Thanks, I have a problem, it seems that I can't add reviewers.

jh7370 · 2024-11-26T10:24:43Z

Thanks for the update. Is there a GitHub issue associated with the problem you're trying to fix? If so, please include "Fixes #####" in your PR description.
Just so you know, I'm not familiar with the area, so somebody who is will need to perform the review. Take a look through past contributors to this file and try pinging them on here/adding them as reviewers.

Thanks, I have a problem, it seems that I can't add reviewers.

From the automated greeting post above:

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

llvmssh · 2024-11-27T10:49:10Z

@vitalybuka Hi，I see you have modified this file before, I have made a little modification, can you help review it?

llvmssh · 2024-11-30T02:10:42Z

@pcc，@vitalybuka Hi, can you help review my code? thank you.
Fix the issue:
During the generation of ModuleSymbolTable, the unordered DenseMap data structure is traversed during the collection of Asm symbols. As a result, the sequence of symbols in the symbol table is inconsistent after the ThinLTO lld link. As a result, binary random inconsistency occurs. Made a simple data structure modification.

llvmssh · 2024-12-02T01:54:43Z

Thanks for the update. Is there a GitHub issue associated with the problem you're trying to fix? If so, please include "Fixes #####" in your PR description.
Just so you know, I'm not familiar with the area, so somebody who is will need to perform the review. Take a look through past contributors to this file and try pinging them on here/adding them as reviewers.

Thanks, I have a problem, it seems that I can't add reviewers.

From the automated greeting post above:

If you wish to, you can add reviewers by using the "Reviewers" section on this page.
If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

Can you add a few reviewers for me?

vitalybuka · 2024-12-02T02:00:29Z

What is "bep issue"?

llvmssh · 2024-12-02T02:10:41Z

What is "bep issue"?

Thanks for your reply. When a compiler of the same version builds the same project twice, the generated binary files are different, which is reflected in the inconsistent sequence in the symbol table. For example:
295: 000000000000 0 FUNC GLOBAL DEFAULT UND __lxstat@GLIBC_2.X
295: 000000000000 0 FUNC GLOBAL DEFAULT UND __libc_start_main@GLIBC_2.X

vitalybuka · 2024-12-02T02:17:37Z

Thanks for your reply. When a compiler of the same version builds the same project twice, the generated binary files are different, which is reflected in the inconsistent sequence in the symbol table. For example: 295: 000000000000 0 FUNC GLOBAL DEFAULT UND __lxstat@GLIBC_2.X 295: 000000000000 0 FUNC GLOBAL DEFAULT UND __libc_start_main@GLIBC_2.X

Sorry, out of curiosity are there definition of "bep"?

Regardless of definition, can we please just call it "nondeterminism" in title and description? I guess that's how we usually call it in LLVM.

llvm/lib/Object/RecordStreamer.h

vitalybuka · 2024-12-02T02:43:09Z

@teresajohnson who added the loop in https://reviews.llvm.org/D30485

When the ModuleSymbolTable is generated, the binary consistency problem occurs due to the unorder of the data structure when collecting asm symbols.

teresajohnson

lgtm

vitalybuka · 2024-12-02T19:38:10Z

I will land when buildkite is done.

github-actions · 2024-12-02T22:16:38Z

@llvmssh Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

vitalybuka · 2024-12-02T22:18:12Z

Thanks for improving consistency!

llvmssh · 2024-12-03T06:49:18Z

@llvmssh Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

The Buildbot has detected a new failure on builder sanitizer-x86_64-linux-android while building llvm.

Full details are available at:
https://lab.llvm.org/buildbot/#/builders/186/builds/4513

Worker for this Build: sanitizer-buildbot-android

BUILD FAILED: failed 'python ../sanitizer_buildbot/sanitizers/zorg/buildbot/builders/sanitizers/buildbot_selector.py' (failure)

Step 2 (annotate) failure: 'python ../sanitizer_buildbot/sanitizers/zorg/buildbot/builders/sanitizers/buildbot_selector.py' (failure) ...
PASS: HWAddressSanitizer-aarch64 :: TestCases/hwasan_symbolize.cpp (527 of 1721)
PASS: HWAddressSanitizer-aarch64 :: TestCases/Posix/ignore_free_hook.cpp (528 of 1721)
PASS: HWAddressSanitizer-aarch64 :: TestCases/hwasan_symbolize_stack_uas.cpp (529 of 1721)
PASS: HWAddressSanitizer-aarch64 :: TestCases/libc_thread_freeres.c (530 of 1721)
PASS: HWAddressSanitizer-aarch64 :: TestCases/heap-buffer-overflow.c (531 of 1721)
PASS: HWAddressSanitizer-aarch64 :: TestCases/longjmp-out-of-range.c (532 of 1721)
PASS: HWAddressSanitizer-aarch64 :: TestCases/deep-recursion.c (533 of 1721)
PASS: HWAddressSanitizer-aarch64 :: TestCases/malloc-test.c (534 of 1721)
PASS: HWAddressSanitizer-aarch64 :: TestCases/malloc-align.c (535 of 1721)
PASS: AddressSanitizer-aarch64-android :: TestCases/use-after-scope-types.cpp (536 of 1721)
FAIL: HWAddressSanitizer-aarch64 :: TestCases/hwasan_symbolize_stack_overflow.cpp (537 of 1721)
******************** TEST 'HWAddressSanitizer-aarch64 :: TestCases/hwasan_symbolize_stack_overflow.cpp' FAILED ******************** Exit Code: 1
I have a problem, I can't see anything in the link above, can't check if my modification caused the above error. @llvmbot @jh7370

vitalybuka · 2024-12-03T06:55:39Z

Probably a flake.

llvmssh · 2024-12-03T07:01:34Z

Probably a flake.

Okay, thanks for your reply. Is it necessary to confirm this?

vitalybuka · 2024-12-03T07:03:23Z

Probably a flake.

Okay, thanks for your reply. Is it necessary to confirm this?

Multiple following builds have no this error and the patch is still there. https://lab.llvm.org/buildbot/#/builders/186

llvmssh · 2024-12-03T07:08:16Z

Probably a flake.

Okay, thanks for your reply. Is it necessary to confirm this?

Multiple following builds have no this error and the patch is still there. https://lab.llvm.org/buildbot/#/builders/186

Ok, thanks.

llvmbot added the llvm:binary-utilities label Nov 25, 2024

llvmssh force-pushed the llvm_ssh_main branch from fa5890f to 86e55de Compare November 26, 2024 01:26

llvmssh force-pushed the llvm_ssh_main branch from 86e55de to de57b7a Compare November 26, 2024 01:43

vitalybuka requested review from pcc and vitalybuka December 2, 2024 01:56

vitalybuka reviewed Dec 2, 2024

View reviewed changes

llvm/lib/Object/RecordStreamer.h Outdated Show resolved Hide resolved

vitalybuka reviewed Dec 2, 2024

View reviewed changes

llvm/lib/Object/RecordStreamer.h Show resolved Hide resolved

llvmssh changed the title ~~Use MapVector to fix lld thinLTO bep issue.~~ Use MapVector to fix lld thinLTO "nondeterminism" issue. Dec 2, 2024

vitalybuka requested a review from teresajohnson December 2, 2024 02:41

vitalybuka self-requested a review December 2, 2024 02:55

Use MapVector to fix lld thinLTO nondeterminism issue.

56e28a9

When the ModuleSymbolTable is generated, the binary consistency problem occurs due to the unorder of the data structure when collecting asm symbols.

llvmssh force-pushed the llvm_ssh_main branch from de57b7a to 56e28a9 Compare December 2, 2024 09:43

teresajohnson approved these changes Dec 2, 2024

View reviewed changes

vitalybuka approved these changes Dec 2, 2024

View reviewed changes

Order includes

5afae5c

vitalybuka merged commit 0c98776 into llvm:main Dec 2, 2024
8 checks passed

Use MapVector to fix lld thinLTO "nondeterminism" issue. #117551

Use MapVector to fix lld thinLTO "nondeterminism" issue. #117551

Uh oh!

Conversation

llvmssh commented Nov 25, 2024 • edited by vitalybuka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 25, 2024

Uh oh!

llvmbot commented Nov 25, 2024

Uh oh!

jh7370 commented Nov 25, 2024

Uh oh!

github-actions bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmssh commented Nov 26, 2024

Uh oh!

jh7370 commented Nov 26, 2024

Uh oh!

llvmssh commented Nov 26, 2024

Uh oh!

jh7370 commented Nov 26, 2024

Uh oh!

llvmssh commented Nov 27, 2024

Uh oh!

llvmssh commented Nov 30, 2024

Uh oh!

llvmssh commented Dec 2, 2024

Uh oh!

vitalybuka commented Dec 2, 2024

Uh oh!

llvmssh commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitalybuka commented Dec 2, 2024

Uh oh!

Uh oh!

Uh oh!

vitalybuka commented Dec 2, 2024

Uh oh!

teresajohnson left a comment

Choose a reason for hiding this comment

Uh oh!

vitalybuka commented Dec 2, 2024

Uh oh!

Uh oh!

github-actions bot commented Dec 2, 2024

Uh oh!

vitalybuka commented Dec 2, 2024

Uh oh!

llvmssh commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitalybuka commented Dec 3, 2024

Uh oh!

llvmssh commented Dec 3, 2024

Uh oh!

vitalybuka commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmssh commented Dec 3, 2024

Uh oh!

Uh oh!

llvmssh commented Nov 25, 2024 •

edited by vitalybuka

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

llvmssh commented Dec 2, 2024 •

edited

Loading

llvmssh commented Dec 3, 2024 •

edited

Loading

vitalybuka commented Dec 3, 2024 •

edited

Loading