Windows test fixes #196

fsfod · 2024-02-04T16:26:25Z

Not all tests pass yet, but they shouldn't crash early with a fatal assert from Clang any more. The library tests could fail in assert builds before because an Error value for a from a Coff symbol in Dyld::BuildBloomFilter was not handled, those errors were actually revealing another issue that search path to load shared libraries to find symbols should be much more restricted instead of PATH environment variable.
I also added the exporting of some MSVC runtime symbols based on the code from Clings cmake files to try and fix STL symbol errors, but tests still fail because of missing emulated TLS symbols __emutls_get_address and __emutls_v._Init_thread_epoch .

Current failing tests are:

 EnumReflectionTest.GetIntegerTypeFromEnumScope
 EnumReflectionTest.GetIntegerTypeFromEnumType
 FunctionReflectionTest.GetFunctionAddress
 FunctionReflectionTest.Construct
 FunctionReflectionTest.Destruct

…fno-delayed-template-parsing

…ed without export errors

codecov · 2024-02-04T16:30:19Z

Codecov Report

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

Comparison is base (5953058) 78.83% compared to head (7bdf6ea) 78.63%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #196      +/-   ##
==========================================
- Coverage   78.83%   78.63%   -0.21%     
==========================================
  Files           8        8              
  Lines        3048     3056       +8     
==========================================
  Hits         2403     2403              
- Misses        645      653       +8

Files	Coverage Δ
lib/Interpreter/CppInterOp.cpp	`86.31% <ø> (ø)`
lib/Interpreter/DynamicLibraryManagerSymbol.cpp	`68.47% <0.00%> (-0.92%)`	⬇️

Files	Coverage Δ
lib/Interpreter/CppInterOp.cpp	`86.31% <ø> (ø)`
lib/Interpreter/DynamicLibraryManagerSymbol.cpp	`68.47% <0.00%> (-0.92%)`	⬇️

github-actions · 2024-02-04T16:32:06Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-02-04T16:52:24Z

lib/Interpreter/CppInterOp.cpp

    ClingArgv.insert(ClingArgv.begin(), MainExecutableName.c_str());
+#ifdef _WIN32
+    // FIXME : Workaround Sema::PushDeclContext assert on windows
+    ClingArgv.push_back("-fno-delayed-template-parsing");


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

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.

Do you think we need a patch in upstream clang?

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

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

vgvassilev

LGTM! @mcbarton, after this PR we might be able to remove from the CI the make-it-all-green for windows and disable only the failing tests.

vgvassilev · 2024-02-05T07:16:55Z

@fsfod, forgot to say - can you apply clang-format on per-commit basis or shall I?

…ectory iterator in Dyld::BuildBloomFilter

github-actions · 2024-02-05T14:57:00Z

clang-tidy review says "All clean, LGTM! 👍"

mcbarton · 2024-02-05T15:05:08Z

LGTM! @mcbarton, after this PR we might be able to remove from the CI the make-it-all-green for windows and disable only the failing tests.

@vgvassilev This would not be possible if it was merged right now, as when CppInterOp uses Cling on Windows it currently aborts in the following tests in the CI

InterpreterTest.Process
DynamicLibraryManagerTest.Sanity

cc: @fsfod Any idea what is causing these tests to fail (and subsequently abort the test runs) in the Cling version , but not the Clang-Repl version?

fsfod · 2024-02-05T15:14:31Z

There are two tests that fail that are not export related:

[ RUN      ] EnumReflectionTest.GetIntegerTypeFromEnumScope
EnumReflectionTest.cpp(127): error: Expected equality of these values:
  Cpp::GetTypeAsString(Cpp::GetIntegerTypeFromEnumScope(Decls[4]))
    Which is: "int"
  "unsigned int"
[ RUN      ] EnumReflectionTest.GetIntegerTypeFromEnumType
EnumReflectionTest.cpp(183): error: Expected equality of these values:
  get_int_type_from_enum_var(Decls[10])
    Which is: "int"
  "unsigned int"

MSVC defaults to int as the base type for enums ;

vgvassilev · 2024-02-05T15:15:50Z

There are two tests that fail that are not export related:

[ RUN      ] EnumReflectionTest.GetIntegerTypeFromEnumScope
EnumReflectionTest.cpp(127): error: Expected equality of these values:
  Cpp::GetTypeAsString(Cpp::GetIntegerTypeFromEnumScope(Decls[4]))
    Which is: "int"
  "unsigned int"
[ RUN      ] EnumReflectionTest.GetIntegerTypeFromEnumType
EnumReflectionTest.cpp(183): error: Expected equality of these values:
  get_int_type_from_enum_var(Decls[10])
    Which is: "int"
  "unsigned int"

MSVC defaults to int as the base type for enums ;

Ok, so the tests need to be adjusted for windows...

github-actions · 2024-02-05T15:35:24Z

clang-tidy review says "All clean, LGTM! 👍"

fsfod · 2024-02-05T16:46:40Z

Any idea what is causing these tests to fail (and subsequently abort the test runs) in the Cling version , but not the Clang-Repl version?

Maybe memory was double free'ed in cling::Value or it was uninitialized data being free'ed?

vgvassilev · 2024-02-05T21:22:57Z

Let’s move forward here and address the remaining issues in a subsequent PR.

fsfod added 2 commits February 3, 2024 16:23

Work around asserts in Sema::PushDeclContext for Windows by setting -…

f79cc29

…fno-delayed-template-parsing

Export MSVC library symbols from clangCppInterOp so STL can be compil…

c42fab9

…ed without export errors

vgvassilev reviewed Feb 4, 2024

View reviewed changes

vgvassilev approved these changes Feb 5, 2024

View reviewed changes

Fix assert crash from the error from getSymbolName CoffObj export_dir…

8d527a5

…ectory iterator in Dyld::BuildBloomFilter

fsfod force-pushed the win_testfixes branch from 55a600e to 8d527a5 Compare February 5, 2024 14:50

Fix some enum tests for windows where the default enum base type is int

7bdf6ea

vgvassilev merged commit eb2f4ef into compiler-research:main Feb 5, 2024

fsfod deleted the win_testfixes branch February 7, 2024 01:34

vgvassilev mentioned this pull request Mar 19, 2024

Add more tests to make sure the package does not regress. conda-forge/cppinterop-feedstock#7

Merged

5 tasks

Windows test fixes #196

Windows test fixes #196

Uh oh!

Conversation

fsfod commented Feb 4, 2024

Uh oh!

codecov bot commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Feb 4, 2024

Uh oh!

vgvassilev Feb 4, 2024

Choose a reason for hiding this comment

Uh oh!

fsfod Feb 4, 2024

Choose a reason for hiding this comment

Uh oh!

vgvassilev Feb 4, 2024

Choose a reason for hiding this comment

Uh oh!

fsfod Feb 4, 2024

Choose a reason for hiding this comment

Uh oh!

vgvassilev Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

vgvassilev commented Feb 5, 2024

Uh oh!

github-actions bot commented Feb 5, 2024

Uh oh!

mcbarton commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fsfod commented Feb 5, 2024

Uh oh!

vgvassilev commented Feb 5, 2024

Uh oh!

github-actions bot commented Feb 5, 2024

Uh oh!

fsfod commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vgvassilev commented Feb 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 4, 2024 •

edited

Loading

mcbarton commented Feb 5, 2024 •

edited

Loading

fsfod commented Feb 5, 2024 •

edited

Loading