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

miscompilation regarding pointer to optional slice #14952

Closed
Tracked by #14647
andrewrk opened this issue Mar 16, 2023 · 0 comments · Fixed by #16598
Closed
Tracked by #14647

miscompilation regarding pointer to optional slice #14952

andrewrk opened this issue Mar 16, 2023 · 0 comments · Fixed by #16598
Labels
backend-llvm The LLVM backend outputs an LLVM IR Module. 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

@andrewrk
Copy link
Member

Zig Version

0.11.0-dev.2148+b3af5d076

Steps to Reproduce and Observed Behavior

Extracted from #14647.

Apply this diff:

diff --git a/lib/std/Build/RunStep.zig b/lib/std/Build/RunStep.zig
index d671ff7ae..30f5ed968 100644
--- a/lib/std/Build/RunStep.zig
+++ b/lib/std/Build/RunStep.zig
@@ -587,8 +587,7 @@ fn runCommand(self: *RunStep, argv: []const []const u8, has_side_effects: bool)
     switch (self.stdio) {
         .check => |checks| for (checks.items) |check| switch (check) {
             .expect_stderr_exact => |expected_bytes| {
-                assert(!result.stderr_null);
-                if (!mem.eql(u8, expected_bytes, result.stderr)) {
+                if (!mem.eql(u8, expected_bytes, result.stderr.?)) {
                     return step.fail(
                         \\
                         \\========= expected this stderr: =========
@@ -599,14 +598,13 @@ fn runCommand(self: *RunStep, argv: []const []const u8, has_side_effects: bool)
                         \\{s}
                     , .{
                         expected_bytes,
-                        result.stderr,
+                        result.stderr.?,
                         try Step.allocPrintCmd(arena, self.cwd, argv),
                     });
                 }
             },
             .expect_stderr_match => |match| {
-                assert(!result.stderr_null);
-                if (mem.indexOf(u8, result.stderr, match) == null) {
+                if (mem.indexOf(u8, result.stderr.?, match) == null) {
                     return step.fail(
                         \\
                         \\========= expected to find in stderr: =========
@@ -617,14 +615,13 @@ fn runCommand(self: *RunStep, argv: []const []const u8, has_side_effects: bool)
                         \\{s}
                     , .{
                         match,
-                        result.stderr,
+                        result.stderr.?,
                         try Step.allocPrintCmd(arena, self.cwd, argv),
                     });
                 }
             },
             .expect_stdout_exact => |expected_bytes| {
-                assert(!result.stdout_null);
-                if (!mem.eql(u8, expected_bytes, result.stdout)) {
+                if (!mem.eql(u8, expected_bytes, result.stdout.?)) {
                     return step.fail(
                         \\
                         \\========= expected this stdout: =========
@@ -635,14 +632,13 @@ fn runCommand(self: *RunStep, argv: []const []const u8, has_side_effects: bool)
                         \\{s}
                     , .{
                         expected_bytes,
-                        result.stdout,
+                        result.stdout.?,
                         try Step.allocPrintCmd(arena, self.cwd, argv),
                     });
                 }
             },
             .expect_stdout_match => |match| {
-                assert(!result.stdout_null);
-                if (mem.indexOf(u8, result.stdout, match) == null) {
+                if (mem.indexOf(u8, result.stdout.?, match) == null) {
                     return step.fail(
                         \\
                         \\========= expected to find in stdout: =========
@@ -653,7 +649,7 @@ fn runCommand(self: *RunStep, argv: []const []const u8, has_side_effects: bool)
                         \\{s}
                     , .{
                         match,
-                        result.stdout,
+                        result.stdout.?,
                         try Step.allocPrintCmd(arena, self.cwd, argv),
                     });
                 }
@@ -675,12 +671,8 @@ fn runCommand(self: *RunStep, argv: []const []const u8, has_side_effects: bool)
 }
 
 const ChildProcResult = struct {
-    // These use boolean flags instead of optionals as a workaround for
-    // https://github.com/ziglang/zig/issues/14783
-    stdout: []const u8,
-    stderr: []const u8,
-    stdout_null: bool,
-    stderr_null: bool,
+    stdout: ?[]const u8,
+    stderr: ?[]const u8,
     term: std.process.Child.Term,
     elapsed_ns: u64,
     peak_rss: usize,
@@ -725,12 +717,8 @@ fn spawnChildAndCollect(
     });
     var timer = try std.time.Timer.start();
 
-    // These are not optionals, as a workaround for
-    // https://github.com/ziglang/zig/issues/14783
-    var stdout_bytes: []const u8 = undefined;
-    var stderr_bytes: []const u8 = undefined;
-    var stdout_null = true;
-    var stderr_null = true;
+    var stdout_bytes: ?[]const u8 = null;
+    var stderr_bytes: ?[]const u8 = null;
 
     if (child.stdout) |stdout| {
         if (child.stderr) |stderr| {
@@ -749,24 +737,22 @@ fn spawnChildAndCollect(
 
             stdout_bytes = try poller.fifo(.stdout).toOwnedSlice();
             stderr_bytes = try poller.fifo(.stderr).toOwnedSlice();
-            stdout_null = false;
-            stderr_null = false;
         } else {
             stdout_bytes = try stdout.reader().readAllAlloc(arena, self.max_stdio_size);
-            stdout_null = false;
         }
     } else if (child.stderr) |stderr| {
         stderr_bytes = try stderr.reader().readAllAlloc(arena, self.max_stdio_size);
-        stderr_null = false;
     }
 
-    if (!stderr_null and stderr_bytes.len > 0) {
-        const stderr_is_diagnostic = switch (self.stdio) {
-            .check => |checks| !checksContainStderr(checks.items),
-            else => true,
-        };
-        if (stderr_is_diagnostic) {
-            try self.step.result_error_msgs.append(arena, stderr_bytes);
+    if (stderr_bytes) |stderr| {
+        if (stderr.len > 0) {
+            const stderr_is_diagnostic = switch (self.stdio) {
+                .check => |checks| !checksContainStderr(checks.items),
+                else => true,
+            };
+            if (stderr_is_diagnostic) {
+                try self.step.result_error_msgs.append(arena, stderr);
+            }
         }
     }
 
@@ -776,8 +762,6 @@ fn spawnChildAndCollect(
     return .{
         .stdout = stdout_bytes,
         .stderr = stderr_bytes,
-        .stdout_null = stdout_null,
-        .stderr_null = stderr_null,
         .term = term,
         .elapsed_ns = elapsed_ns,
         .peak_rss = child.resource_usage_statistics.getMaxRss() orelse 0,

If the diff no longer applies, look for this code:

// These use boolean flags instead of optionals as a workaround for
// https://github.com/ziglang/zig/issues/14783
stdout: []const u8,
stderr: []const u8,
stdout_null: bool,
stderr_null: bool,

zig/lib/std/Build/RunStep.zig

Lines 1117 to 1122 in b4d58e9

// These are not optionals, as a workaround for
// https://github.com/ziglang/zig/issues/14783
var stdout_bytes: []const u8 = undefined;
var stderr_bytes: []const u8 = undefined;
var stdout_null = true;
var stderr_null = true;

Next, run zig build test-behavior and observe the stderr.? safety check trip, even though non-null was assigned to that variable.

I originally reported this as #14783, which has been fixed. But it seems there is another miscompilation.

Expected Behavior

With the diff applied, zig build test should have all passing tests.

@andrewrk andrewrk added 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. backend-llvm The LLVM backend outputs an LLVM IR Module. labels Mar 16, 2023
@andrewrk andrewrk added this to the 0.11.0 milestone Mar 16, 2023
jacobly0 added a commit to jacobly0/zig that referenced this issue Jul 29, 2023
andrewrk pushed a commit that referenced this issue Jul 29, 2023
QusaiHroub pushed a commit to QusaiHroub/zig that referenced this issue Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend-llvm The LLVM backend outputs an LLVM IR Module. 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.

1 participant