search

roboticslab-uc3m / kinematics-dynamics

Kinematics and dynamics solvers and controllers.
https://robots.uc3m.es/kinematics-dynamics/
GNU Lesser General Public License v2.1
19 stars 12 forks source link

swig bindings: Error: Empty character constant #150

Closed jgvictores closed 6 years ago

jgvictores commented 6 years ago 
~/repos/kinematics-dynamics/bindings/build (develop u=)$ make
[ 33%] Swig source
/usr/local/include/ICartesianSolver.h:30: Error: Empty character constant
/usr/local/include/ICartesianSolver.h:30: Error: Syntax error in input(3).
CMakeFiles/_kinematics_dynamics.dir/build.make:61: recipe for target 'kinematics_dynamicsPYTHON_wrap.cxx' failed
make[2]: *** [kinematics_dynamicsPYTHON_wrap.cxx] Error 1
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/_kinematics_dynamics.dir/all' failed
make[1]: *** [CMakeFiles/_kinematics_dynamics.dir/all] Error 2
Makefile:127: recipe for target 'all' failed
make: *** [all] Error 2

Blame: https://github.com/roboticslab-uc3m/kinematics-dynamics/blob/59f87ce6c96a3e365b4db189da470e71be2fd279/libraries/YarpPlugins/ICartesianSolver.h#L23-L32

PeterBowman commented 6 years ago

This post and https://github.com/swig/swig/issues/737 gave me a hint. Probably a regression introduced by https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/59f87ce6c96a3e365b4db189da470e71be2fd279 which triggers when the character to be expanded (e.g. b in VOCAB('c','p','f','b')) is used as a parameter in the macro definition:

https://github.com/roboticslab-uc3m/kinematics-dynamics/blob/59f87ce6c96a3e365b4db189da470e71be2fd279/libraries/YarpPlugins/ICartesianSolver.h#L8

I'll check this.

PeterBowman commented 6 years ago

Probably a regression introduced by 59f87ce...

Confirmed.

...when the character to be expanded (e.g. b in VOCAB('c','p','f','b')) is used as a parameter in the macro definition

True, SWIG compiles successfully with the following definition, both in ICartesianSolver.h and ICartesianControl.h:

#define VOCAB(x1,x2,x3,x4) ((((int)(x4))<<24)+(((int)(x3))<<16)+(((int)(x2))<<8)+((int)(x1)))

It chokes whenever the macro definitions differ between both headers. However, now I get plenty of gcc warnings when compiling the project itself (not the associated bindings) because of the redefinition of VOCAB wrt. YARP's own macro.

PeterBowman commented 6 years ago

Sometimes I hear about C++11 constexpr constants/functions/classes being a modern alternative to preprocessor macros. Perhaps this is something SWIG would not complain about.

jgvictores commented 6 years ago

Okay, so maybe it's late, but... : )

#define ROBOTICSLAB_VOCAB(x1,x2,x3,x4) ((((int)(x4))<<24)+(((int)(x3))<<16)+(((int)(x2))<<8)+((int)(x1)))

PS: Or always avoid a/b/c/d :joy:

PeterBowman commented 6 years ago

constexpr sounds interesting, but does not avoid the redeclaration issue.

The original macro needs to be undefined prior to declaring the constexpr function, which in turn would appear only once: in ICartesianSolver.h.

// ICartesianSolver.h
#undef VOCAB
constexpr int VOCAB(int a, int b, int c, int d) { return (d << 24) + (c << 16) + (b << 8) + a; }

This solution comes at the expense of losing explicitness, note that most of our macros are defined within ICartesianControl. (which, in turn, transcludes the solver header). Your proposal looks simpler and more elegant to me (albeit notably C-ish). On the other hand, consumers may want to define their own vocabs and scratch their head upon choosing between VOCAB and ROBOTICSLAB_VOCAB, then.

PeterBowman commented 6 years ago

Well, with constexprs one could write:

constexpr int VOCAB_CC_OK = VOCAB2('o','k');

instead of:

#define VOCAB_CC_OK VOCAB('o','k',0,0) // note the two zeroes

I guess it's a matter of taste.

jgvictores commented 6 years ago

The original macro needs to be undefined prior to declaring (...)

Wouldn't that result in redefinitions if the compiler runs into YARP's version after running into ours?

PeterBowman commented 6 years ago

Yes, but it works anyway. The other way around is what makes the compiler protest: first the macro, then the constexpr. Hence I had to #undef the macro.

(Actually, it makes sense. All the preprocessor does is a raw substitution, so we end up with an useless VOCAB symbol defined via constexpr and all usages substed in the following code.)

(Edit: I wouldn't call it a redefinition, then :). Macros and constexpr symbols are two different beasts.)

jgvictores commented 6 years ago

Did the #define ROBOTICSLAB_VOCAB(x1,x2,x3,x4) ((((int)(x4))<<24)+(((int)(x3))<<16)+(((int)(x2))<<8)+((int)(x1))) hack. Result:

In [2]: kinematics_dynamics.ICartesianSolver.BASE_FRAME
Out[2]: 'c'
In [3]: kinematics_dynamics.ICartesianSolver.TCP_FRAME
Out[3]: 'c'

However:

In [2]: yarp.YARP_FEATURE_TILT
Out[2]: 18
In [3]: yarp.YARP_FEATURE_TRIGGER
Out[3]: 12
jgvictores commented 6 years ago

Tried with constexpr, which result in a redefinition error.

PeterBowman commented 6 years ago

Tried with constexpr, which result in a redefinition error.

Did you delete the copy in ICartesianControl.h?

jgvictores commented 6 years ago

I thought it should be in both...

Anyway, I just found this code in a YARP enum:

VOCAB_PIXEL_BGRA = VOCAB4(98/*'b'*/,'g','r','a'), /* SWIG BUG */
jgvictores commented 6 years ago

...which traces back to a 2011 commit... https://github.com/robotology/yarp/commit/d7b4099f9ba20f7f7f3795457a8c548a08735e76 (which apparently mentions /me LOL).

PeterBowman commented 6 years ago

I wonder if this has been finally fixed in SWIG 3.0.11 (currently stuck at 3.0.8 in Xenial, but Bionic moved on to 3.0.12), see https://sourceforge.net/p/swig/bugs/1168/.

(Edit: I mean the original issue as explained in the first comments here.)

jgvictores commented 6 years ago

I wonder if this has been finally fixed in SWIG 3.0.11 (currently stuck at 3.0.8 in Xenial, but Bionic moved on to 3.0.12), see https://sourceforge.net/p/swig/bugs/1168/.

Interesting.

jgvictores commented 6 years ago

Just tracking where this is done in YARP at time of writing:

libYARP_sig/include/yarp/sig/Image.h:48:    VOCAB_PIXEL_BGRA = VOCAB4(98/*'b'*/,'g','r','a'), /* SWIG BUG */
libYARP_sig/include/yarp/sig/Image.h:51:    VOCAB_PIXEL_BGR = VOCAB3(98/*'b'*/,'g','r'), /* SWIG BUG */
libYARP_sig/include/yarp/sig/Image.h:60:    VOCAB_PIXEL_ENCODING_BAYER_BGGR8 = VOCAB4(98/*'b'*/, 'g', 'g', 'r'),     //bggr8
libYARP_sig/include/yarp/sig/Image.h:61:    VOCAB_PIXEL_ENCODING_BAYER_BGGR16 = VOCAB4(98/*'b'*/, 'g', '1', '6'),  //bggr16
libYARP_dev/include/yarp/dev/IPidControl.h:28:            VOCAB_PIDTYPE_CURRENT  = VOCAB3(99/*'c'*/, 'u', 'r') // SWIG bug
libYARP_OS/include/yarp/os/PortInfo.h:40:        PORTINFO_CONNECTION = VOCAB4(99/*c*/, 'o', 'n', 'n'),
jgvictores commented 6 years ago

So, maintaining our VOCAB duplicate off YARP, we can simply patch as them:

-            BASE_FRAME = VOCAB('c','p','f','b'), //!< Base frame
-            TCP_FRAME = VOCAB('c','p','f','t')   //!< End-effector frame (TCP)
+            BASE_FRAME = VOCAB(99/*'c'*/,'p','f','b'), //!< Base frame
+            TCP_FRAME = VOCAB(99/*'c'*/,'p','f','t')   //!< End-effector frame (TCP)

And get working results in Python:

In [3]: kinematics_dynamics.ICartesianSolver.BASE_FRAME
Out[3]: 1650880611
In [4]: kinematics_dynamics.ICartesianSolver.TCP_FRAME
Out[4]: 1952870499
jgvictores commented 6 years ago

Note that VOCABs that were breaking were in an enum in a class (the lines stated at the beginning of this issue): https://github.com/roboticslab-uc3m/kinematics-dynamics/blob/59f87ce6c96a3e365b4db189da470e71be2fd279/libraries/YarpPlugins/ICartesianSolver.h#L23-L32

VOCABs outside of that kind of scope do not suffer, or we cannot know as they are not "seen" by SWIG: https://github.com/roboticslab-uc3m/kinematics-dynamics/blob/59f87ce6c96a3e365b4db189da470e71be2fd279/libraries/YarpPlugins/ICartesianControl.h#L87

jgvictores commented 6 years ago

In the context of an enum in a class:

This makes me believe, yes, there are at least 2 issues here.

The only fast and safe mechanism seems to copy and use YARP's.

jgvictores commented 6 years ago

Wait... it seems like the, in the context of an enum in a class... the only good option is the unchanged YARP mechanism PLUS using the number hack ALWAYS... and even this is still untested.

jgvictores commented 6 years ago

Nothing strange from swig -E ICartesianSolver.h (from SWIG Preprocessor docs):

            BASE_FRAME = ((((int)('b'))<<24)+(((int)('f'))<<16)+(((int)('p'))<<8)+((int)(99/*'c'*/))), //!< Base frame
            TCP_FRAME = ((((int)('t'))<<24)+(((int)('f'))<<16)+(((int)('p'))<<8)+((int)(99/*'c'*/))),   //!< End-effector frame (TCP)
            TCP_FRAMEA = ((((int)('t'))<<24)+(((int)('f'))<<16)+(((int)('p'))<<8)+((int)('a'))),   //!< End-effector frame (TCP)
            TCP_FRAMEB = ((((int)('t'))<<24)+(((int)('f'))<<16)+(((int)('p'))<<8)+((int)('b'))),   //!< End-effector frame (TCP)
            TCP_FRAMEC = ((((int)('t'))<<24)+(((int)('f'))<<16)+(((int)('p'))<<8)+((int)('c'))),   //!< End-effector frame (TCP)
            TCP_FRAMED = ((((int)('t'))<<24)+(((int)('f'))<<16)+(((int)('p'))<<8)+((int)('d'))),   //!< End-effector frame (TCP)
            TCP_FRAMEE = ((((int)('t'))<<24)+(((int)('f'))<<16)+(((int)('p'))<<8)+((int)('e'))),   //!< End-effector frame (TCP)
            TCP_FRAMEF = ((((int)('t'))<<24)+(((int)('f'))<<16)+(((int)('p'))<<8)+((int)('f')))   //!< End-effector frame (TCP)
jgvictores commented 6 years ago

I wonder if this has been finally fixed in SWIG 3.0.11 (currently stuck at 3.0.8 in Xenial, but Bionic moved on to 3.0.12)

Must check.

jgvictores commented 6 years ago

Just to update, SWIG bug is not fixed in 3.0.12.

A functional workaround is the one provided by 712894c (SWIG_PREPROCESSOR_SHOULD_SKIP_THIS).

jgvictores commented 6 years ago

Fixed upstream in YARP via PR https://github.com/robotology/yarp/pull/1655 merged at https://github.com/robotology/yarp/commit/c23b8d3944bb0a2cb64f03910e888e2954fa987a

Same fix applied in this repo at 651ceaa (undoing hack too) and 7a4fe85, and additionally in https://github.com/asrob-uc3m/yarp-devices at https://github.com/asrob-uc3m/yarp-devices/commit/400b195ad057723edd1c9f8bed7b47709bfc94c7.

Closing issue!

jgvictores commented 6 years ago

This is in fact the original issue, so continuing here.

The cast was an issue, but can be hacked. I've even tried this across files:

// example.h
#define VOCAB(x1,x2,x3,x4) ((x1+0)<<24)+((x2+0)<<16)+((x3+0)<<8)+(x4+0)
#define VOCAB3(a,b,c) VOCAB(a,b,c,0)

#define TEST1 VOCAB('b','b','c','d')
#define TEST2 VOCAB3('f','b','c')
// example2.h
#include "example.h"

#define TEST3 VOCAB('b','b','c','d')
#define TEST4 VOCAB3('f','b','c')
// example.i
%module example
%{
//#include "example.h" // Not even required for this.
//#include "example2.h" // Not even required for this.
%}
%include "example.h"
%include "example2.h"

Python wrapper can be compiled with:

rm -rf _example.so example_wrap.c example_wrap.o example.py __pycache__
swig -python example.i
gcc -c example_wrap.c -I /usr/include/python3.5m/ -fPIC
ld -shared example.o example_wrap.o -o _example.so

And check variables are created:

grep TEST example_wrap.c example.py

Outputs:

example_wrap.c:  SWIG_Python_SetConstant(d, "TEST1",SWIG_From_int((int)((('b'+0) << 24) +(('b'+0) << 16) +(('c'+0) << 8) +('d'+0))));
example_wrap.c:  SWIG_Python_SetConstant(d, "TEST2",SWIG_From_int((int)((('f'+0) << 24) +(('b'+0) << 16) +(('c'+0) << 8) +(0+0))));
example_wrap.c:  SWIG_Python_SetConstant(d, "TEST3",SWIG_From_int((int)((('b'+0) << 24) +(('b'+0) << 16) +(('c'+0) << 8) +('d'+0))));
example_wrap.c:  SWIG_Python_SetConstant(d, "TEST4",SWIG_From_int((int)((('f'+0) << 24) +(('b'+0) << 16) +(('c'+0) << 8) +(0+0))));
example.py:TEST1 = _example.TEST1
example.py:TEST2 = _example.TEST2
example.py:TEST3 = _example.TEST3
example.py:TEST4 = _example.TEST4

And values via python/ipython are correct.

Issue is most probably something in yarp.i. In fact, the above VOCAB hack allows whatever is in Vocab.h to be generated correctly. There is, however, something strange (typedef? cstring.i?) that makes other files change from a char e.g. 'x' to a variable with the same name (e.g. x), see:

[100%] Building CXX object bindings/python/CMakeFiles/_yarp_python.dir/__/__/lib/python/yarpPYTHON_wrap.cxx.o
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx: In function ‘PyObject* PyInit__yarp()’:
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149928:93: error: ‘f’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_FRAMEGRABBER_IMAGE",SWIG_From_int(static_cast< int >((((f) +0) << 24) +(((g) +0) << 16) +(((i) +0) << 8) +((0) +0))));
                                                                                             ^
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149928:111: error: ‘g’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_FRAMEGRABBER_IMAGE",SWIG_From_int(static_cast< int >((((f) +0) << 24) +(((g) +0) << 16) +(((i) +0) << 8) +((0) +0))));
                                                                                                               ^
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149928:129: error: ‘i’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_FRAMEGRABBER_IMAGE",SWIG_From_int(static_cast< int >((((f) +0) << 24) +(((g) +0) << 16) +(((i) +0) << 8) +((0) +0))));
                                                                                                                                 ^
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149929:148: error: ‘r’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_FRAMEGRABBER_IMAGERAW",SWIG_From_int(static_cast< int >((((f) +0) << 24) +(((g) +0) << 16) +(((i) +0) << 8) +((r) +0))));
                                                                                                                                                    ^
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149930:85: error: ‘b’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_BRIGHTNESS",SWIG_From_int(static_cast< int >((((b) +0) << 24) +(((r) +0) << 16) +(((i) +0) << 8) +((0) +0))));
                                                                                     ^
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149931:83: error: ‘e’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_EXPOSURE",SWIG_From_int(static_cast< int >((((e) +0) << 24) +(((x) +0) << 16) +(((p) +0) << 8) +((o) +0))));
                                                                                   ^
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149931:101: error: ‘x’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_EXPOSURE",SWIG_From_int(static_cast< int >((((e) +0) << 24) +(((x) +0) << 16) +(((p) +0) << 8) +((o) +0))));
                                                                                                     ^
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149931:119: error: ‘p’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_EXPOSURE",SWIG_From_int(static_cast< int >((((e) +0) << 24) +(((x) +0) << 16) +(((p) +0) << 8) +((o) +0))));
                                                                                                                       ^
/home/yo/repos/yarp/build/lib/python/yarpPYTHON_wrap.cxx:149931:135: error: ‘o’ was not declared in this scope
   SWIG_Python_SetConstant(d, "VOCAB_EXPOSURE",SWIG_From_int(static_cast< int >((((e) +0) << 24) +(((x) +0) << 16) +(((p) +0) << 8) +((o) +0))));

This, on the other hand, was in fact to be suspected given the conflicts with vocabs that started with 'b'/'c'/'d'.

jgvictores commented 6 years ago

Fix has been implemented and merged upstream, https://github.com/robotology/yarp/pull/1696