Fixing conflicting macro definitions between curses.h and the standard library.

[nicolerabjohn]

Summary

POSIX allows certain macros to exist with generic names (i.e. refresh(), move(), and erase()) to exist in curses.h which conflict with functions found in std::filesystem, among others. This patch undefs the macros in question and adds them to LIBCPP_PUSH_MACROS and LIBCPP_POP_MACROS.

[philnik]

libcxx/include/__undef_macros

18–26

These should just be #undefed and added to _LIBCPP_PUSH_MACROS and _LIBCPP_POP_MACROS. Arguably they should be replaced in the libc, since these names are used everywhere, not just in <filesystem>.

libcxx/test/libcxx/vendor/ibm/curses_macro_conflict.pass.cpp

14 ↗

(On Diff #510124)

This should just be a .compile.pass.cpp.

[nicolerabjohn]

Addressing reviewer comment to rename testcase and generalizing __undef_macros to work for all platforms.

[nicolerabjohn]

Updating implementation by putting macros within LIBCPP_PUSH_MACROS and LIBCPP_POP_MACROS

[Mordante]

Before approving I really like to know why this only affects <filesystem>. I'm slightly wary to add work-arounds for unrelated external libaries (even when curses is quite Standard).

libcxx/include/__config

1149

I really would like one entry per line, that makes the code a bit longer, but makes it easier to verify. Especially since the entries are not nicely aligned.

libcxx/include/__undef_macros

18–26

Why is this only an issue for filesystem and not for <vector>?

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

12

Why are these IBM specific tests and not using a test feature has-curses?

Or why not something like

#if __has_include(<curses.h>)
#  include <curses.h>
#endif

Then the test could be included in libcxx/test/libcxx/nasty_macros.compile.pass.cpp instead of IBM only.

[nicolerabjohn]

Updating formatting.

[nicolerabjohn]

Before approving I really like to know why this only affects <filesystem>. I'm slightly wary to add work-arounds for unrelated external libaries (even when curses is quite Standard).

Curses in this cases isn't really just any "external" library per-say. It's part of the OS, and standardized by POSIX, so it possible for any compliant UNIX to have this problem depending on the implementation details of the header.

As of now, the conflicts between POSIX-compliant curses.h and these affected headers means that compilers using libc++ (clang, specifically, in this case) can't build CMake on AIX. This feels significant enough to add a workaround for.

libcxx/include/__config

1149

I agree with the formatting here, updated. I had run clang-format on this, but I agree it was not very readable.

libcxx/include/__undef_macros

18–26

It would in theory be an issue for <vector> - the only reason the review wasn't named as such was because the first issue that was noticed came out of <filesystem>. <vector> also already had the LIBCPP_PUSH_MACROS code implemented, so once I added the new macros to the pragma, conflicts within <vector> were fixed. I will update the title/description of the review to be more descriptive.

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

12

So I agree that these don't need to be IBM-specific - libcxx/test/libcxx/nasty_macros.compile.pass.cpp might not be the best choice here either because it's currently XFAILED on AIX for unrelated reasons. AIX is affected by this patch, so I would like these test cases to run on AIX as well. I'll look around and see if I can fix the XFAIL, or find a more appropriate spot for these tests.

[Mordante]

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

12

I think then we really should make sure the AIX tests are fixed, in a separate patch. The nasty macros are really intended for these tests. That also is used to avoid regressions. If a header doesn't push the macros and in C++26 is gets an erase function it will be caught in our CI.

[nicolerabjohn]

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

12

Fair enough - I'll take a look at why it was XFAILed to see if it can be fixed, or if the problematic headers can be split out.

[nicolerabjohn]

There was a fix for nasty_macros.compile.pass.cpp in a recent AIX update. I've put up https://reviews.llvm.org/D148040 to revert the XFAILs. Once that lands, I'll update this patch to put the test cases there.

[nicolerabjohn]

Updating diff with more context.

[nicolerabjohn]

Updating test cases to use nasty_macros.compile.pass.cpp instead of IBM-specific tests.

[nicolerabjohn]

Gentle ping.

[philnik]

Please make sure the CI is green.

libcxx/test/libcxx/nasty_macros.compile.pass.cpp

145–150 ↗

(On Diff #514651)

I think this should just be

#ifndef erase
#. define erase NASTY_MACRO
#endif

to make sure we always push and pop properly.

[nicolerabjohn]

Updating testcase to use NASTY_MACROS

[ldionne]

LGTM w/ comment and green CI.

libcxx/test/libcxx/nasty_macros.compile.pass.cpp

146 ↗

(On Diff #518812)

You can just define all of those unconditionally since there's no header included before. Essentially we know the #ifndef is always true right now.

[philnik]

LGTM with green CI.

[nicolerabjohn]

Rebase

[nicolerabjohn]

Including __undef_macro in more header files - anywhere that move, refresh, or erase are used. This should fix the gcc compat problem.

Lots of files are changed here, so if there are any objections to this direction, I'm happy to hear them - otherwise, I will land in a few days if the CI goes green.

[nicolerabjohn]

Format fix.

[nicolerabjohn]

Fixing failing test case.

[nicolerabjohn]

Fix final test cases.

[ldionne]

I spotted a couple of issues with the new patch.

libcxx/include/optional

258–260 ↗

(On Diff #535896)

Please move this right after

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

like for the other headers.

1673 ↗

(On Diff #535896)

And this needs to go after #endif // _LIBCPP_STD_VER >= 17 then.

libcxx/include/tuple

1860–1862 ↗

(On Diff #535896)

This needs to go after _LIBCPP_END_NAMESPACE_STD, please audit the files you changed to make sure you have the right order. I don't think I saw other problematic ones but just to be sure.

libcxx/include/utility

249–250 ↗

(On Diff #535896)

We shouldn't add it here, we only add it when required.

libcxx/utils/ci/run-buildbot

134 ↗

(On Diff #535896)

This shouldn't be part of this patch.

[nicolerabjohn]

Reordering certain code blocks, general clean-up of the patch.

[nicolerabjohn]

libcxx/include/tuple

1860–1862 ↗

(On Diff #535896)

I found a few, updated them all. They should all follow the same formatting now.

[ldionne]

Thanks, LGTM!

[ldionne]

If you need someone to commit this on your behalf, please let us know the Author Name <email@domain> to use for attribution.

[nicolerabjohn]

Fixing format for CI. Once this goes green, I'll land it.

[nicolerabjohn]

Syncing to arcanist

libcxx/include/__config

 #    define _LIBCPP_ENABLE_CXX20_REMOVED_TYPE_TRAITS
 #  endif // _LIBCPP_ENABLE_CXX20_REMOVED_FEATURES
 
-#  define _LIBCPP_PUSH_MACROS _Pragma("push_macro(\"min\")") _Pragma("push_macro(\"max\")")
-#  define _LIBCPP_POP_MACROS _Pragma("pop_macro(\"min\")") _Pragma("pop_macro(\"max\")")
+#  define _LIBCPP_PUSH_MACROS _Pragma("push_macro(\"min\")") _Pragma("push_macro(\"max\")") _Pragma("push_macro(\"refresh()\")") _Pragma("push_macro(\"move(int, int)\")") _Pragma("push_macro(\"erase()\")")
+#  define _LIBCPP_POP_MACROS _Pragma("pop_macro(\"min\")") _Pragma("pop_macro(\"max\")") _Pragma("pop_macro(\"refresh()\")") _Pragma("pop_macro(\"move(int, int)\")") _Pragma("pop_macro(\"erase()\")")
 
 #  ifndef _LIBCPP_NO_AUTO_LINK
 #    if defined(_LIBCPP_ABI_MICROSOFT) && !defined(_LIBCPP_BUILDING_LIBRARY)

libcxx/include/__undef_macros

 #ifdef max
 #  undef max
 #endif
+
+#ifdef refresh
+#  undef refresh
+#endif
+
+#ifdef move
+#  undef move
+#endif
+
+#ifdef erase
+#  undef erase
+#endif

libcxx/include/__utility/move.h

 #  pragma GCC system_header
 #endif
 
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp>

 
 _LIBCPP_END_NAMESPACE_STD
 
+_LIBCPP_POP_MACROS
+
 #endif // _LIBCPP___UTILITY_MOVE_H

libcxx/test/libcxx/vendor/ibm/curses_filesystem_macro_conflict.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: target={{powerpc.*-ibm-aix.*}}
+
+#include "curses.h"
+#include <filesystem>
+
+int main() { return 0; }

libcxx/test/libcxx/vendor/ibm/curses_utility_macro_conflict.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: target={{powerpc.*-ibm-aix.*}}
+
+#include "curses.h"
+#include <utility>
+
+int main() { return 0; }

libcxx/test/libcxx/vendor/ibm/curses_vector_macro_conflict.compile.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: target={{powerpc.*-ibm-aix.*}}
+
+#include "curses.h"
+#include <vector>
+
+int main() { return 0; }