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

Extend and clean the ccall test suite. #14215

Closed
wants to merge 2 commits into from
Closed

Extend and clean the ccall test suite. #14215

wants to merge 2 commits into from

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Dec 1, 2015

The current ccall test suite doesn't match/test all functionality in libccalltest. This PR removes some unused functionality from libccalltest, but mainly extends libccalltest and test/ccall to align the different structs and test methods.

As part of a small clean-up, I've also scoped most tests in a let...end block, and added a factor-like parameter to the struct test methods to catch cases where the ABI allocates too much memory for a certain parameter (it helped me discover issues with the ARM ABI from #14194).

I've only tested these changes on x86_64, so it's possible that it triggers some issues on other platforms...

cc @vtjnash

[edit by tkelman: fixes https://github.com//issues/13242]

@maleadt
Copy link
Member Author

maleadt commented Dec 1, 2015

Also, as part of #14204 I learned that I should use the C type aliases. Should I convert the ccall test suite to use these types? I've currently kept it at how it was implemented before, except where the types differ (i.e. use Cchar instead of Char, but still use Float64 etc. because these don't differ from the C types).

@vtjnash
Copy link
Member

vtjnash commented Dec 1, 2015

no, there's no need to convert. Char corresponds to wchar_t (usually), not char (similar to how Bool corresponds to unsigned char or C++ bool, not int as is customary in C; and Int corresponds to intptr_t / size_t, not int), so that's really just a matter of misleading aliases. The Cprefix aliases are available to help with that conversion, but are only really necessary for places where platforms differ (for example, Cchar is aware of the platform-specific sign for char, Clong is aware of the number of bits in a long, etc.)

this looks like a good improvement. the previous tests were usually enough to cause failures for the x86_64 ABI, but I'm not surprised they were insufficient to catch other problems.

perhaps you already know this (as advice for #14194), but part of the original value of this file is that you can introspect how clang lowers it as a way of matching the EABI spec to the llvm attributes:
clang -emit-llvm -S -o - ccalltest.c

@maleadt
Copy link
Member Author

maleadt commented Dec 1, 2015

@vtjnash OK thanks for the clarification, much appreciated!
About that clang usage, in a comment on #13752 you used --target=arm-pc-linux-elf, but code generated with that command doesn't use the VFP (I'm not sure if it's EABI). For developing the ABI, I've been using --target=arm-pc-linux-elf-eabi instead. Any idea about the exact differences?

@tkelman
Copy link
Contributor

tkelman commented Dec 1, 2015

Char is not wchar_t on windows.

@maleadt
Copy link
Member Author

maleadt commented Dec 5, 2015

So apparently this trips up the Win32 ABI (unsure about the Win64 one, it just seems to have halted). I guess those ABI's will need fixing before this can be merged? There's also some compilation warnings in ccalltest.c which I should look at.

@tkelman
Copy link
Contributor

tkelman commented Dec 5, 2015

The win64 freeze is probably not your fault, I would restart it but there's a conflict so this needs a rebase.

@vtjnash
Copy link
Member

vtjnash commented Dec 5, 2015

Yep, good catch. It looks like we have a couple mistakes in our ABI. We had copied our ABI from LDC, and they fixed this over the summer: ldc-developers/ldc#856

  • the conditional in use_sret should be size == 1 || size == 2 || size == 4 || size == 8 (instead of size <= 8 -- or this is a clang bug, but i doubt that). this change also needs to be made to the win64 abi and darwin32* abi.
  • needPassByRef should use the x32 logic (minus the special case for complex)
  • preferred_llvm_type logic will need to take into account the change to use_sret (ibid for win64)

[*] source: https://github.com/ldc-developers/ldc/blob/bddd95194815c24c48676e1b2132436c0e4df96d/gen/abi-x86.cpp#L67

@vtjnash vtjnash mentioned this pull request Jan 5, 2016
@maleadt maleadt mentioned this pull request Jan 11, 2016
@ViralBShah
Copy link
Member

Bump. Does this help improve testing, given the recent arm and aarch64 abi implementations?

@maleadt
Copy link
Member Author

maleadt commented Jan 22, 2016

Yeah sorry for not updating this, but I haven't found the time to fix the Windows ABI. Without that, I guess this can't get merged. Maybe somebody else could look at this?

Concerning the ARM ABI, it greatly helped me verify the the implementation and find corner cases, and it should pass this test suite without any issue. Though there are still some untested code paths (I should probably look at coverage statistics some day), this PR greatly improves testing coverage.
Lacking an ARMv8 board, I haven't tested the AArch64 ABI using this PR. Maybe @yuyichao has?

@yuyichao yuyichao added the test This change adds or pertains to unit tests label Jan 23, 2016
@yuyichao
Copy link
Contributor

For the failing test, the llvm ir generated by clang is

; ModuleID = 'test.c'
target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
target triple = "i386-pc-windows-msvc18.0.0"

%struct.struct10 = type { i8, i8, i8, i8 }

; Function Attrs: nounwind
define i32 @test_10(%struct.struct10* byval nocapture align 4 %a) #0 {
  %1 = getelementptr inbounds %struct.struct10, %struct.struct10* %a, i32 0, i32 0
  %2 = bitcast %struct.struct10* %a to i32*
  %3 = load i32, i32* %2, align 4
  %4 = trunc i32 %3 to i16
  %5 = trunc i32 %3 to i8
  %6 = add i8 %5, 1
  store i8 %6, i8* %1, align 4, !tbaa !1
  %7 = getelementptr inbounds %struct.struct10, %struct.struct10* %a, i32 0, i32 1
  %8 = lshr i16 %4, 8
  %9 = trunc i16 %8 to i8
  %10 = add i8 %9, -2
  store i8 %10, i8* %7, align 1, !tbaa !5
  %11 = getelementptr inbounds %struct.struct10, %struct.struct10* %a, i32 0, i32 2
  %12 = lshr i32 %3, 16
  %13 = trunc i32 %12 to i8
  %14 = add i8 %13, 3
  store i8 %14, i8* %11, align 2, !tbaa !6
  %15 = getelementptr inbounds %struct.struct10, %struct.struct10* %a, i32 0, i32 3
  %16 = lshr i32 %3, 24
  %17 = trunc i32 %16 to i8
  %18 = add i8 %17, -4
  store i8 %18, i8* %15, align 1, !tbaa !7
  %19 = bitcast %struct.struct10* %a to i32*
  %20 = load i32, i32* %19, align 4
  ret i32 %20
}

attributes #0 = { nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-features"="+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }

!llvm.ident = !{!0}

!0 = !{!"clang version 3.7.1 (tags/RELEASE_371/final)"}
!1 = !{!2, !3, i64 0}
!2 = !{!"", !3, i64 0, !3, i64 1, !3, i64 2, !3, i64 3}
!3 = !{!"omnipotent char", !4, i64 0}
!4 = !{!"Simple C/C++ TBAA"}
!5 = !{!2, !3, i64 1}
!6 = !{!2, !3, i64 2}
!7 = !{!2, !3, i64 3}

And it appears to be passing the argument by reference but not using sret whereas the logic in abi_win32.cpp seems to be doing neither. Where can I find a complete doc about the win32 calling convention?

@yuyichao
Copy link
Contributor

From the clang output and the ldc code, it seems that the win32 abi use sret for structs that are 1, 2, 4, 8 bytes in size and use pass by reference for all structs.

@yuyichao yuyichao mentioned this pull request Feb 2, 2016
@yuyichao
Copy link
Contributor

yuyichao commented Feb 2, 2016

Subsumed by #14906

@yuyichao yuyichao closed this Feb 2, 2016
@vtjnash vtjnash mentioned this pull request Feb 2, 2016
@maleadt
Copy link
Member Author

maleadt commented Feb 3, 2016

@yuyichao OK, thanks for looking into this.

@maleadt maleadt deleted the pr_ccall_test branch April 20, 2016 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants