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

Initial sparc64-linux bringup #6792

Merged
merged 28 commits into from
Nov 2, 2020
Merged

Conversation

koachan
Copy link
Contributor

@koachan koachan commented Oct 24, 2020

This is the continuation of #6187, to bring basic sparc64 support as requested in #4931.
As of now, it is complete enough that the stage1 compiler manages to compile a "hello world" program and some of the the example codes from the homepage.

@daurnimator daurnimator added arch-sparc 32-bit and 64-bit SPARC os-linux labels Oct 25, 2020
lib/std/os/linux/tls.zig Show resolved Hide resolved
lib/std/os/linux.zig Outdated Show resolved Hide resolved
lib/std/os/linux.zig Show resolved Hide resolved
lib/std/special/c.zig Outdated Show resolved Hide resolved
lib/std/os/linux/sparc64.zig Outdated Show resolved Hide resolved

pub const SIG_BLOCK = 1;
pub const SIG_UNBLOCK = 2;
pub const SIG_SETMASK = 3;
}
else if (is_sparc)
struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of the constants here are wrong, here's a small excerpt from sigaction.h for sparcv9:

#define	SA_NOCLDSTOP 0x00000008  /* Don't send SIGCHLD when children stop.  */
#define SA_NOCLDWAIT 0x00000100  /* Don't create zombie on child death.  */
#define SA_SIGINFO   0x00000200  /* Invoke signal-catching function with
				    three arguments instead of one.  */
#if defined __USE_XOPEN_EXTENDED || defined __USE_MISC
# define SA_ONSTACK   0x00000001 /* Use signal stack by using `sa_restorer'. */
#endif
#if defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8
# define SA_RESTART   0x00000002 /* Restart syscall on signal return.  */
# define SA_NODEFER   0x00000020 /* Don't automatically block the signal when
				    its handler is being executed.  */
# define SA_RESETHAND 0x00000004 /* Reset to SIG_DFL on entry to handler.  */
#endif
#ifdef __USE_MISC
# define SA_INTERRUPT 0x00000010 /* Historical no-op.  */

/* Some aliases for the SA_ constants.  */
# define SA_NOMASK    SA_NODEFER
# define SA_ONESHOT   SA_RESETHAND
# define SA_STACK     SA_ONSTACK
#endif

/* Values for the HOW argument to `sigprocmask'.  */
#define	SIG_BLOCK     1		 /* Block signals.  */
#define	SIG_UNBLOCK   2		 /* Unblock signals.  */
#define	SIG_SETMASK   4		 /* Set the set of blocked signals.  */

We should really stop with this copy-paste madness and write a small Zig script to autogenerate the constants list for all the platforms. @cImport should be good enough to directly parse the libc headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the correction!

We should really stop with this copy-paste madness and write a small Zig script to autogenerate the constants list for all the platforms.

Pretty much this. While working on this I spent quite a lot of time just hunting/copy-pasting/fixing constants, because, among other reasons, Debian-supplied header files can be quite confusing, with the same constant defined multiple times with different values in different files...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much this. While working on this I spent quite a lot of time just hunting/copy-pasting/fixing constants, because, among other reasons, Debian-supplied header files can be quite confusing, with the same constant defined multiple times with different values in different files...

Yeah, and doing everything by hand is a great way to get copy/paste errors and such. Do you mind opening a ticket about this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I've opened a ticket here.

/// the structs are inconsistent across operating systems, and
/// in C, macros are used to hide the differences. Here we use
/// methods to accomplish this.
pub const Stat = extern struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where shit hits the fan, not only in this PR but in every other definition of Stat.
The code uses this definition for both the libc stat and the kernel stat syscall while there's no guarantee the two structures will match!

As you can see here the kernel uses a different struct layout than the libc one you've used here. The problem is partly masked by the lack of rigorous test on the stat fields (as long as the size field is correctly placed you won't see any failure) and partly by musl libc default to 64bit stat and their choice of adopting the kernel definition.

It turns out that a while ago musl decided to do the right thing and decouple the two stat definitions while Zig is still lagging behind (and potentially returning garbage).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks bad, however, I'm not sure how to proceed with this.
Should I make a new struct for the kernel's version of stat so that the library code can use it when it's finally get decoupled, or should I do something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to fix this mess asap in the next few days.
If you just want to get the port going I'd use something like this:

const kernel_stat = extern struct {
// stat according to the kernel...
};
const libc_stat = extern struct {
// stat64 according to the libc
};
const Stat = kernel_stat;

The unused stat definition will come in handy later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, added a separate kernel_stat struct.

lib/std/os/bits/linux/sparc64.zig Outdated Show resolved Hide resolved
lib/std/os/bits/linux/sparc64.zig Outdated Show resolved Hide resolved
gsr: u64,
fprs: u64,
};
pub const fpregset_t = *fpstate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fpregset_t is defined as follows:

typedef struct
  {
    union {				/* FPU floating point regs */
      unsigned		__ctx(fpu_regs)[32];	/* 32 singles */
      double            __ctx(fpu_dregs)[32];	/* 32 doubles */
      long double	__ctx(fpu_qregs)[16];  /* 16 quads */
    } __ctx(fpu_fr);
    struct __fq     *__ctx(fpu_q);		/* ptr to array of FQ entries */
    unsigned long   __ctx(fpu_fsr);		/* FPU status register */
    unsigned char   __ctx(fpu_qcnt);		/* # of entries in saved FQ */
    unsigned char   __ctx(fpu_q_entrysize);	/* # of bytes per FQ entry */
    unsigned char   __ctx(fpu_en);		/* flag signifying fpu in use */
  } fpregset_t;

tz_dsttime: i32,
};

// TODO I'm not sure if the code below is correct, need someone with more
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of this is actually tested, handleSegfaultLinux has no entry for sparcv9.

lib/std/special/c.zig Outdated Show resolved Hide resolved
// placed starting at [stack-start + 128], so we need to account for that too.
// Ref: System V Application Binary Interface: SPARC Version 9 Processor Supplement
// Version 1.35, figure 3-16.
// TODO: find a better way to do this.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define "better".
The formula is o6 plus the stack bias (0x7ff) and the reserved area (16 64bit register = 0x80), you can either do the calculation in the asm block or return the sp and perform the addition in using Zig code.

You should also add a mov %%g0, %%fp (tip: you can use %sp/%fp instead of %o7/%i7 to terminate the frame pointer chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I... dunno. Having a variable named starting_stack_ptr points to 128 bytes past the stack top (or bottom?) just feels somewhat hacky to me. Then again, it seems to be only used to extract argc/argv...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, the name is misleading. Any idea for a better name? I'd suggest something like argc_argv_ptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm, changed it to better reflect the intent. argc_argv_ptr is fine since it's the pointer's only purpose, I think.

lib/std/start.zig Outdated Show resolved Hide resolved
@andrewrk
Copy link
Member

andrewrk commented Oct 26, 2020

Nice work, @koachan. I'm happy to merge this when @LemonBoy gives it the thumbs up.

By the way I believe we have been granted some sparc64 hardware to ssh into for experimentation. If anyone involved in the zig project wants access to this, message me and I will get you some credentials.

@koachan
Copy link
Contributor Author

koachan commented Oct 27, 2020

Okay, so I think I've addressed @LemonBoy's points. The CI builds seem to throw a lot of OutOfMemory errors, though.
Should I be concerned about them?

@LemonBoy
Copy link
Contributor

Okay, so I think I've addressed @LemonBoy's points. The CI builds seem to throw a lot of OutOfMemory errors, though.
Should I be concerned about them?

When you change start.zig you have to update the test-stack-traces tests, try running ./zig build --build-file ../build.zig test-stack-traces.
Have you tried running the behaviour tests?

@koachan
Copy link
Contributor Author

koachan commented Oct 28, 2020

Ah, my bad. The tests only shows reached unreachable code on sparc64 but it does fail in a similar way to the CI errors on x86-64. I've updated the line numbers in the expected output part, it should fix the issue.

@koachan
Copy link
Contributor Author

koachan commented Oct 29, 2020

Finally I got some more time to play around with the stage1 compiler, and found something potentially nasty with the stat structs (commit cbc8750).
If I use the kernel version of the struct, after building stage1, it will often crash with something like this whenever I try to run it:

*** stack smashing detected ***: terminated
Aborted

However, if I do something like this:

  • Change the Stat definition into libc_stat;
  • Build stage1 like usual; then
  • Change Stat back to kernel_stat,

The generated stage1 binary seem to run just fine.

A ldd at the stage1 binary shows that it's linked to libc, so maybe it's trying to use the kernel struct in a libc call? What should I do here? Change the Stat definition into the libc one for the time being, or something else?
@LemonBoy @andrewrk

@LemonBoy
Copy link
Contributor

What should I do here? Change the Stat definition into the libc one for the time being, or something else?

You can also define it according to the builtin.link_libc value:

const Stat = if (std.builtin.link_libc) stat_libc else stat_kernel;

@koachan
Copy link
Contributor Author

koachan commented Oct 30, 2020

Okay, added the selection code, thanks!

@andrewrk andrewrk merged commit 909aae8 into ziglang:master Nov 2, 2020
}
};

pub const kernel_stat = extern struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, check the kernel definition.

// Hack to make the stdlib not complain about atime

Arr, this is not needed, just use the correct structure definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, stat vs stat64 distinction is a confusing one for me.

Arr, this is not needed, just use the correct structure definition.

What I meant is that the stdlib reads the timestamps using atime/ctime/mtime methods, not through direct field reads, so if those three methods aren't defined, the compiler will complain with something like:

lib/std/fs/file.zig:300:25: error: type 'u64' not a function
        const atime = st.atime();
                        ^

I guess I could make the stdlib reads the fields directly instead of going through methods like this, but the stat structs for other architectures need to be fixed first...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, submitted a PR at #6986 to fix this.

// argc is stored after a register window (16 registers) plus stack bias
argc_argv_ptr = asm (
\\ mov %%g0, %%i6
\\ add %%o6, 2175, %[argc]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koachan I'm very late to the party here, but any chance you'd be able to explain the 2175 value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So on sparc64 systems:

  • The first 16 registers worth of space (i.e 128 bytes) at the beginning of every frame is reserved for register window save area.
  • The stack pointer (stored in %o6) is "biased" (i.e it points to 2047 bytes below the actual stack).

argc is stored just beyond the save area, which means that it starts at byte 128 of the frame.
Adding the bias to compensate gives us the constant value in the code: 128 + 2047 = 2175.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the stack bias is what tripped me up. Didn't know about that. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-sparc 32-bit and 64-bit SPARC os-linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants