Bug 534 - Build error with clang: undefined reference to `sigrok::EnumValue<sigrok::LogLevel, sr_loglevel>::_values'
Summary: Build error with clang: undefined reference to `sigrok::EnumValue<sigrok::Log...
Status: RESOLVED FIXED
Alias: None
Product: PulseView
Classification: Unclassified
Component: Other (show other bugs)
Version: unreleased development snapshot
Hardware: All All
: Normal normal
Target Milestone: ---
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-03 23:41 CET by Uwe Hermann
Modified: 2015-01-26 16:05 CET (History)
2 users (show)



Attachments
libsigrok-bindings-cxx-enums-cpp.patch (1.37 KB, patch)
2015-01-04 06:33 CET, Uffe Jakobsen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Uwe Hermann 2015-01-03 23:41:54 CET
When building PulseView (and libsigrok etc) with clang/clang++ there are errors apparently:

[ 94%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/view/moc_trace.cpp.o
[ 95%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/view/moc_tracegroup.cpp.o
Linking CXX executable pulseview
[ 95%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/view/moc_view.cpp.o
[ 95%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/view/moc_viewitem.cpp.o
CMakeFiles/pulseview.dir/main.cpp.o: In function `std::_Rb_tree<sr_loglevel const, std::pair<sr_loglevel const, sigrok::LogLevel const* const>, std::_Select1st<std::pair<sr_loglevel const, sigrok::LogLevel const* const> >, std::less<sr_loglevel const>, std::allocator<std::pair<sr_loglevel const, sigrok::LogLevel const* const> > >::_M_begin() const':
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.9/../../../../include/c++/4.9/bits/stl_tree.h:523: undefined reference to `sigrok::EnumValue<sigrok::LogLevel, sr_loglevel>::_values'
CMakeFiles/pulseview.dir/pv/view/logicsignal.cpp.o: In function `std::_Rb_tree<sr_trigger_matches const, std::pair<sr_trigger_matches const, sigrok::TriggerMatchType const* const>, std::_Select1st<std::pair<sr_trigger_matches const, sigrok::TriggerMatchType const* const> >, std::less<sr_trigger_matches const>, std::allocator<std::pair<sr_trigger_matches const, sigrok::TriggerMatchType const* const> > >::_M_begin() const':
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.9/../../../../include/c++/4.9/bits/stl_tree.h:523: undefined reference to `sigrok::EnumValue<sigrok::TriggerMatchType, sr_trigger_matches>::_values'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
CMakeFiles/pulseview.dir/build.make:3024: recipe for target 'pulseview' failed
make[2]: *** [pulseview] Error 1
CMakeFiles/Makefile2:60: recipe for target 'CMakeFiles/pulseview.dir/all' failed
make[1]: *** [CMakeFiles/pulseview.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 96%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/view/moc_viewport.cpp.o
[ 96%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/view/moc_viewwidget.cpp.o
[ 96%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/widgets/moc_colourbutton.cpp.o
[ 97%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/widgets/moc_colourpopup.cpp.o
[ 97%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/widgets/moc_popup.cpp.o
[ 97%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/widgets/moc_popuptoolbutton.cpp.o
[ 98%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/widgets/moc_sweeptimingwidget.cpp.o
[ 98%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/widgets/moc_wellarray.cpp.o
[ 98%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/data/moc_decoderstack.cpp.o
[ 99%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/view/moc_decodetrace.cpp.o
[ 99%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/widgets/moc_decodergroupbox.cpp.o
[100%] Building CXX object test/CMakeFiles/pulseview-test.dir/__/pv/widgets/moc_decodermenu.cpp.o
Linking CXX executable pulseview-test
CMakeFiles/pulseview-test.dir/__/pv/view/logicsignal.cpp.o: In function `std::_Rb_tree<sr_trigger_matches const, std::pair<sr_trigger_matches const, sigrok::TriggerMatchType const* const>, std::_Select1st<std::pair<sr_trigger_matches const, sigrok::TriggerMatchType const* const> >, std::less<sr_trigger_matches const>, std::allocator<std::pair<sr_trigger_matches const, sigrok::TriggerMatchType const* const> > >::_M_begin() const':
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.9/../../../../include/c++/4.9/bits/stl_tree.h:523: undefined reference to `sigrok::EnumValue<sigrok::TriggerMatchType, sr_trigger_matches>::_values'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
test/CMakeFiles/pulseview-test.dir/build.make:2656: recipe for target 'test/pulseview-test' failed
make[2]: *** [test/pulseview-test] Error 1
CMakeFiles/Makefile2:110: recipe for target 'test/CMakeFiles/pulseview-test.dir/all' failed
make[1]: *** [test/CMakeFiles/pulseview-test.dir/all] Error 2
Makefile:147: recipe for target 'all' failed
make: *** [all] Error 2
Comment 1 Uffe Jakobsen 2015-01-04 06:31:59 CET
Hi,

Follow up: regarding the pulseview link problems using clang/clang++

The problem seems to be related to the use of option -fvisibility=hidden and the "SR_API" visibility attributes.

The problem seems to occour because of references from outside the libsigrokxx library to some of the classes (inside libsigrokxx) that use static initialization through explicit template specialization.

Modifying the autogenerated bindings/cxx/enums.cpp by hand (see diff below) - adding "SR_API" to the explicit template specialization fixes the problem and pulseview is able to compile,link and execute.

One problem is that bindings/cxx/enums.cpp is autogenerated.

The puzzle, that I'm too tired to solve right now, is why the problem only relates to classes "LogLevel" and "TriggerMatchType" ?
There are lots of other classes based on the EnumValue template that does not have this problem - and they all use the same approach for (static) initialization through explicit template specialization...

I need to sleep now

/Uffe

--- bindings/cxx/enums.cpp.orig    2015-01-04 06:11:07.623170506 +0100
+++ bindings/cxx/enums.cpp    2015-01-04 06:11:47.577080031 +0100
@@ -11,7 +11,7 @@
 const LogLevel * const LogLevel::INFO = &LogLevel::_INFO;
 const LogLevel * const LogLevel::DBG = &LogLevel::_DBG;
 const LogLevel * const LogLevel::SPEW = &LogLevel::_SPEW;
-template<> const std::map<const enum sr_loglevel, const LogLevel * const> EnumValue<LogLevel, enum sr_loglevel>::_values = {
+template<> SR_API const std::map<const enum sr_loglevel, const LogLevel * const> EnumValue<LogLevel, enum sr_loglevel>::_values = {
     {SR_LOG_NONE, LogLevel::NONE},
     {SR_LOG_ERR, LogLevel::ERR},
     {SR_LOG_WARN, LogLevel::WARN},
@@ -344,7 +344,7 @@
 const TriggerMatchType * const TriggerMatchType::EDGE = &TriggerMatchType::_EDGE;
 const TriggerMatchType * const TriggerMatchType::OVER = &TriggerMatchType::_OVER;
 const TriggerMatchType * const TriggerMatchType::UNDER = &TriggerMatchType::_UNDER;
-template<> const std::map<const enum sr_trigger_matches, const TriggerMatchType * const> EnumValue<TriggerMatchType, enum sr_trigger_matches>::_values = {
+template<> SR_API const std::map<const enum sr_trigger_matches, const TriggerMatchType * const> EnumValue<TriggerMatchType, enum sr_trigger_matches>::_values = {
     {SR_TRIGGER_ZERO, TriggerMatchType::ZERO},
     {SR_TRIGGER_ONE, TriggerMatchType::ONE},
     {SR_TRIGGER_RISING, TriggerMatchType::RISING},

/Uffe
Comment 2 Uffe Jakobsen 2015-01-04 06:33:59 CET
Created attachment 111 [details]
libsigrok-bindings-cxx-enums-cpp.patch
Comment 3 Uffe Jakobsen 2015-01-04 06:35:53 CET
Sorry about the cross-spamming - I saw the bug ticket after having sent my email to the mailinglist

/Uffe
Comment 4 Martin Ling 2015-01-26 00:22:08 CET
That change needs to be applied to enums.py, which generates enums.cpp:

diff --git a/bindings/cxx/enums.py b/bindings/cxx/enums.py
index 53bd000..aed4212 100644
--- a/bindings/cxx/enums.py
+++ b/bindings/cxx/enums.py
@@ -138,7 +138,7 @@ for enum, (classname, classbrief) in classes.items():
             file=code)
 
     # Define map of enum values to constants
-    print('template<> const std::map<const enum %s, const %s * const> EnumValue<%s, enum %s>::_values = {' % (
+    print('template<> const SR_API std::map<const enum %s, const %s * const> EnumValue<%s, enum %s>::_values = {' % (
         enum_name, classname, classname, enum_name), file=code)
     for name, trimmed_name in zip(member_names, trimmed_names):
         print('\t{%s, %s::%s},' % (name, classname, trimmed_name), file=code)
Comment 5 Uffe Jakobsen 2015-01-26 01:10:36 CET
The change is verified with clang on both ArchLinux and FreeBSD-10.1

pulseview compiles, links and executes with success.

Is there something that I'm missing here - why the need for the change ?

As I wrote earlier in this case:

why the problem only relates to classes "LogLevel" and "TriggerMatchType" ?
There are lots of other classes based on the EnumValue template that does not have this problem - and they all use the same approach for (static) initialization through explicit template specialization...

/Uffe
Comment 6 Martin Ling 2015-01-26 01:27:04 CET
The _values map will only be needed if the get() or values() static methods from the EnumValue class template are used.

E.g. for TriggerMatchType, PV calls this in pv/view/logicsignal.cpp, lines 238 and 387 in revision 5164bbd9.
Comment 7 Uffe Jakobsen 2015-01-26 10:25:15 CET
Yes, but shouldn't it be sufficient to have SR_API in the .hpp header file and not in the .cpp file ?
Comment 8 Uwe Hermann 2015-01-26 15:34:45 CET
Fixed in dc7125bb7cfe34f63695ea928dda17594dfac3d2, thanks guys!

I tested on Linux with clang(++) 3.5, which was having the problem before (and doesn't anymore).
Comment 9 Martin Ling 2015-01-26 16:05:37 CET
(In reply to comment #7)
> Yes, but shouldn't it be sufficient to have SR_API in the .hpp header file
> and not in the .cpp file ?

For normal symbols yes, but this is a template specialisation. I have no idea what the rules ought to be for that really. If you can confirm this shouldn't be required then file a clang bug, but we'll still need this fix for now.