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

SIGILL with new libc qsort_r implementation in BranchFolder::MergePotentialsElt #71312

Closed
crrodriguez opened this issue Nov 5, 2023 · 6 comments · Fixed by #71639 or llvm/llvm-project-release-prs#771

Comments

@crrodriguez
Copy link

llvm_unreachable("Predecessor appears twice");

Now glibc compares an object with itself, just like when _GLIBCXX_DEBUG is enabled therefore explicit unreachable triggers. this breaks at least any rustc compiler available.

#0  0x000015129e40516a in int llvm::array_pod_sort_comparator<llvm::BranchFolder::MergePotentialsElt>(void const*, void const*) () from /home/crrodriguez/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/../lib/[libLLVM-17-rust-1.75.0-nightly.so](http://libllvm-17-rust-1.75.0-nightly.so/)

#1  0x00001512a0043779 in __GI___qsort_r (pbase=0x15128c90bd80, total_elems=<optimized out>, size=0x10, cmp=<optimized out>, arg=<optimized out>) at qsort.c:335

#2  0x000015129e4df6e6 in llvm::BranchFolder::TryTailMergeBlocks(llvm::MachineBasicBlock*, llvm::MachineBasicBlock*, unsigned int) () from /home/crrodriguez/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/../lib/[libLLVM-17-rust-1.75.0-nightly.so](http://libllvm-17-rust-1.75.0-nightly.so/)
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 5, 2023

@llvm/issue-subscribers-libc

Author: Cristian Rodríguez (crrodriguez)

https://github.com/llvm/llvm-project/blob/7c1ad51e018c5a1517ca6fb58a4d7027a8bec64e/llvm/lib/CodeGen/BranchFolding.cpp#L491

Now glibc compares an object with itself, just like when _GLIBCXX_DEBUG is enabled therefore explicit unreachable triggers. this breaks at least any rustc compiler available.

#<!-- -->0  0x000015129e40516a in int llvm::array_pod_sort_comparator&lt;llvm::BranchFolder::MergePotentialsElt&gt;(void const*, void const*) () from /home/crrodriguez/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/../lib/[libLLVM-17-rust-1.75.0-nightly.so](http://libllvm-17-rust-1.75.0-nightly.so/)

#<!-- -->1  0x00001512a0043779 in __GI___qsort_r (pbase=0x15128c90bd80, total_elems=&lt;optimized out&gt;, size=0x10, cmp=&lt;optimized out&gt;, arg=&lt;optimized out&gt;) at qsort.c:335

#<!-- -->2  0x000015129e4df6e6 in llvm::BranchFolder::TryTailMergeBlocks(llvm::MachineBasicBlock*, llvm::MachineBasicBlock*, unsigned int) () from /home/crrodriguez/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/../lib/../lib/[libLLVM-17-rust-1.75.0-nightly.so](http://libllvm-17-rust-1.75.0-nightly.so/)

nikic added a commit to nikic/llvm-project that referenced this issue Nov 8, 2023
`MergePotentialElts::operator<` asserts that the two elements being
compared are not equal. However, sorting functions are allowed
to invoke the comparison function with equal arguments (though they
usually don't for efficiency reasons).

There is an existing special-case that disables the assert if
_GLIBCXX_DEBUG is used, which may invoke the comparator with equal
args to verify strict weak ordering. I believe libc++ also has
strict weak ordering checks under some options nowadays.

Most recently llvm#71312 was reported, where a change to glibc's qsort_r
implementation can also result in comparison between equal elements.
From what I understood this is an inefficiency that will be fixed
on the glibc side as well, but I think at this point we should just
remove this assertion.

Fixes llvm#71312.
@nikic nikic self-assigned this Nov 8, 2023
@nikic nikic added this to the LLVM 17.0.X Release milestone Nov 9, 2023
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Nov 9, 2023
@nikic nikic reopened this Nov 9, 2023
@nikic
Copy link
Contributor

nikic commented Nov 9, 2023

/cherry-pick 74a76a2

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 9, 2023

/branch llvm/llvm-project-release-prs/issue71312

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Nov 9, 2023
`MergePotentialElts::operator<` asserts that the two elements being
compared are not equal. However, sorting functions are allowed to invoke
the comparison function with equal arguments (though they usually don't
for efficiency reasons).

There is an existing special-case that disables the assert if
_GLIBCXX_DEBUG is used, which may invoke the comparator with equal args
to verify strict weak ordering. I believe libc++ also has strict weak
ordering checks under some options nowadays.

Recently, #71312 was reported, where a change to glibc's qsort_r
implementation can also result in comparison between equal elements.
From what I understood, this is an inefficiency that will be fixed on
the glibc side as well, but I think at this point we should just remove
this assertion.

Fixes llvm/llvm-project#71312.

(cherry picked from commit 74a76a288562c486f377121855ef7db0386e0e43)
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 9, 2023

/pull-request llvm/llvm-project-release-prs#771

@EugeneZelenko
Copy link
Contributor

Pull request is still open.

@EugeneZelenko EugeneZelenko reopened this Nov 9, 2023
@tru tru moved this from Needs Triage to Needs Review in LLVM Release Status Nov 13, 2023
tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Nov 14, 2023
`MergePotentialElts::operator<` asserts that the two elements being
compared are not equal. However, sorting functions are allowed to invoke
the comparison function with equal arguments (though they usually don't
for efficiency reasons).

There is an existing special-case that disables the assert if
_GLIBCXX_DEBUG is used, which may invoke the comparator with equal args
to verify strict weak ordering. I believe libc++ also has strict weak
ordering checks under some options nowadays.

Recently, #71312 was reported, where a change to glibc's qsort_r
implementation can also result in comparison between equal elements.
From what I understood, this is an inefficiency that will be fixed on
the glibc side as well, but I think at this point we should just remove
this assertion.

Fixes llvm/llvm-project#71312.

(cherry picked from commit 74a76a288562c486f377121855ef7db0386e0e43)
@tru tru moved this from Needs Review to Done in LLVM Release Status Nov 14, 2023
qihangkong pushed a commit to rvgpu/llvm that referenced this issue Apr 18, 2024
`MergePotentialElts::operator<` asserts that the two elements being
compared are not equal. However, sorting functions are allowed to invoke
the comparison function with equal arguments (though they usually don't
for efficiency reasons).

There is an existing special-case that disables the assert if
_GLIBCXX_DEBUG is used, which may invoke the comparator with equal args
to verify strict weak ordering. I believe libc++ also has strict weak
ordering checks under some options nowadays.

Recently, #71312 was reported, where a change to glibc's qsort_r
implementation can also result in comparison between equal elements.
From what I understood, this is an inefficiency that will be fixed on
the glibc side as well, but I think at this point we should just remove
this assertion.

Fixes llvm/llvm-project#71312.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment