From 2e692312f104e1e5533c9f389d73488ea4a1ef68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Motiejus=20Jak=C5=A1tys?= Date: Mon, 23 Jan 2023 21:59:03 +0200 Subject: [PATCH 1/3] zig cc: support reading from non-files echo 'some C program' | $CC -x c - Is a common pattern to test for compiler or linker features. This patch adds support for reading from non-regular files. This will make at least one more Go test to pass. --- lib/std/Build/Cache.zig | 10 +++++ src/Compilation.zig | 20 ++++----- src/main.zig | 37 +++++++++++++++- test/tests.zig | 93 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 12 deletions(-) diff --git a/lib/std/Build/Cache.zig b/lib/std/Build/Cache.zig index 17429c0370..4fc6784684 100644 --- a/lib/std/Build/Cache.zig +++ b/lib/std/Build/Cache.zig @@ -31,6 +31,16 @@ pub const Directory = struct { } } + pub fn tmpFilePath(self: Directory, ally: Allocator, suffix: []const u8) error{OutOfMemory}![]const u8 { + const s = std.fs.path.sep_str; + const rand_int = std.crypto.random.int(u64); + if (self.path) |p| { + return std.fmt.allocPrint(ally, "{s}" ++ s ++ "tmp" ++ s ++ "{x}-{s}", .{ p, rand_int, suffix }); + } else { + return std.fmt.allocPrint(ally, "tmp" ++ s ++ "{x}-{s}", .{ rand_int, suffix }); + } + } + /// Whether or not the handle should be closed, or the path should be freed /// is determined by usage, however this function is provided for convenience /// if it happens to be what the caller needs. diff --git a/src/Compilation.zig b/src/Compilation.zig index 1b6d805bb3..3ae43773e3 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -3981,7 +3981,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P // We can't know the digest until we do the C compiler invocation, // so we need a temporary filename. - const out_obj_path = try comp.tmpFilePath(arena, o_basename); + const out_obj_path = try comp.local_cache_directory.tmpFilePath(arena, o_basename); var zig_cache_tmp_dir = try comp.local_cache_directory.handle.makeOpenPath("tmp", .{}); defer zig_cache_tmp_dir.close(); @@ -4129,16 +4129,6 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P }; } -pub fn tmpFilePath(comp: *Compilation, ally: Allocator, suffix: []const u8) error{OutOfMemory}![]const u8 { - const s = std.fs.path.sep_str; - const rand_int = std.crypto.random.int(u64); - if (comp.local_cache_directory.path) |p| { - return std.fmt.allocPrint(ally, "{s}" ++ s ++ "tmp" ++ s ++ "{x}-{s}", .{ p, rand_int, suffix }); - } else { - return std.fmt.allocPrint(ally, "tmp" ++ s ++ "{x}-{s}", .{ rand_int, suffix }); - } -} - pub fn addTranslateCCArgs( comp: *Compilation, arena: Allocator, @@ -4597,6 +4587,14 @@ pub const FileExt = enum { => false, }; } + + // maximum length of @tagName(ext: FileExt) + pub const max_len = blk: { + var max: u16 = 0; + inline for (std.meta.tags(FileExt)) |ext| + max = std.math.max(@tagName(ext).len, max); + break :blk max; + }; }; pub fn hasObjectExt(filename: []const u8) bool { diff --git a/src/main.zig b/src/main.zig index a680a5d89e..a1f95aaf21 100644 --- a/src/main.zig +++ b/src/main.zig @@ -3020,6 +3020,41 @@ fn buildOutputType( break :l global_cache_directory; }; + var temp_stdin_file: ?[]const u8 = null; + defer { + if (temp_stdin_file) |file| { + // some garbage may stay in the file system if removal fails. + // Alternatively, we could tell the user that the removal failed, + // but it's not as much of a deal: it's a temporary cache directory + // at all. + local_cache_directory.handle.deleteFile(file) catch {}; + } + } + + for (c_source_files.items) |*src| { + if (!mem.eql(u8, src.src_path, "-")) continue; + + const ext = src.ext orelse + fatal("-E or -x is required when reading from a non-regular file", .{}); + + // "-" is stdin. Dump it to a real file. + const new_file = blk: { + var buf: ["stdin.".len + Compilation.FileExt.max_len]u8 = undefined; + const fname = try std.fmt.bufPrint(&buf, "stdin.{s}", .{@tagName(ext)}); + const new_name = try local_cache_directory.tmpFilePath(arena, fname); + + try local_cache_directory.handle.makePath("tmp"); + var outfile = try local_cache_directory.handle.createFile(new_name, .{}); + defer outfile.close(); + errdefer local_cache_directory.handle.deleteFile(new_name) catch {}; + + try outfile.writeFileAll(io.getStdIn(), .{}); + break :blk new_name; + }; + temp_stdin_file = new_file; + src.src_path = new_file; + } + if (build_options.have_llvm and emit_asm != .no) { // LLVM has no way to set this non-globally. const argv = [_][*:0]const u8{ "zig (LLVM option parsing)", "--x86-asm-syntax=intel" }; @@ -3873,7 +3908,7 @@ fn cmdTranslateC(comp: *Compilation, arena: Allocator, fancy_output: ?*Translate const c_src_basename = fs.path.basename(c_source_file.src_path); const dep_basename = try std.fmt.allocPrint(arena, "{s}.d", .{c_src_basename}); - const out_dep_path = try comp.tmpFilePath(arena, dep_basename); + const out_dep_path = try comp.local_cache_directory.tmpFilePath(arena, dep_basename); break :blk out_dep_path; }; diff --git a/test/tests.zig b/test/tests.zig index 641914aabe..26d81ead12 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -777,6 +777,52 @@ pub fn addCliTests(b: *std.Build) *Step { step.dependOn(&cleanup.step); } + // Test `zig cc -x c -` + // Test author was not able to figure out how to start a child process and + // give an fd to it's stdin. fork/exec works, but we are limiting ourselves + // to POSIX. + // + // TODO: the "zig cc <..." step should be a RunStep.create(...) + // However, how do I create a command (RunStep) that invokes a command + // with a specific file descriptor in the stdin? + //if (builtin.os.tag != .windows) { + // const tmp_path = b.makeTempPath(); + // var dir = std.fs.cwd().openDir(tmp_path, .{}) catch @panic("unhandled"); + // dir.writeFile("truth.c", "int main() { return 42; }") catch @panic("unhandled"); + // var infile = dir.openFile("truth.c", .{}) catch @panic("unhandled"); + + // const outfile = std.fs.path.joinZ( + // b.allocator, + // &[_][]const u8{ tmp_path, "truth" }, + // ) catch @panic("unhandled"); + + // const pid_result = std.os.fork() catch @panic("unhandled"); + // if (pid_result == 0) { // child + // std.os.dup2(infile.handle, std.os.STDIN_FILENO) catch @panic("unhandled"); + // const argv = &[_:null]?[*:0]const u8{ + // b.zig_exe, "cc", + // "-o", outfile, + // "-x", "c", + // "-", + // }; + // const envp = &[_:null]?[*:0]const u8{ + // std.fmt.allocPrintZ(b.allocator, "ZIG_GLOBAL_CACHE_DIR={s}", .{tmp_path}) catch @panic("unhandled"), + // }; + // const err = std.os.execveZ(b.zig_exe, argv, envp); + // std.debug.print("execve error: {any}\n", .{err}); + // std.os.exit(1); + // } + + // const res = std.os.waitpid(pid_result, 0); + // assert(0 == res.status); + + // // run the compiled executable and check if it's telling the truth. + // _ = exec(b.allocator, tmp_path, 42, &[_][]const u8{outfile}) catch @panic("unhandled"); + + // const cleanup = b.addRemoveDirTree(tmp_path); + // step.dependOn(&cleanup.step); + //} + { // Test `zig fmt`. // This test must use a temporary directory rather than a cache @@ -1154,3 +1200,50 @@ pub fn addCases( check_case_exe, ); } + +fn exec( + allocator: std.mem.Allocator, + cwd: []const u8, + expect_code: u8, + argv: []const []const u8, +) !std.ChildProcess.ExecResult { + const max_output_size = 100 * 1024; + const result = std.ChildProcess.exec(.{ + .allocator = allocator, + .argv = argv, + .cwd = cwd, + .max_output_bytes = max_output_size, + }) catch |err| { + std.debug.print("The following command failed:\n", .{}); + printCmd(cwd, argv); + return err; + }; + switch (result.term) { + .Exited => |code| { + if (code != expect_code) { + std.debug.print( + "The following command exited with error code {}, expected {}:\n", + .{ code, expect_code }, + ); + printCmd(cwd, argv); + std.debug.print("stderr:\n{s}\n", .{result.stderr}); + return error.CommandFailed; + } + }, + else => { + std.debug.print("The following command terminated unexpectedly:\n", .{}); + printCmd(cwd, argv); + std.debug.print("stderr:\n{s}\n", .{result.stderr}); + return error.CommandFailed; + }, + } + return result; +} + +fn printCmd(cwd: []const u8, argv: []const []const u8) void { + std.debug.print("cd {s} && ", .{cwd}); + for (argv) |arg| { + std.debug.print("{s} ", .{arg}); + } + std.debug.print("\n", .{}); +} From ae119a9a8d5b8dec21bd314e91afcec122eb8631 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 16 May 2023 16:29:20 -0700 Subject: [PATCH 2/3] CLI: fix stdin dumping behavior * no need to move `tmpFilePath` around * no need for calculating max length of `FileExt` tag name * provide a canonical file extension name for `FileExt` so that, e.g. the file will be named `stdin.S` instead of `stdin.assembly_with_cpp`. * move temp file cleanup to a function to reduce defer bloat in a large function. * fix bug caused by mixing relative and absolute paths in the cleanup logic. * remove commented out test and dead code --- lib/std/Build/Cache.zig | 10 ----- src/Compilation.zig | 39 +++++++++++++---- src/main.zig | 54 ++++++++++++++---------- test/tests.zig | 93 ----------------------------------------- 4 files changed, 62 insertions(+), 134 deletions(-) diff --git a/lib/std/Build/Cache.zig b/lib/std/Build/Cache.zig index 4fc6784684..17429c0370 100644 --- a/lib/std/Build/Cache.zig +++ b/lib/std/Build/Cache.zig @@ -31,16 +31,6 @@ pub const Directory = struct { } } - pub fn tmpFilePath(self: Directory, ally: Allocator, suffix: []const u8) error{OutOfMemory}![]const u8 { - const s = std.fs.path.sep_str; - const rand_int = std.crypto.random.int(u64); - if (self.path) |p| { - return std.fmt.allocPrint(ally, "{s}" ++ s ++ "tmp" ++ s ++ "{x}-{s}", .{ p, rand_int, suffix }); - } else { - return std.fmt.allocPrint(ally, "tmp" ++ s ++ "{x}-{s}", .{ rand_int, suffix }); - } - } - /// Whether or not the handle should be closed, or the path should be freed /// is determined by usage, however this function is provided for convenience /// if it happens to be what the caller needs. diff --git a/src/Compilation.zig b/src/Compilation.zig index 3ae43773e3..0818eaafdd 100644 --- a/src/Compilation.zig +++ b/src/Compilation.zig @@ -3981,7 +3981,7 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P // We can't know the digest until we do the C compiler invocation, // so we need a temporary filename. - const out_obj_path = try comp.local_cache_directory.tmpFilePath(arena, o_basename); + const out_obj_path = try comp.tmpFilePath(arena, o_basename); var zig_cache_tmp_dir = try comp.local_cache_directory.handle.makeOpenPath("tmp", .{}); defer zig_cache_tmp_dir.close(); @@ -4129,6 +4129,16 @@ fn updateCObject(comp: *Compilation, c_object: *CObject, c_obj_prog_node: *std.P }; } +pub fn tmpFilePath(comp: *Compilation, ally: Allocator, suffix: []const u8) error{OutOfMemory}![]const u8 { + const s = std.fs.path.sep_str; + const rand_int = std.crypto.random.int(u64); + if (comp.local_cache_directory.path) |p| { + return std.fmt.allocPrint(ally, "{s}" ++ s ++ "tmp" ++ s ++ "{x}-{s}", .{ p, rand_int, suffix }); + } else { + return std.fmt.allocPrint(ally, "tmp" ++ s ++ "{x}-{s}", .{ rand_int, suffix }); + } +} + pub fn addTranslateCCArgs( comp: *Compilation, arena: Allocator, @@ -4588,13 +4598,26 @@ pub const FileExt = enum { }; } - // maximum length of @tagName(ext: FileExt) - pub const max_len = blk: { - var max: u16 = 0; - inline for (std.meta.tags(FileExt)) |ext| - max = std.math.max(@tagName(ext).len, max); - break :blk max; - }; + pub fn canonicalName(ext: FileExt, target: Target) [:0]const u8 { + return switch (ext) { + .c => ".c", + .cpp => ".cpp", + .cu => ".cu", + .h => ".h", + .m => ".m", + .mm => ".mm", + .ll => ".ll", + .bc => ".bc", + .assembly => ".s", + .assembly_with_cpp => ".S", + .shared_library => target.dynamicLibSuffix(), + .object => target.ofmt.fileExt(target.cpu.arch), + .static_library => target.staticLibSuffix(), + .zig => ".zig", + .def => ".def", + .unknown => "", + }; + } }; pub fn hasObjectExt(filename: []const u8) bool { diff --git a/src/main.zig b/src/main.zig index a1f95aaf21..46dbf1de1e 100644 --- a/src/main.zig +++ b/src/main.zig @@ -705,6 +705,17 @@ const ArgsIterator = struct { } }; +fn cleanupTempStdinFile( + temp_stdin_file: ?[]const u8, + local_cache_directory: Compilation.Directory, +) void { + if (temp_stdin_file) |file| { + // Some garbage may stay in the file system if removal fails; this + // is harmless so no warning is needed. + local_cache_directory.handle.deleteFile(file) catch {}; + } +} + fn buildOutputType( gpa: Allocator, arena: Allocator, @@ -3021,15 +3032,11 @@ fn buildOutputType( }; var temp_stdin_file: ?[]const u8 = null; - defer { - if (temp_stdin_file) |file| { - // some garbage may stay in the file system if removal fails. - // Alternatively, we could tell the user that the removal failed, - // but it's not as much of a deal: it's a temporary cache directory - // at all. - local_cache_directory.handle.deleteFile(file) catch {}; - } - } + // Note that in one of the happy paths, execve() is used to switch to clang + // in which case this cleanup logic does not run and this temp file is + // leaked. Oh well. It's a minor punishment for using `-x c` which nobody + // should be doing. + defer cleanupTempStdinFile(temp_stdin_file, local_cache_directory); for (c_source_files.items) |*src| { if (!mem.eql(u8, src.src_path, "-")) continue; @@ -3038,21 +3045,22 @@ fn buildOutputType( fatal("-E or -x is required when reading from a non-regular file", .{}); // "-" is stdin. Dump it to a real file. - const new_file = blk: { - var buf: ["stdin.".len + Compilation.FileExt.max_len]u8 = undefined; - const fname = try std.fmt.bufPrint(&buf, "stdin.{s}", .{@tagName(ext)}); - const new_name = try local_cache_directory.tmpFilePath(arena, fname); - + const sub_path = blk: { + const sep = fs.path.sep_str; + const sub_path = try std.fmt.allocPrint(arena, "tmp" ++ sep ++ "{x}-stdin{s}", .{ + std.crypto.random.int(u64), ext.canonicalName(target_info.target), + }); try local_cache_directory.handle.makePath("tmp"); - var outfile = try local_cache_directory.handle.createFile(new_name, .{}); - defer outfile.close(); - errdefer local_cache_directory.handle.deleteFile(new_name) catch {}; - - try outfile.writeFileAll(io.getStdIn(), .{}); - break :blk new_name; + var f = try local_cache_directory.handle.createFile(sub_path, .{}); + defer f.close(); + errdefer local_cache_directory.handle.deleteFile(sub_path) catch {}; + try f.writeFileAll(io.getStdIn(), .{}); + break :blk sub_path; }; - temp_stdin_file = new_file; - src.src_path = new_file; + // Relative to `local_cache_directory`. + temp_stdin_file = sub_path; + // Relative to current working directory. + src.src_path = try local_cache_directory.join(arena, &.{sub_path}); } if (build_options.have_llvm and emit_asm != .no) { @@ -3908,7 +3916,7 @@ fn cmdTranslateC(comp: *Compilation, arena: Allocator, fancy_output: ?*Translate const c_src_basename = fs.path.basename(c_source_file.src_path); const dep_basename = try std.fmt.allocPrint(arena, "{s}.d", .{c_src_basename}); - const out_dep_path = try comp.local_cache_directory.tmpFilePath(arena, dep_basename); + const out_dep_path = try comp.tmpFilePath(arena, dep_basename); break :blk out_dep_path; }; diff --git a/test/tests.zig b/test/tests.zig index 26d81ead12..641914aabe 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -777,52 +777,6 @@ pub fn addCliTests(b: *std.Build) *Step { step.dependOn(&cleanup.step); } - // Test `zig cc -x c -` - // Test author was not able to figure out how to start a child process and - // give an fd to it's stdin. fork/exec works, but we are limiting ourselves - // to POSIX. - // - // TODO: the "zig cc <..." step should be a RunStep.create(...) - // However, how do I create a command (RunStep) that invokes a command - // with a specific file descriptor in the stdin? - //if (builtin.os.tag != .windows) { - // const tmp_path = b.makeTempPath(); - // var dir = std.fs.cwd().openDir(tmp_path, .{}) catch @panic("unhandled"); - // dir.writeFile("truth.c", "int main() { return 42; }") catch @panic("unhandled"); - // var infile = dir.openFile("truth.c", .{}) catch @panic("unhandled"); - - // const outfile = std.fs.path.joinZ( - // b.allocator, - // &[_][]const u8{ tmp_path, "truth" }, - // ) catch @panic("unhandled"); - - // const pid_result = std.os.fork() catch @panic("unhandled"); - // if (pid_result == 0) { // child - // std.os.dup2(infile.handle, std.os.STDIN_FILENO) catch @panic("unhandled"); - // const argv = &[_:null]?[*:0]const u8{ - // b.zig_exe, "cc", - // "-o", outfile, - // "-x", "c", - // "-", - // }; - // const envp = &[_:null]?[*:0]const u8{ - // std.fmt.allocPrintZ(b.allocator, "ZIG_GLOBAL_CACHE_DIR={s}", .{tmp_path}) catch @panic("unhandled"), - // }; - // const err = std.os.execveZ(b.zig_exe, argv, envp); - // std.debug.print("execve error: {any}\n", .{err}); - // std.os.exit(1); - // } - - // const res = std.os.waitpid(pid_result, 0); - // assert(0 == res.status); - - // // run the compiled executable and check if it's telling the truth. - // _ = exec(b.allocator, tmp_path, 42, &[_][]const u8{outfile}) catch @panic("unhandled"); - - // const cleanup = b.addRemoveDirTree(tmp_path); - // step.dependOn(&cleanup.step); - //} - { // Test `zig fmt`. // This test must use a temporary directory rather than a cache @@ -1200,50 +1154,3 @@ pub fn addCases( check_case_exe, ); } - -fn exec( - allocator: std.mem.Allocator, - cwd: []const u8, - expect_code: u8, - argv: []const []const u8, -) !std.ChildProcess.ExecResult { - const max_output_size = 100 * 1024; - const result = std.ChildProcess.exec(.{ - .allocator = allocator, - .argv = argv, - .cwd = cwd, - .max_output_bytes = max_output_size, - }) catch |err| { - std.debug.print("The following command failed:\n", .{}); - printCmd(cwd, argv); - return err; - }; - switch (result.term) { - .Exited => |code| { - if (code != expect_code) { - std.debug.print( - "The following command exited with error code {}, expected {}:\n", - .{ code, expect_code }, - ); - printCmd(cwd, argv); - std.debug.print("stderr:\n{s}\n", .{result.stderr}); - return error.CommandFailed; - } - }, - else => { - std.debug.print("The following command terminated unexpectedly:\n", .{}); - printCmd(cwd, argv); - std.debug.print("stderr:\n{s}\n", .{result.stderr}); - return error.CommandFailed; - }, - } - return result; -} - -fn printCmd(cwd: []const u8, argv: []const []const u8) void { - std.debug.print("cd {s} && ", .{cwd}); - for (argv) |arg| { - std.debug.print("{s} ", .{arg}); - } - std.debug.print("\n", .{}); -} From 233a0d399135cdb581c5c080726c4cacc71695fe Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 16 May 2023 16:37:07 -0700 Subject: [PATCH 3/3] CLI: remove cleanup logic for stdin temp file In one of the happy paths, execve() is used to switch to clang in which case any cleanup logic that exists for this temporary file will not run and this temp file will be leaked. Oh well. It's a minor punishment for using `-x c` which nobody should be doing. Therefore, we make no effort to clean up. Using `-` for stdin as a source file always leaks a temp file. Note that the standard `zig build-exe` CLI does not support stdin as an input file. This is only for `zig cc` C compiler compatibility. --- src/main.zig | 49 ++++++++++++++++--------------------------------- 1 file changed, 16 insertions(+), 33 deletions(-) diff --git a/src/main.zig b/src/main.zig index 46dbf1de1e..2fd314def8 100644 --- a/src/main.zig +++ b/src/main.zig @@ -705,17 +705,6 @@ const ArgsIterator = struct { } }; -fn cleanupTempStdinFile( - temp_stdin_file: ?[]const u8, - local_cache_directory: Compilation.Directory, -) void { - if (temp_stdin_file) |file| { - // Some garbage may stay in the file system if removal fails; this - // is harmless so no warning is needed. - local_cache_directory.handle.deleteFile(file) catch {}; - } -} - fn buildOutputType( gpa: Allocator, arena: Allocator, @@ -3031,13 +3020,6 @@ fn buildOutputType( break :l global_cache_directory; }; - var temp_stdin_file: ?[]const u8 = null; - // Note that in one of the happy paths, execve() is used to switch to clang - // in which case this cleanup logic does not run and this temp file is - // leaked. Oh well. It's a minor punishment for using `-x c` which nobody - // should be doing. - defer cleanupTempStdinFile(temp_stdin_file, local_cache_directory); - for (c_source_files.items) |*src| { if (!mem.eql(u8, src.src_path, "-")) continue; @@ -3045,21 +3027,22 @@ fn buildOutputType( fatal("-E or -x is required when reading from a non-regular file", .{}); // "-" is stdin. Dump it to a real file. - const sub_path = blk: { - const sep = fs.path.sep_str; - const sub_path = try std.fmt.allocPrint(arena, "tmp" ++ sep ++ "{x}-stdin{s}", .{ - std.crypto.random.int(u64), ext.canonicalName(target_info.target), - }); - try local_cache_directory.handle.makePath("tmp"); - var f = try local_cache_directory.handle.createFile(sub_path, .{}); - defer f.close(); - errdefer local_cache_directory.handle.deleteFile(sub_path) catch {}; - try f.writeFileAll(io.getStdIn(), .{}); - break :blk sub_path; - }; - // Relative to `local_cache_directory`. - temp_stdin_file = sub_path; - // Relative to current working directory. + const sep = fs.path.sep_str; + const sub_path = try std.fmt.allocPrint(arena, "tmp" ++ sep ++ "{x}-stdin{s}", .{ + std.crypto.random.int(u64), ext.canonicalName(target_info.target), + }); + try local_cache_directory.handle.makePath("tmp"); + // Note that in one of the happy paths, execve() is used to switch + // to clang in which case any cleanup logic that exists for this + // temporary file will not run and this temp file will be leaked. + // Oh well. It's a minor punishment for using `-x c` which nobody + // should be doing. Therefore, we make no effort to clean up. Using + // `-` for stdin as a source file always leaks a temp file. + var f = try local_cache_directory.handle.createFile(sub_path, .{}); + defer f.close(); + try f.writeFileAll(io.getStdIn(), .{}); + + // Convert `sub_path` to be relative to current working directory. src.src_path = try local_cache_directory.join(arena, &.{sub_path}); }