Hi C-folks,

Is the code listed below valid C?
Specifically, is "i < 256" valid?

I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?

Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.

I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.

None of the compilers gave a warning...

Does this phenomenon have a name?
Thanks in advance,
Howerd

#include <stdio.h> /* printf */
#include <stdlib.h>
#include <stdint.h>

int main()
{ uint8_t i;

// below is an infinite loop :
for (i = 0; i < 256; i++)
{
if ((i % 16) == 0) printf("\n");
printf("%i ", i);
}
}

On 08/11/2021 11:11, Howerd Oakford wrote:
> Hi C-folks,
>
> Is the code listed below valid C?
> Specifically, is "i < 256" valid?
>
> I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
>
> Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
>
> I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
>
> None of the compilers gave a warning...
>
> Does this phenomenon have a name?
>
> Thanks in advance,
> Howerd
>
> #include <stdio.h> /* printf */
> #include <stdlib.h>
> #include <stdint.h>
>
> int main()
> {
> uint8_t i;
>
> // below is an infinite loop :
> for (i = 0; i < 256; i++)
> {
> if ((i % 16) == 0) printf("\n");
> printf("%i ", i);
> }
> }
>

The experts will shortly be along to tell you what's what.

But is there any reason to define i as an 8-bit value rather than an
ordinary int?

It causes problems here as it cannot represent values above 255, so will
ALWAYS be less than 256. The loop cannot terminate, as written.

Just use an int.

On Monday, November 8, 2021 at 12:22:59 PM UTC+1, Bart wrote:
> On 08/11/2021 11:11, Howerd Oakford wrote:
> > Hi C-folks,
> >
> > Is the code listed below valid C?
> > Specifically, is "i < 256" valid?
> >
> > I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
> >
> > Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
> >
> > I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
> >
> > None of the compilers gave a warning...
> >
> > Does this phenomenon have a name?
> >
> > Thanks in advance,
> > Howerd
> >
> > #include <stdio.h> /* printf */
> > #include <stdlib.h>
> > #include <stdint.h>
> >
> > int main()
> > {
> > uint8_t i;
> >
> > // below is an infinite loop :
> > for (i = 0; i < 256; i++)
> > {
> > if ((i % 16) == 0) printf("\n");
> > printf("%i ", i);
> > }
> > }
> >
> The experts will shortly be along to tell you what's what.
>
> But is there any reason to define i as an 8-bit value rather than an
> ordinary int?
>
> It causes problems here as it cannot represent values above 255, so will
> ALWAYS be less than 256. The loop cannot terminate, as written.
>
> Just use an int.

Hi Bart,

Thanks for your quick response.

Yes - the fix is easy - use a bigger integer, or a smaller array :-)
But the real issues here are :
1. that this is a disastrous bug in code that I wrote, and
2. that was not detected by the compiler or any Linting tools that we are using (Understand and Eclair).

The actual code is parsing a file one line at a time. I allocated a 256 byte array for the line, and a uint8_t for the pointer.
I was implicitly using the wraparound from 255 to 0 to make sure that the pointer never strays outside the array.

To find that if we receive a file with 256 or more bytes on a line it will cause an infinite loop crash is bad enough, but that this is not noticed by any of the tools we are using is simply astonishing. A colleague spotted this by doing a very thorough unit test on the code.

Presumably this is legal code - I await some expert judgements on this ...

Cheers,
Howerd

On Monday, November 8, 2021 at 9:06:06 AM UTC-3, how...@gmail.com wrote:
> On Monday, November 8, 2021 at 12:22:59 PM UTC+1, Bart wrote:
> > On 08/11/2021 11:11, Howerd Oakford wrote:
> > > Hi C-folks,
> > >
> > > Is the code listed below valid C?
> > > Specifically, is "i < 256" valid?
> > >
> > > I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
> > >
> > > Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
> > >
> > > I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
> > >
> > > None of the compilers gave a warning...
> > >
> > > Does this phenomenon have a name?
> > >
> > > Thanks in advance,
> > > Howerd
> > >
> > > #include <stdio.h> /* printf */
> > > #include <stdlib.h>
> > > #include <stdint.h>
> > >
> > > int main()
> > > {
> > > uint8_t i;
> > >
> > > // below is an infinite loop :
> > > for (i = 0; i < 256; i++)
> > > {
> > > if ((i % 16) == 0) printf("\n");
> > > printf("%i ", i);
> > > }
> > > }
> > >
> > The experts will shortly be along to tell you what's what.
> >
> > But is there any reason to define i as an 8-bit value rather than an
> > ordinary int?
> >
> > It causes problems here as it cannot represent values above 255, so will
> > ALWAYS be less than 256. The loop cannot terminate, as written.
> >
> > Just use an int.
> Hi Bart,
>
> Thanks for your quick response.
>
> Yes - the fix is easy - use a bigger integer, or a smaller array :-)
> But the real issues here are :
> 1. that this is a disastrous bug in code that I wrote, and
> 2. that was not detected by the compiler or any Linting tools that we are using (Understand and Eclair).
>
> The actual code is parsing a file one line at a time. I allocated a 256 byte array for the line, and a uint8_t for the pointer.
> I was implicitly using the wraparound from 255 to 0 to make sure that the pointer never strays outside the array.
>
> To find that if we receive a file with 256 or more bytes on a line it will cause an infinite loop crash is bad enough, but that this is not noticed by any of the tools we are using is simply astonishing. A colleague spotted this by doing a very thorough unit test on the code.
>
> Presumably this is legal code - I await some expert judgements on this ....

This kind of bug is easily detected by lint programs.

I also tried in clang:

warning: result of comparison of constant 256 with expression of type
'uint8_t' (aka 'unsigned char') is always true [-Wtautological-constant-out-of-range-compare]

On 08/11/2021 12:11, Howerd Oakford wrote:
> Hi C-folks,
>
> Is the code listed below valid C?
> Specifically, is "i < 256" valid?

It is valid, it is just not helpful. A uint8_t variable will /always/
be less than 256, so the check is always "true".

>
> I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
>
> Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
>

The uint8_t variable counts up to 255, then wraps to 0 - overflow is
always wrapping for unsigned types (unlike for signed types).
Basically, you loop is like asking to run something every hour until 13
o'clock.

> I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
>

Good.

> None of the compilers gave a warning...
>

gcc will give you a warning if you ask it. The flag is "-Wtype-limits",
and it is enabled with "-Wextra". It's a good idea to have "-Wall
-Wextra" enabled while you are getting the hang of things, then perhaps
disable some warnings if you find they conflict with your style and give
false positives.

> Does this phenomenon have a name?
>
> Thanks in advance,
> Howerd
>
> #include <stdio.h> /* printf */
> #include <stdlib.h>
> #include <stdint.h>
>
> int main()

In c.l.c., we have a long tradition of complaining when people declare
"main" incorrectly. In C (until C23 comes out), if you want to declare
"main" with no parameters, you must write "int main(void)" - that's the
usual form for embedded programming.

> {
> uint8_t i;
>
> // below is an infinite loop :
> for (i = 0; i < 256; i++)

It's generally neater to put the variable declaration in the "for" :

for (uint8_t i = 0; i < 256; i++) {

Of course, you'll want to change that type - or the limit!

> {
> if ((i % 16) == 0) printf("\n");
> printf("%i ", i);
> }
> }
>

The big question is, why are you using "uint8_t" at all here? If you
were programming an AVR, I could understand it. But for the ARM you
generally get most efficient code using int32_t or uint32_t for local
variables.

On Monday, November 8, 2021 at 1:17:39 PM UTC+1, Thiago Adams wrote:
> On Monday, November 8, 2021 at 9:06:06 AM UTC-3, how...@gmail.com wrote:
> > On Monday, November 8, 2021 at 12:22:59 PM UTC+1, Bart wrote:
> > > On 08/11/2021 11:11, Howerd Oakford wrote:
> > > > Hi C-folks,
> > > >
> > > > Is the code listed below valid C?
> > > > Specifically, is "i < 256" valid?
> > > >
> > > > I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
> > > >
> > > > Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
> > > >
> > > > I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
> > > >
> > > > None of the compilers gave a warning...
> > > >
> > > > Does this phenomenon have a name?
> > > >
> > > > Thanks in advance,
> > > > Howerd
> > > >
> > > > #include <stdio.h> /* printf */
> > > > #include <stdlib.h>
> > > > #include <stdint.h>
> > > >
> > > > int main()
> > > > {
> > > > uint8_t i;
> > > >
> > > > // below is an infinite loop :
> > > > for (i = 0; i < 256; i++)
> > > > {
> > > > if ((i % 16) == 0) printf("\n");
> > > > printf("%i ", i);
> > > > }
> > > > }
> > > >
> > > The experts will shortly be along to tell you what's what.
> > >
> > > But is there any reason to define i as an 8-bit value rather than an
> > > ordinary int?
> > >
> > > It causes problems here as it cannot represent values above 255, so will
> > > ALWAYS be less than 256. The loop cannot terminate, as written.
> > >
> > > Just use an int.
> > Hi Bart,
> >
> > Thanks for your quick response.
> >
> > Yes - the fix is easy - use a bigger integer, or a smaller array :-)
> > But the real issues here are :
> > 1. that this is a disastrous bug in code that I wrote, and
> > 2. that was not detected by the compiler or any Linting tools that we are using (Understand and Eclair).
> >
> > The actual code is parsing a file one line at a time. I allocated a 256 byte array for the line, and a uint8_t for the pointer.
> > I was implicitly using the wraparound from 255 to 0 to make sure that the pointer never strays outside the array.
> >
> > To find that if we receive a file with 256 or more bytes on a line it will cause an infinite loop crash is bad enough, but that this is not noticed by any of the tools we are using is simply astonishing. A colleague spotted this by doing a very thorough unit test on the code.
> >
> > Presumably this is legal code - I await some expert judgements on this ....
> This kind of bug is easily detected by lint programs.
>
> I also tried in clang:
>
> warning: result of comparison of constant 256 with expression of type
> 'uint8_t' (aka 'unsigned char') is always true [-Wtautological-constant-out-of-range-compare]

Hi Thiago,

Thanks - I think we will have to take a look at clang!!!

Maybe we have some settings wrong in Understand and Eclair...

Does the warning from clang mean that the code is "illegal", or just "not recommended"?

In this case, the code is seriously wrong - causing a system crash if a file contains a single long line.

Cheers,
Howerd

On Monday, November 8, 2021 at 1:59:20 PM UTC+1, David Brown wrote:
> On 08/11/2021 12:11, Howerd Oakford wrote:
> > Hi C-folks,
> >
> > Is the code listed below valid C?
> > Specifically, is "i < 256" valid?
> It is valid, it is just not helpful. A uint8_t variable will /always/
> be less than 256, so the check is always "true".
> >
> > I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
> >
> > Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
> >
> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
> always wrapping for unsigned types (unlike for signed types).
> Basically, you loop is like asking to run something every hour until 13
> o'clock.
> > I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
> >
> Good.
> > None of the compilers gave a warning...
> >
> gcc will give you a warning if you ask it. The flag is "-Wtype-limits",
> and it is enabled with "-Wextra". It's a good idea to have "-Wall
> -Wextra" enabled while you are getting the hang of things, then perhaps
> disable some warnings if you find they conflict with your style and give
> false positives.
> > Does this phenomenon have a name?
> >
> > Thanks in advance,
> > Howerd
> >
> > #include <stdio.h> /* printf */
> > #include <stdlib.h>
> > #include <stdint.h>
> >
> > int main()
> In c.l.c., we have a long tradition of complaining when people declare
> "main" incorrectly. In C (until C23 comes out), if you want to declare
> "main" with no parameters, you must write "int main(void)" - that's the
> usual form for embedded programming.
> > {
> > uint8_t i;
> >
> > // below is an infinite loop :
> > for (i = 0; i < 256; i++)
> It's generally neater to put the variable declaration in the "for" :
>
> for (uint8_t i = 0; i < 256; i++) {
>
> Of course, you'll want to change that type - or the limit!
> > {
> > if ((i % 16) == 0) printf("\n");
> > printf("%i ", i);
> > }
> > }
> >
> The big question is, why are you using "uint8_t" at all here? If you
> were programming an AVR, I could understand it. But for the ARM you
> generally get most efficient code using int32_t or uint32_t for local
> variables.

Hi David,

Thanks! I added -Wall -Wextra and I get a warning now :-)

> The big question is, why are you using "uint8_t" at all here?
Company policy.

> Basically, your loop is like asking to run something every hour until 13 o'clock.
I like the analogy :-)

> you must write "int main(void)" - that's the usual form for embedded programming.
Sorry - typed in haste ;-)

One correction : Eclair does give a warning now, too.

Thanks for your response,

Cheers,
Howerd

Howerd Oakford <howerdo@gmail.com> writes:

> On Monday, November 8, 2021 at 1:17:39 PM UTC+1, Thiago Adams wrote:
>> On Monday, November 8, 2021 at 9:06:06 AM UTC-3, how...@gmail.com wrote:
<snip>
>> > .. that this
>> > is not noticed by any of the tools we are using is simply
>> > astonishing. A colleague spotted this by doing a very thorough unit
>> > test on the code.

>> I also tried in clang:
>>
>> warning: result of comparison of constant 256 with expression of type
>> 'uint8_t' (aka 'unsigned char') is always true
>> [-Wtautological-constant-out-of-range-compare]
>
> Thanks - I think we will have to take a look at clang!!!

gcc warns as well, but you have to ask (it's one of the -Wextra
warnings). Since for most uses I have gcc "turned up to 11" I get that
sort of warning as a matter of course.

--
Ben.

David Brown <david.brown@hesbynett.no> writes:

[...]

> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
> always wrapping for unsigned types (unlike for signed types).

Technically the word is not ``overflow''.

--8<---------------cut here---------------start------------->8---
A computation involving unsigned operands can never overflow, because a
result that cannot be represented by the resulting unsigned integer type
is reduced modulo the number that is one greater than the largest value
that can be represented by the resulting unsigned integer type. [C89,
seção 3.1.2.5]
--8<---------------cut here---------------end--------------->8---

On 11/8/2021 1:59 PM, David Brown wrote:
> On 08/11/2021 12:11, Howerd Oakford wrote:
>> Hi C-folks,
>>
>> Is the code listed below valid C?
>> Specifically, is "i < 256" valid?
>
> It is valid, it is just not helpful. A uint8_t variable will /always/
> be less than 256, so the check is always "true".
>
>>
>> I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
>>
>> Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
>>
>
> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
> always wrapping for unsigned types (unlike for signed types).
> Basically, you loop is like asking to run something every hour until 13
> o'clock.
>
>> I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
>>
>
> Good.
>
>> None of the compilers gave a warning...
>>
>
> gcc will give you a warning if you ask it. The flag is "-Wtype-limits",
> and it is enabled with "-Wextra". It's a good idea to have "-Wall
> -Wextra" enabled while you are getting the hang of things, then perhaps
> disable some warnings if you find they conflict with your style and give
> false positives.

Maybe this is worth an extra comment. Compilers, includong gcc, can
easily detect this because the expression "i < 256" is easy to check at
compile time. The issue would be a bit harder if it were "i < N" where N
would be an unsigned int with value 256 set somewhere else.
For this more general case the advice would be to give extra care to
comparisons between variables of different size and/or signedness.
In some cases compilers warn about this too, but, as an example, the
following is undetected by gcc -Wall -Wextra:

#include <stdio.h> /* printf */
#include <stdlib.h>
#include <stdint.h>

unsigned int imax()
{ return 256;
}

int main()
{ uint8_t i;

// below is an infinite loop :
for (i = 0; i < imax(); i++)
{
if ((i % 16) == 0) printf("\n");
printf("%i ", i);
}
}

[...]
>
> The big question is, why are you using "uint8_t" at all here? If you
> were programming an AVR, I could understand it. But for the ARM you
> generally get most efficient code using int32_t or uint32_t for local
> variables.
>

Or just a plain '[unsigned] int', which defaults to the machine word,
and is meant to be the general purpose integer type that is optimized
for performance in the absence of specific size requirements.
The standard guarantees that 'int' and 'unsigned int' are both at least
16 bit in size.

Meredith Montgomery <mmontgomery@levado.to> writes:
>David Brown <david.brown@hesbynett.no> writes:
>
>[...]
>
>> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
>> always wrapping for unsigned types (unlike for signed types).
>
>Technically the word is not ``overflow''.
>

Technically, the processor sets the "overflow" flag. If that's not
an overflow, I'm not sure what it.

On 11/8/2021 3:41 PM, Scott Lurndal wrote:
> Meredith Montgomery <mmontgomery@levado.to> writes:
>> David Brown <david.brown@hesbynett.no> writes:
>>
>> [...]
>>
>>> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
>>> always wrapping for unsigned types (unlike for signed types).
>>
>> Technically the word is not ``overflow''.
>>
>
> Technically, the processor sets the "overflow" flag. If that's not
> an overflow, I'm not sure what it.
>

The quote from the standard, that you stripped, is accurate:

> A computation involving unsigned operands can never overflow, because a
> result that cannot be represented by the resulting unsigned integer type
> is reduced modulo the number that is one greater than the largest value
> that can be represented by the resulting unsigned integer type. [C89,
> seção 3.1.2.5]

BTW is is almost identical in n2596, 6.2.5 clause 9.

If it "can never overflow", it's a statement that is clear enough.
I'd say the correct term is wrapping.

On 08/11/2021 14:19, Howerd Oakford wrote:
> On Monday, November 8, 2021 at 1:59:20 PM UTC+1, David Brown wrote:
>> On 08/11/2021 12:11, Howerd Oakford wrote:
>>> Hi C-folks,
>>>
>>> Is the code listed below valid C?
>>> Specifically, is "i < 256" valid?
>> It is valid, it is just not helpful. A uint8_t variable will /always/
>> be less than 256, so the check is always "true".
>>>
>>> I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
>>>
>>> Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
>>>
>> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
>> always wrapping for unsigned types (unlike for signed types).
>> Basically, you loop is like asking to run something every hour until 13
>> o'clock.
>>> I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
>>>
>> Good.
>>> None of the compilers gave a warning...
>>>
>> gcc will give you a warning if you ask it. The flag is "-Wtype-limits",
>> and it is enabled with "-Wextra". It's a good idea to have "-Wall
>> -Wextra" enabled while you are getting the hang of things, then perhaps
>> disable some warnings if you find they conflict with your style and give
>> false positives.
>>> Does this phenomenon have a name?
>>>
>>> Thanks in advance,
>>> Howerd
>>>
>>> #include <stdio.h> /* printf */
>>> #include <stdlib.h>
>>> #include <stdint.h>
>>>
>>> int main()
>> In c.l.c., we have a long tradition of complaining when people declare
>> "main" incorrectly. In C (until C23 comes out), if you want to declare
>> "main" with no parameters, you must write "int main(void)" - that's the
>> usual form for embedded programming.
>>> {
>>> uint8_t i;
>>>
>>> // below is an infinite loop :
>>> for (i = 0; i < 256; i++)
>> It's generally neater to put the variable declaration in the "for" :
>>
>> for (uint8_t i = 0; i < 256; i++) {
>>
>> Of course, you'll want to change that type - or the limit!
>>> {
>>> if ((i % 16) == 0) printf("\n");
>>> printf("%i ", i);
>>> }
>>> }
>>>
>> The big question is, why are you using "uint8_t" at all here? If you
>> were programming an AVR, I could understand it. But for the ARM you
>> generally get most efficient code using int32_t or uint32_t for local
>> variables.
>
> Hi David,
>
> Thanks! I added -Wall -Wextra and I get a warning now :-)
>
>> The big question is, why are you using "uint8_t" at all here?
> Company policy.

Your company policy is /bad/. If you have any influence on it, complain.

Using uint8_t here makes your code incorrect - and it is most certainly
not an appropriate way to make sure you don't access the array out of
bounds.

>
>> Basically, your loop is like asking to run something every hour until 13 o'clock.
> I like the analogy :-)
>
>> you must write "int main(void)" - that's the usual form for embedded programming.
> Sorry - typed in haste ;-)
>
> One correction : Eclair does give a warning now, too.
>
> Thanks for your response,
>
> Cheers,
> Howerd
>

On 08/11/2021 14:55, Meredith Montgomery wrote:
> David Brown <david.brown@hesbynett.no> writes:
>
> [...]
>
>> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
>> always wrapping for unsigned types (unlike for signed types).
>
> Technically the word is not ``overflow''.
>

Yes - you are technically correct. But at the the level of discussion
that is most suitable for a thread like this, it is a useful term to
describe what is happening. I don't think quoting the standard will
help the OP.

On 08/11/2021 15:38, Manfred wrote:
> On 11/8/2021 1:59 PM, David Brown wrote:
>>
>> The big question is, why are you using "uint8_t" at all here?  If you
>> were programming an AVR, I could understand it.  But for the ARM you
>> generally get most efficient code using int32_t or uint32_t for local
>> variables.
>>
>
> Or just a plain '[unsigned] int', which defaults to the machine word,
> and is meant to be the general purpose integer type that is optimized
> for performance in the absence of specific size requirements.
> The standard guarantees that 'int' and 'unsigned int' are both at least
> 16 bit in size.
>

He is doing embedded programming, and it is very common for such code to
be explicit about sizes. Anyone who uses "uint8_t" as a local counter
is likely to prefer "int32_t" to "int" in many cases. But of course
both "int" and "unsigned int" would have been suitable, as well as less
commonly used types such as "(u)int_fast16_t".

On Monday, 8 November 2021 at 15:03:31 UTC+2, how...@gmail.com wrote:
> On Monday, November 8, 2021 at 1:17:39 PM UTC+1, Thiago Adams wrote:
> > On Monday, November 8, 2021 at 9:06:06 AM UTC-3, how...@gmail.com wrote:
> > > On Monday, November 8, 2021 at 12:22:59 PM UTC+1, Bart wrote:
> > > > On 08/11/2021 11:11, Howerd Oakford wrote:
> > > > > Hi C-folks,
> > > > >
> > > > > Is the code listed below valid C?
> > > > > Specifically, is "i < 256" valid?
> > > > >
> > > > > I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
> > > > >
> > > > > Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
> > > > >
> > > > > I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
> > > > >
> > > > > None of the compilers gave a warning...
> > > > >
> > > > > Does this phenomenon have a name?
> > > > >
> > > > > Thanks in advance,
> > > > > Howerd
> > > > >
> > > > > #include <stdio.h> /* printf */
> > > > > #include <stdlib.h>
> > > > > #include <stdint.h>
> > > > >
> > > > > int main()
> > > > > {
> > > > > uint8_t i;
> > > > >
> > > > > // below is an infinite loop :
> > > > > for (i = 0; i < 256; i++)
> > > > > {
> > > > > if ((i % 16) == 0) printf("\n");
> > > > > printf("%i ", i);
> > > > > }
> > > > > }
> > > > >
> > > > The experts will shortly be along to tell you what's what.
> > > >
> > > > But is there any reason to define i as an 8-bit value rather than an
> > > > ordinary int?
> > > >
> > > > It causes problems here as it cannot represent values above 255, so will
> > > > ALWAYS be less than 256. The loop cannot terminate, as written.
> > > >
> > > > Just use an int.
> > > Hi Bart,
> > >
> > > Thanks for your quick response.
> > >
> > > Yes - the fix is easy - use a bigger integer, or a smaller array :-)
> > > But the real issues here are :
> > > 1. that this is a disastrous bug in code that I wrote, and
> > > 2. that was not detected by the compiler or any Linting tools that we are using (Understand and Eclair).
> > >
> > > The actual code is parsing a file one line at a time. I allocated a 256 byte array for the line, and a uint8_t for the pointer.
> > > I was implicitly using the wraparound from 255 to 0 to make sure that the pointer never strays outside the array.
> > >
> > > To find that if we receive a file with 256 or more bytes on a line it will cause an infinite loop crash is bad enough, but that this is not noticed by any of the tools we are using is simply astonishing. A colleague spotted this by doing a very thorough unit test on the code.
> > >
> > > Presumably this is legal code - I await some expert judgements on this ...
> > This kind of bug is easily detected by lint programs.
> >
> > I also tried in clang:
> >
> > warning: result of comparison of constant 256 with expression of type
> > 'uint8_t' (aka 'unsigned char') is always true [-Wtautological-constant-out-of-range-compare]
> Hi Thiago,
>
> Thanks - I think we will have to take a look at clang!!!
>
> Maybe we have some settings wrong in Understand and Eclair...
>
> Does the warning from clang mean that the code is "illegal", or just "not recommended"?

It is not illegal to have conditions that are always true (or always false).. The
likelihood that it is not what programmer did want to write is still high.
If it was deliberate then why programmer did not simply write 1 (or 0)
instead of more complex expression that is also always true? That is why
tools warn.

> In this case, the code is seriously wrong - causing a system crash if a file contains a single long line.

C compilers are only on very rare cases outright required to stop
translating the code. So it is mostly about setting those to give
more diagnostics.

On Monday, November 8, 2021 at 4:21:39 PM UTC+1, David Brown wrote:
> On 08/11/2021 14:19, Howerd Oakford wrote:
> > On Monday, November 8, 2021 at 1:59:20 PM UTC+1, David Brown wrote:
> >> On 08/11/2021 12:11, Howerd Oakford wrote:
> >>> Hi C-folks,
> >>>
> >>> Is the code listed below valid C?
> >>> Specifically, is "i < 256" valid?
> >> It is valid, it is just not helpful. A uint8_t variable will /always/
> >> be less than 256, so the check is always "true".
> >>>
> >>> I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?
> >>>
> >>> Normally, I expect a for ... loop to count up to the value in the middle section, but in the case below it creates an infinite loop.
> >>>
> >> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
> >> always wrapping for unsigned types (unlike for signed types).
> >> Basically, you loop is like asking to run something every hour until 13
> >> o'clock.
> >>> I tried it out on GCC ARM, GCC ARM cross compiler using Visual Studio Code, and MS Visual Studio - all three run for ever.
> >>>
> >> Good.
> >>> None of the compilers gave a warning...
> >>>
> >> gcc will give you a warning if you ask it. The flag is "-Wtype-limits",
> >> and it is enabled with "-Wextra". It's a good idea to have "-Wall
> >> -Wextra" enabled while you are getting the hang of things, then perhaps
> >> disable some warnings if you find they conflict with your style and give
> >> false positives.
> >>> Does this phenomenon have a name?
> >>>
> >>> Thanks in advance,
> >>> Howerd
> >>>
> >>> #include <stdio.h> /* printf */
> >>> #include <stdlib.h>
> >>> #include <stdint.h>
> >>>
> >>> int main()
> >> In c.l.c., we have a long tradition of complaining when people declare
> >> "main" incorrectly. In C (until C23 comes out), if you want to declare
> >> "main" with no parameters, you must write "int main(void)" - that's the
> >> usual form for embedded programming.
> >>> {
> >>> uint8_t i;
> >>>
> >>> // below is an infinite loop :
> >>> for (i = 0; i < 256; i++)
> >> It's generally neater to put the variable declaration in the "for" :
> >>
> >> for (uint8_t i = 0; i < 256; i++) {
> >>
> >> Of course, you'll want to change that type - or the limit!
> >>> {
> >>> if ((i % 16) == 0) printf("\n");
> >>> printf("%i ", i);
> >>> }
> >>> }
> >>>
> >> The big question is, why are you using "uint8_t" at all here? If you
> >> were programming an AVR, I could understand it. But for the ARM you
> >> generally get most efficient code using int32_t or uint32_t for local
> >> variables.
> >
> > Hi David,
> >
> > Thanks! I added -Wall -Wextra and I get a warning now :-)
> >
> >> The big question is, why are you using "uint8_t" at all here?
> > Company policy.
> Your company policy is /bad/. If you have any influence on it, complain.
>
> Using uint8_t here makes your code incorrect - and it is most certainly
> not an appropriate way to make sure you don't access the array out of
> bounds.
> >
> >> Basically, your loop is like asking to run something every hour until 13 o'clock.
> > I like the analogy :-)
> >
> >> you must write "int main(void)" - that's the usual form for embedded programming.
> > Sorry - typed in haste ;-)
> >
> > One correction : Eclair does give a warning now, too.
> >
> > Thanks for your response,
> >
> > Cheers,
> > Howerd
> >

Hi David,

> Your company policy is /bad/. If you have any influence on it, complain.
if (unit8_t influence > 256 ) complain(); // ;-)

> Using uint8_t here makes your code incorrect
Yes, it does :-(

I have enabled warnings, and will pay attention to them more than ever now.

Thanks to you and everyone else on c.l.c for your responses :-)

Cheers,
Howerd

Manfred <noname@add.invalid> writes:

> On 11/8/2021 3:41 PM, Scott Lurndal wrote:
>
>> Meredith Montgomery <mmontgomery@levado.to> writes:
>>
>>> David Brown <david.brown@hesbynett.no> writes:
>>>
>>> [...]
>>>
>>>> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
>>>> always wrapping for unsigned types (unlike for signed types).
>>>
>>> Technically the word is not ``overflow''.
>>
>> Technically, the processor sets the "overflow" flag. If that's not
>> an overflow, I'm not sure what it.
>
> The quote from the standard, that you stripped, is accurate:
>
>> A computation involving unsigned operands can never overflow, because a
>> result that cannot be represented by the resulting unsigned integer type
>> is reduced modulo the number that is one greater than the largest value
>> that can be represented by the resulting unsigned integer type. [...]

(That exact text appears in C90 section 6.1.2.5, paragraph 5.)

> BTW is is almost identical in n2596, 6.2.5 clause 9.

The C90 text was revised slightly in C99 and is unchanged up to n2596.

> If it "can never overflow", it's a statement that is clear enough.

Note that the original code doesn't have any computations
involving unsigned operands, only signed operands. The code does
have /conversions/ from one integer type to another, but
conversions have their own rules, and no conversion between
integer types, either signed or unsigned or both, is "overflow"
in the sense that the C standard uses the term. Converting to
an unsigned type (from another integer type) is always exactly
defined by the C standard, depending only on the value being
converted and the implementation-defined width of the unsigned
type that is the target type of the conversion.

> I'd say the correct term is wrapping.

It's a common term and one that is often used, but I wouldn't
go so far as to call it "the correct" term.

scott@slp53.sl.home (Scott Lurndal) writes:
> Meredith Montgomery <mmontgomery@levado.to> writes:
>>David Brown <david.brown@hesbynett.no> writes:
>>
>>[...]
>>
>>> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
>>> always wrapping for unsigned types (unlike for signed types).
>>
>>Technically the word is not ``overflow''.
>
> Technically, the processor sets the "overflow" flag. If that's not
> an overflow, I'm not sure what it.

Which processor? Who says it even has an overflow flag?

In this context, "overflow" is determined by the C standard, not by the
CPU.

--
Keith Thompson (The_Other_Keith) Keith.S.Thompson+u@gmail.com
Working, but not speaking, for Philips
void Void(void) { Void(); } /* The recursive call of the void */

On 11/8/2021 7:33 PM, Tim Rentsch wrote:
> Manfred <noname@add.invalid> writes:

>
>> If it "can never overflow", it's a statement that is clear enough.
>
> Note that the original code doesn't have any computations
> involving unsigned operands, only signed operands.

On 08/11/2021 12:11, Howerd Oakford wrote:

> uint8_t i;

The code does
> have /conversions/ from one integer type to another, but
> conversions have their own rules, and no conversion between
> integer types, either signed or unsigned or both, is "overflow"
> in the sense that the C standard uses the term. Converting to
> an unsigned type (from another integer type) is always exactly
> defined by the C standard, depending only on the value being
> converted and the implementation-defined width of the unsigned
> type that is the target type of the conversion.
>

Le 08/11/2021 à 12:11, Howerd Oakford a écrit :
> Is the code listed below valid C?
> Specifically, is "i < 256" valid?

It is.

> I ask this because i is an 8 bit unsigned integer, and I would like to know if it is legal to compare it with a value that is outside the range of i?

In C, it is.
You get no warning because C implicitly does "integer promotion". Thus
here i is promoted to an int in the 'i < 256' expression. The expression
is perfectly "legal". But i itself is in the [0..255] range, and C rules
are such that arithmetics on i can be considered modulo 256.

You can consider that it's the equivalent of:

int i;
for (i = 0; (i % 256) < 256; i++) { ... }

or, to be more exact:
for (i = 0; i < 256; i = (i + 1) % 256) { ... }

Neither forms will give a compiler warning either.
And of course, it's an infinite loop.

Yes C arithmetics is full of "traps", so you should definitely learn
about integer promotion and integer arithmetics in C.

So for such a loop, you can either use a wider int type for i, or, if i
should absolutely be a uint8_t for some reason, you have no other choice
than doing the following:

uint8_t i;
for (i = 0; ; i++)
{
if ((i % 16) == 0) printf("\n");
printf("%i ", i);

if (i == 255) break;
}

As you can see, the condition in the for statement is omitted here, as
no expression using 'i' can work properly as one.

Am 08.11.2021 um 15:41 schrieb Scott Lurndal:
> Meredith Montgomery <mmontgomery@levado.to> writes:
>> David Brown <david.brown@hesbynett.no> writes:
>>
>> [...]
>>
>>> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
>>> always wrapping for unsigned types (unlike for signed types).
>>
>> Technically the word is not ``overflow''.
>>
>
> Technically, the processor sets the "overflow" flag.

No, it sets the carry-flag. The overflow-flag is set when
the sign changes without the carry being set, i.e. the
value won't fit in the destination-width.

Le 08/11/2021 à 14:41, Ben Bacarisse a écrit :
> gcc warns as well, but you have to ask (it's one of the -Wextra
> warnings).

Ah, you're right. I had tried only with -Wall.

Interestingly, it will give a warning if 'i' is uint8_t here, but not
for the other 'equivalent' forms I gave in my other post using a '%'
operator.

so for:

int i;
for (i = 0; (i % 256) < 256; i++) {}

GCC doesn't spot any problem. I guess analyzing expressions is a bit
much to ask here.

But as I said, this is still all valid C. The warning here is for
convenience.

David Brown <david.brown@hesbynett.no> writes:
> On 08/11/2021 12:11, Howerd Oakford wrote:
>> Hi C-folks,
>>
>> Is the code listed below valid C?
>> Specifically, is "i < 256" valid?
>
> It is valid, it is just not helpful. A uint8_t variable will /always/
> be less than 256, so the check is always "true".
>
>>
>> I ask this because i is an 8 bit unsigned integer, and I would like
>> to know if it is legal to compare it with a value that is outside the
>> range of i?
>>
>> Normally, I expect a for ... loop to count up to the value in the
>> middle section, but in the case below it creates an infinite loop.
>
> The uint8_t variable counts up to 255, then wraps to 0 - overflow is
> always wrapping for unsigned types (unlike for signed types).
> Basically, you loop is like asking to run something every hour until 13
> o'clock.

That's essentially correct, but it's a bit more complicated for integer
types narrower than int and unsigned int.

You have:

uint8_t i;
...
i++

C has no arithmetic operators for types narrower than int and unsigned
int (and uint8_t is definitely narrower than int, since int is at least
16 bits).

The expression `i++` is equivalent to `i = i + 1`. In `i + 1`, the
value of i is promoted to int. The addition is then down on two
operands of type int, and the result is assigned to i. Assigning an int
value to a uint8_t object requires an implicit conversion. If the
converted value can be represented in the target type (i.e., it's in the
range 0..255), the conversion retains the same value. Otherwise, since
the target type is an unsigned integer type, the conversion converts 256
to 0. (It gets more complicated if the target type is a signed type
like int8_t).

The final result is the same as if it were operating directly on 8-bit
values -- and the compiler might well generate code that does that if
it's more efficient.

[...]

--
Keith Thompson (The_Other_Keith) Keith.S.Thompson+u@gmail.com
Working, but not speaking, for Philips
void Void(void) { Void(); } /* The recursive call of the void */

Howerd Oakford <howerdo@gmail.com> writes:
> On Monday, November 8, 2021 at 1:59:20 PM UTC+1, David Brown wrote:
[...]
>> The big question is, why are you using "uint8_t" at all here?
> Company policy.

Can you elaborate on what the policy says? I presume it doesn't require
you to write incorrect code. I also presume it doesn't require you to
use 8-bit types for loop control variables in all cases.

--
Keith Thompson (The_Other_Keith) Keith.S.Thompson+u@gmail.com
Working, but not speaking, for Philips
void Void(void) { Void(); } /* The recursive call of the void */