[libc++] Add missing cstdalign header

[sbc100]

Summary

https://timsong-cpp.github.io/cppwp/n4659/depr.cstdalign.syn

[sbc100]

• revert

[sbc100]

ping

[ldionne]

I believe this header has been deprecated. According to http://eel.is/c++draft/diff.cpp17.library, the effect of this header is nothing. @mclow.lists can you chime in?

[hubert.reinterpretcast]

The resolution of LWG 2828 means that <cstdalign> exists in C++11 and C++14. The rationale from https://reviews.llvm.org/D96786#2566110 can be taken as saying that adding this header for C++11 conformance is reasonable.

[ldionne]

In D46443#2566394, @hubert.reinterpretcast wrote:

The resolution of LWG 2828 means that <cstdalign> exists in C++11 and C++14. The rationale from https://reviews.llvm.org/D96786#2566110 can be taken as saying that adding this header for C++11 conformance is reasonable.

Yes, I agree, let's add it. But the header needs to be added in include/CMakeLists.txt. There also needs to be a test added to check for the contents of the headers, and the auto-generated header tests should be modified. Take a look at what I did in https://reviews.llvm.org/D97870 for <cuchar>.

[sbc100]

Rebase against mono-repo

[sbc100]

feedback

[curdeius]

libcxx/include/cstdalign

19–22 ↗

(On Diff #328524)

You should test that each of these macros is really defined when including cstdalign.

libcxx/test/libcxx/utilities/any/size_and_alignment.pass.cpp

16 ↗

(On Diff #328524)

This doesn't seem to be necessary. Or?
In C++, alignof is a keyword.

[sbc100]

libcxx/test/libcxx/utilities/any/size_and_alignment.pass.cpp

16 ↗

(On Diff #328524)

You are right its not necessary in C++ since its a keyword. My memory of exactly why I added to this file in the original version of the CL back in 2018 is ... missing. Maybe its to ensure that the inclusion of the C header and the macro don't interfere with C++ usage?

I'm happy to revert this line and find a better way to test.

[sbc100]

libcxx/include/cstdalign

19–22 ↗

(On Diff #328524)

Done. As you can see there is not much to tests since in C++ this is essentially an empty header.

[sbc100]

add tests

[ldionne]

libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp

8 ↗

(On Diff #328538)

I think we should add // REQUIRES: c++03 || c++11 || c++14 || c++17, because a conforming implementation could be not shipping the header in C++20 and beyond.

14–28 ↗

(On Diff #328538)

This can be moved outside of main().

27 ↗

(On Diff #328538)

I'm not seeing __alignof_is_defined anywhere in the spec?

[sbc100]

libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp

14–28 ↗

(On Diff #328538)

I was copying from libcxx/test/std/language.support/cstdint/cstdint.syn/cstdint.pass.cpp which does these checks inside of main.

Will move them.

27 ↗

(On Diff #328538)

Removed

[sbc100]

feedback

[sbc100]

feedback

[curdeius]

LGTM but please wait for libc++ group approval.
If you need someone to land it for you, please provide "Firstname Name <email@org>" for attribution.

[ldionne]

Please wait for CI to finish!

[hubert.reinterpretcast]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

This seems to be assuming that the underlying C library's stdalign.h is C++ friendly. A C11 stdalign.h does define alignof and alignas as macros.

libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp

27 ↗

(On Diff #328538)

Seems like a defect in the old standard. The prose doesn't match the synopsis. __alignof_is_defined is a macro in C11's stdalign.h (and so is alignof). That the C++ committee did not intend for an alignof macro can probably be assumed. I suspect the lack of an __alignof_is_defined macro was also unintended.

[sbc100]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

Should I just remove this #include then?

libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp

27 ↗

(On Diff #328538)

So should I add back the check for __alignof_is_defined?

[hubert.reinterpretcast]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

The idea would be to add a stdalign.h alongside this header that doesn't #include_next the underlying C library's stdalign.h.

libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp

27 ↗

(On Diff #328538)

I think so (in addition to also checking that alignof is not defined as a macro). I think the patch needs to be confirmed again either way by the libc++ approvers though.

[curdeius]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

I'm not sure if that should be the solution. At least gcc's libstdc++ assumes that stdalign.h is C++-compatbile (cf. https://github.com/gcc-mirror/gcc/blob/16e2427f50c208dfe07d07f18009969502c25dc8/libstdc%2B%2B-v3/include/c_global/cstdalign).

Clang provides a compatible header: https://github.com/llvm/llvm-project/commit/8acb4044d83ecc9df81b1c9f327d5bd4325e1756.
Gcc too of course: https://github.com/gcc-mirror/gcc/blob/16e2427f50c208dfe07d07f18009969502c25dc8/gcc/ginclude/stdalign.h.

MSVC's STL on the other hand, doesn't include <stdalign.h> (https://github.com/microsoft/STL/blob/main/stl/inc/cstdalign).

@hubert.reinterpretcast, are you aware of an environment which has non-friendly stdalign.h?

[curdeius]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

FYI, musl is also C++ friendly: https://git.musl-libc.org/cgit/musl/tree/include/stdalign.h.

[hubert.reinterpretcast]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

Quote:

@hubert.reinterpretcast, are you aware of an environment which has non-friendly stdalign.h?

The one GCC provides disagrees with the interpretation I gave of which macros should be present. The one that Clang provides does match my interpretation. It seems the GCC one is non-friendly (albeit a different form of non-friendly than the one I opened with).

[curdeius]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

Oh, you mean that __alignas_is_defined and __alignof_is_defined won't be defined in this case, right?
In this case, I guess we won't avoid having stdalign.h as you had suggested.
And indeed the test fails with gcc:

bin/llvm-lit -vv ../../libcxx/test/std/language.support/cstdalign/ --param=std=c++17 --param=cxx_under_test=`which g++`
...
libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp:21:2: error: #error __alignas_is_defined not defined
   21 | #error __alignas_is_defined not defined
      |  ^~~~~

That's unfortunately a configuration which is not tested in the CI.

libcxx/test/std/language.support/cstdalign/cstdalign.pass.cpp

16–18 ↗

(On Diff #328561)

Please do the same for alignof.

27 ↗

(On Diff #328538)

Please check __alignof_is_defined as well, but guard it with #ifdef _LIBCPP_VERSION to be conforming.
I agree that it seems to be a defect in the old standard.

[hubert.reinterpretcast]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

Yes, it looks like adding a stdalign.h for libc++ is needed to reliably get __alignas_is_defined and __alignof_is_defined.

[sbc100]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

Alternatively, could we just defined them if they are not already and skip including stdalign.h here?

[hubert.reinterpretcast]

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

stdalign.h is a header defined in C++17 like stdbool.h is one. libc++ has a stdbool.h, so I think having a stdalign.h makes sense (since we know that stdalign.h in some environments does not do what we want).

[ldionne]

Trying to summarize the discussion here for the author:

1. Please add a <stdalign.h> header to libc++ that does #include_next <stdalign.h> and then defines both __alignof_is_defined and __alignas_is_defined.
2. Please re-add a test checking for __alignof_is_defined, but guard it with #if defined(_LIBCPP_VER) so that we only test it on libc++ (since technically the Standard doesn't require it, even if it's a defect).
3. Please re-run libcxx/utils/generate_header_tests.py and make sure that creates no diff.

libcxx/include/cstdalign

24 ↗

(On Diff #328561)

However, http://eel.is/c++draft/depr.stdalign.h.syn#1 says:

The contents of the C++ header <stdalign.h> are the same as the C standard library header <stdalign.h>, with the following changes: The header <stdalign.h> does not define a macro named alignas.

I think the most efficient way of ensuring that is to use #include_next, which requires a C++ friendly <stdalign.h> on the platform.

Generally speaking, I'm not in favour of trying to work around platforms that do not provide C++ friendly headers. If we're trying to make C++ work on those platforms, they need to minimally provide C++ friendly headers.

include/cassert

 // -*- C++ -*-
 //===-------------------------- cassert -----------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#ifndef _LIBCPP_CASSERT
+#define _LIBCPP_CASSERT
+
 /*
     cassert synopsis
 
 Macros:
 
     assert
 
 */
 
 #include <__config>
 #include <assert.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
+
+#endif  // _LIBCPP_CASSERT

include/cstdalign

 // -*- C++ -*-
-//===-------------------------- cassert -----------------------------------===//
+//===--------------------------- cstdalign --------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#ifndef _LIBCPP_CSTDALIGN
+#define _LIBCPP_CSTDALIGN
+
 /*
-    cassert synopsis
+    cstdalign synopsis
 
 Macros:
 
-    assert
+   alignof
+   alignas
+    __alignof_is_defined
+    __alignas_is_defined
 
 */
 
 #include <__config>
-#include <assert.h>
+#include <stdalign.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
+
+#endif  // _LIBCPP_CSTDALIGN

test/libcxx/min_max_macros.sh.cpp

 #include <complex.h>
 TEST_MACROS();
 #include <condition_variable>
 TEST_MACROS();
 #include <csetjmp>
 TEST_MACROS();
 #include <csignal>
 TEST_MACROS();
+#include <cstdalign>
+TEST_MACROS();
 #include <cstdarg>
 TEST_MACROS();
 #include <cstdbool>
 TEST_MACROS();
 #include <cstddef>
 TEST_MACROS();
 #include <cstdint>
 TEST_MACROS();

test/libcxx/utilities/any/size_and_alignment.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14
 
 // <any>
 
 // Check that the size and alignment of any are what we expect.
 
 #include <any>
+#include <cstdalign>
 
 int main()
 {
     using std::any;
     static_assert(sizeof(any) == sizeof(void*)*4, "");
     static_assert(alignof(any) == alignof(void*), "");
 }