This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by nicolerabjohn on Mar 31 2023, 1:37 PM.

Details

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.

Diff Detail

Event Timeline

nicolerabjohn created this revision.Mar 31 2023, 1:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 1:37 PM
nicolerabjohn requested review of this revision.Mar 31 2023, 1:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 1:37 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik requested changes to this revision.Mar 31 2023, 1:45 PM
philnik added a subscriber: philnik.
philnik added inline comments.Mar 31 2023, 1:45 PM
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.

This revision now requires changes to proceed.Mar 31 2023, 1:45 PM

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

nicolerabjohn updated this revision to Diff 511440.EditedApr 6 2023, 9:19 AM

Updating implementation by putting macros within LIBCPP_PUSH_MACROS and LIBCPP_POP_MACROS

nicolerabjohn edited the summary of this revision. (Show Details)Apr 6 2023, 9:58 AM

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.

Updating formatting.

nicolerabjohn marked 3 inline comments as done.Apr 11 2023, 8:53 AM

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.

nicolerabjohn retitled this revision from Fixing conflicting macro definitions between curses.h and std::filesystem. to Fixing conflicting macro definitions between curses.h and the standard library..Apr 11 2023, 8:57 AM
nicolerabjohn edited the summary of this revision. (Show Details)
Mordante added inline comments.Apr 11 2023, 9:24 AM
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 marked 2 inline comments as done.Apr 11 2023, 10:07 AM
nicolerabjohn added inline comments.
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.

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.

Updating diff with more context.

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

philnik requested changes to this revision.May 1 2023, 4:20 PM

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.

This revision now requires changes to proceed.May 1 2023, 4:20 PM

Updating testcase to use NASTY_MACROS

ldionne accepted this revision.May 10 2023, 10:35 AM
ldionne added a subscriber: 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 accepted this revision.May 12 2023, 9:06 AM

LGTM with green CI.

This revision is now accepted and ready to land.May 12 2023, 9:06 AM

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.

Fixing failing test case.

Fix final test cases.

ldionne requested changes to this revision.Jul 4 2023, 12:34 PM

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.

This revision now requires changes to proceed.Jul 4 2023, 12:34 PM

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

nicolerabjohn marked 3 inline comments as done.Jul 5 2023, 12:27 PM
nicolerabjohn added inline comments.
libcxx/include/tuple
1860–1862 ↗(On Diff #535896)

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

ldionne accepted this revision.Jul 5 2023, 2:25 PM

Thanks, LGTM!

This revision is now accepted and ready to land.Jul 5 2023, 2:25 PM

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

nicolerabjohn marked an inline comment as done.

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

Syncing to arcanist

This revision was landed with ongoing or failed builds.Jul 6 2023, 10:21 AM
This revision was automatically updated to reflect the committed changes.