search

epics-modules / mrfioc2

EPICS driver for Micro Research Finland event timing system devices
http://epics-modules.github.io/mrfioc2/
Other
8 stars 30 forks source link

Compiler errors on Windows #41

Closed dirk-zimoch closed 8 months ago

dirk-zimoch commented 3 years ago

Something does not work when compiling with Microsoft cl (actually in wine, but probably the same for native Windows). I'll see if I can find the reason...

/opt/wine-msvc-2017/bin/x64/cl -EHsc -GR -DUSE_TYPED_RSET -DDEBUG_PRINT -nologo -FC -DSTDC=0 -D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE -Ox -GL -Oy- -W3 -w44355 -w44344 -w44251 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -MD -DEPICS_BUILD_DLL -DEPICS_CALL_DLL -TP -I. -I../O.Common -I. -I. -I../os/default -I.. -I../../../include/compiler/msvc -I../../../include/os/WIN32 -I../../../include -I/usr/local/epics/base-7.0.4.1/include/compiler/msvc -I/usr/local/epics/base-7.0.4.1/include/os/WIN32 -I/usr/local/epics/base-7.0.4.1/include -c ../flash.cpp flash.cpp z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\mrf\spi.h(59): error C2332: 'struct': missing tag name z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\mrf\spi.h(59): error C2059: syntax error: 'type' z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\mrf\spi.h(59): error C2334: unexpected token(s) preceding '{'; skipping apparent function body z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\mrf\flash.h(20): warning C4099: 'mrf::SPIDevice': type name first seen using 'class' now seen using 'struct' z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\mrf\spi.h(51): note: see declaration of 'mrf::SPIDevice' z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\mrf\flash.h(58): warning C4267: 'argument': conversion from 'size_t' to 'epicsUInt32', possible loss of data z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\mrf\flash.h(62): warning C4267: 'argument': conversion from 'size_t' to 'epicsUInt32', possible loss of data z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\flash.cpp(45): error C2332: 'struct': missing tag name z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrfcommon\src\flash.cpp(45): error C2039: '': is not a member of 'mrf::SPIDevice'

dirk-zimoch commented 3 years ago

It seems Windows defines interface() as a macro which confuses the compiler in inline SPIInterface* interface() const { return spi; } This line is turned into inline SPIInterface* struct() const { return spi; }

dirk-zimoch commented 3 years ago

See also https://stackoverflow.com/questions/11854331/how-to-disable-win32-interface-macro

dirk-zimoch commented 3 years ago

Also Windows does not like std::min() in flash.cpp because min is a macro too. I'll create a branch to fix the Windows problems.

MarkRivers commented 3 years ago

Also Windows does not like std::min() in flash.cpp because min is a macro too.

I looked through the source and I did not see this line. It is needed for std::min on Windows. \<include algorithm>

MarkRivers commented 3 years ago

I edited the above comment to escape the \<> characters.

dirk-zimoch commented 3 years ago

There is another problem with ObjectInst::m_props:

z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrmshared\src\mrmseq.cpp(780): warning C4273: 'm_props': inconsistent dll linkage z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\include\mrf\object.h(462): note: see previous definition of 'private: static std::multimap<std::basic_string<char,std::char_traits,std::allocator >,mrf::detail::unboundPropertyBase ,std::less<std::basic_string<char,std::char_traits,std::allocator > >,std::allocator<std::pair<std::basic_string<char,std::char_traits,std::allocator > const ,mrf::detail::unboundPropertyBase > > > * mrf::ObjectInst<SeqManager,mrf::Object>::m_props' z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrmshared\src\mrmseq.cpp(780): error C2491: 'mrf::ObjectInst<SeqManager,mrf::Object>::m_props': definition of dllimport static data member not allowed z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\mrmshared\src\mrmseq.cpp(782): error C2491: 'mrf::ObjectInst<SeqManager,mrf::Object>::initObject': definition of dllimport function not allowed

It seems Windows does not like to have static members in exported classes or what?

mdavidsaver commented 3 years ago

Also Windows does not like std::min() in flash.cpp because min is a macro too. I'll create a branch to fix the Windows problems.

Subsequent to mrfioc2, the rule I've developed for myself is to avoid using std::min()/max() in public headers, and to add USR_CPPFLAGS_WIN32 += -DNOMINMAX in configure/CONFIG_SITE. By chance I don't see any usage in any mrfioc2 headers, so I think this can be applied here as well. This might be extended to WIN32_LEAN_AND_MEAN as well.

https://github.com/epics-base/pvAccessCPP/blob/53cd4fa3effd38e443555f2ecd345f5eb9b87995/configure/CONFIG_SITE#L26-L27

dirk-zimoch commented 3 years ago

I have pushed a branch with my changes so far: https://github.com/dirk-zimoch/mrfioc2/tree/dirks_windows_fixes Not finished yet...

mdavidsaver commented 3 years ago

It seems Windows does not like to have static members in exported classes or what?

This looks like another example of the sort of header ordering nightmare which has characterized my experience of shareLib.h. I can see two possible approaches.

  1. Replace use of shareLib.h with the *_API pattern. I've done this in several of the PVA modules, and now parts of Base. @MarkRivers has recently done the same for asyn. It's an large, but mostly mechanical change.

  2. Troubleshooting shareLib.h ordering issues. I don't have an automatic recipe for this. The best I could ever do was to examine the dependency files (eg. mrmShared/src/O.linux-x86_64/mrmSeq.d). One overly strict, but fairly easy to audit on Linux, rule is that absolute paths (from external dependencies) and $(TOP)/include paths (internal dependencies) should precede any headers from the current directory. A quick glance at mrmSeq.d shows ../mrmSeq.h right in the middle of the list. So simply adding #define epicsExportSharedSymbols to mrmSeq.cpp isn't going to be sufficient.

MarkRivers commented 3 years ago

@MarkRivers has recently done the same for asyn.

I have also done it for ADCore and ADGenICam. https://github.com/areaDetector/ADCore https://github.com/areaDetector/ADGenICam

mdavidsaver commented 3 years ago

It seems Windows does not like to have static members in exported classes or what?

This looks like another example of the sort of header ordering nightmare ...

Turns out I was wrong. The problem is a deeper flaw with the idea behind class ObjectInst<T>. Specifically, the m_props static data member needs to be dllexport'd from whichever library includes the OBJECT_BEGIN() block for T, and potentially dllimport'd by whichever library instantiates T.

The problem arises because the ObjectInst<T> template class is only decorated to be exported by one library, but needs to be exported from 4.

https://github.com/epics-modules/mrfioc2/blob/340f46aa8eb1c12f511ebd7ad45ea5b06e306f7a/mrfCommon/src/mrf/object.h#L458-L462

Mixing c++ templates and out-of-line definitions is known to be can be tricky, and this is a good example of why... I'm not immediately sure how to deal with this.

FYI. I tried replacing epicsShare* with *_API macros on a branch, and enabled appveyor builds. I've also applied several of your patches. I'll probably merge the change to *_API, and it would simplify merging any further work by you (@dirk-zimoch) if it were based on this branch.

https://github.com/mdavidsaver/mrfioc2/tree/replace-shareLib

https://ci.appveyor.com/project/mdavidsaver/mrfioc2-dev

dirk-zimoch commented 3 years ago

Windows is still not happy with the m_props member. z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\evrapp\src\evr.cpp(242): warning C4273: 'm_props': inconsistent dll linkage z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\include\mrf\object.h(462): note: see previous definition of 'private: static std::multimap<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,mrf::detail::unboundPropertyBase<DelayModuleEvr> *,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> > >,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> > const ,mrf::detail::unboundPropertyBase<DelayModuleEvr> *> > > * mrf::ObjectInst<DelayModuleEvr,mrf::Object>::m_props' z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\evrapp\src\evr.cpp(242): error C2491: 'mrf::ObjectInst<DelayModuleEvr,mrf::Object>::m_props': definition of dllimport static data member not allowed z:\afs\psi.ch\group\8211\dirk\git\mrfioc2\evrapp\src\evr.cpp(248): error C2491: 'mrf::ObjectInst<DelayModuleEvr,mrf::Object>::initObject': definition of dllimport function not allowed I have no idea how to fix this Windows craziness. And the macro/template mix does not make it easier to understand what exactly Windows is complaining about.

dirk-zimoch commented 3 years ago

mrfCommonApi.h switches dllexport/dllimport on BUILDING_MRFCOMMON_API. When evr.cpp is compiled, BUILDING_MRFCOMMON_API is not defined but BUILDING_EVR_API is. This makes the MRFCOMMON_API macro expand as declspec(dllimport) but you need declspec(dllexport), I think.

It seems nothing has really improved changing epicsShareWhatever for *_API.

Maybe @MarkRivers can solve this. I can't.

mdavidsaver commented 3 years ago

@dirk-zimoch @MarkRivers I don't think this issue can be resolved with only changes to dllimport/export. It's going to require at least a partial redesign of mrf/object.h. The way I did this just isn't compatible with how dllimport/export works.

jerzyjamroz commented 8 months ago

If it is still valid then the ticket can be reopened.