Skip to content
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.

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

[BUG] tests fail for Clang CL #241

Closed
mgovers opened this issue Apr 17, 2023 · 0 comments · Fixed by #276
Closed

[BUG] tests fail for Clang CL #241

mgovers opened this issue Apr 17, 2023 · 0 comments · Fixed by #276
Assignees
Labels
bug Something isn't working good first issue Indicates a good issue for first-time contributors

Comments

@mgovers
Copy link
Member

mgovers commented Apr 17, 2023

Repro

cmake -DCMAKE_C_COMPILER="clang-cl.exe" -DCMAKE_CXX_COMPILER=clang-cl.exe "-DCMAKE_CXX_FLAGS=/DWIN32 /D_WINDOWS /W3 /WX /GR /EHsc -Wno-uninitialized-const-reference" -DCMAKE_BUILD_TYPE=RelWithDebInfo -S"." -B"cpp_build/clang-cl-release" -G Ninja  
cmake --build .\cpp_build\clang-cl-release\
.\cpp_build\clang-cl-debug\bin\power_grid_model_unit_tests.exe

Initial investigation

Likely related to this chunk of code not working:

template <class T>
struct trait_pointer_to_member;
template <class StructType, class ValueType>
struct trait_pointer_to_member<ValueType StructType::*> {
    using struct_type = StructType;
    using value_type = ValueType;
};

In get_data_attribute, I logged the related types for every field:

template <typename StructType, auto member_ptr>
inline DataAttribute get_data_attribute(std::string const& name) {
    using value_type = typename trait_pointer_to_member<StructType, decltype(member_ptr)>::value_type;
    using single_data_type = data_type<value_type>;
    std::cout << name << ": "
              << typeid(member_ptr).name() << " - "
              << typeid(value_type).name() << " - "
              << typeid(single_data_type).name() << " - "
              << single_data_type::numpy_type << std::endl;

    DataAttribute attr{};
    // [...]
    return attr;

and I saw that the types do not match at all with the fields, e.g.:

id: int power_grid_model::BaseUpdate::* __ptr64 - int - struct power_grid_model::meta_data::data_type<int,0> - i4
u_rated: double power_grid_model::VoltageSensorUpdate<0>::* __ptr64 - double - struct power_grid_model::meta_data::data_type<double,0> - f8

VoltageSensorUpdate does not contain any u_rated. The wrong pointer is matched with the wrong type, and as a result, the entire memory layout is wrong.

In addition, it is probably a good idea to add a static_assert<std::is_standard_layout_v<T>> for every input, update and output struct

Other notes

FYI: @nitbharambe found multiple bugs for the Clang CL presets (SEGFAULTs, type mismatch, ...). The compiler compiles the code, but the tests fail.

After investigating:

  • I am able to repro on main, which means that it's not a problem on my branch
  • we do need to disable the Clang CL presets for the time being.

Apparently, I did not find the issue before because I just so happened to not run the tests for these specific presets (I did do a matrix test with (gcc, clang, msvc) x (release, debug), but apparently not for clang-cl)

c.c. @TonyXiang8787 @petersalemink95

Originally posted by @mgovers in #239 (comment)

@mgovers mgovers changed the title FYI: @nitbharambe found multiple bugs for the Clang CL presets (SEGFAULTs, type mismatch, ...). The compiler compiles the code, but the tests fail. tests fail for Clang CL Apr 17, 2023
@mgovers mgovers added the bug Something isn't working label Apr 17, 2023
@mgovers mgovers changed the title tests fail for Clang CL [BUG] tests fail for Clang CL Jun 2, 2023
@mgovers mgovers added the good first issue Indicates a good issue for first-time contributors label Jun 2, 2023
@mgovers mgovers linked a pull request Jun 20, 2023 that will close this issue
@mgovers mgovers self-assigned this Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Indicates a good issue for first-time contributors
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants