KiCad

Useless checks in accessor functions

Bug #1777883 reported by Simon Richter on 2018-06-20

This bug affects 1 person

Affects		Status	Importance	Assigned to	Milestone
	KiCad	Fix Released	Low	Jeff Young

Bug Description

The functions

- FP_LIB_TABLE* PROJECT::PcbFootprintLibs( KIWAY& aKiway )
- FP_LIB_TABLE* PROJECT::PcbFootprintLibs()
- SYMBOL_LIB_TABLE* PROJECT::SchSymbolLibTable()
- PART_LIBS* PROJECT::SchLibs()

use dynamic_cast<> to verify that the object they received from GetElem() is of the requested type. As they perform the cast on a pointer that has already been coerced to the target type, the cast is a no-op and the check optimized out.

Enabling the test introduces undefined symbols during linking for tools not linking against pcbcommon.a, as the typeinfo for FP_LIB_TABLE is now required from project.o in common.lib, while it is provided in fp_lib_table.o, so this would pull the entire new IO framework into common.a.

The attached patch activates the test, which causes the linker error mentioned, so it is incomplete.

Revision history for this message

Simon Richter (sjr) wrote on 2018-06-20:

verify_types.patch Edit (2.9 KiB, text/plain)

Revision history for this message

Seth Hillbrand (sethh) wrote on 2018-09-05:

@Simon-

I'm not sure what this report is. Are you saying that the patch introduces an error that didn't exist before?

My apologies but I'm not fully understanding this.

Revision history for this message

Simon Richter (sjr) wrote on 2018-09-06:

The code as it is:

FP_LIB_TABLE* tbl = (FP_LIB_TABLE*) GetElem( ELEM_FPTBL );
wxASSERT( !tbl || dynamic_cast<FP_LIB_TABLE*>( tbl ) );

The C-style cast unconditionally creates a pointer of the desired type, and the dynamic_cast<> is optimized out as a no-op, so no check is performed.

The patch would fix this, by eliminating the C-style cast:

    auto tbl_elem = GetElem( ELEM_FPTBL );
    auto tbl = dynamic_cast<FP_LIB_TABLE*>( tbl_elem );
    wxASSERT( !tbl_elem || tbl );

This does not link however because of missing typeinfo, so the fix for this bug is no longer trivial.

Revision history for this message

Jeff Young (jeyjey) wrote on 2018-09-15:

The lack of RTTI is a cost of our separate apps strategy. I've simply removed the useless checks so we don't have any false sense of security.

Revision history for this message

Jeff Young (jeyjey) wrote on 2018-09-15:

I had a change of heart and implemented a quick-and-dirty poor-man's RTTI for these classes (a m_ClassName member variable).

Changed in kicad:
assignee:	nobody → Jeff Young (jeyjey)
status:	New → In Progress
importance:	Undecided → Low

Revision history for this message

Simon Richter (sjr) wrote on 2018-09-15:

*Another* poor-man's RTTI? We already have two (ClassOf/dyn_cast<> and Type())... :)

Revision history for this message

Jeff Young (jeyjey) wrote on 2018-09-15:

The others are all the same facility (dyn_cast<> uses ClassOf() which uses Type()).

But it's a good point, so I refashioned the new one so that it uses the same semantics and syntax as the above (even though they don't use the same baseclass).

Revision history for this message

KiCad Janitor (kicad-janitor) wrote on 2018-09-15:

Fixed in revision 02a3f8304008285d5684da845d6fc559b34d70fc
https://git.launchpad.net/kicad/patch/?id=02a3f8304008285d5684da845d6fc559b34d70fc