Does anybody have any recommendations for static source code analysis tools based on practical use?
My customer is asking us to apply a static source code analysis tool to an existing medical product. The product is undergoing changes for which the FDA requires another round of testing. The customer was dinged by the FDA for not using an analysis tool so they want to correct the process this time around.
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.
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.
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?
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.
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.
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
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.
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) > In MISRA C, the literal 1 is a char not an int?
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 explicit is the 'UINT32_C' macro - but that one expands to nothing on a
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.
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:
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.)
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).
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.
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 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.
ElectronDepot website is not affiliated with any of the manufacturers or service providers discussed here.
All logos and trade names are the property of their respective owners.