shame on MISRA

Hello CBFalconer,


I top post to make you a pleasure.

FYI: It doesn't make any difference if you do x = x+1 or x++.

By C convention, it is treated as x = (int)x + (int)1 in the either 
variant. Therefore:

u8 x; x++;

will produce the same MISRA warning about downcasting as x = x + 1;


Vladimir Vassilevsky

DSP and Mixed Signal Design Consultant

http://www.abvolt.com



CBFalconer wrote:

> msg wrote:
> 
>>Vladimir Vassilevsky wrote:
>>
>>>Recently they showed me a case when the strict compliance to
>>>MISRA provoked the mistake:
>>>
>>>Original code:
>>>
>>>foobar()  {
>>>  u8 variable;
>>>  variable = something;
>>>/* Explicit type conversion is required by MISRA. This is because
>>>   of the C convention: result of any integer operation is int */
>>>  variable = (u8)(variable + 1);
>>>}
>>>
>>>Later, the code was modified. It was decided that variable should be u16.
>>>  u16 variable;
>>>But this line was not changed:
>>>  variable = (u8)(variable + 1);
>>>So here is a bug. No warnings.
> 
> 
> ** Quote edited to reduce vertical space - cbf
> 
>>Why no warnings? What compiler?  I frequently write for 'SCDE'
>>(Standard C Development Environment') on SVR4 (C 90) and also
>>port stuff developed by others on GCC which often produces lots
>>of warnings about "explicit cast required" for assignments and
>>logical comparisons which the original authors failed to qualify
>>with a cast. Evidently these things pass 'lint' and compile in
>>GCC with no warnings (even at a high warning level).  Most
>>authors are dumbstruck by the need to explicitly cast (sometimes
>>even numeric constants).
> 
> 
> Any compiler.  This shows the dangers of both using peculiar types
> (such as u8 and u16) and of casting.  In general any cast is
> suspicious in C code.
> 
> BTW, use of "variable++;" would have avoided it all.  The original
> programmer was not up to the job.
> 

On Mar 29, 11:35 pm, CBFalconer <cbfalco...@yahoo.com> wrote:
> msg wrote:
>
> ... snip ...
>
> > I should have provided an example.  Here is a line from a popular
> > opensource X10 automation package developed by its author on
> > linux/gcc:
>
> > typedef uint_t  size_t; (unsigned int on both SVR4 and linux)
> > size_t strlen (const char *s);
> > void store_error_message ( char *message );
> > int  space;
> > ...
> >     if ( space < strlen(message) + 1 ) {
>
> > No warnings under GCC. Note that strlen() returns size_t
> > on linux as well as on SVR4.
>
> I disagree.  space should have been defined as size_t.  Then no
> casts, no warnings, no problems.

Or at a minimum, the cast should be in the other direction:

   if ( (size_t)space < strlen(message) + 1 )

It seems to me that it is less likely that "space" is negative than it
would be that "strlen(message)+1" would overflow an int.  And even if
space _is_ negative, the code is very likely broken in ways worse than
an unnecessary cast...

Regards,
   -=Dave

In article <op.tpz0ggkky6p7a2@ragnarok.lan>, Boudewijn Dijkstra says...
> Op Thu, 29 Mar 2007 16:48:48 +0200 schreef Vladimir Vassilevsky
> <antispam_bogus@hotmail.com>:
> > u8 variable;
> > variable = (u8)(variable + 1);
> >
> > u16 variable;
> > variable = (u8)(variable + 1);
> >
> > So here is a bug. No warnings.
> 
> That is why some C compilers (and some C-like languages/compilers too)
> have the typeof operator.

OK, I give up.  How does typeof help here?

Robert

-- 
Posted via a free Usenet account from http://www.teranews.com

Robert Adsett wrote:
> Boudewijn Dijkstra says...
>> schreef Vladimir Vassilevsky <antispam_bogus@hotmail.com>:
>>
>>> u8 variable;
>>> variable = (u8)(variable + 1);
>>>
>>> u16 variable;
>>> variable = (u8)(variable + 1);
>>>
>>> So here is a bug. No warnings.
>>
>> That is why some C compilers (and some C-like languages/compilers too)
>> have the typeof operator.
> 
> OK, I give up.  How does typeof help here?

It doesn't.  The proper action is to remove the cast and let the
compiler perform the default conversions.  It helps to understand
warnings and what they are worrying about.  Then you make an
intelligent decision about ignoring them, and maybe even leave a
comment for the poor maintainer.

typeof is not generally available, and is definitely not standard.

-- 
Chuck F (cbfalconer at maineline dot net)
   Available for consulting/temporary embedded and systems.
   <http://cbfalconer.home.att.net>



-- 
Posted via a free Usenet account from http://www.teranews.com

In article <130p2dpjc2qgdaa@corp.supernews.com>, msg says...
> CBFalconer wrote:
> > msg wrote:
> > I frequently write for 'SCDE'
> >>(Standard C Development Environment') on SVR4 (C 90) and also
> >>port stuff developed by others on GCC which often produces lots
> >>of warnings about "explicit cast required" for assignments and
> >>logical comparisons which the original authors failed to qualify
> >>with a cast. Evidently these things pass 'lint' and compile in
> >>GCC with no warnings (even at a high warning level).  Most
> >>authors are dumbstruck by the need to explicitly cast (sometimes
> >>even numeric constants).
> > 
> > 
> > Any compiler.  This shows the dangers of both using peculiar types
> > (such as u8 and u16) and of casting.  In general any cast is
> > suspicious in C code.
> 
> I should have provided an example.  Here is a line from a popular
> opensource X10 automation package developed by its author on linux/gcc:
> 
> typedef uint_t  size_t; (unsigned int on both SVR4 and linux)
> size_t strlen (const char *s);
> void store_error_message ( char *message );
> int  space;
> ...
>     if ( space < strlen(message) + 1 ) {
> 
> No warnings under GCC. Note that strlen() returns size_t
> on linux as well as on SVR4.
> 
> SCDE C compiler warns:
> 
> "process.c", line 154: warning: semantics of "<" change in ANSI C; use explicit cast
> 
> The line should have been written:
> 
>     if ( space < (int)strlen(message) + 1 ) {
> 
> which eliminates the warnings.  

I thought I'd try PC-Lint on some of these and see what popped out.  
Some overhead left out but here is what I ran along with the resulting 
messages and some of my own commentary.

size_t strlen (const char *s);

void test(const char *mess)
{
extern unsigned char variable1;
extern unsigned int variable2;
extern int  space;
extern size_t  space2;

variable1 = variable1 + 1;

variable1 = (unsigned char)(variable1 + 1);

variable2 = (unsigned char)(variable2 + 1);

variable1++;

variable2++;

+++ No messages for any of the above.  I know PC-Lint checks for a 
subset of the MISRA messages but I'm guessing only for the previous 
version.  It's been a little while since I read the previous version but 
I don't remember a rule that would require a cast in these cases and 
that sort of check is certainly within PC-Lint's capability.  As far as 
there being no complaints when the cast is added I think that falls 
under "If you lie to the compiler it will get its revenge."

if ( space < strlen(mess) + 1 ) {
    }
/*
test2.c  55  Note 961: Violates MISRA
    Advisory Rule 47, dependence placed on C's operator precedence; 
operators:
    '<' and '+'

test2.c  55  Warning 639: Strong type
    mismatch for type 'size_t' in binary operation

test2.c  55  Warning 574: Signed-unsigned mix
    with relational

test2.c  55  Info 737: Loss of sign in
    promotion from int to unsigned long
*/
    
if ( space < (strlen(mess) + 1) ) {
    }
/*
test2.c  71  Warning 639: Strong type
    mismatch for type 'size_t' in binary operation

test2.c  71  Warning 574: Signed-unsigned mix
    with relational

test2.c  71  Info 737: Loss of sign in
    promotion from int to unsigned long
*/

if ( space < (int)strlen(mess) + 1 ) {
    }

/*
e:\cygwin\home\radsett\newlib-lpc\test2.c  84  Note 961: Violates MISRA
    Advisory Rule 47, dependence placed on C's operator precedence; 
operators:
    '<' and '+'
*/

if ( space2 < strlen(mess) + 1 ) {
    }
/*
e:\cygwin\home\radsett\newlib-lpc\test2.c  92  Note 961: Violates MISRA
    Advisory Rule 47, dependence placed on C's operator precedence; 
operators:
    '<' and '+'

e:\cygwin\home\radsett\newlib-lpc\test2.c  92  Warning 639: Strong type
    mismatch for type 'size_t' in binary operation

e:\cygwin\home\radsett\newlib-lpc\test2.c  92  Warning 638: Strong type
    mismatch for type 'size_t' in relational
*/

if ( space2 < (strlen(mess) + (size_t)1) ) {
    }

+++ This last one is the most interesting as far as I'm concerned.  
Although having the value compared to string length being a size_t is 
'the correct thing to do' as far as I'm concerned, having space2 as a 
size_t is not sufficient.  All the little constants involved need to be 
size_t as well.  Ugly.
}

Robert

-- 
Posted via a free Usenet account from http://www.teranews.com

"CBFalconer" <cbfalconer@yahoo.com> skrev i meddelandet 
news:460DC48E.A7B6BE16@yahoo.com...
> Robert Adsett wrote:
>> Boudewijn Dijkstra says...
>>> schreef Vladimir Vassilevsky <antispam_bogus@hotmail.com>:
>>>
>>>> u8 variable;
>>>> variable = (u8)(variable + 1);
>>>>
>>>> u16 variable;
>>>> variable = (u8)(variable + 1);
>>>>
>>>> So here is a bug. No warnings.
>>>
>>> That is why some C compilers (and some C-like languages/compilers too)
>>> have the typeof operator.
>>
>> OK, I give up.  How does typeof help here?
>
> It doesn't.  The proper action is to remove the cast and let the
> compiler perform the default conversions.  It helps to understand
> warnings and what they are worrying about.  Then you make an
> intelligent decision about ignoring them, and maybe even leave a
> comment for the poor maintainer.
>
> typeof is not generally available, and is definitely not standard.
>
> -- 


Maybe we need a new C keyword for such type conversions.

variable = (sortof)(variable + 1);        // Convert, if it is the right 
thing to do :-)

-- 
Best Regards,
Ulf Samuelsson
This is intended to be my personal opinion which may,
or may not be shared by my employer Atmel Nordic AB 

In article <MPG.207797f867c42054989716@free.teranews.com>, Robert Adsett 
<sub2@aeolusdevelopment.com> writes
>In article <130p2dpjc2qgdaa@corp.supernews.com>, msg says...
>I thought I'd try PC-Lint on some of these and see what popped out.

>
>+++ No messages for any of the above.  I know PC-Lint checks for a
>subset of the MISRA messages but I'm guessing only for the previous
>version.

There are MISRA-C files for both versions on their web site.


-- 
\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\
\/\/\/\/\ Chris Hills  Staffs  England     /\/\/\/\/
/\/\/ chris@phaedsys.org      www.phaedsys.org \/\/\
\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/

msg wrote:

> I should have provided an example.  Here is a line from a popular
> opensource X10 automation package developed by its author on linux/gcc:
> 
> typedef uint_t  size_t; (unsigned int on both SVR4 and linux)

If *that* line is actually in the source (as you say), that disqualifies 
the entire example.  Both uint_t and size_t are typedefs provided by 
standard headers.  Definining either of them yourself would be folly 
already.  But defining the older one (size_t) by reference to the newer 
(uint_t) is outright crazy.

In a nutshell: typedef'ing size_t yourself is suicide for any program 
that tries to be portable.

> void store_error_message ( char *message );
> int  space;
> ...
>    if ( space < strlen(message) + 1 ) {
> 
> No warnings under GCC. 

That only demonstrates you don't know how to ask GCC for warnings.  In 
particular, you failed to enable -Wsign-compare.

> The line should have been written:
> 
>    if ( space < (int)strlen(message) + 1 ) {

Not based on the evidence you presented it shouldn't.  It should rather 
be redesigned from the ground up, by answering the following questions:

1) why is "space" typed int instead of size_t?
2) what is supposed to happen if "space" turns negative?
3) what is supposed to happen if (strlen(message) > INT_MAX)?
4) what is supposed to happen if (strlen == SIZE_MAX)?

Vladimir Vassilevsky wrote:

> FYI: It doesn't make any difference if you do x = x+1 or x++.
> 
> By C convention, it is treated as x = (int)x + (int)1 in the either 
> variant. 

Bzzzt, you're out.  Thanks for playing.

You're wrong on two counts here.

1) No, x++ is not equivalent to adding a literal 1 (which would indeed 
have type int).  It's defined to add "the value 1 of the appropriate 
type" (C99 6.5.2.4p2).  I.e. for the case of an 8-bit variable x, it's 
equivalent to

	x += (u8) 1;

The cast that MISRA rules insist on would be implied in using the ++ 
operator.

2) Your u16 type is presumably equivalent to C99's uint16_t, right?
Then the above would be correct only on platforms where int is larger 
than 16 bits.  On 16-bit platforms, ++ applied to a uint16_t variable x 
is actually equivalent to

	x = x + (unsigned int)1;

Hans-Bernhard Br�ker wrote:

> msg wrote:
> 
>> I should have provided an example.  Here is a line from a popular
>> opensource X10 automation package developed by its author on linux/gcc:

(excerpt from system header):
>> typedef uint_t  size_t; (unsigned int on both SVR4 and linux)
> 

> In a nutshell: typedef'ing size_t yourself is suicide for any program 
> that tries to be portable.
> 

As to portability, very few authors of linux-targeted software (which
for the most part is claimed to be portable) ever consider an int size
< 32 bits unfortunately and never ever consider that their code would
be ported beyond their parochial world.

Regards,

Michael