cvpcb can not preview footprints

Bug #1778426 reported by endofexclusive
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
KiCad
Fix Released
Undecided
endofexclusive

Bug Description

# Steps to reproduce:
1. rm -rf $HOME/.config/kicad
2. /opt/kicad/bin/kicad /opt/kicad/share/kicad/demos/video/video.pro
3. Click Schematic layout editor (Create empty symtable when asked.)
4. Click "Run CvPcb..."
5. Click any of the footprints belonging to the "footprints" library in the right-most list.
6. Click "View selected footprint"

# This is happening
* A "Warning" dialog appears with the text "fp-lib-table files contain no library with nickname "footprints".
* The same dialog appears for each footprint selected in the right-most list of CvPcb.
* On Debug build of KiCad, there is some additional information hinting that the warning comes from FP_LIB_TABLE::FindRow().
* The footprint preview window shows up but no footprint is previewed.

This has been tested on many different projects and making sure that the FP library table is correct. The libraries and individual footprints show up correctly in CvPcb. It is just the preview that does not work.

# Cause
In FP_LIB_TABLE::FindRow(), row is nullptr even in the case where findRow() returns non-nullptr. This was verified by inserting some debug printouts.

This issue is probably compiler dependent.

# Solution
The issue is resolved by changing dynamic_cast into static_cast as in the attached patch.
   const FP_LIB_TABLE_ROW* FP_LIB_TABLE::FindRow( const wxString& aNickname )
   {
  - FP_LIB_TABLE_ROW* row = dynamic_cast< FP_LIB_TABLE_ROW* >( findRow( aNickname ) );
  + FP_LIB_TABLE_ROW* row = static_cast< FP_LIB_TABLE_ROW* >( findRow( aNickname ) );

Application: kicad
Version: (5.0.0-rc2-200-g1f6f76beb), release build
Libraries:
    wxWidgets 3.0.2
    libcurl/7.60.0 OpenSSL/1.0.2k zlib/1.2.11 nghttp2/1.31.1
Platform: FreeBSD 11.1-RELEASE amd64, 64 bit, Little endian, wxGTK
Build Info:
    wxWidgets: 3.0.2 (wchar_t,wx containers,compatible with 2.8) GTK+ 2.24
    Boost: 1.66.0
    OpenCASCADE Technology: 7.2.0
    Curl: 7.60.0
    Compiler: Clang 4.0.0 with C++ ABI 1002
Build settings:
    USE_WX_GRAPHICS_CONTEXT=OFF
    USE_WX_OVERLAY=OFF
    KICAD_SCRIPTING=ON
    KICAD_SCRIPTING_MODULES=ON
    KICAD_SCRIPTING_WXPYTHON=ON
    KICAD_SCRIPTING_ACTION_MENU=OFF
    BUILD_GITHUB_PLUGIN=ON
    KICAD_USE_OCE=OFF
    KICAD_USE_OCC=ON
    KICAD_SPICE=OFF

Revision history for this message
endofexclusive (endofexclusive) wrote :
Revision history for this message
Jeff Young (jeyjey) wrote :

@Devs, as I spent the last 5 years of my career in Java-land, I'm nervous about quantifying the risk of this patch. Is this something that should go in 5.0, or wait for 5.1?

Revision history for this message
Seth Hillbrand (sethh) wrote :

I don't think that this is the right way to approach the issue. If the dynamic cast is not functioning then this needs to be fixed in the class structure not by bypassing the cast.

Revision history for this message
Seth Hillbrand (sethh) wrote :

Looking more closely, Martin is running Clang 4.0.0. There are a few know inheritance bugs in 4.0.0 that were fixed in 4.0.1. This might be one of them.

Revision history for this message
Wayne Stambaugh (stambaughw) wrote :

The patch is also specifically to fix a FreeBSD build using clang 4.0.0. I would prefer that we didn't use this patch to fix what appears to be a platform specific compiler bug. I don't think this issue exists on any other platform/compiler combination. Using dynamic_cast makes sense here because static_cast does not guarantee a valid upcast. @Martin, would you be able to compile KiCad with a different compiler to check if this is indeed a compiler bug?

Jeff Young (jeyjey)
Changed in kicad:
status: New → Incomplete
Revision history for this message
endofexclusive (endofexclusive) wrote :

Thanks Jeff, Seth and Wayne for recognizing this bug report.

Rebuilding KiCad with clang 5.0.1 unfortunately gives the same behavior. My system does currently not have an environment available for compiling with GCC.

I also agree that static_cast may not be a good way to approach the issue, and that if the dynamic cast does not work then the class correctness should be investigated. The static_cast option came up when reading up on the different cast variants available.

An optional patch has been prepared. It does not change any functionality but just converts an implicit assumption into an explicit by asserting that the dynamic cast succeeded. That can be useful if the issue shows up on other systems or compilers in the future.

For reference, the build profile for the Clang 5.0.1 build is as in the original post, with following changes:
  kicad$ git describe master
  5.0.0-rc2-216-gb0b5d9139
  kicad$ clang50 -v
  clang version 5.0.1 (tags/RELEASE_501/final)

Revision history for this message
Seth Hillbrand (sethh) wrote :

Thanks for testing with a newer clang.

It looks like Martin's compiler is using the const version of findRow() unless he explicitly requests the non-const version before attempting the cast.

I think that the extra assert is not required. Also it is a good idea to make a comment above this line explaining why you chose this round-about way of coding so that some optimizing soul doesn't come along in a year or two and change it back.

With those mods, I see no issue with the patch.

Revision history for this message
endofexclusive (endofexclusive) wrote :

Regarding the const version of findRow(), I actually have a patch set for that one in the pipe. The function is in fact not const (!). See the attached patch. However, that may not explain the cast issue.

Revision history for this message
Wayne Stambaugh (stambaughw) wrote : Re: [Bug 1778426] Re: cvpcb can not preview footprints

I'm fine with this patch if the unnecessary assert is removed and a
comment is added about why this change was made as Seth suggested.

On 06/28/2018 04:41 PM, Seth Hillbrand wrote:
> Thanks for testing with a newer clang.
>
> It looks like Martin's compiler is using the const version of findRow()
> unless he explicitly requests the non-const version before attempting
> the cast.
>
> I think that the extra assert is not required. Also it is a good idea
> to make a comment above this line explaining why you chose this round-
> about way of coding so that some optimizing soul doesn't come along in a
> year or two and change it back.
>
> With those mods, I see no issue with the patch.
>

Revision history for this message
Wayne Stambaugh (stambaughw) wrote :

The reason these functions are constant is to prevent code outside of
the library table from modifying rows. I would prefer that we figure
out how to handle refreshing the table by calling insureIndex() when
changes to the table are made rather than making these functions
non-constant.

On 6/28/2018 5:06 PM, endofexclusive wrote:
> Regarding the const version of findRow(), I actually have a patch set
> for that one in the pipe. The function is in fact not const (!). See the
> attached patch. However, that may not explain the cast issue.
>
> ** Patch added: "Removed const version of findRow()"
> https://bugs.launchpad.net/kicad/+bug/1778426/+attachment/5157531/+files/0001-LIB_TABLE-removed-const-version-of-findRow.patch
>

Revision history for this message
KiCad Janitor (kicad-janitor) wrote :

Fixed in revision 9b1f2447b6368ff68ae22b0b3bcbd1a72c8f4a45
https://git.launchpad.net/kicad/patch/?id=9b1f2447b6368ff68ae22b0b3bcbd1a72c8f4a45

Changed in kicad:
status: Incomplete → Fix Committed
assignee: nobody → endofexclusive (endofexclusive)
Revision history for this message
Wayne Stambaugh (stambaughw) wrote :

@Martin, I committed your patch with some minor modifications. Thank you contribution to KiCad.

Changed in kicad:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers