compiler-research / CppInterOp

A Clang-based C++ Interoperability Library
Other
38 stars 20 forks source link

Windows test fixes #196

Closed fsfod closed 5 months ago

fsfod commented 5 months ago

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
codecov[bot] commented 5 months ago

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 [![Impacted file tree graph](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/196/graphs/tree.svg?width=650&height=150&src=pr&token=7UWTYSVVT5&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research)](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/196?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 #196 +/- ## ========================================== - Coverage 78.83% 78.63% -0.21% ========================================== Files 8 8 Lines 3048 3056 +8 ========================================== Hits 2403 2403 - Misses 645 653 +8 ``` | [Files](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/196?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) | Coverage Δ | | |---|---|---| | [lib/Interpreter/CppInterOp.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/196?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% <ø> (ø)` | | | [lib/Interpreter/DynamicLibraryManagerSymbol.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/196?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0R5bmFtaWNMaWJyYXJ5TWFuYWdlclN5bWJvbC5jcHA=) | `68.47% <0.00%> (-0.92%)` | :arrow_down: | | [Files](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/196?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research) | Coverage Δ | | |---|---|---| | [lib/Interpreter/CppInterOp.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/196?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% <ø> (ø)` | | | [lib/Interpreter/DynamicLibraryManagerSymbol.cpp](https://app.codecov.io/gh/compiler-research/CppInterOp/pull/196?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=compiler-research#diff-bGliL0ludGVycHJldGVyL0R5bmFtaWNMaWJyYXJ5TWFuYWdlclN5bWJvbC5jcHA=) | `68.47% <0.00%> (-0.92%)` | :arrow_down: |
github-actions[bot] commented 5 months ago

clang-tidy review says "All clean, LGTM! :+1:"

vgvassilev commented 5 months ago

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

github-actions[bot] commented 5 months ago

clang-tidy review says "All clean, LGTM! :+1:"

mcbarton commented 5 months ago

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

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 commented 5 months ago

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 commented 5 months ago

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[bot] commented 5 months ago

clang-tidy review says "All clean, LGTM! :+1:"

fsfod commented 5 months ago

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 commented 5 months ago

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