Implement emit-h by tetsuo-cpp · Pull Request #7111 · ziglang/zig

tetsuo-cpp · 2020-11-14T13:26:12Z

pixelherodev · 2020-11-14T20:07:07Z

I'll review this tonight or tomorrow before getting back to work on the CBE :)

pixelherodev · 2020-11-15T15:47:02Z

Alright, reviewing now! :)

pixelherodev · 2020-11-15T16:58:01Z

+ decl.analysis = .dependency_failure;
+ },
+ else => {
+ try module.failed_decls.ensureCapacity(module.gpa, module.failed_decls.items().len + 1);


I think this makes sense. If emit-h is desired and fails, it should be treated as a failed compilation.

pixelherodev · 2020-11-15T17:05:43Z

@tetsuo-cpp Do you want to fix this up, or do you want me to? I'm happy either way :)

pixelherodev · 2020-11-15T19:41:23Z

Ah, right, one other thing: you need to add tests. However, there's no test harness for headers yet. I was planning on working on tests today anyways, I can add the harness and tests in a separate PR if you want to do the rest of the work and leave the tests to me. Alternately, if you want, I can (as mentioned earlier) take over this PR and do the tests myself, or I can help you with the full PR + tests and leave the work to you on this one.

This is entirely up to you, just let me know how you want to proceed. I'm going to work on unrelated CBE test integration (running the tests with zig cc now that we have stage1+stage2 integration :) in the meanwhile, so no rush.

tetsuo-cpp · 2020-11-15T23:04:41Z

Thanks for taking a look @pixelherodev.

Are you on IRC?

I don't use IRC at the moment. I can get set up if it's helpful.
Alternatively, I'm on the Zig Discord under "asc" if you use that.

Do you want to fix this up, or do you want me to? I'm happy either way :)

I'm happy to continue working on this until it's mergeable. Most of these comments look straightforward so I'll address them shortly.

Ah, right, one other thing: you need to add tests. However, there's no test harness for headers yet. I was planning on working on tests today anyways, I can add the harness and tests in a separate PR if you want to do the rest of the work and leave the tests to me.

Sure, let's do that. Once you've written the harness, I'm happy to help with writing tests if you'd like.

tetsuo-cpp · 2020-11-16T13:00:41Z

@pixelherodev

I've addressed your comments and marked the ones that aren't relevant anymore as "resolved".

Since we decided to continue returning error.AnalysisFail in the TODO case, most of the error handling looks the same.

The main sticking point this time was reverting back to using the Context object as an argument. Some of the stuff in Context (mainly the file) aren't relevant in the header generation path. What I've done is marked this member as an optional and in the header path, assigned it to null. It seems a bit fragile but the idea is that in any of the logic used in header generation, we shouldn't be touching file.

Another alternative would be to rip file out of the Context and just pass it in a separate argument. That way, anything that is used in the header generation path won't take a file argument and we can't make this mistake of trying to use file when it's null. I realize this adds an additional function argument but it seems better to me. What do you think?

Since Context is just an internal thing and link.C interacts with code generation via the file, I need some way of propagating error messages to file. But if we get an error in the header generation logic, then I can't rely on calling ctx.file.fail since in the emit-h case, it will be null. So I've introduced ctx.fail and then added some logic at the top level to transfer the error_msg across to the file so it can be used by the caller. That part is at codegen/c.zig:186.

Sorry if this isn't what you meant, I'm still finding my feet here.

pixelherodev · 2020-11-16T19:46:08Z

file as a separate argument is fine IMO - that's entirely up to you.

I think this is good, just needs the testing work - I'll try to have that ready for you tonight :)

pixelherodev · 2020-11-18T23:36:59Z

Mind if I open a PR against your branch splitting this into multiple commits?

tetsuo-cpp · 2020-11-18T23:42:12Z

Mind if I open a PR against your branch splitting this into multiple commits?

Sure, no problem.

pixelherodev · 2020-11-19T00:09:47Z

Okay, I think I should have a PR for header tests shortly :)

pixelherodev · 2020-11-19T02:02:48Z

@tetsuo-cpp tetsuo-cpp#2

pixelherodev · 2020-11-19T02:04:53Z

Last problem: emit-h in compilation and c_header shouldn't coexist, and should be merged.

pixelherodev · 2020-11-19T02:51:49Z

Actually, this is also missing emit_h enablement in src/main.zig

tetsuo-cpp · 2020-11-19T09:57:50Z

Last problem: emit-h in compilation and c_header shouldn't coexist, and should be merged.

Done. I added an ?EmitLoc member to c_header.

Actually, this is also missing emit_h enablement in src/main.zig

Done. Thanks, I missed that. Am I understanding correctly that if we do build-obj or build-lib, we're supposed to automatically emit a header even without femit-h?

@tetsuo-cpp tetsuo-cpp#2

Thanks for doing that. It looks good to me.

How do we want to do this? I'm thinking that I should merge that PR into my branch now and then I'll push a follow up commit adding some unit tests for the header generation. Do you agree with that?

pixelherodev · 2020-11-19T21:53:24Z

. Am I understanding correctly that if we do build-obj or build-lib, we're supposed to automatically emit a header even without femit-h?

(default) Do not generate a C header file (.h)

Nope!

I'm thinking that I should merge that PR into my branch now and then I'll push a follow up commit adding some unit tests for the header generation. Do you agree with that?

Yep!

Once that's done, we can bump this up to the core team for further review and merging :)

tetsuo-cpp · 2020-11-20T08:04:53Z

(default) Do not generate a C header file (.h)
Nope!

The changes that I uncommented in main.zig look like they're doing that. The argument parsing was already working, that chunk looks like it's enabling it by default in certain scenarios.

Once that's done, we can bump this up to the core team for further review and merging :)

Sounds good! I'll prepare some tests over the weekend.

pixelherodev · 2020-11-20T20:44:29Z

Ah, I see.

I'd say either leave it as is and change the docs (zig build-* --help output), or make it always off without an explicit flag, at your discretion. As long as docs and behavior match, I don't think it's too important - and if it is, it can be changed trivially later instead of bikeshedding it now.

tetsuo-cpp · 2020-11-23T13:29:01Z

@pixelherodev
Ok, I've added some tests now.

The only other changes since last time are that I've disabled header generation unless -femit-h is supplied and I made a minor change to the test code to avoid running with header generation for non-CBE tests since some of them use things that aren't supported yet like u64.

uael · 2020-11-23T14:58:45Z

 }
 }
+
+ // If we've chosen to emit a C header, flush the header the disk.


Suggested change
// If we've chosen to emit a C header, flush the header the disk.
// If we've chosen to emit a C header, flush the header to the disk.

Rebased as part of b323209.

pixelherodev · 2020-11-24T17:35:12Z

Okay,if you merge the suggested fix for the comment, I think this has my approval :)

tetsuo-cpp · 2020-11-25T13:24:38Z

Okay,if you merge the suggested fix for the comment, I think this has my approval :)

Awesome. Done!

phifty · 2020-12-17T09:43:57Z

Are there any news on this?

tetsuo-cpp · 2020-12-17T10:02:26Z

Are there any news on this?

This patch is ready to go from my point of view. Just pending a review + merge from the team.

phifty · 2020-12-17T10:42:59Z

Ok thanks! I'm looking forward to this. It's preventing me to update my zig version.

Vexu · 2020-12-17T16:05:46Z

+ if (expected_output.len != out.len) {
+ std.debug.print("\nTransformed header length differs:\n================\nExpected:\n================\n{}\n================\nFound:\n================\n{}\n================\nTest failed.\n", .{ expected_output, out });
+ std.process.exit(1);
+ }
+ for (expected_output) |e, i| {
+ if (out[i] != e) {
+ std.debug.print("\nTransformed header differs:\n================\nExpected:\n================\n{}\n================\nFound:\n================\n{}\n================\nTest failed.\n", .{ expected_output, out });
+ std.process.exit(1);
+ }
+ }


These should use std.testing.expectEqualStrings.

As @pixelherodev mentioned, it's done like this elsewhere in test.zig. I'm planning to follow up with a PR after this to make this change across the file.

Vexu · 2020-12-17T16:20:11Z

 man.hash.add(comp.bin_file.options.is_test);
 man.hash.add(comp.bin_file.options.emit != null);
- man.hash.addOptionalEmitLoc(comp.emit_h);
+ man.hash.add(comp.c_header != null);


There should be a addOptionalEmitLoc(header.emit_loc.?) here if c_header != null.

Vexu · 2020-12-17T16:27:10Z

- .call => try genCall(&ctx, inst.castTag(.call).?),
- .add => try genBinOp(&ctx, inst.cast(Inst.BinOp).?, "+"),
- .sub => try genBinOp(&ctx, inst.cast(Inst.BinOp).?, "-"),
+ .assembly => try genAsm(&ctx, file, inst.castTag(.assembly).?),


Why remove file from the context if you're just going to pass it to nearly everything anyways?

The file contains everything needed to output C source when the CBE is selected (buffers for main, constants, etc). That doesn't apply when we're generating a header and we may not even have one since we could be using a different backend in combination with emit-h.
Therefore, I've removed it from the context and passed it as a separate argument in the non-header code paths. Another idea could be to make it optional within the context but that seemed more error-prone to me.

pixelherodev · 2020-12-17T19:00:04Z

File should stay in the context. Re testing expect vs print and exit, I think I copied that style from the surrounding code - might be better to leave it alone here and do a full cleanup in a follow up PR.

tetsuo-cpp · 2020-12-22T14:16:57Z

@Vexu
Thanks for the review. I've just rebased to fix the merge conflict.

tetsuo-cpp commented Nov 14, 2020
View reviewed changes

Comment threadsrc/codegen/c.zig Outdated
Comment threadsrc/link/C.zig Outdated

pixelherodev suggested changes Nov 15, 2020
View reviewed changes

alexnask added the frontend Tokenization, parsing, AstGen, Sema, and Liveness. label Nov 16, 2020

tetsuo-cpp force-pushed the emit-h branch from 392b70d to f505b3dCompare November 16, 2020 12:23

tetsuo-cpp force-pushed the emit-h branch 2 times, most recently from b126510 to 2a12c56Compare November 17, 2020 12:28

tetsuo-cpp force-pushed the emit-h branch from d18f5d4 to 2740cb1Compare November 19, 2020 09:46

tetsuo-cpp force-pushed the emit-h branch 2 times, most recently from 9b0184c to cce3138Compare November 23, 2020 11:29

uael reviewed Nov 23, 2020
View reviewed changes

tetsuo-cpp force-pushed the emit-h branch from 870e00d to 73a118bCompare November 25, 2020 13:23

pixelherodev approved these changes Nov 25, 2020
View reviewed changes

tetsuo-cpp force-pushed the emit-h branch 2 times, most recently from 8884f1f to 98a533bCompare December 5, 2020 05:50

uael approved these changes Dec 17, 2020
View reviewed changes

Vexu requested changes Dec 17, 2020
View reviewed changes

tetsuo-cpp requested a review from Vexu December 19, 2020 05:32

Vexu approved these changes Dec 21, 2020
View reviewed changes

tetsuo-cppand others added 12 commits December 23, 2020 01:14

Implement emit-h
40f0275

Enable emit-h by default for obj and lib compilation.
aba273d

Remove redundant emit_h member in Compilation struct.
c87da2f

Fix memory leak
15a148d

CBE: add support for bool,u32
37438dd

CBE: Improve formatting
ac33b10

Add header test harness
8538053

Fix a dumb
8d6066e

Add tests for emit-h functionality.
58bd6c5

Make sure emit-h is off by default.
e834d03

Don't use emit-h in test cases for other backends.
0771aac

Add emit_h path to compilation hash.
9849e89

tetsuo-cpp force-pushed the emit-h branch from e5fb9d0 to 9849e89Compare December 22, 2020 14:15

Vexu merged commit 03113d9 into ziglang:masterDec 22, 2020

	// If we've chosen to emit a C header, flush the header the disk.
	// If we've chosen to emit a C header, flush the header to the disk.

Uh oh!

Uh oh!

Conversation

tetsuo-cpp commented Nov 14, 2020

Uh oh!

Uh oh!

Uh oh!

pixelherodev commented Nov 14, 2020

Uh oh!

pixelherodev commented Nov 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pixelherodev commented Nov 15, 2020

Uh oh!

pixelherodev commented Nov 15, 2020

Uh oh!

tetsuo-cpp commented Nov 15, 2020

Uh oh!

tetsuo-cpp commented Nov 16, 2020

Uh oh!

pixelherodev commented Nov 16, 2020

Uh oh!

pixelherodev commented Nov 18, 2020

Uh oh!

tetsuo-cpp commented Nov 18, 2020

Uh oh!

pixelherodev commented Nov 19, 2020

Uh oh!

pixelherodev commented Nov 19, 2020

Uh oh!

pixelherodev commented Nov 19, 2020

Uh oh!

pixelherodev commented Nov 19, 2020

Uh oh!

tetsuo-cpp commented Nov 19, 2020

Uh oh!

pixelherodev commented Nov 19, 2020

Uh oh!

tetsuo-cpp commented Nov 20, 2020

Uh oh!

pixelherodev commented Nov 20, 2020

Uh oh!

tetsuo-cpp commented Nov 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pixelherodev commented Nov 24, 2020 via email

Uh oh!

tetsuo-cpp commented Nov 25, 2020

Uh oh!

phifty commented Dec 17, 2020

Uh oh!

tetsuo-cpp commented Dec 17, 2020

Uh oh!

phifty commented Dec 17, 2020

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!