Remove C11 requirement for tests and ensure qemu is installed in the RISCV64 CI job

[martin-frbg]

No description provided.

[ChipKerchner]

Maybe instead of adding -std=c11 it should be -std=gnu11?

or

Add this? -D_POSIX_C_SOURCE=199309L

[martin-frbg]

Guess I'll go with the define - still need to check if we should be ending up in that section of common.h at all. Do you happen to know if RISCV64 has a (standard, non-extension) equivalent to x86 "rdtsc" - rdtime ?

[ChipKerchner]

RISC-V has rdcycle but it is NOT included in many older systems. I believe they are working on a standard for that

[ChipKerchner]

#define timer_t          uint64_t

FORCEINLINE timer_t get_rvv_timer()
{
#if 0
  uint64_t val;
  asm volatile("rdcycle %0" : "=r"(val));
  return val;
#else
  struct timespec ts;
  clock_gettime(CLOCK_REALTIME, &ts);
  return (uint64_t)(ts.tv_sec) * 1000000000 + ts.tv_nsec;
#endif
}

[ChipKerchner]

#ifndef _POSIX_C_SOURCE
#define _POSIX_C_SOURCE 199309L
#endif

[ChipKerchner]

Not sure if the define needs to be higher?

_POSIX_C_SOURCE
Defining this macro causes header files to expose definitions as follows:
• The value 1 exposes definitions conforming to POSIX.1-1990 and ISO C (1990).
• The value 2 or greater additionally exposes definitions for POSIX.2-1992.

• The value 199309L or greater additionally exposes definitions for POSIX.1b (real-time extensions).

• The value 199506L or greater additionally exposes definitions for POSIX.1c (threads).

• (Since glibc 2.3.3) The value 200112L or greater exposes definitions corresponding to the POSIX.1-2001 base specification (excluding the XSI extension).

• (Since glibc 2.10) The value 200809L or greater exposes definitions corresponding to the POSIX.1-2008 base specification (excluding the XSI extension).

[ChipKerchner]

Personally I think -std=gnu11 might be better

[martin-frbg]

It's a can of worms as the -std flag would need to be supported by every compiler on every supported platform, and similarly imposing a specific POSIX flavor on all platforms just to ensure RISCV builds with a toolchain will succeed might create "interesting" side effects elsewhere.
I'm more inclined to throw out @Mousius recent addition of these -std=c11 -Werror flags - IMHO if anyone wants them they're welcome to add them to their personal CFLAGS, but it makes little sense to have them for everyone, and in one semi-random subdirectory of the build.

[martin-frbg]

(BTW the whole problem comes about through the fallback path for when there is no "RPCC" procedure defined in the architecture-specific header - this is a helper function probably dating back to GotoBLAS to allow timing of individual level3 drivers. That's why I asked about rdcycle et al., but I'm not even entirely sure building with -DTIMING does anything useful)

[rgommers]

I'm more inclined to throw out @Mousius recent addition of these -std=c11 -Werror flags

In practice, -std=c11 works with any reasonable C compiler (we've being using it in NumPy et al. for a few years); and it may be useful to signal/enforce one is using C11 features. Fortran compilers tend to be more problematic, e.g. -std=legacy isn't universally supported.

-Werror should never be added to any library except for in its own CI, that's an anti-pattern that breaks builds for no good reason.

[martin-frbg]

Well, I can't really force anyone to use a "reasonable" compiler, given that some may not be in a position to install their own. As long as the only requirement for C11 lies in convenience of local variable declaration and slightly better dependency checking, I don't think it should be mandatory for the end user IMHO.

[rgommers]

As long as the only requirement for C11 lies in convenience of local variable declaration and slightly better dependency checking, I don't think it should be mandatory for the end user IMHO.

Sure, no opinion from me on that one either way, just wanted to point out that it works fine for CPython, NumPy et al.

[Mousius]

I'm more inclined to throw out @Mousius recent addition of these -std=c11 -Werror flags

In practice, -std=c11 works with any reasonable C compiler (we've being using it in NumPy et al. for a few years); and it may be useful to signal/enforce one is using C11 features. Fortran compilers tend to be more problematic, e.g. -std=legacy isn't universally supported.

-Werror should never be added to any library except for in its own CI, that's an anti-pattern that breaks builds for no good reason.

Yip, these are just set in tests where having slightly stricter limits seemed useful - not in the main library code itself. I don't think the codebase would ever build consistently with -Werror but it would be nice to be able to use a modern and robust testing framework.

I had hopes we could start to introduce more elaborate C++ tests, but if we can't even guarantee C11 that's going to be tricky 😿

[martin-frbg]

well, "tests" is part of the normal build process of the library so an error there will keep (many) people from successfully installing
and if we're going to introduce a C11 requirement, it doesn't make much sense to me to do it in one somewhat random subdirectory

[Mousius]

well, "tests" is part of the normal build process of the library so an error there will keep (many) people from successfully installing and if we're going to introduce a C11 requirement, it doesn't make much sense to me to do it in one somewhat random subdirectory

As I said, it wasn't random, I had the intention of following up and improving testing further using a modern test framework given I was working on improving the testing around bgemm at the time if I recall. Though I can understand the confusion, there are several different test variants in the codebase.

I acknowledge that tests are part of the default build process though, and I should've added -Werror as a flag or similar, I don't think I can apply it globally because of how many warnings there are though.