kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxxxx>]

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.

> 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

> 
> Thanks,
> 
> Dick
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>