[Clang] Fix __is_array returning true for zero-sized arrays #86652
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nikolas Klauser (philnik777) ChangesFixes #54705 Full diff: https://github.com/llvm/llvm-project/pull/86652.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7fbe2fec6ca065..e6682028537101 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -345,6 +345,8 @@ Bug Fixes in This Version
- Fixes an assertion failure on invalid code when trying to define member
functions in lambdas.
+- ``__is_array`` no longer returns ``true`` for zero-sized arrays. Fixes (#GH54705).
+
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index c34a40fa7c81ac..93d2b4b259fbc3 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5143,6 +5143,10 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT,
case UTT_IsFloatingPoint:
return T->isFloatingType();
case UTT_IsArray:
+ // zero-sized arrays aren't considered arrays in partial specializations,
+ // so __is_array shouldn't consider them arrays either.
+ if (const auto* CAT = C.getAsConstantArrayType(T))
+ return CAT->getSize() != 0;
return T->isArrayType();
case UTT_IsBoundedArray:
if (!T->isVariableArrayType()) {
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index 14ec17989ec7c7..49df4b668513c0 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -25,6 +25,7 @@ typedef Empty EmptyArMB[1][2];
typedef int Int;
typedef Int IntAr[10];
typedef Int IntArNB[];
+typedef Int IntArZero[0];
class Statics { static int priv; static NonPOD np; };
union EmptyUnion {};
union IncompleteUnion; // expected-note {{forward declaration of 'IncompleteUnion'}}
@@ -685,6 +686,7 @@ void is_array()
{
static_assert(__is_array(IntAr));
static_assert(__is_array(IntArNB));
+ static_assert(!__is_array(IntArZero));
static_assert(__is_array(UnionAr));
static_assert(!__is_array(void));
@@ -1804,7 +1806,7 @@ void is_layout_compatible(int n)
static_assert(!__is_layout_compatible(EnumForward, int));
static_assert(!__is_layout_compatible(EnumClassForward, int));
// FIXME: the following should be rejected (array of unknown bound and void are the only allowed incomplete types)
- static_assert(__is_layout_compatible(CStructIncomplete, CStructIncomplete));
+ static_assert(__is_layout_compatible(CStructIncomplete, CStructIncomplete));
static_assert(!__is_layout_compatible(CStruct, CStructIncomplete));
static_assert(__is_layout_compatible(CStructIncomplete[2], CStructIncomplete[2]));
}
|
You can test this locally with the following command:git-clang-format --diff 5b544b511c7133fcb26a5c563b746a4baefb38d6 90f88bdac3dc40635c58f3f67e29354e6a32896e -- clang/lib/Sema/SemaExprCXX.cpp clang/test/SemaCXX/type-traits.cppView the diff from clang-format here.diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 93d2b4b259..46dd14a871 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -5145,7 +5145,7 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT,
case UTT_IsArray:
// zero-sized arrays aren't considered arrays in partial specializations,
// so __is_array shouldn't consider them arrays either.
- if (const auto* CAT = C.getAsConstantArrayType(T))
+ if (const auto *CAT = C.getAsConstantArrayType(T))
return CAT->getSize() != 0;
return T->isArrayType();
case UTT_IsBoundedArray:
|
There was a problem hiding this comment.
My primary question is: then what is it?
We return true for __is_aggregrate (https://godbolt.org/z/67zjeo7Mj), and an aggregate is an array or class type (https://eel.is/c++draft/dcl.init.aggr#1). This isn't a class type... but it is an aggregate... so it must be an array? (We also don't claim it's a pointer or a reference currently... so this thing will be basically invisible to type traits.)
I would think it is an array given that it uses array syntax for declarations and array semantics for accesses, but it's also not an array that's usable so I can see why others say it's not an array. Personally, I think Clang's behavior here makes the most sense -- we claim it's an array, we also claim it's a bounded array, and we claim its extent is zero (https://godbolt.org/z/4GdYTh4GG), and that matches the declaration for the type. So with this change, I'm worried about how type traits can possibly reason about this type -- I'd like to understand better why other implementations decided this isn't an array and what it's classified as with their type traits. Assuming that logic is compelling, there's more work needed here to handle things like claiming it's a bounded array but not an array.
Also, do we need an ABI tag for folks to get the old behavior given that this change almost certainly will impact ABI through type traits?
| @@ -1804,7 +1806,7 @@ void is_layout_compatible(int n) | |||
| static_assert(!__is_layout_compatible(EnumForward, int)); | |||
| static_assert(!__is_layout_compatible(EnumClassForward, int)); | |||
| // FIXME: the following should be rejected (array of unknown bound and void are the only allowed incomplete types) | |||
| static_assert(__is_layout_compatible(CStructIncomplete, CStructIncomplete)); | |||
| static_assert(__is_layout_compatible(CStructIncomplete, CStructIncomplete)); | |||
There was a problem hiding this comment.
Spurious whitespace change.
According to the spec it's ill-formed, so I'm not sure it falls under any sensible category.
For MSVC it's probably because they don't support template <class T>
inline constexpr bool is_array_v = false;
template <class T>
inline constexpr bool is_array_v<T[]> = true;
template <class T, size_t Size>
inline constexpr bool is_array_v<T[Size]> = true;For Another example, with a type trait that returns true: template <class T, unsigned Size>
void takes_an_array(const T(& arr)[Size]) {}
template <class T>
void do_stuff(T&& v) {
if constexpr (__is_array(__remove_cvref(T))) {
takes_an_array(v);
}
}
void call() {
int arr[0] = {};
do_stuff(arr);
}This fails to compile because
We don't use the trait currently in libc++ because of this bug and GCC only added it in trunk (and have the same bug), so probably not. |
It's an extension we support so it's up to us to decide what sensible is.
Understood, but why is this not the bug to be fixed? (It would also fix the
For example, it seems to be used in Unreal Engine: https://github.com/windystrife/UnrealEngine_NVIDIAGameWorks/blob/b50e6338a7c5b26374d66306ebc7807541ff815e/Engine/Source/Runtime/Core/Public/Templates/UnrealTemplate.h#L128 |
Sure. If we end up putting it in the array category we should probably coordinate with GCC.
Because I don't think we can. AFAICT this is valid C++: template <unsigned Size, class = int[Size]>
int func() {
return 1;
}
template <unsigned Size>
int func() {
return 0;
}If clang accepted
If you think it's a significant enough change that we should add an ABI tag I can do that. |
Hmm, I think you're right: https://godbolt.org/z/PjGoc3Yob, the This doesn't leave us with good options, does it? :-( I think we need to consider the following things:
I don't have a good feeling one way or the other yet, so let's hold off for the moment and see what the final scope of the changes are before making a decision. |
Not really. Zero-sized arrays should probably have been supported by C++ all along, but it's too late to change it now.
I'm not sure what you're referring to. AFAICT
Yes. I'll update the PR accordingly.
I guess it should be true? I can't think of any case where a zero-sized array would be different from a non-zero-sized one at least. I don't have a strong opinion either way though.
Maybe we could think of it as array-like? i.e. it has the same traits as an array except that it's not one. I think the semantics only really break down for array-specific features. Just to get a sense, I've gone through a few traits:
Sounds good. |
It doesn't per spec but it does as a common compiler extension: https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html So my question is more that, if
I'm not entirely certain of how this particular type trait is used in practice. Based on https://sourcegraph.com/search?q=context:global+lang:C%2B%2B+std::is_aggregate+-file:.*libcxx.*+-file:.*test.*+-file:.*clang.*+-file:.*gcc.*&patternType=keyword&sm=0 it seems that most uses for this trait are to decide whether you can use aggregate initialization or not, and
My natural inclination is that it is array-like, but... that just makes me want Oh wow, and it gets even more surprising... adding a zero-length array to a structure reduces the size of the structure (in C++): https://godbolt.org/z/5E4YsT1Ye CC @nickdesaulniers @kees @bwendling for more opinions from people who live with this kind of extension... |
Yes. An array is an array, regardless of its size. The size is just a storage characteristic. It'd almost be like arguing that
It was never as simple as zero-length arrays. It was also trailing arrays of any size in structures. Those extensions create huge problems with being able to reason about the characteristics of arrays, and we do have a flag for this: |
I think it may be worth exploring, but I'm not sure it's feasible. We've been using them accidentally a lot in the test suite in libc++ simply because it's really easy to miss that it's an extension, and we're also using it in our
I don't think the float analogy really works here. The fact is that zero-sized arrays aren't acknowledged as being valid by the standard, so they don't behave like other arrays. It's not just a storage characteristic. Having
I'm not sure how the array being able to change its size at runtime makes it harder to claim it isn't one? non-zero-sized arrays don't do that. It's actually another thing that is different from the rest of the arrays. Another option (that I haven't thought deeply about): Make |
|
Another option would be to to not acknowledge zero-sized arrays in the type traits at all, just like vector types: https://godbolt.org/z/aP685vz8q. That may be the most consistent stance on this. Basically "It's non-standard and doesn't fit neatly in any of the standard categories, so it's in no category at all." |
|
I guess I don't have a strong opinion here, since these helpers are specific to C++, and I've been generally trying to remove fixed-size 0-sized arrays in C projects (i.e. the Linux kernel). I do care about C flexible arrays (and their associated extensions), though. I suspect there is some existing weirdness for C++ when using static initializers, though. See https://godbolt.org/z/PnYMcxhfM Specifically |
That's a possibility, though I wonder if it's actually user-friendly to issue a diagnostic if trying to use a non-standard type that can't fit the C++ type traits model. It's annoying for folks writing generic code, but so long as the diagnostic is SFINAE-able, maybe it's okay? My concern is that either returning |
N.B. MSVC used to have the same bug as well, and it affected their |
|
As I've just commented at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114479 the An optimization to speed up compilation of |
Errr, I think there's some circular logic here perhaps:
Except This boils down to a fundamental question: what makes something an array type in the first place? Is it the declaration syntax or is it the semantics? If it's declaration syntax, then So I think I'm arguing for an LWG issue. |
|
Last time I checked, neither GCC nor Clang allowed T[0] to match a partial specialization for |
Oh, I agree, I am learning to loathe this extension the more I look into this. Did you see this lovely bit:
But that's why I'm asking questions about what we should be doing with the feature overall. As it currently stands, this extension seems like it is broken in C++ in multiple ways. Fixing a tiny corner of it is fine, but I'd like to know what the overall big picture is so we can know whether the tiny corner changes fit or not. We could fix this by changing partial specializations to care about zero-sized arrays, too. (Personally, I would love to "fix" this by deprecating the extension as the only justification for it I've been able to dig up is as a hack for flexible arrays -- deprecating this extension outside of GNU89 mode has some appeal, barring other information about its value.) |
|
I'd personally be fine with deprecating it. As I said above, we use it in libc++ for padding inside struct short_string {
<some data>;
char padding[sizeof(value_type) - 1];
<more data>;
};That could be refactored relatively easily as template <size_t Size>
struct padding_t {
char data[Size];
};
template <>
struct padding_t<0> {};
struct short_string {
<some data>;
[[no_unique_address]] padding_t<sizeof(value_type) - 1> padding;
<more data>;
};
so I don't think that's a use-case worth keeping the extension for. |
Fixes #54705