[Guy Harris <guy@xxxxxxxxxxxx>]

On Sep 2, 2008, at 2:04 PM, Maynard, Chris wrote:

> Jaap,
> I assume this is the line it's complaining about?:
>
> fcs_ok = (fcs == ieee802154_crc_tvb(tvb,
> tvb_reported_length(tvb)-IEEE802154_FCS_LEN));
>
> At first glance, there doesn't seem to be anything wrong with the
> comparison since fcs is a guint16 and that's exactly what
> crc16_ccitt_tvb_seed() returns; however, because the comparison is
> actually the following:
>
> fcs_ok = (fcs == (crc16_ccitt_tvb_seed(tvb,
> tvb_reported_length(tvb)-IEEE802154_FCS_LEN, IEEE802154_CRC_SEED) ^
> IEEE802154_CRC_XOROUT));
>
> I think the compiler is interpreting (blah ^  
> IEEE802154_CRC_XOROUT) ...
> where IEEE802154_CRC_XOROUT is defined as 0xFFFF ... as the equivalent
> of (~blah) and that's where your warning is coming from.

At least as I read ANSI X3.159-1989 (i.e., C89), on the platforms on  
which we run (where "int" and "unsigned int" are 32 bits and "short  
int" and "unsigned short int" are 16 bits):

	0xFFFF has type "int";

	in an expression of the type

		{unsigned short int} ^ {int}

	the {unsigned short int} value is converted to int before being XORed  
with the {int} value;

so

	
	(crc16_ccitt_tvb_seed(tvb, tvb_reported_length(tvb)- 
IEEE802154_FCS_LEN, IEEE802154_CRC_SEED) ^ IEEE802154_CRC_XOROUT)

first widens the result of crc16_ccitt_tvb_seed() (which is a uint16,  
i.e. unsigned short int) to an int (which means no sign extension, as  
a uint16 has no sign) and then XORs it with 0xFFFF.  That's not ~-ing  
it, as it only flips the lower 16 bits.

Then, again as I read C89:

	in an expression of the type

		{unsigned short int} == {int}

	the {unsigned short int} value is converted to int before being  
compared with the {int} value

so, in the expression

	fcs == (blah blah blah)

"fcs" is widened to an int (again, no sign extension) and then  
compared with the result.

Searching for information about this found

	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10226

where all of the code they show involves the ~ operator, not XORing.   
The list of messages for that bug is a bit confusing (perhaps because  
some messages include stuff from the previous messages), but I think  
the first comment is

> When two unsigned shorts (16 bit) variables are compared with one  
> being
> inverted the comparison will fail.
> The following code should pass, while it does generate a warning it  
> should
> instead just work.
>
>    unsigned short A = 0xDEAD;
>    unsigned short B;
>    B = ~A;
>    if ( B == ~A) {
>       printf("Pass\n");
>    }
>    else {
>       printf("Fail\n");
>    }
>
> It compares 0xFFFFDEAD == 0x0000DEAD which fails

followed by

> Apparently, you didn't understand the warning. The C standard mandates
> that "~A" will promote A to int first. So gcc is behaving correctly
> here. Any suggestions on how the wording of the message could be
> improved to be clearer?

and

> I'll leave the final decision to the language lawyers, but I don't  
> think
> this is a bug in GCC.  The ~ operator is subject to integer promotion,
> so with the implicit conversions the expression becomes:
>   if ((int) B == ~((int) A))
> which is indeed false in the example above.
>
> In fact, I see the following warning when compiling with -Wall (GCC  
> 3.3):
> warning: comparison of promoted ~unsigned with unsigned
>
> Perhaps "if (B == (unsigned short) ~A)" will behave as you expect.


followed by

> Actually I understood that the warning was tied to that error.
>
> I would suggest:
> warning : ~ operator caused promotion of unsigned short to int
>
> Interestingly: Sun CC passes and Microsoft Fails without warning.

In the case of

	unsigned short A = 0xDEAD;
	unsigned short B;
	B = ~A;
	if ( B == ~A) {
		printf("Pass\n");
	}
	else {
		printf("Fail\n");
	}

on a 32-bit-int platform A (0xDEAD) gets promoted to  
"int" (0x0000DEAD) and then ~ed (0xFFFF2152) and then assigned to a  
"unsigned short" (0x2152).  That "unsigned short" is then compared  
with the un-shortened int, and the "unsigned short" gets widened to an  
"int" first, so 0x2152 gets promoted to 0x00002152, and then compared  
with ~A, which is 0xFFFF2152, and that comparison fails.  In fact,  
it'll always fail, because the comparison will be 0x0000XXXX with  
0xFFFFXXXX.

However, in our case, I think the compiler is over-enthusiastically  
turning XORing with 0xFFFF into ~ing - a simplified version would be

	unsigned short A = 0xDEAD;
	unsigned short B;
	B = A ^ 0xFFFF;
	if ( B == (A ^ 0xFFFF)) {
		printf("Pass\n");
	}
	else {
		printf("Fail\n");
	}

in which case A (0xDEAD) gets promoted to "int" (0x0000DEAD) and then  
XORed with 0xFFFF (0x00002152) and then assigned to an "unsigned  
short" (0x2152), blah blah blah.

So I think this is a GCC bug; you might want to file a bug either  
against Debian or against GCC and note that "{unsigned short} ^  
0xFFFF" is not equivalent to "~{unsigned short}" in an "int longer  
than short" environment (the first of those doesn't flip the upper N  
bits of the promoted "unsigned short", the second of those does).