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

[ORC] Preserve order of constructors with same priority #95532

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Jun 14, 2024

Constructors with the same priority should keep their relative order that was specified. This is important for clang-repl with many const variables after commit 05137ec ("[clang-repl] Emit const variables only once").

Constructors with the same priority should keep their relative order
that was specified. This is important for clang-repl with many const
variables after commit 05137ec ("[clang-repl] Emit const variables
only once").
Copy link
Contributor

@lhames lhames left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks very much @hahnjo!

@hahnjo hahnjo merged commit e5d0627 into llvm:main Jun 18, 2024
7 of 9 checks passed
@hahnjo hahnjo deleted the orc-ctor-order branch June 18, 2024 06:13
hahnjo added a commit that referenced this pull request Jun 18, 2024
@hahnjo
Copy link
Member Author

hahnjo commented Jun 18, 2024

Reverted in edd6f0c because the test fails on 32-bit ARMv8: https://lab.llvm.org/buildbot/#/builders/154/builds/170 It works on 64-bit, I'll need time to investigate...

@hahnjo
Copy link
Member Author

hahnjo commented Jun 18, 2024

I'm having some difficulties debugging this issue since I can only get access to AArch64 machines that are either not properly configured to run 32-bit ARM executables or have too old software (CMake, Python, GCC) to even build the latest LLVM. @DavidSpickett any chance you could help here? For a start, it would be really interesting to know what the actual output is, ie if all constructors were run and in which order...

@DavidSpickett
Copy link
Collaborator

Sure, I'll reproduce it locally.

@DavidSpickett
Copy link
Collaborator

Some of them are missing but they have empty lines where they should be:

$ /home/david.spickett/build-llvm-arm/bin/lli -jit-kind=orc /home/david.spickett/llvm-project/llvm/test/ExecutionEngine/Orc/global-ctor-order.ll
H1

H3

M2

1
2
3
4
5
6
7
8
9
10

12

14

16

@DavidSpickett
Copy link
Collaborator

If I revert the change to stable_sort I see the same gaps though:

$ /home/david.spickett/build-llvm-arm/bin/lli -jit-kind=orc /home/david.spickett/llvm-project/llvm/test/ExecutionEngine/Orc/global-ctor-order.ll
H3
H1

M2



1
2

16

14

12
5
10
9
3
8
7
6
4

@hahnjo
Copy link
Member Author

hahnjo commented Jun 18, 2024

Some of them are missing but they have empty lines where they should be:

Okay, that was my reading of the buildbot failure, but I couldn't think of a good reason why it should be this way...

If I revert the change to stable_sort I see the same gaps though:

Right, and the order matches what I remember seeing on x86, which should mean it's something else. Could you maybe try adding --jit-linker=rtdyld to the lli invocation? One possibility would be that some relocations are not properly resolved and that's why it's printing null strings...

@DavidSpickett
Copy link
Collaborator

DavidSpickett commented Jun 18, 2024

Same result with that:

$ /home/david.spickett/build-llvm-arm/bin/lli -jit-kind=orc --jit-linker=rtdyld /home/david.spickett/llvm-project/llvm/test/ExecutionEngine/Orc/global-ctor-order.ll
H1

H3

M2

1
2
3
4
5
6
7
8
9
10

12

14

16

We do suspect relocations in another recent issue #94994, so it could be that. In that issue the initial value of a variable wasn't being loaded at all. We're not sure of the cause at the moment.

@hahnjo
Copy link
Member Author

hahnjo commented Jun 18, 2024

We do suspect relocations in another recent issue #94994, so it could be that. In that issue the initial value of a variable wasn't being loaded at all. We're not sure of the cause at the moment.

Thanks for the pointer, this indeed sounds related. What does this mean for this fix? Do you think it's acceptable to open a new issue and mark the test as UNSUPPORTED on arm for the moment?

DavidSpickett added a commit that referenced this pull request Jun 18, 2024
…)"

This reverts commit edd6f0c.

The newly added test uncovered a pre-existing issue on Arm 32 bit,
so as we did #94994, disable
it while we find the problem.
@DavidSpickett
Copy link
Collaborator

Yes, I've opened #95911 and relanded the change with the test marked unsupported.

@hahnjo
Copy link
Member Author

hahnjo commented Jun 18, 2024

Thanks! Now I won't get emails though because I'm not anymore the commit author... I'll try to keep an eye on the buildbots manually.

@weliveindetail
Copy link
Contributor

Hey, interesting bug on arm32. I will look into it next week!

devajithvs pushed a commit to root-project/llvm-project that referenced this pull request Aug 21, 2024
…vm#95532)

Constructors with the same priority should keep their relative order
that was specified. This is important for `clang-repl` with many `const`
variables after commit 05137ec ("[clang-repl] Emit const variables
only once").

(cherry picked from commit e5d0627)
hahnjo added a commit to hahnjo/root that referenced this pull request Sep 13, 2024
LLVM had a bug where constructors with the same priority would not be
stably sorted. This has been fixed upstream by
llvm/llvm-project#95532, but to avoid relying
on a backport this pass works around the issue: The idea is that we
lower the default priority of concerned constructors to make them sort
correctly.
hahnjo added a commit to hahnjo/root that referenced this pull request Sep 13, 2024
LLVM had a bug where constructors with the same priority would not be
stably sorted. This has been fixed upstream by
llvm/llvm-project#95532, but to avoid relying
on a backport this commit works around the issue: The idea is that we
lower the default priority of concerned constructors to make them sort
correctly.
hahnjo added a commit to hahnjo/root that referenced this pull request Sep 13, 2024
LLVM had a bug where constructors with the same priority would not be
stably sorted. This has been fixed upstream by
llvm/llvm-project#95532, but to avoid relying
on a backport this commit works around the issue: The idea is that we
lower the default priority of concerned constructors to make them sort
correctly.
hahnjo added a commit to root-project/root that referenced this pull request Sep 18, 2024
LLVM had a bug where constructors with the same priority would not be
stably sorted. This has been fixed upstream by
llvm/llvm-project#95532, but to avoid relying
on a backport this commit works around the issue: The idea is that we
lower the default priority of concerned constructors to make them sort
correctly.
FonsRademakers pushed a commit to root-project/cling that referenced this pull request Sep 18, 2024
LLVM had a bug where constructors with the same priority would not be
stably sorted. This has been fixed upstream by
llvm/llvm-project#95532, but to avoid relying
on a backport this commit works around the issue: The idea is that we
lower the default priority of concerned constructors to make them sort
correctly.
hahnjo added a commit to hahnjo/root that referenced this pull request Sep 18, 2024
LLVM had a bug where constructors with the same priority would not be
stably sorted. This has been fixed upstream by
llvm/llvm-project#95532, but to avoid relying
on a backport this commit works around the issue: The idea is that we
lower the default priority of concerned constructors to make them sort
correctly.

(cherry picked from commit 7db43f7)
hahnjo added a commit to root-project/root that referenced this pull request Sep 18, 2024
LLVM had a bug where constructors with the same priority would not be
stably sorted. This has been fixed upstream by
llvm/llvm-project#95532, but to avoid relying
on a backport this commit works around the issue: The idea is that we
lower the default priority of concerned constructors to make them sort
correctly.

(cherry picked from commit 7db43f7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants