[Insight-developers] Dashboard & NrrdIO warnings

[Gordon L. Kindlmann]

Hello,

Okay, I've looked at this again; it is about the NRRD_INDEX_GEN macro:

/*
******** NRRD_INDEX_GEN
**
** Given a coordinate array "coord", as well as the array sizes "size"
** and dimension "dim", calculates the linear index, and stores it in
** "I".
*/
#define NRRD_INDEX_GEN(I, coord, size, dim)   \
do {                                          \
   int d;                                      \
   for (d=(dim)-1, (I)=(coord)[d--];           \
        d >= 0;                                \
        d--) {                                 \
     (I) = (coord)[d] + (size)[d]*(I);         \
   }                                           \
} while (0)


There was a series of emails about this around Jan 28, which  
unfortunately I didn't have the time to follow then, but I've skimmed  
them now.

Cryptic as it may be, there's no real bug in the macro.  Its been run  
through valgrind on a range of possible inputs (by its use in Teem),  
and that code has been in daily use for about 10 years. I appreciate  
that this loop doesn't match ITK coding styles, but this code is from  
Teem, which has its own coding style.  People are welcome to argue  
about that on the Teem mailing lists :)

The only time that the coord array could be indexed by -1 is if dim is  
0, which is an invalid dimension for a nrrd.  dim can be 1 (nrrd  
represents 1-D arrays just fine), but then d=0, I=coord[0], d-=1, so  
d=-1, but then the second loop expression d >= 0 fails, and the loop  
exits before the body executes (and that's correct; the assignment to  
I is what matters).  If dim is 2, the body executes once, and then the  
loop exits.  The whole thing is in a "do {} while (0)" so that we can  
have a local variable d; simply "{}" would probably also work, but  
that's not the source of the warning.

My priority is to resolve the ITK compiler warnings without altering  
code that is working and (I strongly believe) error free, and to do so  
in a way that minimizes future work (and chances for error) in future  
updates of NrrdIO.  I think the process we've worked through for  
NrrdIO updates is good; it relies on copying and minimally  
transforming sources from Teem.  I'm not yet convinced this macro is a  
problem that needs fixing in Teem.

However, looking at the code and its context again, I can certainly  
see that a compiler could believe that the macro could be called with  
dim=0 (which would lead to indexing coord[-1]).  My belief is that  
this can't happen, given checks that exist elsewhere, but there's no  
reason not to make those checks more explicit/redundant.

How do you think the dim > 0 check would be added?  Is it other  
people's experience that these kinds of warnings can be resolving by  
putting in asserts()?  Or other checks that clearly lead to a  
different execution path?

Thanks for your patience,
Gordon

On Feb 28, 2010, at 2:45 PM, Luis Ibanez wrote:

> Michel, Gordon,
>
>
> The following warnings seems to be related to real bugs:
>
> http://www.cdash.org/CDash/viewBuildError.php?type=1&buildid=551088
>
> /.../Insight/Utilities/NrrdIO/reorder.c:250: warning: array subscript
> is below array bounds
> /.../Insight/Utilities/NrrdIO/subset.c:241: warning: array subscript
> is below array bounds
>
>
> Could you please comment on them ?
>
>
>    Thanks
>
>
>         Luis
>