Source code static analysis tool recommendations

wrote:



What language(s) and what platform should the tool run on?


This is going back a ways [so take with a grain of salt] but circa
2000 when I was in medical imaging the FDA was quite happy with Lint
for C or C++.

If the code never has been linted previously, you are likely to get
many pages of warnings.  Examining them, you are likely to find quite
a large percentage are trivial / ignorable, but checking a large code
base for the 1st time will not go quickly.

All the tools I am familiar with allow to disable particular checks
that consistently produce ignorable results.  But you have to
determine that by actually looking at the code.

Whatever tool you use, you'll need to verify that the FDA will accept
the result ... I've been away from that arena for too long to know
what tools currently are acceptable.


George

On 2/2/2018 3:24 PM, George Neuner wrote:


Thanks George.  The code is in C.  We'd like to run the tools on Windows  
computers.

It happens that the first FDA submission was subject to only PC-Lint.  
The customer told me that the FDA did accept that test but I didn't  
believe it.  PC-Lint is not a deep testing tool, in my opinion.  I'll  
need to take a second look at that.

JJS

wrote:



I haven't seen PC-Lint in many years, so I can't comment on that.
Splint and cppcheck are free alternatives that are worth a look.  

There's an add-in that will integrate cppcheck into any recent Visual
Studio edition.  Also VS Professional (or better) since 2013 has a
pretty decent lint module included.

Splint works fine with VS, but there's no integration available AFAIK.
You can use it as an external tool.


Synopsys's Coverity Scan is quite nice and decently comprehensive, but
note that I only have used the free online version, which uploads your
code to Synopsys's servers for analysis [and license-wise requires
your project to be FOSS].  They do sell a version for proprietary use,
but I have no knowledge of the cost.


At this point, I'm out of suggestions.  Maybe someone else has other
ideas?  

George

Am 03.02.2018 um 09:40 schrieb George Neuner:


Not sure what lint module you're referring to, but Visual Studio has had
an "/analyze" option for quite some while. I remember there some tricks
circulating for enabling that on editions that did not officially
support it. One advantage that tool has over others is that it knows
Windows APIs, and finds things like "this API call returns a handle that
you should close if you don't need it".

The same thing can be said of cppcheck: it knows some POSIX APIs and
tells you you're leaking stuff.



Our company evaluated Coverity vs. Klocwork and ended up at Klocwork,
although I was not involved at the decision.

My main gripe with tools of that kind is that they have tons of checkers
and people turn them on all at once because they can. Klocwork literally
has something to say for every line of code, ending up with tens of
thousands of "issues" in a perfectly working software. Our QA people
defined a subset what to consider critical - about 1% of that - and even
of that remainder, in my team's software only 1% is actual problems, 50%
is style issues ("no 'break' after the last branch of this 'switch'",
"no '&' before this function name used as pointer"), and the remainder
is false positives ("please remove this check for errors, because I can
prove that this function call does not ever produce an error").

If my goal were getting better software, I'd start with turning up
compiler warnings to the max (warnings count as static analysis,
right?), unit tests with coverage analysis, valgrind, and an occasional
cppcheck or cl /analyze.

If the goal is to get a tool permanently into your workflow in a
documented way, a database-based tool like Klocwork doesn't sound too
bad, mainly because it has a way to re-identify findings across source
code changes (you need to tell it only once that that finding is a false
positive, not adjust a suppressions file after every edit). Just be
careful at what checkers you use and what to do about them. "Fix all
findings" is the wrong approach.


  Stefan

I haven't tried splint it years, but it used to be completely
useless for real code.  It would warn you about things like this:


    uint8_t b;
    b = 1;

  or  

    uint8_t i,j;
    [...]    
    i = j+1;

The warning was because the expression on the RHS was of type "int"
(16 or 32 bits on the platforms I cared about) and was being truncated
by the assignment to an 8-bit wide LHS.

On small processors with only a few hundred bytes of RAM one tended to
use the smallest variable types possible, and as a result splint
produced thousands of meaningless warnings unless you typecast most of
your integer assignments -- which made the code completely unreadable.
    
I spent a few days hacking on splint to try to fix issues like

incorporate fixes like mine because splint wasn't intended for use on
real code in the real world because it was solely an academic project
for use studying language concepts.

Have they since pulled their heads out of <whatever>?

Am 03.02.2018 um 17:37 schrieb Grant Edwards:


The latter gives a warning with gcc "-Wconversion" as well. Although I
was long opposing that warning, it's hard to argue your head out of it
if your company's software already had a few incidents with it. For
control code, it is definitively possible to get "-Wconversion"-free;
for computational code, it's some more work but possible (e.g. make a
'sample_add' inline function that does the casting, and use that).

What I find more annoying is that MISRA finds this

     uint32_t MASK = UINT32_C(1) << 17;

objectionable. '1', no matter how spelled, has type 'char' for MISRA,
and cannot be shifted by 17 bits.


  Stefan

In MISRA C, the literal 1 is a char not an int?

Am 04.02.2018 um 16:12 schrieb Grant Edwards:


Yes. MISRA C 6.10.4 "Underlying type".

The problem they're trying to solve isn't too far-fetched: if 'int' has
16 bits only, '1 << 17' is undefined, so you'd better be explicit about
the type you're shifting. The problem now is that C99's way of being
explicit is the 'UINT32_C' macro - but that one expands to nothing on a
32-bit architecture...


  Stefan

Am 05.02.2018 um 18:48 schrieb Stefan Reuther:


Where on earth did you get that idea?  UINT32_C does not even _appear_  
in the C99 standard.

7.20.4 "Macros for integer constants" (assuming the section number is
the same for C99 and C11, which is where I looked).

UINT32_C(x) gives you a uint_least32_t constant.  If the implementation
has support for uint32_t (and almost all systems do), then it will be
the same type as uint_least32_t.

Am 06.02.2018 um 09:34 schrieb David Brown:





C99 7.18.4.  Ah there it is.  Sorry, my bad for not finding that (or  
knowing about it off-hand).

But then Stefan's next statement is wrong.  There's no way this:

 > expands to nothing on a 32-bit architecture...

can be correct.  At the very least, UINT32_C(1) has to change the  
signedness of its constant argument, i.e. it has to expand to either of  
the following, or an equivalent:

    (uint_least32_t)1
    1u

And in both cases, both MISRA and PC-Lint _will_ accept that shift.  
Well, assuming the tool was configured correctly, that is.

There aren't many in this newsgroup who could that section - remember,  
this is the /practical/ group, not comp.lang.c !  And you are  
technically right - "UINT32_C" does not appear anywhere, so you won't  
find it be searching.  (For anyone who is interested enough to still be  
reading, but not interested enough to look at the standards, the  
paragraph talks about UINT/N/_C in general rather than UINT32_C in  
particular.)









<stdint.h> on my Linux system has:

/* Unsigned.  */
# define UINT8_C(c)     c
# define UINT16_C(c)    c
# define UINT32_C(c)    c ## U
# if __WORDSIZE == 64
#  define UINT64_C(c)   c ## UL
# else
#  define UINT64_C(c)   c ## ULL
# endif


I would say that on a 32-bit system, UINT32_C has to include an  
"unsigned" indication (like a cast, or adding a U suffix).  But since  
there is no such thing in C as an integer constant of types smaller than  
int or unsigned int, I can't decide if UINT16_C here should have the U  
suffix or not.  I'm open to ideas either way - or we could punt it to  
comp.lang.c for the opinions (at length) of people who sleep with the C  
standards under the pillow, rather than just having a shortcut on their  
desktop (like me).

Am 06.02.2018 um 23:02 schrieb David Brown:



If only.  At least in the 2004 edition, they pulled an entire  
hypothetical integer type system named "underlying type" out of thin air  
and based some of their rules on that, totally mucking up any clear  
picture of the actual C integer semantics anyone might still have had in  
their mind.

And yes, that utter phantasy of theirs does indeed claim the existence  
of integer constants of a type smaller than int.  Such a thing has never  
existed in the entire history of at least roughly standard-conforming C  
compilers, and it flat-out contradicts the actual semantics of C integer  
constants and conversions.  Didn't stop them, though.

What it means is that a MISRA 2004 checker will indeed pretend that the  
type of 1U is uint8_t(!), totally defeating the purpose of the UINT32C()  
macro if the compiler/libc happens to implement that by suffixing a 'U'.

So in a MISRA 2004 environment, one had better forget those macros even  
exist --- I obviously did :-).  You'll really have to write 1UL,  
(uint32_t)1 or equivalent, instead.

If there's method to that madness, it eludes me.

On 07/02/18 03:34, Richard Damon wrote:






Yes, I agree with that logic.  Then it will be "an integer constant
expression corresponding to the type uint_leastN_t", in that you will
get the same behaviour for the expression "u + x" for any x, when u is
either:

    uint8_t u = 1;

or

    #define u UINT8_C(1)


But you can also say that by adding a U, you turn the constant into type
"unsigned int" rather than "int", and thus it will not be promoted to a
signed int.

I don't know if the standards are clear enough here to require one
interpretation or the other.  The gcc implementations I have looked at
(some with glibc for Linux, some with newlib for embedded ARM) all
follow the first version with no U for sizes less than an "int".  An
older CodeWarrior with its EWL library seems to take the other version
and add a U.  Based purely on "which of these to I trust more to
understand the standards", my vote will be with gcc/glibc/newlib - the
CodeWarrior C99 support and library has never impressed me with its quality.

itto:


If you write "1ul" it should be 32 bits.

Bye Jack

On 06/02/18 08:32, Jack wrote:



No - it should be /at least/ 32 bits.  On 64-bit Linux, it will be 64 bits.

On 05/02/18 18:48, Stefan Reuther wrote:


The literal "1" cannot be shifted by 17 bits if an int has less than 18
bits.  But it never has type "char".

As far as I can tell, it is perfectly acceptable in MISRA to write:

    uint32_t MASK = ((uint32_t) 1) << 17;





No.  You are mixing MISRA rules with language rules.

MISRA provides rules on how to write C - it most certainly does not
/change/ C.  In C, the literal 1 is an decimal integer constant of type
"int" (section 6.4.1.1).

As a coding standard, MISRA /can/ have rules to say that you are not
allowed to /use/ a literal here - but it cannot change its meaning in
the language.



I can't find any such section in MISRA 1998 or MIRCA 2012, nor any
concept of "underlying type".  Could you be more specific in your
reference?  Or use the correct MISRA term?

MISRA does have its "essential type model".  It is wrong, and doesn't
fit C, which is not particularly helpful when you are trying to write
quality C code.  It seems to be based on the idea that integer promotion
in C shouldn't really exist, that "char" is not an integer type, and
that you can't mix types.  There are some good ideas behind it to reduce
misconceptions or possible errors, but it seems a bit jumbled to me.  It
also adds its own bizarre set of promotion and conversion rules that are
in parallel, and sometimes in odds with, the C rules.  It means you have
to learn two sets of rules instead of one.

From Appendix D, detailing "essential types", a signed integer constant
has "essential type" of the smallest signed integer type that can
express it - for 1, this will be "signed char" (but /not/ "char").

MISRA has rules against bit-shifting "essentially signed" types -
inaccurately justified by saying that these are implementation defined.
 But the rules covering shifts and constants are weird:

"1ul" has C type "unsigned long int", and MISRA "essential type" of
"unsigned char".

"1ul << 17" has C type "unsigned long int", value 0x20000, and MISRA
"essential type" of "unsigned long int".  The expression is allowed (as
is "1u << 17" when int is 32-bit) in MISRA, as far as I can see.

I am glad I don't have to program to MISRA.  It has plenty of good
rules, but most of those are either blindingly obvious or covered by
"don't use undefined behaviour, and be careful about implementation
defined behaviour".  And it has a wide range of junk and actively /bad/
rules that should be replaced by "get a decent compiler and enable
warnings, or use a linter if your compiler is crap" and "learn to
program in C".

Am 06.02.2018 um 10:23 schrieb David Brown:


Problem is, we're actually doing C++, where MISRA imposes the same rules
for numbers, but would actually require us to write

      uint32_t MASK = static_cast<uint32_t>(1) << 17;

which is pretty much noise for my taste.

I like to use this actual definition from Windows APIs as a "how not"
example for cast-addicts:

   #define MAKELONG(a, b) ((LONG) (((WORD) (((DWORD_PTR) (a)) & \
      0xffff)) | ((DWORD) ((WORD) (((DWORD_PTR) (b)) & 0xffff))) << 16))



MISRA defines a subset of C, and a program must follow the rules of that
subset. Of course that can be an own type system.



MISRA-C:2004, chapter 6.10.4 "Underlying type". A few paragraphs in, it
defines "The underlying type of an integer constant expression is
therefore defined as follows: 1. if the actual type of the expression is
(signed) int, the underlying type is defined to be the smallest integer
type which is capable of representing its value", followed by a table
where '1' falls into the category '8 bit signed' which would be (signed)
char.

MISRA C++ has a similar rule but I don't have that document at hand.



That sounds like you have a completely different document in front of
you than me - which wouldn't be too surprising given that the document
changed completely between 1998 and 2004.



The most important takeaways I have from MISRA is: think about what you
do and why you do it, and: you need a deviation procedure that allows
you to break every rule. That deviation procedure is what makes MISRA
work because it rules out laziness as a reason for not doing something.
I won't write a deviation request to be allowed to leave out the {}'s,
but if my design requires me to have malloc and recursion, I want to
have that!

That's why my advice to OP avoid the "fix all findings" approach. That's
the easy way for management get some cool downward-pointing charts on
PowerPoint, but also the easy way to completely ruin a moderatly ok
software.


  Stefan

On 06/02/18 19:05, Stefan Reuther wrote:


That's MISRA for you - rules for rules sake, no matter how ugly and  
incomprehensible your code ends up.  In situations like this, it totally  
misses the point.



I doubt if anyone will hold up the Windows API as a shining example of  
good design and clear programming!



No, it cannot - it /has/ to use the C type system when you are  
programming in C.  It can discuss other aspects of types or features in  
your code, and provide extra rules - but that does not change the types  
in the code.



Ah, okay then.  I have MISRA 1998 and MISRA 2012, but not MISRA 2004.  
They seem to change a fair bit in their layout, organisation and wording.



So it does.  I have MISRA C++ 2008.



And then again in 2012...



I have nothing against thinking!  But I /do/ have something against  
having to write a deviation report in order to avoid a cast monstrosity  
just because the rules say that "1" is not an int.



Fair 'nuf.