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

Incorrect assembly emitted for cmpxchg IR instruction on thumbv7m-none-eabi target #38176

Closed
llvmbot opened this issue Sep 4, 2018 · 5 comments
Closed

Incorrect assembly emitted for cmpxchg IR instruction on thumbv7m-none-eabi target #38176

llvmbot opened this issue Sep 4, 2018 · 5 comments
Labels
backend:ARM bugzilla Issues migrated from bugzilla

Comments

@llvmbot
Copy link
Member

llvmbot commented Sep 4, 2018

Bugzilla Link 38828
Resolution FIXED
Resolved on Nov 16, 2018 01:51
Version trunk
OS other
Attachments C program that reproduces the problem, Generated assembly, Generated IR
Reporter LLVM Bugzilla Contributor
CC @efriedma-quic,@kbeyls,@TNorthover
Fixed by commit(s) 341642

Extended Description

When compiled with:

clang -c -target thumbv7m-none-eabi -O1 compare_exchange.c

The attached C program produces invalid assembly code that attempts to load a value from outside the function's stack frame in the first "ldrex" instruction of the compare exchange operation. The rest of the stack access generated by the cmpxchg seem to be correct. More weird results can be obtained by adding/removing "int"s before the atomic one. I've also attached the generated assembly and IR code from my machine.

The problem seems to be on the assembly generation level and not in clang itself (I ran into this while using Rust). The example program uses intrinsics instead of standard C to make sure it's not a header issue, but the same problem happens with stdatomic.h.

Compiling with "-target thumbv7-none-eabi" instead seems to produce correct code, as well as compiling with -O0.

LLVM version: Tip of trunk from SVN, as well as 6.0.0-1ubuntu2 (my machine, from the official Ubuntu repositories)

Godbolt link for quick verification:
https://godbolt.org/z/Ozr2xa
@TNorthover
Copy link
Contributor

The issue appears to be that t2LDREX is being produced with an (offset) FrameIndex operand, but it is marked as AddrModeNone in the .td file. rewriteFrameIndex does not expect this and asserts in a Debug build. Incorrect code is emitted in a release build.

The options for fixing it are:

  1. Implement AddrModet2LDREX (and B, H, possibly ARM too) to do the correct folding.
  2. Prevent t2LDREX (& others?) from getting a FrameIndex in the first place.
  3. Allow AddrModeNone in the frame rewriting code (presumably always booting address calculation out of the instruction).

The first one seems like the highest quality solution.

@efriedma-quic
Copy link
Collaborator

Prevent t2LDREX (& others?)

As far as I can tell, this should only affect t2addrmode_imm0_1020s4, which is only used by t2LDREX and t2STREX .

I would prefer to just take the simple fix of changing SelectT2AddrModeExclusive so it doesn't try to special-case frame indexes, since that path clearly hasn't been tested. If someone cares, we can try to optimize later, but it's unlikely to matter much.

@TNorthover
Copy link
Contributor

I would prefer to just take the simple fix of changing SelectT2AddrModeExclusive so it doesn't try to special-case frame indexes, since that path clearly hasn't been tested. If someone cares, we can try to optimize later, but it's unlikely to matter much.

I'd still prefer to do the right thing. An unused addressing mode is offensive to me.

I'll see what I can come up with tomorrow. After all, it looks like I added the bug back in 2013.

@TNorthover
Copy link
Contributor

I've posted a patch for review at https://reviews.llvm.org/D51678.

@kbeyls
Copy link
Collaborator

kbeyls commented Nov 16, 2018

Based on the comments it seems this was fixed in 341642.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backend:ARM bugzilla Issues migrated from bugzilla
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@TNorthover @kbeyls @efriedma-quic @llvmbot