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

Clobber between alloca and stack arg #63430

Closed
nikic opened this issue Jun 21, 2023 · 5 comments
Closed

Clobber between alloca and stack arg #63430

nikic opened this issue Jun 21, 2023 · 5 comments

Comments

@nikic
Copy link
Contributor

nikic commented Jun 21, 2023

define i1 @test(ptr %a0, ptr %a1, ptr %a2, ptr %a3, ptr %a4, ptr %a5, i128 %x) {
  %alloca = alloca i128
  store i128 %x, ptr %alloca
  store i128 0, ptr %alloca
  %cmp = icmp eq i128 %x, -1
  ret i1 %cmp
}

Results in:

	movq	8(%rsp), %rax
	xorps	%xmm0, %xmm0
	movaps	%xmm0, 8(%rsp)
	andq	16(%rsp), %rax
	cmpq	$-1, %rax
	sete	%al
	retq

The final argument is passed in 8(%rsp) and 16(%rsp). The zero store writes 128 bits to 8(%rsp), clobbering the argument before it is read.

@llvmbot
Copy link
Collaborator

llvmbot commented Jun 21, 2023

@llvm/issue-subscribers-backend-x86

@nikic
Copy link
Contributor Author

nikic commented Jun 21, 2023

This should be a bug in the findArgumentCopyElisionCandidates() optimization.

@nikic
Copy link
Contributor Author

nikic commented Jun 21, 2023

This is another of those issues where I wonder how we never hit this before. The copy elision optimization seems to completely fail to account for the possibility that a stack slot that an argument has been stored into can be clobbered with a different value later.

@nikic nikic self-assigned this Jun 21, 2023
@nikic
Copy link
Contributor Author

nikic commented Jun 21, 2023

Okay, this is less bad than I thought. Apparently the way this is supposed to work is that if such a store gets elided, the load chain becomes the new root, ensuring that these loads happen before any potential clobbers. However, it seems that the code assumes that the argument only has one part, and will only preserve the chain of that first part. So in this case we lose the chain from the second load.

@nikic
Copy link
Contributor Author

nikic commented Jun 21, 2023

Candidate patch: https://reviews.llvm.org/D153432

@nikic nikic closed this as completed in 81ec494 Jun 22, 2023
Chenyang-L pushed a commit to intel/llvm that referenced this issue Jul 11, 2023
…R63430)

When eliding an argument copy, we need to update the chain to ensure
the argument reads are performed before later writes. However, the
code doing this only handled this for the first part of the argument.
If the argument had multiple parts, the chains of the later parts were
dropped. Make sure we preserve all chains.

Fixes llvm/llvm-project#63430.
veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue Aug 30, 2024
…R63430)

When eliding an argument copy, we need to update the chain to ensure
the argument reads are performed before later writes. However, the
code doing this only handled this for the first part of the argument.
If the argument had multiple parts, the chains of the later parts were
dropped. Make sure we preserve all chains.

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

No branches or pull requests

3 participants