On Thu, 9 May 2024 at 07:09, Theodore Ts'o <ty...@mit.edu> wrote:
>
> struct ext2_super {
> ...
> __u32 time_lo;
> __u32 time_high;
> ...
> }
>
> time_t now;
>
> sb->time_low = now;
> sb->time_high = now >> 32;
>
> This is obviously (to a human) correct,
Actually, it's not correct at all.
If time_t is 32-bit, that "now >> 32" is broken due to how C shifts
are defined. For a 32-bit type, only shifts of 0-31 bits are
well-defined (due to hardware differences).
> but because of stupid compiler
> tricks, in order to silence compiler-level and ubsan complaints, this
> got turned into:
>
> sb->time_low = now & 0xffffffff;
> #if (SIZEOF_TIME_T > 4)
> sb->time_high = (now >> 32) & EXT4_EPOCH_MASK;
> #else
> sb->time_high = 0;
> #endi
This is the wrong solution.
But the solution to the undefined shift isn't some #ifdef.
The idiomatic C solution is to do
high_bits = (all_bits >> 16) >> 16;
which works even if 'all_bits' is 32 bit, and the compiler will know
this idiom, and turn it into just 0.
Going the other way is similar:
all_bits = low_bits + ((u64) high_bits << 16) << 16);
and again, the compiler will recognize this idiom and do the right
thing (and if 'all_bits' is only 32-bit, the compiler will optimize
the high bit noise away).
And yes, this is not great, but there you have it: C was designed to
be a high-level assembler, and you see the effects of "this is how
many hardware shifters work". The shift instructions on many (most?)
architectures end up being limited to the word width.
We actually have some helpers for this in <linux/wordpart.h>:
#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
#define lower_32_bits(n) ((u32)((n) & 0xffffffff))
but we don't have that "combine 32 bits into 64 bits" helper. Maybe
worth adding.
Linus
