Sign in to follow this  

C++ Static Code Analysis Failure

This topic is 2014 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi,

This is the exact C++ code in question:[CODE]threads = new glLibInternal_MetaWorker*[glLibInternal_numof_threads];
for (int i=0;i<glLibInternal_numof_threads;++i) threads[i] = new glLibInternal_MetaWorker(this,mesh);
int i = 0;
for (int x=0;x<grid_res[0];++x) { for (int y=0;y<grid_res[1];++y) { for (int z=0;z<grid_res[2];++z) {
threads[i]->owned_coords.push_back(x);
threads[i]->owned_coords.push_back(y);
threads[i]->owned_coords.push_back(z);
++i;
if (i==glLibInternal_numof_threads) i = 0;
}}}[/CODE]The variable glLibInternal_numof_threads is an external global int that, at runtime, is set to 12. grid_res[3] is an int array, with all elements equal to 16.

As general good practice, I ran this through static code analysis. Visual Studio gives (on the fifth line in the above source):
warning C6385: Invalid data: accessing 'threads', the readable size is 'int glLib::glLibInternal_numof_threads*4' bytes, but '16' bytes might be read

My question is how? Am I missing something obvious?

Thanks,
Ian Edited by Geometrian

Share this post


Link to post
Share on other sites
Probably because of the shadowing of the variable "i". Shadowing is generally a bad idea and can lead to subtle bugs. I suggest naming the outer loop counter something different, or alternatively not using "i" as the name of the secondary counter.


Or, more likely: what's the type of "threads"? Edited by ApochPiQ

Share this post


Link to post
Share on other sites
True. There's no shadowing here. The only potential problem i see here is when glLibInternal_numof_threads is zero. In that case the second loop (in which the warning is triggered) would still access threads[0], despite there being no threads. Also, static analyzers aren't perfect, so maybe the static analyzer is just confused by the way wraparound is handled for i. Maybe try >= instead of ==. Edited by l0calh05t

Share this post


Link to post
Share on other sites
[quote name='ApochPiQ' timestamp='1339358628' post='4948004']
[quote name='RulerOfNothing' timestamp='1339315695' post='4947849']
What outer loop? The loop that uses 'i' as a counter is only one line long.
[/quote]

Heh, good catch. That code is bloody hard to read.
[/quote]
I can only second this - code analysis and a compiler will do so much, but to a human being reading it, it just looks [i]wrong[/i].

Share this post


Link to post
Share on other sites
Even some whitespace helps - try this for more readable:[code]threads = new glLibInternal_MetaWorker *[glLibInternal_numof_threads];

for (int i = 0; i < glLibInternal_numof_threads; ++i)
threads[i] = new glLibInternal_MetaWorker (this, mesh);

int i = 0;

for (int x = 0; x < grid_res[0]; ++x)
{
for (int y = 0; y < grid_res[1]; ++y)
{
for (int z = 0; z < grid_res[2]; ++z)
{
threads[i]->owned_coords.push_back (x);
threads[i]->owned_coords.push_back (y);
threads[i]->owned_coords.push_back (z);

++i;

if (i == glLibInternal_numof_threads) i = 0;
}
}
}[/code]

Share this post


Link to post
Share on other sites

This topic is 2014 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this