Skip to content

Fix: incorrect integer extension for Unsigned in pointerref and pointerset. - #583

Draft
jack-dunham wants to merge 1 commit into
JuliaLLVM:mainfrom
jack-dunham:unsigned-index
Draft

Fix: incorrect integer extension for Unsigned in pointerref and pointerset.#583
jack-dunham wants to merge 1 commit into
JuliaLLVM:mainfrom
jack-dunham:unsigned-index

Conversation

@jack-dunham

Copy link
Copy Markdown

LLVM seems to assume signed integers by default in its getelementptr instruction which (I assume) means Julia employs sext when generating the LLVM IR. However, if the index is an Unsigned that is smaller than the index width of the target, this gets interpreted as Signed and incorrectly extended using sext instead of zext. This can lead to incorrect memory accesses or segmentation faults. See:

using LLVM, InteractiveUtils
using Core: LLVMPtr

load_u32(p::LLVMPtr{Int8,0}, i::UInt32) = unsafe_load(p, i)
load_i64(p::LLVMPtr{Int8,0}, i::Int64)  = unsafe_load(p, i)

# ---- 1. the generated IR ----------------------------------------------------
println("--- unsafe_load(p, i::UInt32) ---")
@code_llvm debuginfo=:none load_u32(LLVMPtr{Int8,0}(0), UInt32(1))
println("--- unsafe_load(p, i::Int64) ----")
@code_llvm debuginfo=:none load_i64(LLVMPtr{Int8,0}(0), Int64(1))

# ---- 2. an actual wrong load ------------------------------------------------
# 4 GiB buffer; put the pointer in the middle so that BOTH the correct (+2^31)
# and the sign-extended (-2^31) offsets land inside valid memory.
buf = fill(Int8(0), 2^32 + 8)
buf[1]        = Int8(11)      # what a sign-extended index will hit
buf[2^32 + 1] = Int8(22)      # what the correct index should hit

GC.@preserve buf begin
    mid = reinterpret(LLVMPtr{Int8,0}, pointer(buf) + 2^31)
    i = 2^31 + 1              # i-1 == 2^31, high bit of the i32 is set
    println("\ni          = ", i)
    println("Int64  index -> ", load_i64(mid, Int64(i)),  "  (expected 22)")
    println("UInt32 index -> ", load_u32(mid, UInt32(i)), "  (expected 22)")
end

which outputs:

--- unsafe_load(p, i::UInt32) ---
; Function Signature: load_u32(Core.LLVMPtr{Int8, 0}, UInt32)
define i8 @julia_load_u32_1117(ptr %"p::LLVMPtr", i32 zeroext %"i::UInt32") #0 {
top:
  %0 = add i32 %"i::UInt32", -1
  %1 = sext i32 %0 to i64
  %2 = getelementptr inbounds i8, ptr %"p::LLVMPtr", i64 %1
  %3 = load i8, ptr %2, align 1
  ret i8 %3
}
--- unsafe_load(p, i::Int64) ----
; Function Signature: load_i64(Core.LLVMPtr{Int8, 0}, Int64)
define i8 @julia_load_i64_1128(ptr %"p::LLVMPtr", i64 signext %"i::Int64") #0 {
top:
  %0 = getelementptr i8, ptr %"p::LLVMPtr", i64 %"i::Int64"
  %1 = getelementptr i8, ptr %0, i64 -1
  %2 = load i8, ptr %1, align 1
  ret i8 %2
}

i          = 2147483649
Int64  index -> 22  (expected 22)
UInt32 index -> 11  (expected 22)

The UInt32 gets interpreted as a i32 and then extended to i64 via sext. This gives a wrong address whenever the index is between typemax(Int32) and typemax(UInt32).

This PR promotes any index to Int64 which alters the LLVM IR for Unsigned to first use zext before computing the address:

--- unsafe_load(p, i::UInt32) ---
; Function Signature: load_u32(Core.LLVMPtr{Int8, 0}, UInt32)
define i8 @julia_load_u32_1202(ptr %"p::LLVMPtr", i32 zeroext %"i::UInt32") #0 {
top:
  %0 = zext i32 %"i::UInt32" to i64
  %1 = getelementptr i8, ptr %"p::LLVMPtr", i64 %0
  %2 = getelementptr i8, ptr %1, i64 -1
  %3 = load i8, ptr %2, align 1
  ret i8 %3
}
--- unsafe_load(p, i::Int64) ----
; Function Signature: load_i64(Core.LLVMPtr{Int8, 0}, Int64)
define i8 @julia_load_i64_1211(ptr %"p::LLVMPtr", i64 signext %"i::Int64") #0 {
top:
  %0 = getelementptr i8, ptr %"p::LLVMPtr", i64 %"i::Int64"
  %1 = getelementptr i8, ptr %0, i64 -1
  %2 = load i8, ptr %1, align 1
  ret i8 %2
}

i          = 2147483649
Int64  index -> 22  (expected 22)
UInt32 index -> 22  (expected 22)

The LLVM IR generated for Signed is identical and for index widths of 32bits I believe LLVM should optimize the extension followed by truncation to a no-op.

Tests to follow so leaving this as a draft, but this should fix JuliaGPU/CUDA.jl#3220.

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

Successfully merging this pull request may close these issues.

Illegal memory access in permutedims! on matrices with typemax(Int32) <= N <= typemax(UInt32) elements.

1 participant