ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
34.37k stars 2.51k forks source link

implement variadic builtins (i.e. `@cVaStart`) for aarch64-linux #15389

Open mitchellh opened 1 year ago

mitchellh commented 1 year ago

Zig Version

0.11.0-dev.2661+d2a799c65

Steps to Reproduce and Observed Behavior

You can see it explicitly disabled here:

https://github.com/ziglang/zig/blob/a774f9334473822fd7c6c828dd3fb873788b2f72/lib/std/builtin.zig#L685

It is noted as disabled due to miscompilations but I'm not sure if there is a tracking issue for this. I couldn't find it.

Expected Behavior

Works!

kubkon commented 1 year ago

Are we positive this is a linker-related issue? I have to admit I don't understand how this could be the case.

ifreund commented 1 year ago

I wonder if this is related to the fact that va_list in C is a plain struct type rather than an array type of length 1 containing a struct as is commonly the case on other architectures such as x86_64:

Aarch64 (source)

typedef struct  va_list {
    void * stack; // next stack param
    void * gr_top; // end of GP arg reg save area
    void * vr_top; // end of FP/SIMD arg reg save area
    int gr_offs; // offset from  gr_top to next GP register arg
    int vr_offs; // offset from  vr_top to next FP/SIMD register arg
} va_list;

x86_64 (source)

typedef struct {
    unsigned int gp_offset;
    unsigned int fp_offset;
    void *overflow_arg_area;
    void *reg_save_area;
} va_list[1];

This difference does not affect the memory layout of the type but may affect how an argument of type va_list is passed to a function. The x86_64 version is an array so it will always be passed as a pointer to e.g. vprintf()

int vprintf(const char *format, va_list ap);

However it seems to me this may differ for Aarch64. Currently in Zig we have to write *std.builtin.VaList in order to pass a va_list as an argument on x86_64.

https://github.com/ziglang/zig/blob/423d7b848b1953173df99fde1f83166dc68c2a2c/test/behavior/var_args.zig#L167-L171

I think this may cause issues on other architectures (e.g. Aarch64) that do not define va_list as either a pointer or an array type though.

It could be the case that there is no way to define std.builtin.VaList correctly in "userspace" Zig for all architectures and that we need to add some new special c_va_list type for the language.

andrewrk commented 1 year ago

Not a miscompilation because that compile error is preventing one. It is a bug however because it is documented to work but it does not.

fawzi commented 9 months ago

I really think it is as ifreund explained, and I see no way out that does not partially break current code. The only way out I can see is need to have a c_va_list, and having a function that transforms @cVaList to a c_va_list. We have to represent what is passed around (either as ? or as struct), and something that can be used to reserve the correct amount of memory. In zig, as far as I know you you will need two types, you cannot get away with one as in C, and assuming it is always a pointer as we do now will not cut it on linux aarch64.