hylo-lang / Swifty-LLVM

Swifty interface for the LLVM compiler infrastructure.
Apache License 2.0
28 stars 5 forks source link

Likely LLVM precondition violation #28

Closed dabrahams closed 8 months ago

dabrahams commented 8 months ago

https://github.com/hylo-lang/Swifty-LLVM/blob/291474a13e3304e5ac072b1149b6032600744027/Tests/LLVMTests/Values/Constants/StringConstantTests.swift#L22 causes LLVM built with assertions on to assert:

Assertion failed: Val && "isa<> used on a null pointer", file S:\src\llvm-17.0.6\llvm\include\llvm/Support/Casting.h, line 109

kyouko-taiga commented 8 months ago

That's quite puzzling. This line calls llvm::IntegerType::get and llvm::constant::getNullValue, none of which has any documented precondition.

Could you to split the RHS into two operations to hopefully narrow down the problem?

let t = IntegerType(64, in: &m)
let u = t.zero

One possibility might be that the destructor of m ran before IntegerType.zero is applied, as Swift might have concluded that the useful lifetime of m's value has ended after IntegerType.init. If that's the case, I guess we'd need some sort of API to safely extend the lifetime of a module, at least to write tests.

dabrahams commented 8 months ago

Doing something to make usage reliable only in tests seems a little problematic, because regular normal code will violate the lifetime requirements but that violation won't always show up in testing. We should start with a safe API and then only introduce unsafety driven by performance problems.

dabrahams commented 8 months ago

I started to try to fix this, and have done a lot of work on it, but… what is the reason for not using https://github.com/llvm-swift/LLVMSwift in the end?

dabrahams commented 8 months ago

FYI, adding

    defer {withExtendedLifetime(m) { _ in }}

to that test has no effect. Splitting up the line as you suggested has no effect, but I misreported the line that caused the crash: it's crashing in the final line in a call to LLVMIsConstantString.

dabrahams commented 8 months ago

FYI, you should be able to get a debug build of llvm here in a few hours (except for Windows, which may need a little extra help).

kyouko-taiga commented 8 months ago

but I misreported the line that caused the crash: it's crashing in the final line in a call to LLVMIsConstantString.

~Oh but then it makes much more sense!~ https://llvm.org/doxygen/classllvm_1_1ConstantDataSequential.html#a3486cc0d00c60d90076132d7a1829326

Nevermind, the C bindings don't actually call this method.

dabrahams commented 8 months ago

Stack trace:

Thread 1 Queue : com.apple.main-thread (serial)
#0  0x000000018470e0dc in __pthread_kill ()
#1  0x0000000184745cc0 in pthread_kill ()
#2  0x0000000184651a40 in abort ()
#3  0x0000000184650d30 in __assert_rtn ()
#4  0x00000002846f49e4 in decltype(auto) llvm::cast<llvm::ConstantDataSequential, llvm::Value>(llvm::Value*) at /Users/runner/work/llvm-build/llvm-build/SourceCache/llvm-project/llvm/include/llvm/Support/Casting.h:578
#5  0x00000002846e1ff8 in llvm::ConstantDataSequential* llvm::unwrap<llvm::ConstantDataSequential>(LLVMOpaqueValue*) at /Users/runner/work/llvm-build/llvm-build/SourceCache/llvm-project/llvm/include/llvm/IR/Value.h:1039
#6  0x00000002846e201c in ::LLVMIsConstantString(LLVMValueRef) at /Users/runner/work/llvm-build/llvm-build/SourceCache/llvm-project/llvm/lib/IR/Core.cpp:1483
#7  0x000000028006aa2c in StringConstant.init(_:) at /Users/dave/src/Swifty-LLVM/Sources/SwiftyLLVM/Values/Constants/StringConstant.swift:19
#8  0x0000000280026160 in implicit closure #2 in StringConstantTests.testConversion() ()
#9  0x00000002800261dc in partial apply for implicit closure #2 in StringConstantTests.testConversion() ()
#10 0x0000000100355e74 in closure #1 in XCTAssertNil(_:_:file:line:) ()
#11 0x0000000100355ecc in partial apply for closure #1 in XCTAssertNil(_:_:file:line:) ()
#12 0x0000000100355a08 in partial apply for closure #1 in XCTAssertNil(_:_:file:line:) ()
#13 0x0000000100355378 in closure #1 in _XCTRunThrowableBlock(_:) ()
#14 0x0000000100354d28 in thunk for @callee_guaranteed () -> () ()
#15 0x0000000100354d50 in thunk for @escaping @callee_guaranteed () -> () ()
#16 0x00000001003266c0 in _XCTRunThrowableBlockBridge ()
#17 0x00000001003550d0 in _XCTRunThrowableBlock(_:) ()
#18 0x000000010035573c in XCTAssertNil(_:_:file:line:) ()
#19 0x0000000280025fc0 in StringConstantTests.testConversion() at /Users/dave/src/Swifty-LLVM/Tests/LLVMTests/Values/Constants/StringConstantTests.swift:23
kyouko-taiga commented 8 months ago

This stack trace seems to confirm my suspicion that calling unwrap on an arbitrary LLVMValueRef is wrong, as I mentioned on Slack:

So I strongly suspect that the assertion gets triggered by the call to unwrap in this function:

LLVMBool LLVMIsConstantString(LLVMValueRef C) {
  return unwrap<ConstantDataSequential>(C)->isString();
}

The code that is defining unwrap is beyond my understanding of C++ but I would not be surprised that it eventually runs the assertion check. It seems extremely fishy that we could cast an arbitrary llvm::Value pointer to ConstantDataSequential.

dabrahams commented 8 months ago

If you want to see the LLVM sources in your trace, you can use a variant of this lldb command:

(lldb) settings set target.source-map /Users/runner/work/llvm-build/llvm-build/SourceCache/llvm-project /Users/dave/src/llvm-project

I do this at a breakpoint in my own code; I don't know how to make it look for the llvm sources after it already tried and came up empty.

dabrahams commented 8 months ago

I think I agree with your analysis. The very first thing unwrap does is to call cast, which according to the docs is going to assert if the thing doesn't have the right type. So this C API looks pretty broken to me. Its implementation doesn't match its contract.

It's worth noting that in LLVM's own codebase it is (only) used this way and it's really hard to find any other uses in all of GitHub.

dabrahams commented 8 months ago

https://github.com/llvm/llvm-project/pull/82519