[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

[Eli Friedman via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG27a574962567: [Sema][Windows] Don't special-case void* 
in __unaligned conversions. (authored by efriedma).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120936

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/MicrosoftExtensions.cpp

Index: clang/test/SemaCXX/MicrosoftExtensions.cpp
===
--- clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -85,18 +85,22 @@
   foo_unaligned(p2);
 
   __unaligned B_unaligned *p3 = 0;
-  int p4 = foo_unaligned(p3);
+  int p4 = foo_unaligned(p3); // expected-error {{cannot initialize a variable of type 'int' with an rvalue of type 'void *'}}
+  // expected-warning@-1 {{implicit cast from type '__unaligned B_unaligned *' to type 'B_unaligned *' drops __unaligned qualifier}}
 
-  B_unaligned *p5 = p3; // expected-error {{cannot initialize a variable of type 'B_unaligned *' with an lvalue of type '__unaligned B_unaligned *'}}
+  B_unaligned *p5 = p3;
+  // expected-warning@-1 {{implicit cast from type '__unaligned B_unaligned *' to type 'B_unaligned *' drops __unaligned qualifier}}
 
   __unaligned B_unaligned *p6 = p3;
 
   p1_aligned_type4 = p2_aligned_type4;
-  p2_aligned_type4 = p1_aligned_type4; // expected-error {{assigning to 'int aligned_type4::*' from incompatible type '__unaligned int aligned_type4::*'}}
+  p2_aligned_type4 = p1_aligned_type4;
+  // expected-warning@-1 {{implicit cast from type '__unaligned int aligned_type4::*' to type 'int aligned_type4::*' drops __unaligned qualifier}}
   p3_aligned_type4 = p1_aligned_type4;
 
   __unaligned int a[10];
-  int *b = a; // expected-error {{cannot initialize a variable of type 'int *' with an lvalue of type '__unaligned int[10]'}}
+  int *b = a;
+  // expected-warning@-1 {{implicit cast from type '__unaligned int[10]' to type 'int *' drops __unaligned qualifier}}
 }
 
 // Test from PR27367
@@ -115,13 +119,18 @@
 // We should accept type conversion of __unaligned to non-__unaligned references
 typedef struct in_addr {
 public:
-  in_addr(in_addr &a) {} // expected-note {{candidate constructor not viable: no known conversion from '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to 'in_addr &' for 1st argument; dereference the argument with *}}
-  in_addr(in_addr *a) {} // expected-note {{candidate constructor not viable: 1st argument ('__unaligned IN_ADDR *' (aka '__unaligned in_addr *')) would lose __unaligned qualifier}}
+  in_addr(in_addr &a) {} // expected-note {{candidate constructor not viable: expects an lvalue for 1st argument}}
+  in_addr(in_addr *a) {} // expected-note {{candidate constructor not viable: no known conversion from 'IN_ADDR' (aka 'in_addr') to 'in_addr *' for 1st argument}}
 } IN_ADDR;
 
 void f(IN_ADDR __unaligned *a) {
   IN_ADDR local_addr = *a;
-  IN_ADDR local_addr2 = a; // expected-error {{no viable conversion from '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to 'IN_ADDR' (aka 'in_addr')}}
+  // FIXME: MSVC accepts the following; not sure why clang tries to
+  // copy-construct an in_addr.
+  IN_ADDR local_addr2 = a; // expected-error {{no viable constructor copying variable of type 'IN_ADDR' (aka 'in_addr')}}
+  // expected-warning@-1 {{implicit cast from type '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to type 'in_addr *' drops __unaligned qualifier}}
+  IN_ADDR local_addr3(a);
+  // expected-warning@-1 {{implicit cast from type '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to type 'in_addr *' drops __unaligned qualifier}}
 }
 
 template void h1(T (__stdcall M::* const )()) { }
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -3195,9 +3195,8 @@
   Qualifiers FromQuals = FromType.getQualifiers();
   Qualifiers ToQuals = ToType.getQualifiers();
 
-  // Ignore __unaligned qualifier if this type is void.
-  if (ToType.getUnqualifiedType()->isVoidType())
-FromQuals.removeUnaligned();
+  // Ignore __unaligned qualifier.
+  FromQuals.removeUnaligned();
 
   // Objective-C ARC:
   //   Check Objective-C lifetime conversions.
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -4641,6 +4641,13 @@
 From->getType()->getPointeeType().getAddressSpace())
   CK = CK_AddressSpaceConversion;
 
+if (!isCast(CCK) &&
+!ToType->getPointeeType().getQualifiers().hasUnaligned() &&
+From->getType()->getPointeeType().getQualifiers().hasUnaligned()) {
+  Diag(From->getBeginLoc(), diag::warn_imp_cast_drops_unal

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120936

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

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

This looks fine to me, though we should wait for @rnk to decide whether the 
warning is sufficient for him.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120936

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

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

In D120936#3368566 , @rnk wrote:

> I wasn't able to find any history for why `void*` was special here.

As far as I can tell, nobody ever thought `void*` was special.  The check was 
an attempt to restrict dubious conversions while still remaining compatible 
with existing code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120936

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

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 413989.
efriedma added a comment.

Added warning.  This roughly matches the corresponding MSVC warning, as far as 
I can tell.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120936

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/MicrosoftExtensions.cpp

Index: clang/test/SemaCXX/MicrosoftExtensions.cpp
===
--- clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -85,18 +85,22 @@
   foo_unaligned(p2);
 
   __unaligned B_unaligned *p3 = 0;
-  int p4 = foo_unaligned(p3);
+  int p4 = foo_unaligned(p3); // expected-error {{cannot initialize a variable of type 'int' with an rvalue of type 'void *'}}
+  // expected-warning@-1 {{implicit cast from type '__unaligned B_unaligned *' to type 'B_unaligned *' drops __unaligned qualifier}}
 
-  B_unaligned *p5 = p3; // expected-error {{cannot initialize a variable of type 'B_unaligned *' with an lvalue of type '__unaligned B_unaligned *'}}
+  B_unaligned *p5 = p3;
+  // expected-warning@-1 {{implicit cast from type '__unaligned B_unaligned *' to type 'B_unaligned *' drops __unaligned qualifier}}
 
   __unaligned B_unaligned *p6 = p3;
 
   p1_aligned_type4 = p2_aligned_type4;
-  p2_aligned_type4 = p1_aligned_type4; // expected-error {{assigning to 'int aligned_type4::*' from incompatible type '__unaligned int aligned_type4::*'}}
+  p2_aligned_type4 = p1_aligned_type4;
+  // expected-warning@-1 {{implicit cast from type '__unaligned int aligned_type4::*' to type 'int aligned_type4::*' drops __unaligned qualifier}}
   p3_aligned_type4 = p1_aligned_type4;
 
   __unaligned int a[10];
-  int *b = a; // expected-error {{cannot initialize a variable of type 'int *' with an lvalue of type '__unaligned int[10]'}}
+  int *b = a;
+  // expected-warning@-1 {{implicit cast from type '__unaligned int[10]' to type 'int *' drops __unaligned qualifier}}
 }
 
 // Test from PR27367
@@ -115,13 +119,18 @@
 // We should accept type conversion of __unaligned to non-__unaligned references
 typedef struct in_addr {
 public:
-  in_addr(in_addr &a) {} // expected-note {{candidate constructor not viable: no known conversion from '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to 'in_addr &' for 1st argument; dereference the argument with *}}
-  in_addr(in_addr *a) {} // expected-note {{candidate constructor not viable: 1st argument ('__unaligned IN_ADDR *' (aka '__unaligned in_addr *')) would lose __unaligned qualifier}}
+  in_addr(in_addr &a) {} // expected-note {{candidate constructor not viable: expects an lvalue for 1st argument}}
+  in_addr(in_addr *a) {} // expected-note {{candidate constructor not viable: no known conversion from 'IN_ADDR' (aka 'in_addr') to 'in_addr *' for 1st argument}}
 } IN_ADDR;
 
 void f(IN_ADDR __unaligned *a) {
   IN_ADDR local_addr = *a;
-  IN_ADDR local_addr2 = a; // expected-error {{no viable conversion from '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to 'IN_ADDR' (aka 'in_addr')}}
+  // FIXME: MSVC accepts the following; not sure why clang tries to
+  // copy-construct an in_addr.
+  IN_ADDR local_addr2 = a; // expected-error {{no viable constructor copying variable of type 'IN_ADDR' (aka 'in_addr')}}
+  // expected-warning@-1 {{implicit cast from type '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to type 'in_addr *' drops __unaligned qualifier}}
+  IN_ADDR local_addr3(a);
+  // expected-warning@-1 {{implicit cast from type '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to type 'in_addr *' drops __unaligned qualifier}}
 }
 
 template void h1(T (__stdcall M::* const )()) { }
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -3195,9 +3195,8 @@
   Qualifiers FromQuals = FromType.getQualifiers();
   Qualifiers ToQuals = ToType.getQualifiers();
 
-  // Ignore __unaligned qualifier if this type is void.
-  if (ToType.getUnqualifiedType()->isVoidType())
-FromQuals.removeUnaligned();
+  // Ignore __unaligned qualifier.
+  FromQuals.removeUnaligned();
 
   // Objective-C ARC:
   //   Check Objective-C lifetime conversions.
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -4641,6 +4641,13 @@
 From->getType()->getPointeeType().getAddressSpace())
   CK = CK_AddressSpaceConversion;
 
+if (!isCast(CCK) &&
+!ToType->getPointeeType().getQualifiers().hasUnaligned() &&
+From->getType()->getPointeeType().getQualifiers().hasUnaligned()) {
+  Diag(From->getBeginLoc(), diag::warn_imp_cast_drops_unaligned)
+  << InitialFromTy

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I remember writing up feedback on this patch, but it's gone now, oh well.

I wasn't able to find any history for why `void*` was special here.

I see that MSVC doesn't warn when converting from `__unaligned int*` to `int*`, 
but that seems dangerous. Our team has an active feature request to tighten up 
clang warnings to make it harder to form unaligned pointers, so I'd like to 
insist that we warn when the `__unaligned` qualifier gets lost.




Comment at: clang/test/SemaCXX/MicrosoftExtensions.cpp:90
 
-  B_unaligned *p5 = p3; // expected-error {{cannot initialize a variable of 
type 'B_unaligned *' with an lvalue of type '__unaligned B_unaligned *'}}
+  B_unaligned *p5 = p3;
 

erichkeane wrote:
> MSVC is warning on this one, its a shame we don't have something similar.
I agree this is important, and I think it's a blocking issue. We don't want to 
allow new constructs, introduce a warning for them later, and then ask people 
to clean it up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120936

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

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I don't particularly understand this qualifier, is this something that perhaps 
@rnk  could take a look at?




Comment at: clang/test/SemaCXX/MicrosoftExtensions.cpp:88
   __unaligned B_unaligned *p3 = 0;
-  int p4 = foo_unaligned(p3);
+  int p4 = foo_unaligned(p3); // expected-error {{cannot initialize a variable 
of type 'int' with an rvalue of type 'void *'}}
 

Hmm... this one was quite suspicious.  In this case, it was choosing the 'int' 
returning overload, MSVC doesn't do that here, it seems to always choose the 
best match without the  __unaligned (as you're doing now).



Comment at: clang/test/SemaCXX/MicrosoftExtensions.cpp:90
 
-  B_unaligned *p5 = p3; // expected-error {{cannot initialize a variable of 
type 'B_unaligned *' with an lvalue of type '__unaligned B_unaligned *'}}
+  B_unaligned *p5 = p3;
 

MSVC is warning on this one, its a shame we don't have something similar.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120936

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

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

[Eli Friedman via Phabricator via cfe-commits]

efriedma created this revision.
efriedma added reviewers: rnk, mstorsjo, andreybokhanko, majnemer, 
aaron.ballman.
Herald added a project: All.
efriedma requested review of this revision.
Herald added a project: clang.

As far as I can tell, MSVC allows the relevant conversions for all pointer 
types.  Found compiling a Windows SDK header.

I've verified that the updated errors in MicrosoftExtensions.cpp match the ones 
that MSVC actually emits, except for the one with a FIXME.  (Not sure why this 
wasn't done for the patch that added the tests.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120936

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCXX/MicrosoftExtensions.cpp


Index: clang/test/SemaCXX/MicrosoftExtensions.cpp
===
--- clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -85,18 +85,18 @@
   foo_unaligned(p2);
 
   __unaligned B_unaligned *p3 = 0;
-  int p4 = foo_unaligned(p3);
+  int p4 = foo_unaligned(p3); // expected-error {{cannot initialize a variable 
of type 'int' with an rvalue of type 'void *'}}
 
-  B_unaligned *p5 = p3; // expected-error {{cannot initialize a variable of 
type 'B_unaligned *' with an lvalue of type '__unaligned B_unaligned *'}}
+  B_unaligned *p5 = p3;
 
   __unaligned B_unaligned *p6 = p3;
 
   p1_aligned_type4 = p2_aligned_type4;
-  p2_aligned_type4 = p1_aligned_type4; // expected-error {{assigning to 'int 
aligned_type4::*' from incompatible type '__unaligned int aligned_type4::*'}}
+  p2_aligned_type4 = p1_aligned_type4;
   p3_aligned_type4 = p1_aligned_type4;
 
   __unaligned int a[10];
-  int *b = a; // expected-error {{cannot initialize a variable of type 'int *' 
with an lvalue of type '__unaligned int[10]'}}
+  int *b = a;
 }
 
 // Test from PR27367
@@ -115,13 +115,15 @@
 // We should accept type conversion of __unaligned to non-__unaligned 
references
 typedef struct in_addr {
 public:
-  in_addr(in_addr &a) {} // expected-note {{candidate constructor not viable: 
no known conversion from '__unaligned IN_ADDR *' (aka '__unaligned in_addr *') 
to 'in_addr &' for 1st argument; dereference the argument with *}}
-  in_addr(in_addr *a) {} // expected-note {{candidate constructor not viable: 
1st argument ('__unaligned IN_ADDR *' (aka '__unaligned in_addr *')) would lose 
__unaligned qualifier}}
+  in_addr(in_addr &a) {} // expected-note {{candidate constructor not viable: 
expects an lvalue for 1st argument}}
+  in_addr(in_addr *a) {} // expected-note {{candidate constructor not viable: 
no known conversion from 'IN_ADDR' (aka 'in_addr') to 'in_addr *' for 1st 
argument}}
 } IN_ADDR;
 
 void f(IN_ADDR __unaligned *a) {
   IN_ADDR local_addr = *a;
-  IN_ADDR local_addr2 = a; // expected-error {{no viable conversion from 
'__unaligned IN_ADDR *' (aka '__unaligned in_addr *') to 'IN_ADDR' (aka 
'in_addr')}}
+  // FIXME: MSVC accepts the following; not sure why clang tries to
+  // copy-construct an in_addr.
+  IN_ADDR local_addr2 = a; // expected-error {{no viable constructor copying 
variable of type 'IN_ADDR' (aka 'in_addr')}}
 }
 
 template void h1(T (__stdcall M::* const )()) { }
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -3195,9 +3195,8 @@
   Qualifiers FromQuals = FromType.getQualifiers();
   Qualifiers ToQuals = ToType.getQualifiers();
 
-  // Ignore __unaligned qualifier if this type is void.
-  if (ToType.getUnqualifiedType()->isVoidType())
-FromQuals.removeUnaligned();
+  // Ignore __unaligned qualifier.
+  FromQuals.removeUnaligned();
 
   // Objective-C ARC:
   //   Check Objective-C lifetime conversions.


Index: clang/test/SemaCXX/MicrosoftExtensions.cpp
===
--- clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -85,18 +85,18 @@
   foo_unaligned(p2);
 
   __unaligned B_unaligned *p3 = 0;
-  int p4 = foo_unaligned(p3);
+  int p4 = foo_unaligned(p3); // expected-error {{cannot initialize a variable of type 'int' with an rvalue of type 'void *'}}
 
-  B_unaligned *p5 = p3; // expected-error {{cannot initialize a variable of type 'B_unaligned *' with an lvalue of type '__unaligned B_unaligned *'}}
+  B_unaligned *p5 = p3;
 
   __unaligned B_unaligned *p6 = p3;
 
   p1_aligned_type4 = p2_aligned_type4;
-  p2_aligned_type4 = p1_aligned_type4; // expected-error {{assigning to 'int aligned_type4::*' from incompatible type '__unaligned int aligned_type4::*'}}
+  p2_aligned_type4 = p1_aligned_type4;
   p3_aligned_type4 = p1_aligned_type4;
 
   __unaligned int a[10];
-  int *b = a; // expected-error {{cannot initialize a variable of type 'int *' with an lvalue of type '__unaligned int[10]'}}
+  int *b = a;
 }
 
 // Test from PR27367
@@ -115,13 +115,15 @@
 // We should accept type conv