Check assumptions on integer implementation at compile time

[sipa]

A compile-time check is implemented in a new src/assumptions.h which verifies several aspects that are implementation-defined in C:

• size of bytes
• conversion between unsigned and (negative) signed types
• right-shifts of negative signed types.

[sipa]

It seems fairly easy to turn this into a compile time check, by using (that_expression - 1) as the size of array.

However, maybe we want to include endianness into the check, and I'm not sure that can be done at compile time.

[gmaxwell]

as the size of array.

Var arrays are C99 and "optional" in later standards and not supported by MSVC.

[sipa]

@gmaxwell Yes, but this isn't relying on vararrays - the expression is a compile-time constant.

In fact, it has to be constant. A vararray of negative size could compile. So you have to do something like define a struct with a member array (where a vararray isn't allowed).

[sipa]

@gmaxwell this seems to work in gcc/clang/icc/msvc: https://godbolt.org/z/6dsar5

[gmaxwell]

Yeah, I tested it with GCC 2.95 on i386 and tinyc and it seems to be happy there too. One downside is that when it fails you get an extremely opaque error.

[sipa]

Added conversions from signed to signed, and a check for CHAR_BIT. Anything else? Do we have any unusual assumptions on the size of int etc?

[real-or-random]

@sipa Are you aware of #792 ? :P

[real-or-random]

One downside is that when it fails you get an extremely opaque error.

We could use static_assert if it's supported by the compiler but I don't think it's worth the additional #ifdefs. These errors should be super rare, so it's acceptable that they're somewhat opaque. And it's probably better to have a compiler error instead of a runtime error.

I read up a little bit on these techniques. The issues with VLAs was something else: In some cases this array trick silently didn't do anything when the expression is non-constant, e.g., when you have a compiler like GCC that supports VLAs as an extension and you happen to pass a non-constant value, then it can't be a compiler error even if the size is negative. This hit the Linux kernel for example.

But what we do here is robust against this failure mode because VLAs are never supported on a file level / global scope. I verified this in godbolt. Even GCC errors out if you pass a non-constant value. (Clang fails at another stage but the message is pretty harsh: fields must have a constant size: 'variable length array in structure' extension will never be supported :D) The difference in the Linux kernel is probably that they don't want to collect all the assumptions in a single place and they want a statement instead.

Approach ACK

Maybe we want to move the assumptions to a separate header like in Bitcoin Core (https://github.com/bitcoin/bitcoin/blob/master/src/compat/assumptions.h). That's cleaner and it's easier to point people to the file, e.g., in the README.

[gmaxwell]

A separate file might make it marginally more likely to get a more useful error message (e.g. telling you what file it was in) from more compilers.

[sipa]

A separate file might make it marginally more likely to get a more useful error message (e.g. telling you what file it was in) from more compilers.

That's a good point.

Moved.

[gmaxwell]

You might want to a comment above it that says something like:

/* This library, like most software, relies on a number of compiler implementation defined (not undefined) behaviours. Although the behaviours we require are essentially universal we test them specifically here to reduce the odds of experiencing an unwelcome surprise. */

[sipa]

@gmaxwell Added.

[gmaxwell]

Include it in gen_context.c too. Sorry for dribbling changes.

[sipa]

Done. Also added to bench_internal.c, tests_exhaustive.c, and valgrind_ctime_test.c.

[gmaxwell]

ACK fdb6dbc

[real-or-random]

Added conversions from signed to signed, and a check for CHAR_BIT. Anything else? Do we have any unusual assumptions on the size of int etc?

ACK except nit. We should then add more assumptions from #792 later.

[sipa]

@real-or-random Sounds good.

[gmaxwell]

ACK 7c06899

[real-or-random]

ACK 7c06899 code review and tested