Bring emit-h back to life! #20353

SuperAuguste · 2024-06-20T00:40:04Z

See the current results here: https://zigbin.io/a90812

Some things I still need to do:

Use export names rather than the names of the exported decls (can be different w/ @export for example)
~~Give structs better generated names(?)~~ Decided to keep simple, not fully-qualified names for now. Can definitely cause issues, but this requires a larger rethinking potentially that should be in a proposal issue.
Figure out why ZIR indices on structs, unions, and enums seem to disappear (there's probably a good reason for this that I'm not aware of, or a bug)
Add parent_fix from CBE logic I stole back to remove unnecessary parens
Extract param names from functions
Better styling for output code (a little cramped? maybe group functions together?)
~~Probably add a couple tests~~ Not blocking for merge, can be added later.

But it does work 100% better than the current emit-h! :P

mlugg · 2024-06-20T00:49:41Z

Figure out why ZIR indices on structs, unions, and enums seem to disappear

If you're referring to the zir_decl_index field on Decl, this is by design -- the doc comment explains what's going on. Unfortunately, this code still overuses Decl -- this type currently has a lot of responsibilities, and I'm actively working on splitting it into some more atomic types. However, that need not block this PR -- from a quick glance, this work is a good baseline which I can build on as a part of my work on Decl.

mlugg

There are several TODOs you've noted, and a few more potential issues I've flagged up. However, this change overall seems pretty good. Since emit-h is completely broken today, I think this PR is reasonable to merge as-is to unblock at least some use cases (such as your own!).

Upon merge, I will open a follow-up issue detailing the remaining issues with emit-h. I will likely end up solving some of them in my upcoming Decl refactor, which is another reason I think this should be merged as-is.

mlugg · 2024-06-20T00:50:00Z

src/Compilation.zig

+
+                    std.debug.assert(emit_h.allocated_emit_h.len == emit_h.decl_table.count());
+
+                    var emitter = EmitH{


Suggested change

var emitter = EmitH{

var emitter: EmitH = .{

mlugg · 2024-06-20T00:52:03Z

src/Sema.zig

@@ -6572,6 +6572,7 @@ fn addExport(mod: *Module, export_init: Module.Export) error{OutOfMemory}!void {
            const de_gop = mod.decl_exports.getOrPutAssumeCapacity(decl_index);
            if (!de_gop.found_existing) de_gop.value_ptr.* = .{};
            try de_gop.value_ptr.append(gpa, new_export);
+            try mod.emitHDecl(decl_index);
        },
        .value => |value| {


We also need to handle this value case -- this corresponds to an @export of a comptime-known value. That doesn't need to block this merge.

mlugg · 2024-06-20T19:37:45Z

src/EmitH.zig

@@ -0,0 +1,731 @@
+const std = @import("std");


This file's naming is a little confusing due to the existence of Zcu.EmitH, but that need not block merge. Zcu.EmitH will need to be restructured soon anyway to deal with serialization.

mlugg · 2024-06-20T19:44:03Z

src/EmitH.zig

+        }
+    }
+
+    if (zcu.decl_exports.get(emitter.decl_index)) |exports| {


There's a subtly weird case here: if a decl foo is @exported by two different decls bar and qux, and bar's emit_h_decl job is run before qux is analyzed, then we'll miss the export from qux.

Now that I see this issue, I think I understand why the old implementation queued this work in Zcu.semaDecl: presumably, it was (at least trying to) use export_owners to deal with the exports created by the just-analyzed Decl. However, I don't think that ought to block this PR, because using Decl in this way is currently a little borked anyway (something my upcoming refactors will improve).

mlugg · 2024-06-20T19:46:58Z

src/EmitH.zig

+            .Struct => {
+                const should_make_opaque = switch (value_as_type.containerLayout(zcu)) {
+                    .@"extern" => false,
+                    .@"packed" => switch (value_as_type.bitSizeAdvanced(zcu, null) catch unreachable) {


Suggested change

.@"packed" => switch (value_as_type.bitSizeAdvanced(zcu, null) catch unreachable) {

.@"packed" => switch (value_as_type.bitSize(zcu)) {

jacobly0 · 2024-06-21T07:19:34Z

Suggested future work:

I'm pretty sure emit-h should not be using zig.h (and as has been pointed out, the type emulation code performed by zig.h is not appropriate for emit-h at all), the amount of support code actually required by emit-h is very minimal and the parts that are actually used can be conditionally written to the top of the file.
There's no chance that type decls are being emitted in the correct dependency order without a type pool. The emit-h-specific type transformations should be a separate mode when adding to the pool, not special-cased here.
Why is this using a generic (and non-stable!) sort to perform a bucket sort, preferably just keep them in separate data structures per section to begin with.

SuperAuguste · 2024-06-21T13:52:00Z

Thanks for the feedback folks. I will try to address everything today.

andrewrk · 2024-10-15T08:34:48Z

draft status, no updates in 30+ days

wainejr · 2024-10-18T23:28:27Z

@andrewrk can you provide some explanation on the issue closing and the plans on supporting emit-h?
Anyone that works in a codebase mixing C/C++ and Zig knows the pain of having to maintain duplicate functions and types declarations. Even with all the testing passed and the approved status of the reviewer, we wonder why was this feature not added, or even if it will be added (and when).

"draft status, no updated in 30+ days" seems a bit too open and no solution is provided. Does it have to wait to something? Is there need to create a group of discussion? Why is it a draft not a "proper" solution? Is this because of who created it?

If you could make these decisions more clear, it would be very nice.

squeek502 · 2024-10-19T03:33:15Z

@wainejr here's some context:

SuperAuguste marked this pull request as draft 4 months ago

See here if you're not already familiar with what a draft pull request is.

SuperAuguste force-pushed the fix-emit-h branch from ae30cd1 to 46c27e0 Compare June 20, 2024 00:45

SuperAuguste force-pushed the fix-emit-h branch from ccc0cb8 to 41f8bfa Compare June 20, 2024 19:18

mlugg approved these changes Jun 20, 2024

View reviewed changes

SuperAuguste marked this pull request as draft June 21, 2024 13:52

SuperAuguste added 7 commits June 21, 2024 09:54

Initial work for fixing emit-h

aef4047

Small refactor of emit-h logic

e1e445e

Emit more exported declarations

6bf0e6f

Port over part of CBE type logic to emit-h

d967b05

Emit doc comments for struct, union, enum fields

5feb1bb

Use export names, remove extra generated parens

74b38e2

Make headers a little prettier

8e536fa

SuperAuguste force-pushed the fix-emit-h branch from 41f8bfa to 8e536fa Compare June 21, 2024 13:58

Rebase on master

95e6542

alexrp mentioned this pull request Jul 11, 2024

Support -femit-h for Zig projects vezel-dev/zig-sdk#27

Open

andrewrk closed this Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring emit-h back to life! #20353

Bring emit-h back to life! #20353

SuperAuguste commented Jun 20, 2024 •

edited

Loading

mlugg commented Jun 20, 2024

mlugg left a comment

mlugg Jun 20, 2024

mlugg Jun 20, 2024 •

edited

Loading

mlugg Jun 20, 2024

mlugg Jun 20, 2024

mlugg Jun 20, 2024

jacobly0 commented Jun 21, 2024 •

edited

Loading

SuperAuguste commented Jun 21, 2024

andrewrk commented Oct 15, 2024

wainejr commented Oct 18, 2024

squeek502 commented Oct 19, 2024 •

edited

Loading


		std.debug.assert(emit_h.allocated_emit_h.len == emit_h.decl_table.count());

		var emitter = EmitH{

	.@"packed" => switch (value_as_type.bitSizeAdvanced(zcu, null) catch unreachable) {
	.@"packed" => switch (value_as_type.bitSize(zcu)) {

Bring emit-h back to life! #20353

Bring emit-h back to life! #20353

Conversation

SuperAuguste commented Jun 20, 2024 • edited Loading

mlugg commented Jun 20, 2024

mlugg left a comment

Choose a reason for hiding this comment

mlugg Jun 20, 2024

Choose a reason for hiding this comment

mlugg Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

mlugg Jun 20, 2024

Choose a reason for hiding this comment

mlugg Jun 20, 2024

Choose a reason for hiding this comment

mlugg Jun 20, 2024

Choose a reason for hiding this comment

jacobly0 commented Jun 21, 2024 • edited Loading

SuperAuguste commented Jun 21, 2024

andrewrk commented Oct 15, 2024

wainejr commented Oct 18, 2024

squeek502 commented Oct 19, 2024 • edited Loading

SuperAuguste commented Jun 20, 2024 •

edited

Loading

mlugg Jun 20, 2024 •

edited

Loading

jacobly0 commented Jun 21, 2024 •

edited

Loading

squeek502 commented Oct 19, 2024 •

edited

Loading