Wireshark-dev: Re: [Wireshark-dev] packet-ieee802154.c compilation error
Date Index · Thread Index · Other Months · All Mailing Lists
Date Prev · Date Next · Thread Prev · Thread Next
From: Guy Harris <guy@xxxxxxxxxxxx>
Date: Wed, 3 Sep 2008 02:46:53 -0700

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).