[PATCH] D55543: [CodeGen] Fix assertion on throwing object with inlined inherited constructor and non-trivial destructor.

2018-12-11 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

I'd tried this exact same patch back in https://reviews.llvm.org/D44619, but I 
was running into a bunch of check-clang failures with it, and I was never able 
to figure them out. It looks like it works now though, so I'm glad :) Richard's 
comment from that diff might still be relevant:

> Please add a test to ensure that we still destroy function parameters in the 
> right order and at the right times (for both the exceptional and 
> non-exceptional cleanup cases).

This will also resolve https://bugs.llvm.org/show_bug.cgi?id=36748


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

https://reviews.llvm.org/D55543



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


[PATCH] D44619: [CodeGen] Add cleanup scope to EmitInlinedInheritingCXXConstructorCall

2018-12-11 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai abandoned this revision.
smeenai added a comment.

https://reviews.llvm.org/D55543 does the same thing


Repository:
  rC Clang

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

https://reviews.llvm.org/D44619



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-11-15 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai planned changes to this revision.
smeenai added a comment.

In https://reviews.llvm.org/D52674#1298115, @rjmccall wrote:

> In https://reviews.llvm.org/D52674#1297893, @smeenai wrote:
>
> > In https://reviews.llvm.org/D52674#1297879, @rjmccall wrote:
> >
> > > I'm not worried about the mangler being re-used for multiple 
> > > declarations, I'm worried about a global flag changing how we mangle all 
> > > components of a type when we only mean to change it at the top level.
> >
> >
> > Hmm, but don't we want it to affect all components? For example, consider 
> > something like:
> >
> >   @interface I
> >   @end
> >  
> >   template  class C {};
> >  
> >   void f();
> >   void g() {
> > try {
> >   f();
> > } catch (C *) {
> > }
> >   }
> >
> >
> > I would say that we want the RTTI for `C *` to have the discriminator 
> > for `I`.
>
>
> Why?  IIUC, you're adding the discriminator to distinguish between two 
> different RTTI objects.  It's not like there are two different types.  You 
> should mangle `C` normally here.


You're right – I wasn't thinking this through correctly. Back to the drawing 
board.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D54536: [AST] Fix typo in MicrosoftMangle

2018-11-14 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC346882: [AST] Fix typo in MicrosoftMangle (authored by 
smeenai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D54536?vs=174064=174075#toc

Repository:
  rC Clang

https://reviews.llvm.org/D54536

Files:
  lib/AST/MicrosoftMangle.cpp

Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -309,7 +309,7 @@
 const MethodVFTableLocation );
   void mangleNumber(int64_t Number);
   void mangleTagTypeKind(TagTypeKind TK);
-  void mangleArtificalTagType(TagTypeKind TK, StringRef UnqualifiedName,
+  void mangleArtificialTagType(TagTypeKind TK, StringRef UnqualifiedName,
   ArrayRef NestedNames = None);
   void mangleType(QualType T, SourceRange Range,
   QualifierMangleMode QMM = QMM_Mangle);
@@ -1070,7 +1070,7 @@
   if (PointersAre64Bit)
 Out << 'E';
   Out << 'A';
-  mangleArtificalTagType(TTK_Struct,
+  mangleArtificialTagType(TTK_Struct,
  Discriminate("__block_literal", Discriminator,
   ParameterDiscriminator));
   Out << "@Z";
@@ -1365,7 +1365,7 @@
 // It's a global variable.
 Out << '3';
 // It's a struct called __s_GUID.
-mangleArtificalTagType(TTK_Struct, "__s_GUID");
+mangleArtificialTagType(TTK_Struct, "__s_GUID");
 // It's const.
 Out << 'B';
 return;
@@ -1521,9 +1521,9 @@
 
   Stream << "?$";
   Extra.mangleSourceName("Protocol");
-  Extra.mangleArtificalTagType(TTK_Struct, PD->getName());
+  Extra.mangleArtificialTagType(TTK_Struct, PD->getName());
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleObjCLifetime(const QualType Type,
@@ -1552,7 +1552,7 @@
   Extra.manglePointerExtQualifiers(Quals, Type);
   Extra.mangleType(Type, Range);
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleObjCKindOfType(const ObjCObjectType *T,
@@ -1569,7 +1569,7 @@
->getAs(),
Quals, Range);
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleQualifiers(Qualifiers Quals,
@@ -1765,7 +1765,7 @@
   ArgBackRefMap::iterator Found = TypeBackReferences.find(TypePtr);
 
   if (Found == TypeBackReferences.end()) {
-mangleArtificalTagType(TTK_Enum, "__pass_object_size" + llvm::utostr(Type),
+mangleArtificialTagType(TTK_Enum, "__pass_object_size" + llvm::utostr(Type),
{"__clang"});
 
 if (TypeBackReferences.size() < 10) {
@@ -1951,13 +1951,13 @@
 llvm_unreachable("placeholder types shouldn't get to name mangling");
 
   case BuiltinType::ObjCId:
-mangleArtificalTagType(TTK_Struct, "objc_object");
+mangleArtificialTagType(TTK_Struct, "objc_object");
 break;
   case BuiltinType::ObjCClass:
-mangleArtificalTagType(TTK_Struct, "objc_class");
+mangleArtificialTagType(TTK_Struct, "objc_class");
 break;
   case BuiltinType::ObjCSel:
-mangleArtificalTagType(TTK_Struct, "objc_selector");
+mangleArtificialTagType(TTK_Struct, "objc_selector");
 break;
 
 #define IMAGE_TYPE(ImgType, Id, SingletonId, Access, Suffix) \
@@ -1967,40 +1967,40 @@
 #include "clang/Basic/OpenCLImageTypes.def"
   case BuiltinType::OCLSampler:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_sampler");
+mangleArtificialTagType(TTK_Struct, "ocl_sampler");
 break;
   case BuiltinType::OCLEvent:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_event");
+mangleArtificialTagType(TTK_Struct, "ocl_event");
 break;
   case BuiltinType::OCLClkEvent:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_clkevent");
+mangleArtificialTagType(TTK_Struct, "ocl_clkevent");
 break;
   case BuiltinType::OCLQueue:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_queue");
+mangleArtificialTagType(TTK_Struct, "ocl_queue");
 break;
   case BuiltinType::OCLReserveID:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_reserveid");
+mangleArtificialTagType(TTK_Struct, "ocl_reserveid");
 break;
 #define EXT_OPAQUE_TYPE(ExtType, Id, Ext) \
   case BuiltinType::Id: \
-mangleArtificalTagType(TTK_Struct, "ocl_" #ExtType); \
+mangleArtificialTagType(TTK_Struct, "ocl_" #ExtType); \
 break;
 #include "clang/Basic/OpenCLExtensionTypes.def"
 
   case BuiltinType::NullPtr:
 Out << "$$T";
 break;
 
   case BuiltinType::Float16:
-

[PATCH] D54536: [AST] Fix typo in MicrosoftMangle

2018-11-14 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: majnemer, rnk.

I noticed that we were spelling it as Artifical instead of Artificial
when working on the mangling for Obj-C RTTI. I'm putting this up for
review instead of committing it directly because I'm not 100% sure what
the policy here would be; it does clutter up the blame, but at the same
time it's fixing a gotcha for people writing new code.


Repository:
  rC Clang

https://reviews.llvm.org/D54536

Files:
  lib/AST/MicrosoftMangle.cpp

Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -309,7 +309,7 @@
 const MethodVFTableLocation );
   void mangleNumber(int64_t Number);
   void mangleTagTypeKind(TagTypeKind TK);
-  void mangleArtificalTagType(TagTypeKind TK, StringRef UnqualifiedName,
+  void mangleArtificialTagType(TagTypeKind TK, StringRef UnqualifiedName,
   ArrayRef NestedNames = None);
   void mangleType(QualType T, SourceRange Range,
   QualifierMangleMode QMM = QMM_Mangle);
@@ -1070,7 +1070,7 @@
   if (PointersAre64Bit)
 Out << 'E';
   Out << 'A';
-  mangleArtificalTagType(TTK_Struct,
+  mangleArtificialTagType(TTK_Struct,
  Discriminate("__block_literal", Discriminator,
   ParameterDiscriminator));
   Out << "@Z";
@@ -1365,7 +1365,7 @@
 // It's a global variable.
 Out << '3';
 // It's a struct called __s_GUID.
-mangleArtificalTagType(TTK_Struct, "__s_GUID");
+mangleArtificialTagType(TTK_Struct, "__s_GUID");
 // It's const.
 Out << 'B';
 return;
@@ -1521,9 +1521,9 @@
 
   Stream << "?$";
   Extra.mangleSourceName("Protocol");
-  Extra.mangleArtificalTagType(TTK_Struct, PD->getName());
+  Extra.mangleArtificialTagType(TTK_Struct, PD->getName());
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleObjCLifetime(const QualType Type,
@@ -1552,7 +1552,7 @@
   Extra.manglePointerExtQualifiers(Quals, Type);
   Extra.mangleType(Type, Range);
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleObjCKindOfType(const ObjCObjectType *T,
@@ -1569,7 +1569,7 @@
->getAs(),
Quals, Range);
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleQualifiers(Qualifiers Quals,
@@ -1765,7 +1765,7 @@
   ArgBackRefMap::iterator Found = TypeBackReferences.find(TypePtr);
 
   if (Found == TypeBackReferences.end()) {
-mangleArtificalTagType(TTK_Enum, "__pass_object_size" + llvm::utostr(Type),
+mangleArtificialTagType(TTK_Enum, "__pass_object_size" + llvm::utostr(Type),
{"__clang"});
 
 if (TypeBackReferences.size() < 10) {
@@ -1951,13 +1951,13 @@
 llvm_unreachable("placeholder types shouldn't get to name mangling");
 
   case BuiltinType::ObjCId:
-mangleArtificalTagType(TTK_Struct, "objc_object");
+mangleArtificialTagType(TTK_Struct, "objc_object");
 break;
   case BuiltinType::ObjCClass:
-mangleArtificalTagType(TTK_Struct, "objc_class");
+mangleArtificialTagType(TTK_Struct, "objc_class");
 break;
   case BuiltinType::ObjCSel:
-mangleArtificalTagType(TTK_Struct, "objc_selector");
+mangleArtificialTagType(TTK_Struct, "objc_selector");
 break;
 
 #define IMAGE_TYPE(ImgType, Id, SingletonId, Access, Suffix) \
@@ -1967,40 +1967,40 @@
 #include "clang/Basic/OpenCLImageTypes.def"
   case BuiltinType::OCLSampler:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_sampler");
+mangleArtificialTagType(TTK_Struct, "ocl_sampler");
 break;
   case BuiltinType::OCLEvent:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_event");
+mangleArtificialTagType(TTK_Struct, "ocl_event");
 break;
   case BuiltinType::OCLClkEvent:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_clkevent");
+mangleArtificialTagType(TTK_Struct, "ocl_clkevent");
 break;
   case BuiltinType::OCLQueue:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_queue");
+mangleArtificialTagType(TTK_Struct, "ocl_queue");
 break;
   case BuiltinType::OCLReserveID:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_reserveid");
+mangleArtificialTagType(TTK_Struct, "ocl_reserveid");
 break;
 #define EXT_OPAQUE_TYPE(ExtType, Id, Ext) \
   case BuiltinType::Id: \
-mangleArtificalTagType(TTK_Struct, "ocl_" #ExtType); \
+mangleArtificialTagType(TTK_Struct, "ocl_" #ExtType); \
 break;
 

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-11-13 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D52674#1297879, @rjmccall wrote:

> I'm not worried about the mangler being re-used for multiple declarations, 
> I'm worried about a global flag changing how we mangle all components of a 
> type when we only mean to change it at the top level.


Hmm, but don't we want it to affect all components? For example, consider 
something like:

  @interface I
  @end
  
  template  class C {};
  
  void f();
  void g() {
try {
  f();
} catch (C *) {
}
  }

I would say that we want the RTTI for `C *` to have the discriminator for 
`I`. It turns out my current patch doesn't actually do that; I guess there's a 
sub-mangler being constructed somewhere that's not inheriting the RTTI-ness. Or 
did you mean something else?


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-11-13 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

@rnk pointed out on IRC that the MicrosoftCXXNameMangler is actually 
specifically designed to manage the mangling of only a single name, in which 
case adding state to it for handling RTTI seems like a natural approach. 
@rjmccall, what do you think? I think this is much cleaner than having to 
thread through the RTTI state to every individual method. The ForRTTI_t enum is 
modeled after the ForDefinition_t enum used in CGM, but I'm happy to switch to 
a more general struct (as you'd mentioned before) if you prefer.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-11-13 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai updated this revision to Diff 173965.
smeenai added a comment.

Stateful approach


Repository:
  rC Clang

https://reviews.llvm.org/D52674

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm

Index: test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -fobjc-runtime=gnustep-2.0 -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefix=X86 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fobjc-runtime=gnustep-2.0 -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefix=X64 %s
+
+// Ensure we have the __ObjC::class discriminator in the RTTI and the RTTI name.
+// X86-DAG: @"??_R0PAU?$Class@Uobjc_object@@@__ObjC@@@8" = linkonce_odr global %{{[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [36 x i8] c".PAU?$Class@Uobjc_object@@@__ObjC@@\00" }, comdat
+// X64-DAG: @"??_R0PEAU?$Class@Uobjc_object@@@__ObjC@@@8" = linkonce_odr global %{{[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [37 x i8] c".PEAU?$Class@Uobjc_object@@@__ObjC@@\00" }, comdat
+
+@class I;
+// X86-DAG: @"??_R0PAU?$Class@UI@@@__ObjC@@@8" = linkonce_odr global %{{[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [26 x i8] c".PAU?$Class@UI@@@__ObjC@@\00" }, comdat
+// X64-DAG: @"??_R0PEAU?$Class@UI@@@__ObjC@@@8" = linkonce_odr global %{{[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [27 x i8] c".PEAU?$Class@UI@@@__ObjC@@\00" }, comdat
+
+void f();
+void g() {
+  @try {
+f();
+  } @catch (I *) {
+  } @catch (id) {
+  }
+}
Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -252,6 +252,11 @@
 /// MicrosoftCXXNameMangler - Manage the mangling of a single name for the
 /// Microsoft Visual C++ ABI.
 class MicrosoftCXXNameMangler {
+public:
+  enum QualifierMangleMode { QMM_Drop, QMM_Mangle, QMM_Escape, QMM_Result };
+  enum ForRTTI_t : bool { NotForRTTI = false, ForRTTI = true };
+
+private:
   MicrosoftMangleContextImpl 
   raw_ostream 
 
@@ -276,25 +281,35 @@
   // this check into mangleQualifiers().
   const bool PointersAre64Bit;
 
-public:
-  enum QualifierMangleMode { QMM_Drop, QMM_Mangle, QMM_Escape, QMM_Result };
+  ForRTTI_t IsForRTTI;
 
+public:
   MicrosoftCXXNameMangler(MicrosoftMangleContextImpl , raw_ostream _)
   : Context(C), Out(Out_), Structor(nullptr), StructorType(-1),
 PointersAre64Bit(C.getASTContext().getTargetInfo().getPointerWidth(0) ==
- 64) {}
+ 64),
+IsForRTTI(NotForRTTI) {}
 
   MicrosoftCXXNameMangler(MicrosoftMangleContextImpl , raw_ostream _,
   const CXXConstructorDecl *D, CXXCtorType Type)
   : Context(C), Out(Out_), Structor(getStructor(D)), StructorType(Type),
 PointersAre64Bit(C.getASTContext().getTargetInfo().getPointerWidth(0) ==
- 64) {}
+ 64),
+IsForRTTI(NotForRTTI) {}
 
   MicrosoftCXXNameMangler(MicrosoftMangleContextImpl , raw_ostream _,
   const CXXDestructorDecl *D, CXXDtorType Type)
   : Context(C), Out(Out_), Structor(getStructor(D)), StructorType(Type),
 PointersAre64Bit(C.getASTContext().getTargetInfo().getPointerWidth(0) ==
- 64) {}
+ 64),
+IsForRTTI(NotForRTTI) {}
+
+  MicrosoftCXXNameMangler(MicrosoftMangleContextImpl , raw_ostream _,
+  ForRTTI_t IsForRTTI)
+  : Context(C), Out(Out_), Structor(nullptr), StructorType(-1),
+PointersAre64Bit(C.getASTContext().getTargetInfo().getPointerWidth(0) ==
+ 64),
+IsForRTTI(IsForRTTI) {}
 
   raw_ostream () const { return Out; }
 
@@ -339,6 +354,7 @@
   void
   mangleTemplateInstantiationName(const TemplateDecl *TD,
   const TemplateArgumentList );
+  void mangleObjCClassName(StringRef Name);
   void mangleObjCMethodName(const ObjCMethodDecl *MD);
 
   void mangleArgumentType(QualType T, SourceRange Range);
@@ -1277,6 +1293,27 @@
   }
 }
 
+void MicrosoftCXXNameMangler::mangleObjCClassName(StringRef Name) {
+  // Obj-C classes are normally mangled as C++ structs with the same name, but
+  // we want to be able to distinguish a C++ struct X from an Obj-C class X for
+  // the purposes of exception handling, so we add a discriminator when mangling
+  // for RTTI.
+  if (!IsForRTTI) {
+mangleArtificalTagType(TTK_Struct, Name);
+return;
+  }
+
+  llvm::SmallString<64> TemplateMangling;
+  llvm::raw_svector_ostream Stream(TemplateMangling);
+  MicrosoftCXXNameMangler Extra(Context, Stream);
+
+  Stream << "?$";
+  Extra.mangleSourceName("Class");
+  Extra.mangleArtificalTagType(TTK_Struct, Name);
+
+  

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-11-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D52674#1293180, @rjmccall wrote:

> In https://reviews.llvm.org/D52674#1291284, @smeenai wrote:
>
> > In https://reviews.llvm.org/D52674#1281747, @rjmccall wrote:
> >
> > > In https://reviews.llvm.org/D52674#1271814, @smeenai wrote:
> > >
> > > > @rjmccall I prototyped the ForRTTI parameter approach in 
> > > > https://reviews.llvm.org/D53546. It could definitely be cleaned up a 
> > > > bit, but it demonstrates the problems I saw with the added parameter. 
> > > > Namely, `mangleType(QualType, SourceRange, QualifierMangleMode)` has a 
> > > > bunch of additional logic which is needed for correctness, so we need 
> > > > to thread the parameter through the entire chain, and the only way I 
> > > > could think of doing that without adding the parameter to every single 
> > > > `mangleType` overload was to have an additional switch to dispatch to 
> > > > the overloads with the added ForRTTI parameter, which is pretty ugly.
> > > >
> > > > As I see it, there are a few ways to proceed here:
> > > >
> > > > - The approach in https://reviews.llvm.org/D53546 (cleaned up a bit). I 
> > > > think it's pretty ugly, but you may have suggestions on how to do it 
> > > > better.
> > > > - In the `mangleType` overload for `ObjCObjectPointerType`, skipping 
> > > > the generic `mangleType` dispatch and going directly to either the 
> > > > `ObjCObjectType` or `ObjCInterfaceType` overloads. That avoids the 
> > > > ugliness in the generic `mangleType`, but it also requires us to 
> > > > duplicate some logic from it in the `ObjCObjectPointerType` overload, 
> > > > which doesn't seem great either.
> > > > - Maintaining `ForRTTI` state in the mangler instead of threading a 
> > > > parameter through (I'm generally not a fan of state like that, but it 
> > > > might be cleaner than the threading in this case?)
> > > > - Just sticking with what I'm doing in this patch.
> > > >
> > > >   What do you think?
> > >
> > >
> > > Sorry for the delay.  I think duplicating the dispatch logic for 
> > > `ObjCObjectPointerType` is probably the best approach; the pointee type 
> > > will always an `ObjCObjectType` of some sort, and there are only two 
> > > kinds of those.
> >
> >
> > Sorry, I'm still not sure how this will work.
> >
> > Duplicating the dispatch logic for `ObjCObjectPointerType` ends up looking 
> > like https://reviews.llvm.org/P8114, which is fine. However, when we're 
> > actually mangling RTTI or RTTI names, we're still going through the main 
> > `mangleType(QualType, SourceRange, QualifierMangleMode)` overload, which 
> > still requires us to thread `ForRTTI` through that function, which still 
> > requires us to duplicate the switch in that function (to handle the 
> > `ForRTTI` case, since the other switch is generated via TypeNodes.def and 
> > not easily modifiable), which is the main ugliness in my opinion. Do you 
> > also want me to add special dispatching to `mangleCXXRTTI` and 
> > `mangleCXXRTTIName` to just call the `ObjCObjectPointerType` function 
> > directly when they're dealing with that type? That's certainly doable, but 
> > at that point just keeping some state around in the demangler starts to 
> > feel cleaner, at least to me.
>
>
> Well, you could check for an `ObjCObjectPointerType` in the top-level routine.
>
> But yeah, probably the cleanest thing to do would to be push the state 
> through the main dispatch.  Don't push a single `bool` through, though; make 
> a `TypeManglingOptions` struct to encapsulate it, in case we have a similar 
> problem elsewhere.  It should have a method for getting options that should 
> be propagated down to component type manglings, and that method can drop the 
> "for RTTI" bit; that's the part that `ObjCObjectPointerType` can handle 
> differently.


All right, the struct is a good idea. It would require adding the 
`TypeManglingOptions` parameter to all the `mangleType` overloads (unless we 
want to stick with the second switch in the main `mangleType` dispatch 
function, but that seems super ugly, especially if we're doing a more general 
solution), so I wanted to confirm @rnk is okay with adding a parameter to all 
those overloads as well before proceeding.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-11-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Sorry, had a leftover draft which I forgot to clean up. Edited in Phabricator 
now.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-11-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D52674#1281747, @rjmccall wrote:

> In https://reviews.llvm.org/D52674#1271814, @smeenai wrote:
>
> > @rjmccall I prototyped the ForRTTI parameter approach in 
> > https://reviews.llvm.org/D53546. It could definitely be cleaned up a bit, 
> > but it demonstrates the problems I saw with the added parameter. Namely, 
> > `mangleType(QualType, SourceRange, QualifierMangleMode)` has a bunch of 
> > additional logic which is needed for correctness, so we need to thread the 
> > parameter through the entire chain, and the only way I could think of doing 
> > that without adding the parameter to every single `mangleType` overload was 
> > to have an additional switch to dispatch to the overloads with the added 
> > ForRTTI parameter, which is pretty ugly.
> >
> > As I see it, there are a few ways to proceed here:
> >
> > - The approach in https://reviews.llvm.org/D53546 (cleaned up a bit). I 
> > think it's pretty ugly, but you may have suggestions on how to do it better.
> > - In the `mangleType` overload for `ObjCObjectPointerType`, skipping the 
> > generic `mangleType` dispatch and going directly to either the 
> > `ObjCObjectType` or `ObjCInterfaceType` overloads. That avoids the ugliness 
> > in the generic `mangleType`, but it also requires us to duplicate some 
> > logic from it in the `ObjCObjectPointerType` overload, which doesn't seem 
> > great either.
> > - Maintaining `ForRTTI` state in the mangler instead of threading a 
> > parameter through (I'm generally not a fan of state like that, but it might 
> > be cleaner than the threading in this case?)
> > - Just sticking with what I'm doing in this patch.
> >
> >   What do you think?
>
>
> Sorry for the delay.  I think duplicating the dispatch logic for 
> `ObjCObjectPointerType` is probably the best approach; the pointee type will 
> always an `ObjCObjectType` of some sort, and there are only two kinds of 
> those.


To be clear, we would need to duplicate (or factor out into a common function) 
some mangling logic as well, because e.g. adding the `E`

In https://reviews.llvm.org/D52674#1281747, @rjmccall wrote:

> In https://reviews.llvm.org/D52674#1271814, @smeenai wrote:
>
> > @rjmccall I prototyped the ForRTTI parameter approach in 
> > https://reviews.llvm.org/D53546. It could definitely be cleaned up a bit, 
> > but it demonstrates the problems I saw with the added parameter. Namely, 
> > `mangleType(QualType, SourceRange, QualifierMangleMode)` has a bunch of 
> > additional logic which is needed for correctness, so we need to thread the 
> > parameter through the entire chain, and the only way I could think of doing 
> > that without adding the parameter to every single `mangleType` overload was 
> > to have an additional switch to dispatch to the overloads with the added 
> > ForRTTI parameter, which is pretty ugly.
> >
> > As I see it, there are a few ways to proceed here:
> >
> > - The approach in https://reviews.llvm.org/D53546 (cleaned up a bit). I 
> > think it's pretty ugly, but you may have suggestions on how to do it better.
> > - In the `mangleType` overload for `ObjCObjectPointerType`, skipping the 
> > generic `mangleType` dispatch and going directly to either the 
> > `ObjCObjectType` or `ObjCInterfaceType` overloads. That avoids the ugliness 
> > in the generic `mangleType`, but it also requires us to duplicate some 
> > logic from it in the `ObjCObjectPointerType` overload, which doesn't seem 
> > great either.
> > - Maintaining `ForRTTI` state in the mangler instead of threading a 
> > parameter through (I'm generally not a fan of state like that, but it might 
> > be cleaner than the threading in this case?)
> > - Just sticking with what I'm doing in this patch.
> >
> >   What do you think?
>
>
> Sorry for the delay.  I think duplicating the dispatch logic for 
> `ObjCObjectPointerType` is probably the best approach; the pointee type will 
> always an `ObjCObjectType` of some sort, and there are only two kinds of 
> those.


Sorry, I'm still not sure how this will work.

Duplicating the dispatch logic for `ObjCObjectPointerType` ends up looking like 
https://reviews.llvm.org/P8114, which is fine. However, when we're actually 
mangling RTTI or RTTI names, we're still going through the main 
`mangleType(QualType, SourceRange, QualifierMangleMode)` overload, which still 
requires us to thread `ForRTTI` through that function, which still requires us 
to duplicate the switch in that function (to handle the `ForRTTI` case, since 
the other switch is generated via TypeNodes.def and not easily modifiable), 
which is the main ugliness in my opinion. Do you also want me to add special 
dispatching to `mangleCXXRTTI` and `mangleCXXRTTIName` to just call the 
`ObjCObjectPointerType` function directly when they're dealing with that type? 
That's certainly doable, but at that point just keeping some state around in 
the demangler starts to feel cleaner, at least to me.


Repository:
  

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-10-30 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Ping @rjmccall. Let me know if the approach in https://reviews.llvm.org/D53546 
is what you'd been envisioning, or if I'm just doing something completely 
brain-dead :)


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-10-22 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

@rjmccall I prototyped the ForRTTI parameter approach in 
https://reviews.llvm.org/D53546. It could definitely be cleaned up a bit, but 
it demonstrates the problems I saw with the added parameter. Namely, 
`mangleType(QualType, SourceRange, QualifierMangleMode)` has a bunch of 
additional logic which is needed for correctness, so we need to thread the 
parameter through the entire chain, and the only way I could think of doing 
that without adding the parameter to every single `mangleType` overload was to 
have an additional switch to dispatch to the overloads with the added ForRTTI 
parameter, which is pretty ugly.

As I see it, there are a few ways to proceed here:

- The approach in https://reviews.llvm.org/D53546 (cleaned up a bit). I think 
it's pretty ugly, but you may have suggestions on how to do it better.
- In the `mangleType` overload for `ObjCObjectPointerType`, skipping the 
generic `mangleType` dispatch and going directly to either the `ObjCObjectType` 
or `ObjCInterfaceType` overloads. That avoids the ugliness in the generic 
`mangleType`, but it also requires us to duplicate some logic from it in the 
`ObjCObjectPointerType` overload, which doesn't seem great either.
- Maintaining `ForRTTI` state in the mangler instead of threading a parameter 
through (I'm generally not a fan of state like that, but it might be cleaner 
than the threading in this case?)
- Just sticking with what I'm doing in this patch.

What do you think?


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52990: [MinGW] Allow using ubsan

2018-10-08 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: lib/Driver/ToolChains/MinGW.cpp:266
+  // directives in the object files, but the static library needs
+  // -lpsapi unless the sanitizer was built targeting >= win7.
+  CmdArgs.push_back("-lpsapi");

rnk wrote:
> mstorsjo wrote:
> > smeenai wrote:
> > > Isn't Windows 7 our minimum supported Windows version anyway? I can't 
> > > find the documentation pointing to that, but I thought that was the 
> > > policy. In particular, we require VS 2015 or above, which doesn't support 
> > > anything older than Server 2008 anyway (including Vista and XP), and I 
> > > doubt we'd have anyone using Server 2008.
> > For llvm itself, yes, but this is for the runtimes. Or does the policy 
> > cover that as well?
> > 
> > Mingw upstream still(!) default to supporting xp onwards, while I'm 
> > configuring my own setups to default to vista. I guess making that 7 
> > wouldn't be too much of an issue though.
> > 
> > FWIW, I included a corresponding change for ASAN here: 
> > https://reviews.llvm.org/rCRT343074, in adding a `append_list_if(MINGW 
> > psapi ASAN_DYNAMIC_LIBS)` in compiler-rt/trunk/lib/asan/CMakeLists.txt.
> I think most of our policies have to do with LLVM's own build requirements. 
> Clang has supported targeting older OSs for a while, and we shouldn't 
> intentionally break it, at least. We probably don't want to support running 
> the sanitizers on old Windows OSs, since they tend to want to use modern OS 
> APIs. For example, I used the slim reader writer lock APIs in ASan to fix a 
> race.
> 
> In any case, do you think it would be nicer to put this directive in the 
> ubsan object files that use psapi? It seems lame for clang to have to know 
> about what libraries the sanitizers use.
Ah, I wasn't thinking of the LLVM/runtimes distinction. SRWLocks would require 
Vista and above though, and at that point just going for 7 and above would make 
sense.

This is a bit orthogonal to the patch though, so sorry for side-tracking 
things. +1 for Reid's suggestion of embedding the psapi directive in object 
files.


Repository:
  rC Clang

https://reviews.llvm.org/D52990



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


[PATCH] D52990: [MinGW] Allow using ubsan

2018-10-08 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: lib/Driver/ToolChains/MinGW.cpp:266
+  // directives in the object files, but the static library needs
+  // -lpsapi unless the sanitizer was built targeting >= win7.
+  CmdArgs.push_back("-lpsapi");

Isn't Windows 7 our minimum supported Windows version anyway? I can't find the 
documentation pointing to that, but I thought that was the policy. In 
particular, we require VS 2015 or above, which doesn't support anything older 
than Server 2008 anyway (including Vista and XP), and I doubt we'd have anyone 
using Server 2008.


Repository:
  rC Clang

https://reviews.llvm.org/D52990



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-10-04 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D52674#1253408, @rjmccall wrote:

> In https://reviews.llvm.org/D52674#1253401, @smeenai wrote:
>
> > Actually, I take that back ... I just misread the stack trace.
> >
> > There are a bunch of hops between the `mangleCXXRTTI` call and the ultimate 
> > mangling function:
> >
> >   MicrosoftMangleContextImpl::mangleCXXRTTI(QualType, raw_ostream &)
> >   MicrosoftCXXNameMangler::mangleType(QualType, SourceRange, 
> > QualifierMangleMode)
> >   MicrosoftCXXNameMangler::mangleType(const ObjCObjectPointerType *, 
> > Qualifiers, SourceRange)
> >   MicrosoftCXXNameMangler::mangleType(QualType, SourceRange, 
> > QualifierMangleMode)
> >   MicrosoftCXXNameMangler::mangleType(const ObjCObjectType *, Qualifiers, 
> > SourceRange)
> >
> >
> > (the last one will be `ObjCInterfaceType` instead of `ObjCObjectType` if 
> > catching anything other than `id`)
> >
> > Threading a `ForRTTI` flag or similar all the way to the final call seems 
> > pretty tricky. I can add an optional paramater to `mangleType(QualType, 
> > SourceRange, QualifierMangleMode)`, but that function uses a generated 
> > switch case 
> > 
> >  to call the specific `mangleType` functions, and I don't know how to 
> > special-case certain types in that switch case (to pass the extra parameter 
> > along) without doing something super ugly. Adding the extra parameter to 
> > every single `mangleType` overload seems highly non-ideal, which is why I 
> > was thinking of maintaining some internal state instead.
>
>
> Well, that's why I was talking about how the pointee type of an 
> `ObjCObjectPointerType` is always an `ObjCObjectType` (of which 
> `ObjCInterfaceType` is a subclass) — the implication being that you don't 
> actually have to do the `switch` to dispatch to one of those two cases.


The intermediate `mangleType(QualType, SourceRange, QualifierMangleMode)` calls 
have a bunch of logic 

 in them; it's not just a direct dispatch to the actual mangling function, so I 
don't think we can skip over it. I think it's hard to discuss this in the 
abstract though (and it's also entirely possible I'm missing your point 
completely), so I'll just actually try out the parameter approach and put up 
the resulting patch and we can see how it turns out.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52581: [AST] Revert mangling changes from r339428

2018-10-04 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC343808: [AST] Revert mangling changes from r339428 (authored 
by smeenai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52581?vs=167219=168354#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52581

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenObjCXX/arc-marker-funclet.mm
  test/CodeGenObjCXX/microsoft-abi-arc-param-order.mm
  test/CodeGenObjCXX/msabi-objc-extensions.mm
  test/CodeGenObjCXX/msabi-objc-types.mm

Index: test/CodeGenObjCXX/msabi-objc-extensions.mm
===
--- test/CodeGenObjCXX/msabi-objc-extensions.mm
+++ test/CodeGenObjCXX/msabi-objc-extensions.mm
@@ -7,92 +7,91 @@
 @class J;
 
 void f(id, id, id, id) {}
-// CHECK-LABEL: "?f@@YAXPAU?$.objc_object@U?$Protocol@UP@@@__ObjCPAU.objc_object@@01@Z"
+// CHECK-LABEL: "?f@@YAXPAU?$objc_object@U?$Protocol@UP@@@__ObjCPAUobjc_object@@01@Z"
 
 void f(id, id, id, id) {}
-// CHECK-LABEL: "?f@@YAXPAU.objc_object@@PAU?$.objc_object@U?$Protocol@UP@@@__ObjC10@Z"
+// CHECK-LABEL: "?f@@YAXPAUobjc_object@@PAU?$objc_object@U?$Protocol@UP@@@__ObjC10@Z"
 
 void f(id, id) {}
-// CHECK-LABEL: "?f@@YAXPAU?$.objc_object@U?$Protocol@UP@@@__ObjC0@Z"
+// CHECK-LABEL: "?f@@YAXPAU?$objc_object@U?$Protocol@UP@@@__ObjC0@Z"
 
 void f(id) {}
-// CHECK-LABEL: "?f@@YAXPAU?$.objc_object@U?$Protocol@UP@@@__ObjC@Z"
+// CHECK-LABEL: "?f@@YAXPAU?$objc_object@U?$Protocol@UP@@@__ObjC@Z"
 
 void f(id) {}
-// CHECK-LABEL: "?f@@YAXPAU?$.objc_object@U?$Protocol@UP@@@__ObjC@@U?$Protocol@UQ@@@2Z"
+// CHECK-LABEL: "?f@@YAXPAU?$objc_object@U?$Protocol@UP@@@__ObjC@@U?$Protocol@UQ@@@2Z"
 
 void f(Class) {}
-// CHECK-LABEL: "?f@@YAXPAU?$.objc_class@U?$Protocol@UP@@@__ObjC@Z"
+// CHECK-LABEL: "?f@@YAXPAU?$objc_class@U?$Protocol@UP@@@__ObjC@Z"
 
 void f(Class) {}
-// CHECK-LABEL: "?f@@YAXPAU?$.objc_class@U?$Protocol@UP@@@__ObjC@@U?$Protocol@UQ@@@2Z"
+// CHECK-LABEL: "?f@@YAXPAU?$objc_class@U?$Protocol@UP@@@__ObjC@@U?$Protocol@UQ@@@2Z"
 
 void f(I *) {}
-// CHECK-LABEL: "?f@@YAXPAU?$.objc_cls_I@U?$Protocol@UP@@@__ObjC@Z"
+// CHECK-LABEL: "?f@@YAXPAU?$I@U?$Protocol@UP@@@__ObjC@Z"
 
 void f(I *) {}
-// CHECK-LABEL: "?f@@YAXPAU?$.objc_cls_I@U?$Protocol@UP@@@__ObjC@@U?$Protocol@UQ@@@2Z"
+// CHECK-LABEL: "?f@@YAXPAU?$I@U?$Protocol@UP@@@__ObjC@@U?$Protocol@UQ@@@2Z"
 
 template 
 struct S {};
 
 void f(S<__unsafe_unretained id>) {}
-// CHECK-LABEL: "?f@@YAXU?$S@PAU.objc_object@Z"
+// CHECK-LABEL: "?f@@YAXU?$S@PAUobjc_object@Z"
 
 void f(S<__autoreleasing id>) {}
-// CHECK-LABEL: "?f@@YAXU?$S@U?$Autoreleasing@PAU.objc_object@@@__ObjC@Z"
+// CHECK-LABEL: "?f@@YAXU?$S@U?$Autoreleasing@PAUobjc_object@@@__ObjC@Z"
 
 void f(S<__strong id>) {}
-// CHECK-LABEL: "?f@@YAXU?$S@U?$Strong@PAU.objc_object@@@__ObjC@Z"
+// CHECK-LABEL: "?f@@YAXU?$S@U?$Strong@PAUobjc_object@@@__ObjC@Z"
 
 void f(S<__weak id>) {}
-// CHECK-LABEL: "?f@@YAXU?$S@U?$Weak@PAU.objc_object@@@__ObjC@Z"
+// CHECK-LABEL: "?f@@YAXU?$S@U?$Weak@PAUobjc_object@@@__ObjC@Z"
 
 void w(__weak id) {}
-// CHECK-LABEL: "?w@@YAXPAU.objc_object@@@Z"
+// CHECK-LABEL: "?w@@YAXPAUobjc_object@@@Z"
 
 void s(__strong id) {}
-// CHECK-LABEL: "?s@@YAXPAU.objc_object@@@Z"
+// CHECK-LABEL: "?s@@YAXPAUobjc_object@@@Z"
 
 void a(__autoreleasing id) {}
-// CHECK-LABEL: "?a@@YAXPAU.objc_object@@@Z"
+// CHECK-LABEL: "?a@@YAXPAUobjc_object@@@Z"
 
 void u(__unsafe_unretained id) {}
-// CHECK-LABEL: "?u@@YAXPAU.objc_object@@@Z"
+// CHECK-LABEL: "?u@@YAXPAUobjc_object@@@Z"
 
 S<__autoreleasing id> g() { return S<__autoreleasing id>(); }
-// CHECK-LABEL: "?g@@YA?AU?$S@U?$Autoreleasing@PAU.objc_object@@@__ObjCXZ"
+// CHECK-LABEL: "?g@@YA?AU?$S@U?$Autoreleasing@PAUobjc_object@@@__ObjCXZ"
 
 __autoreleasing id h() { return nullptr; }
-// CHECK-LABEL: "?h@@YAPAU.objc_object@@XZ"
+// CHECK-LABEL: "?h@@YAPAUobjc_object@@XZ"
 
 void f(I *) {}
-// CHECK-LABEL: "?f@@YAXPAU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXPAUI@@@Z"
 
 void f(__kindof I *) {}
-// CHECK-LABEL: "?f@@YAXPAU?$KindOf@U.objc_cls_I@@@__ObjC@@@Z"
+// CHECK-LABEL: "?f@@YAXPAU?$KindOf@UI@@@__ObjC@@@Z"
 
 void f(__kindof I *) {}
-// CHECK-LABEL: "?f@@YAXPAU?$KindOf@U?$.objc_cls_I@U?$Protocol@UP@@@__ObjC@__ObjC@@@Z"
+// CHECK-LABEL: "?f@@YAXPAU?$KindOf@U?$I@U?$Protocol@UP@@@__ObjC@__ObjC@@@Z"
 
 void f(S) {}
-// CHECK-LABEL: "?f@@YAXU?$S@U?$Strong@PAU.objc_cls_I@@@__ObjC@Z"
+// CHECK-LABEL: "?f@@YAXU?$S@U?$Strong@PAUI@@@__ObjC@Z"
 
 void f(S<__kindof I *>) {}
-// CHECK-LABEL: "?f@@YAXU?$S@U?$Strong@PAU?$KindOf@U.objc_cls_I@@@__ObjC@@@__ObjC@Z"
+// CHECK-LABEL: "?f@@YAXU?$S@U?$Strong@PAU?$KindOf@UI@@@__ObjC@@@__ObjC@Z"
 
 void f(S<__kindof I *>) {}
-// CHECK-LABEL: "?f@@YAXU?$S@U?$Strong@PAU?$KindOf@U?$.objc_cls_I@U?$Protocol@UP@@@__ObjC@__ObjC@@@__ObjC@Z"
+// CHECK-LABEL: 

[PATCH] D52843: Update Clang Windows getting started docs

2018-10-03 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: clang/www/get_started.html:197
+If you are using Visual Studio 2017:
+  cmake -G "Visual Studio 15 2017 Win64" ..\llvm
+This will generate x64 binaries by default, which should perform 
better.

Does the Win64 generator automatically imply `-Thost=x64`?


https://reviews.llvm.org/D52843



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-10-02 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai requested review of this revision.
smeenai added a comment.

Actually, I take that back ... I just misread the stack trace.

There are a bunch of hops between the `mangleCXXRTTI` call and the ultimate 
mangling function:

  MicrosoftMangleContextImpl::mangleCXXRTTI(QualType, raw_ostream &)
  MicrosoftCXXNameMangler::mangleType(QualType, SourceRange, 
QualifierMangleMode)
  MicrosoftCXXNameMangler::mangleType(const ObjCObjectPointerType *, 
Qualifiers, SourceRange)
  MicrosoftCXXNameMangler::mangleType(QualType, SourceRange, 
QualifierMangleMode)
  MicrosoftCXXNameMangler::mangleType(const ObjCObjectType *, Qualifiers, 
SourceRange)

(the last one will be `ObjCInterfaceType` instead of `ObjCObjectType` if 
catching anything other than `id`)

Threading a `ForRTTI` flag or similar all the way to the final call seems 
pretty tricky. I can add an optional paramater to `mangleType(QualType, 
SourceRange, QualifierMangleMode)`, but that function uses a generated switch 
case 

 to call the specific `mangleType` functions, and I don't know how to 
special-case certain types in that switch case (to pass the extra parameter 
along) without doing something super ugly. Adding the extra parameter to every 
single `mangleType` overload seems highly non-ideal, which is why I was 
thinking of maintaining some internal state instead.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-10-02 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai planned changes to this revision.
smeenai added a comment.

In https://reviews.llvm.org/D52674#1251931, @rjmccall wrote:

> In https://reviews.llvm.org/D52674#1251439, @smeenai wrote:
>
> > In https://reviews.llvm.org/D52674#1251419, @rjmccall wrote:
> >
> > > Conceptually this seems fine, but I think it would be good to stop and 
> > > make sure we're using a consistent style when mangling extensions.  
> > > Currently it feels like every patch to add a Clang extension to the 
> > > Microsoft mangling ends up inventing its own rules and crossing its 
> > > fingers.
> >
> >
> > That's a fair concern.
> >
> > I believe most of the Obj-C extensions have been handled by @compnerd, and 
> > he's been following a pretty consistent scheme using the `__Objc` 
> > namespace, e.g. `void f(id) {}` is mangled as `void __cdecl f(struct 
> > objc_object > *)`. I could certainly try 
> > to implement something similar here, except as I mentioned, I'm pretty sure 
> > it would require maintaining some state in the demangler for indicating 
> > whether we were mangling for RTTI.
>
>
> State wouldn't really be the right solution anyway, although it might work 
> because of the structural restrictions on Objective-C types.  If you wanted 
> to keep that same rule, I think you could probably just pass a 
> (default=`false`) flag down to the `ObjCObjectPointerType`, `ObjCObjectType`, 
> and `ObjCInterfaceType` cases.  (The pointee type of the former will always 
> be one of the latter two.)


Ah, there are fewer hops between the `mangleType` call and its ultimate 
destination than I was expecting, so a default parameter would work. I'll 
change this accordingly, thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-10-01 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D52674#1251419, @rjmccall wrote:

> Conceptually this seems fine, but I think it would be good to stop and make 
> sure we're using a consistent style when mangling extensions.  Currently it 
> feels like every patch to add a Clang extension to the Microsoft mangling 
> ends up inventing its own rules and crossing its fingers.


That's a fair concern.

I believe most of the Obj-C extensions have been handled by @compnerd, and he's 
been following a pretty consistent scheme using the `__Objc` namespace, e.g. 
`void f(id) {}` is mangled as `void __cdecl f(struct objc_object > *)`. I could certainly try to implement something 
similar here, except as I mentioned, I'm pretty sure it would require 
maintaining some state in the demangler for indicating whether we were mangling 
for RTTI.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-09-28 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai updated this revision to Diff 167555.
smeenai added a comment.

arc fail


Repository:
  rC Clang

https://reviews.llvm.org/D52674

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm


Index: test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -fobjc-runtime=gnustep-2.0 
-fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefix=X86 
%s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fobjc-runtime=gnustep-2.0 
-fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefix=X64 
%s
+
+// Ensure we have the .objc discriminator in the RTTI and the RTTI name.
+// X86-DAG: @"??_R0objc.PAUobjc_object@@@8" = linkonce_odr global %{{[^ ]+}} { 
i8** @"??_7type_info@@6B@", i8* null, [23 x i8] c".objc.PAUobjc_object@@\00" }, 
comdat
+// X64-DAG: @"??_R0objc.PEAUobjc_object@@@8" = linkonce_odr global %{{[^ ]+}} 
{ i8** @"??_7type_info@@6B@", i8* null, [24 x i8] c".objc.PEAUobjc_object@@\00" 
}, comdat
+
+@class I;
+// X86-DAG: @"??_R0objc.PAUI@@@8" = linkonce_odr global %{{[^ ]+}} { i8** 
@"??_7type_info@@6B@", i8* null, [13 x i8] c".objc.PAUI@@\00" }, comdat
+// X64-DAG: @"??_R0objc.PEAUI@@@8" = linkonce_odr global %{{[^ ]+}} { i8** 
@"??_7type_info@@6B@", i8* null, [14 x i8] c".objc.PEAUI@@\00" }, comdat
+
+void f();
+void g() {
+  @try {
+f();
+  } @catch (I *) {
+  } @catch (id) {
+  }
+}
Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -3001,14 +3001,22 @@
   msvc_hashing_ostream MHO(Out);
   MicrosoftCXXNameMangler Mangler(*this, MHO);
   Mangler.getStream() << "??_R0";
+  // Obj-C classes are mangled as C++ structs with the same name, but we want 
to
+  // be able to distinguish a C++ struct X from an Obj-C class X for the
+  // purposes of exception handling, so we add a discriminator.
+  if (T->isObjCObjectPointerType())
+Mangler.getStream() << "objc.";
   Mangler.mangleType(T, SourceRange(), MicrosoftCXXNameMangler::QMM_Result);
   Mangler.getStream() << "@8";
 }
 
 void MicrosoftMangleContextImpl::mangleCXXRTTIName(QualType T,
raw_ostream ) {
   MicrosoftCXXNameMangler Mangler(*this, Out);
   Mangler.getStream() << '.';
+  // See the comment in MicrosoftMangleContextImpl::mangleCXXRTTI.
+  if (T->isObjCObjectPointerType())
+Mangler.getStream() << "objc.";
   Mangler.mangleType(T, SourceRange(), MicrosoftCXXNameMangler::QMM_Result);
 }
 


Index: test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple i686-windows-msvc -fobjc-runtime=gnustep-2.0 -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefix=X86 %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -fobjc-runtime=gnustep-2.0 -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefix=X64 %s
+
+// Ensure we have the .objc discriminator in the RTTI and the RTTI name.
+// X86-DAG: @"??_R0objc.PAUobjc_object@@@8" = linkonce_odr global %{{[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [23 x i8] c".objc.PAUobjc_object@@\00" }, comdat
+// X64-DAG: @"??_R0objc.PEAUobjc_object@@@8" = linkonce_odr global %{{[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [24 x i8] c".objc.PEAUobjc_object@@\00" }, comdat
+
+@class I;
+// X86-DAG: @"??_R0objc.PAUI@@@8" = linkonce_odr global %{{[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [13 x i8] c".objc.PAUI@@\00" }, comdat
+// X64-DAG: @"??_R0objc.PEAUI@@@8" = linkonce_odr global %{{[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [14 x i8] c".objc.PEAUI@@\00" }, comdat
+
+void f();
+void g() {
+  @try {
+f();
+  } @catch (I *) {
+  } @catch (id) {
+  }
+}
Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -3001,14 +3001,22 @@
   msvc_hashing_ostream MHO(Out);
   MicrosoftCXXNameMangler Mangler(*this, MHO);
   Mangler.getStream() << "??_R0";
+  // Obj-C classes are mangled as C++ structs with the same name, but we want to
+  // be able to distinguish a C++ struct X from an Obj-C class X for the
+  // purposes of exception handling, so we add a discriminator.
+  if (T->isObjCObjectPointerType())
+Mangler.getStream() << "objc.";
   Mangler.mangleType(T, SourceRange(), MicrosoftCXXNameMangler::QMM_Result);
   Mangler.getStream() << "@8";
 }
 
 void MicrosoftMangleContextImpl::mangleCXXRTTIName(QualType T,
raw_ostream ) {
   MicrosoftCXXNameMangler Mangler(*this, Out);
   

[PATCH] D52581: [AST] Revert mangling changes from r339428

2018-09-28 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

https://reviews.llvm.org/D52674 adds the RTTI Obj-C discriminator. I put it up 
as a separate change so it could be reviewed independently and more thoroughly.


Repository:
  rC Clang

https://reviews.llvm.org/D52581



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


[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

2018-09-28 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: compnerd, DHowett-MSFT, rjmccall, rnk, theraven.
Herald added a subscriber: erik.pilkington.

Obj-C classes are mangled as C++ structs with the same name (in both the
Itanium and the Microsoft ABIs), but we want to be able to distinguish
them for the purposes of exception handling, so we need to add a
discriminator to the RTTI and the RTTI name.

The chosen discriminator (`.objc`) is fairly arbitrary, and I'm open to
other suggestions. It's also perhaps not ideal to have the discriminator
as a prefix, since then the RTTI won't demangle (RTTI names never
demangle anyway, so that's less of a concern). Having it infix would
make for a cleaner mangling, but it would also require keeping some
state around in the mangler to indicate when we're mangling for RTTI,
which seems like a much uglier implementation and not worth it.


Repository:
  rC Clang

https://reviews.llvm.org/D52674

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenObjCXX/arc-marker-funclet.mm
  test/CodeGenObjCXX/microsoft-abi-arc-param-order.mm
  test/CodeGenObjCXX/msabi-objc-exceptions-gnustep.mm
  test/CodeGenObjCXX/msabi-objc-extensions.mm
  test/CodeGenObjCXX/msabi-objc-types.mm

Index: test/CodeGenObjCXX/msabi-objc-types.mm
===
--- test/CodeGenObjCXX/msabi-objc-types.mm
+++ test/CodeGenObjCXX/msabi-objc-types.mm
@@ -3,166 +3,166 @@
 @class I;
 
 id kid;
-// CHECK: @"?kid@@3PAU.objc_object@@A" =  dso_local global
+// CHECK: @"?kid@@3PAUobjc_object@@A" =  dso_local global
 
 Class klass;
-// CHECK: @"?klass@@3PAU.objc_class@@A" = dso_local global
+// CHECK: @"?klass@@3PAUobjc_class@@A" = dso_local global
 
 I *kI;
-// CHECK: @"?kI@@3PAU.objc_cls_I@@A" = dso_local global
+// CHECK: @"?kI@@3PAUI@@A" = dso_local global
 
 void f(I *) {}
-// CHECK-LABEL: "?f@@YAXPAU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXPAUI@@@Z"
 
 void f(const I *) {}
-// CHECK-LABEL: "?f@@YAXPBU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXPBUI@@@Z"
 
 void f(I &) {}
-// CHECK-LABEL: "?f@@YAXAAU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXAAUI@@@Z"
 
 void f(const I &) {}
-// CHECK-LABEL: "?f@@YAXABU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXABUI@@@Z"
 
 void f(const I &&) {}
-// CHECK-LABEL: "?f@@YAX$$QBU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAX$$QBUI@@@Z"
 
 void g(id) {}
-// CHECK-LABEL: "?g@@YAXPAU.objc_object@@@Z"
+// CHECK-LABEL: "?g@@YAXPAUobjc_object@@@Z"
 
 void g(id &) {}
-// CHECK-LABEL: "?g@@YAXAAPAU.objc_object@@@Z"
+// CHECK-LABEL: "?g@@YAXAAPAUobjc_object@@@Z"
 
 void g(const id &) {}
-// CHECK-LABEL: "?g@@YAXABQAU.objc_object@@@Z"
+// CHECK-LABEL: "?g@@YAXABQAUobjc_object@@@Z"
 
 void g(id &&) {}
-// CHECK-LABEL: "?g@@YAX$$QAPAU.objc_object@@@Z"
+// CHECK-LABEL: "?g@@YAX$$QAPAUobjc_object@@@Z"
 
 void h(Class) {}
-// CHECK-LABEL: "?h@@YAXPAU.objc_class@@@Z"
+// CHECK-LABEL: "?h@@YAXPAUobjc_class@@@Z"
 
 void h(Class &) {}
-// CHECK-LABEL: "?h@@YAXAAPAU.objc_class@@@Z"
+// CHECK-LABEL: "?h@@YAXAAPAUobjc_class@@@Z"
 
 void h(const Class &) {}
-// CHECK-LABEL: "?h@@YAXABQAU.objc_class@@@Z"
+// CHECK-LABEL: "?h@@YAXABQAUobjc_class@@@Z"
 
 void h(Class &&) {}
-// CHECK-LABEL: "?h@@YAX$$QAPAU.objc_class@@@Z"
+// CHECK-LABEL: "?h@@YAX$$QAPAUobjc_class@@@Z"
 
 I *i() { return nullptr; }
-// CHECK-LABEL: "?i@@YAPAU.objc_cls_I@@XZ"
+// CHECK-LABEL: "?i@@YAPAUI@@XZ"
 
 const I *j() { return nullptr; }
-// CHECK-LABEL: "?j@@YAPBU.objc_cls_I@@XZ"
+// CHECK-LABEL: "?j@@YAPBUI@@XZ"
 
 I () { return *kI; }
-// CHECK-LABEL: "?k@@YAAAU.objc_cls_I@@XZ"
+// CHECK-LABEL: "?k@@YAAAUI@@XZ"
 
 const I () { return *kI; }
-// CHECK-LABEL: "?l@@YAABU.objc_cls_I@@XZ"
+// CHECK-LABEL: "?l@@YAABUI@@XZ"
 
 void m(const id) {}
-// CHECK-LABEL: "?m@@YAXQAU.objc_object@@@Z"
+// CHECK-LABEL: "?m@@YAXQAUobjc_object@@@Z"
 
 void m(const I *) {}
-// CHECK-LABEL: "?m@@YAXPBU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?m@@YAXPBUI@@@Z"
 
 void n(SEL) {}
-// CHECK-LABEL: "?n@@YAXPAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXPAUobjc_selector@@@Z"
 
 void n(SEL *) {}
-// CHECK-LABEL: "?n@@YAXPAPAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXPAPAUobjc_selector@@@Z"
 
 void n(const SEL *) {}
-// CHECK-LABEL: "?n@@YAXPBQAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXPBQAUobjc_selector@@@Z"
 
 void n(SEL &) {}
-// CHECK-LABEL: "?n@@YAXAAPAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXAAPAUobjc_selector@@@Z"
 
 void n(const SEL &) {}
-// CHECK-LABEL: "?n@@YAXABQAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXABQAUobjc_selector@@@Z"
 
 void n(SEL &&) {}
-// CHECK-LABEL: "?n@@YAX$$QAPAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAX$$QAPAUobjc_selector@@@Z"
 
 struct __declspec(dllexport) s {
   struct s =(const struct s &) = delete;
 
   void m(I *) {}
-  // CHECK-LABEL: "?m@s@@QAAXPAU.objc_cls_I@@@Z"
+  // CHECK-LABEL: "?m@s@@QAAXPAUI@@@Z"
 
   void m(const I *) {}
-  // CHECK-LABEL: "?m@s@@QAAXPBU.objc_cls_I@@@Z"
+  // CHECK-LABEL: "?m@s@@QAAXPBUI@@@Z"
 
   void m(I &) {}
-  // CHECK-LABEL: 

[PATCH] D52664: Update CMakeLists.txt snippet so that example compiles

2018-09-28 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Yep, that's my doing (https://reviews.llvm.org/rL319840). I didn't think about 
the documentation, whoops.


Repository:
  rC Clang

https://reviews.llvm.org/D52664



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


[PATCH] D51714: CMake: Deprecate using llvm-config to detect llvm installation

2018-09-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Is there anything holding this up?


Repository:
  rC Clang

https://reviews.llvm.org/D51714



___
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

2018-09-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: lib/CodeGen/CGObjCMac.cpp:7457-7460
 CGObjCNonFragileABIMac::GetEHType(QualType T) {
   // There's a particular fixed type info for 'id'.
   if (T->isObjCIdType() || T->isObjCQualifiedIdType()) {
+if (CGM.getTriple().isWindowsMSVCEnvironment())

rjmccall wrote:
> smeenai wrote:
> > smeenai wrote:
> > > rjmccall wrote:
> > > > smeenai wrote:
> > > > > rjmccall wrote:
> > > > > > rnk wrote:
> > > > > > > @rjmccall how should this be organized in the long run? At this 
> > > > > > > point, the naming seems totally wrong. Is the non-fragile ABI 
> > > > > > > sort of the canonical way forward for Obj-C, i.e. it's what a new 
> > > > > > > platform would want to use to best stay in sync with the future 
> > > > > > > of obj-c?
> > > > > > For Darwin, yes, absolutely.
> > > > > > 
> > > > > > I think this method should probably just completely delegate to the 
> > > > > > `CGCXXABI` using a new `getAddrOfObjCRTTIDescriptor` method.
> > > > > To be clear, you'd want the entirety of the EHType emission logic to 
> > > > > be shifted to CGCXXABI? I think that would make sense, and I can look 
> > > > > into it.
> > > > Right.
> > > Sorry, getting back to this now.
> > > 
> > > What did you have in mind for handling the different Obj-C runtimes? Were 
> > > you envisioning the new getAddrOfObjCRTTIDescriptor method supporting 
> > > just the non-fragile Mac ABI or all of them?
> > Looked into this more. ItaniumRTTIBuilder already has a 
> > BuildObjCObjectTypeInfo, which confused me for a bit until I realized it's 
> > only ever used for the fragile ABI, and even then only when using C++ 
> > try/catch (instead of Obj-C style @try/@catch), which is a bit strange. 
> > Everything else does its own thing.
> > 
> > From what I've been able to make out, these are the current possibilities 
> > for generating Obj-C RTTI for the Itanium ABI:
> > * Fragile macOS runtime using Obj-C @try/@catch: doesn't actually seem to 
> > generate any RTTI as far as I can tell, and uses `objc_exception_match` 
> > instead.
> > * Fragile macOS runtime using C++ try/catch: goes through 
> > `ItaniumRTTIBuilder::BuildObjCObjectTypeInfo`, which generates C++ 
> > compatible RTTI referencing a C++ class type info.
> > * Non-fragile macOS runtime: generates its own C++ compatible RTTI 
> > referencing `objc_ehtype_vtable`.
> > * GNUStep for Objective-C++: generates its own C++ compatible RTTI 
> > referencing `_ZTVN7gnustep7libobjc22__objc_class_type_infoE`.
> > * All other GNU runtimes (including GNUStep for Objective-C): just uses the 
> > identifier name string of the interface as its "RTTI".
> > 
> > I think it makes sense to only have the new `getAddrOfObjCRTTIDescriptor` 
> > method handle the non-fragile macOS runtime for now, and perhaps 
> > `ItaniumRTTIBuilder::BuildObjCObjectTypeInfo` should be renamed (or at 
> > least have a comment added) to indicate that it's only used for the fragile 
> > macOS runtime when catching an Obj-C type with C++ catch.
> Yeah, that makes sense to me for now.
@rjmccall Sorry, I've been severely distracted, but I'm trying to come back and 
finish this up now. I'm fully aware you've probably lost all context on this by 
now as well, and I do apologize for the delay.

I did try implementing this approach, where I had `getAddrOfObjCRTTIDescriptor` 
as part of `CGCXXABI`. I ran into issues with trying to take the existing 
Itanium implementation of `CGObjCNonFragileABIMac::GetInterfaceEHType` and 
porting it over to to `CGCXXABI`, however. Specifically, it calls 
`GetClassName` and `GetClassGlobal`, both of which are non-trivial internal 
functions, and which I would then have to make accessible to `CGCXXABI` to end 
up with equivalent RTT generation.

I can put up a diff demonstrating what it would end up looking like if you want 
to see it, but I'm less convinced now that moving the entire RTTI emission 
logic out to `CGCXXABI` will end up being cleaner.


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] D52581: [AST] Revert mangling changes from r339428

2018-09-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a subscriber: rnk.
smeenai added a comment.

Adding @rnk, since this'll touch MS ABI mangling. For context, we want `struct 
X` to have the same mangling as `@interface X` normally, but we want to be able 
to distinguish them for the purpose of exception handling. See this diff's 
summary for more details.

The simplest option is something like https://reviews.llvm.org/P8109, where we 
add a `.objc` discriminator when mangling the RTTI itself. It would require the 
GNUStep runtime for Windows to be altered accordingly (e.g. 
https://github.com/gnustep/libobjc2/blob/master/eh_win32_msvc.cc#L85 would have 
to use `".objc.PAU"` instead of `".PAU.objc_cls_"`); would that be acceptable, 
@theraven and @DHowett-MSFT?

If we want to keep the Obj-C discriminator as an infix instead of a prefix, 
we'll probably have to keep some state around in the mangler indicating if 
we're mangling for RTTI or not. That seems ugly, but I don't know if there's 
any other way to do it.


Repository:
  rC Clang

https://reviews.llvm.org/D52581



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


[PATCH] D52581: [AST] Revert mangling changes from r339428

2018-09-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D52581#1247409, @theraven wrote:

> > I would have done the same for the GNUstep RTTI here, except I don't 
> > actually
> >  see the code for that anywhere, and no tests seem to break either, so I
> >  believe it's not upstreamed yet.
>
> I'm not sure I understand this comment.  The compiler code is in LLVM and the 
> tests that you've modified are the ones that would fail.  The corresponding 
> code is upstreamed in the runtime, to generate EH metadata for the throw with 
> the same mangling.  If you are going to change the mangling, please make sure 
> that the existing mangling is preserved for EH when compiling for the GNUstep 
> / Microsoft ABI on Windows.
>
> I'm also deeply unconvinced by the idea that it's okay to have a `struct I` 
> in one compilation unit and a `@class I` in another.  The `struct` version 
> will not do any of the correct things with respect to memory management.  
> Code that has this idiom is very likely to be broken.


Ah, it looks like it's sharing the C++ RTTI codepath. What I meant by tests 
specifically was ones like in https://reviews.llvm.org/D47233, where you're 
explicitly checking the RTTI generated for catch handlers. None of the tests 
that I'm modifying in this diff provide that sort of coverage, and no other 
tests break, so we don't have that coverage at all.

The header idiom is used by WebKit among others 
(https://github.com/WebKit/webkit/blob/d68641002aa054fd1f5fdf06d957be3912185e14/Source/WTF/wtf/Compiler.h#L291),
 so I think it's worth giving them the benefit of the doubt.

I'm assuming https://github.com/gnustep/libobjc2/blob/master/eh_win32_msvc.cc 
is the corresponding runtime code. I'll adjust the mangling of RTTI emission 
accordingly, but I also really think you should explicitly add regression tests 
in clang itself for the RTTI emission, otherwise it's liable to break in the 
future as well. (I'll try to add a few basic regression tests if I have the 
time.)


Repository:
  rC Clang

https://reviews.llvm.org/D52581



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


[PATCH] D52581: [AST] Revert mangling changes from r339428

2018-09-26 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: compnerd, rjmccall, theraven, DHowett-MSFT.

As discussed in https://reviews.llvm.org/D50144, we want Obj-C classes
to have the same mangling as C++ structs, to support headers like the
following:

  #ifdef __OBJC__
  @class I;
  #else
  struct I;
  #endif
  
  void f(I *);

since the header can be used from both C++ and Obj-C++ TUs, and we want
a consistent mangling across the two to prevent link errors. Itanium
mangles both the same way, and so should the MS ABI.

The main concern with having the same mangling for C++ structs and Obj-C
classes was that we want to treat them differently for the purposes of
exception handling, e.g. we don't want a C++ catch statement for a
struct to be able to catch an Obj-C class with the same name as the
struct. We can accomplish this by mangling Obj-C class names differently
in their RTTI, which I'll do in https://reviews.llvm.org/D47233. I would
have done the same for the GNUstep RTTI here, except I don't actually
see the code for that anywhere, and no tests seem to break either, so I
believe it's not upstreamed yet.


Repository:
  rC Clang

https://reviews.llvm.org/D52581

Files:
  lib/AST/MicrosoftMangle.cpp
  test/CodeGenObjCXX/arc-marker-funclet.mm
  test/CodeGenObjCXX/microsoft-abi-arc-param-order.mm
  test/CodeGenObjCXX/msabi-objc-extensions.mm
  test/CodeGenObjCXX/msabi-objc-types.mm

Index: test/CodeGenObjCXX/msabi-objc-types.mm
===
--- test/CodeGenObjCXX/msabi-objc-types.mm
+++ test/CodeGenObjCXX/msabi-objc-types.mm
@@ -3,166 +3,166 @@
 @class I;
 
 id kid;
-// CHECK: @"?kid@@3PAU.objc_object@@A" =  dso_local global
+// CHECK: @"?kid@@3PAUobjc_object@@A" =  dso_local global
 
 Class klass;
-// CHECK: @"?klass@@3PAU.objc_class@@A" = dso_local global
+// CHECK: @"?klass@@3PAUobjc_class@@A" = dso_local global
 
 I *kI;
-// CHECK: @"?kI@@3PAU.objc_cls_I@@A" = dso_local global
+// CHECK: @"?kI@@3PAUI@@A" = dso_local global
 
 void f(I *) {}
-// CHECK-LABEL: "?f@@YAXPAU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXPAUI@@@Z"
 
 void f(const I *) {}
-// CHECK-LABEL: "?f@@YAXPBU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXPBUI@@@Z"
 
 void f(I &) {}
-// CHECK-LABEL: "?f@@YAXAAU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXAAUI@@@Z"
 
 void f(const I &) {}
-// CHECK-LABEL: "?f@@YAXABU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAXABUI@@@Z"
 
 void f(const I &&) {}
-// CHECK-LABEL: "?f@@YAX$$QBU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?f@@YAX$$QBUI@@@Z"
 
 void g(id) {}
-// CHECK-LABEL: "?g@@YAXPAU.objc_object@@@Z"
+// CHECK-LABEL: "?g@@YAXPAUobjc_object@@@Z"
 
 void g(id &) {}
-// CHECK-LABEL: "?g@@YAXAAPAU.objc_object@@@Z"
+// CHECK-LABEL: "?g@@YAXAAPAUobjc_object@@@Z"
 
 void g(const id &) {}
-// CHECK-LABEL: "?g@@YAXABQAU.objc_object@@@Z"
+// CHECK-LABEL: "?g@@YAXABQAUobjc_object@@@Z"
 
 void g(id &&) {}
-// CHECK-LABEL: "?g@@YAX$$QAPAU.objc_object@@@Z"
+// CHECK-LABEL: "?g@@YAX$$QAPAUobjc_object@@@Z"
 
 void h(Class) {}
-// CHECK-LABEL: "?h@@YAXPAU.objc_class@@@Z"
+// CHECK-LABEL: "?h@@YAXPAUobjc_class@@@Z"
 
 void h(Class &) {}
-// CHECK-LABEL: "?h@@YAXAAPAU.objc_class@@@Z"
+// CHECK-LABEL: "?h@@YAXAAPAUobjc_class@@@Z"
 
 void h(const Class &) {}
-// CHECK-LABEL: "?h@@YAXABQAU.objc_class@@@Z"
+// CHECK-LABEL: "?h@@YAXABQAUobjc_class@@@Z"
 
 void h(Class &&) {}
-// CHECK-LABEL: "?h@@YAX$$QAPAU.objc_class@@@Z"
+// CHECK-LABEL: "?h@@YAX$$QAPAUobjc_class@@@Z"
 
 I *i() { return nullptr; }
-// CHECK-LABEL: "?i@@YAPAU.objc_cls_I@@XZ"
+// CHECK-LABEL: "?i@@YAPAUI@@XZ"
 
 const I *j() { return nullptr; }
-// CHECK-LABEL: "?j@@YAPBU.objc_cls_I@@XZ"
+// CHECK-LABEL: "?j@@YAPBUI@@XZ"
 
 I () { return *kI; }
-// CHECK-LABEL: "?k@@YAAAU.objc_cls_I@@XZ"
+// CHECK-LABEL: "?k@@YAAAUI@@XZ"
 
 const I () { return *kI; }
-// CHECK-LABEL: "?l@@YAABU.objc_cls_I@@XZ"
+// CHECK-LABEL: "?l@@YAABUI@@XZ"
 
 void m(const id) {}
-// CHECK-LABEL: "?m@@YAXQAU.objc_object@@@Z"
+// CHECK-LABEL: "?m@@YAXQAUobjc_object@@@Z"
 
 void m(const I *) {}
-// CHECK-LABEL: "?m@@YAXPBU.objc_cls_I@@@Z"
+// CHECK-LABEL: "?m@@YAXPBUI@@@Z"
 
 void n(SEL) {}
-// CHECK-LABEL: "?n@@YAXPAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXPAUobjc_selector@@@Z"
 
 void n(SEL *) {}
-// CHECK-LABEL: "?n@@YAXPAPAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXPAPAUobjc_selector@@@Z"
 
 void n(const SEL *) {}
-// CHECK-LABEL: "?n@@YAXPBQAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXPBQAUobjc_selector@@@Z"
 
 void n(SEL &) {}
-// CHECK-LABEL: "?n@@YAXAAPAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXAAPAUobjc_selector@@@Z"
 
 void n(const SEL &) {}
-// CHECK-LABEL: "?n@@YAXABQAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAXABQAUobjc_selector@@@Z"
 
 void n(SEL &&) {}
-// CHECK-LABEL: "?n@@YAX$$QAPAU.objc_selector@@@Z"
+// CHECK-LABEL: "?n@@YAX$$QAPAUobjc_selector@@@Z"
 
 struct __declspec(dllexport) s {
   struct s =(const struct s &) = delete;
 
   void m(I *) {}
-  // CHECK-LABEL: "?m@s@@QAAXPAU.objc_cls_I@@@Z"
+  // CHECK-LABEL: 

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

2018-09-21 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D52344#1242530, @rjmccall wrote:

> In https://reviews.llvm.org/D52344#1242451, @kristina wrote:
>
> > Would you be okay with me renaming `cfstring` to `.cfstring` for ELF so 
> > it's in line with ELF section naming conventions (I'm not entirely sure if 
> > that could cause issues with ObjC stuff)? And yes I think it's best to 
> > avoid that code-path altogether if it turns out to be a constant as that's 
> > likely from the declaration of the type, I'll write a FileCheck test in a 
> > moment and check that I can apply the same logic to ELF aside from the DLL 
> > stuff as CoreFoundation needs to export the symbol somehow while preserving 
> > the implicit extern attribute for every other TU that comes from the 
> > builtin that `CFSTR` expands to. Is what I'm suggesting making sense? If 
> > not, let me know, I may be misunderstanding what's happening here.
>
>
> Following the ELF conventions seems like the right thing to do, assuming it 
> doesn't cause compatibility problems.  David Chisnall might know best here.


We would lose the `__start_` and `__end_` markers (which @compnerd mentioned) 
if the section was renamed to `.cfstring`, because the linker only generates 
those markers for sections whose names are valid C identifiers, so I don't 
think that rename would be valid. Here's how LLD does it, for example: 
https://reviews.llvm.org/diffusion/L/browse/lld/trunk/ELF/Writer.cpp;342772$1764-1776


Repository:
  rC Clang

https://reviews.llvm.org/D52344



___
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.

2018-08-22 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a subscriber: rnk.
smeenai added a comment.

Sorry for the belated response; I was on vacation.

I'm confused by the funclet-based unwinding support. Do you intend GNUStep to 
be used with windows-msvc triples? If so, how is the runtime throwing 
exceptions? We took the approach of having the Obj-C runtime wrap Obj-C 
exceptions in C++ exceptions and then have vcruntime handle the rest (see 
https://reviews.llvm.org/D47233 and https://reviews.llvm.org/D47988), but I 
don't see any of the associated RTTI emission changes in this patch.

I also don't see any tests added for the codegen changes around `@finally`. I'm 
planning on changing that approach anyway (by pushing 
https://reviews.llvm.org/D47988) through, but it still seems like something 
that was missing from this patch.




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

theraven wrote:
> smeenai wrote:
> > theraven wrote:
> > > compnerd wrote:
> > > > DHowett-MSFT wrote:
> > > > > I'm interested in @smeenai's take on this, as well.
> > > > Hmm, doesn't this break ObjC/ObjC++ interoperability?  I don't think 
> > > > that we should be changing the decoration here for the MS ABI case.
> > > No, this fixes ObjC++ interop.  Throwing an exception from a `@throw` and 
> > > catching it with `catch` now works and we avoid the problem that a C++ 
> > > compilation unit declares a `struct` that happens to have the same name 
> > > as an Objective-C class (which is very common for wrapper code) may catch 
> > > things of the wrong type.
> > I've seen a lot of code which does something like this in a header
> > 
> > ```lang=cpp
> > #ifdef __OBJC__
> > @class I;
> > #else
> > struct I;
> > #endif
> > 
> > void f(I *);
> > ```
> > 
> > You want `f` to be mangled consistently across C++ and Objective-C++, 
> > otherwise you'll get link errors if `f` is e.g. defined in a C++ TU and 
> > referenced in an Objective-C++ TU. This was the case before your change and 
> > isn't the case after your change, which is what @compnerd was pointing out. 
> > They are mangled consistently in the Itanium ABI, and we want them to be 
> > mangled consistently in the MS ABI as well.
> > 
> > Not wanting the catch mix-up is completely reasonable, of course, but it 
> > can be done in a more targeted way. I'll put up a patch that undoes this 
> > part of the change while still preventing incorrect catching.
> With a non-fragile ABI, there is not guarantee that `struct I` and `@class I` 
> have the same layout.  This is why `@defs` has gone away.  Any code that does 
> this and treats `struct I` as anything other than an opaque type is broken.  
> No other platform supports throwing an Objective-C object and catching it as 
> a struct, and this is an intentional design decision.  I would be strongly 
> opposed to a change that intentionally supported this, because it is breaking 
> correct code in order to make incorrect code do something that may or may not 
> work.
To be clear, I'm aware of the layout differences. My plan was to revert the 
mangling here to what it was before (which would also be consistent with 
Itanium), and then adjust the RTTI emission for Obj-C exception types (which 
I'm handling in https://reviews.llvm.org/D47233) to differentiate between C++ 
and Obj-C exceptions, which would prevent unintended catches.



Comment at: lib/CodeGen/CGObjCRuntime.cpp:203
+const Stmt *FinallyBlock = Finally->getFinallyBody();
+HelperCGF.startOutlinedSEHHelper(CGF, /*isFilter*/false, FinallyBlock);
+

This is *very* limiting. This outlining doesn't handle capturing self, 
capturing block variables, and lots of other cases that come up in practice. I 
attempted to implement some of the missing cases, before meeting with @compnerd 
and @rnk and deciding that it was better to use CapturedStmt instead. See 
https://reviews.llvm.org/D47564 for more discussion, and 
https://reviews.llvm.org/D47988 implements the associated codegen 
(unfortunately I wasn't able to get that through before going on vacation).


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.

2018-08-15 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



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

theraven wrote:
> compnerd wrote:
> > DHowett-MSFT wrote:
> > > I'm interested in @smeenai's take on this, as well.
> > Hmm, doesn't this break ObjC/ObjC++ interoperability?  I don't think that 
> > we should be changing the decoration here for the MS ABI case.
> No, this fixes ObjC++ interop.  Throwing an exception from a `@throw` and 
> catching it with `catch` now works and we avoid the problem that a C++ 
> compilation unit declares a `struct` that happens to have the same name as an 
> Objective-C class (which is very common for wrapper code) may catch things of 
> the wrong type.
I've seen a lot of code which does something like this in a header

```lang=cpp
#ifdef __OBJC__
@class I;
#else
struct I;
#endif

void f(I *);
```

You want `f` to be mangled consistently across C++ and Objective-C++, otherwise 
you'll get link errors if `f` is e.g. defined in a C++ TU and referenced in an 
Objective-C++ TU. This was the case before your change and isn't the case after 
your change, which is what @compnerd was pointing out. They are mangled 
consistently in the Itanium ABI, and we want them to be mangled consistently in 
the MS ABI as well.

Not wanting the catch mix-up is completely reasonable, of course, but it can be 
done in a more targeted way. I'll put up a patch that undoes this part of the 
change while still preventing incorrect catching.


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

2018-07-16 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: lib/CodeGen/CGObjCMac.cpp:7457-7460
 CGObjCNonFragileABIMac::GetEHType(QualType T) {
   // There's a particular fixed type info for 'id'.
   if (T->isObjCIdType() || T->isObjCQualifiedIdType()) {
+if (CGM.getTriple().isWindowsMSVCEnvironment())

smeenai wrote:
> rjmccall wrote:
> > smeenai wrote:
> > > rjmccall wrote:
> > > > rnk wrote:
> > > > > @rjmccall how should this be organized in the long run? At this 
> > > > > point, the naming seems totally wrong. Is the non-fragile ABI sort of 
> > > > > the canonical way forward for Obj-C, i.e. it's what a new platform 
> > > > > would want to use to best stay in sync with the future of obj-c?
> > > > For Darwin, yes, absolutely.
> > > > 
> > > > I think this method should probably just completely delegate to the 
> > > > `CGCXXABI` using a new `getAddrOfObjCRTTIDescriptor` method.
> > > To be clear, you'd want the entirety of the EHType emission logic to be 
> > > shifted to CGCXXABI? I think that would make sense, and I can look into 
> > > it.
> > Right.
> Sorry, getting back to this now.
> 
> What did you have in mind for handling the different Obj-C runtimes? Were you 
> envisioning the new getAddrOfObjCRTTIDescriptor method supporting just the 
> non-fragile Mac ABI or all of them?
Looked into this more. ItaniumRTTIBuilder already has a 
BuildObjCObjectTypeInfo, which confused me for a bit until I realized it's only 
ever used for the fragile ABI, and even then only when using C++ try/catch 
(instead of Obj-C style @try/@catch), which is a bit strange. Everything else 
does its own thing.

From what I've been able to make out, these are the current possibilities for 
generating Obj-C RTTI for the Itanium ABI:
* Fragile macOS runtime using Obj-C @try/@catch: doesn't actually seem to 
generate any RTTI as far as I can tell, and uses `objc_exception_match` instead.
* Fragile macOS runtime using C++ try/catch: goes through 
`ItaniumRTTIBuilder::BuildObjCObjectTypeInfo`, which generates C++ compatible 
RTTI referencing a C++ class type info.
* Non-fragile macOS runtime: generates its own C++ compatible RTTI referencing 
`objc_ehtype_vtable`.
* GNUStep for Objective-C++: generates its own C++ compatible RTTI referencing 
`_ZTVN7gnustep7libobjc22__objc_class_type_infoE`.
* All other GNU runtimes (including GNUStep for Objective-C): just uses the 
identifier name string of the interface as its "RTTI".

I think it makes sense to only have the new `getAddrOfObjCRTTIDescriptor` 
method handle the non-fragile macOS runtime for now, and perhaps 
`ItaniumRTTIBuilder::BuildObjCObjectTypeInfo` should be renamed (or at least 
have a comment added) to indicate that it's only used for the fragile macOS 
runtime when catching an Obj-C type with C++ catch.


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] D49345: [CMake] Use correct variable as header install prefix

2018-07-14 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

Seems like an obvious enough fix, so I'm jumping in to LGTM.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49345



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


[PATCH] D49227: Override a bit fields layout from an external source

2018-07-12 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai resigned from this revision.
smeenai added a comment.

I'm not familiar enough with this to review it.


Repository:
  rC Clang

https://reviews.llvm.org/D49227



___
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

2018-07-10 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: lib/CodeGen/CGObjCMac.cpp:7457-7460
 CGObjCNonFragileABIMac::GetEHType(QualType T) {
   // There's a particular fixed type info for 'id'.
   if (T->isObjCIdType() || T->isObjCQualifiedIdType()) {
+if (CGM.getTriple().isWindowsMSVCEnvironment())

rjmccall wrote:
> smeenai wrote:
> > rjmccall wrote:
> > > rnk wrote:
> > > > @rjmccall how should this be organized in the long run? At this point, 
> > > > the naming seems totally wrong. Is the non-fragile ABI sort of the 
> > > > canonical way forward for Obj-C, i.e. it's what a new platform would 
> > > > want to use to best stay in sync with the future of obj-c?
> > > For Darwin, yes, absolutely.
> > > 
> > > I think this method should probably just completely delegate to the 
> > > `CGCXXABI` using a new `getAddrOfObjCRTTIDescriptor` method.
> > To be clear, you'd want the entirety of the EHType emission logic to be 
> > shifted to CGCXXABI? I think that would make sense, and I can look into it.
> Right.
Sorry, getting back to this now.

What did you have in mind for handling the different Obj-C runtimes? Were you 
envisioning the new getAddrOfObjCRTTIDescriptor method supporting just the 
non-fragile Mac ABI or all of them?


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] D49059: [MinGW] Skip adding default win32 api libraries if -lwindowsapp is specified

2018-07-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: test/Driver/mingw-windowsapp.c:5-6
+// CHECK_DEFAULT: "-lmsvcrt" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32" 
"-lmingw32"
+// CHECK_WINDOWSAPP: "-lwindowsapp" "-lmingw32"
+// CHECK_WINDOWSAPP-SAME: "-lmsvcrt" "-lmingw32"

mstorsjo wrote:
> smeenai wrote:
> > Why do we end up with -lmingw32 twice, and why not just check the full 
> > line, like you're doing for the default case?
> I don't remember exactly why -lmingw32 ends up multiple times; I think it 
> comes from legacy compat with binutils ld, where the library ordering matters 
> more (a later static library doesn't trigger search in an earlier one).
> 
> I'm checking twice, since there are other unrelated entries between these 
> that I didn't want to spell out, to avoid making the test overly specific (to 
> avoid having to update the test in case some of those are changed).
Makes sense; I didn't realize before that all the other libraries in the 
default case were sandwiched between the -lmsvcrt and the -lmingw32 in the 
CHECK_WINDOWSAPP-SAME case.

A -NOT check would perhaps have been more obvious, but I think that might end 
up interacting poorly with the other checks, so I'm fine with this as is.


Repository:
  rC Clang

https://reviews.llvm.org/D49059



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


[PATCH] D49059: [MinGW] Skip adding default win32 api libraries if -lwindowsapp is specified

2018-07-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: test/Driver/mingw-windowsapp.c:5-6
+// CHECK_DEFAULT: "-lmsvcrt" "-ladvapi32" "-lshell32" "-luser32" "-lkernel32" 
"-lmingw32"
+// CHECK_WINDOWSAPP: "-lwindowsapp" "-lmingw32"
+// CHECK_WINDOWSAPP-SAME: "-lmsvcrt" "-lmingw32"

Why do we end up with -lmingw32 twice, and why not just check the full line, 
like you're doing for the default case?


Repository:
  rC Clang

https://reviews.llvm.org/D49059



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


[PATCH] D49059: [MinGW] Skip adding default win32 api libraries if -lwindowsapp is specified

2018-07-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM, particularly given r314138.

There are other umbrella libraries as well, e.g. OneCore and OneCoreUAP. Do you 
care about those as well or just WindowsApp?




Comment at: lib/Driver/ToolChains/MinGW.cpp:207
+if (Lib == "windowsapp")
+  HasWindowsApp = true;
+

I don't think it matters very much, but you could break out here.



Comment at: lib/Driver/ToolChains/MinGW.cpp:232
+  if (!HasWindowsApp) {
+// add system libraries
+if (Args.hasArg(options::OPT_mwindows)) {

Might be worth adding a short note to this comment about why we skip adding 
these libraries in the WindowsApp case?


Repository:
  rC Clang

https://reviews.llvm.org/D49059



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


[PATCH] D49054: [MinGW] Treat any -lucrt* as replacing -lmsvcrt

2018-07-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D49054



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


[PATCH] D48694: [libc++abi] Limit libc++ header search to specified paths

2018-06-29 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336032: [libc++abi] Limit libc++ header search to specified 
paths (authored by smeenai, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D48694

Files:
  libcxxabi/trunk/CMakeLists.txt


Index: libcxxabi/trunk/CMakeLists.txt
===
--- libcxxabi/trunk/CMakeLists.txt
+++ libcxxabi/trunk/CMakeLists.txt
@@ -116,6 +116,7 @@
 ${CMAKE_BINARY_DIR}/${LIBCXXABI_LIBCXX_INCLUDES}
 ${LIBCXXABI_LIBCXX_INCLUDE_DIRS}
 ${LLVM_INCLUDE_DIR}/c++/v1
+  NO_DEFAULT_PATH
   NO_CMAKE_FIND_ROOT_PATH
   )
 


Index: libcxxabi/trunk/CMakeLists.txt
===
--- libcxxabi/trunk/CMakeLists.txt
+++ libcxxabi/trunk/CMakeLists.txt
@@ -116,6 +116,7 @@
 ${CMAKE_BINARY_DIR}/${LIBCXXABI_LIBCXX_INCLUDES}
 ${LIBCXXABI_LIBCXX_INCLUDE_DIRS}
 ${LLVM_INCLUDE_DIR}/c++/v1
+  NO_DEFAULT_PATH
   NO_CMAKE_FIND_ROOT_PATH
   )
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D48694: [libc++abi] Limit libc++ header search to specified paths

2018-06-29 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D48694#1148718, @EricWF wrote:

> > For whether it makes sense to search for includes in the system path, that 
> > boils down to @ldionne's question of whether building libc++abi against a 
> > system-installed libc++ is supported. I actually don't know the answer to 
> > that myself ... @dexonsmith and @EricWF, what are your thoughts on that? 
> > The current search won't find the system-installed libc++ headers on Darwin 
> > anyway though, where they're placed in the compiler's include directory 
> > rather than a system include directory.
>
> Building libc++abi against an installed libc++ doesn't make sense IMO, so I 
> don't care if we try to support it. Libc++abi is a lower lever component with 
> libc++ being built on top of it.
>  If you want to update libc++abi, you should be building a new libc++ as 
> well. (Note: Using system libc++abi headers to build libc++ would make sense 
> though).
>
> As an aside, libc++abi should really live in the libc++ repository. That way 
> it would always have the correct headers available. But every time I pitch 
> that idea I get a ton of push back.


All right. In that case I'll go ahead and commit this and make the `__config` 
change in a follow-up.


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D48694



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


[PATCH] D48694: [libc++abi] Limit libc++ header search to specified paths

2018-06-29 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D48694#1146232, @EricWF wrote:

> LGTM, but I'm a bit confused. You seem to argue that no system places C++ 
> headers on the default search paths, but also that it would be a problem if 
> such a system did.
>  Why wouldn't the conclusion be true? That is, although C++ includes aren't 
> normally found along the default paths, when they are found, we should still 
> consider them?
>
> That being said, the current behavior of searching certian default include 
> paths first does seem incorrect. So I'm OK with disabling it.
>
> I also agree that we shouldn't necessarily be looking for `vector` header, 
> how about looking for `__config` instead, since it's libc++ specific?


Switching to `__config` is a good idea. I can do that separately, since it 
should be pretty uncontroversial.

For whether it makes sense to search for includes in the system path, that 
boils down to @ldionne's question of whether building libc++abi against a 
system-installed libc++ is supported. I actually don't know the answer to that 
myself ... @dexonsmith and @EricWF, what are your thoughts on that? The current 
search won't find the system-installed libc++ headers on Darwin anyway though, 
where they're placed in the compiler's include directory rather than a system 
include directory.

If we do decide that searching for C++ headers in the system is important, we 
can do two separate find_path calls, to ensure our specified paths are searched 
before the system paths. (We may also want to tweak the second call to actually 
look for the system C++ headers in the right places in that case.)


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D48694



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


[PATCH] D48694: [libc++abi] Limit libc++ header search to specified paths

2018-06-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

This doesn't map to any command line change; it's purely a CMake thing. It just 
changes where libc++abi's build system looks for libc++ headers. Before this 
patch, it would look for a file named "vector" in all the standard system 
include directories (`/usr/local/include`, `/usr/include`, anything else in 
your PATH, etc.) and then look for it in the paths specified by the PATHS 
argument. After this patch it will only look for that file in the paths 
specified by the PATHS argument. The actual value of LIBCXXABI_LIBCXX_INCLUDES 
gets added to the header search path when compiling the libc++abi sources, but 
this diff isn't changing that part (it just potentially changes the value that 
gets computed for LIBCXXABI_LIBCXX_INCLUDES, but as I detailed in the 
description I don't think that should actually change on any system I'm aware 
of, and if it does cause a change then there's a good chance the old value was 
incorrect).


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D48694



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


[PATCH] D48675: [libc++abi] Limit libc++ header search to specified paths

2018-06-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai abandoned this revision.
smeenai added a comment.

https://reviews.llvm.org/D48694


Repository:
  rL LLVM

https://reviews.llvm.org/D48675



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


[PATCH] D48694: [libc++abi] Limit libc++ header search to specified paths

2018-06-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: beanz, compnerd, dexonsmith, EricWF, mclow.lists.
Herald added subscribers: cfe-commits, ldionne, christof, mgorny.

Right now, when libc++abi is locating libc++ headers, it specifies
several search locations, but it also doesn't prevent CMake from looking
for those headers in system directories. I don't know if this was
intentional or an oversight, but it has several issues:

- We're looking specifically for the vector header, which could just as easily 
be found in a libstdc++ (or other C++ library) installation.
- No system I know of places their C++ headers directly in system include 
directories (they're always under a C++ subdirectory), so the system search 
will never succeed.
- find_path searches system paths before the user-specified PATHS, so if some 
system does happen to have C++ headers in its system include directories, those 
headers will be preferred, which doesn't seem desirable.

It makes sense to me to limit this header search to the explicitly
specified paths (using NO_DEFAULT_PATH, as is done for the other
find_path call in this file), but I'm putting it up for review in case
there's some use case I'm not thinking of.


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D48694

Files:
  CMakeLists.txt


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -116,6 +116,7 @@
 ${CMAKE_BINARY_DIR}/${LIBCXXABI_LIBCXX_INCLUDES}
 ${LIBCXXABI_LIBCXX_INCLUDE_DIRS}
 ${LLVM_INCLUDE_DIR}/c++/v1
+  NO_DEFAULT_PATH
   NO_CMAKE_FIND_ROOT_PATH
   )
 


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -116,6 +116,7 @@
 ${CMAKE_BINARY_DIR}/${LIBCXXABI_LIBCXX_INCLUDES}
 ${LIBCXXABI_LIBCXX_INCLUDE_DIRS}
 ${LLVM_INCLUDE_DIR}/c++/v1
+  NO_DEFAULT_PATH
   NO_CMAKE_FIND_ROOT_PATH
   )
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D48675: [libc++abi] Limit libc++ header search to specified paths

2018-06-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai edited subscribers, added: cfe-commits; removed: llvm-commits.
smeenai added a comment.

Huh, this went to llvm-commits instead of cfe-commits automatically. I guess 
the auto mailing list subscribing doesn't work great with the monorepo?


Repository:
  rL LLVM

https://reviews.llvm.org/D48675



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


[PATCH] D48626: New option -fwindows-filesystem, affecting #include paths.

2018-06-27 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added subscribers: smeenai, compnerd, zturner.
smeenai added a comment.

Adding some people who are interested in Windows cross-compilation.


Repository:
  rC Clang

https://reviews.llvm.org/D48626



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


[PATCH] D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.

2018-06-25 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Okay, upon playing with this further, the following seems to do what I want 
(for a custom sysroot), and seems to be a pretty common pattern as well:

  set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)
  set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)
  set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE ONLY)
  set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)


Repository:
  rC Clang

https://reviews.llvm.org/D48459



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


[PATCH] D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.

2018-06-25 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Ah, the documentation is confusing, but CMAKE_FIND_ROOT_PATH and 
CMAKE_FIND_ROOT_PATH_MODE_PACKAGE only have an effect when using the config 
mode of find_package, whereas this invocation is using the module mode. That's 
a bummer, and definitely makes custom sysroots and cross-compilation harder 
than it should be. Sorry for the noise.


Repository:
  rC Clang

https://reviews.llvm.org/D48459



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


[PATCH] D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.

2018-06-25 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Actually, I would imagine that if you're cross-compiling or using a custom 
sysroot, you should probably also specify CMAKE_FIND_ROOT_PATH and set 
CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to ONLY, to limit all these searches to the 
desired directories?

(I'm actually playing around with a bunch of this stuff myself right now. I'm 
curious about your CMake setup for a custom sysroot or cross-compilation and 
what issues you've come across.)


Repository:
  rC Clang

https://reviews.llvm.org/D48459



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


[PATCH] D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.

2018-06-25 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added reviewers: beanz, phosek.
smeenai added a comment.

I think it might be more appropriate to pass NO_SYSTEM_ENVIRONMENT_PATH to 
find_package in case CMAKE_SYSROOT or CMAKE_CROSSCOMPILING are set? That way, 
we won't search the host system for libxml2 in those cases, but we'll still be 
able to find it in the custom sysroot or cross-compilation SDK if it's present.


Repository:
  rC Clang

https://reviews.llvm.org/D48459



___
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

2018-06-22 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: lib/CodeGen/CGObjCMac.cpp:7457-7460
 CGObjCNonFragileABIMac::GetEHType(QualType T) {
   // There's a particular fixed type info for 'id'.
   if (T->isObjCIdType() || T->isObjCQualifiedIdType()) {
+if (CGM.getTriple().isWindowsMSVCEnvironment())

rjmccall wrote:
> rnk wrote:
> > @rjmccall how should this be organized in the long run? At this point, the 
> > naming seems totally wrong. Is the non-fragile ABI sort of the canonical 
> > way forward for Obj-C, i.e. it's what a new platform would want to use to 
> > best stay in sync with the future of obj-c?
> For Darwin, yes, absolutely.
> 
> I think this method should probably just completely delegate to the 
> `CGCXXABI` using a new `getAddrOfObjCRTTIDescriptor` method.
To be clear, you'd want the entirety of the EHType emission logic to be shifted 
to CGCXXABI? I think that would make sense, and I can look into it.


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] D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions

2018-06-22 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: lib/CodeGen/CGCXXABI.h:248
+llvm_unreachable("Only needed for the Microsoft ABI");
+  }
 

rjmccall wrote:
> Should you just generalize the existing method to only take a VarDecl* so it 
> can be used for either kind of catch?
The Itanium version of emitBeginCatch actually uses the statement for location 
info (it calls `getLocStart` on it). I suppose I could generalize the existing 
method to take both a VarDecl* and a SourceLocation though.


Repository:
  rC Clang

https://reviews.llvm.org/D47988



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


[PATCH] D48356: [libcxx] [CMake] Convert paths to the right form in standalone builds on Windows

2018-06-20 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: cmake/Modules/HandleOutOfTreeLLVM.cmake:52
 else()
-  set(LLVM_CMAKE_PATH
-  "${LLVM_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm")
+  file(TO_CMAKE_PATH ${LLVM_BINARY_DIR} LLVM_BINARY_DIR_CMAKE_STYLE)
+  set(LLVM_CMAKE_PATH 
"${LLVM_BINARY_DIR_CMAKE_STYLE}/lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm")

mstorsjo wrote:
> smeenai wrote:
> > Is there ever a chance LLVM_BINARY_DIR will have backslashes, or are you 
> > just being extra careful (or going for consistency)?
> I'm mostly just going for consistency and copy+paste; this path identically 
> matches a part from compiler-rt/cmake/Modules/CompilerRTUtils.cmake - and 
> that part actually seems to have been added intentionally to fix some case: 
> https://reviews.llvm.org/rL203789
Fair enough; sounds good to me.


Repository:
  rCXX libc++

https://reviews.llvm.org/D48356



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


[PATCH] D48353: [libunwind] [CMake] Convert paths to the right form in standalone builds on Windows

2018-06-20 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM with the same comments as https://reviews.llvm.org/D48356 (add quotes, and 
is the translation for LLVM_BINARY_DIR necessary?)


Repository:
  rUNW libunwind

https://reviews.llvm.org/D48353



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


[PATCH] D48355: [libcxxabi] [CMake] Convert paths to the right form in standalone builds on Windows

2018-06-20 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

LGTM with the same comments as https://reviews.llvm.org/D48356 (add quotes, and 
is the translation for LLVM_BINARY_DIR necessary?)


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D48355



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


[PATCH] D48356: [libcxx] [CMake] Convert paths to the right form in standalone builds on Windows

2018-06-20 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

Looks good with the quotes added.




Comment at: cmake/Modules/HandleCompilerRT.cmake:17
   string(STRIP "${LIBRARY_FILE}" LIBRARY_FILE)
+  file(TO_CMAKE_PATH ${LIBRARY_FILE} LIBRARY_FILE)
   string(REPLACE "builtins" "${name}" LIBRARY_FILE "${LIBRARY_FILE}")

Nit: cmake recommends always putting quotes around the path argument to ensure 
it's treated as a single argument, i.e.

  file(TO_CMAKE_PATH "${LIBRARY_FILE}" LIBRARY_FILE)

That needs to happen in all other uses as well.



Comment at: cmake/Modules/HandleOutOfTreeLLVM.cmake:52
 else()
-  set(LLVM_CMAKE_PATH
-  "${LLVM_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm")
+  file(TO_CMAKE_PATH ${LLVM_BINARY_DIR} LLVM_BINARY_DIR_CMAKE_STYLE)
+  set(LLVM_CMAKE_PATH 
"${LLVM_BINARY_DIR_CMAKE_STYLE}/lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm")

Is there ever a chance LLVM_BINARY_DIR will have backslashes, or are you just 
being extra careful (or going for consistency)?


Repository:
  rCXX libc++

https://reviews.llvm.org/D48356



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


[PATCH] D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions

2018-06-18 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Ping.


Repository:
  rC Clang

https://reviews.llvm.org/D47988



___
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

2018-06-18 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Ping.


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] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

2018-06-12 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D47233#1129810, @DHowett-MSFT wrote:

> 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 
> .


We solved the x64 issue by just calling RaiseException directly and supplying a 
fake ImageBase. It's a bit kludgy, but it works well. (_CxxThrowException's 
source is included with MSVC, so we just looked at how that was calling 
RaiseException and altered it accordingly.)


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] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

2018-06-11 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Wrapping an Objective-C exception inside a C++ exception means dynamically 
constructing a C++ exception object and traversing the class hierarchy of the 
thrown Obj-C object to populate the catchable types array of the C++ exception. 
Microsoft's C++ runtime will perform the appropriate catch type comparisons, 
and this patch makes the compiler emit the right typeinfos into the exception 
handling tables for all of that to work. 
https://github.com/Microsoft/libobjc2/blob/f2e4c5ac4b3ac17f413a38bbc7ee1242f9efd0f7/msvc/eh_winobjc.cc#L116
 is how WinObjC does this wrapping, for example.

Essentially, with this patch, `@catch (I *)` in Obj-C ends up generating the 
same exception handling table as `catch (struct I *)` in C++ would. If you're 
throwing an `I *` (or any derived class), the runtime will generate an 
exception object which is catchable by this handler (the catchable types array 
for that exception object will contain the appropriate typeinfo). Successive 
catch clauses don't need any special handling; they work the same way as they 
would in C++. The runtime is generating that exception object based on a 
dynamic traversal of the class hierarchy, so I think Obj-C's dynamic semantics 
should be respected.


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] D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions

2018-06-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

@rnk remember how I was asking you about inlining into catchpads on IRC a few 
days back? It was in relation to this change.

If I have a file `finally1.m`:

  void f(void);
  void g() {
@try {
  f();
} @finally {
  f();
}
  }

and compile it with:

  clang -cc1 -triple i686-windows-msvc -fobjc-runtime=ios-6.0 -fexceptions 
-fobjc-exceptions -Os -emit-llvm -o - finally1.m

the @finally calls out to the captured statement instead of inlining it:

  finally.catchall: ; preds = %catch.dispatch
%1 = catchpad within %0 [i8* null, i32 64, i8* null]
call fastcc void @__captured_stmt() [ "funclet"(token %1) ]
call void @_CxxThrowException(i8* null, %eh.ThrowInfo* null) #3 [ 
"funclet"(token %1) ]
unreachable

If I add an outer try-catch scope, as in `finally2.m`:

  void f(void);
  void g() {
@try {
  @try {
f();
  } @finally {
f();
  }
} @catch (...) {
}
  }

and run the same compilation command:

  clang -cc1 -triple i686-windows-msvc -fobjc-runtime=ios-6.0 -fexceptions 
-fobjc-exceptions -Os -emit-llvm -o - finally2.m

the @finally inlines the captured statement:

  finally.catchall: ; preds = %catch.dispatch
%2 = catchpad within %0 [i8* null, i32 64, i8* null]
invoke void @f() #2 [ "funclet"(token %2) ]
to label %invoke.cont1 unwind label %catch.dispatch4
  
  invoke.cont1: ; preds = %finally.catchall
invoke void @_CxxThrowException(i8* null, %eh.ThrowInfo* null) #3 [ 
"funclet"(token %2) ]
to label %unreachable unwind label %catch.dispatch4

Any idea why we would see inlining in one case and not the other? i686 vs. 
x86-64 doesn't make any difference, and neither does -Os vs. -O1 vs. -O2.


Repository:
  rC Clang

https://reviews.llvm.org/D47988



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


[PATCH] D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions

2018-06-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: DHowett-MSFT, compnerd, majnemer, rjmccall, rnk.

We're implementing funclet-compatible code generation for Obj-C
exceptions when using the MSVC ABI. The idea is that the Obj-C runtime
will wrap Obj-C exceptions inside C++ exceptions, which allows for
interoperability with C++ exceptions (for Obj-C++) and zero-cost
exceptions. This is the approach taken by e.g. WinObjC, and I believe it
to be the best approach for Obj-C exceptions in the MSVC ABI.

This change implements emitting the actual funclet-compatible IR. The
generic exceptions codegen already takes care of emitting the proper
catch dispatch structures, but we need to ensure proper handling of
catch parameters and scoping (emitting the catchret). Finally blocks are
handled quite differently from Itanium; they're expected to be outlined
by the frontend, which limits some control flow possibilities but also
greatly simplifies their codegen. See r334251 for further discussion of
why frontend outlining is used.

Worked on with Saleem Abdulrasool .


Repository:
  rC Clang

https://reviews.llvm.org/D47988

Files:
  lib/CodeGen/CGCXXABI.h
  lib/CodeGen/CGException.cpp
  lib/CodeGen/CGObjCGNU.cpp
  lib/CodeGen/CGObjCMac.cpp
  lib/CodeGen/CGObjCRuntime.cpp
  lib/CodeGen/CGStmt.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/MicrosoftCXXABI.cpp
  test/CodeGenObjC/catch-lexical-block.m
  test/CodeGenObjC/exceptions-msvc.m

Index: test/CodeGenObjC/exceptions-msvc.m
===
--- test/CodeGenObjC/exceptions-msvc.m
+++ test/CodeGenObjC/exceptions-msvc.m
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
-// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
-// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
-// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 --enable-var-scope %s
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86,ARC --enable-var-scope %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 --enable-var-scope %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64,ARC --enable-var-scope %s
 
 #if __has_feature(objc_arc)
 #define WEAK __weak
@@ -67,8 +67,9 @@
 @protocol P;
 
 void f(void);
+void __declspec(nothrow) g(void);
 
-void g() {
+void generate_ehtypes() {
   @try {
 f();
   } @catch (I *) {
@@ -79,3 +80,393 @@
   } @catch (id) {
   }
 }
+
+void basic_try_catch_1() {
+  @try {
+f();
+  } @catch (...) {
+g();
+  }
+}
+
+// CHECK-LABEL: define {{.*}}void @basic_try_catch_1(){{.*}} {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:invoke void @f()
+// CHECK-NEXT:to label %[[INVOKECONT:[^ ]+]] unwind label %[[DISPATCH:[^ ]+]]
+// CHECK:   [[DISPATCH]]:
+// CHECK-NEXT:%[[CATCHSWITCH:[^ ]+]] = catchswitch within none [label %[[CATCH:[^ ]+]]] unwind to caller
+// CHECK:   [[INVOKECONT]]:
+// CHECK-NEXT:br label %[[EHCONT:[^ ]+]]
+// CHECK:   [[EHCONT]]:
+// CHECK-NEXT:ret void
+// CHECK:   [[CATCH]]:
+// CHECK-NEXT:%[[CATCHPAD:[^ ]+]] = catchpad within %[[CATCHSWITCH]] [i8* null, i32 64, i8* null]
+// CHECK-NEXT:call void @g(){{.*}} [ "funclet"(token %[[CATCHPAD]]) ]
+// CHECK-NEXT:catchret from %[[CATCHPAD]] to label %[[CATCHRETDEST:[^ ]+]]
+// CHECK:   [[CATCHRETDEST]]:
+// CHECK-NEXT:br label %[[EHCONT]]
+// CHECK-NEXT:  }
+
+void basic_try_catch_2() {
+  @try {
+f();
+  } @catch (id) {
+g();
+  }
+}
+
+// CHECK-LABEL: define {{.*}}void @basic_try_catch_2(){{.*}} {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:invoke void @f()
+// CHECK-NEXT:to label %[[INVOKECONT:[^ ]+]] unwind label %[[DISPATCH:[^ ]+]]
+// CHECK:   [[DISPATCH]]:
+// CHECK-NEXT:%[[CATCHSWITCH:[^ ]+]] = catchswitch within none [label %[[CATCH:[^ ]+]]] unwind to caller
+// CHECK:   [[INVOKECONT]]:
+// CHECK-NEXT:br label %[[EHCONT:[^ ]+]]
+// CHECK:   [[EHCONT]]:
+// CHECK-NEXT:ret void
+// CHECK:   

[PATCH] D47862: [CodeGen] Always use MSVC personality for windows-msvc targets

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC334253: [CodeGen] Always use MSVC personality for 
windows-msvc targets (authored by smeenai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D47862?vs=150243=150437#toc

Repository:
  rC Clang

https://reviews.llvm.org/D47862

Files:
  lib/CodeGen/CGException.cpp
  test/CodeGenObjC/personality.m
  test/CodeGenObjCXX/personality.mm

Index: test/CodeGenObjC/personality.m
===
--- test/CodeGenObjC/personality.m
+++ test/CodeGenObjC/personality.m
@@ -11,14 +11,14 @@
 // RUN: %clang_cc1 -triple i686-unknown-linux-gnu -fexceptions -fseh-exceptions -fobjc-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-OBJFW-SEH
 // RUN: %clang_cc1 -triple i686-unknown-linux-gnu -fexceptions -fsjlj-exceptions -fobjc-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-OBJFW-SJLJ
 
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=ios -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=watchos -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-1.7 -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-GNUSTEP-1_7
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-GNUSTEP
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=gcc -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-GCC
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-OBJFW
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=ios -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=watchos -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-1.7 -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=gcc -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
 
 // RUN: %clang_cc1 -triple i686-unknown-windows-gnu -fexceptions -fobjc-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-MACOSX-FRAGILE
 // RUN: %clang_cc1 -triple i686-unknown-windows-gnu -fexceptions -fdwarf-exceptions -fobjc-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-MACOSX-FRAGILE-MINGW-DWARF
@@ -49,8 +49,7 @@
 // CHECK-OBJFW-SEH: personality i8* bitcast (i32 (...)* @__gnu_objc_personality_seh0 to i8*)
 // CHECK-OBJFW-SJLJ: personality i8* bitcast (i32 (...)* @__gnu_objc_personality_sj0 to i8*)
 
-// CHECK-WIN-MACOSX-FRAGILE: personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
-// CHECK-WIN-NS: personality i8* bitcast (i32 (...)* @__CxxFrameHandler3  to i8*)
+// CHECK-WIN-MSVC: personality i8* bitcast (i32 (...)* @__CxxFrameHandler3  to i8*)
 
 // CHECK-MACOSX-FRAGILE-MINGW-DWARF: personality i8* bitcast (i32 (...)* @__gcc_personality_v0 to i8*)
 // CHECK-MACOSX-FRAGILE-MINGW-SEH: personality i8* bitcast (i32 (...)* @__gcc_personality_seh0 to i8*)
Index: test/CodeGenObjCXX/personality.mm
===

[PATCH] D47862: [CodeGen] Always use MSVC personality for windows-msvc targets

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added inline comments.



Comment at: lib/CodeGen/CGException.cpp:134-135
   const llvm::Triple  = Target.getTriple();
+  if (T.isWindowsMSVCEnvironment())
+return EHPersonality::MSVC_CxxFrameHandler3;
+

rnk wrote:
> I guess previously we carefully arranged to go to whatever the regular objc 
> personality would be, but we're not doing that now with funclet windows eh.
> Maybe we could drastically simplify this by checking for an msvc environment 
> and seh before checking each language?
I gave this a shot. EHPersonality::get already has code to make SEH functions 
always get the SEH personality (line 223 below), so I added another condition 
below to make an MSVC triple always get the MSVC personality, and then removed 
all those conditionals from the individual get*Personality functions. 
Unfortunately, there's also code below (in SimplifyPersonality) that calls 
getCXXPersonality directly, so we still need to leave the MSVC check in there, 
and having the check in there but not any of the other get*Personality 
functions seems really ugly. The other option is to adjust SimplifyPersonality 
instead, which ends up looking like P8086; do you think that's worth it? Or did 
you mean something else entirely?


Repository:
  rC Clang

https://reviews.llvm.org/D47862



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


[PATCH] D47853: [Frontend] Disallow non-MSVC exception models for windows-msvc targets

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334243: [Frontend] Disallow non-MSVC exception models for 
windows-msvc targets (authored by smeenai, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47853

Files:
  cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/personality.c
  cfe/trunk/test/CodeGenCXX/ms-eh-personality.cpp
  cfe/trunk/test/CodeGenCXX/personality.cpp
  cfe/trunk/test/CodeGenObjC/personality.m
  cfe/trunk/test/CodeGenObjCXX/personality.mm
  cfe/trunk/test/Frontend/windows-exceptions.cpp

Index: cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -118,6 +118,8 @@
 "invalid value '%1' in '%0'; alignment must be a power of 2">;
 def err_fe_invalid_wchar_type
 : Error<"invalid wchar_t type '%0'; must be one of 'char', 'short', 'int'">;
+def err_fe_invalid_exception_model
+   : Error<"invalid exception model '%0' for target '%1'">;
 
 def warn_fe_serialized_diag_merge_failure : Warning<
 "unable to merge a subprocess's serialized diagnostics">,
Index: cfe/trunk/test/Frontend/windows-exceptions.cpp
===
--- cfe/trunk/test/Frontend/windows-exceptions.cpp
+++ cfe/trunk/test/Frontend/windows-exceptions.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fsyntax-only %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fdwarf-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-DWARF %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fseh-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-SEH %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fsjlj-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-SJLJ %s
+
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fdwarf-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-DWARF %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fseh-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-SEH %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fsjlj-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-SJLJ %s
+
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fdwarf-exceptions %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fseh-exceptions %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fsjlj-exceptions %s
+
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fdwarf-exceptions %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fseh-exceptions %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fsjlj-exceptions %s
+
+// MSVC-X86-DWARF: error: invalid exception model 'fdwarf-exceptions' for target 'i686--windows-msvc'
+// MSVC-X86-SEH: error: invalid exception model 'fseh-exceptions' for target 'i686--windows-msvc'
+// MSVC-X86-SJLJ: error: invalid exception model 'fsjlj-exceptions' for target 'i686--windows-msvc'
+
+// MSVC-X64-DWARF: error: invalid exception model 'fdwarf-exceptions' for target 'x86_64--windows-msvc'
+// MSVC-X64-SEH: error: invalid exception model 'fseh-exceptions' for target 'x86_64--windows-msvc'
+// MSVC-X64-SJLJ: error: invalid exception model 'fsjlj-exceptions' for target 'x86_64--windows-msvc'
Index: cfe/trunk/test/CodeGen/personality.c
===
--- cfe/trunk/test/CodeGen/personality.c
+++ cfe/trunk/test/CodeGen/personality.c
@@ -4,13 +4,13 @@
 // RUN: %clang_cc1 -triple i686-unknown-linux-gnu -fexceptions -fsjlj-exceptions -fblocks -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-SJLJ
 
 // RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fblocks -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fdwarf-exceptions -fblocks -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-DWARF
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -D __SEH_EXCEPTIONS__ -fms-extensions -fexceptions -fblocks -fseh-exceptions -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-SEH -check-prefix CHECK-WIN-SEH-X86
-// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -D __SEH_EXCEPTIONS__ -fms-extensions -fexceptions -fblocks -fseh-exceptions -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-SEH -check-prefix CHECK-WIN-SEH-X64
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fblocks 

[PATCH] D47912: [CMake] Consider LLVM_APPEND_VC_REV when generating SVNVersion.inc

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

I believe LLVM_APPEND_VC_REV controls whether the revision is appended to LLVM 
version string (e.g. the output of `llvm-config --version`), whereas the 
SVNRevision.inc file (which is what's causing the relink after amending) is 
used for e.g. the `clang --version` output, so I'm not sure this is the right 
thing to do. I would also love for an option to disable the SVNRevision stuff 
entirely though.


Repository:
  rC Clang

https://reviews.llvm.org/D47912



___
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

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Ping @rjmccall


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] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai updated this revision to Diff 150399.
smeenai added a comment.

Address objc_exception attribute case


Repository:
  rC Clang

https://reviews.llvm.org/D47233

Files:
  lib/CodeGen/CGCXXABI.h
  lib/CodeGen/CGObjCMac.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  test/CodeGenObjC/dllstorage.m
  test/CodeGenObjC/exceptions-msvc.m

Index: test/CodeGenObjC/exceptions-msvc.m
===
--- /dev/null
+++ test/CodeGenObjC/exceptions-msvc.m
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+
+#if __has_feature(objc_arc)
+#define WEAK __weak
+#else
+#define WEAK
+#endif
+
+// CHECK-DAG: $OBJC_EHTYPE_id = comdat any
+// X86-DAG: @OBJC_EHTYPE_id = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [18 x i8] c".PAUobjc_object@@\00" }, comdat
+// X64-DAG: @OBJC_EHTYPE_id = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [19 x i8] c".PEAUobjc_object@@\00" }, comdat
+
+@class I;
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_I" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_I" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUI@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_I" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUI@@\00" }, comdat
+
+@class J;
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_J" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_J" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUJ@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_J" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUJ@@\00" }, comdat
+
+// The EHType shouldn't be exported
+__declspec(dllexport)
+__attribute__((objc_root_class))
+@interface K
+@end
+
+@implementation K
+@end
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_K" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_K" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUK@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_K" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUK@@\00" }, comdat
+
+__attribute__((objc_root_class))
+__attribute__((objc_runtime_name("NotL")))
+@interface L
+@end
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_NotL" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_NotL" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUL@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_NotL" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUL@@\00" }, comdat
+
+@class M;
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_M" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_M" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUM@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_M" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUM@@\00" }, comdat
+
+// The EHType shouldn't be generated for this definition, since it's not referenced.
+__attribute__((objc_root_class))
+__attribute__((objc_exception))
+@interface N
+@end
+
+@implementation N
+@end
+
+// CHECK-NOT: @"OBJC_EHTYPE_$_N"
+
+@protocol P;
+
+void f(void);
+
+void g() {
+  @try {
+f();
+  } @catch (I *) {
+  } @catch (J *) {
+  } @catch (K *) {
+  } @catch (L *) {
+  } @catch (M *WEAK) {
+  } @catch (id) {
+  }
+}
Index: test/CodeGenObjC/dllstorage.m
===
--- test/CodeGenObjC/dllstorage.m
+++ test/CodeGenObjC/dllstorage.m
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fdeclspec -fobjc-runtime=ios -fobjc-exceptions -S -emit-llvm -o - %s | FileCheck -check-prefix CHECK-IR %s
-// RUN: %clang_cc1 -triple i686-windows-itanium -fms-extensions -fobjc-runtime=macosx -fdeclspec -fobjc-exceptions -S -emit-llvm -o - %s | FileCheck -check-prefix CHECK-IR %s
+// RUN: %clang_cc1 -triple i686-windows-itanium -fms-extensions -fobjc-runtime=macosx -fdeclspec -fobjc-exceptions -S -emit-llvm -o - %s | FileCheck -check-prefix CHECK-IR -check-prefix CHECK-ITANIUM %s
 // RUN: %clang_cc1 -triple i686-windows-itanium -fms-extensions -fobjc-runtime=objfw -fdeclspec -fobjc-exceptions -S -emit-llvm -o - %s | FileCheck -check-prefix CHECK-FW %s
 
 // CHECK-IR-DAG: @_objc_empty_cache = external dllimport global %struct._objc_cache

[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334224: [Parse] Use CapturedStmt for @finally on MSVC 
(authored by smeenai, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47564

Files:
  cfe/trunk/include/clang/AST/Stmt.h
  cfe/trunk/include/clang/Basic/CapturedStmt.h
  cfe/trunk/include/clang/Sema/ScopeInfo.h
  cfe/trunk/lib/Parse/ParseObjc.cpp
  cfe/trunk/test/SemaObjC/finally-msvc.m


Index: cfe/trunk/test/SemaObjC/finally-msvc.m
===
--- cfe/trunk/test/SemaObjC/finally-msvc.m
+++ cfe/trunk/test/SemaObjC/finally-msvc.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fexceptions -fobjc-exceptions 
-ast-dump %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fexceptions -fobjc-exceptions 
-ast-dump %s 2>&1 | FileCheck %s
+
+void f() {
+  @try {
+  } @finally {
+  }
+}
+
+// CHECK:  ObjCAtFinallyStmt
+// CHECK-NEXT:   CapturedStmt
+// CHECK-NEXT: CapturedDecl
+// CHECK-NEXT:   CompoundStmt
+// CHECK-NEXT:   ImplicitParamDecl
Index: cfe/trunk/lib/Parse/ParseObjc.cpp
===
--- cfe/trunk/lib/Parse/ParseObjc.cpp
+++ cfe/trunk/lib/Parse/ParseObjc.cpp
@@ -2585,13 +2585,26 @@
   ParseScope FinallyScope(this,
   Scope::DeclScope | Scope::CompoundStmtScope);
 
+  bool ShouldCapture =
+  getTargetInfo().getTriple().isWindowsMSVCEnvironment();
+  if (ShouldCapture)
+Actions.ActOnCapturedRegionStart(Tok.getLocation(), getCurScope(),
+ CR_ObjCAtFinally, 1);
+
   StmtResult FinallyBody(true);
   if (Tok.is(tok::l_brace))
 FinallyBody = ParseCompoundStatementBody();
   else
 Diag(Tok, diag::err_expected) << tok::l_brace;
-  if (FinallyBody.isInvalid())
+
+  if (FinallyBody.isInvalid()) {
 FinallyBody = Actions.ActOnNullStmt(Tok.getLocation());
+if (ShouldCapture)
+  Actions.ActOnCapturedRegionError();
+  } else if (ShouldCapture) {
+FinallyBody = Actions.ActOnCapturedRegionEnd(FinallyBody.get());
+  }
+
   FinallyStmt = Actions.ActOnObjCAtFinallyStmt(AtCatchFinallyLoc,
FinallyBody.get());
   catch_or_finally_seen = true;
Index: cfe/trunk/include/clang/AST/Stmt.h
===
--- cfe/trunk/include/clang/AST/Stmt.h
+++ cfe/trunk/include/clang/AST/Stmt.h
@@ -2133,7 +2133,7 @@
 
   /// The pointer part is the implicit the outlined function and the 
   /// int part is the captured region kind, 'CR_Default' etc.
-  llvm::PointerIntPair CapDeclAndKind;
+  llvm::PointerIntPair CapDeclAndKind;
 
   /// The record for captured variables, a RecordDecl or CXXRecordDecl.
   RecordDecl *TheRecordDecl = nullptr;
Index: cfe/trunk/include/clang/Sema/ScopeInfo.h
===
--- cfe/trunk/include/clang/Sema/ScopeInfo.h
+++ cfe/trunk/include/clang/Sema/ScopeInfo.h
@@ -748,6 +748,8 @@
 switch (CapRegionKind) {
 case CR_Default:
   return "default captured statement";
+case CR_ObjCAtFinally:
+  return "Objective-C @finally statement";
 case CR_OpenMP:
   return "OpenMP region";
 }
Index: cfe/trunk/include/clang/Basic/CapturedStmt.h
===
--- cfe/trunk/include/clang/Basic/CapturedStmt.h
+++ cfe/trunk/include/clang/Basic/CapturedStmt.h
@@ -16,6 +16,7 @@
 /// The different kinds of captured statement.
 enum CapturedRegionKind {
   CR_Default,
+  CR_ObjCAtFinally,
   CR_OpenMP
 };
 


Index: cfe/trunk/test/SemaObjC/finally-msvc.m
===
--- cfe/trunk/test/SemaObjC/finally-msvc.m
+++ cfe/trunk/test/SemaObjC/finally-msvc.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fexceptions -fobjc-exceptions -ast-dump %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fexceptions -fobjc-exceptions -ast-dump %s 2>&1 | FileCheck %s
+
+void f() {
+  @try {
+  } @finally {
+  }
+}
+
+// CHECK:  ObjCAtFinallyStmt
+// CHECK-NEXT:   CapturedStmt
+// CHECK-NEXT: CapturedDecl
+// CHECK-NEXT:   CompoundStmt
+// CHECK-NEXT:   ImplicitParamDecl
Index: cfe/trunk/lib/Parse/ParseObjc.cpp
===
--- cfe/trunk/lib/Parse/ParseObjc.cpp
+++ cfe/trunk/lib/Parse/ParseObjc.cpp
@@ -2585,13 +2585,26 @@
   ParseScope FinallyScope(this,
   Scope::DeclScope | Scope::CompoundStmtScope);
 
+  bool ShouldCapture =
+  getTargetInfo().getTriple().isWindowsMSVCEnvironment();
+  if (ShouldCapture)
+Actions.ActOnCapturedRegionStart(Tok.getLocation(), 

[PATCH] D47853: [Frontend] Disallow non-MSVC exception models for windows-msvc targets

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai updated this revision to Diff 150394.
smeenai marked 2 inline comments as done.
smeenai added a comment.

Add back missing MinGW coverage


Repository:
  rC Clang

https://reviews.llvm.org/D47853

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/personality.c
  test/CodeGenCXX/ms-eh-personality.cpp
  test/CodeGenCXX/personality.cpp
  test/CodeGenObjC/personality.m
  test/CodeGenObjCXX/personality.mm
  test/Frontend/windows-exceptions.cpp

Index: test/Frontend/windows-exceptions.cpp
===
--- /dev/null
+++ test/Frontend/windows-exceptions.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fsyntax-only %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fdwarf-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-DWARF %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fseh-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-SEH %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fsjlj-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-SJLJ %s
+
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fdwarf-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-DWARF %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fseh-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-SEH %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fsjlj-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-SJLJ %s
+
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fdwarf-exceptions %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fseh-exceptions %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fsjlj-exceptions %s
+
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fdwarf-exceptions %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fseh-exceptions %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fsjlj-exceptions %s
+
+// MSVC-X86-DWARF: error: invalid exception model 'fdwarf-exceptions' for target 'i686--windows-msvc'
+// MSVC-X86-SEH: error: invalid exception model 'fseh-exceptions' for target 'i686--windows-msvc'
+// MSVC-X86-SJLJ: error: invalid exception model 'fsjlj-exceptions' for target 'i686--windows-msvc'
+
+// MSVC-X64-DWARF: error: invalid exception model 'fdwarf-exceptions' for target 'x86_64--windows-msvc'
+// MSVC-X64-SEH: error: invalid exception model 'fseh-exceptions' for target 'x86_64--windows-msvc'
+// MSVC-X64-SJLJ: error: invalid exception model 'fsjlj-exceptions' for target 'x86_64--windows-msvc'
Index: test/CodeGenObjCXX/personality.mm
===
--- test/CodeGenObjCXX/personality.mm
+++ test/CodeGenObjCXX/personality.mm
@@ -26,31 +26,13 @@
 // RUN: %clang_cc1 -triple i686-unknown-linux-gnu -fexceptions -fsjlj-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-OBJFW-SJLJ
 
 // RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fdwarf-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE-DWARF
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fseh-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE-SEH
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fsjlj-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE-SJLJ
 // RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fdwarf-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS-DWARF
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fseh-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fsjlj-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix 

[PATCH] D47853: [Frontend] Disallow non-MSVC exception models for windows-msvc targets

2018-06-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai marked 2 inline comments as done.
smeenai added inline comments.



Comment at: test/CodeGen/personality.c:10
-// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -D __SEH_EXCEPTIONS__ 
-fms-extensions -fexceptions -fblocks -fseh-exceptions -S -emit-llvm %s -o - | 
FileCheck %s -check-prefix CHECK-WIN-SEH -check-prefix CHECK-WIN-SEH-X64
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fblocks 
-fsjlj-exceptions -S -emit-llvm %s -o - | FileCheck %s -check-prefix 
CHECK-WIN-SJLJ
 

mstorsjo wrote:
> I'd prefer if you didn't remove these tests, but instead retarget them to use 
> a `-gnu` triplet, to keep testing where you can explicitly choose between 
> sjlj/dwarf/seh for mingw setups.
I'll re-add those back; I didnt' realize they weren't already covered. I'm 
gonna drop the __SEH_EXCEPTIONS__ thing for MinGW, since AFAIK SEH 
`__try`/`__finally` doesn't work there.



Comment at: test/CodeGenCXX/personality.cpp:9
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions 
-fseh-exceptions -fcxx-exceptions -S -emit-llvm %s -o - | FileCheck %s 
-check-prefix CHECK-WIN-SEH
-// %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fsjlj-exceptions 
-fcxx-exceptions -S -emit-llvm %s -o - | FileCheck %s -check-prefix 
CHECK-WIN-SJLJ
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -D __SEH_EXCEPTIONS__ 
-fms-extensions -fexceptions -fseh-exceptions -fcxx-exceptions -S -emit-llvm %s 
-o - | FileCheck %s -check-prefix CHECK-WIN-SEH-X86

mstorsjo wrote:
> Same here, please keep the existing tests but retarget them to gnu/mingw.
This should all be covered by the group of RUN lines right below this one.


Repository:
  rC Clang

https://reviews.llvm.org/D47853



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


[PATCH] D47862: [CodeGen] Always use MSVC personality for windows-msvc targets

2018-06-06 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: compnerd, DHowett-MSFT, rnk.

The windows-msvc target is meant to be ABI compatible with MSVC,
including the exception handling. Ensure that a windows-msvc triple
always equates to the MSVC personality being used.

This mostly affects the GNUStep and ObjFW Obj-C runtimes. To the best of
my knowledge, those are normally not used with windows-msvc triples. I
believe WinObjC is based on GNUStep (or it at least uses libobjc2), but
that also takes the approach of wrapping Obj-C exceptions in C++
exceptions, so the MSVC personality function is the right one to use
there as well.


Repository:
  rC Clang

https://reviews.llvm.org/D47862

Files:
  lib/CodeGen/CGException.cpp
  test/CodeGenObjC/personality.m
  test/CodeGenObjCXX/personality.mm

Index: test/CodeGenObjCXX/personality.mm
===
--- test/CodeGenObjCXX/personality.mm
+++ test/CodeGenObjCXX/personality.mm
@@ -25,14 +25,14 @@
 // RUN: %clang_cc1 -triple i686-unknown-linux-gnu -fexceptions -fseh-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-OBJFW-SEH
 // RUN: %clang_cc1 -triple i686-unknown-linux-gnu -fexceptions -fsjlj-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-OBJFW-SJLJ
 
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=ios -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=watchos -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=gnustep-1.7 -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-GNUSTEP-1_7
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=gnustep -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-GNUSTEP
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=gcc -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-GCC
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-OBJFW
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=ios -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=watchos -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=gnustep-1.7 -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=gnustep -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=gcc -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
+// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MSVC
 
 // RUN: %clang_cc1 -triple i686-unknown-windows-gnu -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-MACOSX-FRAGILE
 // RUN: %clang_cc1 -triple i686-unknown-windows-gnu -fexceptions -fdwarf-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-MACOSX-FRAGILE
@@ -80,8 +80,7 @@
 // CHECK-OBJFW-SEH: personality i8* bitcast (i32 (...)* 

[PATCH] D47853: [Frontend] Disallow non-MSVC exception models for windows-msvc targets

2018-06-06 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: compnerd, mstorsjo, rnk.

The windows-msvc target is used for MSVC ABI compatibility, including
the exceptions model. It doesn't make sense to pair a windows-msvc
target with a non-MSVC exception model. This would previously cause an
assertion failure; explicitly error out for it in the frontend instead.
This also allows us to reduce the matrix of target/exception models a
bit (see the modified tests), and we can possibly simplify some of the
personality code in a follow-up.


Repository:
  rC Clang

https://reviews.llvm.org/D47853

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/personality.c
  test/CodeGenCXX/ms-eh-personality.cpp
  test/CodeGenCXX/personality.cpp
  test/CodeGenObjC/personality.m
  test/CodeGenObjCXX/personality.mm
  test/Frontend/windows-exceptions.cpp

Index: test/Frontend/windows-exceptions.cpp
===
--- /dev/null
+++ test/Frontend/windows-exceptions.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fsyntax-only %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fdwarf-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-DWARF %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fseh-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-SEH %s
+// RUN: not %clang_cc1 -triple i686--windows-msvc -fsyntax-only -fsjlj-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X86-SJLJ %s
+
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fdwarf-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-DWARF %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fseh-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-SEH %s
+// RUN: not %clang_cc1 -triple x86_64--windows-msvc -fsyntax-only -fsjlj-exceptions %s 2>&1 | FileCheck -check-prefix=MSVC-X64-SJLJ %s
+
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fdwarf-exceptions %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fseh-exceptions %s
+// RUN: %clang_cc1 -triple i686--windows-gnu -fsyntax-only -fsjlj-exceptions %s
+
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fdwarf-exceptions %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fseh-exceptions %s
+// RUN: %clang_cc1 -triple x86_64--windows-gnu -fsyntax-only -fsjlj-exceptions %s
+
+// MSVC-X86-DWARF: error: invalid exception model 'fdwarf-exceptions' for target 'i686--windows-msvc'
+// MSVC-X86-SEH: error: invalid exception model 'fseh-exceptions' for target 'i686--windows-msvc'
+// MSVC-X86-SJLJ: error: invalid exception model 'fsjlj-exceptions' for target 'i686--windows-msvc'
+
+// MSVC-X64-DWARF: error: invalid exception model 'fdwarf-exceptions' for target 'x86_64--windows-msvc'
+// MSVC-X64-SEH: error: invalid exception model 'fseh-exceptions' for target 'x86_64--windows-msvc'
+// MSVC-X64-SJLJ: error: invalid exception model 'fsjlj-exceptions' for target 'x86_64--windows-msvc'
Index: test/CodeGenObjCXX/personality.mm
===
--- test/CodeGenObjCXX/personality.mm
+++ test/CodeGenObjCXX/personality.mm
@@ -26,31 +26,13 @@
 // RUN: %clang_cc1 -triple i686-unknown-linux-gnu -fexceptions -fsjlj-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=objfw -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-OBJFW-SJLJ
 
 // RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fdwarf-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE-DWARF
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fseh-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE-SEH
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fsjlj-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx-fragile -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-MACOSX-FRAGILE-SJLJ
 // RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS
-// RUN: %clang_cc1 -triple i686-unknown-windows-msvc -fexceptions -fdwarf-exceptions -fobjc-exceptions -fcxx-exceptions -fobjc-runtime=macosx -S -emit-llvm %s -o - | FileCheck %s -check-prefix CHECK-WIN-NS-DWARF
-// RUN: 

[PATCH] D47850: [Driver] Stop passing -fseh-exceptions for x86_64-windows-msvc

2018-06-06 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334145: [Driver] Stop passing -fseh-exceptions for 
x86_64-windows-msvc (authored by smeenai, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47850

Files:
  cfe/trunk/lib/Driver/ToolChain.cpp
  cfe/trunk/test/Driver/windows-exceptions.cpp


Index: cfe/trunk/test/Driver/windows-exceptions.cpp
===
--- cfe/trunk/test/Driver/windows-exceptions.cpp
+++ cfe/trunk/test/Driver/windows-exceptions.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang -target i686-windows-msvc -c %s -### 2>&1 | FileCheck 
-check-prefix=MSVC %s
+// RUN: %clang -target x86_64-windows-msvc -c %s -### 2>&1 | FileCheck 
-check-prefix=MSVC %s
+// RUN: %clang -target i686-windows-gnu -c %s -### 2>&1 | FileCheck 
-check-prefix=MINGW-DWARF %s
+// RUN: %clang -target x86_64-windows-gnu -c %s -### 2>&1 | FileCheck 
-check-prefix=MINGW-SEH %s
+
+MSVC-NOT: -fdwarf-exceptions
+MSVC-NOT: -fseh-exceptions
+MINGW-DWARF: -fdwarf-exceptions
+MINGW-SEH: -fseh-exceptions
Index: cfe/trunk/lib/Driver/ToolChain.cpp
===
--- cfe/trunk/lib/Driver/ToolChain.cpp
+++ cfe/trunk/lib/Driver/ToolChain.cpp
@@ -471,8 +471,6 @@
 
 llvm::ExceptionHandling
 ToolChain::GetExceptionModel(const llvm::opt::ArgList ) const {
-  if (Triple.isOSWindows() && Triple.getArch() != llvm::Triple::x86)
-return llvm::ExceptionHandling::WinEH;
   return llvm::ExceptionHandling::None;
 }
 


Index: cfe/trunk/test/Driver/windows-exceptions.cpp
===
--- cfe/trunk/test/Driver/windows-exceptions.cpp
+++ cfe/trunk/test/Driver/windows-exceptions.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang -target i686-windows-msvc -c %s -### 2>&1 | FileCheck -check-prefix=MSVC %s
+// RUN: %clang -target x86_64-windows-msvc -c %s -### 2>&1 | FileCheck -check-prefix=MSVC %s
+// RUN: %clang -target i686-windows-gnu -c %s -### 2>&1 | FileCheck -check-prefix=MINGW-DWARF %s
+// RUN: %clang -target x86_64-windows-gnu -c %s -### 2>&1 | FileCheck -check-prefix=MINGW-SEH %s
+
+MSVC-NOT: -fdwarf-exceptions
+MSVC-NOT: -fseh-exceptions
+MINGW-DWARF: -fdwarf-exceptions
+MINGW-SEH: -fseh-exceptions
Index: cfe/trunk/lib/Driver/ToolChain.cpp
===
--- cfe/trunk/lib/Driver/ToolChain.cpp
+++ cfe/trunk/lib/Driver/ToolChain.cpp
@@ -471,8 +471,6 @@
 
 llvm::ExceptionHandling
 ToolChain::GetExceptionModel(const llvm::opt::ArgList ) const {
-  if (Triple.isOSWindows() && Triple.getArch() != llvm::Triple::x86)
-return llvm::ExceptionHandling::WinEH;
   return llvm::ExceptionHandling::None;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47850: [Driver] Stop passing -fseh-exceptions for x86_64-windows-msvc

2018-06-06 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: compnerd, mstorsjo, rnk.
Herald added subscribers: JDevlieghere, aprantl.

-fseh-exceptions is only meaningful for MinGW targets, and that driver
already has logic to pass either -fdwarf-exceptiosn or -fseh-exceptions
as appropriate. -fseh-exceptions is just a no-op for MSVC triples, and
passing it to cc1 causes unnecessary confusion.


Repository:
  rC Clang

https://reviews.llvm.org/D47850

Files:
  lib/Driver/ToolChain.cpp
  test/Driver/windows-exceptions.cpp


Index: test/Driver/windows-exceptions.cpp
===
--- /dev/null
+++ test/Driver/windows-exceptions.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang -target i686-windows-msvc -c %s -### 2>&1 | FileCheck 
-check-prefix=MSVC %s
+// RUN: %clang -target x86_64-windows-msvc -c %s -### 2>&1 | FileCheck 
-check-prefix=MSVC %s
+// RUN: %clang -target i686-windows-gnu -c %s -### 2>&1 | FileCheck 
-check-prefix=MINGW-DWARF %s
+// RUN: %clang -target x86_64-windows-gnu -c %s -### 2>&1 | FileCheck 
-check-prefix=MINGW-SEH %s
+
+MSVC-NOT: -fdwarf-exceptions
+MSVC-NOT: -fseh-exceptions
+MINGW-DWARF: -fdwarf-exceptions
+MINGW-SEH: -fseh-exceptions
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -471,8 +471,6 @@
 
 llvm::ExceptionHandling
 ToolChain::GetExceptionModel(const llvm::opt::ArgList ) const {
-  if (Triple.isOSWindows() && Triple.getArch() != llvm::Triple::x86)
-return llvm::ExceptionHandling::WinEH;
   return llvm::ExceptionHandling::None;
 }
 


Index: test/Driver/windows-exceptions.cpp
===
--- /dev/null
+++ test/Driver/windows-exceptions.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang -target i686-windows-msvc -c %s -### 2>&1 | FileCheck -check-prefix=MSVC %s
+// RUN: %clang -target x86_64-windows-msvc -c %s -### 2>&1 | FileCheck -check-prefix=MSVC %s
+// RUN: %clang -target i686-windows-gnu -c %s -### 2>&1 | FileCheck -check-prefix=MINGW-DWARF %s
+// RUN: %clang -target x86_64-windows-gnu -c %s -### 2>&1 | FileCheck -check-prefix=MINGW-SEH %s
+
+MSVC-NOT: -fdwarf-exceptions
+MSVC-NOT: -fseh-exceptions
+MINGW-DWARF: -fdwarf-exceptions
+MINGW-SEH: -fseh-exceptions
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -471,8 +471,6 @@
 
 llvm::ExceptionHandling
 ToolChain::GetExceptionModel(const llvm::opt::ArgList ) const {
-  if (Triple.isOSWindows() && Triple.getArch() != llvm::Triple::x86)
-return llvm::ExceptionHandling::WinEH;
   return llvm::ExceptionHandling::None;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-06-04 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D47564#1119925, @rjmccall wrote:

> In https://reviews.llvm.org/D47564#1119874, @smeenai wrote:
>
> > In https://reviews.llvm.org/D47564#1118424, @rjmccall wrote:
> >
> > > No, it was just a general question.  Have you gotten this to a point 
> > > where it's testable?
> >
> >
> > Yup, it's been working fine in my local testing. There's one more patch 
> > that I need to put up, which actually handles doing proper codegen for 
> > @try/@catch/@finally; I'm working on cleaning that up right now.
>
>
> Okay.  And simple tests with throwing exceptions out of the `@finally` block 
> seem to work?


Yup, it works fine. It's essentially the same as calling a function inside a 
catch block which throws (since the finally is modeled as a catch-all, and the 
finally body is outlined into a function which gets called by that catch-all).


Repository:
  rC Clang

https://reviews.llvm.org/D47564



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


[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-06-01 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D47564#1118424, @rjmccall wrote:

> In https://reviews.llvm.org/D47564#1118423, @smeenai wrote:
>
> > In https://reviews.llvm.org/D47564#1118381, @rjmccall wrote:
> >
> > > That's an interesting idea.  I don't see any particular reason not to do 
> > > it this way if you're willing to accept that it's never going to support 
> > > the full control-flow possibilities of @finally.  You will need to add 
> > > JumpDiagnostics logic to prevent branches out of the block, and I don't 
> > > know how this will interact with attempts to throw an exception out.
> >
> >
> > There's already some logic in CapturedStmt to prevent branches out of the 
> > block:
> >
> > - Attempting to return will produce "cannot return from Objective-C 
> > @finally statement"
> > - Attempting to goto out of the block will result in "use of undeclared 
> > label", which is a bad diagnostic (and should be improved), but it does 
> > error
>
>
> Alright, that makes sense.
>
> > It should be possible to add support for returns, at least; the idea we'd 
> > discussed with @rnk was setting a flag in the captured function to indicate 
> > a return having been executed, and then reading that flag outside the 
> > captured function and acting on it appropriately. gotos would be more 
> > complicated, but I think we could make them work if we really wanted to.
> > 
> > Throwing an exception out should just work, I think; the outlined function 
> > will just participate normally in exception handling. Did you have a 
> > specific case you were thinking of?
>
> No, it was just a general question.  Have you gotten this to a point where 
> it's testable?


Yup, it's been working fine in my local testing. There's one more patch that I 
need to put up, which actually handles doing proper codegen for 
@try/@catch/@finally; I'm working on cleaning that up right now. The other 
piece of the puzzle is https://reviews.llvm.org/D47233, which emits the proper 
typeinfo required for this.


Repository:
  rC Clang

https://reviews.llvm.org/D47564



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


[PATCH] D47669: [cmake] Support LLD for CLANG_ORDER_FILE

2018-06-01 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL333810: [cmake] Support LLD for CLANG_ORDER_FILE (authored 
by smeenai, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47669

Files:
  cfe/trunk/tools/driver/CMakeLists.txt


Index: cfe/trunk/tools/driver/CMakeLists.txt
===
--- cfe/trunk/tools/driver/CMakeLists.txt
+++ cfe/trunk/tools/driver/CMakeLists.txt
@@ -98,13 +98,16 @@
   set(TOOL_INFO_BUILD_VERSION)
 endif()
 
-if(CLANG_ORDER_FILE AND (LD64_EXECUTABLE OR GOLD_EXECUTABLE))
+if(CLANG_ORDER_FILE AND
+   (LD64_EXECUTABLE OR LLVM_LINKER_IS_GOLD OR LLVM_LINKER_IS_LLD))
   include(CheckLinkerFlag)
 
   if (LD64_EXECUTABLE)
 set(LINKER_ORDER_FILE_OPTION "-Wl,-order_file,${CLANG_ORDER_FILE}")
-  elseif (GOLD_EXECUTABLE)
+  elseif (LLVM_LINKER_IS_GOLD)
 set(LINKER_ORDER_FILE_OPTION 
"-Wl,--section-ordering-file,${CLANG_ORDER_FILE}")
+  elseif (LLVM_LINKER_IS_LLD)
+set(LINKER_ORDER_FILE_OPTION 
"-Wl,--symbol-ordering-file,${CLANG_ORDER_FILE}")
   endif()
 
   # This is a test to ensure the actual order file works with the linker.


Index: cfe/trunk/tools/driver/CMakeLists.txt
===
--- cfe/trunk/tools/driver/CMakeLists.txt
+++ cfe/trunk/tools/driver/CMakeLists.txt
@@ -98,13 +98,16 @@
   set(TOOL_INFO_BUILD_VERSION)
 endif()
 
-if(CLANG_ORDER_FILE AND (LD64_EXECUTABLE OR GOLD_EXECUTABLE))
+if(CLANG_ORDER_FILE AND
+   (LD64_EXECUTABLE OR LLVM_LINKER_IS_GOLD OR LLVM_LINKER_IS_LLD))
   include(CheckLinkerFlag)
 
   if (LD64_EXECUTABLE)
 set(LINKER_ORDER_FILE_OPTION "-Wl,-order_file,${CLANG_ORDER_FILE}")
-  elseif (GOLD_EXECUTABLE)
+  elseif (LLVM_LINKER_IS_GOLD)
 set(LINKER_ORDER_FILE_OPTION "-Wl,--section-ordering-file,${CLANG_ORDER_FILE}")
+  elseif (LLVM_LINKER_IS_LLD)
+set(LINKER_ORDER_FILE_OPTION "-Wl,--symbol-ordering-file,${CLANG_ORDER_FILE}")
   endif()
 
   # This is a test to ensure the actual order file works with the linker.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47669: [cmake] Support LLD for CLANG_ORDER_FILE

2018-06-01 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: beanz, compnerd, phosek.
Herald added a subscriber: mgorny.
smeenai added a subscriber: alexshap.

LLD also supports order files using the `--symbol-ordering-file` option.
As the name would suggest, the order file format is slightly different
from gold; gold's order files specify section names, whereas LLD's
specify symbol names. Assuming you have an order file in the correct
format though, we should support using it with LLD.

Switch the check to actually use LLVM's linker detection rather than
just checking for the presence of the gold executable, since we might
have a gold executable present but be using LLD (or bfd for that matter)
as our linker.


Repository:
  rC Clang

https://reviews.llvm.org/D47669

Files:
  tools/driver/CMakeLists.txt


Index: tools/driver/CMakeLists.txt
===
--- tools/driver/CMakeLists.txt
+++ tools/driver/CMakeLists.txt
@@ -98,13 +98,16 @@
   set(TOOL_INFO_BUILD_VERSION)
 endif()
 
-if(CLANG_ORDER_FILE AND (LD64_EXECUTABLE OR GOLD_EXECUTABLE))
+if(CLANG_ORDER_FILE AND
+   (LD64_EXECUTABLE OR LLVM_LINKER_IS_GOLD OR LLVM_LINKER_IS_LLD))
   include(CheckLinkerFlag)
 
   if (LD64_EXECUTABLE)
 set(LINKER_ORDER_FILE_OPTION "-Wl,-order_file,${CLANG_ORDER_FILE}")
-  elseif (GOLD_EXECUTABLE)
+  elseif (LLVM_LINKER_IS_GOLD)
 set(LINKER_ORDER_FILE_OPTION 
"-Wl,--section-ordering-file,${CLANG_ORDER_FILE}")
+  elseif (LLVM_LINKER_IS_LLD)
+set(LINKER_ORDER_FILE_OPTION 
"-Wl,--symbol-ordering-file,${CLANG_ORDER_FILE}")
   endif()
 
   # This is a test to ensure the actual order file works with the linker.


Index: tools/driver/CMakeLists.txt
===
--- tools/driver/CMakeLists.txt
+++ tools/driver/CMakeLists.txt
@@ -98,13 +98,16 @@
   set(TOOL_INFO_BUILD_VERSION)
 endif()
 
-if(CLANG_ORDER_FILE AND (LD64_EXECUTABLE OR GOLD_EXECUTABLE))
+if(CLANG_ORDER_FILE AND
+   (LD64_EXECUTABLE OR LLVM_LINKER_IS_GOLD OR LLVM_LINKER_IS_LLD))
   include(CheckLinkerFlag)
 
   if (LD64_EXECUTABLE)
 set(LINKER_ORDER_FILE_OPTION "-Wl,-order_file,${CLANG_ORDER_FILE}")
-  elseif (GOLD_EXECUTABLE)
+  elseif (LLVM_LINKER_IS_GOLD)
 set(LINKER_ORDER_FILE_OPTION "-Wl,--section-ordering-file,${CLANG_ORDER_FILE}")
+  elseif (LLVM_LINKER_IS_LLD)
+set(LINKER_ORDER_FILE_OPTION "-Wl,--symbol-ordering-file,${CLANG_ORDER_FILE}")
   endif()
 
   # This is a test to ensure the actual order file works with the linker.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-05-31 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D47564#1118381, @rjmccall wrote:

> That's an interesting idea.  I don't see any particular reason not to do it 
> this way if you're willing to accept that it's never going to support the 
> full control-flow possibilities of @finally.  You will need to add 
> JumpDiagnostics logic to prevent branches out of the block, and I don't know 
> how this will interact with attempts to throw an exception out.


There's already some logic in CapturedStmt to prevent branches out of the block:

- Attempting to return will produce "cannot return from Objective-C @finally 
statement"
- Attempting to goto out of the block will result in "use of undeclared label", 
which is a bad diagnostic (and should be improved), but it does error

Are there any other branches you had in mind? I could add tests for them, 
perhaps, though it's also covered by `test/Sema/captured-statements.c`.

It should be possible to add support for returns, at least; the idea we'd 
discussed with @rnk was setting a flag in the captured function to indicate a 
return having been executed, and then reading that flag outside the captured 
function and acting on it appropriately. gotos would be more complicated, but I 
think we could make them work if we really wanted to.

Throwing an exception out should just work, I think; the outlined function will 
just participate normally in exception handling. Did you have a specific case 
you were thinking of?


Repository:
  rC Clang

https://reviews.llvm.org/D47564



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


[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-05-31 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai updated this revision to Diff 149351.
smeenai edited the summary of this revision.
smeenai added a comment.

@rnk comment


Repository:
  rC Clang

https://reviews.llvm.org/D47564

Files:
  include/clang/AST/Stmt.h
  include/clang/Basic/CapturedStmt.h
  include/clang/Sema/ScopeInfo.h
  lib/Parse/ParseObjc.cpp
  test/SemaObjC/finally-msvc.m


Index: test/SemaObjC/finally-msvc.m
===
--- /dev/null
+++ test/SemaObjC/finally-msvc.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fexceptions -fobjc-exceptions 
-ast-dump %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fexceptions -fobjc-exceptions 
-ast-dump %s 2>&1 | FileCheck %s
+
+void f() {
+  @try {
+  } @finally {
+  }
+}
+
+// CHECK:  ObjCAtFinallyStmt
+// CHECK-NEXT:   CapturedStmt
+// CHECK-NEXT: CapturedDecl
+// CHECK-NEXT:   CompoundStmt
+// CHECK-NEXT:   ImplicitParamDecl
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -2585,13 +2585,26 @@
   ParseScope FinallyScope(this,
   Scope::DeclScope | Scope::CompoundStmtScope);
 
+  bool ShouldCapture =
+  getTargetInfo().getTriple().isWindowsMSVCEnvironment();
+  if (ShouldCapture)
+Actions.ActOnCapturedRegionStart(Tok.getLocation(), getCurScope(),
+ CR_ObjCAtFinally, 1);
+
   StmtResult FinallyBody(true);
   if (Tok.is(tok::l_brace))
 FinallyBody = ParseCompoundStatementBody();
   else
 Diag(Tok, diag::err_expected) << tok::l_brace;
-  if (FinallyBody.isInvalid())
+
+  if (FinallyBody.isInvalid()) {
 FinallyBody = Actions.ActOnNullStmt(Tok.getLocation());
+if (ShouldCapture)
+  Actions.ActOnCapturedRegionError();
+  } else if (ShouldCapture) {
+FinallyBody = Actions.ActOnCapturedRegionEnd(FinallyBody.get());
+  }
+
   FinallyStmt = Actions.ActOnObjCAtFinallyStmt(AtCatchFinallyLoc,
FinallyBody.get());
   catch_or_finally_seen = true;
Index: include/clang/Sema/ScopeInfo.h
===
--- include/clang/Sema/ScopeInfo.h
+++ include/clang/Sema/ScopeInfo.h
@@ -748,6 +748,8 @@
 switch (CapRegionKind) {
 case CR_Default:
   return "default captured statement";
+case CR_ObjCAtFinally:
+  return "Objective-C @finally statement";
 case CR_OpenMP:
   return "OpenMP region";
 }
Index: include/clang/Basic/CapturedStmt.h
===
--- include/clang/Basic/CapturedStmt.h
+++ include/clang/Basic/CapturedStmt.h
@@ -16,6 +16,7 @@
 /// The different kinds of captured statement.
 enum CapturedRegionKind {
   CR_Default,
+  CR_ObjCAtFinally,
   CR_OpenMP
 };
 
Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -2133,7 +2133,7 @@
 
   /// The pointer part is the implicit the outlined function and the 
   /// int part is the captured region kind, 'CR_Default' etc.
-  llvm::PointerIntPair CapDeclAndKind;
+  llvm::PointerIntPair CapDeclAndKind;
 
   /// The record for captured variables, a RecordDecl or CXXRecordDecl.
   RecordDecl *TheRecordDecl = nullptr;


Index: test/SemaObjC/finally-msvc.m
===
--- /dev/null
+++ test/SemaObjC/finally-msvc.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fexceptions -fobjc-exceptions -ast-dump %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fexceptions -fobjc-exceptions -ast-dump %s 2>&1 | FileCheck %s
+
+void f() {
+  @try {
+  } @finally {
+  }
+}
+
+// CHECK:  ObjCAtFinallyStmt
+// CHECK-NEXT:   CapturedStmt
+// CHECK-NEXT: CapturedDecl
+// CHECK-NEXT:   CompoundStmt
+// CHECK-NEXT:   ImplicitParamDecl
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -2585,13 +2585,26 @@
   ParseScope FinallyScope(this,
   Scope::DeclScope | Scope::CompoundStmtScope);
 
+  bool ShouldCapture =
+  getTargetInfo().getTriple().isWindowsMSVCEnvironment();
+  if (ShouldCapture)
+Actions.ActOnCapturedRegionStart(Tok.getLocation(), getCurScope(),
+ CR_ObjCAtFinally, 1);
+
   StmtResult FinallyBody(true);
   if (Tok.is(tok::l_brace))
 FinallyBody = ParseCompoundStatementBody();
   else
 Diag(Tok, diag::err_expected) << tok::l_brace;
-  if (FinallyBody.isInvalid())
+
+  if (FinallyBody.isInvalid()) {
 FinallyBody = 

[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-05-30 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai updated this revision to Diff 149220.
smeenai added a comment.

Proper diff


Repository:
  rC Clang

https://reviews.llvm.org/D47564

Files:
  include/clang/AST/Stmt.h
  include/clang/Basic/CapturedStmt.h
  include/clang/Sema/ScopeInfo.h
  lib/Parse/ParseObjc.cpp
  test/SemaObjC/finally-msvc.m


Index: test/SemaObjC/finally-msvc.m
===
--- /dev/null
+++ test/SemaObjC/finally-msvc.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fexceptions -fobjc-exceptions 
-ast-dump %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fexceptions -fobjc-exceptions 
-ast-dump %s 2>&1 | FileCheck %s
+
+void f() {
+  @try {
+  } @finally {
+  }
+}
+
+// CHECK:  ObjCAtFinallyStmt
+// CHECK-NEXT:   CapturedStmt
+// CHECK-NEXT: CapturedDecl
+// CHECK-NEXT:   CompoundStmt
+// CHECK-NEXT:   ImplicitParamDecl
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -2585,13 +2585,28 @@
   ParseScope FinallyScope(this,
   Scope::DeclScope | Scope::CompoundStmtScope);
 
+  bool ShouldCapture = Actions.getASTContext()
+   .getTargetInfo()
+   .getTriple()
+   .isWindowsMSVCEnvironment();
+  if (ShouldCapture)
+Actions.ActOnCapturedRegionStart(Tok.getLocation(), getCurScope(),
+ CR_ObjCAtFinally, 1);
+
   StmtResult FinallyBody(true);
   if (Tok.is(tok::l_brace))
 FinallyBody = ParseCompoundStatementBody();
   else
 Diag(Tok, diag::err_expected) << tok::l_brace;
-  if (FinallyBody.isInvalid())
+
+  if (FinallyBody.isInvalid()) {
 FinallyBody = Actions.ActOnNullStmt(Tok.getLocation());
+if (ShouldCapture)
+  Actions.ActOnCapturedRegionError();
+  } else if (ShouldCapture) {
+FinallyBody = Actions.ActOnCapturedRegionEnd(FinallyBody.get());
+  }
+
   FinallyStmt = Actions.ActOnObjCAtFinallyStmt(AtCatchFinallyLoc,
FinallyBody.get());
   catch_or_finally_seen = true;
Index: include/clang/Sema/ScopeInfo.h
===
--- include/clang/Sema/ScopeInfo.h
+++ include/clang/Sema/ScopeInfo.h
@@ -748,6 +748,8 @@
 switch (CapRegionKind) {
 case CR_Default:
   return "default captured statement";
+case CR_ObjCAtFinally:
+  return "Objective-C @finally statement";
 case CR_OpenMP:
   return "OpenMP region";
 }
Index: include/clang/Basic/CapturedStmt.h
===
--- include/clang/Basic/CapturedStmt.h
+++ include/clang/Basic/CapturedStmt.h
@@ -16,6 +16,7 @@
 /// The different kinds of captured statement.
 enum CapturedRegionKind {
   CR_Default,
+  CR_ObjCAtFinally,
   CR_OpenMP
 };
 
Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -2133,7 +2133,7 @@
 
   /// The pointer part is the implicit the outlined function and the 
   /// int part is the captured region kind, 'CR_Default' etc.
-  llvm::PointerIntPair CapDeclAndKind;
+  llvm::PointerIntPair CapDeclAndKind;
 
   /// The record for captured variables, a RecordDecl or CXXRecordDecl.
   RecordDecl *TheRecordDecl = nullptr;


Index: test/SemaObjC/finally-msvc.m
===
--- /dev/null
+++ test/SemaObjC/finally-msvc.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fexceptions -fobjc-exceptions -ast-dump %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fexceptions -fobjc-exceptions -ast-dump %s 2>&1 | FileCheck %s
+
+void f() {
+  @try {
+  } @finally {
+  }
+}
+
+// CHECK:  ObjCAtFinallyStmt
+// CHECK-NEXT:   CapturedStmt
+// CHECK-NEXT: CapturedDecl
+// CHECK-NEXT:   CompoundStmt
+// CHECK-NEXT:   ImplicitParamDecl
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -2585,13 +2585,28 @@
   ParseScope FinallyScope(this,
   Scope::DeclScope | Scope::CompoundStmtScope);
 
+  bool ShouldCapture = Actions.getASTContext()
+   .getTargetInfo()
+   .getTriple()
+   .isWindowsMSVCEnvironment();
+  if (ShouldCapture)
+Actions.ActOnCapturedRegionStart(Tok.getLocation(), getCurScope(),
+ CR_ObjCAtFinally, 1);
+
   StmtResult FinallyBody(true);
   if (Tok.is(tok::l_brace))
 FinallyBody = ParseCompoundStatementBody();
   else
   

[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-05-30 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: rjmccall, rnk, rsmith.
Herald added a subscriber: cfe-commits.

The body of a @finally needs to be executed on both exceptional and
non-exceptional paths. On landingpad platforms, this is straightforward:
the @finally body is emitted as a normal (non-exceptional) cleanup, and
then a catch-all is emitted which branches to that cleanup (the cleanup
has code to conditionally re-throw based on a flag which is set by the
catch-all).

Unfortunately, we can't use the same approach for MSVC exceptions, where
the catch-all will be emitted as a catchpad. We can't just branch to the
cleanup from within the catchpad, since we can only exit it via a
catchret, at which point the exception is destroyed and we can't
rethrow. We could potentially emit the finally body inside the catchpad
and have the normal cleanup path somehow branch into it, but that would
require some new IR construct that could branch into a catchpad.

Instead, after discussing it with Reid Kleckner, we decided that
frontend outlining was the best approach, similar to how SEH __finally
works today. We decided to use CapturedStmt rather than CaptureFinder
(which is what __finally uses) since the latter doesn't handle a lot of
cases we care about, e.g. self accesses, property accesses, block
captures, etc. Extending CaptureFinder to handle those additional cases
proved unwieldy, whereas CapturedStmt already took care of all of those.
In theory __finally could also be moved over to CapturedStmt, which
would remove some existing limitations (e.g. the inability to capture
this), although CaptureFinder would still be needed for SEH filters.

The one case supported by @finally but not CapturedStmt (or
CaptureFinder for that matter) is arbitrary control flow out of the
@finally, e.g. having a return statement inside a @finally. We can add
that support as a follow-up, but in practice we've found it to be used
very rarely anyway.


Repository:
  rC Clang

https://reviews.llvm.org/D47564

Files:
  include/clang/AST/Stmt.h
  include/clang/Basic/CapturedStmt.h
  include/clang/Sema/ScopeInfo.h
  lib/CodeGen/CGCXXABI.h
  lib/CodeGen/CGObjCMac.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  lib/Parse/ParseObjc.cpp
  test/CodeGenObjC/dllstorage.m
  test/CodeGenObjC/exceptions-msvc.m
  test/SemaObjC/finally-msvc.m

Index: test/SemaObjC/finally-msvc.m
===
--- /dev/null
+++ test/SemaObjC/finally-msvc.m
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fexceptions -fobjc-exceptions -ast-dump %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fexceptions -fobjc-exceptions -ast-dump %s 2>&1 | FileCheck %s
+
+void f() {
+  @try {
+  } @finally {
+  }
+}
+
+// CHECK:  ObjCAtFinallyStmt
+// CHECK-NEXT:   CapturedStmt
+// CHECK-NEXT: CapturedDecl
+// CHECK-NEXT:   CompoundStmt
+// CHECK-NEXT:   ImplicitParamDecl
Index: test/CodeGenObjC/exceptions-msvc.m
===
--- /dev/null
+++ test/CodeGenObjC/exceptions-msvc.m
@@ -0,0 +1,69 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+
+#if __has_feature(objc_arc)
+#define WEAK __weak
+#else
+#define WEAK
+#endif
+
+// CHECK-DAG: $OBJC_EHTYPE_id = comdat any
+// X86-DAG: @OBJC_EHTYPE_id = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [18 x i8] c".PAUobjc_object@@\00" }, comdat
+// X64-DAG: @OBJC_EHTYPE_id = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [19 x i8] c".PEAUobjc_object@@\00" }, comdat
+
+@class I;
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_I" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_I" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUI@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_I" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUI@@\00" }, comdat
+
+@class J;
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_J" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_J" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUJ@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_J" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUJ@@\00" }, comdat
+
+// The EHType shouldn't be exported

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

2018-05-30 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D47233#1115630, @DHowett-MSFT wrote:

> 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.


Thanks for the pointer!

To clarify, when you're talking about mangling the EH names, do you mean the 
names of the typeinfo structures themselves (OBJC_EHTYPE_* in my 
implementation), or the typeinfo name strings inside those structures? The 
latter should be equivalent to structs for us too, i.e. `@catch (I *)` and 
`catch (struct I *)` would produce the same name in the generated type info.


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] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

2018-05-22 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai updated this revision to Diff 148133.
smeenai added a comment.

Colocate CHECK lines with declarations


Repository:
  rC Clang

https://reviews.llvm.org/D47233

Files:
  lib/CodeGen/CGCXXABI.h
  lib/CodeGen/CGObjCMac.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  test/CodeGenObjC/dllstorage.m
  test/CodeGenObjC/exceptions-msvc.m

Index: test/CodeGenObjC/exceptions-msvc.m
===
--- /dev/null
+++ test/CodeGenObjC/exceptions-msvc.m
@@ -0,0 +1,69 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+
+#if __has_feature(objc_arc)
+#define WEAK __weak
+#else
+#define WEAK
+#endif
+
+// CHECK-DAG: $OBJC_EHTYPE_id = comdat any
+// X86-DAG: @OBJC_EHTYPE_id = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [18 x i8] c".PAUobjc_object@@\00" }, comdat
+// X64-DAG: @OBJC_EHTYPE_id = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [19 x i8] c".PEAUobjc_object@@\00" }, comdat
+
+@class I;
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_I" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_I" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUI@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_I" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUI@@\00" }, comdat
+
+@class J;
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_J" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_J" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUJ@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_J" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUJ@@\00" }, comdat
+
+// The EHType shouldn't be exported
+__declspec(dllexport)
+__attribute__((objc_root_class))
+@interface K
+@end
+
+@implementation K
+@end
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_K" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_K" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUK@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_K" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUK@@\00" }, comdat
+
+__attribute__((objc_runtime_name("NotL")))
+@interface L
+@end
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_NotL" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_NotL" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUL@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_NotL" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUL@@\00" }, comdat
+
+@class M;
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_M" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_M" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUM@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_M" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUM@@\00" }, comdat
+
+@protocol P;
+
+void f(void);
+
+void g() {
+  @try {
+f();
+  } @catch (I *) {
+  } @catch (J *) {
+  } @catch (K *) {
+  } @catch (L *) {
+  } @catch (M *WEAK) {
+  } @catch (id) {
+  }
+}
Index: test/CodeGenObjC/dllstorage.m
===
--- test/CodeGenObjC/dllstorage.m
+++ test/CodeGenObjC/dllstorage.m
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fdeclspec -fobjc-runtime=ios -fobjc-exceptions -S -emit-llvm -o - %s | FileCheck -check-prefix CHECK-IR %s
-// RUN: %clang_cc1 -triple i686-windows-itanium -fms-extensions -fobjc-runtime=macosx -fdeclspec -fobjc-exceptions -S -emit-llvm -o - %s | FileCheck -check-prefix CHECK-IR %s
+// RUN: %clang_cc1 -triple i686-windows-itanium -fms-extensions -fobjc-runtime=macosx -fdeclspec -fobjc-exceptions -S -emit-llvm -o - %s | FileCheck -check-prefix CHECK-IR -check-prefix CHECK-ITANIUM %s
 // RUN: %clang_cc1 -triple i686-windows-itanium -fms-extensions -fobjc-runtime=objfw -fdeclspec -fobjc-exceptions -S -emit-llvm -o - %s | FileCheck -check-prefix CHECK-FW %s
 
 // CHECK-IR-DAG: @_objc_empty_cache = external dllimport global %struct._objc_cache
@@ -100,20 +100,20 @@
 @implementation N : I
 @end
 
-// CHECK-IR-DAG: @"OBJC_EHTYPE_$_N" = dso_local dllexport global %struct._objc_typeinfo
+// CHECK-ITANIUM-DAG: @"OBJC_EHTYPE_$_N" = dso_local dllexport global %struct._objc_typeinfo
 
 __declspec(dllimport)
 

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

2018-05-22 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai created this revision.
smeenai added reviewers: DHowett-MSFT, compnerd, majnemer, rjmccall, rnk.

We're implementing funclet-compatible code generation for Obj-C
exceptions when using the MSVC ABI. The idea is that the Obj-C runtime
will wrap Obj-C exceptions inside C++ exceptions, which allows for
interoperability with C++ exceptions (for Obj-C++) and zero-cost
exceptions. This is the approach taken by e.g. WinObjC, and I believe it
to be the best approach for Obj-C exceptions in the MSVC ABI.

The first step is emitting proper RTTI for Obj-C exception types. Since
we're wrapping Obj-C exceptions in C++ exceptions, the RTTI should be
identical, barring the name of the RTTI variable (OBJC_EHTYPE_$_*).
Since the MSVC ABI does not easily allow for cross-DLL data references
from within other data, we instead emit the RTTI locally wherever
needed, which is also how C++ RTTI works on that ABI.

Follow-up diffs will add code generation support for @try itself, but
I'm splitting it up to get early feedback and make review more
manageable.

Worked on with Saleem Abdulrasool .


Repository:
  rC Clang

https://reviews.llvm.org/D47233

Files:
  lib/CodeGen/CGCXXABI.h
  lib/CodeGen/CGObjCMac.cpp
  lib/CodeGen/MicrosoftCXXABI.cpp
  test/CodeGenObjC/dllstorage.m
  test/CodeGenObjC/exceptions-msvc.m

Index: test/CodeGenObjC/exceptions-msvc.m
===
--- /dev/null
+++ test/CodeGenObjC/exceptions-msvc.m
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -triple i686--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X86 %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+// RUN: %clang_cc1 -triple x86_64--windows-msvc -fobjc-runtime=ios-6.0 -fobjc-arc -fdeclspec -fexceptions -fobjc-exceptions -emit-llvm -o - %s | FileCheck -check-prefixes=CHECK,X64 %s
+
+// CHECK-DAG: $OBJC_EHTYPE_id = comdat any
+// X86-DAG: @OBJC_EHTYPE_id = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [18 x i8] c".PAUobjc_object@@\00" }, comdat
+// X64-DAG: @OBJC_EHTYPE_id = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [19 x i8] c".PEAUobjc_object@@\00" }, comdat
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_I" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_I" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUI@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_I" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUI@@\00" }, comdat
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_J" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_J" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUJ@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_J" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUJ@@\00" }, comdat
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_K" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_K" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUK@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_K" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUK@@\00" }, comdat
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_NotL" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_NotL" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUL@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_NotL" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUL@@\00" }, comdat
+
+// CHECK-DAG: $"OBJC_EHTYPE_$_M" = comdat any
+// X86-DAG: @"OBJC_EHTYPE_$_M" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [8 x i8] c".PAUM@@\00" }, comdat
+// X64-DAG: @"OBJC_EHTYPE_$_M" = linkonce_odr global {{%[^ ]+}} { i8** @"??_7type_info@@6B@", i8* null, [9 x i8] c".PEAUM@@\00" }, comdat
+
+#if __has_feature(objc_arc)
+#define WEAK __weak
+#else
+#define WEAK
+#endif
+
+@class I;
+@class J;
+
+// The EHType shouldn't be exported
+__declspec(dllexport)
+__attribute__((objc_root_class))
+@interface K
+@end
+
+@implementation K
+@end
+
+__attribute__((objc_runtime_name("NotL")))
+@interface L
+@end
+
+@class M;
+
+@protocol P;
+
+void f(void);
+
+void g() {
+  @try {
+f();
+  } @catch (I *) {
+  } @catch (J *) {
+  } @catch (K *) {
+  } @catch (L *) {
+  } @catch (M *WEAK) {
+  } @catch (id) {
+  }
+}
Index: test/CodeGenObjC/dllstorage.m
===
--- test/CodeGenObjC/dllstorage.m
+++ test/CodeGenObjC/dllstorage.m
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fdeclspec 

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

2018-05-11 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Emailed cfe-dev: http://lists.llvm.org/pipermail/cfe-dev/2018-May/057934.html


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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


[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

2018-05-09 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Yeah, I think we all agree now that a portability warning isn't really 
tractable. Note that even for the warnings that motivated this diff, they 
should have only fired if `size_t` and NSInteger had separate types, so it 
wasn't a portability warning in that sense to begin with (as in, it would only 
warn if there was a mismatch for your current target, not if there was a 
potential mismatch for any target).

We still have two options:

1. Special-case NSInteger/NSUInteger on Apple platforms so that they can always 
be printed using `%z` without any issues.
2. Relax the format specifier warnings (either under a new warning option, e.g. 
`-Wformat-relaxed`, or by relaxing `-Wformat` itself and adding something like 
`-Wformat-pedantic`) so that you don't warn when the specifier and the actual 
type have the same size and alignment, even when the actual type is different 
(which would also cover the case in 1).

I'm personally in favor of 2, and I can start a discussion on cfe-dev if you 
think we should try to achieve a broader consensus. Whichever option we went 
with, we would also have to ensure that the optimizer didn't do anything bad 
(as @aaron.ballman pointed out), both now and in the future.


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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


[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

2018-05-08 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D42933#1092048, @rjmccall wrote:

> I agree that the format-specifier checker is not intended to be a portability 
> checker.
>
> Any attempt to check portability problems has to account for two things:
>
> - Not all code is intended to be portable.  If you're going to warn about 
> portability problems, you need some way to not annoy programmers who might 
> have good reason to say that they only care about their code running on Linux 
> / macOS / 64-bit / 32-bit / whatever.  Generally this means splitting the 
> portability warning out as a separate warning group.
> - There are always established idioms for solving portability problems.  In 
> this case, a certain set of important typedefs from the C standard have been 
> blessed with dedicated format modifiers like "z", but every other typedef in 
> the world has to find some other solution, either by asserting that some 
> existing format is good enough (as NSInteger does) or by introducing a macro 
> that expands to an appropriate format (as some things in POSIX do).  A proper 
> format-portability warning would have to know about all of these conventions 
> (in order to check that e.g. part of the format string came from the right 
> macro), which just seems wildly out-of-scope for a compiler warning.


Apple's current recommendation for using printf with the NSInteger types is 
casting to a long, per 
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html.
 Are you suggesting that recommendation would change to using `%zd` instead?


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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


[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

2018-05-08 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a subscriber: rsmith.
smeenai added a comment.

In https://reviews.llvm.org/D42933#1091943, @jyknight wrote:

> In https://reviews.llvm.org/D42933#1091817, @jfb wrote:
>
> > In https://reviews.llvm.org/D42933#1091809, @jyknight wrote:
> >
> > > I also think that special casing these two specifiers doesn't make sense. 
> > > The problem is a general issue -- and one I've often found irritating. 
> > > This exact same situation comes up all the time in non-Darwin contexts 
> > > too.
> >
> >
> > I don't think that's true. In this very specific case the platform 
> > guarantees that `(sizeof(size_t) == sizeof(NSInteger)) && (sizeof(ssize_t) 
> > == sizeof(NSUInteger))` for all architectures this platform supports. This 
> > exact same situation does not come up all the time in other contexts 
> > because the `int` / `long` / `long long` distinction isn't backed by a 
> > portability guarantee. The warning is there to say "this code isn't 
> > portable!", but in the very specific case of `NSInteger` and `NSUInteger` 
> > it is and users rely on it so it cannot be broken. The warning is therefore 
> > spurious, users therefore rightly complain.
>
>
> The printf format specifier warning is not primarily a cross-platform 
> portability checker. And, although in a few limited cases it can act somewhat 
> like one, in general it does not. Take my previous example -- you get no 
> warning on a platform that has int64_t as a typedef for long -- if this 
> feature is to be useful as a portability checker, it should require that you 
> used the PRId64 macro. Or, given `ssize_t x = 0; printf("%ld", x);`, it 
> doesn't tell you to use "%zd" instead if ssize_t is a typedef for long -- 
> although to be portable you ought to.
>
> No, the major usefulness of the printf warning is to tell you that your code 
> is incorrect for the _current_ platform. And //most// importantly when you 
> chose the wrong size for your argument.
>
> Those types which have matched size and alignment are still different types, 
> and so it's technically appropriate to warn about using the wrong specifier 
> there, too. But it's also entirely reasonable to not want to be bothered by 
> such useless trivia, so skipping the warning when there's only a technical 
> and not actual mismatch seems entirely sensible. (I might suggest that it 
> should even be the default behavior for the warning, and if you want the 
> stricter checks you'd ask for them...but I bet I'll get more pushback on 
> that...)


+1 to everything you said; what are other people's opinions on this? @rsmith 
perhaps?


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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


[PATCH] D46593: Allow copy elision in path concatenation

2018-05-08 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Generating the patch from libc++ is fine (and this patch looks like it has sane 
paths).


https://reviews.llvm.org/D46593



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


[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

2018-05-08 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D42933#1091234, @aaron.ballman wrote:

> In https://reviews.llvm.org/D42933#1090268, @jfb wrote:
>
> > I was just looking at this, and I think @arphaman's patch is pretty much 
> > the right approach (with 2 suggested fixes below).
> >
> > I don't think the code we're currently warning on is broken: a user code 
> > has `NSInteger` with `%zd` or `NSUInteger` with `%zu`, and on all platforms 
> > which support those types the implementor has guaranteed that 
> > `(sizeof(size_t) == sizeof(NSInteger)) && (sizeof(ssize_t) == 
> > sizeof(NSUInteger))`.
>
>
> Yes, but is this guaranteed to be the case or does it happen to be the case? 
> I'm worried about the less mainstream uses where you might find ObjC code 
> where this does not hold but -Wformat points out a portability concern.


Also keep in mind that Obj-C might be used on non-Apple platforms, e.g. there's 
an active review going on for GNUstep ABI v2 right now 
(https://reviews.llvm.org/D46052), and WinObjC is also a thing. Those platforms 
might not necessarily provide the same guarantees or consistency that Apple's 
do.

>> I agree that, if we're playing C++ pedant and look at the typedefs, then 
>> it's undefined behavior and the code is broken.
> 
> This is reason alone to diagnose the code, because the optimizer is welcome 
> to use that UB to perform transformations the programmer did not expect. 
> However, I'm not certain our optimizer is currently paying attention to that 
> UB directly, so this may not be an immediate issue (but could be a pitfall 
> for the future) but I'm not certain about other optimizers for which 
> portability would still be a concern.

I would honestly find it a bit surprising (and scary) if the optimizer actually 
took advantage of UB in the case where the size and alignment of the specifier 
and the actual type matches.


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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


[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

2018-05-07 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

Note that the alignment matters in addition to the size.

The pattern I've seen internally is people using `%zd` for NSInteger and `%tu` 
for NSUInteger, since until clang 6 neither of those were format-checked at all.

I'd be fine with adding an option to relax the printf checking if the size and 
alignment of the specifier and the actual type match, even if the types 
themselves differ (`-Wformat-relaxed` or something similar), so that you'd 
still get warnings on cases where the specifier mismatch could cause runtime 
issues. I think that would be preferable to special-casing the Apple types.


Repository:
  rC Clang

https://reviews.llvm.org/D42933



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


[PATCH] D45713: [libclang] Fix the type of 'int (Foo);'

2018-05-01 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL331306: [libclang] Fix the type of int (Foo); 
(authored by smeenai, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D45713

Files:
  cfe/trunk/test/Index/paren-type.c
  cfe/trunk/test/Index/print-type.c
  cfe/trunk/tools/libclang/CXType.cpp


Index: cfe/trunk/tools/libclang/CXType.cpp
===
--- cfe/trunk/tools/libclang/CXType.cpp
+++ cfe/trunk/tools/libclang/CXType.cpp
@@ -119,6 +119,10 @@
 if (auto *ATT = T->getAs()) {
   return MakeCXType(ATT->getModifiedType(), TU);
 }
+// Handle paren types as the original type
+if (auto *PTT = T->getAs()) {
+  return MakeCXType(PTT->getInnerType(), TU);
+}
 
 ASTContext  = cxtu::getASTUnit(TU)->getASTContext();
 if (Ctx.getLangOpts().ObjC1) {
Index: cfe/trunk/test/Index/print-type.c
===
--- cfe/trunk/test/Index/print-type.c
+++ cfe/trunk/test/Index/print-type.c
@@ -23,11 +23,11 @@
 // CHECK: TypeRef=FooType:1:13 [type=FooType] [typekind=Typedef] 
[canonicaltype=int] [canonicaltypekind=Int] [isPOD=1]
 // CHECK: ParmDecl=arr:3:40 (Definition) [type=int [5]] 
[typekind=ConstantArray] [isPOD=1]
 // CHECK: IntegerLiteral= [type=int] [typekind=Int] [isPOD=1]
-// CHECK: ParmDecl=fn:3:55 (Definition) [type=void (*)(int)] 
[typekind=Pointer] [canonicaltype=void (*)(int)] [canonicaltypekind=Pointer] 
[isPOD=1] [pointeetype=void (int)] [pointeekind=Unexposed]
+// CHECK: ParmDecl=fn:3:55 (Definition) [type=void (*)(int)] 
[typekind=Pointer] [canonicaltype=void (*)(int)] [canonicaltypekind=Pointer] 
[isPOD=1] [pointeetype=void (int)] [pointeekind=FunctionProto]
 // CHECK: ParmDecl=:3:62 (Definition) [type=int] [typekind=Int] [isPOD=1]
 // CHECK: CompoundStmt= [type=] [typekind=Invalid] [isPOD=0]
 // CHECK: CallExpr=fn:3:55 [type=void] [typekind=Void] [args= [int] [Int]] 
[isPOD=0]
-// CHECK: DeclRefExpr=fn:3:55 [type=void (*)(int)] [typekind=Pointer] 
[canonicaltype=void (*)(int)] [canonicaltypekind=Pointer] [isPOD=1] 
[pointeetype=void (int)] [pointeekind=Unexposed]
+// CHECK: DeclRefExpr=fn:3:55 [type=void (*)(int)] [typekind=Pointer] 
[canonicaltype=void (*)(int)] [canonicaltypekind=Pointer] [isPOD=1] 
[pointeetype=void (int)] [pointeekind=FunctionProto]
 // CHECK: UnaryOperator= [type=int] [typekind=Int] [isPOD=1]
 // CHECK: DeclRefExpr=p:3:13 [type=int *] [typekind=Pointer] [isPOD=1] 
[pointeetype=int] [pointeekind=Int]
 // CHECK: DeclStmt= [type=] [typekind=Invalid] [isPOD=0]
Index: cfe/trunk/test/Index/paren-type.c
===
--- cfe/trunk/test/Index/paren-type.c
+++ cfe/trunk/test/Index/paren-type.c
@@ -0,0 +1,16 @@
+// RUN: c-index-test -test-print-type %s | FileCheck --check-prefix=CHECK-TYPE 
%s
+// RUN: c-index-test -test-print-type-declaration %s | FileCheck 
--check-prefix=CHECK-TYPEDECL %s
+
+// CHECK-TYPE: VarDecl=VariableWithParentheses:
+// CHECK-TYPE-SAME: [type=int] [typekind=Int]
+// CHECK-TYPE-NOT: canonicaltype
+// CHECK-TYPE-SAME: isPOD
+extern int (VariableWithParentheses);
+
+typedef int MyTypedef;
+// CHECK-TYPE: VarDecl=VariableWithParentheses2:
+// CHECK-TYPE-SAME: [type=MyTypedef] [typekind=Typedef]
+// CHECK-TYPE-SAME: [canonicaltype=int] [canonicaltypekind=Int]
+// CHECK-TYPEDECL: VarDecl=VariableWithParentheses2
+// CHECK-TYPEDECL-SAME: [typedeclaration=MyTypedef] [typekind=Typedef]
+extern MyTypedef (VariableWithParentheses2);


Index: cfe/trunk/tools/libclang/CXType.cpp
===
--- cfe/trunk/tools/libclang/CXType.cpp
+++ cfe/trunk/tools/libclang/CXType.cpp
@@ -119,6 +119,10 @@
 if (auto *ATT = T->getAs()) {
   return MakeCXType(ATT->getModifiedType(), TU);
 }
+// Handle paren types as the original type
+if (auto *PTT = T->getAs()) {
+  return MakeCXType(PTT->getInnerType(), TU);
+}
 
 ASTContext  = cxtu::getASTUnit(TU)->getASTContext();
 if (Ctx.getLangOpts().ObjC1) {
Index: cfe/trunk/test/Index/print-type.c
===
--- cfe/trunk/test/Index/print-type.c
+++ cfe/trunk/test/Index/print-type.c
@@ -23,11 +23,11 @@
 // CHECK: TypeRef=FooType:1:13 [type=FooType] [typekind=Typedef] [canonicaltype=int] [canonicaltypekind=Int] [isPOD=1]
 // CHECK: ParmDecl=arr:3:40 (Definition) [type=int [5]] [typekind=ConstantArray] [isPOD=1]
 // CHECK: IntegerLiteral= [type=int] [typekind=Int] [isPOD=1]
-// CHECK: ParmDecl=fn:3:55 (Definition) [type=void (*)(int)] [typekind=Pointer] [canonicaltype=void (*)(int)] [canonicaltypekind=Pointer] [isPOD=1] [pointeetype=void (int)] [pointeekind=Unexposed]
+// CHECK: ParmDecl=fn:3:55 (Definition) [type=void (*)(int)] [typekind=Pointer] [canonicaltype=void (*)(int)] [canonicaltypekind=Pointer] [isPOD=1] 

[PATCH] D45779: [ARM] Remove redundant #if in test

2018-05-01 Thread Shoaib Meenai via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC331305: [ARM] Remove redundant #if in test. NFC (authored by 
smeenai, committed by ).
Herald added a reviewer: javed.absar.

Changed prior to commit:
  https://reviews.llvm.org/D45779?vs=142969=144775#toc

Repository:
  rC Clang

https://reviews.llvm.org/D45779

Files:
  test/CodeGen/arm-aapcs-vfp.c


Index: test/CodeGen/arm-aapcs-vfp.c
===
--- test/CodeGen/arm-aapcs-vfp.c
+++ test/CodeGen/arm-aapcs-vfp.c
@@ -17,11 +17,7 @@
 // RUN:   -ffreestanding \
 // RUN:   -emit-llvm -w -o - %s | FileCheck -check-prefix=CHECK64 %s
 
-#ifdef __arm64__
 #include 
-#else
-#include 
-#endif
 
 struct homogeneous_struct {
   float f[2];


Index: test/CodeGen/arm-aapcs-vfp.c
===
--- test/CodeGen/arm-aapcs-vfp.c
+++ test/CodeGen/arm-aapcs-vfp.c
@@ -17,11 +17,7 @@
 // RUN:   -ffreestanding \
 // RUN:   -emit-llvm -w -o - %s | FileCheck -check-prefix=CHECK64 %s
 
-#ifdef __arm64__
 #include 
-#else
-#include 
-#endif
 
 struct homogeneous_struct {
   float f[2];
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46287: [Driver] Don't add -dwarf-column-info when using -gcodeview on non-msvc targets

2018-04-30 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai accepted this revision.
smeenai added a comment.
This revision is now accepted and ready to land.

Makes sense to me. You may wanna wait for @zturner, but I can't really imagine 
any benefit to restricting this to MSVC.


Repository:
  rC Clang

https://reviews.llvm.org/D46287



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


[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

2018-04-25 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment.

In https://reviews.llvm.org/D45639#1078494, @thakis wrote:

> > because the headers that are part of Clang toolchain are incompatible with 
> > the system library
>
> Do you have details on this? This isn't supposed to be the case as far as I 
> know. We link chrome/mac against system libc++ with the bundled headers, and 
> it at least seems to work fine (and, from what I understand, is supposed to 
> work as well).


Agreed; this sounds bad. libc++ goes to great pains to maintain both forward 
and backward compatibility in its headers; @dexonsmith should weigh in here.


Repository:
  rC Clang

https://reviews.llvm.org/D45639



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


  1   2   3   4   >