[Insight-developers] Dashboard & NrrdIO warnings

[Michel Audette]

Gordon,

the fix that Luis had suggested was this:
#define NRRD_INDEX_GEN(I, coord, size, dim)     \
{                                               \
  int d;                                        \
  d = (dim) - 1;                                \
  if ( (d) >= 0 )                               \
  {                                             \
    (I) = (coord)[d];                           \
    d--;                                        \
    while( d >= 0 )                             \
    {                                           \
    (I) = (coord)[d] + (size)[d] * (I);         \
    d--;                                        \
    }                                           \
  }                                             \
}

The Sourceforge repository should be updated accordingly.

On another note, the fix that you made
privateNrrd.h:#include "itk_zlib.h"

entails that the pre-GNUmakefile should have a ITK_SRC_ROOT defined,
does it not? Otherwise, how to tell it where to find the header?

Last, I believe that I neglected to incorporate that recent format
change %.17lg, and should ensure that this is up to date.

Cheers,

Michel



On Mon, Mar 1, 2010 at 9:58 AM, Michel Audette
<michel.audette at kitware.com> wrote:
> 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.
>



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