Skip to content

Windows test fixes #196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vgvassilev merged 4 commits into from
Feb 5, 2024
Merged

Windows test fixes #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f79cc29
Work around asserts in Sema::PushDeclContext for Windows by setting -…
fsfod Feb 3, 2024
c42fab9
Export MSVC library symbols from clangCppInterOp so STL can be compil…
fsfod Feb 3, 2024
8d527a5
Fix assert crash from the error from getSymbolName CoffObj export_dir…
fsfod Feb 5, 2024
7bdf6ea
Fix some enum tests for windows where the default enum base type is int
fsfod Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,55 @@ option(CPPINTEROP_ENABLE_DOXYGEN "Use doxygen to generate CppInterOp interal API
option(CPPINTEROP_ENABLE_SPHINX "Use sphinx to generage CppInterOp user documentation")
option(CPPINTEROP_ENABLE_TESTING "Enables the testing infrastructure." ON)

if(MSVC)

set(MSVC_EXPORTLIST
??_7type_info@@6B@
?nothrow@std@@3Unothrow_t@1@B
_Init_thread_header
_Init_thread_footer
??_7type_info@@6B@
?_Facet_Register@std@@YAXPEAV_Facet_base@1@@Z
)

set(MSVC_EXPORTLIST ${MSVC_EXPORTLIST}
??2@YAPEAX_K@Z
??3@YAXPEAX@Z
??3@YAXPEAX_K@Z
??_U@YAPEAX_K@Z
??_V@YAXPEAX@Z
??_V@YAXPEAX_K@Z
??2@YAPEAX_KAEBUnothrow_t@std@@@Z
??_U@YAPEAX_KAEBUnothrow_t@std@@@Z
??6?$basic_ostream@DU?$char_traits@D@std@@@std@@QEAAAEAV01@H@Z
??6?$basic_ostream@DU?$char_traits@D@std@@@std@@QEAAAEAV01@M@Z
??6?$basic_ostream@DU?$char_traits@D@std@@@std@@QEAAAEAV01@N@Z
??6?$basic_ostream@DU?$char_traits@D@std@@@std@@QEAAAEAV01@PEBX@Z
??6?$basic_ostream@DU?$char_traits@D@std@@@std@@QEAAAEAV01@P6AAEAV01@AEAV01@@Z@Z
??$?6U?$char_traits@D@std@@@std@@YAAEAV?$basic_ostream@DU?$char_traits@D@std@@@0@AEAV10@D@Z
??$?6U?$char_traits@D@std@@@std@@YAAEAV?$basic_ostream@DU?$char_traits@D@std@@@0@AEAV10@PEBD@Z
?_Facet_Register@std@@YAXPEAV_Facet_base@1@@Z
)

if(MSVC_VERSION LESS 1914)
set(MSVC_EXPORTLIST ${MSVC_EXPORTLIST} ??3@YAXPAX0@Z ??_V@YAXPAX0@Z)
endif()

if(MSVC_VERSION GREATER_EQUAL 1936)
set(MSVC_EXPORTLIST ${MSVC_EXPORTLIST}
__std_find_trivial_1
__std_find_trivial_2
__std_find_trivial_4
__std_find_trivial_8
)
endif()

foreach(sym ${MSVC_EXPORTLIST})
set(MSVC_EXPORTS "${MSVC_EXPORTS} /EXPORT:${sym}")
endforeach(sym ${MSVC_EXPORTLIST})

endif()

if (CPPINTEROP_INCLUDE_DOCS)
add_subdirectory(docs)
endif()
Expand Down
4 changes: 4 additions & 0 deletions lib/Interpreter/CppInterOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2463,6 +2463,10 @@ namespace Cpp {
std::vector<const char *> ClingArgv = {"-resource-dir", ResourceDir.c_str(),
"-std=c++14"};
ClingArgv.insert(ClingArgv.begin(), MainExecutableName.c_str());
#ifdef _WIN32
// FIXME : Workaround Sema::PushDeclContext assert on windows
ClingArgv.push_back("-fno-delayed-template-parsing");
Copy link
Copy Markdown
Contributor

@vgvassilev vgvassilev Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need -femulated-tls for the TLS related issues? We do this here in cling:

https://github.com/root-project/root/blob/fda4630190de40d817b850efa240c4ed72a20b8e/interpreter/cling/lib/Interpreter/IncrementalJIT.cpp#L388-L390

Copy link
Copy Markdown
Contributor Author

@fsfod fsfod Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried -fno-emulated-tls there and still got the same emulated TLS symbol errors. It looks like its defaulted to on from JITTargetMachineBuilder's constructor.

Copy link
Copy Markdown
Contributor

@vgvassilev vgvassilev Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we need a patch in upstream clang?

Copy link
Copy Markdown
Contributor Author

@fsfod fsfod Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't really know how it should work for windows.

Copy link
Copy Markdown
Contributor

@vgvassilev vgvassilev Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, understood. Let's move forward with this PR and fix the remaining tests later.

#endif
ClingArgv.insert(ClingArgv.end(), Args.begin(), Args.end());
// To keep the Interpreter creation interface between cling and clang-repl
// to some extent compatible we should put Args and GpuArgs together. On the
Expand Down
11 changes: 10 additions & 1 deletion lib/Interpreter/DynamicLibraryManagerSymbol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,8 +911,17 @@ namespace Cpp {
// All the symbols are already flagged as exported.
// We cannot really ignore symbols based on flags as we do on unix.
StringRef Name;
if (I->getSymbolName(Name))
auto Err = I->getSymbolName(Name);

if (Err) {
std::string Message;
handleAllErrors(std::move(Err), [&](llvm::ErrorInfoBase& EIB) {
Message += EIB.message() + "; ";
});
LLVM_DEBUG(dbgs() << "Dyld::BuildBloomFilter: Failed to read symbol "
<< Message << "\n");
continue;
}
if (Name.empty())
continue;

Expand Down
1 change: 1 addition & 0 deletions unittests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ function(add_cppinterop_unittest name)

if(WIN32)
target_link_libraries(${name} PUBLIC ${ARG_LIBRARIES} ${gtest_libs})
set_property(TARGET ${name} APPEND_STRING PROPERTY LINK_FLAGS "${MSVC_EXPORTS}")
else()
target_link_libraries(${name} PUBLIC ${ARG_LIBRARIES} ${gtest_libs} pthread)
endif()
Expand Down
9 changes: 9 additions & 0 deletions unittests/CppInterOp/EnumReflectionTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,12 @@ TEST(EnumReflectionTest, GetIntegerTypeFromEnumScope) {
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetIntegerTypeFromEnumScope(Decls[1])), "char");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetIntegerTypeFromEnumScope(Decls[2])), "int");
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetIntegerTypeFromEnumScope(Decls[3])), "long long");
#ifdef _WIN32
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetIntegerTypeFromEnumScope(Decls[4])),
"int");
#else
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetIntegerTypeFromEnumScope(Decls[4])), "unsigned int");
#endif
EXPECT_EQ(Cpp::GetTypeAsString(Cpp::GetIntegerTypeFromEnumScope(Decls[5])),"NULL TYPE");
}

Expand Down Expand Up @@ -180,7 +185,11 @@ TEST(EnumReflectionTest, GetIntegerTypeFromEnumType) {
EXPECT_EQ(get_int_type_from_enum_var(Decls[7]), "char");
EXPECT_EQ(get_int_type_from_enum_var(Decls[8]), "int");
EXPECT_EQ(get_int_type_from_enum_var(Decls[9]), "long long");
#ifdef _WIN32
EXPECT_EQ(get_int_type_from_enum_var(Decls[10]), "int");
#else
EXPECT_EQ(get_int_type_from_enum_var(Decls[10]), "unsigned int");
#endif
EXPECT_EQ(get_int_type_from_enum_var(Decls[11]), "NULL TYPE"); // When a non Enum Type variable is used
}

Expand Down