[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm created this revision.
rsandifo-arm added reviewers: sdesmalen, efriedma, rovka, rjmccall.
Herald added subscribers: cfe-commits, psnobl, rkruppe, tschuett.
Herald added a reviewer: rengolin.
Herald added a project: clang.
rsandifo-arm added parent revisions: D75570: [AST][SVE] Add new Type queries 
for sizeless types, D75571: [Sema][SVE] Add tests for valid and invalid type 
usage.

clang current accepts:

  void foo1(__SVInt8_t *x, __SVInt8_t *y) { *x = *y; }
  void foo2(__SVInt8_t *x, __SVInt8_t *y) {
memcpy(y, x, sizeof(__SVInt8_t));
  }

The first function is valid ACLE code and generates correct LLVM IR.
The second function is invalid ACLE code and generates a zero-length
memcpy.

The patch means that we diagnose the problem in the second case,
rather than silently generating incorrect code.

There's no similar wrong-code bug for alignof.  However, the SVE ACLE
conservatively treats alignof in the same way as sizeof, just as the
C++ standard does for incomplete types.  The idea is that layout of
sizeless types is an implementation property and isn't defined at
the language level.

We could relax the alignof rules in future if they turn out to be
too restrictive.  It would be harder to go the other way though,
and forbid alignof after previously treating it as valid.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75572

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/aarch64-sve-types.c
  clang/test/Sema/sizeless-1.c
  clang/test/SemaCXX/sizeless-1.cpp

Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -19,6 +19,20 @@
 typedef svint8_t int8_typedef;
 typedef svint8_t *int8_ptr_typedef;
 
+int sizeof_int8 = sizeof(svint8_t); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var = sizeof(*extern_int8_ptr); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var_ptr = sizeof(extern_int8_ptr);
+
+#if __cplusplus >= 201103L
+int alignof_int8 = alignof(svint8_t); // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = alignof(*extern_int8_ptr); // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+#else
+int alignof_int8 = _Alignof(svint8_t); // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = _Alignof(*extern_int8_ptr); // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = _Alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+#endif
+
 void pass_int8(svint8_t); // expected-note {{no known conversion}}
 
 svint8_t return_int8();
@@ -49,6 +63,8 @@
   svint8_t local_int8;
   svint16_t local_int16;
 
+  int _Alignas(svint8_t) aligned_int; // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+
   // Using pointers to sizeless data isn't wrong here, but because the
   // type is incomplete, it doesn't provide any alignment guarantees.
   _Static_assert(__atomic_is_lock_free(1, &local_int8) == __atomic_is_lock_free(1, incomplete_ptr), "");
Index: clang/test/Sema/sizeless-1.c
===
--- clang/test/Sema/sizeless-1.c
+++ clang/test/Sema/sizeless-1.c
@@ -15,6 +15,14 @@
 typedef svint8_t int8_typedef;
 typedef svint8_t *int8_ptr_typedef;
 
+int sizeof_int8 = sizeof(svint8_t); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var = sizeof(*extern_int8_ptr); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var_ptr = sizeof(extern_int8_ptr);
+
+int alignof_int8 = _Alignof(svint8_t); // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = _Alignof(*extern_int8_ptr); // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = _Alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+
 void pass_int8(svint8_t); // expected-note {{passing argument to parameter here}}
 
 svint8_t return_int8();
@@ -46,6 +54,8 @@
   svint8_t local_int8;
   svint16_t local_int16;
 
+  int _Alignas(svint8_t) aligned_int; // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+
   // Using pointers to sizeless data isn't wrong here, but because the
   // type is incomplete, it doesn't provide any alignment guarantees.
   _Static_assert(__atomic_is_lock_free(1, &local_int8) == __atomic_is_lock_free(1, incompl

[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

The planned changes to RequireCompleteType should catch this, right?  If we 
want a specialized diagnostic for sizeless types in RequireCompleteType, we 
should probably just pass it to RequireCompleteType.  (Otherwise, you'll end up 
with the "wrong" diagnostic in templates.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75572/new/

https://reviews.llvm.org/D75572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Just looked at the comment on D75572 ; not 
sure if RequireCompleteType is going to reject sizeless types.  In any case, 
you have to instantiate templates using RequireCompleteType before you can 
determine whether a type is sizeless.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75572/new/

https://reviews.llvm.org/D75572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added a comment.

In D75572#1904415 , @efriedma wrote:

> The planned changes to RequireCompleteType should catch this, right?  If we 
> want a specialized diagnostic for sizeless types in RequireCompleteType, we 
> should probably just pass it to RequireCompleteType.  (Otherwise, you'll end 
> up with the "wrong" diagnostic in templates.)


Yeah, the planned changes to RequireCompleteType would catch this by default.  
If RequireCompleteType should always get the first shot, I guess there are 
three possible approaches:

1. Stick with the approach in D62962 , without 
specialised error messages.  I can break that patch up into smaller chunks if 
the general approach seems right.
2. Stick with the approach in this patch of having seperate isSizelessType 
checks and diagnostics, but test isSizelessType after RequireCompleteType 
instead of before it.  The later changes to RequireCompleteType allow sizeless 
types to be let through on demand, and the isSizelessType handling would be 
similar to the isFunctionType handling just below.
3. Like you say, stick with separate diagnostics for incomplete and sizeless 
types but make RequireCompleteType emit them both.

I guess one way doing 3. would be to use %select{incomplete|sizeless} and pass 
the result of isSizelessType() as one of the diagnostic parameters to 
RequireCompleteType.  But in some ways that feels a bit clunky because the 
caller is then doing the work to prove that the use is invalid (which it always 
is if isSizelessType()) but isn:t actually doing the work to emit the 
associated diagnostic.  Another option would be to make RequireCompleteType 
have a choice between three actions:

- the normal, standard-defined behaviour, with no special error messages for 
sizeless types.  This would remain the default if no action is explicitly 
specified.
- a mode that explicitly allows sizeless types
- a mode that explicitly disallows sizeless types, with the first diagnostic 
parameter being whether the type is sizeless or not.  In contrast to the above, 
RequireCompleteType would add this parameter automatically.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75572/new/

https://reviews.llvm.org/D75572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I think the specialized error messages are useful.  I don't have a strong 
opinion on what order the patches should land.

The difference between emitting the diagnostic in RequireCompleteType, vs. 
letting the caller do it, is basically just that it's harder to forget to check 
whether the type is sizeless.  I think that's important enough to be worth 
complicating the API a bit; better to forbid scalable types, rather than crash 
or miscompile later.  Probably the entry point that allows sizeless types 
should have a different name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75572/new/

https://reviews.llvm.org/D75572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm updated this revision to Diff 248215.
rsandifo-arm added a comment.

Changes in v2:

- Emit the diagnostic from RequireCompleteTypeImpl instead of checking for 
sizeless types separately.  This implements option 3. from the earlier 
discussion.

- Reformat the patch using git-clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75572/new/

https://reviews.llvm.org/D75572

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/aarch64-sve-types.c
  clang/test/Sema/sizeless-1.c
  clang/test/SemaCXX/sizeless-1.cpp

Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -21,6 +21,20 @@
 typedef svint8_t int8_typedef;
 typedef svint8_t *int8_ptr_typedef;
 
+int sizeof_int8 = sizeof(svint8_t); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var = sizeof(*extern_int8_ptr); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var_ptr = sizeof(extern_int8_ptr);
+
+#if __cplusplus >= 201103L
+int alignof_int8 = alignof(svint8_t);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = alignof(*extern_int8_ptr);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+#else
+int alignof_int8 = _Alignof(svint8_t);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = _Alignof(*extern_int8_ptr);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = _Alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+#endif
+
 void pass_int8(svint8_t); // expected-note {{no known conversion}}
 
 svint8_t return_int8();
@@ -51,6 +65,8 @@
   svint8_t local_int8;
   svint16_t local_int16;
 
+  int _Alignas(svint8_t) aligned_int; // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+
   // Using pointers to sizeless data isn't wrong here, but because the
   // type is incomplete, it doesn't provide any alignment guarantees.
   _Static_assert(__atomic_is_lock_free(1, &local_int8) == __atomic_is_lock_free(1, incomplete_ptr), "");
Index: clang/test/Sema/sizeless-1.c
===
--- clang/test/Sema/sizeless-1.c
+++ clang/test/Sema/sizeless-1.c
@@ -15,6 +15,14 @@
 typedef svint8_t int8_typedef;
 typedef svint8_t *int8_ptr_typedef;
 
+int sizeof_int8 = sizeof(svint8_t); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var = sizeof(*extern_int8_ptr); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var_ptr = sizeof(extern_int8_ptr);
+
+int alignof_int8 = _Alignof(svint8_t);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = _Alignof(*extern_int8_ptr);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = _Alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+
 void pass_int8(svint8_t); // expected-note {{passing argument to parameter here}}
 
 svint8_t return_int8();
@@ -46,6 +54,8 @@
   svint8_t local_int8;
   svint16_t local_int16;
 
+  int _Alignas(svint8_t) aligned_int; // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+
   // Using pointers to sizeless data isn't wrong here, but because the
   // type is incomplete, it doesn't provide any alignment guarantees.
   _Static_assert(__atomic_is_lock_free(1, &local_int8) == __atomic_is_lock_free(1, incomplete_ptr), "");
Index: clang/test/Sema/aarch64-sve-types.c
===
--- clang/test/Sema/aarch64-sve-types.c
+++ clang/test/Sema/aarch64-sve-types.c
@@ -1,52 +1,39 @@
 // RUN: %clang_cc1 %s -triple aarch64-none-linux-gnu -target-feature +sve -fsyntax-only -verify
 
-// This test is invalid under the sizeless type extension and is a stop-gap
-// until that extension is added.  The test makes sure that sizeof and
-// alignof queries are handled without assertion failures, since at
-// present there is nothing to prevent such queries being made.
-//
-// Under this scheme, sizeof returns 0 for all built-in sizeless types.
-// This is compatible with correct usage but it relies on the user being
-// careful to avoid constructs that depe

[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added a comment.

In D75572#1904517 , @efriedma wrote:

> I think the specialized error messages are useful.  I don't have a strong 
> opinion on what order the patches should land.


OK.  The new version keeps the tailored diagnostic but emits it from 
RequireCompleteTypeImpl instead.

> The difference between emitting the diagnostic in RequireCompleteType, vs. 
> letting the caller do it, is basically just that it's harder to forget to 
> check whether the type is sizeless.  I think that's important enough to be 
> worth complicating the API a bit; better to forbid scalable types, rather 
> than crash or miscompile later.  Probably the entry point that allows 
> sizeless types should have a different name.

Yeah, the plan is to make RequireCompleteType reject sizeless types unless the 
caller has explicitly said otherwise.  We want to do that whatever approach is 
taken for emitting the diagnostics.  However, starting off with a patch that 
makes the types incomplete and then gradually relaxing the rules would 
interfere too much with other people's work, so the idea was to do it the other 
way around.  By building the series up this way, the final patch that makes 
sizeless types incomplete ends up being much smaller (and hopefully more 
reviewable!) than D62962  was.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75572/new/

https://reviews.llvm.org/D75572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a reviewer: rsmith.
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM

Please give it a day or two before you merge in case someone else has an 
opinion on the new API.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75572/new/

https://reviews.llvm.org/D75572



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75572: [Sema][SVE] Reject sizeof and alignof for sizeless types

[Richard Sandiford via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG39969c7d3a6d: [Sema][SVE] Reject sizeof and alignof for 
sizeless types (authored by rsandifo-arm).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75572/new/

https://reviews.llvm.org/D75572

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/Sema/aarch64-sve-types.c
  clang/test/Sema/sizeless-1.c
  clang/test/SemaCXX/sizeless-1.cpp

Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -17,6 +17,20 @@
 typedef svint8_t int8_typedef;
 typedef svint8_t *int8_ptr_typedef;
 
+int sizeof_int8 = sizeof(svint8_t); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var = sizeof(*extern_int8_ptr); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var_ptr = sizeof(extern_int8_ptr);
+
+#if __cplusplus >= 201103L
+int alignof_int8 = alignof(svint8_t);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = alignof(*extern_int8_ptr);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+#else
+int alignof_int8 = _Alignof(svint8_t);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = _Alignof(*extern_int8_ptr);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = _Alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+#endif
+
 void pass_int8(svint8_t); // expected-note {{no known conversion}}
 
 svint8_t return_int8();
@@ -47,6 +61,8 @@
   svint8_t local_int8;
   svint16_t local_int16;
 
+  int _Alignas(svint8_t) aligned_int; // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+
   // Using pointers to sizeless data isn't wrong here, but because the
   // type is incomplete, it doesn't provide any alignment guarantees.
   _Static_assert(__atomic_is_lock_free(1, &local_int8) == __atomic_is_lock_free(1, incomplete_ptr), "");
Index: clang/test/Sema/sizeless-1.c
===
--- clang/test/Sema/sizeless-1.c
+++ clang/test/Sema/sizeless-1.c
@@ -12,6 +12,14 @@
 typedef svint8_t int8_typedef;
 typedef svint8_t *int8_ptr_typedef;
 
+int sizeof_int8 = sizeof(svint8_t); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var = sizeof(*extern_int8_ptr); // expected-error {{invalid application of 'sizeof' to sizeless type 'svint8_t'}}
+int sizeof_int8_var_ptr = sizeof(extern_int8_ptr);
+
+int alignof_int8 = _Alignof(svint8_t);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+int alignof_int8_var = _Alignof(*extern_int8_ptr);// expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}} expected-warning {{GNU extension}}
+int alignof_int8_var_ptr = _Alignof(extern_int8_ptr); // expected-warning {{GNU extension}}
+
 void pass_int8(svint8_t); // expected-note {{passing argument to parameter here}}
 
 svint8_t return_int8();
@@ -43,6 +51,8 @@
   svint8_t local_int8;
   svint16_t local_int16;
 
+  int _Alignas(svint8_t) aligned_int; // expected-error {{invalid application of 'alignof' to sizeless type 'svint8_t'}}
+
   // Using pointers to sizeless data isn't wrong here, but because the
   // type is incomplete, it doesn't provide any alignment guarantees.
   _Static_assert(__atomic_is_lock_free(1, &local_int8) == __atomic_is_lock_free(1, incomplete_ptr), "");
Index: clang/test/Sema/aarch64-sve-types.c
===
--- clang/test/Sema/aarch64-sve-types.c
+++ clang/test/Sema/aarch64-sve-types.c
@@ -1,52 +1,39 @@
 // RUN: %clang_cc1 %s -triple aarch64-none-linux-gnu -target-feature +sve -fsyntax-only -verify
 
-// This test is invalid under the sizeless type extension and is a stop-gap
-// until that extension is added.  The test makes sure that sizeof and
-// alignof queries are handled without assertion failures, since at
-// present there is nothing to prevent such queries being made.
-//
-// Under this scheme, sizeof returns 0 for all built-in sizeless types.
-// This is compatible with correct usage but it relies on the user being
-// careful to avoid constructs that depend directly or indirectly on the
-// value of sizeof.  (The sizeless type extension avoids this by treating
-//