AcademySoftwareFoundation / AcademySoftwareFoundation/Imath

PyImath Bindings fail to build when using Clang on Windows (Release branch, 3.17, issue still present on Main)

Open
#324 10 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
488
Forks
161
Avg merge
1d 3h
Merged PRs (30d)
3

Description

OS: Windows 10 22H2, 19045.2965
Tools:
- bundled with Visual Studio 2022 Community Edition
- CMake 3.26.0-msvc3
- clang-cl 15.0.1
- MSVC cl version 19.36.32532 for x64
- Visual Studio Code (text editor)
- CMake Tools

Libraries:
- Boost 1.82.0 (compiled locally using cmake)
- Python 3.10
- Numpy 1.24.3

cmake configuration:
```
cmake --no-warn-unused-cli -DCMAKE_INSTALL_PREFIX:STRING=C:/lib/imath \
-DCMAKE_VERBOSE_MAKEFILE:BOOL=TRUE \
-DBUILD_TESTING:BOOL=TRUE \
-DBoost_ROOT:STRING=C:/lib/boost \
-DBoost_NO_BOOST_CMAKE:BOOL=FALSE \
-DBoost_USE_STATIC_LIBS:BOOL=FALSE \
-DBoost_USE_MULTITHREADED:BOOL=TRUE \
-DBoost_USE_STATIC_RUNTIME:BOOL=FALSE \
-DPYTHON:BOOL=TRUE \
-DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=TRUE \
-DCMAKE_C_COMPILER=clang-cl \
-DCMAKE_CXX_COMPILER=clang-cl \
-SD:/Code/lib/Imathfork \
-Bd:/Code/lib/Imathfork/build \
-G "Ninja Multi-Config"
```
I've tested using both the Ninja generator and the Visual Studio generator, as well as specifying cxx standards 11 and 17.

Using clang (or clang-cl) to compile the release branch (commit `a4f9d5c`) will error out on the python bindings. I've truncated the log to one of the relevant sections. Apologies if it's messy, it's from VS Code's output window.

```
[build] FAILED: src/python/PyImath/CMakeFiles/PyImath_Python3_10.dir/Release/PyImathAutovectorize.cpp.obj
[build] "C:\PROGRA~1\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\x64\bin\clang-cl.exe" -TP -DBOOST_CONTAINER_DYN_LINK -DBOOST_CONTAINER_NO_LIB -DBOOST_PYTHON_DYN_LINK -DBOOST_PYTHON_NO_LIB -DIMATH_DLL -DPYIMATH_BUILD -DPyImath_Python3_10_EXPORTS -DCMAKE_INTDIR=\"Release\" -ID:\Code\lib\Imath\build\src\python\PyImath -ID:\Code\lib\Imath\src\python\PyImath -ID:\Code\lib\Imath\src\Imath -ID:\Code\lib\Imath\build\config -imsvcC:\lib\boost\include\boost-1_82 -imsvcC:\Python310\include /clang:-march=native /MD /O2 /Ob2 /DNDEBUG /EHsc -std:c++17 /showIncludes /Fosrc\python\PyImath\CMakeFiles\PyImath_Python3_10.dir\Release\PyImathAutovectorize.cpp.obj /Fdsrc\python\PyImath\CMakeFiles\PyImath_Python3_10.dir\Release\ -c -- D:\Code\lib\Imath\src\python\PyImath\PyImathAutovectorize.cpp
[build] In file included from D:\Code\lib\Imath\src\python\PyImath\PyImathAutovectorize.cpp:8:
[build] In file included from D:\Code\lib\Imath\src\python\PyImath/PyImathAutovectorize.h:34:
[build] In file included from D:\Code\lib\Imath\src\python\PyImath/PyImathFixedArray.h:17:
[build] D:\Code\lib\Imath\src\python\PyImath/PyImathUtil.h(43,20): error: attribute 'dllexport' cannot be applied to a deleted function
[build] PYIMATH_EXPORT PyAcquireLock(const PyAcquireLock& other) = delete;
[build] ^
[build] D:\Code\lib\Imath\src\python\PyImath/PyImathUtil.h(44,36): error: attribute 'dllexport' cannot be applied to a deleted function
[build] PYIMATH_EXPORT PyAcquireLock & operator = (PyAcquireLock& other) = delete;
[build] ^
[build] D:\Code\lib\Imath\src\python\PyImath/PyImathUtil.h(45,20): error: attribute 'dllexport' cannot be applied to a deleted function
[build] PYIMATH_EXPORT PyAcquireLock(PyAcquireLock&& other) = delete;
[build] ^
[build] D:\Code\lib\Imath\src\python\PyImath/PyImathUtil.h(46,36): error: attribute 'dllexport' cannot be applied to a deleted function
[build] PYIMATH_EXPORT PyAcquireLock & operator = (PyAcquireLock&& other) = delete;
[build] ^
[build] D:\Code\lib\Imath\src\python\PyImath/PyImathUtil.h(67,20): error: attribute 'dllexport' cannot be applied to a deleted function
[build] PYIMATH_EXPORT PyReleaseLock(const PyReleaseLock& other) = delete;
[build] ^
[build] D:\Code\lib\Imath\src\python\PyImath/PyImathUtil.h(68,36): error: attribute 'dllexport' cannot be applied to a deleted function
[build] PYIMATH_EXPORT PyReleaseLock & operator = (PyReleaseLock& other) = delete;
[build] ^
[build] D:\Code\lib\Imath\src\python\PyImath/PyImathUtil.h(69,20): error: attribute 'dllexport' cannot be applied to a deleted function
[build] PYIMATH_EXPORT PyReleaseLock(PyReleaseLock&& other) = delete;
[build] ^
[build] D:\Code\lib\Imath\src\python\PyImath/PyImathUtil.h(70,36): error: attribute 'dllexport' cannot be applied to a deleted function
[build] PYIMATH_EXPORT PyReleaseLock & operator = (PyReleaseLock&& other) = delete;
[build] ^
[build] 8 errors generated.
```

The issue is tied to `src/python/PyImath/PyImathUtil.h` specifically, where `PYIMATH_EXPORT` is assigned to the deleted copy and move constructors/operators. Clang on Windows will provide a `_MSC_VER` definition, so this resolves to `__declspec(dllexport)` in `PyImathExport.h`. The issue is with the declarations of PyAcquireLock and PyReleaseLock.

> This
>```c++
>class PyAcquireLock
> {
> public:
> PYIMATH_EXPORT PyAcquireLock();
> PYIMATH_EXPORT ~PyAcquireLock();
>
> PYIMATH_EXPORT PyAcquireLock(const PyAcquireLock& other) = delete;
> PYIMATH_EXPORT PyAcquireLock & operator = (PyAcquireLock& other) = delete;
> PYIMATH_EXPORT PyAcquireLock(PyAcquireLock&& other) = delete;
> PYIMATH_EXPORT PyAcquireLock & operator = (PyAcquireLock&& other) = delete;
>
> private:
> PyGILState_STATE _gstate;
> };
> ```
> ends up compiling as
>```c++
>class PyAcquireLock
> {
> public:
> __declspec(dllexport) PyAcquireLock();
> __declspec(dllexport) ~PyAcquireLock();
>
> __declspec(dllexport) PyAcquireLock(const PyAcquireLock& other) = delete;
> __declspec(dllexport) PyAcquireLock & operator = (PyAcquireLock& other) = delete;
> __declspec(dllexport) PyAcquireLock(PyAcquireLock&& other) = delete;
> __declspec(dllexport) PyAcquireLock & operator = (PyAcquireLock&& other) = delete;
>
> private:
> PyGILState_STATE _gstate;
> };
> ```

Clang explicitly disallows dllexport to be assigned to deleted functions; while clang-cl in general attempts to mimic the behavior of MSVC, it would appear that it is rigid in this scenario, while MSVC will still compile. My understanding is that dllexports only apply to non-deleted functions, and as such it's not good practice to be declaring them on deleted functions regardless. Looking through the dumpbin on the generated library file from MSVC, it appears that MSVC's behavior is to not export the deleted functions (the constructors/destructors for `PyAcquireLock` and `PyReleaseLock` are still properly exported), implying it sees the declaration and ignores it for the deleted copy/move constructor/operator functions. Performing a diff on the dumpbins compiled using MSVC with and without the `PYIMATH_EXPORT` declarations on the deleted functions shows that output is equivalent.

Changing the code to
```c++
class PyAcquireLock
{
public:
PYIMATH_EXPORT PyAcquireLock();
PYIMATH_EXPORT ~PyAcquireLock();

PyAcquireLock(const PyAcquireLock& other) = delete;
PyAcquireLock & operator = (PyAcquireLock& other) = delete;
PyAcquireLock(PyAcquireLock&& other) = delete;
PyAcquireLock & operator = (PyAcquireLock&& other) = delete;

private:
PyGILState_STATE _gstate;
};
```
works.

The next issue has to do with the exports declared in an anonymous namespace in `PyImathStringTable.cpp` -- these have internal linkage, and so Clang errors out because `__declspec(dllexport)` was used on functions without external linkage. I'm actually unsure as to how MSVC handles this case (I haven't checked a dump for these symbols), but I'm assuming that it's similar to the above scenario.

> PyImathStringTable.cpp lines 91-94
> ```c++
> namespace {
> template class PYIMATH_EXPORT StringTableDetailT;
> template class PYIMATH_EXPORT StringTableDetailT;
> }
> ```

I'm currently working on a fork to fix compile errors when using Clang on Windows. I've managed to get the program to compile; I still need to run tests to verify that I didn't inadvertently break the python bindings. Before I propose the removal of `PYIMATH_EXPORT` being assigned to the deleted functions, I did want to ask if there's a reason for it to be there -- I'll admit I'm not very familiar with building libraries for sharing, and know little about how symbol visibility and exports are handled beyond what I've managed to look up in response to this error, or how `__attribute__((visibility("default")))` behaves in environments where that is declared instead. If it wouldn't cause issues (and assuming it's not too minor for a PR), I'll submit a PR once I've got things working -- currently I'm fixing my PATH to include the required DLL's from boost for the python tests to run.

Contributor guide

Open the contributing guide

Research direction

Start with src/python/PyImath/PyImathUtil.h and src/python/PyImath/PyImathStringTable.cpp, then inspect how PYIMATH_EXPORT is defined in PyImathExport.h for clang-cl on Windows. Build the Python bindings with clang-cl, address the reported export errors, and run the Python tests to verify the bindings still work.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake, cpp, python
Domain
build-system
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.