search

epics-base / pvDataCPP

pvDataCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvDataCPP/
Other
6 stars 16 forks source link

workarounds for clang-cl #87

Open xiaoqiangwang opened 1 year ago

xiaoqiangwang commented 1 year ago

This is related to https://github.com/epics-base/epics-base/pull/391 to add clang-cl compiler support to EPICS base.

I put this draft here and wish to hear whether clang-cl support is worth the rather ugly workaround.

AppVeyorBot commented 1 year ago

:white_check_mark: Build pvDataCPP 1.0.31 completed (commit https://github.com/epics-base/pvDataCPP/commit/949f930bb9 by @xiaoqiangwang)

mdavidsaver commented 1 year ago

I put this draft here and wish to hear whether clang-cl support is worth the rather ugly workaround.

This is indeed an ugly change. I'm not sure it is standards defined behavior. It is certainly not common practice. Although "common practice" would likely be a set of explicit specializations, which would be even more verbose.

cf. Why can’t I separate the definition of my templates class from its declaration and put it inside a .cpp file?.

mdavidsaver commented 1 year ago

I've encountered a linker error while trying to build this change locally. This is on Linux with GCC 10.2.1. For my locally builds I set -fvisibility=hidden -fvisibility-inlines-hidden, which I now notice that none of the CI builds do. This matches my understanding that marking a class with __attribute__ ((visibility("default"))) is not identical to marking all of on the members individually. Marking the class is necessary to make certain other symbols visible (typeinfo and I think some symbols involved in exception unwinding).


/usr/bin/g++ -o testOverrunBitSet  -L/home/mdavidsaver/work/epics/pv/pvData/lib/linux-x86_64 -L/home/mdavidsaver/work/epics/base-git/lib/linux-x86_64 -Wl,-rpath-link,/home/mdavidsaver/work/epics/pv/pvData/lib/linux-x86_64 -Wl,-rpath,\$ORIGIN/../../lib/linux-x86_64 -Wl,-rpath-link,/home/mdavidsaver/work/epics/base-git/lib/linux-x86_64 -Wl,-rpath,\$ORIGIN/../../../../base-git/lib/linux-x86_64          -rdynamic -m64         testOverrunBitSet.o    -lpvData -lCom  -lz 
/usr/bin/ld: testOverrunBitSet.o: warning: relocation against `_ZTIN5epics6pvData8PVStringE' in read-only section `.text'
/usr/bin/ld: testOverrunBitSet.o: in function `std::shared_ptr<epics::pvData::PVString> std::dynamic_pointer_cast<epics::pvData::PVString, epics::pvData::PVField>(std::shared_ptr<epics::pvData::PVField> const&)':
/usr/include/c++/10/bits/shared_ptr.h:602: undefined reference to `typeinfo for epics::pvData::PVString'
/usr/bin/ld: warning: creating DT_TEXTREL in a PIE

There is a possibility that doing something like explicitly instantiating the destructor while have the effect of making the typeinfo visible, but this is a guess on my part.

Overall, I expect that some alternate solution/workaround will need to be found.

mdavidsaver commented 1 year ago

From my reading of the clang issue, the trigger is PVScalarValue::typeCode.

https://github.com/epics-base/pvDataCPP/blob/c16f19c80e714e37462e4ada9fa6f263d88adcd0/src/pv/pvData.h#L386

and the associated explicit specializations.

https://github.com/epics-base/pvDataCPP/blob/c16f19c80e714e37462e4ada9fa6f263d88adcd0/src/factory/PVDataCreateFactory.cpp#L34-L35

An alternative pattern/trick to include constant values in a template instance, which might work better, would be to replace the static member typeCode with an enum. Again, specialized for each value type. eg. ScalarTypeID

https://github.com/epics-base/pvDataCPP/blob/c16f19c80e714e37462e4ada9fa6f263d88adcd0/src/pv/pvIntrospect.h#L1434-L1439

This is a bit uglier as it requires casting the "dummy" enum back into the actual type. eg.

https://github.com/epics-base/pvDataCPP/blob/c16f19c80e714e37462e4ada9fa6f263d88adcd0/src/misc/pv/sharedVector.h#L703

However, an enum has no associated symbols, and so causes no additional dllimport/export issues.

mdavidsaver commented 1 year ago

Merge branch 'epics-base:master' into fix_clang_windows

When updating, please git rebase. Do not include merge commits in PRs for the module.

mdavidsaver commented 1 year ago

Also as a note, I'm still not certain that I understand why this change can work at all. My thinking is that the presence of the explicit specializations of typeCode have the side effect of emitting some/all symbols for these classes into PVDataCreateFactory.o. And if this is so, I'm not clear of how portable this behavior is.

https://github.com/epics-base/pvDataCPP/blob/c16f19c80e714e37462e4ada9fa6f263d88adcd0/src/factory/PVDataCreateFactory.cpp#L34-L35

Reading https://en.cppreference.com/w/cpp/language/class_template I find this paragraph under "Explicit instantiation".

When an explicit instantiation names a class template specialization, it serves as an explicit instantiation of the same kind (declaration or definition) of each of its non-inherited non-template members that has not been previously explicitly specialized in the translation unit. If this explicit instantiation is a definition, it is also an explicit instantiation definition only for the members that have been defined at this point.

Which almost describes the situation, except for the non-template members exclusion.

xiaoqiangwang commented 1 year ago

As searching for a better workaround, I found clang-cl works if typeCode has the constexpr specifier. The following change makes use of constexpr if C++11 is detected. https://github.com/xiaoqiangwang/pvDataCPP/commit/c09f6d8425421ed94931311eb8daa46e3d9c6565

The change assumes that user code will not specialize PVScalarValue and PVValueArray templates with other data types.

mdavidsaver commented 1 year ago

... makes use of constexpr if C++11 is detected ...

Which would then be required when clang-cl is used? I guess this seems workable for what is effectively a new compiler, with its own set of quirks.

Will you be updating your PR? (I'd suggest retaining your current set of changes as a local branch)

Also, how can clang-cl be distinguished from "classic" clang (clang-gcc?) or actual MSVC? Or for that matter, from some of the clang + mingw variants (which I've only read about, but not used).

eg. if we want an explicit error if this limitation is encountered, instead of failing with an obscure linker error. Would something like the following work?

#if defined(_MSC_VER) && defined(__clang__) && __cplusplus < 201103L
#  error clang-ci requires >= c++11
#endif
xiaoqiangwang commented 1 year ago

Will you be updating your PR? (I'd suggest retaining your current set of changes as a local branch)

Do I understand that to keep this PR, I would need to reset this branch (after backing up) and then apply the constexpr fixes.

Also, how can clang-cl be distinguished from "classic" clang (clang-gcc?) or actual MSVC? Or for that matter, from some of the clang + mingw variants (which I've only read about, but not used).

I have the same question in the https://github.com/epics-base/epics-base/pull/391#issuecomment-1562700555.

eg. if we want an explicit error if this limitation is encountered, instead of failing with an obscure linker error. Would something like the following work?

I checked that "llvm-rc.exe"(needed for libcom module) is not created until LLVM 7, which defaults to C++14. So the minimum requirement could be probably simply imposed on the whole support of windows clang.

Update: llvm-rc.exe cannot compile the Com.rc file until LLVM 9 13. So that would be the minimum required version.

AppVeyorBot commented 1 year ago

:white_check_mark: Build pvDataCPP 1.0.34 completed (commit https://github.com/epics-base/pvDataCPP/commit/71cbcf24a7 by @xiaoqiangwang)