[Insight-developers] Dashboard & NrrdIO warnings

[Michel Audette]

Hi Luis,

sorry, I'd made a change to this recently, but in the process of
adopting the latest NrrdIO stuff, based on the wiki procedure, I
neglected to propagate the change. It was a fix to NrrdIO.h... I will
make the fix this morning.

Cheers,

Michel


On Mon, Mar 1, 2010 at 7:42 AM, Gordon L. Kindlmann <glk at uchicago.edu> wrote:
> 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
>>
>
>



-- 
Michel Audette, Ph.D.
R & D Engineer,
Kitware Inc.,
Chapel Hill, N.C.