search

cdisselkoen / llvm-ir

LLVM IR in natural Rust data structures
MIT License
550 stars 46 forks source link

Test failures from LLVM C API accessor issues #44

Open Benjins opened 1 year ago

Benjins commented 1 year ago

I tried running the tests via cargo test --features=llvm-15, and was getting a SIGILL termination. I tracked it down to what seems to be a bug in the LLVM C API, where LLVMGetOrdering does not support Fence instructions. Running a build of LLVM with assertions enabled also led to a few other issues

The main issue: LLVMGetOrdering does not support Fence instructions. It will instead cast it to an unrelated type AtomicRMWInst which will read a garbage value for the memory order (see https://github.com/llvm/llvm-project/blob/77c67436d9e7e63c50752d990a271cf860ba9a0e/llvm/lib/IR/Core.cpp#L3754-L3764 ). The problem is that if this is not a valid enum value, then MemoryOrdering::from_llvm will cause UB since none of the match arms will be hit (LLVMGetOrdering returns the LLVMAtomicOrdering enum): https://github.com/cdisselkoen/llvm-ir/blob/23dac164112b2d384b883657f163631bbdba72b6/src/instruction.rs#L3377-L3380 I believe this is what was what triggered the SIGILL for me. The exact test can be run with cargo test --features=llvm-15 --test llvm_10_tests atomicrmw_binops (the atomics function in tests/llvm_bc/compatibility.ll.bc)

I also found some other, more benign issues with the C API, which trigger assertion failures. I'm mentioning them in case anyone else runs the tests w/ a copy of LLVM that has assertions enabled

  1. LLVMIsAtomicSingleThread does not properly support Fence, Load, or Store instructions (called via SynchronizationScope::from_llvm_ref). This is a similar issue where the C API does not check for those types. It happens to work now because loads/stores, fences, and AtomicCmpXchgInst all define the sync scope ID field at the same offset
  2. LLVMGetNormalDest does not support CallBr instructions (called via CallBr::from_llvm_ref). It casts the value to an InvokeInst. This is technically an unrelated type, but it seems to work out because the methods seem to be pulling the operand from the same place, but I'm not sure if this will work in all cases
  3. LLVMGetThreadLocalMode does an overly-restrictive cast to GlobalVariable, when it should be using GlobalValue. It is called via GlobalAlias::from_llvm_ref. Global aliases are subclasses of GlobalValue, but not GlobalVariable. However this isn't really an issue since it will only access a method that is on GlobalValue, so global aliases work out

I've filed an upstream LLVM issue for the atomic-related issues, since the memory order one is causing problems for me on a normal build: https://github.com/llvm/llvm-project/issues/65227 However, even if this lands promptly it likely won't make it to a release until LLVM 18.x. Not sure what the preferred path is for handling it in the meantime: the UB issue could at least be detected by transmuting LLVMAtomicOrdering to an integer, and checking if it's an expected value. However even then the enum value is probably not going to be correct

Benjins commented 1 year ago

Silly me should have searched a bit harder for a pre-existing issue: looks like it was identified but not fully diagnosed, unless there's other context I'm missing? https://github.com/cdisselkoen/llvm-ir/issues/15

cdisselkoen commented 1 year ago

Yep in #15 and its linked issues we determined the problem is likely in actual LLVM code, which your work here confirms. However, we hadn't traced it as far as you have. Thanks for the detailed investigation and for filing the upstream issue.

Benjins commented 6 months ago

A status update on this: as of LLVM 18, the LLVMGetOrdering and LLVMIsAtomicSingleThread issues have been fixed. However, LLVM 18 also introduced a new issue where uinc_wrap and udec_wrap were added as atomicrmw binary operations, but not exposed in the C API's enum for them. llvm-sys exposes these as a rust enum, so trying to read a bitcode file that has those operations (e.g. in the LLVM 17 compatibility test file) will cause a crash in debug mode. This has been fixed upstream as of https://github.com/llvm/llvm-project/pull/87163, which will land in LLVM 19 Going forward, I may try to build llvm-ir with top-of-tree LLVM a month or so before release to try and catch things like this