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

Call ABI broken for >=7 arguments with commit ae26b7e4e! (LLVM-73) #26

Closed
arjanmels opened this issue May 16, 2020 · 7 comments
Closed

Comments

@arjanmels
Copy link

arjanmels commented May 16, 2020

With the latest commit ae26b7e, the call abi got broken for functions with 7 or more arguments.

Rust example:

#[no_mangle]
#[inline(never)]
fn CALLING_CONVENTION(a: u32, b: u32, c: u32, d: u32, e: u32, f: u32, g: u32) -> u32 {
    a + b + c + d + e + f + g
}

#[no_mangle]
#[inline(never)]
fn CALL_CALLING_CONVENTION() -> u32 {
    CALLING_CONVENTION(1, 2, 3, 4, 5, 6, 7)
}

fn main () -> ! {
	let res = CALL_CALLING_CONVENTION();
	assert!(res==28);
	...
}

Generated asm in release mode (debug mode has same problem, but generates a lot of check code):

As you can see the 7th argument is not retrieved from the stack, but a direct "add.n a2,a8,a9" is done (a9 is the address in memory).

(Also I wonder what the mov.n a8,a1 does at the start, it seem superfluous.)

CALLING_CONVENTION:
	entry	a1, 32
	mov.n	a8, a1
	add.n	a8, a3, a2
	add.n	a8, a8, a4
	add.n	a8, a8, a5
	add.n	a8, a8, a6
	add.n	a8, a8, a7
	addi	a9, a1, 32
	add.n	a2, a8, a9
	retw.n

With the previous commit (a5a002f) the resulting code was:

Notice the l32i.n instead of addi

CALLING_CONVENTION:
	entry	a1, 32
	mov.n	a8, a1
	add.n	a8, a3, a2
	add.n	a8, a8, a4
	add.n	a8, a8, a5
	add.n	a8, a8, a6
	add.n	a8, a8, a7
	l32i.n	a9, a1, 32
	add.n	a2, a8, a9
	retw.n
@arjanmels arjanmels changed the title *Call ABI broken for >=7 arguments* Call ABI broken for >=7 arguments! May 16, 2020
@github-actions github-actions bot changed the title Call ABI broken for >=7 arguments! Call ABI broken for >=7 arguments! (LLVM-73) May 16, 2020
@arjanmels arjanmels changed the title Call ABI broken for >=7 arguments! (LLVM-73) Call ABI broken for >=7 arguments with commit ae26b7e! (LLVM-73) May 16, 2020
@arjanmels arjanmels changed the title Call ABI broken for >=7 arguments with commit ae26b7e! (LLVM-73) Call ABI broken for >=7 arguments with commit ae26b7e4eb0938601f8a8744ff50c178a3ef0847! (LLVM-73) May 16, 2020
@arjanmels arjanmels changed the title Call ABI broken for >=7 arguments with commit ae26b7e4eb0938601f8a8744ff50c178a3ef0847! (LLVM-73) Call ABI broken for >=7 arguments with commit ae26b7e4e! (LLVM-73) May 16, 2020
@andreisfr
Copy link
Collaborator

andreisfr commented May 21, 2020

Hi @arjanmels ! Thank you very much for reported problem. Could I ask you to attach llvm IR file and compiler arguments to reproduce the problem with broken abi? We have created some test coverage for ABI functionality and now I verified similar example with latest version of project (I've downloaded and built it from scratch):

int abi_test(int a1, int a2, int a3, int a4, int a5, int a6, int a7)
{

 return a1 + a2 + a3 + a4 + a5 + a6 + a7;

}

the result of compilation with options "-target xtensa -mcpu=esp32 -Os -S test.c -o test.S" is like this:

abi_test:
entry a1, 32
mov.n a8, a7
mov.n a7, a1
add.n a9, a3, a2
add.n a9, a9, a4
add.n a9, a9, a5
add.n a9, a9, a6
add.n a8, a9, a8
l32i.n a9, a7, 32
add.n a2, a8, a9
movsp a1, a7
retw.n

There is still some redundant mov instructions, but last argument is loaded from stack. Our call abi testsuite probably not cover all cases, so we can correct the problem, once we will be able to reproduce it. Thanks again.

@arjanmels
Copy link
Author

arjanmels commented May 24, 2020

The IR is in both cases identical:

; ModuleID = 'test'
source_filename = "test"
target datalayout = "e-m:e-p:32:32-i8:8:32-i16:16:32-i64:64-n32"
target triple = "xtensa-none-unknown-elf"

; Function Attrs: noinline norecurse nounwind readnone
define i32 @CALLING_CONVENTION(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e, i32 %f, i32 %g) unnamed_addr #0 {
start:
  %_12 = add i32 %b, %a
  %_11 = add i32 %_12, %c
  %_10 = add i32 %_11, %d
  %_9 = add i32 %_10, %e
  %_8 = add i32 %_9, %f
  %0 = add i32 %_8, %g
  ret i32 %0
}

; Function Attrs: noinline norecurse nounwind readnone
define i32 @CALL_CALLING_CONVENTION() unnamed_addr #0 {
start:
  %0 = tail call i32 @CALLING_CONVENTION(i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7)
  ret i32 %0
}

attributes #0 = { noinline norecurse nounwind readnone "target-cpu"="esp32" }

Hope this helps.

@arjanmels
Copy link
Author

I also ran the c version through clang and confirm I get the same assembly code as you do.

When I generate llvm-IR, I get below. It looks like clang handles the call ABI here (last argument is a pointer).

; ModuleID = 'test.c'
source_filename = "test.c"
target datalayout = "e-m:e-p:32:32-i8:8:32-i16:16:32-i64:64-n32"
target triple = "xtensa"

; Function Attrs: norecurse nounwind optsize readonly
define dso_local i32 @abi_test(i32, i32, i32, i32, i32, i32, i32* nocapture readonly byval(i32) align 4) local_unnamed_addr #0 {
  %8 = load i32, i32* %6, align 4, !tbaa !2
  %9 = add nsw i32 %1, %0
  %10 = add nsw i32 %9, %2
  %11 = add nsw i32 %10, %3
  %12 = add nsw i32 %11, %4
  %13 = add nsw i32 %12, %5
  %14 = add nsw i32 %13, %8
  ret i32 %14
}

attributes #0 = { norecurse nounwind optsize readonly "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="esp32" "unsafe-fp-math"="false" "use-soft-float"="false" }

!llvm.module.flags = !{!0}
!llvm.ident = !{!1}

!0 = !{i32 1, !"wchar_size", i32 1}
!1 = !{!"clang version 9.0.1 (git@github.com:espressif/llvm-project.git ec9a331ae70eb4011b46698bfe267d4315256cf6)"}
!2 = !{!3, !3, i64 0}
!3 = !{!"int", !4, i64 0}
!4 = !{!"omnipotent char", !5, i64 0}
!5 = !{!"Simple C/C++ TBAA"}

@arjanmels
Copy link
Author

@andreisfr Hi, was the supplied info useful and sufficient?

@andreisfr
Copy link
Collaborator

Hi, @arjanmels! I can reproduce the problem now, thank you very much!

@arjanmels
Copy link
Author

Is there any update on this? This prevents me from using newest version.

@andreisfr
Copy link
Collaborator

andreisfr commented Jul 31, 2020

Hi @arjanmels ! Please verify new version 10.0.1, this error should be corrected now.

@gerekon gerekon closed this as completed Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants