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

[libc++][test] std/containers/views/mdspan/mdspan/conversion.pass.cpp Fails on PowerPC targets #64427

Closed
amy-kwan opened this issue Aug 4, 2023 · 10 comments

Comments

@amy-kwan
Copy link
Contributor

amy-kwan commented Aug 4, 2023

This test was introduced in https://reviews.llvm.org/D154367.

Assertion seen:

Command Output (stdout):
--
$ ":" "COMPILED WITH"
$ "/home/amyk/llvm/community/build/bootstrap/stage1/build/./bin/clang++" "/home/amyk/llvm/community/llvm-project/libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp" "-pthread" "--target=powerpc64le-unknown-linux-gnu" "-nostdinc++" "-I" "/home/amyk/llvm/community/build/bootstrap/stage1/build/include/c++/v1" "-I" "/home/amyk/llvm/community/build/bootstrap/stage1/build/include/powerpc64le-unknown-linux-gnu/c++/v1" "-I" "/home/amyk/llvm/community/llvm-project/libcxx/test/support" "-std=c++26" "-Werror" "-Wall" "-Wctad-maybe-unsupported" "-Wextra" "-Wshadow" "-Wundef" "-Wunused-template" "-Wno-unused-command-line-argument" "-Wno-attributes" "-Wno-pessimizing-move" "-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wno-reserved-module-identifier" "-Wno-user-defined-literals" "-Wno-tautological-compare" "-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" "-Wno-unused-local-typedef" "-Wno-local-type-template-args" "-Wno-c++11-extensions" "-Wno-unknown-pragmas" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-D_LIBCPP_ENABLE_EXPERIMENTAL" "-Werror=thread-safety" "-Wuser-defined-warnings" "-lc++experimental" "-nostdlib++" "-L" "/home/amyk/llvm/community/build/bootstrap/stage1/build/./lib/powerpc64le-unknown-linux-gnu" "-Wl,-rpath,/home/amyk/llvm/community/build/bootstrap/stage1/build/./lib/powerpc64le-unknown-linux-gnu" "-lc++" "-o" "/home/amyk/llvm/community/build/bootstrap/stage1/build/runtimes/runtimes-bins/test/std/containers/views/mdspan/mdspan/Output/conversion.pass.cpp.dir/t.tmp.exe"
$ ":" "EXECUTED AS"
$ "/usr/bin/python3.8" "/home/amyk/llvm/community/llvm-project/libcxx/test/../utils/run.py" "--execdir" "/home/amyk/llvm/community/build/bootstrap/stage1/build/runtimes/runtimes-bins/test/std/containers/views/mdspan/mdspan/Output/conversion.pass.cpp.dir" "--" "/home/amyk/llvm/community/build/bootstrap/stage1/build/runtimes/runtimes-bins/test/std/containers/views/mdspan/mdspan/Output/conversion.pass.cpp.dir/t.tmp.exe"
# command stderr:
t.tmp.exe: /home/amyk/llvm/community/llvm-project/libcxx/test/std/containers/views/mdspan/mdspan/conversion.pass.cpp:87: void test_conversion(FromMDS) [ToMDS = std::mdspan<const int, std::extents<int>, std::layout_left, conv_test_accessor_c<int, false, true, true, true>>, FromMDS = std::mdspan<int, std::extents<int>, std::layout_left, conv_test_accessor_nc<int, true, true>>]: Assertion `to_mds.data_handle() == from_mds.data_handle()' failed.

error: command failed with exit status: 250

This issue occurs on both LLVM 17.0.0 rc1 and main.

@amy-kwan amy-kwan added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 4, 2023
@amy-kwan amy-kwan added this to the LLVM 17.0.X Release milestone Aug 4, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 4, 2023

@llvm/issue-subscribers-backend-powerpc

@amy-kwan
Copy link
Contributor Author

amy-kwan commented Aug 4, 2023

@crtrott Just wanted to let you know that I have discovered this when testing libc++.

@crtrott
Copy link
Contributor

crtrott commented Aug 4, 2023

This looks suspiciously simmilar to the MinGW issue reported by me #64077. I will have a look.

@crtrott
Copy link
Contributor

crtrott commented Aug 4, 2023

We should try the godbolt example seen in #64077 on PowerPC and check whether it has the same issue there.

@crtrott
Copy link
Contributor

crtrott commented Aug 4, 2023

I confirmed that the godbolt example https://godbolt.org/z/KK8aj5bs7 (minus the concept and span include which isn't needed) fails on PowerPC with Clang 14 too. No extra options needed just clang++ main.cpp -std=c++20; ./a.out will do it.

@mstorsjo
Copy link
Member

mstorsjo commented Aug 7, 2023

https://reviews.llvm.org/D157332 has got a potential fix for this issue.

@EugeneZelenko EugeneZelenko added clang:codegen and removed libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. backend:PowerPC labels Aug 7, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 7, 2023

@llvm/issue-subscribers-clang-codegen

@amy-kwan
Copy link
Contributor Author

I can confirm that https://reviews.llvm.org/D157332 resolves the issue.

mstorsjo added a commit that referenced this issue Aug 15, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes #64253, and
possibly #64077
and #64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332
llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 15, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm/llvm-project#64253, and
possibly llvm/llvm-project#64077
and llvm/llvm-project#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332

(cherry picked from commit d60c3d0)
tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 16, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm/llvm-project#64253, and
possibly llvm/llvm-project#64077
and llvm/llvm-project#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332

(cherry picked from commit d60c3d0)
@tru
Copy link
Collaborator

tru commented Aug 21, 2023

Can this be closed now? I see linked commits to this issue.

@mstorsjo
Copy link
Member

Can this be closed now? I see linked commits to this issue.

Yes, some version of the fix was confirmed as fixing this issue, and as the final forms of the fix, in 4d502f1, did essentially the same thing, I think we can close it, and reopen if it (unlikely) turns out to not be fixed after all.

@EugeneZelenko EugeneZelenko removed this from the LLVM 17.0.X Release milestone Aug 21, 2023
razmser pushed a commit to SuduIDE/llvm-project that referenced this issue Oct 2, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm#64253, and
possibly llvm#64077
and llvm#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332
razmser pushed a commit to SuduIDE/llvm-project that referenced this issue Oct 2, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm#64253, and
possibly llvm#64077
and llvm#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332
razmser pushed a commit to SuduIDE/llvm-project that referenced this issue Oct 2, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm#64253, and
possibly llvm#64077
and llvm#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332
razmser pushed a commit to SuduIDE/llvm-project that referenced this issue Oct 3, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm#64253, and
possibly llvm#64077
and llvm#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332
razmser pushed a commit to SuduIDE/llvm-project that referenced this issue Oct 3, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm#64253, and
possibly llvm#64077
and llvm#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332
razmser pushed a commit to SuduIDE/llvm-project that referenced this issue Oct 6, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm#64253, and
possibly llvm#64077
and llvm#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332
razmser pushed a commit to SuduIDE/llvm-project that referenced this issue Oct 11, 2023
An empty struct is handled as a struct with a dummy i8, on all targets.

Most targets treat an empty struct return value as essentially
void - but some don't. (Currently, at least x86_64-windows-* and
powerpc64le-* don't treat it as void.)

When intializing a struct with such a no_unique_address member,
make sure we don't write the dummy i8 into the struct where there's
no space allocated for it.

Previously it would clobber the actual valid data of the struct.

Fixes llvm#64253, and
possibly llvm#64077
and llvm#64427 as well.

We should omit the store for any empty record (not only ones
declared with no_unique_address); we can have a situation where a
class doesn't have the no_unique_address attribute, but is embedded
in an outer struct with the no_unique_address attribute - like this:

    struct S {};
    S f();
    struct S2 : public S { S2();};
    S2::S2() : S(f()) {}
    struct S3 { int x; [[no_unique_address]] S2 y; S3(); };
    S3::S3() : x(1), y() {}

Here, the problematic store (which this patch omits) is in
the constructor of S2. In the case of S3, S2 has no valid storage
and aliases x - thus the constructor of S2 should omit the dummy
store.

Differential Revision: https://reviews.llvm.org/D157332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants