Various windows fixes #181

fsfod · 2024-01-15T21:12:42Z

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.

github-actions

clang-tidy made some suggestions

github-actions · 2024-01-16T07:44:59Z

lib/Interpreter/CppInterOp.cpp

@@ -1084,7 +1084,7 @@ namespace Cpp {
      auto GD = GlobalDecl(VD);
      std::string mangledName;
      compat::maybeMangleDeclName(GD, mangledName);
-      auto address = dlsym(/*whole_process=*/0, mangledName.c_str());
+      auto address = llvm::sys::DynamicLibrary::SearchForAddressOfSymbol(mangledName.c_str());


warning: 'auto address' can be declared as 'auto *address' [llvm-qualified-auto]

Suggested change

auto address = llvm::sys::DynamicLibrary::SearchForAddressOfSymbol(mangledName.c_str());

auto *address = llvm::sys::DynamicLibrary::SearchForAddressOfSymbol(mangledName.c_str());

lib/Interpreter/Paths.cpp

github-actions

clang-tidy made some suggestions

lib/Interpreter/DynamicLibraryManager.h

lib/Interpreter/Paths.cpp

codecov · 2024-01-16T19:35:40Z

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

@@            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	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`100.00% <ø> (ø)`
lib/Interpreter/Compatibility.h	`93.63% <ø> (ø)`
lib/Interpreter/DynamicLibraryManager.cpp	`75.12% <100.00%> (+0.12%)`	⬆️
lib/Interpreter/DynamicLibraryManager.h	`92.85% <ø> (ø)`
lib/Interpreter/DynamicLibraryManagerSymbol.cpp	`68.81% <ø> (-0.34%)`	⬇️
lib/Interpreter/CppInterOp.cpp	`86.31% <50.00%> (ø)`
lib/Interpreter/Paths.cpp	`35.40% <80.64%> (+0.89%)`	⬆️

Files	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`100.00% <ø> (ø)`
lib/Interpreter/Compatibility.h	`93.63% <ø> (ø)`
lib/Interpreter/DynamicLibraryManager.cpp	`75.12% <100.00%> (+0.12%)`	⬆️
lib/Interpreter/DynamicLibraryManager.h	`92.85% <ø> (ø)`
lib/Interpreter/DynamicLibraryManagerSymbol.cpp	`68.81% <ø> (-0.34%)`	⬇️
lib/Interpreter/CppInterOp.cpp	`86.31% <50.00%> (ø)`
lib/Interpreter/Paths.cpp	`35.40% <80.64%> (+0.89%)`	⬆️

Codebase changes to be made in compiler-research#181 to allow Windows CI to pass

vgvassilev · 2024-01-17T20:58:06Z

Can you rebase this PR?

…brary

mcbarton · 2024-01-17T22:21:17Z

@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 · 2024-01-17T22:22:26Z

sure

github-actions

clang-tidy made some suggestions

lib/Interpreter/Paths.cpp

fsfod · 2024-01-18T15:25:32Z

Its now just failing to find google test libs.

mcbarton · 2024-01-18T15:29:10Z

@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 · 2024-01-18T15:36:37Z

Its now just failing to find google test libs.

Probably here we need to fix the library extensions:

CppInterOp/cmake/modules/GoogleTest.cmake

Lines 3 to 7 in aa3521b

    
           set(_gtest_byproducts 
        
             ${_gtest_byproduct_binary_dir}/lib/libgtest.a 
        
             ${_gtest_byproduct_binary_dir}/lib/libgtest_main.a 
        
             ${_gtest_byproduct_binary_dir}/lib/libgmock.a 
        
             ${_gtest_byproduct_binary_dir}/lib/libgmock_main.a

fsfod · 2024-01-18T17:16:51Z

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 · 2024-01-18T17:37:41Z

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 · 2024-01-18T17:40:47Z

@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 · 2024-01-18T17:54:15Z

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 · 2024-01-18T17:58:52Z

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 · 2024-01-18T18:14:36Z

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 · 2024-01-18T19:14:35Z

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 · 2024-01-18T19:32:18Z

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 · 2024-01-18T19:43:35Z

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

fsfod · 2024-01-18T21:36:35Z

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 · 2024-01-18T21:52:56Z

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

vgvassilev · 2024-01-18T21:54:26Z

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

fsfod · 2024-01-18T22:11:33Z

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

…l in GetVariableOffset

Change clang::SmallVector to llvm::SmallVector and just set the smaller buffer size directly to 256 instead using platform defines

…ded by DynamicLibraryManager

…rary

…g for fs::real_path output

… windows

github-actions

clang-tidy made some suggestions

lib/Interpreter/Paths.cpp

vgvassilev · 2024-01-20T09:12:33Z

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 · 2024-01-20T10:09:51Z

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 · 2024-01-20T10:37:50Z

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...

github-actions bot reviewed Jan 16, 2024

View reviewed changes

vgvassilev reviewed Jan 16, 2024

View reviewed changes

lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved

lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved

github-actions bot reviewed Jan 16, 2024

View reviewed changes

lib/Interpreter/DynamicLibraryManager.h Show resolved Hide resolved

lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved

lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved

mcbarton added a commit to mcbarton/CppInterOp that referenced this pull request Jan 17, 2024

Add Windows CppInterOp build to CI

de0aef9

Codebase changes to be made in compiler-research#181 to allow Windows CI to pass

mcbarton mentioned this pull request Jan 17, 2024

[CI] Update CI to include a Windows build of CppInterOp + other general CI improvements #180

Merged

Fix rvalue error for std::string in DynamicLibraryManager::isSharedLi…

814479a

…brary

fsfod force-pushed the win_fixes branch from 60b9728 to 50a7565 Compare January 17, 2024 20:59

fsfod force-pushed the win_fixes branch from 50a7565 to b59af22 Compare January 18, 2024 01:39

github-actions bot reviewed Jan 18, 2024

View reviewed changes

lib/Interpreter/Paths.cpp Outdated Show resolved Hide resolved

mcbarton mentioned this pull request Jan 18, 2024

Add WASM CI Support #183

Closed

fsfod added 23 commits January 20, 2024 08:49

Fix isa compile error in Compatibility.h

f3ad010

Fix rvalue errors from LookupLibrary returning const std::string

b5f7150

Move include of dlfcn.h to non windows only in CppInterOp.cpp

306708f

Replace dlsym with llvm::sys::DynamicLibrary::SearchForAddressOfSymbo…

466bf4c

…l in GetVariableOffset

Use llvm::sys::fs::real_path in NormalizePath instead of realpath

67248b4

Change clang::SmallVector to llvm::SmallVector and just set the smaller buffer size directly to 256 instead using platform defines

Move dlfcn.h include to be guarded on LLVM_ON_UNIX in Paths.cpp

67d8567

Make GetSystemLibraryPaths a nop on windows since PATH was already ad…

2c72340

…ded by DynamicLibraryManager

Make DyLibHandle non const so we can pass it to llvm API

b0684dd

Add windows implementations of DLOpen\DLClose using llvm's DynamicLib…

652abef

…rary

Don't try link dl on windows for clangCppInterOp

0264765

Implement IsDLL in the same style as identify_magic

4f0ae2d

Only declare Popen in Paths.cpp for LLVM_ON_UNIX

11c6fe4

Fix unit tests failing because NormalizePath was not using SmallStrin…

6875276

…g for fs::real_path output

Define STDOUT_FILENO to 1 for windows if not already defined

b300623

Fix git apply patch command in README.md

15490eb

Only link pthread for unit tests on non windows platforms

745590f

Fix gtest library link paths on windows

e91f9c0

Fix TestSharedLib not exporting ret_zero on windows

abc6f6f

Don't use BUILD_SHARED_LIB for windows

31db0ca

Set an build config to run tests since VS is multi-config generator

59c5f1d

Don't build gtest with different runtime library type/build config on…

c8c6928

… windows

Just build gtest using the current config on all platforms

d8e7b95

Make the DynamicLibraryManagerTests pass

cc2dfc4

vgvassilev force-pushed the win_fixes branch from 0ff4ee5 to cc2dfc4 Compare January 20, 2024 08:49

github-actions bot reviewed Jan 20, 2024

View reviewed changes

vgvassilev merged commit bf29fcd into compiler-research:main Jan 20, 2024
15 of 19 checks passed

fsfod deleted the win_fixes branch February 7, 2024 01:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various windows fixes #181

Various windows fixes #181

fsfod commented Jan 15, 2024

github-actions bot left a comment

github-actions bot Jan 16, 2024

github-actions bot left a comment

codecov bot commented Jan 16, 2024 •

edited

Loading

vgvassilev commented Jan 17, 2024

mcbarton commented Jan 17, 2024

fsfod commented Jan 17, 2024

github-actions bot left a comment

fsfod commented Jan 18, 2024

mcbarton commented Jan 18, 2024

vgvassilev commented Jan 18, 2024

fsfod commented Jan 18, 2024

vgvassilev commented Jan 18, 2024

mcbarton commented Jan 18, 2024

mcbarton commented Jan 18, 2024 •

edited

Loading

vgvassilev commented Jan 18, 2024

fsfod commented Jan 18, 2024

mcbarton commented Jan 18, 2024

mcbarton commented Jan 18, 2024 •

edited

Loading

fsfod commented Jan 18, 2024

fsfod commented Jan 18, 2024

vgvassilev commented Jan 18, 2024

vgvassilev commented Jan 18, 2024

fsfod commented Jan 18, 2024

github-actions bot left a comment

vgvassilev commented Jan 20, 2024

mcbarton commented Jan 20, 2024

vgvassilev commented Jan 20, 2024

	auto address = llvm::sys::DynamicLibrary::SearchForAddressOfSymbol(mangledName.c_str());
	auto *address = llvm::sys::DynamicLibrary::SearchForAddressOfSymbol(mangledName.c_str());

Various windows fixes #181

Various windows fixes #181

Conversation

fsfod commented Jan 15, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jan 16, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 16, 2024 • edited Loading

Codecov Report

vgvassilev commented Jan 17, 2024

mcbarton commented Jan 17, 2024

fsfod commented Jan 17, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

fsfod commented Jan 18, 2024

mcbarton commented Jan 18, 2024

vgvassilev commented Jan 18, 2024

fsfod commented Jan 18, 2024

vgvassilev commented Jan 18, 2024

mcbarton commented Jan 18, 2024

mcbarton commented Jan 18, 2024 • edited Loading

vgvassilev commented Jan 18, 2024

fsfod commented Jan 18, 2024

mcbarton commented Jan 18, 2024

mcbarton commented Jan 18, 2024 • edited Loading

fsfod commented Jan 18, 2024

fsfod commented Jan 18, 2024

vgvassilev commented Jan 18, 2024

vgvassilev commented Jan 18, 2024

fsfod commented Jan 18, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

vgvassilev commented Jan 20, 2024

mcbarton commented Jan 20, 2024

vgvassilev commented Jan 20, 2024

codecov bot commented Jan 16, 2024 •

edited

Loading

mcbarton commented Jan 18, 2024 •

edited

Loading

mcbarton commented Jan 18, 2024 •

edited

Loading