zig cc: support reading from stdin via "-x LANG -" #14462

motiejus · 2023-01-26T03:55:31Z

echo 'some C program' | $CC -x c -

Is a common pattern to test for compiler or linker features. This PR adds support for reading from non-regular files.

This will make at least one more Go test to pass.

motiejus · 2023-01-26T12:54:52Z

~~The build fails like this:~~

: && /home/ci/deps/zig+llvm+lld+clang-x86_64-linux-musl-0.11.0-dev.971+19056cb68/bin/zig  cc -target x86_64-linux-musl -mcpu=baseline -g -rdynamic CMakeFiles/zig1.dir/zig1.c.o CMakeFiles/zig1.dir/stage1/wasi.c.o -o zig1  -lm && :
ld.lld: error: undefined symbol: wasi_snapshot_preview1_fd_seek
>>> referenced by zig1.c:31166

~~However, I can see this in wasm.zig:~~

pub extern "wasi_snapshot_preview1" fn fd_seek(fd: fd_t, offset: filedelta_t, whence: whence_t, newoffset: *filesize_t) errno_t;

Edit: fixed.

src/main.zig

test/cli.zig

andrewrk · 2023-02-01T21:11:47Z

Have you considered the alternate implementation using clang_passthrough_mode instead of buffering the input to a temporary file?

I don't want to support the use case of stdin as a root source file in any case other than C/C++ compiler compatibility mode.

motiejus · 2023-02-02T00:02:39Z

Have you considered the alternate implementation using clang_passthrough_mode instead of buffering the input to a temporary file?

I ruled it out for a reason I can no longer remember.

This is how I understand it: if the input file is -, set clang_passthrough_mode = true for the zig clang command (which will cause it to read from stdin), and then do some dancing with the filenames from the subsequent steps like linking.

I will look at this again.

I don't want to support the use case of stdin as a root source file in any case other than C/C++ compiler compatibility mode.

I agree.

motiejus · 2023-02-02T06:20:59Z

I started poking at this and remember better now. I did indeed consider the clang passthrough mode; we had this discussion before (around Oct 2022) and you advised me to slurp the file and pass it on to the remaining compilation chain as a regular file. :)

Here was my argument: everything in src/main.zig, src/Compilation.zig and src/Cache.zig assume that all the input files are files. So we have 2 choices:

add special handling for stdin in those 3 files, or
slurp stdin and keep assuming everything is a file. Like in this PR.

For a taste on how many things will need to change to adjust this, here is a start:

diff --git a/src/Compilation.zig b/src/Compilation.zig
index 7d42d3b61..8b351270e 100644
--- a/src/Compilation.zig
+++ b/src/Compilation.zig
@@ -210,7 +210,10 @@ pub const LangToExt = std.ComptimeStringMap(FileExt, .{
 
 /// For passing to a C compiler.
 pub const CSourceFile = struct {
-    src_path: []const u8,
+    src: union(enum) {
+        stdin,
+        path: []const u8,
+    },
     extra_flags: []const []const u8 = &.{},
     /// Same as extra_flags except they are not added to the Cache hash.
     cache_exempt_flags: []const []const u8 = &.{},

There are many places in code that will change as a result of this. I can do it, but:

it will become more complex due to this single case.
we will lose caching for all compilations that arrive over stdin. This may or may not be a good thing.

Let me know.

motiejus · 2023-02-02T15:49:03Z

I can reduce the surface area of this feature this way: allow only - as a non-file entry; all other non-files except - would be not allowed.

This is useful for tests that want to `execve` zig directly. The string is already null-terminated, so this will just expose it as such, removing an extra allocation from the test. Will be used in ziglang#14462

motiejus · 2023-03-17T13:42:25Z

Rebased, rewrote the test for the "build system v2".

Also, simplified the change to only account for - as stdin. No other streams will be accepted.

This is useful for tests that want to `execve` zig directly. The string is already null-terminated, so this will just expose it as such, removing an extra allocation from the test. Will be used in #14462

This is useful for tests that want to `execve` zig directly. The string is already null-terminated, so this will just expose it as such, removing an extra allocation from the test. Will be used in ziglang#14462

andrewrk

Thanks for adding this. I have some requested changes. Let me know if you want any help with them.

lib/std/Build/Cache.zig

src/main.zig

andrewrk · 2023-04-07T21:58:47Z

src/main.zig

+    // if a c_source_file is "-", dump it to a tempdir and replace the src_name
+    // with the newly created file. translate-c works fine with streams; other
+    // actions don't.
+    if (arg_mode != .translate_c) {


Where do you get the idea that translate-c handles - arguments? I don't see that.

It does handle /dev/stdin well without the intermediate files.

The previous commits have support for any streams (so /dev/stdin would work with anything), but later I decided to drop support for everything but -. That simplifies things a lot.

This comment is a left-over from "those times".

andrewrk · 2023-04-07T22:02:06Z

test/tests.zig

+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", .{});
+}


this should not be added

Which section(s) are you referring to? It's not quite clear from the comment.

andrewrk · 2023-04-07T22:04:04Z

test/tests.zig

+    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);
+    }


This test as is does the following things directly in the configure phase rather than using build steps like it must do in order to play nicely with the other build steps:

making a temp dir and writing a file

calling fork, execve, and wait

calling exec

If you look at the Test Godbolt API code block above, it shows how to use the build system API for testing. It will be a combination of WriteFileStep, and RunStep.

I'm happy to provide assistance in writing this test.

I originally did everything in the configure phase, because I couldn't find how to execute a command and change it's stdin file descriptor.

After rebasing this test breaks zig build altogether, so I commented the full test.

I would really appreciate help here. Not asking to write the full code, rather a sequence of how Step , RunStep should be structured, keeping in mind the "need to pass this fd to stdin, which was acquired earlier by opening that temporary file".

motiejus · 2023-04-20T11:38:49Z

Note to myself: handle this case: #15338 (comment)

motiejus · 2023-04-25T09:40:45Z

I have addressed all the review comments for the code, but not for the tests. Need some help there.

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.

* 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

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.

andrewrk · 2023-05-16T23:49:37Z

Alright, I have made some executive decisions and finished this patch. The final diff ended up being quite small and tidy.

I removed the dead code added to tests.zig; adding test coverage for this CLI usage can be a separate enhancement. Here is a clue should you wish to pursue this: you'll want to have a Run step with the stdin field populated with your C source code, and the stdio field set to check with some checks against stderr or stdout. If you want to run the created binary, you can use addOutputArg on the Run step, and use that with a second Run step with addFileSourceArg.

I have tested this locally and force-pushed to your branch, with auto-merge enabled.

motiejus · 2023-05-17T08:05:54Z

src/main.zig

+        // 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.


we could minimize the damage by:

hashing the file contents (while writing to the file, so no additional allocations)

renaming the filename to the hash.

That way, we avoid a small chance of hash collision (I know) and multiple invocations of echo | zig cc <...> produce only one garbage-file.

Thoughts?

OK, sounds reasonable

#15770 and #15772

motiejus force-pushed the infile_stdin branch from c872d08 to 9de2ef5 Compare January 26, 2023 18:31

matu3ba reviewed Jan 27, 2023

View reviewed changes

src/main.zig Outdated Show resolved Hide resolved

test/cli.zig Outdated Show resolved Hide resolved

motiejus closed this Feb 1, 2023

motiejus reopened this Feb 2, 2023

motiejus mentioned this pull request Mar 17, 2023

Build.zig_exe: make it sentinel-aware #14985

Merged

motiejus force-pushed the infile_stdin branch from ed6134a to 1f162fc Compare March 17, 2023 13:41

motiejus force-pushed the infile_stdin branch from 1f162fc to 8280d0e Compare March 17, 2023 13:56

motiejus force-pushed the infile_stdin branch 2 times, most recently from eb6f34d to 90e6657 Compare March 24, 2023 16:56

motiejus mentioned this pull request Apr 7, 2023

[zig cc/elf] does not pass "-r" to ld.lld #11683

Closed

andrewrk requested changes Apr 7, 2023

View reviewed changes

This was referenced Apr 20, 2023

zig run/cc: recognize "-x language" #13544

Merged

Go failing unit tests with CC="zig cc" #15338

Closed

motiejus force-pushed the infile_stdin branch from 90e6657 to 9886497 Compare April 25, 2023 09:04

motiejus mentioned this pull request Apr 25, 2023

when passing -### ("dry_run") avoid actually running compilation and linking child processes #7170

Open

motiejus requested a review from andrewrk April 25, 2023 09:40

motiejus changed the title ~~zig cc: support reading from non-files~~ zig cc: support reading from stdin via -x <lang> - Apr 25, 2023

motiejus changed the title ~~zig cc: support reading from stdin via -x <lang> -~~ zig cc: support reading from stdin via -x LANG - Apr 25, 2023

motiejus changed the title ~~zig cc: support reading from stdin via -x LANG -~~ zig cc: support reading from stdin via "-x LANG -" Apr 25, 2023

andrewrk force-pushed the infile_stdin branch from 9886497 to 2d48565 Compare April 25, 2023 17:46

andrewrk force-pushed the infile_stdin branch from 2d48565 to 7f41876 Compare May 16, 2023 23:39

motiejus and others added 3 commits May 16, 2023 16:40

zig cc: support reading from non-files

2e69231

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.

andrewrk force-pushed the infile_stdin branch from 7f41876 to 233a0d3 Compare May 16, 2023 23:41

andrewrk enabled auto-merge May 16, 2023 23:41

motiejus mentioned this pull request May 17, 2023

zig cc: implement -### (dry run) #15738

Merged

motiejus commented May 17, 2023

View reviewed changes

andrewrk merged commit fd213ac into ziglang:master May 17, 2023

motiejus deleted the infile_stdin branch May 19, 2023 11:10

Uh oh!

zig cc: support reading from stdin via "-x LANG -" #14462

zig cc: support reading from stdin via "-x LANG -" #14462

Uh oh!

Conversation

motiejus commented Jan 26, 2023

Uh oh!

motiejus commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewrk commented Feb 1, 2023

Uh oh!

motiejus commented Feb 2, 2023

Uh oh!

motiejus commented Feb 2, 2023

Uh oh!

motiejus commented Feb 2, 2023

Uh oh!

motiejus commented Mar 17, 2023

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

motiejus commented Apr 20, 2023

Uh oh!

motiejus commented Apr 25, 2023

Uh oh!

andrewrk commented May 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

motiejus commented Jan 26, 2023 •

edited

Loading