[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: lib/CodeGen/CGBlocks.cpp:1276
+InitVar->setSection(".CRT$XCLa");
+CGM.addUsedGlobal(InitVar);
+  }

rjmccall wrote:
> Is the priority system not good enough?
My reading of the LLVM language reference leads me to believe it’s only ordered 
per-module. If that’s the case, the benefit of emitting into `XC*` is that it 
provides guaranteed order over all linker input.

`llvm.global_ctors` excerpt:

> The functions referenced by this array will be called in ascending order of 
> priority (i.e. lowest first) when the module is loaded.

Now if the priority system _is_ guaranteed over all linker input, will that 
guarantee hold for mixed Clang and CL objects?


Repository:
  rC Clang

https://reviews.llvm.org/D50144



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

[PATCH] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT accepted this revision.
DHowett-MSFT added a comment.
This revision is now accepted and ready to land.

This largely matches what we've done in the WinObjC clang patchset here 
,
 with the critical exception that we've chosen to mangle the EH names as though 
they were for structs named after their classes.

Is there some safe generalization that will apply well to all Objective-C 
runtimes, and that we could put in a common place?

Overall, the change seems reasonable to me, but my signoff cannot constitute 
strong-enough acceptance.


Repository:
  rC Clang

https://reviews.llvm.org/D47233



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

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT accepted this revision.
DHowett-MSFT added a comment.
This revision is now accepted and ready to land.

This looks good to me, but I don't have a strong understanding of "outlining."




Comment at: lib/CodeGen/CGBlocks.cpp:1262
+  if (IsWindows) {
+auto *Init = llvm::Function::Create(llvm::FunctionType::get(CGM.VoidTy,
+  {}), llvm::GlobalValue::InternalLinkage, ".block_isa_init",

Is there value in emitting a list of blocks that need to be initialized, then 
initializing them in one go in a COMDAT-foldable function?



Comment at: lib/CodeGen/CGObjCGNU.cpp:1522
+  GetClassVar(clsAlias.first) }, sectionName());
+// On ELF platforms, add a null value fore each special section so that we
+// can always guarantee that the _start and _stop symbols will exist and be

nit: `fore`



Comment at: lib/CodeGen/CGObjCRuntime.cpp:234
+if ((CPI = 
dyn_cast_or_null(Handler.Block->getFirstNonPHI( {
+CGF.CurrentFuncletPad = CPI;
+CPI->setOperand(2, CGF.getExceptionSlot().getPointer());

this region may need reformatting



Comment at: lib/CodeGen/CGObjCRuntime.cpp:274
 CGF.EmitBranchThroughCleanup(Cont);
-  }
+CGF.CurrentFuncletPad = nullptr;
+  }  

There's a scoped save/restore helper for this that'll offer better support for 
nested funclets.

```
SaveAndRestore 
RestoreCurrentFuncletPad(CGF.CurrentFuncletPad);
```


Repository:
  rC Clang

https://reviews.llvm.org/D50144



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

[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a subscriber: smeenai.
DHowett-MSFT added inline comments.



Comment at: include/clang/Driver/Options.td:1491
 // Objective-C ABI options.
-def fobjc_runtime_EQ : Joined<["-"], "fobjc-runtime=">, Group, 
Flags<[CC1Option]>,
+def fobjc_runtime_EQ : Joined<["-"], "fobjc-runtime=">, Group, 
Flags<[CC1Option, CoreOption]>,
   HelpText<"Specify the target Objective-C runtime kind and version">;

Is this so it's exposed to clang-cl?



Comment at: lib/AST/MicrosoftMangle.cpp:1886
   case BuiltinType::ObjCId:
-mangleArtificalTagType(TTK_Struct, "objc_object");
+mangleArtificalTagType(TTK_Struct, ".objc_object");
 break;

I'm interested in @smeenai's take on this, as well.


Repository:
  rC Clang

https://reviews.llvm.org/D50144



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

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT created this revision.
DHowett-MSFT added reviewers: theraven, rjmccall.
DHowett-MSFT added a project: clang.
Herald added a subscriber: cfe-commits.

This commit makes valid the following code:

  // objective-c++
  #define nil ((id)nullptr)
  ...
  void (^f)() = ^{};
  if (f == nil) {
  }
  …

Where it would previously fail with the error `invalid operands to binary 
expression ('void (^)()' and 'id')`.

Comparisons are now allowed between block types and `id`, `id`, 
`id`, and `NSObject*`. No other comparisons are changed.


Repository:
  rC Clang

https://reviews.llvm.org/D44580

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/objc_block_compare.mm


Index: test/Sema/objc_block_compare.mm
===
--- /dev/null
+++ test/Sema/objc_block_compare.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN: -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject 
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id qualifiedId;
+
+  id poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or 
a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary 
expression ('void (^)()' and 'id')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary 
expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary 
expression ('id' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary 
expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7745,8 +7745,8 @@
   return IntToBlockPointer;
 }
 
-// id -> T^
-if (getLangOpts().ObjC1 && RHSType->isObjCIdType()) {
+// id (or strictly compatible object type) -> T^
+if (getLangOpts().ObjC1 && 
RHSType->isBlockCompatibleObjCPointerType(Context)) {
   Kind = CK_AnyPointerToBlockPointerCast;
   return Compatible;
 }
@@ -10028,6 +10028,18 @@
 RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BitCast);
   return ResultTy;
 }
+
+if(!IsRelational &&
+   LHSType->isBlockPointerType() &&
+   RHSType->isBlockCompatibleObjCPointerType(Context)) {
+  LHS = ImpCastExprToType(LHS.get(), RHSType, 
CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+} else if(!IsRelational &&
+  LHSType->isBlockCompatibleObjCPointerType(Context) &&
+  RHSType->isBlockPointerType()) {
+  RHS = ImpCastExprToType(RHS.get(), LHSType, 
CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+}
   }
   if ((LHSType->isAnyPointerType() && RHSType->isIntegerType()) ||
   (LHSType->isIntegerType() && RHSType->isAnyPointerType())) {


Index: test/Sema/objc_block_compare.mm
===
--- /dev/null
+++ test/Sema/objc_block_compare.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN: -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject 
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id qualifiedId;
+
+  id poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary expression ('void (^)()' and 'id')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary expression ('id' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}
Index: lib/Sema/SemaExpr.cpp
===

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a subscriber: lebedev.ri.
DHowett-MSFT added a comment.

In https://reviews.llvm.org/D44580#1040540, @lebedev.ri wrote:

> Why not `test/SemaObjC/block_compare.mm` ?


I wasn't are that it existed. It may even be a good fit for 
`test/SemaObjC/block-type-safety.m`, which already exists.


Repository:
  rC Clang

https://reviews.llvm.org/D44580



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

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 138759.
DHowett-MSFT removed a subscriber: lebedev.ri.
DHowett-MSFT added a comment.

Moved files around slightly.


Repository:
  rC Clang

https://reviews.llvm.org/D44580

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaObjC/block-compare.mm


Index: test/SemaObjC/block-compare.mm
===
--- /dev/null
+++ test/SemaObjC/block-compare.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN: -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject 
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id qualifiedId;
+
+  id poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or 
a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary 
expression ('void (^)()' and 'id')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary 
expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary 
expression ('id' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary 
expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7745,8 +7745,8 @@
   return IntToBlockPointer;
 }
 
-// id -> T^
-if (getLangOpts().ObjC1 && RHSType->isObjCIdType()) {
+// id (or strictly compatible object type) -> T^
+if (getLangOpts().ObjC1 && 
RHSType->isBlockCompatibleObjCPointerType(Context)) {
   Kind = CK_AnyPointerToBlockPointerCast;
   return Compatible;
 }
@@ -10028,6 +10028,18 @@
 RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BitCast);
   return ResultTy;
 }
+
+if(!IsRelational &&
+   LHSType->isBlockPointerType() &&
+   RHSType->isBlockCompatibleObjCPointerType(Context)) {
+  LHS = ImpCastExprToType(LHS.get(), RHSType, 
CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+} else if(!IsRelational &&
+  LHSType->isBlockCompatibleObjCPointerType(Context) &&
+  RHSType->isBlockPointerType()) {
+  RHS = ImpCastExprToType(RHS.get(), LHSType, 
CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+}
   }
   if ((LHSType->isAnyPointerType() && RHSType->isIntegerType()) ||
   (LHSType->isIntegerType() && RHSType->isAnyPointerType())) {


Index: test/SemaObjC/block-compare.mm
===
--- /dev/null
+++ test/SemaObjC/block-compare.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN: -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject 
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id qualifiedId;
+
+  id poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary expression ('void (^)()' and 'id')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary expression ('id' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7745,8 +7745,8 @@
   return IntToBlockPointer;
 }
 
-// id -> T^
-if (getLangOpts().ObjC1 && RHSType->isObjCIdType()) {
+// id (or strictly compatible object type) -> T^
+if (getLangOpts().ObjC1 && RHSType->isBlockCompatibleObjCPointerType(Context)) {
   Kind = CK_AnyPointerToBlockPoint

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT planned changes to this revision.
DHowett-MSFT marked an inline comment as done.
DHowett-MSFT added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:7749
+// id (or strictly compatible object type) -> T^
+if (getLangOpts().ObjC1 && 
RHSType->isBlockCompatibleObjCPointerType(Context)) {
   Kind = CK_AnyPointerToBlockPointerCast;

theraven wrote:
> Do we want to allow implicit casts for all object types to block types for 
> assignment, or only for null pointers?  We definitely want to allow `nil` to 
> be assigned to a block type, but I would lean slightly to requiring an 
> implicit cast.
> 
> Ideally, I think we'd allow this but warn, because casting from an arbitrary 
> ObjC type to a block incorrectly can cause exciting security vulnerabilities 
> if it's done incorrectly and we should encourage people to check these casts 
> (`nil` is always safe though - as long as somewhere else checks the 
> nullability attributes).
I don't actually have a compelling use case for widening assignability here. 
I'm dropping this part of the patch.

I do think it should be valid, perhaps with a warning. It feels incorrect for 
there to be valid comparison cases that are not valid assignment cases (here), 
but that position doesn't hold up under scrutiny.


Repository:
  rC Clang

https://reviews.llvm.org/D44580



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

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 138868.
DHowett-MSFT marked an inline comment as done.
DHowett-MSFT added a comment.

Backed out changes to block type assignment.


Repository:
  rC Clang

https://reviews.llvm.org/D44580

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaObjC/block-compare.mm


Index: test/SemaObjC/block-compare.mm
===
--- /dev/null
+++ test/SemaObjC/block-compare.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN: -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject 
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id qualifiedId;
+
+  id poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or 
a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary 
expression ('void (^)()' and 'id')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary 
expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary 
expression ('id' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary 
expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10028,6 +10028,18 @@
 RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BitCast);
   return ResultTy;
 }
+
+if(!IsRelational &&
+   LHSType->isBlockPointerType() &&
+   RHSType->isBlockCompatibleObjCPointerType(Context)) {
+  LHS = ImpCastExprToType(LHS.get(), RHSType, 
CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+} else if(!IsRelational &&
+  LHSType->isBlockCompatibleObjCPointerType(Context) &&
+  RHSType->isBlockPointerType()) {
+  RHS = ImpCastExprToType(RHS.get(), LHSType, 
CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+}
   }
   if ((LHSType->isAnyPointerType() && RHSType->isIntegerType()) ||
   (LHSType->isIntegerType() && RHSType->isAnyPointerType())) {


Index: test/SemaObjC/block-compare.mm
===
--- /dev/null
+++ test/SemaObjC/block-compare.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN: -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject 
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id qualifiedId;
+
+  id poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary expression ('void (^)()' and 'id')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary expression ('id' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10028,6 +10028,18 @@
 RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BitCast);
   return ResultTy;
 }
+
+if(!IsRelational &&
+   LHSType->isBlockPointerType() &&
+   RHSType->isBlockCompatibleObjCPointerType(Context)) {
+  LHS = ImpCastExprToType(LHS.get(), RHSType, CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+} else if(!IsRelational &&
+  LHSType->isBlockCompatibleObjCPointerType(Context) &&
+  RHSType->isBlockPointerType()) {
+  RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+}
   }
   if ((LHSType->isAnyPointerTy

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT created this revision.
DHowett-MSFT added reviewers: rnk, majnemer.
DHowett-MSFT added a project: clang.
Herald added subscribers: cfe-commits, eraman.

The MSVC compiler rejects `__forceinline` on variadic functions with the 
following warning (at /https://reviews.llvm.org/W4):

  C4714: function 'void msvc_variadic(int,...)' marked as __forceinline not 
inlined

This fixes a bug in LLVM where a variadic was getting inlined into a function 
of calling convention x86_thiscallcc. The LLVM lowering passes cannot consume 
an `@llvm.va_start` intrinsic call in a thiscall function without emitting an 
assertion.

Inlining variadics should almost certainly be possible; however, this is a fix 
to bring Clang in line with MSVC.


Repository:
  rC Clang

https://reviews.llvm.org/D44646

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/ms-forceinline-on-variadic.cpp


Index: test/Sema/ms-forceinline-on-variadic.cpp
===
--- /dev/null
+++ test/Sema/ms-forceinline-on-variadic.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-windows -verify 
-fms-extensions %s \
+// RUN:| FileCheck %s
+//
+// (instruction scheduling crash test)
+// RUN: %clang_cc1 -S -o - -triple i686-windows -fms-extensions %s
+
+inline __attribute__((always_inline)) void attribute_variadic(int f, ...) { } 
// expected-warning {{inlining attribute 'always_inline' ignored on variadic 
function}}
+
+__forceinline inline void msvc_variadic(int f, ...) { // expected-warning 
{{inlining attribute '__forceinline' ignored on variadic function}}
+// CHECK-DAG: define {{.*}} void [[MSVC_VARIADIC:@".*msvc_variadic.*"]](
+__builtin_va_list ap;
+__builtin_va_start(ap, f);
+__builtin_va_end(ap);
+}
+
+struct a {
+// members are, by default, thiscall; enforce it for the purposes of the 
test
+void __thiscall dispatcher();
+};
+
+void __thiscall a::dispatcher() {
+msvc_variadic(1, 2, 3);
+// CHECK-DAG: define dso_local x86_thiscallcc void @"{{.*dispatcher.*}}"(
+// CHECK-DAG: call void {{.*}} [[MSVC_VARIADIC]]
+}
+
+void t() {
+a{}.dispatcher();
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3886,6 +3886,12 @@
 return nullptr;
   }
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft()
+ && hasFunctionProto(D) && isFunctionOrMethodVariadic(D)) {
+Diag(Range.getBegin(), diag::warn_always_inline_on_variadic) << Ident;
+return nullptr;
+  }
+
   if (D->hasAttr())
 return nullptr;
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2694,6 +2694,9 @@
 def warn_attribute_after_definition_ignored : Warning<
   "attribute %0 after definition is ignored">,
InGroup;
+def warn_always_inline_on_variadic : Warning<
+  "inlining attribute %0 ignored on variadic function">,
+   InGroup;
 def warn_unknown_attribute_ignored : Warning<
   "unknown attribute %0 ignored">, InGroup;
 def warn_cxx11_gnu_attribute_on_type : Warning<


Index: test/Sema/ms-forceinline-on-variadic.cpp
===
--- /dev/null
+++ test/Sema/ms-forceinline-on-variadic.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-windows -verify -fms-extensions %s \
+// RUN:| FileCheck %s
+//
+// (instruction scheduling crash test)
+// RUN: %clang_cc1 -S -o - -triple i686-windows -fms-extensions %s
+
+inline __attribute__((always_inline)) void attribute_variadic(int f, ...) { } // expected-warning {{inlining attribute 'always_inline' ignored on variadic function}}
+
+__forceinline inline void msvc_variadic(int f, ...) { // expected-warning {{inlining attribute '__forceinline' ignored on variadic function}}
+// CHECK-DAG: define {{.*}} void [[MSVC_VARIADIC:@".*msvc_variadic.*"]](
+__builtin_va_list ap;
+__builtin_va_start(ap, f);
+__builtin_va_end(ap);
+}
+
+struct a {
+// members are, by default, thiscall; enforce it for the purposes of the test
+void __thiscall dispatcher();
+};
+
+void __thiscall a::dispatcher() {
+msvc_variadic(1, 2, 3);
+// CHECK-DAG: define dso_local x86_thiscallcc void @"{{.*dispatcher.*}}"(
+// CHECK-DAG: call void {{.*}} [[MSVC_VARIADIC]]
+}
+
+void t() {
+a{}.dispatcher();
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3886,6 +3886,12 @@
 return nullptr;
   }
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft()
+ && hasFunctionProto(D) && isFunctionOrMethodVariadic(D)) {
+Diag(Range.getBegin(), diag::warn_always_inline_on_variadic) << Ident;
+return nullptr

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

Apologies if I've chosen the wrong set of reviewers.


Repository:
  rC Clang

https://reviews.llvm.org/D44646



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

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 138995.
DHowett-MSFT added a comment.

Fixed formatting.


Repository:
  rC Clang

https://reviews.llvm.org/D44646

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/ms-forceinline-on-variadic.cpp


Index: test/Sema/ms-forceinline-on-variadic.cpp
===
--- /dev/null
+++ test/Sema/ms-forceinline-on-variadic.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-windows -verify 
-fms-extensions %s \
+// RUN:| FileCheck %s
+//
+// (instruction scheduling crash test)
+// RUN: %clang_cc1 -S -o - -triple i686-windows -fms-extensions %s
+
+inline __attribute__((always_inline)) void attribute_variadic(int f, ...) { } 
// expected-warning {{inlining attribute 'always_inline' ignored on variadic 
function}}
+
+__forceinline inline void msvc_variadic(int f, ...) { // expected-warning 
{{inlining attribute '__forceinline' ignored on variadic function}}
+// CHECK-DAG: define {{.*}} void [[MSVC_VARIADIC:@".*msvc_variadic.*"]](
+__builtin_va_list ap;
+__builtin_va_start(ap, f);
+__builtin_va_end(ap);
+}
+
+struct a {
+// members are, by default, thiscall; enforce it for the purposes of the 
test
+void __thiscall dispatcher();
+};
+
+void __thiscall a::dispatcher() {
+msvc_variadic(1, 2, 3);
+// CHECK-DAG: define dso_local x86_thiscallcc void @"{{.*dispatcher.*}}"(
+// CHECK-DAG: call void {{.*}} [[MSVC_VARIADIC]]
+}
+
+void t() {
+a{}.dispatcher();
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3886,6 +3886,12 @@
 return nullptr;
   }
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+  hasFunctionProto(D) && isFunctionOrMethodVariadic(D)) {
+Diag(Range.getBegin(), diag::warn_always_inline_on_variadic) << Ident;
+return nullptr;
+  }
+
   if (D->hasAttr())
 return nullptr;
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2694,6 +2694,9 @@
 def warn_attribute_after_definition_ignored : Warning<
   "attribute %0 after definition is ignored">,
InGroup;
+def warn_always_inline_on_variadic : Warning<
+  "inlining attribute %0 ignored on variadic function">,
+   InGroup;
 def warn_unknown_attribute_ignored : Warning<
   "unknown attribute %0 ignored">, InGroup;
 def warn_cxx11_gnu_attribute_on_type : Warning<


Index: test/Sema/ms-forceinline-on-variadic.cpp
===
--- /dev/null
+++ test/Sema/ms-forceinline-on-variadic.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-windows -verify -fms-extensions %s \
+// RUN:| FileCheck %s
+//
+// (instruction scheduling crash test)
+// RUN: %clang_cc1 -S -o - -triple i686-windows -fms-extensions %s
+
+inline __attribute__((always_inline)) void attribute_variadic(int f, ...) { } // expected-warning {{inlining attribute 'always_inline' ignored on variadic function}}
+
+__forceinline inline void msvc_variadic(int f, ...) { // expected-warning {{inlining attribute '__forceinline' ignored on variadic function}}
+// CHECK-DAG: define {{.*}} void [[MSVC_VARIADIC:@".*msvc_variadic.*"]](
+__builtin_va_list ap;
+__builtin_va_start(ap, f);
+__builtin_va_end(ap);
+}
+
+struct a {
+// members are, by default, thiscall; enforce it for the purposes of the test
+void __thiscall dispatcher();
+};
+
+void __thiscall a::dispatcher() {
+msvc_variadic(1, 2, 3);
+// CHECK-DAG: define dso_local x86_thiscallcc void @"{{.*dispatcher.*}}"(
+// CHECK-DAG: call void {{.*}} [[MSVC_VARIADIC]]
+}
+
+void t() {
+a{}.dispatcher();
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3886,6 +3886,12 @@
 return nullptr;
   }
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+  hasFunctionProto(D) && isFunctionOrMethodVariadic(D)) {
+Diag(Range.getBegin(), diag::warn_always_inline_on_variadic) << Ident;
+return nullptr;
+  }
+
   if (D->hasAttr())
 return nullptr;
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2694,6 +2694,9 @@
 def warn_attribute_after_definition_ignored : Warning<
   "attribute %0 after definition is ignored">,
InGroup;
+def warn_always_inline_on_variadic : Warning<
+  "inlining attribute %0 ignored on variadic function">,
+   InGroup;
 def warn_unknown_attribute_ignored : Warning<
   "unknown attribute %0 ignored">, InGroup;
 def warn_cxx11_gnu_attribu

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 138996.
DHowett-MSFT added a comment.

Ran `clang-format` over changed lines. Reuploaded diff with full context.


Repository:
  rC Clang

https://reviews.llvm.org/D44580

Files:
  lib/Sema/SemaExpr.cpp
  test/SemaObjC/block-compare.mm


Index: test/SemaObjC/block-compare.mm
===
--- /dev/null
+++ test/SemaObjC/block-compare.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN: -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject 
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id qualifiedId;
+
+  id poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or 
a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary 
expression ('void (^)()' and 'id')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary 
expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary 
expression ('id' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary 
expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10028,6 +10028,19 @@
 RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BitCast);
   return ResultTy;
 }
+
+if (!IsRelational && LHSType->isBlockPointerType() &&
+RHSType->isBlockCompatibleObjCPointerType(Context)) {
+  LHS = ImpCastExprToType(LHS.get(), RHSType,
+  CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+} else if (!IsRelational &&
+   LHSType->isBlockCompatibleObjCPointerType(Context) &&
+   RHSType->isBlockPointerType()) {
+  RHS = ImpCastExprToType(RHS.get(), LHSType,
+  CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+}
   }
   if ((LHSType->isAnyPointerType() && RHSType->isIntegerType()) ||
   (LHSType->isIntegerType() && RHSType->isAnyPointerType())) {


Index: test/SemaObjC/block-compare.mm
===
--- /dev/null
+++ test/SemaObjC/block-compare.mm
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -S -o - -triple i686-windows -verify -fblocks \
+// RUN: -Wno-unused-comparison %s
+
+#pragma clang diagnostic ignored "-Wunused-comparison"
+
+#define nil ((id)nullptr)
+
+@protocol NSObject
+@end
+
+@protocol NSCopying
+@end
+
+@protocol OtherProtocol
+@end
+
+__attribute__((objc_root_class))
+@interface NSObject 
+@end
+
+__attribute__((objc_root_class))
+@interface Test
+@end
+
+int main() {
+  void (^block)() = ^{};
+  NSObject *object;
+  id qualifiedId;
+
+  id poorlyQualified1;
+  Test *objectOfWrongType;
+
+  block == nil;
+  block == object;
+  block == qualifiedId;
+
+  nil == block;
+  object == block;
+  qualifiedId == block;
+
+  // these are still not valid: blocks must be compared with id, NSObject*, or a protocol-qualified id
+  // conforming to NSCopying or NSObject.
+
+  block == poorlyQualified1; // expected-error {{invalid operands to binary expression ('void (^)()' and 'id')}}
+  block == objectOfWrongType; // expected-error {{invalid operands to binary expression ('void (^)()' and 'Test *')}}
+
+  poorlyQualified1 == block; // expected-error {{invalid operands to binary expression ('id' and 'void (^)()')}}
+  objectOfWrongType == block; // expected-error {{invalid operands to binary expression ('Test *' and 'void (^)()')}}
+
+  return 0;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10028,6 +10028,19 @@
 RHS = ImpCastExprToType(RHS.get(), LHSType, CK_BitCast);
   return ResultTy;
 }
+
+if (!IsRelational && LHSType->isBlockPointerType() &&
+RHSType->isBlockCompatibleObjCPointerType(Context)) {
+  LHS = ImpCastExprToType(LHS.get(), RHSType,
+  CK_BlockPointerToObjCPointerCast);
+  return ResultTy;
+} else if (!IsRelational &&
+   LHSType->isBlockCompatibleObjCPointerType(Context) &&
+   RHSType->isBlockPointerType()) {
+  RHS = ImpCastExprToType(RHS.get(), LHSType,
+  CK_B

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

In https://reviews.llvm.org/D44646#1042347, @efriedma wrote:

> The compiler shouldn't inline functions which call va_start, whether or not 
> they're marked always_inline.  That should work correctly, I think, at least 
> on trunk.  (See https://reviews.llvm.org/D42556 .)
>
> If you want to warn anyway, that's okay.


Interesting. I certainly hit the lowering/instruction scheduling crash after 
that change landed. Perhaps it's a valuable thing for me to file a bug on.


Repository:
  rC Clang

https://reviews.llvm.org/D44646



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

[PATCH] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

In https://reviews.llvm.org/D47233#1129207, @rjmccall wrote:

> In https://reviews.llvm.org/D47233#1129110, @smeenai wrote:
>
> > WinObjC does this wrapping, for example.
>
>
> I see.  The idea of creating the type descriptors and mangled names ad-hoc 
> for the catchable-types array is clever, and it's nice that the ABI is 
> defined in a way that makes that work.  (Expensive, but hey, it's the 
> exceptions path.)


We ran into some critical issues with this approach on x64. The pointers in the 
exception record are supposed to be image-relative instead of absolute, so I 
tried to make them absolute to libobjc2's load address, but I never quite 
solved it.

A slightly better-documented and cleaner version of the code you linked is here 
.


Repository:
  rC Clang

https://reviews.llvm.org/D47233



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

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

This looks great, and takes up the parts of my patch that I cared about. Thank 
you for doing this.
My primary concern is that the patch includes my "early init" changes -- while 
I support it and think it's the right solution, especially where it reduces 
double indirection on class pointers, there may be issues left to iron out.




Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188
 
+  StringRef SymbolPrefix() {
+return CGM.getTriple().isOSBinFormatCOFF() ? "_" : "._";

Should this be `SymbolPrefix()` or something more like `MangleSymbol(StringRef 
sym)`? I know that right now we only need to prepend `_` or `._`, but is it 
more future-proof to make it generic?



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:1528
 InitStructBuilder.addInt(Int64Ty, 0);
-for (auto *s : SectionsBaseNames) {
-  auto bounds = GetSectionBounds(s);
-  InitStructBuilder.add(bounds.first);
-  InitStructBuilder.add(bounds.second);
-};
+if (CGM.getTriple().isOSBinFormatCOFF()) {
+  for (auto *s : PECOFFSectionsBaseNames) {

We may be able to reduce the duplication here (aside: it is very cool that 
Phabricator shows duplicated lines) by capturing `auto sectionVec = 
&SectionsBaseNames;` and switching which is in use based on bin format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58724



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

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188
 
+  StringRef SymbolPrefix() {
+return CGM.getTriple().isOSBinFormatCOFF() ? "_" : "._";

theraven wrote:
> DHowett-MSFT wrote:
> > Should this be `SymbolPrefix()` or something more like 
> > `MangleSymbol(StringRef sym)`? I know that right now we only need to 
> > prepend `_` or `._`, but is it more future-proof to make it generic?
> I have refactored this, and then tried adding a $ instead of the . for 
> mangling.  On further testing, the latest link.exe from VS 2017 no longer 
> complains about symbols starting with a dot, so I'm inclined to revert that 
> part of it entirely (lld-link.exe was always happy).  I'd prefer to minimise 
> differences between COFF and ELF and this means that we have different 
> section names, but aside from needing the extra global initialisation on 
> COFF, everything else is the same.  
The issue I had with symbols starting with `.` was in `.DEF` files specifically.

Linking a shared object containing:
```
@.exp_with_dot = dllexport global i32 0, align 4
```

without a def file, yields:

```
ordinal hint RVA  name

  10 00012900 .exp_with_dot
```

However, linking the same library with a definition file:

###test.def
```
EXPORTS
 .exp_with_dot
```

yields

###output
```
test.def : fatal error LNK1242: '.exp_with_dot' is an invalid export symbol name
```

This still reproduces with 15.9.8, sadly. LINK version 14.16.27027.1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58724



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

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188
 
+  StringRef SymbolPrefix() {
+return CGM.getTriple().isOSBinFormatCOFF() ? "_" : "._";

theraven wrote:
> DHowett-MSFT wrote:
> > theraven wrote:
> > > DHowett-MSFT wrote:
> > > > Should this be `SymbolPrefix()` or something more like 
> > > > `MangleSymbol(StringRef sym)`? I know that right now we only need to 
> > > > prepend `_` or `._`, but is it more future-proof to make it generic?
> > > I have refactored this, and then tried adding a $ instead of the . for 
> > > mangling.  On further testing, the latest link.exe from VS 2017 no longer 
> > > complains about symbols starting with a dot, so I'm inclined to revert 
> > > that part of it entirely (lld-link.exe was always happy).  I'd prefer to 
> > > minimise differences between COFF and ELF and this means that we have 
> > > different section names, but aside from needing the extra global 
> > > initialisation on COFF, everything else is the same.  
> > The issue I had with symbols starting with `.` was in `.DEF` files 
> > specifically.
> > 
> > Linking a shared object containing:
> > ```
> > @.exp_with_dot = dllexport global i32 0, align 4
> > ```
> > 
> > without a def file, yields:
> > 
> > ```
> > ordinal hint RVA  name
> > 
> >   10 00012900 .exp_with_dot
> > ```
> > 
> > However, linking the same library with a definition file:
> > 
> > ###test.def
> > ```
> > EXPORTS
> >  .exp_with_dot
> > ```
> > 
> > yields
> > 
> > ###output
> > ```
> > test.def : fatal error LNK1242: '.exp_with_dot' is an invalid export symbol 
> > name
> > ```
> > 
> > This still reproduces with 15.9.8, sadly. LINK version 14.16.27027.1.
> Does it work with a $ instead of a .?
Hey, `$` actually works!

```
EXPORTS
 $exp_with_dollar
```

```
ordinal hint RVA  name

  10 00012900 $exp_with_dollar
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58724



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

[PATCH] D58807: [CodeGen] COMDAT-fold block descriptors

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT created this revision.
DHowett-MSFT added a project: clang.
Herald added a subscriber: cfe-commits.

Without this change, linking multiple objects containing block
descriptors together on Windows will generate duplicate symbol errors.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58807

Files:
  clang/lib/CodeGen/CGBlocks.cpp


Index: clang/lib/CodeGen/CGBlocks.cpp
===
--- clang/lib/CodeGen/CGBlocks.cpp
+++ clang/lib/CodeGen/CGBlocks.cpp
@@ -274,6 +274,8 @@
  /*constant*/ true, linkage, AddrSpace);
 
   if (linkage == llvm::GlobalValue::LinkOnceODRLinkage) {
+if (CGM.supportsCOMDAT())
+  global->setComdat(CGM.getModule().getOrInsertComdat(descName));
 global->setVisibility(llvm::GlobalValue::HiddenVisibility);
 global->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
   }


Index: clang/lib/CodeGen/CGBlocks.cpp
===
--- clang/lib/CodeGen/CGBlocks.cpp
+++ clang/lib/CodeGen/CGBlocks.cpp
@@ -274,6 +274,8 @@
  /*constant*/ true, linkage, AddrSpace);
 
   if (linkage == llvm::GlobalValue::LinkOnceODRLinkage) {
+if (CGM.supportsCOMDAT())
+  global->setComdat(CGM.getModule().getOrInsertComdat(descName));
 global->setVisibility(llvm::GlobalValue::HiddenVisibility);
 global->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:1231
+break;
+
+auto Storage = llvm::GlobalValue::DefaultStorageClass;

After we get the `ObjCInterfaceDecl`, we have to make sure it's the one 
corresponding to the `@interface` and not one for a forward declaration 
(`@class`).

```  
+// The first Interface we find may be a @class,
+// which should only be treated as the source of
+// truth in the absence of a true declaration.
+const ObjCInterfaceDecl *OIDDef = OID->getDefinition();
+if (OIDDef != nullptr)
+  OID = OIDDef;
+
```

Failure to do so can result in us either failing to import the `CLASS_REF` or, 
if it has been exported as `CONSTANT`, attempt a method dispatch against a 
pointer to our own import table.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58724



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

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188
 
+  Twine ManglePublicSymbol(StringRef Name) {
+return StringRef(CGM.getTriple().isOSBinFormatCOFF() ? "$_" : "._") + Name;

As of the latest revision, this now fails at runtime:

```
  0x01342976 (0x03D8D530 0x03D8DCA0 0x04045A08 0x04045A08), llvm::Twine::str() 
+ 0x166 bytes(s), e:\src\llvm\lib\suppor
  t\twine.cpp, line 29 + 0x5F byte(s)
  0x01664F99 (0x03D8D5C4 0x000A 0x 0x03D8DCA0), `anonymous 
namespace'::CGObjCGNUstep2::GetClassVar() + 0xB9
   bytes(s), e:\src\llvm\tools\clang\lib\codegen\cgobjcgnu.cpp, line 1207 + 
0x10 byte(s)
```

I believe we're running afoul of StringRef's lifetime here. I haven't had a 
chance to dig in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58724



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

[PATCH] D58724: [gnustep-objc] Make the GNUstep v2 ABI work for Windows DLLs.

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:188
 
+  Twine ManglePublicSymbol(StringRef Name) {
+return StringRef(CGM.getTriple().isOSBinFormatCOFF() ? "$_" : "._") + Name;

DHowett-MSFT wrote:
> As of the latest revision, this now fails at runtime:
> 
> ```
>   0x01342976 (0x03D8D530 0x03D8DCA0 0x04045A08 0x04045A08), 
> llvm::Twine::str() + 0x166 bytes(s), e:\src\llvm\lib\suppor
>   t\twine.cpp, line 29 + 0x5F byte(s)
>   0x01664F99 (0x03D8D5C4 0x000A 0x 0x03D8DCA0), `anonymous 
> namespace'::CGObjCGNUstep2::GetClassVar() + 0xB9
>bytes(s), e:\src\llvm\tools\clang\lib\codegen\cgobjcgnu.cpp, line 1207 + 
> 0x10 byte(s)
> ```
> 
> I believe we're running afoul of StringRef's lifetime here. I haven't had a 
> chance to dig in.
Alright, I don't completely understand why Twine is the way that it is, but 
here:

```
  Twine ManglePublicSymbol(StringRef Name)
```

When we construct `Twine(const char*, StringRef)`, the newly-minted Twine 
contains a _pointer to_ the passed-in StringRef. It's invalid immediately after 
`ManglePublicSymbol` returns. After a few layers of stack pop off, we end up 
with random garbage and undefined behavior.

A quick and effective fix is to switch `Name` to be of type `const Twine&`.

```
  Twine ManglePublicSymbol(const Twine& Name)
```

Name ends up being a twine rvalue with a LHSType of cString, and all is right 
in the world.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58724



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

[PATCH] D58807: [CodeGen] COMDAT-fold block descriptors

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 189183.
DHowett-MSFT added a comment.

This change caused a test to fail, so I took the opportunity to augment the 
test with COMDAT entry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58807

Files:
  clang/lib/CodeGen/CGBlocks.cpp
  test/CodeGenObjC/block-desc-str.m


Index: test/CodeGenObjC/block-desc-str.m
===
--- test/CodeGenObjC/block-desc-str.m
+++ test/CodeGenObjC/block-desc-str.m
@@ -1,9 +1,13 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm 
-fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc 
-fblocks -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm 
-fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck 
--check-prefix=CHECK-COMDAT %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc 
-fblocks -o - %s | FileCheck --check-prefix=CHECK-COMDAT %s
 // RUN: %clang_cc1 -triple=x86_64-apple-darwin10 -emit-llvm -fblocks -o - %s | 
FileCheck %s
 
 // Test that descriptor symbol names don't include '@'.
 
+// CHECK-COMDAT: $"__block_descriptor_40_8_32o_e5_v8\01?0l" = comdat any
+// CHECK-COMDAT: @[[STR:.*]] = private unnamed_addr constant [6 x i8] 
c"v8@?0\00"
+// CHECK-COMDAT: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr 
hidden unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 
40, i8* bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr 
inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, comdat, align 8
+
 // CHECK: @[[STR:.*]] = private unnamed_addr constant [6 x i8] c"v8@?0\00"
 // CHECK: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr hidden 
unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 40, i8* 
bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr 
inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, align 8
 
Index: clang/lib/CodeGen/CGBlocks.cpp
===
--- clang/lib/CodeGen/CGBlocks.cpp
+++ clang/lib/CodeGen/CGBlocks.cpp
@@ -274,6 +274,8 @@
  /*constant*/ true, linkage, AddrSpace);
 
   if (linkage == llvm::GlobalValue::LinkOnceODRLinkage) {
+if (CGM.supportsCOMDAT())
+  global->setComdat(CGM.getModule().getOrInsertComdat(descName));
 global->setVisibility(llvm::GlobalValue::HiddenVisibility);
 global->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
   }


Index: test/CodeGenObjC/block-desc-str.m
===
--- test/CodeGenObjC/block-desc-str.m
+++ test/CodeGenObjC/block-desc-str.m
@@ -1,9 +1,13 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm -fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc -fblocks -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm -fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck --check-prefix=CHECK-COMDAT %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc -fblocks -o - %s | FileCheck --check-prefix=CHECK-COMDAT %s
 // RUN: %clang_cc1 -triple=x86_64-apple-darwin10 -emit-llvm -fblocks -o - %s | FileCheck %s
 
 // Test that descriptor symbol names don't include '@'.
 
+// CHECK-COMDAT: $"__block_descriptor_40_8_32o_e5_v8\01?0l" = comdat any
+// CHECK-COMDAT: @[[STR:.*]] = private unnamed_addr constant [6 x i8] c"v8@?0\00"
+// CHECK-COMDAT: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr hidden unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 40, i8* bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, comdat, align 8
+
 // CHECK: @[[STR:.*]] = private unnamed_addr constant [6 x i8] c"v8@?0\00"
 // CHECK: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr hidden unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 40, i8* bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, align 8
 
Index: clang/lib/CodeGen/CGBlocks.cpp
===
--- clang/lib/CodeGen/CGBlocks.cpp
+++ clang/lib/CodeGen/CGBlocks.cpp
@@ -274,6 +274,8 @@
  /*constant*/ true, linkage, AddrSpace);
 
   if (linkage == llvm::GlobalValue::LinkOnceODRLinkage) {
+if (CGM.supportsCOMDAT())
+  global->setComdat(CGM.getModule().getOrInsertComdat(descName));
 global->setVisibility(llvm::GlobalValue::HiddenVisibility);
 global->setUnnamedAddr(llvm::GlobalValue::Unname

[PATCH] D58807: [CodeGen] COMDAT-fold block descriptors

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 189193.
DHowett-MSFT added a comment.

Fixed corrupted patch. I know that the -str test case is not the _ideal_ 
location for the comdat test, but it is the only test whose invariants failed 
(the IR for the descriptor definition now contains `comdat, align 4`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58807

Files:
  clang/lib/CodeGen/CGBlocks.cpp
  clang/test/CodeGenObjC/block-desc-str.m


Index: clang/test/CodeGenObjC/block-desc-str.m
===
--- clang/test/CodeGenObjC/block-desc-str.m
+++ clang/test/CodeGenObjC/block-desc-str.m
@@ -1,9 +1,13 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm 
-fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc 
-fblocks -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm 
-fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck 
--check-prefix=CHECK-COMDAT %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc 
-fblocks -o - %s | FileCheck --check-prefix=CHECK-COMDAT %s
 // RUN: %clang_cc1 -triple=x86_64-apple-darwin10 -emit-llvm -fblocks -o - %s | 
FileCheck %s
 
 // Test that descriptor symbol names don't include '@'.
 
+// CHECK-COMDAT: $"__block_descriptor_40_8_32o_e5_v8\01?0l" = comdat any
+// CHECK-COMDAT: @[[STR:.*]] = private unnamed_addr constant [6 x i8] 
c"v8@?0\00"
+// CHECK-COMDAT: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr 
hidden unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 
40, i8* bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr 
inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, comdat, align 8
+
 // CHECK: @[[STR:.*]] = private unnamed_addr constant [6 x i8] c"v8@?0\00"
 // CHECK: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr hidden 
unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 40, i8* 
bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr 
inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, align 8
 
Index: clang/lib/CodeGen/CGBlocks.cpp
===
--- clang/lib/CodeGen/CGBlocks.cpp
+++ clang/lib/CodeGen/CGBlocks.cpp
@@ -274,6 +274,8 @@
  /*constant*/ true, linkage, AddrSpace);
 
   if (linkage == llvm::GlobalValue::LinkOnceODRLinkage) {
+if (CGM.supportsCOMDAT())
+  global->setComdat(CGM.getModule().getOrInsertComdat(descName));
 global->setVisibility(llvm::GlobalValue::HiddenVisibility);
 global->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
   }


Index: clang/test/CodeGenObjC/block-desc-str.m
===
--- clang/test/CodeGenObjC/block-desc-str.m
+++ clang/test/CodeGenObjC/block-desc-str.m
@@ -1,9 +1,13 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm -fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc -fblocks -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm -fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck --check-prefix=CHECK-COMDAT %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc -fblocks -o - %s | FileCheck --check-prefix=CHECK-COMDAT %s
 // RUN: %clang_cc1 -triple=x86_64-apple-darwin10 -emit-llvm -fblocks -o - %s | FileCheck %s
 
 // Test that descriptor symbol names don't include '@'.
 
+// CHECK-COMDAT: $"__block_descriptor_40_8_32o_e5_v8\01?0l" = comdat any
+// CHECK-COMDAT: @[[STR:.*]] = private unnamed_addr constant [6 x i8] c"v8@?0\00"
+// CHECK-COMDAT: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr hidden unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 40, i8* bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, comdat, align 8
+
 // CHECK: @[[STR:.*]] = private unnamed_addr constant [6 x i8] c"v8@?0\00"
 // CHECK: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr hidden unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 40, i8* bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, align 8
 
Index: clang/lib/CodeGen/CGBlocks.cpp
===
--- clang/lib/CodeGen/CGBlocks.cpp
+++ clang/lib/CodeGen/CGBlocks.cpp
@@ -274,6 +274,8 @@
  /*constant*/ true, linkage, AddrSpace);
 
   if (linkage == llvm::GlobalValue::LinkOnceODRLinkage) {
+if (CGM.supportsCOMDAT())
+  global->setComdat

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

Hi! Is there anything else holding up this patch? Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D45305



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

[PATCH] D46052: GNUstep Objective-C ABI version 2

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: lib/CodeGen/CGObjCGNU.cpp:439
+ ArrayRef IvarOffsets,
+ ArrayRef IvarAlign,
+ ArrayRef IvarOwnership);

While we're here, is there value in storing the ivar size in layout as well, so 
that the runtime doesn't need to calculate it from the distance to the next 
ivar/end of the instance?



Comment at: lib/CodeGen/CGObjCGNU.cpp:1402
+Stop->setVisibility(llvm::GlobalValue::HiddenVisibility);
+return { Start, Stop };
+  }

This should be readily expandable for PE/COFF, but we'll need to change some of 
the section names to fit better. Is it worth abstracting the names of the 
sections across the target format somehow?

(pe/coff will need to emit COMDAT symbols pointing into lexicographically 
sortable section names that the linker can fold away)



Comment at: lib/CodeGen/CGObjCGNU.cpp:1446
+/*isConstant*/true, llvm::GlobalValue::LinkOnceAnyLinkage,
+LoadFunction, ".objc_ctor");
+// Check that this hasn't been renamed.  This shouldn't happen, because

Is there a way to ensure that objc_load happens before other static 
initializers across the entire set of linker inputs?


Repository:
  rC Clang

https://reviews.llvm.org/D46052



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

[PATCH] D138453: [clang] Add serialization for loop hint annotation tokens

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

In D138453#4105209 , @shafik wrote:

> It looks like https://github.com/llvm/llvm-project/issues/60543 is hitting 
> your added `llvm_unreachable("missing serialization code for annotation 
> token");` is this expected.

I believe it's expected that we hit this assertion - it's clearer than the 
original message, and more directly indicates that `#pragma pack` cannot be 
serialized.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138453

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

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT created this revision.
Herald added a project: All.
DHowett-MSFT updated this revision to Diff 495195.
DHowett-MSFT added a comment.
DHowett-MSFT published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Add a test


DHowett-MSFT added a comment.

This is a fix for https://github.com/llvm/llvm-project/issues/60543.

I originally approached this fix by moving `PragmaPackOptions` up to 
`Lex/Token.h` to live near `PragmaLoopHintInfo`; however, that required moving 
much more of the `PragmaMsStack` machinery than I was comfortable with.

There is precedent for some pragma infos living in `Sema` as well as 
`ASTReader`/`ASTWriter` poking into `Sema` to do their jobs.

I've included a test that both validates that the frontend no longer crashes 
_and_ that the resultant structures are packed as expected.


Without this, GCH serialization of late template expansions will
encounter an assertion failure.

Changes were required to ASTReader::ReadString to add support for
reading a string from a RecordDataImpl (which is the type consumed by
ReadToken) rather than a RecordData.

`#pragma pack` slot names are read into the preexisting string storage
PragmaAlignPackStrings.

This requires incidental changes [Parse,Sema] to move PragmaPackInfo to Sema

There is precedent for ASTReader/ASTWriter to poke into Sema.

Fixes https://github.com/llvm/llvm-project/issues/60543


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143410

Files:
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/PCH/delayed-template-with-pragma-pack.cpp

Index: clang/test/PCH/delayed-template-with-pragma-pack.cpp
===
--- /dev/null
+++ clang/test/PCH/delayed-template-with-pragma-pack.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -emit-pch -o %t.pch %s
+// RUN: %clang_cc1 -fdelayed-template-parsing -emit-pch -o %t.delayed.pch %s
+// RUN: %clang_cc1 -DMAIN_FILE \
+// RUN:   -include-pch %t.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+// RUN: %clang_cc1 -DMAIN_FILE -fdelayed-template-parsing \
+// RUN:   -include-pch %t.delayed.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+
+#ifndef MAIN_FILE
+
+template 
+int func() {
+#pragma pack(push, 1)
+  struct s { short a; T b; };
+#pragma pack(pop)
+  return sizeof(s);
+}
+
+#else
+
+// CHECK-LABEL: define linkonce_odr dso_local noundef i32 @"??$func@D@@YAHXZ"(
+// CHECK: ret i32 3
+int foo() {
+  return func();
+}
+
+// CHECK-LABEL: define linkonce_odr dso_local noundef i32 @"??$func@F@@YAHXZ"(
+// CHECK: ret i32 4
+int bar() {
+  return func();
+}
+
+// expected-no-diagnostics
+
+#endif
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4412,6 +4412,14 @@
 AddToken(T, Record);
   break;
 }
+case tok::annot_pragma_pack: {
+  auto *Info =
+  static_cast(Tok.getAnnotationValue());
+  Record.push_back(static_cast(Info->Action));
+  AddString(Info->SlotLabel, Record);
+  AddToken(Info->Alignment, Record);
+  break;
+}
 // Some annotation tokens do not use the PtrData field.
 case tok::annot_pragma_openmp:
 case tok::annot_pragma_openmp_end:
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1687,6 +1687,15 @@
   Tok.setAnnotationValue(static_cast(Info));
   break;
 }
+case tok::annot_pragma_pack: {
+  auto *Info = new (PP.getPreprocessorAllocator()) Sema::PragmaPackInfo;
+  Info->Action = static_cast(Record[Idx++]);
+  PragmaAlignPackStrings.push_back(ReadString(Record, Idx));
+  Info->SlotLabel = PragmaAlignPackStrings.back();
+  Info->Alignment = ReadToken(F, Record, Idx);
+  Tok.setAnnotationValue(static_cast(Info));
+  break;
+}
 // Some annotation tokens do not use the PtrData field.
 case tok::annot_pragma_openmp:
 case tok::annot_pragma_openmp_end:
@@ -9089,7 +9098,7 @@
 }
 
 // Read a string
-std::string ASTReader::ReadString(const RecordData &Record, unsigned &Idx) {
+std::string ASTReader::ReadString(const RecordDataImpl &Record, unsigned &Idx) {
   unsigned Len = Record[Idx++];
   std::string Result(Record.data() + Idx, Record.data() + Idx + Len);
   Idx += Len;
Index: clang/lib/Parse/ParsePragma.cpp
===
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePragma.cpp
@@ -665,18 +665,10 @@
   Actions.ActOnPragmaVisibility(VisType, VisLoc);
 }
 
-namespace {
-struct PragmaPackInfo {
-  Sema::Pragm

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

While this fixes the assertion failure and the immediate issue of whether 
packing _works_ inside delay-parsed templates in a PCH, it does reveal a 
follow-on issue that I can't quite trace out.

  c++
  template 
  void foo() {
  #pragma pack(push, 1)
  #pragma pack(show)
  #pragma pack(pop)
  }

results in...

  unterminated '#pragma pack (push, ...)' at end of file

I've stepped through ASTReader and verified that we do get all three expected 
pack pragma nodes

  {PSK_Push, ..., 1}
  {PSK_Show}
  {PSK_Pop, ...}

suggesting that there's something later down the line that messes up the pack 
stack.




Comment at: clang/test/PCH/delayed-template-with-pragma-pack.cpp:14
+int func() {
+#pragma pack(push, 1)
+  struct s { short a; T b; };

mikerice wrote:
> Your test should include testing of the slot string too, so we are sure all 
> fields are serialized and restored correctly. Otherwise this looks reasonable 
> to me.
So, I've identified an additional wrinkle...

Unlike `#pragma omp`, `#pragma pack` and `... align` don't show up in the AST 
dump/reprint and I cannot verify their continued existence; this is true _even 
without delayed template expansion or PCH generation._

alignment/packing seem to not be stored as Attrs, so they don't get printed via 
attr print on the AST nodes or (prettily) via the tablegen-generated pretty 
printers.

Fixing this is somewhat beyond me at the moment. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143410

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

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 495285.
DHowett-MSFT added a comment.

@mikerice Alright, it turns out that PragmaPackAlignStorage isn't suitable for 
this (actually, I'm not sure what it _is_
suitable for) because it's deleted by the time the template gets parsed again.

The `push, slot` case failed nondeterministically because the slot name is 
being UAF'd.

For now (and I am not sure this is correct), I've chosen to store the slot name 
in the preprocessor storage.

I suspect that other serialized pack info (not from the pragma, but the attr 
version that is stored in PragmaPackAlignStorage) is subject to the same 
limitation...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143410

Files:
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/PCH/delayed-template-with-pragma-pack.cpp

Index: clang/test/PCH/delayed-template-with-pragma-pack.cpp
===
--- /dev/null
+++ clang/test/PCH/delayed-template-with-pragma-pack.cpp
@@ -0,0 +1,78 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-pch -o %t.pch %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fdelayed-template-parsing -emit-pch -o %t.delayed.pch %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -DMAIN_FILE \
+// RUN:   -include-pch %t.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -DMAIN_FILE -fdelayed-template-parsing \
+// RUN:   -include-pch %t.delayed.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+
+#ifndef MAIN_FILE
+
+extern "C" void consume(int b);
+
+template 
+void function() {
+#pragma pack(push, 1)
+  struct packedAt1 {
+char a;
+unsigned long long b;
+char c;
+unsigned long long d;
+// 18 bytes total
+  };
+#pragma pack(push, slot1, 2)
+  struct packedAt2 {
+char a; // +1 byte of padding
+unsigned long long b;
+char c; // +1 byte of padding
+unsigned long long d;
+// 20 bytes total
+  };
+#pragma pack(push, 4)
+  struct packedAt4 {
+char a; // +3 bytes of padding
+unsigned long long b;
+char c; // +3 bytes of padding
+unsigned long long d;
+// 24 bytes total
+  };
+#pragma pack(push, 16)
+  struct packedAt16 {
+char a; // +7 bytes of padding
+unsigned long long b;
+char c; // +7 bytes of padding
+unsigned long long d;
+// 32 bytes total
+  };
+#pragma pack(pop, slot1) // This should return packing to 1 (established before push(slot1))
+  struct packedAfterPopBackTo1 {
+char a;
+unsigned long long b;
+char c;
+unsigned long long d;
+  };
+#pragma pack(pop)
+
+  consume(sizeof(packedAt1)); // 18
+  consume(sizeof(packedAt2)); // 20
+  consume(sizeof(packedAt4)); // 24
+  consume(sizeof(packedAt16)); // 32
+  consume(sizeof(packedAfterPopBackTo1)); // 18 again
+}
+
+#else
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??$function@$0A@@@YAXXZ"(
+// CHECK: call void @consume(i32 noundef 18)
+// CHECK-NEXT: call void @consume(i32 noundef 20)
+// CHECK-NEXT: call void @consume(i32 noundef 24)
+// CHECK-NEXT: call void @consume(i32 noundef 32)
+// CHECK-NEXT: call void @consume(i32 noundef 18)
+void foo() {
+  function<0>();
+}
+
+// expected-no-diagnostics
+
+#endif
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4412,6 +4412,14 @@
 AddToken(T, Record);
   break;
 }
+case tok::annot_pragma_pack: {
+  auto *Info =
+  static_cast(Tok.getAnnotationValue());
+  Record.push_back(static_cast(Info->Action));
+  AddString(Info->SlotLabel, Record);
+  AddToken(Info->Alignment, Record);
+  break;
+}
 // Some annotation tokens do not use the PtrData field.
 case tok::annot_pragma_openmp:
 case tok::annot_pragma_openmp_end:
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -120,6 +120,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SaveAndRestore.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/VersionTuple.h"
@@ -1672,6 +1673,8 @@
   Tok.setFlag((Token::TokenFlags)Record[Idx++]);
 
   if (Tok.isAnnotation()) {
+llvm::StringSaver PreprocessorStringStorage(PP.getPreprocessorAllocator());
+
 Tok.setAnnotationEndLoc(ReadSourceLocation(F, Record, Idx));
 switch (Tok.getKind()) {
 case tok::annot_pragma_loop_hint: {
@@ -1687,6 +1690,15 @@
   

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: test/Sema/ms-forceinline-on-variadic.cpp:1
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-windows -verify 
-fms-extensions %s \
+// RUN:| FileCheck %s

compnerd wrote:
> Personally, I prefer the fully canonicalized triple name 
> `i686-unknown-windows-msvc`.
I'll do this.



Comment at: test/Sema/ms-forceinline-on-variadic.cpp:14
+__builtin_va_end(ap);
+}
+

compnerd wrote:
> Would be nice to have a second test that uses the Microsoft definitions 
> (`char *` and the offsetting handling for the `va_list` since when building 
> against the Windows SDK, that is the way that `va_list` and the `va_*` family 
> of functions will get handled.
Should/do those still result in the intrinsic being emitted? If not, LLVM 
shouldn't have trouble during instruction scheduling, but inlining may still be 
broken. Regardless, though, this test exists only to make sure the function 
doesn't end up truly inlined, regardless of its contents.


Repository:
  rC Clang

https://reviews.llvm.org/D44646



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

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added inline comments.



Comment at: test/Sema/ms-forceinline-on-variadic.cpp:14
+__builtin_va_end(ap);
+}
+

efriedma wrote:
> compnerd wrote:
> > DHowett-MSFT wrote:
> > > compnerd wrote:
> > > > Would be nice to have a second test that uses the Microsoft definitions 
> > > > (`char *` and the offsetting handling for the `va_list` since when 
> > > > building against the Windows SDK, that is the way that `va_list` and 
> > > > the `va_*` family of functions will get handled.
> > > Should/do those still result in the intrinsic being emitted? If not, LLVM 
> > > shouldn't have trouble during instruction scheduling, but inlining may 
> > > still be broken. Regardless, though, this test exists only to make sure 
> > > the function doesn't end up truly inlined, regardless of its contents.
> > That would still be lowered with the intrinsics.  The intent is to make 
> > sure that the different implementation does get lowered appropriately.
> To get correct lowering in LLVM, the va_start macro *must* be translated to 
> __builtin_va_start(); otherwise, the generated IR is nonsense and you'll 
> miscompile.  (See also https://bugs.llvm.org/show_bug.cgi?id=24320 .)
This sounds like an argument for not including a test of the Microsoft 
definition of `va_start`.


Repository:
  rC Clang

https://reviews.llvm.org/D44646



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

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT marked 5 inline comments as done.
DHowett-MSFT added a comment.

@compnerd CL warns for `__forceinline` variadics in C code as well:

  test.c(1): warning C4714: function 'void hello(int,...)' marked as 
__forceinline not inlined


Repository:
  rC Clang

https://reviews.llvm.org/D44646



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

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

Thank you for the review! I don't have the means to check this in myself.


Repository:
  rC Clang

https://reviews.llvm.org/D44580



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

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 141055.
DHowett-MSFT added a comment.

Switched from `i686-windows` to `i686-unknown-windows-msvc` as per @compnerd's 
suggestion.
Based on the prior discussion around MSVC's definition of `va_{start,end,arg}` 
and how it is not a valid implementation that we do not pattern-match and 
convert into the intrinsid, I do not see the need in implementing an MSVC 
`va_start` test.

Regardless of that, the behaviour under test is that the function is _not 
inlined_, not that the intrinsic is emitted properly.


Repository:
  rC Clang

https://reviews.llvm.org/D44646

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/ms-forceinline-on-variadic.cpp


Index: test/Sema/ms-forceinline-on-variadic.cpp
===
--- /dev/null
+++ test/Sema/ms-forceinline-on-variadic.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-unknown-windows-msvc -verify 
-fms-extensions %s \
+// RUN:| FileCheck %s
+//
+// (instruction scheduling crash test)
+// RUN: %clang_cc1 -S -o - -triple i686-unknown-windows-msvc -fms-extensions %s
+
+inline __attribute__((always_inline)) void attribute_variadic(int f, ...) { } 
// expected-warning {{inlining attribute 'always_inline' ignored on variadic 
function}}
+
+__forceinline inline void msvc_variadic(int f, ...) { // expected-warning 
{{inlining attribute '__forceinline' ignored on variadic function}}
+// CHECK-DAG: define {{.*}} void [[MSVC_VARIADIC:@".*msvc_variadic.*"]](
+__builtin_va_list ap;
+__builtin_va_start(ap, f);
+__builtin_va_end(ap);
+}
+
+struct a {
+// members are, by default, thiscall; enforce it for the purposes of the 
test
+void __thiscall dispatcher();
+};
+
+void __thiscall a::dispatcher() {
+msvc_variadic(1, 2, 3);
+// CHECK-DAG: define dso_local x86_thiscallcc void @"{{.*dispatcher.*}}"(
+// CHECK-DAG: call void {{.*}} [[MSVC_VARIADIC]]
+}
+
+void t() {
+a{}.dispatcher();
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3886,6 +3886,12 @@
 return nullptr;
   }
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+  hasFunctionProto(D) && isFunctionOrMethodVariadic(D)) {
+Diag(Range.getBegin(), diag::warn_always_inline_on_variadic) << Ident;
+return nullptr;
+  }
+
   if (D->hasAttr())
 return nullptr;
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2694,6 +2694,9 @@
 def warn_attribute_after_definition_ignored : Warning<
   "attribute %0 after definition is ignored">,
InGroup;
+def warn_always_inline_on_variadic : Warning<
+  "inlining attribute %0 ignored on variadic function">,
+   InGroup;
 def warn_unknown_attribute_ignored : Warning<
   "unknown attribute %0 ignored">, InGroup;
 def warn_cxx11_gnu_attribute_on_type : Warning<


Index: test/Sema/ms-forceinline-on-variadic.cpp
===
--- /dev/null
+++ test/Sema/ms-forceinline-on-variadic.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-unknown-windows-msvc -verify -fms-extensions %s \
+// RUN:| FileCheck %s
+//
+// (instruction scheduling crash test)
+// RUN: %clang_cc1 -S -o - -triple i686-unknown-windows-msvc -fms-extensions %s
+
+inline __attribute__((always_inline)) void attribute_variadic(int f, ...) { } // expected-warning {{inlining attribute 'always_inline' ignored on variadic function}}
+
+__forceinline inline void msvc_variadic(int f, ...) { // expected-warning {{inlining attribute '__forceinline' ignored on variadic function}}
+// CHECK-DAG: define {{.*}} void [[MSVC_VARIADIC:@".*msvc_variadic.*"]](
+__builtin_va_list ap;
+__builtin_va_start(ap, f);
+__builtin_va_end(ap);
+}
+
+struct a {
+// members are, by default, thiscall; enforce it for the purposes of the test
+void __thiscall dispatcher();
+};
+
+void __thiscall a::dispatcher() {
+msvc_variadic(1, 2, 3);
+// CHECK-DAG: define dso_local x86_thiscallcc void @"{{.*dispatcher.*}}"(
+// CHECK-DAG: call void {{.*}} [[MSVC_VARIADIC]]
+}
+
+void t() {
+a{}.dispatcher();
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3886,6 +3886,12 @@
 return nullptr;
   }
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+  hasFunctionProto(D) && isFunctionOrMethodVariadic(D)) {
+Diag(Range.getBegin(), diag::warn_always_inline_on_variadic) << Ident;
+return nullptr;
+  }
+
   if (D->hasAttr())
 return nullptr;
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT created this revision.
DHowett-MSFT added reviewers: rjmccall, theraven.
DHowett-MSFT added a project: clang.
Herald added a subscriber: cfe-commits.

Protocols that were being referenced but could not be fully realized were being 
emitted without `properties`/`optional_properties`. Since they're packed 
end-to-end, the lack of these fields is catastrophic for the runtime.

As an example, the runtime cannot know here 
 that 
`properties` and `optional_properties` are invalid if they overlap with another 
v3 protocol.


Repository:
  rC Clang

https://reviews.llvm.org/D45305

Files:
  lib/CodeGen/CGObjCGNU.cpp


Index: lib/CodeGen/CGObjCGNU.cpp
===
--- lib/CodeGen/CGObjCGNU.cpp
+++ lib/CodeGen/CGObjCGNU.cpp
@@ -1813,11 +1813,13 @@
   llvm::ConstantInt::get(Int32Ty, ProtocolVersion), IdTy));
 
   Elements.add(MakeConstantString(ProtocolName, ".objc_protocol_name"));
-  Elements.add(ProtocolList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
+  Elements.add(ProtocolList); /* .protocol_list */
+  Elements.add(MethodList);   /* .instance_methods */
+  Elements.add(MethodList);   /* .class_methods */
+  Elements.add(MethodList);   /* .optional_instance_methods */
+  Elements.add(MethodList);   /* .optional_class_methods */
+  Elements.add(NULLPtr);  /* .properties */
+  Elements.add(NULLPtr);  /* .optional_properties */
   return Elements.finishAndCreateGlobal(".objc_protocol",
 CGM.getPointerAlign());
 }


Index: lib/CodeGen/CGObjCGNU.cpp
===
--- lib/CodeGen/CGObjCGNU.cpp
+++ lib/CodeGen/CGObjCGNU.cpp
@@ -1813,11 +1813,13 @@
   llvm::ConstantInt::get(Int32Ty, ProtocolVersion), IdTy));
 
   Elements.add(MakeConstantString(ProtocolName, ".objc_protocol_name"));
-  Elements.add(ProtocolList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
+  Elements.add(ProtocolList); /* .protocol_list */
+  Elements.add(MethodList);   /* .instance_methods */
+  Elements.add(MethodList);   /* .class_methods */
+  Elements.add(MethodList);   /* .optional_instance_methods */
+  Elements.add(MethodList);   /* .optional_class_methods */
+  Elements.add(NULLPtr);  /* .properties */
+  Elements.add(NULLPtr);  /* .optional_properties */
   return Elements.finishAndCreateGlobal(".objc_protocol",
 CGM.getPointerAlign());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44646: Sema: in msvc compatibility mode, don't allow forceinline on variadics

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

Thanks! I don't have the means to check this in myself.


Repository:
  rC Clang

https://reviews.llvm.org/D44646



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

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 141450.
DHowett-MSFT added a comment.

Added a test per @rjmccall's suggestion. I chose to test this by emitting 
assembly because the llvm ir is significantly harder to pick apart for the true 
_size_ of the protocol.


Repository:
  rC Clang

https://reviews.llvm.org/D45305

Files:
  lib/CodeGen/CGObjCGNU.cpp
  test/CodeGenObjC/undefined-protocol-v3.m


Index: test/CodeGenObjC/undefined-protocol-v3.m
===
--- /dev/null
+++ test/CodeGenObjC/undefined-protocol-v3.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fobjc-runtime=gnustep-1.9 -S -o 
- %s | FileCheck %s
+
+@protocol X;
+
+__attribute__((objc_root_class))
+@interface Z 
+@end
+
+@implementation Z
+@end
+
+// CHECK-DAG: .objc_protocol:
+// CHECK-DAG:   .long 3
+// CHECK-DAG:   .size   .objc_protocol, 36
Index: lib/CodeGen/CGObjCGNU.cpp
===
--- lib/CodeGen/CGObjCGNU.cpp
+++ lib/CodeGen/CGObjCGNU.cpp
@@ -1813,11 +1813,13 @@
   llvm::ConstantInt::get(Int32Ty, ProtocolVersion), IdTy));
 
   Elements.add(MakeConstantString(ProtocolName, ".objc_protocol_name"));
-  Elements.add(ProtocolList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
+  Elements.add(ProtocolList); /* .protocol_list */
+  Elements.add(MethodList);   /* .instance_methods */
+  Elements.add(MethodList);   /* .class_methods */
+  Elements.add(MethodList);   /* .optional_instance_methods */
+  Elements.add(MethodList);   /* .optional_class_methods */
+  Elements.add(NULLPtr);  /* .properties */
+  Elements.add(NULLPtr);  /* .optional_properties */
   return Elements.finishAndCreateGlobal(".objc_protocol",
 CGM.getPointerAlign());
 }


Index: test/CodeGenObjC/undefined-protocol-v3.m
===
--- /dev/null
+++ test/CodeGenObjC/undefined-protocol-v3.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fobjc-runtime=gnustep-1.9 -S -o - %s | FileCheck %s
+
+@protocol X;
+
+__attribute__((objc_root_class))
+@interface Z 
+@end
+
+@implementation Z
+@end
+
+// CHECK-DAG: .objc_protocol:
+// CHECK-DAG:   .long 3
+// CHECK-DAG:   .size   .objc_protocol, 36
Index: lib/CodeGen/CGObjCGNU.cpp
===
--- lib/CodeGen/CGObjCGNU.cpp
+++ lib/CodeGen/CGObjCGNU.cpp
@@ -1813,11 +1813,13 @@
   llvm::ConstantInt::get(Int32Ty, ProtocolVersion), IdTy));
 
   Elements.add(MakeConstantString(ProtocolName, ".objc_protocol_name"));
-  Elements.add(ProtocolList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
+  Elements.add(ProtocolList); /* .protocol_list */
+  Elements.add(MethodList);   /* .instance_methods */
+  Elements.add(MethodList);   /* .class_methods */
+  Elements.add(MethodList);   /* .optional_instance_methods */
+  Elements.add(MethodList);   /* .optional_class_methods */
+  Elements.add(NULLPtr);  /* .properties */
+  Elements.add(NULLPtr);  /* .optional_properties */
   return Elements.finishAndCreateGlobal(".objc_protocol",
 CGM.getPointerAlign());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

It seems more fragile to check the contents of the protocol rather than the 
invariant we care most about (its size).
I'm willing to do that, and am updating the patch accordingly, but I don't want 
to commit to a more fragile test than is necessary.
Illustratively, if the GNUstep runtime one day chooses to emit empty v3 
protocols as `{3, null, null, null, null, null, null, null}` (which it may want 
to do to avoid emitting empty method lists, and which seems to work at 
runtime), a test capturing the body of the protocol will fail.


Repository:
  rC Clang

https://reviews.llvm.org/D45305



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

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 141701.
DHowett-MSFT edited the summary of this revision.
DHowett-MSFT added a comment.

I've fixed the test to check the LLVM IR; I chose to use `CHECK-SAME` and use 
indentation to clarify what was going on. We're now checking that an empty 
protocol is emitted in full, and that all of its members match our expectations.


Repository:
  rC Clang

https://reviews.llvm.org/D45305

Files:
  lib/CodeGen/CGObjCGNU.cpp
  test/CodeGenObjC/gnu-empty-protocol-v3.m


Index: test/CodeGenObjC/gnu-empty-protocol-v3.m
===
--- /dev/null
+++ test/CodeGenObjC/gnu-empty-protocol-v3.m
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fobjc-runtime=gnustep-1.9 
-emit-llvm -o - %s | FileCheck %s
+
+@protocol X;
+
+__attribute__((objc_root_class))
+@interface Z 
+@end
+
+@implementation Z
+@end
+
+// CHECK:  @.objc_protocol_list = internal global { i8*, i32, [0 x i8*] } 
zeroinitializer, align 4
+// CHECK:  @.objc_method_list = internal global { i32, [0 x { i8*, i8* }] 
} zeroinitializer, align 4
+// CHECK:  @.objc_protocol_name = private unnamed_addr constant [2 x i8] 
c"X\00", align 1
+// CHECK:  @.objc_protocol = internal global { i8*, i8*, { i8*, i32, [0 x 
i8*] }*, { i32, [0 x { i8*, i8* }] }*, { i32, [0 x { i8*, i8* }] }*, { i32, [0 
x { i8*, i8* }] }*, { i32, [0 x { i8*, i8* }] }*, i8*, i8* } {
+// CHECK-SAME: i8* inttoptr (i32 3 to i8*),
+// CHECK-SAME: i8* getelementptr inbounds ([2 x i8], [2 x i8]* 
@.objc_protocol_name, i32 0, i32 0),
+// CHECK-SAME: { i8*, i32, [0 x i8*] }* @.objc_protocol_list,
+// CHECK-SAME: { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME: { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME: { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME: { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME: i8* null,
+// CHECK-SAME: i8* null
+// CHECK-SAME: }, align 4
Index: lib/CodeGen/CGObjCGNU.cpp
===
--- lib/CodeGen/CGObjCGNU.cpp
+++ lib/CodeGen/CGObjCGNU.cpp
@@ -1813,11 +1813,13 @@
   llvm::ConstantInt::get(Int32Ty, ProtocolVersion), IdTy));
 
   Elements.add(MakeConstantString(ProtocolName, ".objc_protocol_name"));
-  Elements.add(ProtocolList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
+  Elements.add(ProtocolList); /* .protocol_list */
+  Elements.add(MethodList);   /* .instance_methods */
+  Elements.add(MethodList);   /* .class_methods */
+  Elements.add(MethodList);   /* .optional_instance_methods */
+  Elements.add(MethodList);   /* .optional_class_methods */
+  Elements.add(NULLPtr);  /* .properties */
+  Elements.add(NULLPtr);  /* .optional_properties */
   return Elements.finishAndCreateGlobal(".objc_protocol",
 CGM.getPointerAlign());
 }


Index: test/CodeGenObjC/gnu-empty-protocol-v3.m
===
--- /dev/null
+++ test/CodeGenObjC/gnu-empty-protocol-v3.m
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple i686-pc-linux-gnu -fobjc-runtime=gnustep-1.9 -emit-llvm -o - %s | FileCheck %s
+
+@protocol X;
+
+__attribute__((objc_root_class))
+@interface Z 
+@end
+
+@implementation Z
+@end
+
+// CHECK:  @.objc_protocol_list = internal global { i8*, i32, [0 x i8*] } zeroinitializer, align 4
+// CHECK:  @.objc_method_list = internal global { i32, [0 x { i8*, i8* }] } zeroinitializer, align 4
+// CHECK:  @.objc_protocol_name = private unnamed_addr constant [2 x i8] c"X\00", align 1
+// CHECK:  @.objc_protocol = internal global { i8*, i8*, { i8*, i32, [0 x i8*] }*, { i32, [0 x { i8*, i8* }] }*, { i32, [0 x { i8*, i8* }] }*, { i32, [0 x { i8*, i8* }] }*, { i32, [0 x { i8*, i8* }] }*, i8*, i8* } {
+// CHECK-SAME: i8* inttoptr (i32 3 to i8*),
+// CHECK-SAME: i8* getelementptr inbounds ([2 x i8], [2 x i8]* @.objc_protocol_name, i32 0, i32 0),
+// CHECK-SAME: { i8*, i32, [0 x i8*] }* @.objc_protocol_list,
+// CHECK-SAME: { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME: { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME: { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME: { i32, [0 x { i8*, i8* }] }* @.objc_method_list,
+// CHECK-SAME: i8* null,
+// CHECK-SAME: i8* null
+// CHECK-SAME: }, align 4
Index: lib/CodeGen/CGObjCGNU.cpp
===
--- lib/CodeGen/CGObjCGNU.cpp
+++ lib/CodeGen/CGObjCGNU.cpp
@@ -1813,11 +1813,13 @@
   llvm::ConstantInt::get(Int32Ty, ProtocolVersion), IdTy));
 
   Elements.add(MakeConstantString(ProtocolName, ".objc_protocol_name"));
-  Elements.add(ProtocolList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(MethodList);
-  Elements.add(Meth

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT marked 2 inline comments as done.
DHowett-MSFT added a comment.

The new test checks how tightly things were packed across a stack of packs, 
pushed and popped. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143410

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

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT updated this revision to Diff 495314.
DHowett-MSFT added a comment.

- Use .copy() like the other nearby code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143410

Files:
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Parse/ParsePragma.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/PCH/delayed-template-with-pragma-pack.cpp

Index: clang/test/PCH/delayed-template-with-pragma-pack.cpp
===
--- /dev/null
+++ clang/test/PCH/delayed-template-with-pragma-pack.cpp
@@ -0,0 +1,78 @@
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-pch -o %t.pch %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fdelayed-template-parsing -emit-pch -o %t.delayed.pch %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -DMAIN_FILE \
+// RUN:   -include-pch %t.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -DMAIN_FILE -fdelayed-template-parsing \
+// RUN:   -include-pch %t.delayed.pch \
+// RUN:   -emit-llvm -verify -o - %s | FileCheck %s
+
+#ifndef MAIN_FILE
+
+extern "C" void consume(int b);
+
+template 
+void function() {
+#pragma pack(push, 1)
+  struct packedAt1 {
+char a;
+unsigned long long b;
+char c;
+unsigned long long d;
+// 18 bytes total
+  };
+#pragma pack(push, slot1, 2)
+  struct packedAt2 {
+char a; // +1 byte of padding
+unsigned long long b;
+char c; // +1 byte of padding
+unsigned long long d;
+// 20 bytes total
+  };
+#pragma pack(push, 4)
+  struct packedAt4 {
+char a; // +3 bytes of padding
+unsigned long long b;
+char c; // +3 bytes of padding
+unsigned long long d;
+// 24 bytes total
+  };
+#pragma pack(push, 16)
+  struct packedAt16 {
+char a; // +7 bytes of padding
+unsigned long long b;
+char c; // +7 bytes of padding
+unsigned long long d;
+// 32 bytes total
+  };
+#pragma pack(pop, slot1) // This should return packing to 1 (established before push(slot1))
+  struct packedAfterPopBackTo1 {
+char a;
+unsigned long long b;
+char c;
+unsigned long long d;
+  };
+#pragma pack(pop)
+
+  consume(sizeof(packedAt1)); // 18
+  consume(sizeof(packedAt2)); // 20
+  consume(sizeof(packedAt4)); // 24
+  consume(sizeof(packedAt16)); // 32
+  consume(sizeof(packedAfterPopBackTo1)); // 18 again
+}
+
+#else
+
+// CHECK-LABEL: define linkonce_odr dso_local void @"??$function@$0A@@@YAXXZ"(
+// CHECK: call void @consume(i32 noundef 18)
+// CHECK-NEXT: call void @consume(i32 noundef 20)
+// CHECK-NEXT: call void @consume(i32 noundef 24)
+// CHECK-NEXT: call void @consume(i32 noundef 32)
+// CHECK-NEXT: call void @consume(i32 noundef 18)
+void foo() {
+  function<0>();
+}
+
+// expected-no-diagnostics
+
+#endif
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -4412,6 +4412,14 @@
 AddToken(T, Record);
   break;
 }
+case tok::annot_pragma_pack: {
+  auto *Info =
+  static_cast(Tok.getAnnotationValue());
+  Record.push_back(static_cast(Info->Action));
+  AddString(Info->SlotLabel, Record);
+  AddToken(Info->Alignment, Record);
+  break;
+}
 // Some annotation tokens do not use the PtrData field.
 case tok::annot_pragma_openmp:
 case tok::annot_pragma_openmp_end:
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1687,6 +1687,16 @@
   Tok.setAnnotationValue(static_cast(Info));
   break;
 }
+case tok::annot_pragma_pack: {
+  auto *Info = new (PP.getPreprocessorAllocator()) Sema::PragmaPackInfo;
+  Info->Action = static_cast(Record[Idx++]);
+  auto SlotLabel = ReadString(Record, Idx);
+  Info->SlotLabel =
+  llvm::StringRef(SlotLabel).copy(PP.getPreprocessorAllocator());
+  Info->Alignment = ReadToken(F, Record, Idx);
+  Tok.setAnnotationValue(static_cast(Info));
+  break;
+}
 // Some annotation tokens do not use the PtrData field.
 case tok::annot_pragma_openmp:
 case tok::annot_pragma_openmp_end:
@@ -9089,7 +9099,7 @@
 }
 
 // Read a string
-std::string ASTReader::ReadString(const RecordData &Record, unsigned &Idx) {
+std::string ASTReader::ReadString(const RecordDataImpl &Record, unsigned &Idx) {
   unsigned Len = Record[Idx++];
   std::string Result(Record.data() + Idx, Record.data() + Idx + Len);
   Idx += Len;
Index: clang/lib/Parse/ParsePragma.cpp
===
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePrag

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

Test failures seem related to recent driver changes. There's a commit 
6a8a423c1864ced060a7041bf6ada7574f35ad4d 
 that 
purports to fix them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143410

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

[PATCH] D143410: [Serialization] Add support for (de)serializing #pragma pack

[Dustin L. Howett via Phabricator via cfe-commits]

DHowett-MSFT added a comment.

Thanks for the review!

I’ll need somebody to help land this, as I don’t have write access to the 
project. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143410

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