ando@OpenLDAP.org writes:
config.c 1.506 -> 1.507 silence signedness warnings
} else if ( rc == ARG_BAD_CONF ) {
} else if ( (unsigned long)rc == ARG_BAD_CONF ) {
That breaks working code. Never shut up signedness warnings without working through what the expressions do, it's too easy to go wrong.
rc is an int. ARG_BAD_CONF = 0xdead0000: an unsigned int if int is 32-bit. Consider 32-bit int, 64-bit long, with rc == (int)ARG_BAD_CONF: rc < 0 so (unsigned long)rc sign-extends to 0xffffffffdead0000, but ARG_BAD_CONF itself is positive so it is not sign-extended.
The old code worked as far as I can tell: It converted rc to unsigned int, yielding the original ARG_BAD_CONF.
The same functinality change happens with the other changes below that one, I haven't looked at whether that is a fix of broken code or if it breaks working code. (Or it is breaks broken code in a new way:-)
ando@OpenLDAP.org writes:
config.c 1.506 -> 1.507 silence signedness warnings
} else if ( rc == ARG_BAD_CONF ) {
} else if ( (unsigned long)rc == ARG_BAD_CONF ) {That breaks working code. Never shut up signedness warnings without working through what the expressions do, it's too easy to go wrong.
rc is an int. ARG_BAD_CONF = 0xdead0000: an unsigned int if int is 32-bit. Consider 32-bit int, 64-bit long, with rc == (int)ARG_BAD_CONF: rc < 0 so (unsigned long)rc sign-extends to 0xffffffffdead0000, but ARG_BAD_CONF itself is positive so it is not sign-extended.
My PC is 32-bit, so int is 32-bit. If I do
int rc = 0xdead0000;
(0xdead0000 == (unsigned long)rc) is true. Am I doing anything wrong?
The old code worked as far as I can tell: It converted rc to unsigned int, yielding the original ARG_BAD_CONF.
The same functinality change happens with the other changes below that one, I haven't looked at whether that is a fix of broken code or if it breaks working code. (Or it is breaks broken code in a new way:-)
The other changes should eb just fine, since the values of the signed variables are expected to be non-negative, and this test is done without casting.
p.
masarati@aero.polimi.it writes:
My PC is 32-bit, so int is 32-bit. If I do
int rc = 0xdead0000;(0xdead0000 == (unsigned long)rc) is true. Am I doing anything wrong?
Yes, I explained how. Test (0xdead0000 == (unsigned long long)rc) instead, then you'll see the problem with casting to a wider type than int.
The same functinality change happens with the other changes below that one, I haven't looked at whether that is a fix of broken code or if it breaks working code. (Or it is breaks broken code in a new way:-)
The other changes should eb just fine, since the values of the signed variables are expected to be non-negative, and this test is done without casting.
Let's see, this makes explicit the integer promotion which already happened since slap_mask_t is unsigned long and *iptr is int:
- if ( *iptr == aux[i].mask ) { + if ( (slap_mask_t)*iptr == aux[i].mask ) {
Can (*iptr & 0x80000000) == 0x80000000? If so, this code was buggy just the same way your 0xdead0000 patch is - and the new warning just hides the bug. Casting both *iptr and mask to unsigned int would be better, I expect.
The same applies to the next patch, for ival.
The real fix would be to use unsigned types, and preferably the same types everywhere, but changing the signedness of flags requres careful review of all code using those flags, looking for effects like this patch.
-
Hallvard B Furuseth -
masaratiļ¼ aero.polimi.it