zig icon indicating copy to clipboard operation
zig copied to clipboard

Plan9: Fix The Backend

Open g-w1 opened this issue 2 years ago • 7 comments

This adds

  • unnamed decl support
  • relocation support Hello world now works again

g-w1 avatar Oct 06 '22 00:10 g-w1

CI checks failing

andrewrk avatar Oct 15 '22 14:10 andrewrk

Yes, I saw that. I am not sure why they are failing because I didn't change anything in codegen relating to branches and cannot repro this on a personal computer. I'll look into it soon, thanks for bringing it to my attention.

g-w1 avatar Oct 15 '22 22:10 g-w1

Overflow error is due to an error in consolidating instructions between parent, then, and else branches in airCondBr lowering. This usually can be one of two things: either a bug in consolidation code, or an incorrect/incomplete implementation of some lowering for the target. You should first rule out this issue can be reproed on x86_64-linux - if yes, it's a wider bug in the codegen. Otherwise, could it be that you have missed some Plan9 specific prong(s) in the codegen?

kubkon avatar Oct 16 '22 16:10 kubkon

Funnily enough, I get this on master also when doing zig build test-compare: https://paste.rs/m0G And this is without the Plan9 test case in there. I couldn't repro this with build-exe.

g-w1 avatar Oct 16 '22 19:10 g-w1

@g-w1 related question, do you want to type up some release notes for zig's plan9 support to go into the upcoming 0.10.0 release notes?

andrewrk avatar Oct 19 '22 01:10 andrewrk

Sure, but I don't know if I will have anything to say in the release notes. If the CI bug on this PR gets resolved, then I will have something to type up. Thanks for asking!

g-w1 avatar Oct 19 '22 10:10 g-w1

Blocking on #13231 Which I believe is the same bug as the one on the CI here.

g-w1 avatar Oct 19 '22 23:10 g-w1

Rebasing on latest master makes the original panic go away which is quite worrying and I'll investigate locally why that is. In the meantime, in order not to block your PR, there are a couple more fixes required to make the CI green. First though, could you please rebase on latest master? Next, when you build the compiler with LLVM enabled, it assumes Plan9 is a valid target of LLVM's and tries to build libc.a and libcompiler_rt.a. As we currently lack any way of doing it for Plan9 we need to set Plan9 as incapable of building any of those libs until further notice.

I tried pushing the fixes to you branch directly but you have not allowed it in your fork sadly, so I am pasting the diff here:

--- a/src/Compilation.zig
+++ b/src/Compilation.zig
@@ -1088,10 +1088,10 @@ pub fn create(gpa: Allocator, options: InitOptions) !*Compilation {
     // Once they are capable this condition could be removed. When removing this condition,
     // also test the use case of `build-obj -fcompiler-rt` with the native backends
     // and make sure the compiler-rt symbols are emitted.
-    const capable_of_building_compiler_rt = build_options.have_llvm;
+    const capable_of_building_compiler_rt = build_options.have_llvm and options.target.os.tag != .plan9;
 
-    const capable_of_building_zig_libc = build_options.have_llvm;
-    const capable_of_building_ssp = build_options.have_llvm;
+    const capable_of_building_zig_libc = build_options.have_llvm and options.target.os.tag != .plan9;
+    const capable_of_building_ssp = build_options.have_llvm and options.target.os.tag != .plan9;
 
     const comp: *Compilation = comp: {
         // For allocations that have the same lifetime as Compilation. This arena is used only during this
diff --git a/src/link.zig b/src/link.zig
index b0c829448..dc1174992 100644
--- a/src/link.zig
+++ b/src/link.zig
@@ -581,7 +581,7 @@ pub const File = struct {
             .macho => return @fieldParentPtr(MachO, "base", base).updateDeclLineNumber(module, decl),
             .c => return @fieldParentPtr(C, "base", base).updateDeclLineNumber(module, decl),
             .wasm => return @fieldParentPtr(Wasm, "base", base).updateDeclLineNumber(module, decl),
-            .plan9 => @panic("TODO: implement updateDeclLineNumber for plan9"),
+            .plan9 => return @fieldParentPtr(Plan9, "base", base).updateDeclLineNumber(module, decl),
             .spirv, .nvptx => {},
         }
     }
diff --git a/src/link/Plan9.zig b/src/link/Plan9.zig
index 86c3c1817..7e7e7be90 100644
--- a/src/link/Plan9.zig
+++ b/src/link/Plan9.zig
@@ -956,6 +956,14 @@ pub fn allocateDeclIndexes(self: *Plan9, decl_index: Module.Decl.Index) !void {
     _ = self;
     _ = decl_index;
 }
+
+/// Must be called only after a successful call to `updateDecl`.
+pub fn updateDeclLineNumber(self: *Plan9, mod: *Module, decl: *const Module.Decl) !void {
+    _ = self;
+    _ = mod;
+    _ = decl;
+}
+
 pub fn getDeclVAddr(
     self: *Plan9,
     decl_index: Module.Decl.Index,
diff --git a/test/cases/plan9/exit.zig b/test/cases/plan9/exit.zig
index 735818fb8..5c5cf2176 100644
--- a/test/cases/plan9/exit.zig
+++ b/test/cases/plan9/exit.zig
@@ -1,5 +1,5 @@
 pub fn main() void {}
 
 // run
-// target=x86_64-plan9,aarch64-plan9
+// target=x86_64-plan9
 //

Please note I've had to remove aarch64-plan9 as you haven't updated that backend for lowering unnamed consts for Plan9 target.

kubkon avatar Oct 24 '22 15:10 kubkon

Re the original error (overflow): it was caused because we tried building libcompiler_rt.a with x86_64 backend which not advanced enough and an overflow in airCondBr is usually caused by an unhandled lowering. Applying the above patch I suggested fixes it for me too regardless of rebasing.

kubkon avatar Oct 24 '22 16:10 kubkon

Thank you so much @kubkon . I'll try to apply those changes as soon as possible. I'll also take a look if rebasing fixes #13231 and close it if necessary

g-w1 avatar Oct 24 '22 17:10 g-w1

Thank you so much @kubkon . I'll try to apply those changes as soon as possible. I'll also take a look if rebasing fixes #13231 and close it if necessary

I don't think your PR and #13231 are actually related beyond hitting the same panic message which can easily be triggered by trying out more behavior tests with x86 backend :-)

kubkon avatar Oct 24 '22 18:10 kubkon

Oh, I see. Thanks anyways. I think I have checked the box to allow maintainers to make edits, so I am not sure what is going on.

g-w1 avatar Oct 24 '22 21:10 g-w1