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.

Sign up for GitHub

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

Jump to bottom

Coroutine miscompilation on arm64 leading to invalid memory access #51843

Closed
ztlpn opened this issue Nov 14, 2021 · 4 comments
Closed

Coroutine miscompilation on arm64 leading to invalid memory access #51843

ztlpn opened this issue Nov 14, 2021 · 4 comments
Labels
bugzilla Issues migrated from bugzilla coroutines C++20 coroutines

Comments

@ztlpn
Copy link

ztlpn commented Nov 14, 2021

Bugzilla Link 52501
Version 13.0
OS Linux
Attachments test case source code
CC @zygoloid

Extended Description

Hi, the bug is best described by the (very simple) attached test case: there is a coroutine do_run with an infinite loop containing a suspension point. Variable val is created on the stack inside the loop. Address of this variable is then taken, stuck into the args struct and passed to the func function which dereferences the pointer and returns the result. We then check that the returned value is equal to the original value of val. The code in main() simply resumes a coroutine a few times.

When compiled on arm64 this results in invalid memory accesses by the program (bogus values returned from func/use-of-uninitialized-value under MSan).

Checked this with clang-13 downloaded from https://apt.llvm.org/focal/ although earlier versions exhibit the same behavior.

Compilation command: clang++-13 -std=c++20 -stdlib=libc++ -fcoroutines-ts -O2 test_coro.cc

Here is the compiler explorer link (compiled with a slightly older clang): https://godbolt.org/z/sxraMMvP5

AFAICT miscompilation is already present in the generated LLVM IR (https://gist.github.com/ztlpn/453b1f906e5f838fa6e47434b03c65e6): the args struct is placed into the coroutine frame and args.value_ is incorrectly initialized with a pointer to memory allocated in the stack frame of the coroutine entry function (line 27, 38). By contrast, if I compile on x86_64, memory for val is correctly allocated in the stack frame of the coroutine resume function and the program doesn't crash.
@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
@MatzeB
Copy link
Contributor

MatzeB commented Dec 16, 2022

We were running into the same problem. I put up a fix in https://reviews.llvm.org/D140231

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 16, 2022

@llvm/issue-subscribers-backend-aarch64

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 16, 2022

@llvm/issue-subscribers-coroutines

@MatzeB
Copy link
Contributor

MatzeB commented Jan 4, 2023

I pushed a fix with https://reviews.llvm.org/D140231

@MatzeB MatzeB closed this as completed Jan 4, 2023
@ztlpn ztlpn mentioned this issue Feb 25, 2023
Closed
6 tasks
mmaslankaprv added a commit to mmaslankaprv/redpanda that referenced this issue Feb 27, 2023
@mmaslankaprv
k/alter_reassignments: converted handler to continuation chain
830e098 
Converted `AlterPartitionReassignmentRequest` handler to use
continuation chain. This change was required becuase of bug in clang
(llvm/llvm-project#51843)

Fixes: redpanda-data#9089

Signed-off-by: Michal Maslanka <michal@redpanda.com>
mmaslankaprv added a commit to mmaslankaprv/redpanda that referenced this issue Feb 27, 2023
@mmaslankaprv
k/alter_reassignments: converted handler to continuation chain
9419d3a 
Converted `AlterPartitionReassignmentRequest` handler to use
continuation chain. This change was required becuase of bug in clang
(llvm/llvm-project#51843)

Fixes: redpanda-data#9089

Signed-off-by: Michal Maslanka <michal@redpanda.com>
NyaliaLui pushed a commit to NyaliaLui/redpanda that referenced this issue Feb 28, 2023
@mmaslankaprv @NyaliaLui
k/alter_reassignments: converted handler to continuation chain
f40514c 
Converted `AlterPartitionReassignmentRequest` handler to use
continuation chain. This change was required becuase of bug in clang
(llvm/llvm-project#51843)

Fixes: redpanda-data#9089

Signed-off-by: Michal Maslanka <michal@redpanda.com>
NyaliaLui pushed a commit to NyaliaLui/redpanda that referenced this issue Feb 28, 2023
@mmaslankaprv @NyaliaLui
k/alter_reassignments: converted handler to continuation chain
5b2f95b 
Converted `AlterPartitionReassignmentRequest` handler to use
continuation chain. This change was required becuase of bug in clang
(llvm/llvm-project#51843)

Fixes: redpanda-data#9089

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@NyaliaLui NyaliaLui mentioned this issue Feb 28, 2023
Merged
6 tasks
NyaliaLui pushed a commit to NyaliaLui/redpanda that referenced this issue Feb 28, 2023
@mmaslankaprv @NyaliaLui
k/alter_reassignments: converted handler to continuation chain
2e12f2f 
Converted `AlterPartitionReassignmentRequest` handler to use
continuation chain. This change was required becuase of bug in clang
(llvm/llvm-project#51843)

Fixes: redpanda-data#9089

Signed-off-by: Michal Maslanka <michal@redpanda.com>
@BenPope BenPope mentioned this issue Feb 28, 2023
Open
vbotbuildovich pushed a commit to vbotbuildovich/redpanda that referenced this issue Feb 28, 2023
@mmaslankaprv @vbotbuildovich
k/alter_reassignments: converted handler to continuation chain
8889ba3 
Converted `AlterPartitionReassignmentRequest` handler to use
continuation chain. This change was required becuase of bug in clang
(llvm/llvm-project#51843)

Fixes: redpanda-data#9089

Signed-off-by: Michal Maslanka <michal@redpanda.com>
(cherry picked from commit 5b2f95b)
@avikivity avikivity mentioned this issue May 21, 2023
Closed
@michoecho michoecho mentioned this issue Jul 25, 2023
Closed
michoecho added a commit to michoecho/scylla that referenced this issue Aug 1, 2023
@michoecho
locator: util: add a workaround for a clang 15 coroutine miscompile
efcf024 
llvm/llvm-project#51843 causes clang-15 to miscompile some coroutines.
We hit it with describe_ring() on ARM.

The fmt::to_string() call inside the coroutine triggers the bug.
The workaround is to move the call out of the coroutine.

Fixes scylladb#14412
michoecho added a commit to michoecho/scylla that referenced this issue Aug 1, 2023
@michoecho
locator: util: add a workaround for a clang 15 coroutine miscompile
7e599bd 
llvm/llvm-project#51843 causes clang-15 to miscompile some coroutines.
We hit it with describe_ring() on ARM.

The fmt::to_string() call inside the coroutine triggers the bug.
The workaround is to move the call out of the coroutine.

Fixes scylladb#14412
@michoecho michoecho mentioned this issue Aug 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla coroutines C++20 coroutines
Projects
C++ 20 in Clang
Status: Done
Milestone
No milestone
Development

No branches or pull requests
4 participants
@MatzeB @ztlpn @EugeneZelenko @llvmbot