Re: [Libguestfs] [libnbd PATCH v3 03/19] socket activation: avoid manipulating the sign bit

On 3/23/23 14:59, Eric Blake wrote: Thanks, I'll include it then. The reason I'm particularly pedantic about hygiene here is the following. Quite frequently, you'll see a pattern like: flags &= ~FLAG_MASK_WHATEVER; Unfortunately, whether this is correct or bogus cannot be determined at face value. I've seen *multiple bugs* where: - "flags" was of type "unsigned long" or "unsigned long long" (that is, it had an integer conversion rank higher than that of "int"), AND (: - FLAG_MASK_WHATEVER had type int (such as from #define FLAG_MASK_WHATEVER 0x1), OR - FLAG_MASK_WHATEVER had type unsigned int (such as from #define FLAG_MASK_WHATEVER 0x1u)). Now, consider what happens: - If FLAG_MASK_WHATEVER has type unsigned int (0x1u), then ~FLAG_MASK_WHATEVER also has type unsigned int, and the assignment ends up clearing even those bits from "flags" that an unsigned long (or unsigned long long) can represent, but an unsigned int cannot. Boom, bug ("flip-before-widen"). - If FLAG_MASK_WHATEVER has type int (0x1), then ~FLAG_MASK_WHATEVER has type int, and it has a negative value (because the sign bit gets negated, so it becomes negative, regardless of one's complement vs. sign-and-magnitude vs. two's complement representation). Assume no padding bits, and two's complement; okay. Then this negative value -- representing the current example in a 32-bit int: 0xFFFF_FFFE, i.e. (-2) -- gets converted to unsigned long, as a part of the usual arithmetic conversions, for the sake of the & operator hidden inside the &= compound assignment operator. The conversion is well-defined, and the result -- per standard -- is (ULONG_MAX+1)-2 == (ULONG_MAX-1) = 0xFFFF_FFFF_FFFF_FFFE. This *happens* to be the proper mask, and it will clear just the right bit from "flags". People that play mostly with assembly and microarchitecture like to call these things "zero-extend" and "sign-extend", and while that's right at the uarch/asm level, it's not proper talk at the C language level IMO. At the C language level: - the first case is a plain bug, - and the second case *happens* not to be a bug, but only because two's complement saves the day (when the initial bit-neg results in (-2)). And most people don't even realize this; they don't understand that they in fact do "flags &= (-2)", and rely on two's complement even for the (-2). (Not to mention that most people assume that 0x1 has type "unsigned int" -- that's not the case. The prefix 0x only *permits* the compiler to consider unsigned types for determining the proper ("minimal") type for the integer constant, it does not "require" the compiler to choose an unsigned type. The "u" suffix does the forcing.) Therefore I like to write flags &= ~(unsigned_type_of_flags)FLAG_MASK_WHATEVER; because then the bit-neg will properly affect all bits of "flags", regardless of the type of FLAG_MASK_WHATEVER. Now, *IF* the C standard starts mandating two's complement, then (a) defining FLAG_MASK_WHATEVER as a signed int (with positive value), such as 0x1, *and* (b) writing just "flags &= ~FLAG_MASK_WHATEVER" will become beneficial. That's because (a) the "happenstance" will be taken out of (~FLAG_MASK_WHATEVER) resulting in the *proper* negative value (which plays well with the modular reduction in the conversion to unsigned integer types), and (b) it won't matter whether "flags" has type unsigned int, unsigned long, or unsigned long long, even without an explicit typecast, as the negative value will automatically be added to UINT_MAX+1, ULONG_MAX+1, or ULLONG_MAX+1 as needed. In other words, this form is going to force "sign extension" (which is what we ultimately want here), but with proper C language foundation. People will still be doing "flags &= (-2)" and not know it, but at least they won't have to know it anymore. Until C17 becomes a project's baseline though, I'll likely stick to the form flags &= ~(unsigned_type_of_flags)FLAG_MASK_WHATEVER; because that's the safest form. Laszlo