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

release/18.x: [libc++] Simplify the implementation of <stddef.h> (#86843) #87374

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Apr 2, 2024

Backport 2950283

Requested by: @ian-twilightcoder

@llvmbot llvmbot added this to the LLVM 18.X Release milestone Apr 2, 2024
@llvmbot llvmbot requested a review from a team as a code owner April 2, 2024 17:12
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 2, 2024
@llvmbot
Copy link
Collaborator Author

llvmbot commented Apr 3, 2024

@llvm/pr-subscribers-libcxx

Author: None (llvmbot)

Changes

Backport 2950283

Requested by: @ian-twilightcoder


Full diff: https://github.com/llvm/llvm-project/pull/87374.diff

1 Files Affected:

  • (modified) libcxx/include/stddef.h (+8-17)
diff --git a/libcxx/include/stddef.h b/libcxx/include/stddef.h
index 887776b150e49d..470b5408336c6d 100644
--- a/libcxx/include/stddef.h
+++ b/libcxx/include/stddef.h
@@ -7,18 +7,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-#if defined(__need_ptrdiff_t) || defined(__need_size_t) || defined(__need_wchar_t) || defined(__need_NULL) ||          \
-    defined(__need_wint_t)
-
-#  if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-#    pragma GCC system_header
-#  endif
-
-#  include_next <stddef.h>
-
-#elif !defined(_LIBCPP_STDDEF_H)
-#  define _LIBCPP_STDDEF_H
-
 /*
     stddef.h synopsis
 
@@ -36,16 +24,19 @@
 
 */
 
-#  include <__config>
+#include <__config>
+
+// Note: This include is outside of header guards because we sometimes get included multiple times
+//       with different defines and the underlying <stddef.h> will know how to deal with that.
+#include_next <stddef.h>
+
+#ifndef _LIBCPP_STDDEF_H
+#  define _LIBCPP_STDDEF_H
 
 #  if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #    pragma GCC system_header
 #  endif
 
-#  if __has_include_next(<stddef.h>)
-#    include_next <stddef.h>
-#  endif
-
 #  ifdef __cplusplus
 typedef decltype(nullptr) nullptr_t;
 #  endif

@ian-twilightcoder
Copy link
Contributor

XPASS: llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_ctor_conversion.pass.cpp

Doesn't seem to have anything to do with this change.

@ldionne
Copy link
Member

ldionne commented Apr 3, 2024

I agree the test failure here is a fluke.

Merging this to LLVM 18 would be fine with me, however we are fairly late in the release cycle and this change is a bit tricky: we sometimes have surprises when changing C compatibility headers. I would have preferred merging this earlier in the release cycle.

We definitely need this fix downstream, however since we haven't seen any issue related to this upstream, I would also be OK with not merging to release/18.x and only cherry-picking this downstream for our needs.

I'll let the release manager decide.

@ian-twilightcoder
Copy link
Contributor

ian-twilightcoder commented Apr 4, 2024

@tstellar what do you think? I'm a little nervous someone else will run into mysterious errors where rsize_t isn't declared in C++ mode, and if they aren't intimately familiar with the workings of the clang header they're going to be very confused for a very long time. But on the other hand no one else has run into that yet as far as we know, and stddef.h is particularly touchy. I would still personally err on the side of taking this, at it effectively fixes a C++ regression caused by https://reviews.llvm.org/D157757

Libc++'s own <stddef.h> is complicated by the need to handle various
platform-specific macros and to support duplicate inclusion. In reality,
we only need to add a declaration of nullptr_t to it, so we can simply
include the underlying <stddef.h> outside of our guards to let it handle
re-inclusion itself.

(cherry picked from commit 2950283)
@tstellar tstellar merged commit bffecba into llvm:release/18.x Apr 10, 2024
8 of 9 checks passed
@nico
Copy link
Contributor

nico commented Apr 10, 2024

As mentioned on #86843, this breaks using -pedantic in code that includes libc++ headers. Since this got cherry-picked, we also must cherry-pick #88214.

@ian-twilightcoder
Copy link
Contributor

Well, I wish I would've known that. We cherry-picked it because it's a regression that was introduced in LLVM 18, and I am worried that it will cause issues on some platforms.

@ldionne
Copy link
Member

ldionne commented Apr 11, 2024

As mentioned on #86843, this breaks using -pedantic in code that includes libc++ headers. Since this got cherry-picked, we also must cherry-pick #88214.

I just started that process here: #88214 (comment)

@tstellar
Copy link
Collaborator

Hi @ian-twilightcoder (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix.

@ian-twilightcoder
Copy link
Contributor

I don't think this needs a release note, it's a pretty minor change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants