C, bit shift . 2 consecutive shifts

Question

static inline void ext4_ext_store_pblock(struct ext4_extent *ex,
                     ext4_fsblk_t pb)
{
    ex->ee_start_lo = cpu_to_le32((unsigned long) (pb & 0xffffffff));
    ex->ee_start_hi = cpu_to_le16((unsigned long) ((pb >> 31) >> 1) &
                      0xffff);
}

This code is from linux kernel. See last line. it does pb>>31 then >>1 Is this same as pb >> 32 why not do that?

thank you

EDITED: thank you all. sent patch to ext4 maillist

Answer 1

The two approaches are not quite the same thing, depending on the underlying data types. The standard (C11, 6.5.7 Bitwise shift operators) states that:

If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.

Hence, if you were to shift a 32-bit wide integer type 32 bits to the right, it would result in undefined behaviour. However, shifting it 31 bits and then another bit is well defined.

That particular point, while a reason why someone may do this, is probably moot in this particular case. Given your type is unsigned long long (guaranteed 64 bits width), there should be no difference between (x >> 31) >> 1 and x >> 32.

However, if some platform actually defines ext4_fsblk_t as a 32-bit type (or if it's carried forward from an earlier implementation which allowed a 32-bit type ^(a)), you would find yourself having to resort to the two-stage shift to guarantee defined behaviour.

---

^(a): There was an interim stage between ext3 and ext4 which allowed ext3 to move to 48 bits with its extents, a stepping stone on the way to ext4 with its 64-bit data types. Prior to that, 8TB was the practical limit to file systems.

It's possible, though I haven't confirmed, that this code snippet is a hangover from that transition, before the underlying data types went up to 64 bits wide.

The type that came to be used, sector_t, was conditionally defined as either u64 or unsigned long, which may explain why the code is there in the first place. In situations where it was defined as the latter, a two-stage shift may have been needed.

However, given that ext4 has left this interim stage behind, I'm not sure the two-stage shift is needed any more. Best bet is to raise a change request and see if it gets shot down by the developers :-)

Answer 2

I could imagine that the code has ben copied from ext3 where ext3_fsblk_t is defined as unsigned long (32 bits). In this case paxdiablo's argument was correct:

The standard (C11, 6.5.7 Bitwise shift operators) states that:

If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.

Hence, if you shift a 32-bit integer 32 bits to the right, it's undefined. However, shifting it 31 bits and then another bit is defined.

For ext4_fsblk_t is defined as unsigned long long and long long is in turn guaranteed to be 64 bit this code does not make sense anymore.