Passing many parameters to fun binding violates ABI

[Papierkorb]

Hi,

While working on the Qt5 bindings, I noticed that some methods taking many strings crashed the process. Upon investigating it appears that passing much data through parameters breaks after a certain amount of total bytes if a previous parameter was of a different size.

The following test-case was modeled after the original causing method: It takes a pointer, and up to three strings following that. In the bindings, this is done through the pointer, and then passing three times a structure of type CrystalString:

# foo.cr
@[Link(ldflags: "#{__DIR__}/foo.c")] # Same result for gcc and clang
lib Bind
  struct CrystalString
    ptr : LibC::Char*
    size : LibC::Int
  end

#   fun bla(eins : CrystalString, zwei : CrystalString, drei : CrystalString, vier : CrystalString) : Void
  fun bla(eins : Void*, zwei : CrystalString, drei : CrystalString, vier : CrystalString) : Void
end

def wrap(str)
  Bind::CrystalString.new(ptr: str.to_unsafe, size: str.size)
end

ptr = Pointer(Void).new(0x1122334455667788)
# Bind.bla(wrap("Eins"), wrap("Zwei"), wrap("Drei"), wrap("Vier"))
Bind.bla(ptr, wrap("Zwei"), wrap("Drei"), wrap("Vier"))

// foo.c
#include <stdio.h>

struct CrystalString {
  char *ptr;
  int size;
};

/* // This one works fine - note that it only passes the same structure type.
void bla(struct CrystalString eins, struct CrystalString zwei, struct CrystalString drei, struct CrystalString vier) {
  printf("eins: %iB @ %p\n", eins.size, eins.ptr);
  printf("zwei: %iB @ %p\n", zwei.size, zwei.ptr);
  printf("drei: %iB @ %p\n", drei.size, drei.ptr);
  printf("vier: %iB @ %p\n", vier.size, vier.ptr);
}
*/

void bla(void *eins, struct CrystalString zwei, struct CrystalString drei, struct CrystalString vier) {
  printf("eins: %p\n", eins); // These output good values
  printf("zwei: %iB @ %p\n", zwei.size, zwei.ptr);
  printf("drei: %iB @ %p\n", drei.size, drei.ptr);
  printf("vier: %iB @ %p\n", vier.size, vier.ptr); // Will output non-sensible values
}

I was surprised to find that indeed, passing only the structure type and not a pointer before it "fixed" the issue. That's why I assume it's a aligning issue, as per disassembly, the C function expects all of these on the stack.

I may dig around a bit tomorrow.

[RX14]

Have you tried using different llvm versions on the compiler?

[Papierkorb]

I haven't. I wrote a C function however calling the target function like in Crystal, and ran that through clang. The interesting point is that Clang unpacks the structure and then passes the first two as separate parameters (Which should end up just like what Crystal does), but the last parameter is not unpacked, but explicitly passed by value.

So my best guess right now is that Crystal not unpacking the structure is fine (as far this issue is concerned), but it has to do another check if the following parameters have to be passed byval explicitly.

With that, it doesn't look like an issue with alignment at all like I first assumed.

Clang

store i8* inttoptr (i64 1234605616436508552 to i8*), i8** %2, align 8
  %6 = bitcast %struct.CrystalString* %3 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %6, i8* bitcast (%struct.CrystalString* @main.zwei to i8*), i64 16, i32 8, i1 false)
  %7 = bitcast %struct.CrystalString* %4 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %7, i8* bitcast (%struct.CrystalString* @main.drei to i8*), i64 16, i32 8, i1 false)
  %8 = bitcast %struct.CrystalString* %5 to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %8, i8* bitcast (%struct.CrystalString* @main.vier to i8*), i64 16, i32 8, i1 false)
  %9 = load i8*, i8** %2, align 8
  %10 = bitcast %struct.CrystalString* %3 to { i8*, i32 }*
  %11 = getelementptr inbounds { i8*, i32 }, { i8*, i32 }* %10, i32 0, i32 0
  %12 = load i8*, i8** %11, align 8
  %13 = getelementptr inbounds { i8*, i32 }, { i8*, i32 }* %10, i32 0, i32 1
  %14 = load i32, i32* %13, align 8
  %15 = bitcast %struct.CrystalString* %4 to { i8*, i32 }*
  %16 = getelementptr inbounds { i8*, i32 }, { i8*, i32 }* %15, i32 0, i32 0
  %17 = load i8*, i8** %16, align 8
  %18 = getelementptr inbounds { i8*, i32 }, { i8*, i32 }* %15, i32 0, i32 1
  %19 = load i32, i32* %18, align 8
  call void @bla(i8* %9, i8* %12, i32 %14, i8* %17, i32 %19, %struct.CrystalString* byval align 8 %5)

Crystal

initialized67:                                    ; preds = %not_initialized68, %initialized65
  %87 = call i8* @"*Pointer(Void)@Pointer(T)::new<Int64>:Pointer(Void)"(i32 642, i64 1234605616436508552), !dbg !77
  store i8* %87, i8** %ptr, !dbg !78
  call void asm "xchg %bx, %bx", ""(), !dbg !78
  %88 = call %"struct.Bind::CrystalString" @"*wrap<String>:struct.Bind::CrystalString"(%String* bitcast ({ i32, i32, i32, [5 x i8] }* @"'Eins'" to %String*)), !dbg !20
  store %"struct.Bind::CrystalString" %88, %"struct.Bind::CrystalString"* %7, !dbg !20
  %89 = bitcast { i64, i64 }* %8 to i8*, !dbg !20
  %90 = getelementptr inbounds %"struct.Bind::CrystalString", %"struct.Bind::CrystalString"* %7, i32 0, i32 0, !dbg !20
  %91 = bitcast i8** %90 to i8*, !dbg !20
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %89, i8* %91, i32 16, i32 8, i1 false), !dbg !20
  %92 = load { i64, i64 }, { i64, i64 }* %8, !dbg !20
  %93 = call %"struct.Bind::CrystalString" @"*wrap<String>:struct.Bind::CrystalString"(%String* bitcast ({ i32, i32, i32, [5 x i8] }* @"'Zwei'" to %String*)), !dbg !21
  store %"struct.Bind::CrystalString" %93, %"struct.Bind::CrystalString"* %9, !dbg !21
  %94 = bitcast { i64, i64 }* %10 to i8*, !dbg !21
  %95 = getelementptr inbounds %"struct.Bind::CrystalString", %"struct.Bind::CrystalString"* %9, i32 0, i32 0, !dbg !21
  %96 = bitcast i8** %95 to i8*, !dbg !21
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %94, i8* %96, i32 16, i32 8, i1 false), !dbg !21
  %97 = load { i64, i64 }, { i64, i64 }* %10, !dbg !21
  %98 = call %"struct.Bind::CrystalString" @"*wrap<String>:struct.Bind::CrystalString"(%String* bitcast ({ i32, i32, i32, [5 x i8] }* @"'Drei'" to %String*)), !dbg !22
  store %"struct.Bind::CrystalString" %98, %"struct.Bind::CrystalString"* %11, !dbg !22
  %99 = bitcast { i64, i64 }* %12 to i8*, !dbg !22
  %100 = getelementptr inbounds %"struct.Bind::CrystalString", %"struct.Bind::CrystalString"* %11, i32 0, i32 0, !dbg !22
  %101 = bitcast i8** %100 to i8*, !dbg !22
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %99, i8* %101, i32 16, i32 8, i1 false), !dbg !22
  %102 = load { i64, i64 }, { i64, i64 }* %12, !dbg !22
  %103 = call %"struct.Bind::CrystalString" @"*wrap<String>:struct.Bind::CrystalString"(%String* bitcast ({ i32, i32, i32, [5 x i8] }* @"'Vier'" to %String*)), !dbg !23
  store %"struct.Bind::CrystalString" %103, %"struct.Bind::CrystalString"* %13, !dbg !23
  %104 = bitcast { i64, i64 }* %14 to i8*, !dbg !23
  %105 = getelementptr inbounds %"struct.Bind::CrystalString", %"struct.Bind::CrystalString"* %13, i32 0, i32 0, !dbg !23
  %106 = bitcast i8** %105 to i8*, !dbg !23
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %104, i8* %106, i32 16, i32 8, i1 false), !dbg !23
  %107 = load { i64, i64 }, { i64, i64 }* %14, !dbg !23
  call void @bla({ i64, i64 } %92, { i64, i64 } %97, { i64, i64 } %102, { i64, i64 } %107), !dbg !79
  ret void, !dbg !79

[asterite]

Unfortunately LLVM knows nothing about the C ABI. Check src/llvm/abi* in this repo. All the code has been copied from Rust. Maybe Rust had this bug too in the past and fixed it. Or maybe it was copied wrong. It should be updated (with a test).

I can confirm this is a bug in the ABI logic in Crystal.

[asterite]

Indeed, it seems after I implemented this (more than 2 years ago) Rust's code had many changes. If anyone wants to re-port this code (seems much more complex than before because Rust's code is now a bit tied with Rust's type system), the relevant file in Rust is https://github.com/rust-lang/rust/blob/master/src/librustc_trans/cabi_x86_64.rs and in Crystal it's https://github.com/crystal-lang/crystal/blob/master/src/llvm/abi/x86_64.cr with specs in https://github.com/crystal-lang/crystal/blob/master/spec/compiler/codegen/c_abi/c_abi_x86_64_spec.cr . Of course other ABIs should be updated too.

[ysbaddaden]

BTW I've always been confused by this need to declare an ABI manually. It sounds exactly like something that LLVM should deal with, and looking into LLVM it seems that the ABI is already declared by the Data Layout string, for which we just reuse what LLVM is telling us...

[asterite]

@ysbaddaden I know. "That's the whole point of using LLVM", I thought. But LLVM doesn't take care of that... just search "llvm c abi" in google and you'll see a bunch of results that clearly show or suggest that LLVM doesn't take care of that. Instead, all that logic is in Clang, and replicated in Rust, Crystal, and I don't know how many other frontends.

Had I known that, I would probably have started with a C backend. That is, compile Crystal to C code. Then using clang or any other C compiler would take care of the C ABI... so much simpler. And maybe even faster, LLVM is a turtle. Nim got it right, they compile to C.

[RX14]

I can't believe that compiling to C would be faster. You have to emit and reparse the C files, plus clang is already the fastest C compiler I believe. I can't think of much other advantages from compiling to C either, apart from the ABI issue, and this is the first ever ABI bug I've seen.

Regardless, there's little point discussing what would be impractical to change.

[oprypin]

Duplicate of #9519

Marking this one because that report is more precise. It has been said before that the issues are related, and now I confirmed that #11344 would fix this one as well.