[Crystal-develop] Static source code analysis of crystalspace

Discussion:

John Smith

2014-02-25 14:51:14 UTC

Hi,

Just for fun, I ran the llvm/clang static source code analyzer on
(parts of) the (svn trunk /head) crystalspace code.

What stood out most to me in the results are not just the regular
reports, but the items listed at the bottom in the 'analyzer failures'
section. All of these items (besides the very first one listed, which
is an actual analyzer crash) are instances were the clang compiler
failed to correctly parse the code. I guess gcc is being more
permissive here.

Anyway, the full results can be found here:
http://lbalbalba.url.ph/clang/cs/

Regards,

John Smith

Matthieu Kraus

2014-02-25 15:10:28 UTC

Permalink

It's actually not an issue with gcc (or VS) being more permissive, but
with clang rejecting standard code.
I already had a look at those specific pieces of code when I was
re-visiting CS on FreeBSD last year (as FreeBSD moved to clang as core
compiler). The problem there is related to template classes and
friends which fail in clang due to clang lacking support for proper
friend relationships (there's also a bug on the clang/llvm tracker
about it as the way friend is currently implemented in clang is more a
hack than a proper implementation).

Kind regards,
RlyDontKnow

Post by John Smith
Hi,
Just for fun, I ran the llvm/clang static source code analyzer on
(parts of) the (svn trunk /head) crystalspace code.
What stood out most to me in the results are not just the regular
reports, but the items listed at the bottom in the 'analyzer failures'
section. All of these items (besides the very first one listed, which
is an actual analyzer crash) are instances were the clang compiler
failed to correctly parse the code. I guess gcc is being more
permissive here.
http://lbalbalba.url.ph/clang/cs/
Regards,
John Smith
------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
Crystal-develop mailing list
https://lists.sourceforge.net/lists/listinfo/crystal-develop

John Smith

2014-02-25 15:45:20 UTC

Permalink

Hi,

Thank you very much for taking the time to clarify that for me; that
explains all the 'analyzer failures'. Ill look up the report in the
clang/llvm bug tracker. Thanks.

But what about the 'reports' ?

Regards,

John Smith

On Tue, Feb 25, 2014 at 4:10 PM, Matthieu Kraus

Post by Matthieu Kraus
It's actually not an issue with gcc (or VS) being more permissive, but
with clang rejecting standard code.
I already had a look at those specific pieces of code when I was
re-visiting CS on FreeBSD last year (as FreeBSD moved to clang as core
compiler). The problem there is related to template classes and
friends which fail in clang due to clang lacking support for proper
friend relationships (there's also a bug on the clang/llvm tracker
about it as the way friend is currently implemented in clang is more a
hack than a proper implementation).
Kind regards,
RlyDontKnow

------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
Crystal-develop mailing list
https://lists.sourceforge.net/lists/listinfo/crystal-develop

Matthieu Kraus

2014-02-25 15:59:05 UTC

Permalink

I don't have the time to check all by hand right now, but a quick
check on random ones seems to yield the same so far: it complains
about pointers being processed while potentially being null for
functions that assert that pointer mustn't be null. Basically various
sanity checks (e.g. out-of-bounds for arrays, null for a vast majority
of pointer containers like strings, ...) are only performed in debug
mode using asserts and shouldn't occur in production code as they're
input errors.
Besides that there's the sizeof mismatch caused by a pointer being
reinterpreted to a different pointer type which would break if the
size of different pointer types was different which is nothing to
really worry about, either (hint: this is only true for very exotic
setups which wouldn't run CS for various other reasons, anyway).

Post by John Smith
Hi,
Thank you very much for taking the time to clarify that for me; that
explains all the 'analyzer failures'. Ill look up the report in the
clang/llvm bug tracker. Thanks.
But what about the 'reports' ?
Regards,
John Smith
On Tue, Feb 25, 2014 at 4:10 PM, Matthieu Kraus

John Smith

2014-02-25 16:08:49 UTC

Permalink

Hi,

On Tue, Feb 25, 2014 at 4:59 PM, Matthieu Kraus

I don't have the time to check all by hand right now, but a quick check

Thank you for taking the time to look into at least some of the
reports, and explaining them.

I think ill just crawl back under my rock now
;)

Thanks again,

Regards,

John Smith.

Eric Sunshine

2014-02-26 01:12:58 UTC

Permalink

Post by John Smith
On Tue, Feb 25, 2014 at 4:59 PM, Matthieu Kraus

I don't have the time to check all by hand right now, but a quick check

Thank you for taking the time to look into at least some of the
reports, and explaining them.
I think ill just crawl back under my rock now

These are not necessarily all false positives. For instance, the
"assignment of undefined or garbage value" at [1] is certainly
suspect. If we know that that function is always called with non-zero
iNumSpokes, then it's a false positive, but if not, it's a legitimate
complaint. At the very least, that code would benefit from a
CS_ASSERT(iNumSpokes != 0).

John, we would be happy to grant you commit access to the repository
if you want to investigate the reported issues and patch the
legitimate ones.

[1]: http://lbalbalba.url.ph/clang/cs/report-ae26f9.html#EndPath

John Smith

2014-02-26 15:48:06 UTC

Permalink

Post by Eric Sunshine
John, we would be happy to grant you commit access to the repository
if you want to investigate the reported issues and patch the
legitimate ones.

Thank you for the offer, but I have nowhere near the skillset required
for that task: I can barely write helloworld.c at this point.
;)

I was hoping to contribute by running the analyzer and generating the
reports, in the hope that more knowledgeable people than I would be
able to fix the reported items (assuming there would be more real
issues found than false positives). As it is, it seems that the
current state of clang results in too much noise for this particular
codebase too be useful at this point in time, at least.

Thanks all for the positive responses,

Regards,

John Smith.