Various windows fixes

[fsfod]

Fix some of the windows build errors, It still won't fully build with just these changes, but it gets it much closer. I don't know much of the llvm internal API, but i did switched from some Linux specific function that were failing to build on windows with llvm API like SearchForAddressOfSymbol and real_path.

[codecov[bot]]

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (aa3521b) 78.68% compared to head (cc2dfc4) 78.67%.

Additional details and impacted files

[](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) ```diff @@ Coverage Diff @@ ## main #181 +/- ## ========================================== - Coverage 78.68% 78.67% -0.02% ========================================== Files 8 8 Lines 3050 3053 +3 ========================================== + Hits 2400 2402 +2 - Misses 650 651 +1 ``` | [Files](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) | Coverage Δ | | |---|---|---| | [include/clang/Interpreter/CppInterOp.h](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-aW5jbHVkZS9jbGFuZy9JbnRlcnByZXRlci9DcHBJbnRlck9wLmg=) | `100.00% <ø> (ø)` | | | [lib/Interpreter/Compatibility.h](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0NvbXBhdGliaWxpdHkuaA==) | `93.63% <ø> (ø)` | | | [lib/Interpreter/DynamicLibraryManager.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0R5bmFtaWNMaWJyYXJ5TWFuYWdlci5jcHA=) | `75.12% <100.00%> (+0.12%)` | :arrow_up: | | [lib/Interpreter/DynamicLibraryManager.h](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0R5bmFtaWNMaWJyYXJ5TWFuYWdlci5o) | `92.85% <ø> (ø)` | | | [lib/Interpreter/DynamicLibraryManagerSymbol.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0R5bmFtaWNMaWJyYXJ5TWFuYWdlclN5bWJvbC5jcHA=) | `68.81% <ø> (-0.34%)` | :arrow_down: | | [lib/Interpreter/CppInterOp.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0NwcEludGVyT3AuY3Bw) | `86.31% <50.00%> (ø)` | | | [lib/Interpreter/Paths.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL1BhdGhzLmNwcA==) | `35.40% <80.64%> (+0.89%)` | :arrow_up: | | [Files](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) | Coverage Δ | | |---|---|---| | [include/clang/Interpreter/CppInterOp.h](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-aW5jbHVkZS9jbGFuZy9JbnRlcnByZXRlci9DcHBJbnRlck9wLmg=) | `100.00% <ø> (ø)` | | | [lib/Interpreter/Compatibility.h](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0NvbXBhdGliaWxpdHkuaA==) | `93.63% <ø> (ø)` | | | [lib/Interpreter/DynamicLibraryManager.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0R5bmFtaWNMaWJyYXJ5TWFuYWdlci5jcHA=) | `75.12% <100.00%> (+0.12%)` | :arrow_up: | | [lib/Interpreter/DynamicLibraryManager.h](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0R5bmFtaWNMaWJyYXJ5TWFuYWdlci5o) | `92.85% <ø> (ø)` | | | [lib/Interpreter/DynamicLibraryManagerSymbol.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0R5bmFtaWNMaWJyYXJ5TWFuYWdlclN5bWJvbC5jcHA=) | `68.81% <ø> (-0.34%)` | :arrow_down: | | [lib/Interpreter/CppInterOp.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0NwcEludGVyT3AuY3Bw) | `86.31% <50.00%> (ø)` | | | [lib/Interpreter/Paths.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL1BhdGhzLmNwcA==) | `35.40% <80.64%> (+0.89%)` | :arrow_up: |

[vgvassilev]

Can you rebase this PR?

[mcbarton]

@fsfod In one of your future commits can you fix a mistake I made in the readme build instructions in my last PR. The following line https://github.com/fsfod/CppInterOp/blob/50a7565a9c10deea39735ff02a2e0912e53ce874/README.md?plain=1#L82 should say

git apply -v ../patches/llvm/clang17-*.patch

In my haste I copied from the ci and forgot to change it.

[fsfod]

sure

[fsfod]

Its now just failing to find google test libs.

[mcbarton]

@fsfod I also see this error in the CI. LINK : fatal error LNK1104: cannot open file 'pthread.lib' . pthreads seem to be unix only, so this also needs to be solved.

[vgvassilev]

Its now just failing to find google test libs.

Probably here we need to fix the library extensions: https://github.com/compiler-research/CppInterOp/blob/aa3521b00af1aab1ae36df60865a998ceb9d1dd7/cmake/modules/GoogleTest.cmake#L3-L7

[fsfod]

Where does the lib path come from for linking thoses libs because it doesn't match that list. Locally it fails with fatal error LNK1104: cannot open file '..\googletest-prefix\src\googletest-build\lib\\gtest.lib'

[vgvassilev]

Where does the lib path come from for linking thoses libs because it doesn't match that list. Locally it fails with fatal error LNK1104: cannot open file '..\googletest-prefix\src\googletest-build\lib\\gtest.lib'

It is probably in \Debug|Release\lib?

[mcbarton]

@fsfod I also see this error in the CI. LINK : fatal error LNK1104: cannot open file 'pthread.lib' . pthreads seem to be unix only, so this also needs to be solved.

The pthread library issue appears to be here https://github.com/fsfod/CppInterOp/blob/b59af225385cda1bf1936507eea8be07e2d71240/unittests/CMakeLists.txt#L27C1-L27C79

[mcbarton]

Where does the lib path come from for linking thoses libs because it doesn't match that list. Locally it fails with fatal error LNK1104: cannot open file '..\googletest-prefix\src\googletest-build\lib\\gtest.lib'

I believe the gtest library location is being set here on Windows. We are using the MSVC compiler in the CI, so I believe this is where the library path is being set. I'm not 100% certain though.

https://github.com/fsfod/CppInterOp/blob/b59af225385cda1bf1936507eea8be07e2d71240/cmake/modules/GoogleTest.cmake#L10C1-L17C74

[vgvassilev]

Where does the lib path come from for linking thoses libs because it doesn't match that list. Locally it fails with fatal error LNK1104: cannot open file '..\googletest-prefix\src\googletest-build\lib\\gtest.lib'

It is probably in \Debug|Release\lib?

Maybe something like: ${_gtest_byproduct_binary_dir}/${CMAKE_CFG_INTDIR}/lib/

[fsfod]

Where does the lib path come from for linking thoses libs because it doesn't match that list. Locally it fails with fatal error LNK1104: cannot open file '..\googletest-prefix\src\googletest-build\lib\\gtest.lib'

I believe the gtest library location is being set here on Windows. We are using the MSVC compiler in the CI, so I believe this is where the library path is being set. I'm not 100% certain though.

fsfod/CppInterOp@b59af22/cmake/modules/GoogleTest.cmake#L10C1-L17C74

All those paths for point to CppInterOp/build/Clang-Release/downloads/googletest-prefix/src/googletest-build/lib/ where the libs are for me.

[mcbarton]

The cling build of CppInterOp is now getting a large number of 'unresolved external symbol' errors after the last commit. Its not clear to me what may be causing it.

[mcbarton]

All the gtest import paths are being set here. https://github.com/fsfod/CppInterOp/blob/b59af225385cda1bf1936507eea8be07e2d71240/cmake/modules/GoogleTest.cmake#L81C1-L81C1

Maybe try changing in all the 4 paths ${_G_LIBRARY_PATH} to ${_gtest_byproduct_binary_dir}

This suggestion is working under the assumption that ${CMAKE_STATIC_LIBRARY_PREFIX} = lib/

I have to admit this is all kind of guess work on my part.

[fsfod]

I just figured that out, also ${CMAKE_STATIC_LIBRARY_PREFIX} is empty for windows

[fsfod]

I don't get any of those linker errors locally that there failing with now. Are the tests relying on symbol visibility for shared libraries being public? because for windows it private default and class or functions need to be explicitly annotated __declspec(dllexport) or the linker has to be passed file of mangled symbol names to be exported.

[vgvassilev]

Yes. LLVM has export_executable_symbols() but I think that’s mostly for executables.

[vgvassilev]

How is the symbol export solved for the llvm libraries on windows?

[fsfod]

I'm not really sure its fully solved for shared library builds https://discourse.llvm.org/t/supporting-llvm-build-llvm-dylib-on-windows/58891

[fsfod]

Maybe the CI llvm build could be rebuilt with -DBUILD_SHARED_LIBS=OFF for windows. Some other alternatives are:

• Decorate the functions declared in CppInterOp.h/CppInterOp.cpp with macros that are turned in to decl(dllexport) and decl(dllimport) for windows, but this doesn't solve import errors for clang::Value.
• Use libclang or even add a new public API in CppInterOp to get the mangled names of functions thats used in a script to gather functions declared in CppInterOp.h to create a linker def file that contains the mangled export list.

[vgvassilev]

Maybe the CI llvm build could be rebuilt with -DBUILD_SHARED_LIBS=OFF for windows.

That makes sense. CppInterOp should encapsulate all symbols and not dynamically link anything llvm/clang-specific. I've cleaned the cache.

Unrelated, could you @mcbarton and @fsfod reach out to me by email off-list?

[fsfod]

Some of the other link errors look like google test built with the msvc debug runtime instead of release, but they may not happen for non shared library builds.

[mcbarton]

If the ci build passes now it will fix more than just Windows builds. I have done some checks locally and this PR fixes https://github.com/compiler-research/CppInterOp/issues/175 . I am currently checking to see whether this error still occurs or not https://github.com/compiler-research/xeus-clang-repl/issues/78 . If it fixes this issue too, then a ci should be able to pass the same level on arm as it does on x86, for osx and Ubuntu . I shall know shortly.

[mcbarton]

Unfortunately this PR didn't manage to fix https://github.com/compiler-research/xeus-clang-repl/issues/78 too, but this PR does still fix https://github.com/compiler-research/CppInterOp/issues/175 in its current state, which is an added bonus.

[vgvassilev]

We might need some inspiration from maybe here: https://github.com/root-project/root/blob/7a853eb2bdee6d9d247adfded6712d7202f0adbe/cmake/modules/SearchInstalledSoftware.cmake#L1930-L1941

[vgvassilev]

We can probably disable the failing tests for now and consider this PR success...

[fsfod]

The DynamicLibraryManager test mostly just needed different binary output paths in cmake to fix it.

[mcbarton]

I also agree that I think its probably best to disable the failing tests on Windows, renaming this PR as fix Windows build issues, and raise an issue about the failing tests. If you think you can solve the test issues on your local machine, put in a PR which fixes that issue, once they are all passing locally.

[fsfod]

I think all of the test executables are crashing early with an assert in llvm code so the tests executables can't be run by windows builds in the CI if you want it to pass atm.

[vgvassilev]

Where do you see this assert? Tomorrow I will run clang-format on each commit individually and merge the pr. We can continue addressing the rest of the problems separately.

[fsfod]

https://github.com/compiler-research/CppInterOp/actions/runs/7588907781/job/20672453834?pr=181#step:21:248 Assertion failed: DC->getLexicalParent() == CurContext && "The next DeclContext should be lexically contained in the current one.", file D:\a\CppInterOp\CppInterOp\llvm-project\clang\lib\Sema\SemaDecl.cpp, line 1347

[vgvassilev]

We might need something like https://github.com/llvm/llvm-project/blob/cd05ade13a66d4fb2daff489b2c02ac1ae2070d1/clang/unittests/Interpreter/InterpreterTest.cpp#L282

[fsfod]

That does seem to be implicitly set for me locally, i guess that could be why lots of the tests pass for me locally.

[fsfod]

I wonder target triple the tests default to in the windows CI build.

[vgvassilev]

I wonder target triple the tests default to in the windows CI build.

I don't know but we could add a -v to the interpreter arguments and will be probably print it out.

[mcbarton]

Probably not the best place to put this message, but sometime later today I will put in a PR to fix a few typos and mistakes in the README.md .

[vgvassilev]

I wonder target triple the tests default to in the windows CI build.

I don't know but we could add a -v to the interpreter arguments and will be probably print it out.

Yes, let's merge this PR because it started to hit the cache limits in github and will be slow to make progress since it will start evicting caches for the builds...