Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] FreeBSD support #1661

Closed
wants to merge 15 commits into from

Conversation

Projects
None yet
5 participants
@myfreeweb
Copy link
Contributor

myfreeweb commented Oct 17, 2018

Trying to continue the freebsd branch by @tiehuis… (well, by starting over and cherry-picking from there as needed)

  • the vmovdqa problem from Windows also appears here
  • weird issue with cache_hash cache_hit: buf_eql_buf(this_path, chf->path) fails because the chf->path is the same path but 1 character longer (extra zero)
  • for now,
    • debug info is not implemented
    • Dir nextFreebsd is stubbed out
    • only getrandom (very recent syscall — FreeBSD 12) is used
    • path realC uses a Linux-style (fdescfs) approach instead of libc
    • selfExePath reads procfs (should use sysctl like the C++ version)
    • libc is used for sysctl and kqueue (copied from darwin)
    • only amd64 is there (I'll do aarch64 once amd64 is done)

Current status (of the self-hosted compiler):

Test 1/68 compare-output hello world with libc (Debug)...OK
Test 2/68 compare-output hello world with libc (ReleaseSafe)...OK
Test 3/68 compare-output hello world with libc (ReleaseFast)...OK
Test 4/68 compare-output hello world with libc (ReleaseSmall)...OK
Test 5/68 compare-output multiple files with private function (Debug)...OK
Test 6/68 compare-output multiple files with private function (ReleaseSafe)...Process /usr/home/greg/src/github.com/ziglang/zig/zig-cache/test terminated unexpectedly
error: TestFailed
* thread #1, name = 'test', stop reason = signal SIGSEGV: invalid address (fault address: 0xfffff80003cd2530)
    frame #0: 0x00000000002014ad test`File_OutStream_writeFn at index.zig:357
   354      const max_bytes_len = 0x7ffff000;
   355
   356      var index: usize = 0;
-> 357      while (index < bytes.len) {
   358          const amt_to_write = math.min(bytes.len - index, usize(max_bytes_len));
   359          const rc = posix.write(fd, bytes.ptr + index, amt_to_write);
   360          const write_err = posix.getErrno(rc);
(lldb) fr v
([]u8) bytes = (ptr = <Bad address>, len = 0)

This is a weird bug. That test binary actually completes successfully when run under truss (syscall tracing), but does this when run on its own. I have no idea how that is happening…

Also, why does the test thing generate test binaries for all the other platforms before the native one?? How do I turn that off?

@euantorano

This comment has been minimized.

Copy link

euantorano commented Oct 18, 2018

Awesome! I started having a look at this but hit some issues on the C++ side of things that I didn't get a hance to look into at all.

@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Oct 18, 2018

Thanks for doing this work. Is there any way to auto run tests on FreeBSD on master branch pushes?

* the `chf->path` is the same path but 1 character longer (extra zero)

This is probably a bug in one of the os.cpp functions, setting an incorrect buffer length.

@@ -9,9 +9,13 @@
find_path(LLD_INCLUDE_DIRS NAMES lld/Common/Driver.h
PATHS
/usr/lib/llvm-6.0/include
/usr/local/llvm60/include

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 18, 2018

Member

These 6s should be 7s (master branch is incorrect)

@@ -343,7 +343,8 @@ Error cache_hit(CacheHash *ch, Buf *out_digest) {
return ErrorInvalidFormat;
}
Buf *this_path = buf_create_from_slice(file_path);
if (chf->path != nullptr && !buf_eql_buf(this_path, chf->path)) {
// XXX: on FreeBSD, chf->path is same text but with extra \0 at the end

This comment has been minimized.

Copy link
@andrewrk

andrewrk Oct 18, 2018

Member

Gotta track this bug down. Figure out where chf->path is getting set, figure out how the buf len gets set incorrectly. Can't just put a bandage on the symptom.

This comment has been minimized.

Copy link
@myfreeweb

myfreeweb Oct 19, 2018

Author Contributor

It's the SplitIterator leaving a '\0' in the last component. Would it be okay to add || byte == 0 to SplitIterator_isSplitByte?

@euantorano

This comment has been minimized.

Copy link

euantorano commented Oct 18, 2018

Is there any way to auto run tests on FreeBSD on master branch pushes?

I had a browse around and there don't seem to be any services like Travis CI that run tests on FreeBSD, so I think the easiest solution might be to just set up Jenkins with a few build agents on various operating systems to cover the BSDs and some other platforms.

@ararslan

This comment has been minimized.

Copy link

ararslan commented Oct 18, 2018

A bit tangential but just FYI, the Julia language repo has self-hosted FreeBSD CI set up on a contributor's server, managed via Buildbot. If you want to go this route, you can have a look at our setup.

@myfreeweb

This comment has been minimized.

Copy link
Contributor Author

myfreeweb commented Oct 18, 2018

Looks like there's already a buildbot for linux/aarch64

@myfreeweb

This comment has been minimized.

Copy link
Contributor Author

myfreeweb commented Oct 19, 2018

Okay, testing with -Dskip-release=true gets much further, but…

Test 402/504 behavior-freebsd-x86_64-Debug-bare packed struct 24bits...Process 1943 stopped
* thread #1, name = 'test', stop reason = signal SIGBUS: hardware error
    frame #0: 0x0000000000217d8e test`behavior-freebsd-x86_64-Debug-bare packed struct 24bits at struct.zig:259
   256          assert(@sizeOf(Foo96Bits) == 12);
   257      }
   258
-> 259      var value = Foo96Bits.{
   260          .a = 0,
   261          .b = 0,
   262          .c = 0,
(lldb) dis
test`behavior-freebsd-x86_64-Debug-bare packed struct 24bits:
    0x217d80 <+0>:   pushq  %rbp
    0x217d81 <+1>:   movq   %rsp, %rbp
    0x217d84 <+4>:   subq   $0x80, %rsp
    0x217d8b <+11>:  xorps  %xmm0, %xmm0
->  0x217d8e <+14>:  movaps %xmm0, -0x10(%rbp)
    0x217d92 <+18>:  leaq   -0x10(%rbp), %rax
    0x217d96 <+22>:  movzbl -0xe(%rbp), %ecx

SIMD registers again, just like in the vmovdqa issue!

@myfreeweb

This comment has been minimized.

Copy link
Contributor Author

myfreeweb commented Oct 19, 2018

So here's a simplified IR of that Zig test case:

%Foo96Bits = type <{ i24, i24, i24, i24 }>
@0 = internal unnamed_addr constant %Foo96Bits zeroinitializer, align 1

; @"behavior-freebsd-x86_64-Debug-barepacked struct 24bits"
define i32 @main(i32) unnamed_addr #0 {
Entry:
  %value = alloca %Foo96Bits, align 1
  %1 = bitcast %Foo96Bits* %value to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %1, i8* align 1 bitcast (%Foo96Bits* @0 to i8*), i64 16, i1 false)
  ret i32 0
}

declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i1) #4
# %bb.0:                                # %Entry
        xorps   %xmm0, %xmm0
        movaps  %xmm0, -24(%rsp)
        xorl    %eax, %eax
        retq

But for a "similar" C program:

#include <string.h>

struct test {
   unsigned int a : 24;
   unsigned int b : 24;
   unsigned int c : 24;
   unsigned int d : 24;
};

static struct test two = { 0, 0, 0, 0 };

int main() {
   struct test one;
   memcpy((void*)&one, (void*)&two, sizeof(struct test));
}

Clang emits:

%struct.test = type { i24, i24, i24, i24 }

@two = internal global { i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 } { i8 0, i8 0, i8 0, i8 undef, i8 0, i8 0, i8 0, i8 undef, i8 0, i8 0, i8 0, i8 undef, i8 0, i8 0, i8 0, i8 undef }, align 4

define dso_local i32 @main() #0 {
entry:
  %one = alloca %struct.test, align 4
  %0 = bitcast %struct.test* %one to i8*
  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %0, i8* align 1 getelementptr inbounds ({ i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 }, { i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8, i8 }* @two, i32 0, i32 0), i64 16, i1 false)
  ret i32 0
}

declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i1) #1

Which compiles fine:

# %bb.0:                                # %entry
        movq    two+8(%rip), %rax
        movq    %rax, -8(%rsp)
        movq    two(%rip), %rax
        movq    %rax, -16(%rsp)
        xorl    %eax, %eax
        retq
@tiehuis

This comment has been minimized.

Copy link
Member

tiehuis commented Oct 19, 2018

This looks related to #1217. I didnt get any further on this issue.

@myfreeweb

This comment has been minimized.

Copy link
Contributor Author

myfreeweb commented Oct 20, 2018

I figured it out. Stack alignment:

diff --git i/src/codegen.cpp w/src/codegen.cpp
index 5845ff8c..1101e8d9 100644
--- i/src/codegen.cpp
+++ w/src/codegen.cpp
@@ -568,9 +568,9 @@ static LLVMValueRef fn_llvm_value(CodeGen *g, ZigFn *fn_table_entry) {
         maybe_import_dll(g, fn_table_entry->llvm_value, linkage);
     }
 
-    if (fn_table_entry->alignstack_value != 0) {
-        addLLVMFnAttrInt(fn_table_entry->llvm_value, "alignstack", fn_table_entry->alignstack_value);
-    }
+    //if (fn_table_entry->alignstack_value != 0) {
+        addLLVMFnAttrInt(fn_table_entry->llvm_value, "alignstack", 4);
+    //}
 
     addLLVMFnAttr(fn_table_entry->llvm_value, "nounwind");
     add_uwtable_attr(g, fn_table_entry->llvm_value);

Now up to test 426…

@myfreeweb myfreeweb force-pushed the myfreeweb:freebsd-up branch from 0965be8 to f3bc1c3 Oct 20, 2018

@myfreeweb

This comment has been minimized.

Copy link
Contributor Author

myfreeweb commented Oct 20, 2018

Now behavior tests pass in debug mode…

Anyway, I'd like to discuss the direct syscalls vs libc thing.
I see that Linux support is implemented directly, while Darwin support relies on libc.

I think I'd like to use libc on FreeBSD, because:

  • the Linux reasons for avoiding libc (like the diversity of libcs across distros) do not apply here
  • implementing direct syscall support for things like threads doesn't sound fun
  • implementing syscall wrappers for all CPU architectures doesn't sound fun
  • I'd prefer LD_PRELOAD hooks that override libc functions to work on all applications, including Zig ones
@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Oct 24, 2018

  • implementing direct syscall support for things like threads doesn't sound fun
  • implementing syscall wrappers for all CPU architectures doesn't sound fun

These are ok reasons for starting off FreeBSD support with depending on libc (for syscalls only) but are not long term reasons for sticking with libc. I'm ok with adding freebsd support that depends on libc as a first pass and then later going the next step of eliminating the libc dependency.

  • I'd prefer LD_PRELOAD hooks that override libc functions to work on all applications, including Zig ones

There is a pattern in the std lib you can see here:

zig/std/os/index.zig

Lines 168 to 175 in b480118

/// Raises a signal in the current kernel thread, ending its execution.
/// If linking against libc, this calls the abort() libc function. Otherwise
/// it uses the zig standard library implementation.
pub fn abort() noreturn {
@setCold(true);
if (builtin.link_libc) {
c.abort();
}

This accomplishes both supporting LD_PRELOAD hooks, as well as non-libc static binaries, depending on whether the user links with libc.

There is another strong reason to avoid a libc dependency when possible, and it is that it gives the zig compiler full knowledge of the entire source. This means that Zig can do some program-wide static analysis, such as stack upper bound calculation (see #157).

That said - I am ok with depending on libc as a first pass, and then let's come back to it later and see if we want to take the extra step of eliminating the libc dependency.

@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Nov 19, 2018

@myfreeweb hey thanks for continuing this effort. I just wanted to let you know that I have installed FreeBSD on a Raspberry Pi and so I will use that to pick up where you left off here and try to get this merged with all tests passing.

@myfreeweb

This comment has been minimized.

Copy link
Contributor Author

myfreeweb commented Nov 19, 2018

yeah, sorry, I've been busy with work and stuff. I'm glad that you're interested in FreeBSD!

@andrewrk andrewrk added this to the 0.5.0 milestone Nov 19, 2018

@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Nov 19, 2018

I merged master into this branch and resolved the conflicts, and then pushed it to freebsd2 branch. I'm closing this PR now. Feel free to open additional PRs that are based on the freebsd2 branch (rather than master). I'll take charge of slowly working on the freebsd2 branch, getting all the tests passing, getting CI integration going for FreeBSD, and then merging into master.

@andrewrk andrewrk closed this Nov 19, 2018

@andrewrk andrewrk referenced this pull request Nov 19, 2018

Open

Tier 1 FreeBSD Support for x86_64 #1759

5 of 7 tasks complete
@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Nov 19, 2018

See #1759

@@ -47,7 +47,7 @@ bool ptr_eq(const void *a, const void *b) {
// Ported from std/mem.zig.
bool SplitIterator_isSplitByte(SplitIterator *self, uint8_t byte) {
for (size_t i = 0; i < self->split_bytes.len; i += 1) {
if (byte == self->split_bytes.ptr[i]) {
if (byte == self->split_bytes.ptr[i] || byte == 0) {

This comment has been minimized.

Copy link
@andrewrk

andrewrk Nov 28, 2018

Member

What was this for? I'm reverting this change - this is a bandage, but let's fix the actual bug.

This comment has been minimized.

Copy link
@myfreeweb

myfreeweb Nov 28, 2018

Author Contributor

see beginning of thread

the chf->path is the same path but 1 character longer (extra zero)

This comment has been minimized.

Copy link
@andrewrk

andrewrk Nov 28, 2018

Member

This likely represents a bug in os.cpp having to do with file paths that needs to be fixed. I'll see if I can reproduce it.

This comment has been minimized.

Copy link
@andrewrk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.