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

LTO miscompilation on some rust<->C/C++ calls #58307

Closed
glandium opened this issue Oct 12, 2022 · 6 comments · Fixed by llvm/llvm-project-release-prs#199
Closed

LTO miscompilation on some rust<->C/C++ calls #58307

glandium opened this issue Oct 12, 2022 · 6 comments · Fixed by llvm/llvm-project-release-prs#199

Comments

@glandium
Copy link
Contributor

glandium commented Oct 12, 2022

The Rust compiler doesn't create IR identical to clang for the same ABI: rust-lang/rust#102174. When doing cross-language LTO, this leads LLVM to do the wrong things when it inlines those calls. This is a regression from 6c8adc5.

STR:

  • Create the following files:
    foo.ll (derived from rustc output, see further below for original source)
target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
target triple = "i686-unknown-linux-gnu"

%Foo = type { i32 }
define i32 @foo (ptr byval(%Foo) %foo) {
  %1 = load i32, ptr %foo, align 4
  ret i32 %1
}

bar.ll (derived from clang output, see further below for original source)

target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
target triple = "i686-unknown-linux-gnu"

define i32 @bar(i32 %0) {
  %2 = tail call i32 @foo(i32 %0)
  ret i32 %2
}

declare i32 @foo(i32)
  • llvm-as foo.ll
  • llvm-as bar.ll
  • llvm-lto foo.bc bar.bc -filetype=asm -exported-symbol=bar -o -
	.text
	.file	"ld-temp.o"
	.globl	bar
	.p2align	4, 0x90
	.type	bar,@function
bar:
	movl	4(%esp), %eax
	movl	(%eax), %eax
	retl
.Lfunc_end0:
	.size	bar, .Lfunc_end0-bar

	.section	".note.GNU-stack","",@progbits

movl (%eax), %eax shouldn't be there, it's dereferencing something that is not originally a pointer.

What happens is that InstCombine creates the mess by handling the i32->byval conversion as a i32->ptr conversion:

*** IR Dump After InstCombinePass on bar ***
; Function Attrs: mustprogress nofree nosync readnone willreturn
define i32 @bar(i32 %0) local_unnamed_addr #1 {
  %2 = inttoptr i32 %0 to ptr
  %3 = tail call i32 @foo(ptr %2)
  ret i32 %3
}

It should be something like

%2 = alloca i32
store i32 %0, i32* %2

instead of inttoptr, or it should bail out (i.e. what happened before 6c8adc5).

Patch for the latter incoming.

Original source:
foo.rs

// rustc --emit=llvm-ir foo.rs --crate-type=lib --target=i686-unknown-linux-gnu
#[repr(C)]
pub struct Foo(pub u32);

#[no_mangle]
pub unsafe extern "C" fn foo(foo: Foo) -> u32 {
    foo.0
}

bar.c

// clang -O3 -o - -S bar.c -emit-llvm --target=i686-unknown-linux-gnu
struct Foo { int a; };

extern int foo(struct Foo f);

int bar(struct Foo f) {
    return foo(f);
}
@glandium
Copy link
Contributor Author

@glandium
Copy link
Contributor Author

@tru can this be added to the 15.0.4 milestone?

@tru tru added this to the LLVM 15.0.4 Release milestone Oct 21, 2022
@tru
Copy link
Collaborator

tru commented Oct 21, 2022

When the fix has landed in main. Run the cherry pick comment command to queue up a pick to the release branch.

@tru
Copy link
Collaborator

tru commented Oct 23, 2022

/cherry-pick 86e57e6

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 23, 2022

/branch llvm/llvm-project-release-prs/issue58307

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 23, 2022

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

tru pushed a commit that referenced this issue Oct 24, 2022
…al is involved.

Fixes #58307

Reviewed By: nikic

Differential Revision: https://reviews.llvm.org/D135738

(cherry picked from commit 86e57e6)
sid8123 pushed a commit to sid8123/llvm-project that referenced this issue Oct 25, 2022
virnarula pushed a commit to virnarula/llvm-project that referenced this issue Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants