Skip to content

[libcxx] Remove the second inclusion of the system header #120946

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 23, 2024
Merged

[libcxx] Remove the second inclusion of the system header #120946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions libcxx/include/stdio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,6 @@ void perror(const char* s);
# ifndef _LIBCPP_STDIO_H
# define _LIBCPP_STDIO_H

# if __has_include_next(<stdio.h>)
# include_next <stdio.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It had been done that way because the first include_next <stdio.h> could potentially be the target of a __need_size_t macro (for example), which would result in the first inclusion only declaring size_t, not everything that should be in stdio.h.

After your patch, I believe that the following might not work anymore:

#define __need_size_t
#include <stdio.h>

int main() {
  puts("foo");
}

That's because I'd assume #include <stdio.h> to only define size_t when it gets included after defining the macro. I would expect e.g. puts to only be defined the second time around stdio.h is included, which you removed.

But if I'm wrong (and I'm happy to be), then I don't mind this patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't what you describe exactly the user asked for though? If we wanted to not support this we could just #undef all the __need_ macros and be done with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some system headers do rely on __need macros working though, that's why things broke when I attempted https://reviews.llvm.org/D131425.

Take for example wchar.h. If it gets included with a __need macro, our current implementation would fail because the C library's wchar.h would not define e.g. wcsstr(...), but we use it in our implementation later in libc++'s own wchar.h. That's why I was including the header once again after the first time, with the intent that all required declarations would surely be available after including the C library's wchar.h header for the second time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's indeed a problem. It doesn't seem to affect the CI though, which makes me wonder whether there's actually a __need_ macro in wchar.h or stdlib.h (I couldn't find any in my glibc). Maybe this problem solved itself somehow? stddef.h shouldn't be affected, since we don't actually use any of the libc-provided symbols in stddef.h directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrhosek What version of glibc were you folks using when you saw the issues in #119025?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrhosek What version of glibc were you folks using when you saw the issues in #119025?

2.19

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only support glibc 2.24 and above as documented in https://libcxx.llvm.org. Since this change is technically incorrect (as explained above), I'm not sure what to do with this. When are you moving away from glibc 2.19?

Copy link
Member Author

@petrhosek petrhosek Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We support glibc 2.19 because some of the Linux binaries we build run in Android CI which still uses Ubuntu 14.04, so the answer is "Whenever Android CI stops using Ubuntu 14.04." According to https://wiki.ubuntu.com/Releases, Ubuntu 14.04 EOL is April 2026, so I expect it'll happen sometime before that, but I don't know if there's any date set already. @pirama-arumuga-nainar or @kongy might know more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce the issue I mentioned above. Since we're also not seeing any issues in our CI, let's leave things as-is but pay attention in case a bug report gets filed during the LLVM 20 release.

# endif

# ifdef __cplusplus

# undef getc
Expand Down
4 changes: 0 additions & 4 deletions libcxx/include/stdlib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,6 @@ void *aligned_alloc(size_t alignment, size_t size); // C11
# if !defined(_LIBCPP_STDLIB_H)
# define _LIBCPP_STDLIB_H

# if __has_include_next(<stdlib.h>)
# include_next <stdlib.h>
# endif

# ifdef __cplusplus
extern "C++" {
// abs
Expand Down
4 changes: 0 additions & 4 deletions libcxx/include/wchar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,6 @@ size_t wcsrtombs(char* restrict dst, const wchar_t** restrict src, size_t len,
# include <__mbstate_t.h> // provide mbstate_t
# include <stddef.h> // provide size_t

# if __has_include_next(<wchar.h>)
# include_next <wchar.h>
# endif

// Determine whether we have const-correct overloads for wcschr and friends.
# if defined(_WCHAR_H_CPLUSPLUS_98_CONFORMANCE_)
# define _LIBCPP_WCHAR_H_HAS_CONST_OVERLOADS 1
Expand Down
Loading