[clang] Reject VLAs in __is_layout_compatible()

[Endilll]

This is a follow-up to #81506. Since __is_layout_compatible() is a C++ intrinsic (

llvm-project/clang/include/clang/Basic/TokenKinds.def

Line 523 in ff1e72d

TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)

), I don't think we should define how it interacts with VLA extension unless we have a compelling reason to.

Since #81506 was merged after 18 cut-off, we don't have to follow any kind of deprecation process.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)

Changes

This is a follow-up to #81506. Since __is_layout_compatible() is a C++ intrinsic (

llvm-project/clang/include/clang/Basic/TokenKinds.def

Line 523 in ff1e72d

TYPE_TRAIT_2(__is_layout_compatible, IsLayoutCompatible, KEYCXX)

), I don't think we should define how it interacts with VLA extension unless we have a compelling reason to.

Since #81506 was merged after 18 cut-off, we don't have to follow any kind of deprecation process.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaExprCXX.cpp (+3)
• (modified) clang/test/SemaCXX/type-traits.cpp (+4-2)

diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 76bb78aa8b5458..db84f181012268 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -6026,6 +6026,9 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, QualType LhsT,
     return false;
   }
   case BTT_IsLayoutCompatible: {
+    if (LhsT->isVariableArrayType() || RhsT->isVariableArrayType())
+      Self.Diag(KeyLoc, diag::err_vla_unsupported)
+          << 1 << tok::kw___is_layout_compatible;
     return Self.IsLayoutCompatible(LhsT, RhsT);
   }
     default: llvm_unreachable("not a BTT");
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index 14ec17989ec7c7..28653e82e5a16e 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -1741,8 +1741,10 @@ void is_layout_compatible(int n)
   static_assert(!__is_layout_compatible(unsigned char, signed char));
   static_assert(__is_layout_compatible(int[], int[]));
   static_assert(__is_layout_compatible(int[2], int[2]));
-  static_assert(!__is_layout_compatible(int[n], int[2])); // FIXME: VLAs should be rejected
-  static_assert(!__is_layout_compatible(int[n], int[n])); // FIXME: VLAs should be rejected
+  static_assert(!__is_layout_compatible(int[n], int[2]));
+  // expected-error@-1 {{variable length arrays are not supported for '__is_layout_compatible'}}
+  static_assert(!__is_layout_compatible(int[n], int[n]));
+  // expected-error@-1 {{variable length arrays are not supported for '__is_layout_compatible'}}
   static_assert(__is_layout_compatible(int&, int&));
   static_assert(!__is_layout_compatible(int&, char&));
   static_assert(__is_layout_compatible(void(int), void(int)));

[cor3ntin]

I think the current behavior is reasonable-ish. Rejecting specific types is a bit weird... I think VLA should model incomplete types (but we currently don't reject that either, which is a bug)

IE, I would expect __is_layout_compatible to return false in the presence of VLAs, incomplete types, and FAM

[Endilll]

I think the current behavior is reasonable-ish. Rejecting specific types is a bit weird... I think VLA should model incomplete types (but we currently don't reject that either, which is a bug)

IE, I would expect __is_layout_compatible to return false in the presence of VLAs, incomplete types, and FAM

My reading of your comment is that in the first paragraph you're asking both VLAs and incomplete types to be rejected, then in the second paragraph you're asking to extend the intrinsic to accept them and yield false. Can you pick one?

(but we currently don't reject that either, which is a bug)

I'll address that in a follow-up PR, even though rejecting __is_layout_compatible(IncompleteStruct, IncompleteStruct) doesn't make sense to me.

[cor3ntin]

We should not reject (ie, make the programm ill-form) any type. Just return false in all of these cases

[AaronBallman]

We should not reject (ie, make the programm ill-form) any type. Just return false in all of these cases

GCC rejects incomplete types: https://godbolt.org/z/xWbes5Wsc
Both Clang and GCC reject instantiating templates with VLAs but accept it in the builtin: https://godbolt.org/z/b95GEGcGx
Both Clang and GCC accept flexible arrays: https://godbolt.org/z/fWbfExMTb

I think the behavior from GCC is defensible, so I think we should reject incomplete types, but accept flexible arrays. VLAs I'm ambivalent about because of the template instantiation behavior. CC @ldionne @philnik777 @mordante for libc++ perspective.

[cor3ntin]

The library does require complete types, interesting https://eel.is/c++draft/meta
However, at the language level, I cannot find any wording either way.

[philnik777]

I think clang should reject incomplete types when the standard says so. It doesn't seem particularly useful to accept some special cases but reject incomplete types in general. All the traits should probably be audited once. It looks like Clang has other problematic cases: https://godbolt.org/z/hajWfq7a6

I don't really care whether Clang should reject VLAs when using the builtin, since the trait will be used through some template and that's rejected that anyways. FWIW it'd be more consistent, since in my mind __some_type_trait(T, U) behaves the same as std::some_type_trait_v<T, U>.

Flexible arrays are accepted because they are arrays of unknown bounds in the type system, which is part of the standard. The extension is only that they aren't rejected at the end of a struct and have some special meaning there. They should definitely not be rejected.

[Endilll]

However, at the language level, I cannot find any wording either way.

In my reading, http://eel.is/c++draft/basic.types.general#11 makes any type layout-compatible with itself, and even ignores cv-qualification:

Two types cv1 T1 and cv2 T2 are layout-compatible types if T1 and T2 are the same type, layout-compatible enumerations, or layout-compatible standard-layout class types.

I find the gap between core language term and type trait rather unfortunate.

[AaronBallman]

However, at the language level, I cannot find any wording either way.

In my reading, http://eel.is/c++draft/basic.types.general#11 makes any type layout-compatible with itself, and even ignores cv-qualification:

Two types cv1 T1 and cv2 T2 are layout-compatible types if T1 and T2 are the same type, layout-compatible enumerations, or layout-compatible standard-layout class types.

I find the gap between core language term and type trait rather unfortunate.

There are a lot of gaps between core language terms and type traits. For another example, copy constructible in the library is quite different from it in the language, volatile types are almost entirely ignored by the library, etc.

At the end of the day, these builtins are intended to be used as the underlying implementation for the type traits, so when in doubt, following the library rules is usually a safe bet.

[AaronBallman]

I think where we ended up is:

1. Reject incomplete types (still needs to be done)
2. Reject VLAs (done, looks good)
3. FAMs are accepted (no changes needed)

So I think you should implement #1 and add tests for it, then add tests for #3, and I think that finishes this.

[Endilll]

I recognize that discussion has spiraled out of VLAs into incomplete types and flexible array members, but can I keep the original scope of this PR, focusing on just VLAs?

[AaronBallman]

I'm fine with splitting the work across multiple patches so long as we get to the desired end state (or have issues filed to track what that end state should be).

[Endilll]

Yes, other items will be addressed in subsequent PRs.

[shafik]

I think focusing on VLAs here is fine, that was the original scope and I don't see an urgent need to expand it here.

[cor3ntin]

Following discussions, LGTM but I'd like to make sure incomplete types don't fall through the cracks