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

Stage2 miscompiles call to std.math.big.int.Managed.shiftLeft #11897

Closed
Tracked by #11899 ...
Vexu opened this issue Jun 20, 2022 · 0 comments · Fixed by #11964
Closed
Tracked by #11899 ...

Stage2 miscompiles call to std.math.big.int.Managed.shiftLeft #11897

Vexu opened this issue Jun 20, 2022 · 0 comments · Fixed by #11964
Assignees
Labels
bug Observed behavior contradicts documented or intended behavior frontend Tokenization, parsing, AstGen, Sema, and Liveness. miscompilation The compiler reports success but produces semantically incorrect code.
Milestone

Comments

@Vexu
Copy link
Member

Vexu commented Jun 20, 2022

test {
    var p = try std.math.big.int.Managed.initSet(testing.allocator, 1);
    defer p.deinit();
    var a2 = try p.clone();
    defer a2.deinit();

    var shift: isize = 54;
    try a2.shiftLeft(a2, @intCast(usize, shift)); // segfault

    // var shift2 = @intCast(usize, shift);
    // try a2.shiftLeft(a2, shift2); // works as expected
}

The biggest difference between the two seems to be that the in the second one the a2 passed by value is copied first but the doc comments on shiftLeft says that aliasing here should be okay:

diff --git a/foo.ll b/foo.ll
index 7368ad2c8..29f41a7d7 100644
--- a/foo.ll
+++ b/foo.ll
@@ -1,7 +1,7 @@
 ; Function Attrs: nounwind
 define internal fastcc i16 @a.test_0(%builtin.StackTrace* nonnull %0) unnamed_addr #0 {
 Entry:
-  %1 = alloca i64, align 8
+  %1 = alloca %math.big.int.Managed, align 8
   %2 = alloca i64, align 8
   %3 = alloca { %math.big.int.Managed, i16 }, align 8
   %4 = alloca %math.big.int.Managed, align 8
@@ -46,9 +46,12 @@ TryCont2:                                         ; preds = %TryCont
   call void @llvm.dbg.declare(metadata %math.big.int.Managed* %4, metadata !4856, metadata !DIExpression()),
   store i64 54, i64* %2, align 8,
   call void @llvm.dbg.declare(metadata i64* %2, metadata !4857, metadata !DIExpression()),
-  %23 = load i64, i64* %2, align 8,
-  %24 = icmp sge i64 %23, 0,
-  br i1 %24, label %Then, label %Else,
+  %23 = bitcast %math.big.int.Managed* %1 to i8*,
+  %24 = bitcast %math.big.int.Managed* %4 to i8*,
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %23, i8* align 8 %24, i64 40, i1 false),
+  %25 = load i64, i64* %2, align 8,
+  %26 = icmp sge i64 %25, 0,
+  br i1 %26, label %Then, label %Else,
 
 Then:                                             ; preds = %TryCont2
   br label %Block,
@@ -58,18 +61,15 @@ Else:                                             ; preds = %TryCont2
   unreachable,
 
 Block:                                            ; preds = %Then
-  store i64 %23, i64* %1, align 8,
-  call void @llvm.dbg.declare(metadata i64* %1, metadata !4859, metadata !DIExpression()),
-  %25 = load i64, i64* %1, align 8,
-  %26 = call fastcc i16 @math.big.int.Managed.shiftLeft(%builtin.StackTrace* %0, %math.big.int.Managed* %4, %math.big.int.Managed* %4, i64 %25),
-  %27 = icmp ne i16 %26, 0,
-  br i1 %27, label %TryRet3, label %TryCont4,
+  %27 = call fastcc i16 @math.big.int.Managed.shiftLeft(%builtin.StackTrace* %0, %math.big.int.Managed* %4, %math.big.int.Managed* %1, i64 %25),
+  %28 = icmp ne i16 %27, 0,
+  br i1 %28, label %TryRet3, label %TryCont4,
 
 TryRet3:                                          ; preds = %Block
   call fastcc void @math.big.int.Managed.deinit(%math.big.int.Managed* %4),

This is blocking std.math.big.Rational.toFloat tests.

@Vexu Vexu added frontend Tokenization, parsing, AstGen, Sema, and Liveness. miscompilation The compiler reports success but produces semantically incorrect code. backend-llvm The LLVM backend outputs an LLVM IR Module. labels Jun 20, 2022
@Vexu Vexu added this to the 0.10.0 milestone Jun 20, 2022
@Vexu Vexu added the bug Observed behavior contradicts documented or intended behavior label Jun 20, 2022
@Vexu Vexu mentioned this issue Jun 20, 2022
7 tasks
andrewrk added a commit that referenced this issue Jun 29, 2022
Many of the Managed methods accepted by-val parameters which could
reference Limb slices that became invalid memory after any
ensureCapacity calls. Now, Managed methods accept `*const Managed`
parameters so that if the function allows aliasing and the
ensure-capacity call resizes the Limb slice, it also affects the
aliased parameters, avoiding use-after-free bugs.

This is a breaking change that reduces the requirement for callsites to
manually make the ensure-capacity changes prior to calling many of the
Managed methods.

Closes #11897
@andrewrk andrewrk self-assigned this Jun 29, 2022
@andrewrk andrewrk removed the backend-llvm The LLVM backend outputs an LLVM IR Module. label Jun 29, 2022
andrewrk added a commit that referenced this issue Jun 30, 2022
Many of the Managed methods accepted by-val parameters which could
reference Limb slices that became invalid memory after any
ensureCapacity calls. Now, Managed methods accept `*const Managed`
parameters so that if the function allows aliasing and the
ensure-capacity call resizes the Limb slice, it also affects the
aliased parameters, avoiding use-after-free bugs.

This is a breaking change that reduces the requirement for callsites to
manually make the ensure-capacity changes prior to calling many of the
Managed methods.

Closes #11897
andrewrk added a commit that referenced this issue Jul 19, 2022
Many of the Managed methods accepted by-val parameters which could
reference Limb slices that became invalid memory after any
ensureCapacity calls. Now, Managed methods accept `*const Managed`
parameters so that if the function allows aliasing and the
ensure-capacity call resizes the Limb slice, it also affects the
aliased parameters, avoiding use-after-free bugs.

This is a breaking change that reduces the requirement for callsites to
manually make the ensure-capacity changes prior to calling many of the
Managed methods.

Closes #11897
wooster0 pushed a commit to wooster0/zig that referenced this issue Jul 24, 2022
Many of the Managed methods accepted by-val parameters which could
reference Limb slices that became invalid memory after any
ensureCapacity calls. Now, Managed methods accept `*const Managed`
parameters so that if the function allows aliasing and the
ensure-capacity call resizes the Limb slice, it also affects the
aliased parameters, avoiding use-after-free bugs.

This is a breaking change that reduces the requirement for callsites to
manually make the ensure-capacity changes prior to calling many of the
Managed methods.

Closes ziglang#11897
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior frontend Tokenization, parsing, AstGen, Sema, and Liveness. miscompilation The compiler reports success but produces semantically incorrect code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants