Skip to content

[libc++] Remove potential 0-sized array in __compressed_pair_padding #109028

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

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

serge-sans-paille
Copy link
Collaborator

No description provided.

@serge-sans-paille serge-sans-paille requested a review from a team as a code owner September 17, 2024 18:42
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 17, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 17, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-libcxx

Author: None (serge-sans-paille)

Changes

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

1 Files Affected:

  • (modified) libcxx/include/__memory/compressed_pair.h (+6-4)
diff --git a/libcxx/include/__memory/compressed_pair.h b/libcxx/include/__memory/compressed_pair.h
index 629e3ad8848ffa..cc56912f5e34c5 100644
--- a/libcxx/include/__memory/compressed_pair.h
+++ b/libcxx/include/__memory/compressed_pair.h
@@ -52,13 +52,15 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 
 #ifndef _LIBCPP_ABI_NO_COMPRESSED_PAIR_PADDING
 
-template <class _ToPad>
+template <class _ToPad,
+          bool _Empty = ((is_empty<_ToPad>::value && !__libcpp_is_final<_ToPad>::value) || is_reference<_ToPad>::value)>
 class __compressed_pair_padding {
-  char __padding_[((is_empty<_ToPad>::value && !__libcpp_is_final<_ToPad>::value) || is_reference<_ToPad>::value)
-                      ? 0
-                      : sizeof(_ToPad) - __datasizeof_v<_ToPad>];
+  char __padding_[sizeof(_ToPad) - __datasizeof_v<_ToPad>];
 };
 
+template <class _ToPad>
+class __compressed_pair_padding<_ToPad, true> {};
+
 #  define _LIBCPP_COMPRESSED_PAIR(T1, Initializer1, T2, Initializer2)                                                  \
     _LIBCPP_NO_UNIQUE_ADDRESS __attribute__((__aligned__(_LIBCPP_ALIGNOF(T2)))) T1 Initializer1;                       \
     _LIBCPP_NO_UNIQUE_ADDRESS ::std::__compressed_pair_padding<T1> _LIBCPP_CONCAT3(__padding1_, __LINE__, _);          \

@serge-sans-paille
Copy link
Collaborator Author

In the spirit of #105865 - I don't think we can use a similar implementation as what we did there, but that's the same idea / motivation

@serge-sans-paille serge-sans-paille force-pushed the feature/remove-empty-array branch 2 times, most recently from a64e67a to 0c70565 Compare September 18, 2024 09:32
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 18, 2024
@serge-sans-paille serge-sans-paille force-pushed the feature/remove-empty-array branch from 0c70565 to dae40cd Compare September 18, 2024 10:29
char __padding_[((is_empty<_ToPad>::value && !__libcpp_is_final<_ToPad>::value) || is_reference<_ToPad>::value)
? 0
: sizeof(_ToPad) - __datasizeof_v<_ToPad>];
char __padding_[sizeof(_ToPad) - __datasizeof_v<_ToPad>];
Copy link
Member

Choose a reason for hiding this comment

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

Can't we reuse the same __padding struct we now have in <string>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed

@serge-sans-paille
Copy link
Collaborator Author

It's not clear to me if the issue here come from the CI system or from my patch :-/

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

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

This needs a more detailed summary, e.g. what is the motivation for this change?

@ldionne
Copy link
Member

ldionne commented Sep 25, 2024

Please rebase onto main to re-trigger CI. The CI instability should be resolved now.

@serge-sans-paille serge-sans-paille force-pushed the feature/remove-empty-array branch from 3d28f77 to 1112cfd Compare September 30, 2024 05:34
@serge-sans-paille serge-sans-paille force-pushed the feature/remove-empty-array branch 2 times, most recently from ba11fce to cb61492 Compare September 30, 2024 08:40
Copy link

github-actions bot commented Sep 30, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@serge-sans-paille serge-sans-paille force-pushed the feature/remove-empty-array branch 2 times, most recently from 87b87bd to 4ddd5a7 Compare September 30, 2024 09:02
@serge-sans-paille serge-sans-paille force-pushed the feature/remove-empty-array branch from 4ddd5a7 to 0ab7824 Compare September 30, 2024 09:06
@serge-sans-paille
Copy link
Collaborator Author

@shafik : I'm trying to have libcxx compile with -Wzero-length-array. With these two commits, we're done.

@ldionne : I undid the reuse of __padding. The motivation is two folds: 1st it would create an extra header dependency (moving __padding definition to another header so that it can be included both in string and compressed_pair.h) and second compressed_pair is now scarcely used, so it's probably ok to keep related stuff in the same header (?).

@ldionne ldionne merged commit 0eb2602 into llvm:main Oct 1, 2024
60 checks passed
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants