Non-conforming `printf` prototypes in the GCC/Rust testsuite

[tschwinge]

Just for fun, I tried enabling GCC/Rust in my GCC/nvptx target testing (see https://gcc.gnu.org/wiki/nvptx).

As you'd hope, it generally works.

One issue, however, is that in our hacked-up printf test cases, we use:

extern "C" {
    fn printf(s: *const i8, ...);
}

..., which GCC/nvptx turns into:

.extern .func printf (.param.u64 %in_ar0, .param.u64 %in_ar1);

In contrast, the libc one is declared/defined as:

.visible .func (.param .u32 %value_out) printf (.param .u64 %in_ar0, .param .u64 %in_ar1);

(That is, with the expected int return type; don't ask me why unsigned .u32 instead of signed .s32, but apparently that's OK.)

Therefore, all those execution tests FAIL:

error   : Prototype doesn't match for 'printf' [...]
nvptx-run: cuLinkAddData failed: unknown error (CUDA_ERROR_UNKNOWN, 999)

Should I fix all those up like (a):

 extern "C" {
-   fn printf(s: *const i8, ...);
+   fn printf(s: *const i8, ...) -> i32;
 }

..., which should be correst most of all times, or (b):

+pub type c_int = i32;
 extern "C" {
-   fn printf(s: *const i8, ...);
+   fn printf(s: *const i8, ...) -> c_int;
 }

..., to make that more obvious, or (c) something else?

(See https://doc.rust-lang.org/stable/std/ffi/type.c_int.html.)

[CohenArthur]

I think either solution is ok. c_int will not always resolve to an i32 depending on the target. I'm not sure how this will affect the testsuite on some weirder architectures. I think it's definitely more correct to declare printf as returning an int32_t than nothing at all, but I'm afraid we'll run into some weird build failures on some systems.

[tschwinge]

Given libc int printf(const char *restrict format, ...);, I think my preference is to actually turn our prototypes into the most-correct (per my understanding) fn printf(format: *const c_char, ...) -> c_int;, alongside with local definitions of c_char and c_int. The latter may then be removed once we've got them defined properly. That is:

+// See <https://github.com/Rust-GCC/gccrs/issues/1887> "Non-conforming `printf` prototypes in the GCC/Rust testsuite".
+pub type c_char = i8;
+pub type c_int = i32;
 extern "C" {
-   fn printf(s: *const i8, ...);
+   fn printf(format: *const c_char, ...) -> c_int;
 }

Well, like here: https://docs.rs/libc/latest/libc/fn.printf.html.

Sounds good to you?

[CohenArthur]

Yes, that looks good :) I like it. Do you want to do the PR or leave it as a good-first-pr @tschwinge ?

[tschwinge]

Either way is fine for me -- but I'm not sure, is this a good-first-pr given that it's just some repetitive, tedious editing?

[CohenArthur]

Either way is fine for me -- but I'm not sure, is this a good-first-pr given that it's just some repetitive, tedious editing?

In that case, it might just be easier to sed the whole testsuite and not care about pub type c_int = i32, and just return i32 when declaring printf.

[powerboat9]

Can we define __c_int as a builtin type, like i32 or i8? Perhaps gated behind a compiler flag.

[CohenArthur]

Can we define __c_int as a builtin type, like i32 or i8? Perhaps gated behind a compiler flag.

I think this could cause issues down the line. The definition is in the core crate, we just need to get there :) for now I think we should do as we have done and include the necessary core items for our testcases - in this case, type c_int = i32