kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 12/05/2011 10:35 AM, Wayne Stambaugh wrote:
> On 12/5/2011 9:48 AM, Dick Hollenbeck wrote:
>> On 12/05/2011 08:02 AM, Wayne Stambaugh wrote:
>>> On 12/5/2011 12:30 AM, Dick Hollenbeck wrote:
>>>> Wayne,
>>>>
>>>> I think the lines involving operator new() may blow up at runtime when the corresponding
>>>> delete [] is called.
>>> Dick,
>>>
>>> I'm not using
>>>
>>> delete[] foo;        // This is not the same as delete foo[nn];
>>>
>>> where the contents of foo are allocated using
>>>
>>> foo = new();
>>>
>>> I'm using
>>>
>>> delete foo[nn];      // This is the same as tmp* = foo[nn]; delete tmp;
>>>
>>> where the contents of the array of foo pointers is assigned using
>>>
>>> foo[nn] = new( sizeof(foo) );
>>>
>>> In the case of m_BoardSide it is defined in the board class as
>>>
>>> MATRIX_CELL* m_BoardSide[MAX_SIDES_COUNT];
>>>
>>> so I'm just filling the array of pointers with the new() operator so delete()
>>> should work properly.
>>>
>>>> Have you tested this code?
>>> I ran the auto router and didn't it segfault and it generated the same results
>>> as the old code so it appears to be correct.
>>>
>>> Wayne
>> Ok.  A related concern on this line:
>>
>>         /* allocate Board & initialize everything to empty */
>>         m_BoardSide[kk] = (MATRIX_CELL*) operator new( ii * sizeof(MATRIX_CELL) );
>>
>>
>> My understanding is that the old code also did not run the constructor MATRIX_CELL on each
>> element within the range [0] < [ii-1].
> Yep.  I originally tried to use new[] but calling the constructor broke the
> auto router.  The original ZMalloc code did not call the constructor either so
> using new( count ) was the best short term solution.  I know it is not the most
> elegant solution but my main goal was to stop using malloc() because no one was
> testing if the result of the allocation was NULL.  At least new() will raise an
> assertion that will get caught further up the stack by wxWidgets.  It may still
> crash but a least you will know the the type of assertion that cause the
> problem even in release builds.

Your concept is sound.  I just wanted to point out that the behavior of the new code is
different, and that the comment seems out of date.  No initialization is taking place,
whereas before, there was.  The memory was being zeroed out.  This makes the false comment
even more dangerous, because it is misleading one into thinking something equivalent is
happening.


>> But that the Mzalloc() simply zeroed out the memory before returning it?
>>
>> I'm wondering if your comment is now accurate, since we:
>>
>> a) do not run the constructors, and
>> b) do not zero out the memory.
> Not zeroing out the memory would have been a problem if it was tested somewhere
> in the code if used as an end of list indicator but is is not.  If it weren't
> for the function SetCell() and this code it autoplace.cpp:
>
>     /* Initialize top layer. */
>     if( Board.m_BoardSide[TOP] )
>         memcpy( Board.m_BoardSide[TOP], Board.m_BoardSide[BOTTOM],
>                 NbCells * sizeof(MATRIX_CELL) );
>
> then zeroing may have been an issue and I would have called memcpy() to zero it
> out.  I can still do this if it poses a problem that I overlooked.  That's why
> I ran the auto router on a board and looked for any visual difference in the
> results.
>
> I'm guessing that this is some very old legacy C code that needs some serious
> refactoring.  The fact that memcpy() is still in there makes me take pause.
> There are multiple stand alone functions accessing the public member variables
> in multiple classes and lots of other difficult to follow code paths in the
> auto routing code.  Some day I'll revisit it along with a "few" other places in
> Pcbnew that need some attention.
>
> Wayne

I can live with changing the comment to reflect the fact that no initialization is taking
place.

Thanks,

Dick