[PATCH] D29001: [Modules] Fallback to building the module if a timeout occurs

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
Herald added a subscriber: fhahn.

After https://reviews.llvm.org/D28299 gets in, clang will use PCMCache to 
manage memory buffers for pcm files. This means the lock file manager isn't 
needed for correctness anymore, but only as an optimization: clang waits for 
other processes to finish up building a module if they are already doing it.

Emit remark instead of errors to notify users when a timeout occures and 
actually build the module in case that happens.


https://reviews.llvm.org/D29001

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  lib/Frontend/CompilerInstance.cpp


Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1180,10 +1180,14 @@
 llvm::LockFileManager Locked(ModuleFileName);
 switch (Locked) {
 case llvm::LockFileManager::LFS_Error:
-  Diags.Report(ModuleNameLoc, diag::err_module_lock_failure)
+  // PCMCache takes care of correctness and locks are only necessary for
+  // performance. If there are errors creating the lock, do not use it
+  // and fallback to building the module ourselves.
+  Diags.Report(ModuleNameLoc, diag::remark_module_lock_failure)
   << Module->Name << Locked.getErrorMessage();
-  return false;
-
+  // Clear the lock file in case there's some leftover around.
+  Locked.unsafeRemoveLockFile();
+  // FALLTHROUGH
 case llvm::LockFileManager::LFS_Owned:
   // We're responsible for building the module ourselves.
   if (!compileModuleImpl(ImportingInstance, ModuleNameLoc, Module,
@@ -1203,11 +1207,14 @@
   case llvm::LockFileManager::Res_OwnerDied:
 continue; // try again to get the lock.
   case llvm::LockFileManager::Res_Timeout:
-Diags.Report(ModuleNameLoc, diag::err_module_lock_timeout)
+// Since PCMCache takes care of correctness, we try waiting for another
+// process to complete the build so that this isn't done twice. If we
+// reach a timeout, it's not a problem, try to build it ourselves then.
+Diags.Report(ModuleNameLoc, diag::remark_module_lock_timeout)
 << Module->Name;
 // Clear the lock file so that future invokations can make progress.
 Locked.unsafeRemoveLockFile();
-return false;
+continue;
   }
   break;
 }
Index: include/clang/Basic/DiagnosticCommonKinds.td
===
--- include/clang/Basic/DiagnosticCommonKinds.td
+++ include/clang/Basic/DiagnosticCommonKinds.td
@@ -88,10 +88,10 @@
   "module '%0' %select{is incompatible with|requires}1 feature '%2'">;
 def err_module_header_missing : Error<
   "%select{|umbrella }0header '%1' not found">;
-def err_module_lock_failure : Error<
-  "could not acquire lock file for module '%0': %1">, DefaultFatal;
-def err_module_lock_timeout : Error<
-  "timed out waiting to acquire lock file for module '%0'">, DefaultFatal;
+def remark_module_lock_failure : Remark<
+  "could not acquire lock file for module '%0': %1">, InGroup;
+def remark_module_lock_timeout : Remark<
+  "timed out waiting to acquire lock file for module '%0'">, 
InGroup;
 def err_module_cycle : Error<"cyclic dependency in module '%0': %1">, 
   DefaultFatal;
 def err_module_prebuilt : Error<


Index: lib/Frontend/CompilerInstance.cpp
===
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1180,10 +1180,14 @@
 llvm::LockFileManager Locked(ModuleFileName);
 switch (Locked) {
 case llvm::LockFileManager::LFS_Error:
-  Diags.Report(ModuleNameLoc, diag::err_module_lock_failure)
+  // PCMCache takes care of correctness and locks are only necessary for
+  // performance. If there are errors creating the lock, do not use it
+  // and fallback to building the module ourselves.
+  Diags.Report(ModuleNameLoc, diag::remark_module_lock_failure)
   << Module->Name << Locked.getErrorMessage();
-  return false;
-
+  // Clear the lock file in case there's some leftover around.
+  Locked.unsafeRemoveLockFile();
+  // FALLTHROUGH
 case llvm::LockFileManager::LFS_Owned:
   // We're responsible for building the module ourselves.
   if (!compileModuleImpl(ImportingInstance, ModuleNameLoc, Module,
@@ -1203,11 +1207,14 @@
   case llvm::LockFileManager::Res_OwnerDied:
 continue; // try again to get the lock.
   case llvm::LockFileManager::Res_Timeout:
-Diags.Report(ModuleNameLoc, diag::err_module_lock_timeout)
+// Since PCMCache takes care of correctness, we try waiting for another
+// process to complete the build so that this isn't done twice. If we
+// reach a timeout, it's not a problem, try to build it ourselves then.
+Diags.Report(ModuleNameLoc, 

[PATCH] D27257: [CodeCompletion] Ensure that ObjC root class completes instance methods from protocols and categories as well

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

Ok. Thanks Alex, LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D27257



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

[PATCH] D27257: [CodeCompletion] Ensure that ObjC root class completes instance methods from protocols and categories as well

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

How does this interact (if at all) with classes annotated with  
`__attribute__((objc_root_class))`?




Comment at: lib/Sema/SemaCodeComplete.cpp:5259
SelIdents, CurContext, Selectors,
AllowSameLength, Results, false);
 

`false` -> `false /*IsRootClass*/`


Repository:
  rL LLVM

https://reviews.llvm.org/D27257



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

[PATCH] D28299: Module: use PCMCache to manage memory buffers for pcm files.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Serialization/ASTReader.cpp:3692
+ValidationConflicts);
+for (auto N : ValidationConflicts)
+  Diag(diag::err_module_ancestor_conflict) << N;

This should honor `ARR_OutOfDate`, so that the module can be rebuilt in case a 
user module mismatch via a different diagnostics 
(lib/Serialization/ASTReader.cpp:4076) and returns OutOfDate instead of Success:

if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
  for (auto N : ValidationConflicts)
Diag(diag::err_module_ancestor_conflict) << N;


https://reviews.llvm.org/D28299



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

[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.

This fixes PR31863, a regression introduced in r276159.

Consider this snippet:

struct FVector;
struct FVector {};
struct FBox {

  FVector Min;
  FBox(int);

};
namespace {
FBox InvalidBoundingBox(0);
}

While parsing the DECL_VAR for 'struct FBox', clang recursively read all the
dep decls until it finds the DECL_CXX_RECORD forward declaration for 'struct
FVector'. Then, it resumes all the way up back to DECL_VAR handling in
`ReadDeclRecord`, where it checks if `isConsumerInterestedIn` for the decl.

One of the condition for `isConsumerInterestedIn` to return false is if the
VarDecl is imported from a module `D->getImportedOwningModule()`, because it
will get emitted when we import the relevant module. However, before checking
if it comes from a module, clang checks if `Ctx.DeclMustBeEmitted(D)`, which
triggers the emission of 'struct FBox'. Since one of its fields is still
incomplete, it crashes.

Instead, check if `D->getImportedOwningModule()` is true before calling
`Ctx.DeclMustBeEmitted(D)`.


https://reviews.llvm.org/D29753

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/PCH/empty-def-fwd-struct.h


Index: test/PCH/empty-def-fwd-struct.h
===
--- /dev/null
+++ test/PCH/empty-def-fwd-struct.h
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -emit-pch -x c++-header %s -std=c++14 -o %t.pch
+// RUN: %clang_cc1 -emit-llvm -x c++ /dev/null -std=c++14 -include-pch %t.pch 
-o %t.o
+struct FVector;
+struct FVector {};
+struct FBox {
+  FVector Min;
+  FBox(int);
+};
+namespace {
+FBox InvalidBoundingBox(0);
+}
+
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2517,8 +2517,8 @@
 
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() 
&&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 
   if (isa(D) || 


Index: test/PCH/empty-def-fwd-struct.h
===
--- /dev/null
+++ test/PCH/empty-def-fwd-struct.h
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -emit-pch -x c++-header %s -std=c++14 -o %t.pch
+// RUN: %clang_cc1 -emit-llvm -x c++ /dev/null -std=c++14 -include-pch %t.pch -o %t.o
+struct FVector;
+struct FVector {};
+struct FBox {
+  FVector Min;
+  FBox(int);
+};
+namespace {
+FBox InvalidBoundingBox(0);
+}
+
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -2517,8 +2517,8 @@
 
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() &&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 
   if (isa(D) || 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28832: Improve redefinition errors pointing to the same header.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.

Diagnostics related to redefinition errors that point to the same header file 
do not provide much information that helps fixing the issue. In modules context 
it usually happens because of a non modular include, in non-module context it 
might happen because of the lack of header guardas. This patch tries to improve 
this situation by enhancing diagnostics in this particular scenario. If the 
approach seems reasonable, I can extend it to other relevant redefinition_error 
diagnostic call sites.

Modules
---

In file included from x.c:2:
Inputs4/C.h:3:5: error: redefinition of 'c'
int c = 1;

  ^

In module 'X' imported from x.c:1:
Inputs4/C.h:3:5: note: previous definition is here
int c = 1;

  ^

1 warning and 1 error generated.

After this patch

In file included from x.c:2:
Inputs4/C.h:3:5: error: redefinition of 'c'
int c = 1;

  ^

In module 'X' imported from x.c:1:
Inputs4/B.h:3:10: note: 'Inputs4/C.h' included multiple times, additional 
(likely non-modular) include site in module 'X.B'
#include "C.h"

  ^

1 error generated.
Inputs4/module.modulemap:6:10: note: consider adding 'Inputs4/C.h' as part of 
'X.B' definition in

  module B {
 ^

1 error generated.

Without Modules
---

In file included from x.c:2:
./a.h:1:5: error: redefinition of 'yyy'
int yyy = 42;

  ^

./a.h:1:5: note: previous definition is here
int yyy = 42;

  ^

1 error generated.

After this patch

In file included from x.c:2:
./a.h:1:5: error: redefinition of 'yyy'
int yyy = 42;

  ^

x.c:1:10: note: './a.h' included multiple times, consider augmenting this 
header with #ifdef guards
#include "a.h"

  ^

1 error generated.


https://reviews.llvm.org/D28832

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaDecl.cpp
  test/Modules/Inputs/SameHeader/A.h
  test/Modules/Inputs/SameHeader/B.h
  test/Modules/Inputs/SameHeader/C.h
  test/Modules/Inputs/SameHeader/module.modulemap
  test/Modules/redefinition-same-header.m
  test/Sema/redefinition-same-header.c

Index: test/Sema/redefinition-same-header.c
===
--- /dev/null
+++ test/Sema/redefinition-same-header.c
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'int yyy = 42;' > %t/a.h
+// RUN: %clang_cc1 -fsyntax-only %s -I%t  -verify
+
+// expected-error@a.h:1 {{redefinition of 'yyy'}}
+// expected-note-re@redefinition-same-header.c:9 {{'{{.*}}/a.h' included multiple times, consider augmenting this header with #ifdef guards}}
+
+#include "a.h"
+#include "a.h"
+
+int foo() { return yyy; }
Index: test/Modules/redefinition-same-header.m
===
--- /dev/null
+++ test/Modules/redefinition-same-header.m
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t.tmp
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SameHeader -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t.tmp %s -verify
+
+// expected-error@Inputs/SameHeader/C.h:3 {{redefinition of 'c'}}
+// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional (likely non-modular) include site in module 'X.B'}}
+// expected-note-re@Inputs/SameHeader/module.modulemap:6 {{consider adding '{{.*}}/C.h' as part of 'X.B' definition in}}
+
+#include "A.h" // maps to a modular
+#include "C.h" // textual include
Index: test/Modules/Inputs/SameHeader/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/SameHeader/module.modulemap
@@ -0,0 +1,11 @@
+module X {
+  module A {
+header "A.h"
+export *
+  }
+  module B {
+header "B.h"
+export *
+  }
+  export *
+}
Index: test/Modules/Inputs/SameHeader/C.h
===
--- /dev/null
+++ test/Modules/Inputs/SameHeader/C.h
@@ -0,0 +1,4 @@
+#ifndef __C_h__
+#define __C_h__
+int c = 1;
+#endif
Index: test/Modules/Inputs/SameHeader/B.h
===
--- /dev/null
+++ test/Modules/Inputs/SameHeader/B.h
@@ -0,0 +1,4 @@
+#ifndef __B_h__
+#define __B_h__
+#include "C.h"
+#endif
Index: test/Modules/Inputs/SameHeader/A.h
===
--- /dev/null
+++ test/Modules/Inputs/SameHeader/A.h
@@ -0,0 +1,3 @@
+#ifndef __A_h__
+#define __A_h__
+#endif
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -3728,6 +3728,49 @@
 New->setImplicitlyInline();
 }
 
+void Sema::diagnoseRedefinition(SourceLocation Old, SourceLocation New) {
+  SourceManager  = getSourceManager();
+  auto FNewDecLoc = SrcMgr.getDecomposedLoc(New);
+  auto FOldDecLoc = SrcMgr.getDecomposedLoc(Old);
+  auto *FNew = SrcMgr.getFileEntryForID(FNewDecLoc.first);
+  auto *FOld = SrcMgr.getFileEntryForID(FOldDecLoc.first);
+  // Is it the 

[PATCH] D25213: Fix PR28181: Prevent operator overloading related assertion failure crash that happens in C only

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM!




Comment at: lib/Sema/SemaExpr.cpp:11523
 // being assigned to.
 if (Opc == BO_Assign && pty->getKind() == BuiltinType::Overload) {
+  if (getLangOpts().CPlusPlus &&

Looks like you can fold both conditions below into one and check 
`getLangOpts().CPlusPlus` only once


Repository:
  rL LLVM

https://reviews.llvm.org/D25213



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

[PATCH] D28832: Improve redefinition errors pointing to the same header.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Thanks Vassil.

@rsmith is this ok for you?


https://reviews.llvm.org/D28832



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

[PATCH] D26955: Fix bitwidth for x87 extended-precision floating-point type

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Basic/TargetInfo.cpp:229
   switch (BitWidth) {
-  case 96:
+  case 80:
 if (() == ::APFloat::x87DoubleExtended)

The change makes sense but I believe there's some historical reason why this is 
96 instead of 80? What problem have you found in practice? Do you have a 
testcase or unittest that exercise the issue (and that could be added to the 
patch)?


https://reviews.llvm.org/D26955



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

[PATCH] D26916: [ObjC] Avoid a @try/@finally/@autoreleasepool fixit when parsing an expression

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Alex, thanks for working on this




Comment at: lib/Parse/ParseObjc.cpp:2877
+if (GetLookAheadToken(1).is(tok::l_brace) &&
+ExprStatementTokLoc == AtLoc) {
   char ch = Tok.getIdentifierInfo()->getNameStart()[0];

Does this only triggers when `Res.isInvalid()` returns true in the first part 
of the patch? I wonder if it's also safe to allow  `ExprStatementTokLoc = 
AtLoc;` for every path or only when it fails.


Repository:
  rL LLVM

https://reviews.llvm.org/D26916



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

[PATCH] D25866: [Sema] Support implicit scalar to vector conversions

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:8064
+  ScalarCast = CK_FloatingCast;
+} else if (ScalarTy->isIntegralType(S.Context)) {
+  // Determine if the integer constant can be expressed as a floating point

sdardis wrote:
> bruno wrote:
> > I don't see why it's necessary to check for all specific cases where the 
> > scalar is a constant. For all the others scenarios it should be enough to 
> > get the right answer via `getIntegerTypeOrder` or `getFloatTypeOrder`. For 
> > this is specific condition, the `else` part for the `CstScalar` below 
> > should also handle the constant case, right? 
> > 
> > 
> If we have a scalar constant, it is necessary to examine it's actual value to 
> see if it can be casted without truncation. The scalar constant's type is 
> somewhat irrelevant. Consider:
> 
>v4f32 f(v4f32 a) {
>  return a + (double)1.0;
>}
> 
> In this case examining the order only works if the vector's order is greater 
> than or equal to the scalar constant's order. Instead, if we ask whether the 
> scalar constant can be represented as a constant scalar of the vector's 
> element type, then we know if we can convert without the loss of precision.
> 
> For integers, the case is a little more contrived due to native integer 
> width, but the question is the same. Can a scalar constant X be represented 
> as a given floating point type. Consider:
> 
>v4f32 f(v4f32 a) {
>  return a + (signed int)1;
> }
> 
> This would rejected for platforms where a signed integer's width is greater 
> than the precision of float if we examined it based purely on types and their 
> sizes. Instead, if we convert the integer to the float point's type with 
> APFloat and convert back to see if we have the same value, we can determine 
> if the scalar constant can be safely converted without the loss of precision.
> 
> I based the logic examining the value of the constant scalar based on GCC's 
> behaviour, which implicitly converts scalars regardless of their type if they 
> can be represented in the same type of the vector's element type without the 
> loss of precision. If the scalar cannot be evaluated to a constant at compile 
> time, then the size in bits for the scalar's type determines if it can be 
> converted safely.
Right. Can you split the scalarTy <-> vectorEltTy checking logic into separate 
functions; one for cst-scalar-int-to-vec-elt-int and another for 
scalar-int-to-vec-elt-float? 



Comment at: lib/Sema/SemaExpr.cpp:8267
+  }
+
   // Otherwise, use the generic diagnostic.

sdardis wrote:
> bruno wrote:
> > This change seems orthogonal to this patch. Can you make it a separated 
> > patch with the changes from test/Sema/vector-cast.c?
> This change is a necessary part of this patch and it can't be split out in 
> sensible fashion.
> 
> For example, line 329 of test/Sema/zvector.c adds a scalar signed character 
> to a vector of signed characters. With just this change we would report 
> "cannot convert between scalar type 'signed char' and vector type '__vector 
> signed char' (vector of 16 'signed char' values) as implicit conversion would 
> cause truncation".
> 
> This is heavily misleading for C and C++ code as we don't perform implicit 
> conversions and signed char could be implicitly converted to a vector of 
> signed char without the loss of precision.
> 
> To make this diagnostic precise without performing implicit conversions 
> requires determining cases where implicit conversion would cause truncation 
> and reporting that failure reason, or defaulting to the generic diagnostic.
> 
> Keeping this change as part of this patch is cleaner and simpler as it covers 
> both implicit conversions and more precise diagnostics.
Can you double check whether we should support these GCC semantics for 
getLangOpts().ZVector? If not, then this should be introduced in a separate 
patch/commit.



Comment at: lib/Sema/SemaExpr.cpp:8020
+  if (Order < 0)
+return true;
+}

Please also early return `!CstScalar && Order < 0` like you did below.



Comment at: lib/Sema/SemaExpr.cpp:8064
+ !ScalarTy->hasSignedIntegerRepresentation());
+bool ignored = false;
+Float.convertToInteger(

`ignored` => `Ignored` 



Comment at: lib/Sema/SemaExpr.cpp:8171
+return LHSType;
+}
   }

It's not clear to me whether clang should support the GCC semantics when in 
`getLangOpts().AltiVec || getLangOpts().ZVector`, do you?

You can remove the curly braces for the `if` and `else` here.



Comment at: lib/Sema/SemaExpr.cpp:8182
+return RHSType;
+}
   }

Also remove braces here.


https://reviews.llvm.org/D25866



___
cfe-commits mailing list

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

@rsmith ping!


https://reviews.llvm.org/D26267



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

[PATCH] D27546: [ASTReader] Sort RawComments before merging

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
bruno added reviewers: manmanren, akyrtzi, rsmith.
bruno added a subscriber: cfe-commits.

`RawComments` are sorted by comparing underlying `SourceLocation`'s. This is 
done by comparing `FileID` and `Offset`; when the `FileID` is the same it means
the locations are within the same TU and the `Offset` is used, etc.

FileID, from the source code: "A mostly-opaque identifier, where 0 is 
"invalid", >0 is this module, and <-1 is something loaded from another
module.". That said, when de-serializing SourceLocations, FileID's from 
RawComments loaded from other modules get negative IDs where previously they
were positive. This makes imported RawComments unsorted, leading to a wrong 
merge with other comments from the current TU. Fix that by sorting RawComments
properly after de-serialization and before merge.

This fixes an assertion in `ASTContext::getRawCommentForDeclNoCache`, which 
fires only in a debug build of clang. There's seems to be no reliable way to 
test this.
Additionally, I tried to use `llvm::array_pod_sort`, but that didn't seem to 
cope well with `BeforeThanCompare(SourceMgr)` or lambdas, and a 
`SourceMgr` is needed to perform the sort.


https://reviews.llvm.org/D27546

Files:
  lib/Serialization/ASTReader.cpp


Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -8481,6 +8481,10 @@
   }
 }
   NextCursor:
+// De-serialized SourceLocations get negative FileIDs for other modules,
+// potentially invalidating the original order. Sort it again.
+std::sort(Comments.begin(), Comments.end(),
+  BeforeThanCompare(SourceMgr));
 Context.Comments.addDeserializedComments(Comments);
   }
 }


Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -8481,6 +8481,10 @@
   }
 }
   NextCursor:
+// De-serialized SourceLocations get negative FileIDs for other modules,
+// potentially invalidating the original order. Sort it again.
+std::sort(Comments.begin(), Comments.end(),
+  BeforeThanCompare(SourceMgr));
 Context.Comments.addDeserializedComments(Comments);
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Any thoughts on this @rsmith ?


https://reviews.llvm.org/D26267



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

[PATCH] D27604: [Driver] Add compiler option to generate a reproducer

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 80981.
bruno added a comment.

Update after Mehdi's review!


https://reviews.llvm.org/D27604

Files:
  docs/UsersManual.rst
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/Driver.h
  include/clang/Driver/Options.td
  lib/Driver/Driver.cpp
  test/Driver/crash-report-crashfile.m
  tools/driver/driver.cpp

Index: tools/driver/driver.cpp
===
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -460,8 +460,11 @@
 Res = TheDriver.ExecuteCompilation(*C, FailingCommands);
 
   // Force a crash to test the diagnostics.
-  if (::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH")) {
-Diags.Report(diag::err_drv_force_crash) << "FORCE_CLANG_DIAGNOSTICS_CRASH";
+  if (TheDriver.GenReproducer) {
+Diags.Report(diag::err_drv_force_crash)
+<< (!::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH")
+? "option '-gen-reproducer'"
+: "environment variable 'FORCE_CLANG_DIAGNOSTICS_CRASH'");
 
 // Pretend that every command failed.
 FailingCommands.clear();
Index: test/Driver/crash-report-crashfile.m
===
--- test/Driver/crash-report-crashfile.m
+++ test/Driver/crash-report-crashfile.m
@@ -3,15 +3,32 @@
 // RUN: mkdir -p %t/i %t/m %t
 
 // RUN: not env FORCE_CLANG_DIAGNOSTICS_CRASH= TMPDIR=%t TEMP=%t TMP=%t \
-// RUN: %clang -fsyntax-only %s -I %S/Inputs/module -isysroot %/t/i/\
-// RUN: -fmodules -fmodules-cache-path=%t/m/ -DFOO=BAR 2>&1 | FileCheck %s
+// RUN: %clang -fsyntax-only %s \
+// RUN:   -I %S/Inputs/module -isysroot %/t/i/ \
+// RUN:   -fmodules -fmodules-cache-path=%t/m/ -DFOO=BAR 2>&1 | \
+// RUN:   FileCheck -check-prefix=CRASH_ENV %s
+
+// RUN: not env TMPDIR=%t TEMP=%t TMP=%t \
+// RUN: %clang -gen-reproducer -fsyntax-only %s \
+// RUN:   -I %S/Inputs/module -isysroot %/t/i/ \
+// RUN:   -fmodules -fmodules-cache-path=%t/m/ -DFOO=BAR 2>&1 | \
+// RUN:   FileCheck -check-prefix=CRASH_FLAG %s
 
 @import simple;
 const int x = MODULE_MACRO;
 
-// CHECK: Preprocessed source(s) and associated run script(s) are located at:
-// CHECK-NEXT: note: diagnostic msg: {{.*}}.m
-// CHECK-NEXT: note: diagnostic msg: {{.*}}.cache
-// CHECK-NEXT: note: diagnostic msg: {{.*}}.sh
-// CHECK-NEXT: note: diagnostic msg: Crash backtrace is located in
-// CHECK-NEXT: note: diagnostic msg: {{.*}}Library/Logs/DiagnosticReports{{.*}}
+// CRASH_ENV: failing because environment variable 'FORCE_CLANG_DIAGNOSTICS_CRASH' is set
+// CRASH_ENV: Preprocessed source(s) and associated run script(s) are located at:
+// CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.m
+// CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.cache
+// CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.sh
+// CRASH_ENV-NEXT: note: diagnostic msg: Crash backtrace is located in
+// CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}Library/Logs/DiagnosticReports{{.*}}
+
+// CRASH_FLAG: failing because option '-gen-reproducer' is set
+// CRASH_FLAG: Preprocessed source(s) and associated run script(s) are located at:
+// CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.m
+// CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.cache
+// CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.sh
+// CRASH_FLAG-NEXT: note: diagnostic msg: Crash backtrace is located in
+// CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}Library/Logs/DiagnosticReports{{.*}}
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -62,7 +62,7 @@
   CCCPrintBindings(false), CCPrintHeaders(false), CCLogDiagnostics(false),
   CCGenDiagnostics(false), DefaultTargetTriple(DefaultTargetTriple),
   CCCGenericGCCName(""), CheckInputsExist(true), CCCUsePCH(true),
-  SuppressMissingInputWarning(false) {
+  GenReproducer(false), SuppressMissingInputWarning(false) {
 
   // Provide a sane fallback if no VFS is specified.
   if (!this->VFS)
@@ -596,6 +596,9 @@
 CCCGenericGCCName = A->getValue();
   CCCUsePCH =
   Args.hasFlag(options::OPT_ccc_pch_is_pch, options::OPT_ccc_pch_is_pth);
+  GenReproducer = Args.hasFlag(options::OPT_gen_reproducer,
+   options::OPT_fno_crash_diagnostics,
+   !!::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH"));
   // FIXME: DefaultTargetTriple is used by the target-prefixed calls to as/ld
   // and getToolChain is const.
   if (IsCLMode()) {
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -184,6 +184,8 @@
 def arcmt_migrate_emit_arc_errors : Flag<["-"], "arcmt-migrate-emit-errors">,
   HelpText<"Emit ARC errors even if the migrator can fix them">,
   Flags<[CC1Option]>;
+def gen_reproducer: Flag<["-"], "gen-reproducer">, InternalDebugOpt,
+  HelpText<"Auto-generates preprocessed source files and a 

[PATCH] D27604: [Driver] Add compiler option to generate a reproducer

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



> I am wondering if there is clearer name than `-gen-reproducer`.

Probably, I'm open to suggestions :-)

> IIUC, the goal is to extract all environment (system) dependent pieces, 
> copying them to a folder where the compiler can be chroot-ed (via 
> `-isysroot`).

It doesn't necessarily needs a `-isysroot` to work, only if the original 
invocation used it, then we need to use it as well to ask the VFS 
(transparently) for the right path locations.

> In some sense, what we do seems close to what `-header-include-file 
> list_of_includes.txt` does and additionally copying the contents of the 
> list_of_includes.txt (and possibly mounting a vfs overlay).

`-header-include-file` does something similar but more limited, I added some 
specific callbacks for the module collector back in the day that it might not 
collect yet though, for instance, it collects module.modulemap's and will soon 
collect header maps and some other stuff.

It also has some special handling for symlinks; it doesn't copy both files, but 
generate two yaml entries that point to the same "physical" location.


https://reviews.llvm.org/D27604



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

[PATCH] D27604: [Driver] Add compiler option to generate a reproducer

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
bruno added reviewers: rsmith, v.g.vassilev.
bruno added subscribers: cfe-commits, mehdi_amini.

One way to currently test the reproducers is to setup
"FORCE_CLANG_DIAGNOSTICS_CRASH=1" before invoking clang. This simulates
a crash and produces the same contents needed by the reproducers.

The reproducers are specially useful when triaging Modules issues, not
only on crashes, but also for reproducing misleading warnings, errors,
etc. I propose we add '-gen-reproducer' driver option to clang (or
any similar name) and give users a flag option.

Additionally, clang already have -fno-crash-diagnostics, which disables
the crash reproducers. I've decided not to propose "-fcrash-diagnostics"
since it doesn't convey the ideia of reproduction despite a crash.


https://reviews.llvm.org/D27604

Files:
  docs/UsersManual.rst
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/Driver.h
  include/clang/Driver/Options.td
  lib/Driver/Driver.cpp
  test/Driver/crash-report-crashfile.m
  tools/driver/driver.cpp

Index: tools/driver/driver.cpp
===
--- tools/driver/driver.cpp
+++ tools/driver/driver.cpp
@@ -460,8 +460,12 @@
 Res = TheDriver.ExecuteCompilation(*C, FailingCommands);
 
   // Force a crash to test the diagnostics.
-  if (::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH")) {
-Diags.Report(diag::err_drv_force_crash) << "FORCE_CLANG_DIAGNOSTICS_CRASH";
+  if (TheDriver.GenReproducer) {
+if (!::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH"))
+  Diags.Report(diag::err_drv_force_crash) << 1 << "-gen-reproducer";
+else
+  Diags.Report(diag::err_drv_force_crash)
+  << 0 << "FORCE_CLANG_DIAGNOSTICS_CRASH";
 
 // Pretend that every command failed.
 FailingCommands.clear();
Index: test/Driver/crash-report-crashfile.m
===
--- test/Driver/crash-report-crashfile.m
+++ test/Driver/crash-report-crashfile.m
@@ -3,15 +3,32 @@
 // RUN: mkdir -p %t/i %t/m %t
 
 // RUN: not env FORCE_CLANG_DIAGNOSTICS_CRASH= TMPDIR=%t TEMP=%t TMP=%t \
-// RUN: %clang -fsyntax-only %s -I %S/Inputs/module -isysroot %/t/i/\
-// RUN: -fmodules -fmodules-cache-path=%t/m/ -DFOO=BAR 2>&1 | FileCheck %s
+// RUN: %clang -fsyntax-only %s \
+// RUN:   -I %S/Inputs/module -isysroot %/t/i/ \
+// RUN:   -fmodules -fmodules-cache-path=%t/m/ -DFOO=BAR 2>&1 | \
+// RUN:   FileCheck -check-prefix=CRASH_ENV %s
+
+// RUN: not env TMPDIR=%t TEMP=%t TMP=%t \
+// RUN: %clang -gen-reproducer -fsyntax-only %s \
+// RUN:   -I %S/Inputs/module -isysroot %/t/i/ \
+// RUN:   -fmodules -fmodules-cache-path=%t/m/ -DFOO=BAR 2>&1 | \
+// RUN:   FileCheck -check-prefix=CRASH_FLAG %s
 
 @import simple;
 const int x = MODULE_MACRO;
 
-// CHECK: Preprocessed source(s) and associated run script(s) are located at:
-// CHECK-NEXT: note: diagnostic msg: {{.*}}.m
-// CHECK-NEXT: note: diagnostic msg: {{.*}}.cache
-// CHECK-NEXT: note: diagnostic msg: {{.*}}.sh
-// CHECK-NEXT: note: diagnostic msg: Crash backtrace is located in
-// CHECK-NEXT: note: diagnostic msg: {{.*}}Library/Logs/DiagnosticReports{{.*}}
+// CRASH_ENV: failing because environment variable 'FORCE_CLANG_DIAGNOSTICS_CRASH' is set
+// CRASH_ENV: Preprocessed source(s) and associated run script(s) are located at:
+// CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.m
+// CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.cache
+// CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}.sh
+// CRASH_ENV-NEXT: note: diagnostic msg: Crash backtrace is located in
+// CRASH_ENV-NEXT: note: diagnostic msg: {{.*}}Library/Logs/DiagnosticReports{{.*}}
+
+// CRASH_FLAG: failing because option '-gen-reproducer' is set
+// CRASH_FLAG: Preprocessed source(s) and associated run script(s) are located at:
+// CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.m
+// CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.cache
+// CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}.sh
+// CRASH_FLAG-NEXT: note: diagnostic msg: Crash backtrace is located in
+// CRASH_FLAG-NEXT: note: diagnostic msg: {{.*}}Library/Logs/DiagnosticReports{{.*}}
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -62,7 +62,7 @@
   CCCPrintBindings(false), CCPrintHeaders(false), CCLogDiagnostics(false),
   CCGenDiagnostics(false), DefaultTargetTriple(DefaultTargetTriple),
   CCCGenericGCCName(""), CheckInputsExist(true), CCCUsePCH(true),
-  SuppressMissingInputWarning(false) {
+  GenReproducer(false), SuppressMissingInputWarning(false) {
 
   // Provide a sane fallback if no VFS is specified.
   if (!this->VFS)
@@ -596,6 +596,9 @@
 CCCGenericGCCName = A->getValue();
   CCCUsePCH =
   Args.hasFlag(options::OPT_ccc_pch_is_pch, options::OPT_ccc_pch_is_pth);
+  GenReproducer = Args.hasFlag(options::OPT_gen_reproducer,
+   options::OPT_fno_crash_diagnostics,
+ 

[PATCH] D27796: Fix printf specifier handling: invalid specifier should not be marked as "consuming data arguments"

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D27796



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

[PATCH] D27546: [ASTReader] Sort RawComments before merging

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping!


https://reviews.llvm.org/D27546



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

[PATCH] D16533: Bug 20796 - GCC's -Wstrict-prototypes warning not implemented in Clang

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D16533



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

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 81478.
bruno added a comment.

Here is the Decl dump in the end of `ReadDeclRecord` for each Decl before it 
fails:

- ParmVarDecl 0x7ffe23053b20 

 col:12 in libc.math hidden 'int' -- FunctionDecl 0x7ffe23053a48 
 col:5 in libc.math hidden abs 'int (int)' `-ParmVarDecl 0x7ffe23053b20 
 col:12 in libc.math hidden 'int' -- TypedefDecl 0x7ffe23053f90 
 col:26 in libc.stddef hidden ptrdiff_t 'long' `-BuiltinType 
0x7ffe2300f9f0 'long' -- TypedefDecl 0x7ffe23054018 
 col:15 in libc.POSIX.sys.types hidden ptrdiff_t 'int *' `-PointerType 
0x7ffe23054070 'int *' imported `-BuiltinType 0x7ffe2300f9d0 'int' While 
building module 'libc++' imported from 
/Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stdio.h:4:
 In file included from :1: In file included from 
/Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h:7:
 In file included from 
/Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits:4:
 
/Users/bruno/Dev/srcs/llvm/tools/clang/test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef:7:9:
 error: unknown type name 'ptrdiff_t' typedef ptrdiff_t my_ptrdiff_t; ^

So, both `libc.stddef` and `libc++.stddef`, answer for the builtin 
`/Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h`. 
`libc++.stddef` is being currently built, that's why the definition isn't there 
yet. Looks like the right thing is to indeed re-enter 
`/Users/bruno/Dev/srcs/llvm/build/bin/../lib/clang/4.0.0/include/stddef.h`, so 
it can be put in `libc++.stddef`.

I've updated the patch to do what you suggested in the first place: track 
modules per header. Can you comment on the general approach? One consideration 
though; it's expensive to have the additional SmallVector per each 
HeaderFileInfo, since most headers (except builtins and textuals) aren't 
actually expected to have more than one associated Module. Maybe a map from 
HFIs to associated modules, but I'm not sure where to store it, suggestions? 
The Decl's loaded from the AST now make more sense (output resuming from the 
previously failing point):

  TypedefDecl 0x7f81ed0f0840 
 col:23 in libc.stddef hidden size_t 'unsigned long'
  `-BuiltinType 0x7f81ed0ab890 'unsigned long'
  --
  TypedefDecl 0x7f81ed0f0938 
 col:23 in libc.stddef hidden rsize_t 'unsigned long'
  `-BuiltinType 0x7f81ed0ab890 'unsigned long'
  --
  TypedefDecl 0x7f81ed076778 
 col:26 in libc.stddef hidden ptrdiff_t 'long'
  `-BuiltinType 0x7f81ed03a9f0 'long'
  --
  TypedefDecl 0x7f81ed076800 
 col:15 in libc.POSIX.sys.types hidden ptrdiff_t 'int *'
  `-PointerType 0x7f81ed076850 'int *' imported
`-BuiltinType 0x7f81ed03a9d0 'int'
  --
  TypedefDecl 0x7f81ed0768b0 prev 0x7f81ed076778 
 col:26 in libc++.stddef hidden referenced ptrdiff_t 'long'
  `-BuiltinType 0x7f81ed03a9f0 'long'
  --
  TypedefDecl 0x7f81ed0769d0 
 col:19 in libc++.cstddef hidden my_ptrdiff_t 'ptrdiff_t':'long'
  `-TypedefType 0x7f81ed076920 'ptrdiff_t' sugar imported
|-Typedef 0x7f81ed0768b0 'ptrdiff_t'
`-BuiltinType 0x7f81ed03a9f0 'long'


https://reviews.llvm.org/D26267

Files:
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/Preprocessor.h
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/module.modulemap
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stddef.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
  test/Modules/builtin-import.mm

Index: test/Modules/builtin-import.mm
===
--- /dev/null
+++ test/Modules/builtin-import.mm
@@ -0,0 +1,12 @@
+// REQUIRES: system-darwin
+
+// RUN: rm -rf %t
+// RUN: %clang -cc1 -fsyntax-only -nostdinc++ -isysroot %S/Inputs/libc-libcxx/sysroot -isystem %S/Inputs/libc-libcxx/sysroot/usr/include/c++/v1 -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c++ -fmodules-local-submodule-visibility %s
+
+#include 
+#include 
+#include 
+
+typedef ptrdiff_t try1_ptrdiff_t;
+typedef my_ptrdiff_t try2_ptrdiff_t;
+
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
===
--- /dev/null
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
@@ -0,0 +1,6 @@
+#ifndef _SYS_TYPES_UMBRELLA
+#define _SYS_TYPES_UMBRELLA
+
+#include "_ptrdiff_t.h"
+
+#endif
Index: 

[PATCH] D24933: Enable configuration files in clang

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: include/clang/Driver/Driver.h:287
+  const std::string () const { return ConfigFile; }
+  void setConfigFile(StringRef x, unsigned N) {
+ConfigFile = x;

x -> FileName



Comment at: lib/Driver/Driver.cpp:172
+NumConfigArgs = static_cast(NumCfgArgs);
+  }
+

If `NumCfgArgs == 0` it would be nice to warn that the config file is empty?



Comment at: lib/Driver/Driver.cpp:2698
+  for (unsigned I = 0; I < NumConfigArgs; ++I)
+C.getArgs().getArgs()[I]->claim();
+

Why shouldn't we warn about those? Should clang warn and point to the config 
file instead?



Comment at: tools/driver/driver.cpp:318
+
+/// Deduce configuration name if it is encoded in the executable name.
+///

Use  `\brief` here?



Comment at: tools/driver/driver.cpp:333
+  size_t Pos = PName.find("-clang");
+  if (Pos != StringRef::npos) {
+ConfigFile.append(PName.begin(), PName.begin() + Pos);

You can early `return false` here if `Pos == StringRef::npos`


https://reviews.llvm.org/D24933



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

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

@rsmith ping!


https://reviews.llvm.org/D26267



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

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno closed this revision.
bruno added a comment.

Thanks for the review Richard.

Committed 291644




Comment at: lib/Lex/HeaderSearch.cpp:1082
 
+  auto TryEnterImported = [&](void) -> bool {
+if (!ModulesEnabled)

rsmith wrote:
> Maybe add a FIXME here indicating that this is a workaround for the lack of 
> proper modules-aware support for `#import` / `#pragma once`?
Done. 


https://reviews.llvm.org/D26267



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

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Frontend/FrontendActions.cpp:227
+IsBuiltin = true;
+  addHeaderInclude(H.NameAsWritten, Includes, LangOpts, Module->IsExternC,
+   IsBuiltin /*AlwaysInclude*/);

rsmith wrote:
> I don't understand why this would be necessary. If these headers are in fact 
> textual headers, they won't be in the `HK_Normal` or `HK_Private` lists at 
> all (they'll be in the `HK_Private` or `HK_PrivateTextual` lists instead), so 
> your `IsBuiltin = true;` line should be unreachable. (Checking for an 
> absolute path also seems suspicious here.)
> 
> I suspect the problem is instead that `#import` just doesn't work properly 
> with modules (specifically, either with local submodule visibility or with 
> modules that do not `export *`), because it doesn't care whether the previous 
> inclusion is visible or not, and as a result it assumes "I've heard about 
> someone #including this ever" means the same thing as "the contents of this 
> file are visible right now". I know that `#pragma once` has this issue, so 
> I'd expect `#import` to also suffer from it.
In fact, I can achieve the same desired result by changing libcxx modulemap to:

  @@ -161,11 +161,15 @@ module std [system] {
   module cstddef {
 header "cstddef"
 export *
  +  // FIXME: #import on Darwin requires explicit re-export
  +  export Darwin.POSIX.sys.types
   }
   module cstdint {
 header "cstdint"
 export depr.stdint_h
 export *
  +  // FIXME: #import on Darwin requires explicit re-export
  +  export Darwin.C.stdint
   }

But I would like to avoid adding darwin specific stuff there. 

> I don't understand why this would be necessary. If these headers are in fact 
> textual headers,
> they won't be in the HK_Normal or HK_Private lists at all (they'll be in the 
> HK_Private or
> HK_PrivateTextual lists instead), so your IsBuiltin = true; line should be 
> unreachable.

AFAIU, builtins are added twice (1) with the full path with 
`//bin/../lib/clang/4.0.0/include/.h`, as 
`NormalHeader`, which maps to `HK_Normal` and (2) `.h` as 
`TextualHeader`, mapping to `HK_Textual`. This happens in the snippet 
(lib/Lex/ModuleMap.cpp:1882) below:

  if (BuiltinFile) {
// FIXME: Taking the name from the FileEntry is unstable and can give
// different results depending on how we've previously named that file
// in this build.
Module::Header H = { BuiltinFile->getName(), BuiltinFile };
Map.addHeader(ActiveModule, H, Role); <- (1)

// If we have both a builtin and system version of the file, the
// builtin version may want to inject macros into the system header, so
// force the system header to be treated as a textual header in this
// case.
Role = ModuleMap::ModuleHeaderRole(Role | ModuleMap::TextualHeader);
  }

  // Record this header.
  Module::Header H = { RelativePathName.str(), File };
  Map.addHeader(ActiveModule, H, Role); <- (2)
  
As an experiment, I changed `collectModuleHeaderIncludes` to skip 
`addHeaderInclude` for builtin headers with `HK_Normal`, but it caused 
regressions while compiling Darwin SDK even for non local submodule visibility.

> (Checking for an absolute path also seems suspicious here.)

Right, this was done to speedup checking for built-ins, since they are expanded 
to absolute paths. But this isn't necessary for the logic to work and can be 
removed.

> I suspect the problem is instead that #import just doesn't work properly with 
> modules
> (specifically, either with local submodule visibility or with modules that do 
> not export *),
> because it doesn't care whether the previous inclusion is visible or not, and 
> as a result it assumes
> "I've heard about someone #including this ever" means the same thing as "the 
> contents of this file
> are visible right now". I know that #pragma once has this issue, so I'd 
> expect #import to also suffer from it.

Taking a look at this, any ideas on what's the right approach with the 
#import's here? So instead of entering the header, is there a way to make the 
contents for the module matching the built-in header to become 
available/visible at that point?


https://reviews.llvm.org/D26267



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

[PATCH] D27298: [Frontend] Fix an issue where a quoted search path is incorrectly removed as a duplicate header search path

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.

LGTM!


Repository:
  rL LLVM

https://reviews.llvm.org/D27298



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

[PATCH] D26916: [ObjC] Avoid a @try/@finally/@autoreleasepool fixit when parsing an expression

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

Thanks for the explanation, LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D26916



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

[PATCH] D27429: [Chrono][Darwin] On Darwin use CLOCK_UPTIME_RAW instead of CLOCK_MONOTONIC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
bruno added reviewers: mclow.lists, dexonsmith, EricWF.
bruno added a subscriber: cfe-commits.

CLOCK_MONOTONIC is only defined on Darwin on libc versions >= 1133 and its 
behaviour differs from Linux. CLOCK_UPTIME on Darwin actually matches
CLOCK_MONOTONIC on Linux, due to historical coincidence (Linux doesn't match 
POSIX here but Darwin does). Use CLOCK_UPTIME_RAW on Darwin since the _RAW 
version gives nanosecond precision and is lower overhead.


https://reviews.llvm.org/D27429

Files:
  src/chrono.cpp


Index: src/chrono.cpp
===
--- src/chrono.cpp
+++ src/chrono.cpp
@@ -75,8 +75,19 @@
 steady_clock::now() _NOEXCEPT
 {
 struct timespec tp;
+#if defined(__APPLE__) && defined(CLOCK_UPTIME_RAW)
+// CLOCK_MONOTONIC is only defined on Darwin on libc versions >= 1133 and
+// its behaviour differs from Linux.
+// CLOCK_UPTIME on Darwin actually matches CLOCK_MONOTONIC on Linux, due to
+// historical coincidence (Linux doesn't match POSIX here but Darwin does).
+// Use CLOCK_UPTIME_RAW on Darwin since the _RAW version gives nanosecond
+// precision and is lower overhead.
+if (0 != clock_gettime(CLOCK_UPTIME_RAW, ))
+__throw_system_error(errno, "clock_gettime(CLOCK_UPTIME_RAW) failed");
+#else
 if (0 != clock_gettime(CLOCK_MONOTONIC, ))
 __throw_system_error(errno, "clock_gettime(CLOCK_MONOTONIC) failed");
+#endif
 return time_point(seconds(tp.tv_sec) + nanoseconds(tp.tv_nsec));
 }
 


Index: src/chrono.cpp
===
--- src/chrono.cpp
+++ src/chrono.cpp
@@ -75,8 +75,19 @@
 steady_clock::now() _NOEXCEPT
 {
 struct timespec tp;
+#if defined(__APPLE__) && defined(CLOCK_UPTIME_RAW)
+// CLOCK_MONOTONIC is only defined on Darwin on libc versions >= 1133 and
+// its behaviour differs from Linux.
+// CLOCK_UPTIME on Darwin actually matches CLOCK_MONOTONIC on Linux, due to
+// historical coincidence (Linux doesn't match POSIX here but Darwin does).
+// Use CLOCK_UPTIME_RAW on Darwin since the _RAW version gives nanosecond
+// precision and is lower overhead.
+if (0 != clock_gettime(CLOCK_UPTIME_RAW, ))
+__throw_system_error(errno, "clock_gettime(CLOCK_UPTIME_RAW) failed");
+#else
 if (0 != clock_gettime(CLOCK_MONOTONIC, ))
 __throw_system_error(errno, "clock_gettime(CLOCK_MONOTONIC) failed");
+#endif
 return time_point(seconds(tp.tv_sec) + nanoseconds(tp.tv_nsec));
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16533: Bug 20796 - GCC's -Wstrict-prototypes warning not implemented in Clang

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Alex, thanks for following up with this!




Comment at: lib/Sema/SemaDecl.cpp:11819
+  //   Warn if K function is defined without previous declaration
+  //   declaration. This warning is issued only if the difinition itself
+  //   does not provide a prototype. Only K definitions do not

There's an extra 'declaration' here



Comment at: lib/Sema/SemaType.cpp:4189
+  if (D.getFunctionDefinitionKind() == FDK_Declaration &&
+  FTI.NumParams == 0 && !LangOpts.CPlusPlus) {
+S.Diag(DeclType.Loc, diag::warn_strict_prototypes)

No need for the curly braces here!


Repository:
  rL LLVM

https://reviews.llvm.org/D16533



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

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

> The right thing to do would be to track the set of headers whose #import / 
> #include-with-#pragma-once is
>  visible, and only skip inclusions if there is a *visible* #import / 
> #include-with-#pragma-once of that header.

This makes sense, but the situation is a bit more weird with the builtins: at 
the time clang skips entering 
/Users/bruno/Dev/srcs/llvm/debug/bin/../lib/clang/4.0.0/include/stddef.h 
because there's already a #import associated with  it, clang tries to get the 
module, in this case it finds "libc++.stddef", but `makeModuleVisible` does not 
export the contents
of /Users/bruno/Dev/srcs/llvm/debug/bin/../lib/clang/4.0.0/include/stddef.h:

  // If we don't need to enter the file, stop now.
  if (!ShouldEnter) {
// If this is a module import, make it visible if needed.
if (auto *M = SuggestedModule.getModule()) {
  makeModuleVisible(M, HashLoc);
  ...
}
return;
  }
  
  (lldb) p M->dump()
  module stddef   [system] {
header 
"/Users/bruno/Dev/srcs/llvm/debug/bin/../lib/clang/4.0.0/include/stddef.h"
textual header "stddef.h"
export *
  }

Is this what it's supposed to happen with a (textual) builtin header? It seems 
to me that since the first #import was done after collecting the headers to 
build the module, the content should already been present/available in the AST 
for the module.


https://reviews.llvm.org/D26267



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

[PATCH] D27429: [Chrono][Darwin] On Darwin use CLOCK_UPTIME_RAW instead of CLOCK_MONOTONIC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

@EricWF ok to commit?


https://reviews.llvm.org/D27429



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

[PATCH] D27429: [Chrono][Darwin] On Darwin use CLOCK_UPTIME_RAW instead of CLOCK_MONOTONIC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 83158.
bruno added a comment.

Thanks for the reviews. @howard.hinnant thanks for the great explanation & 
examples.

Attached a new version of the patch, addressing all suggestions. The logic 
became a bit simpler after Saleem's r290804, which already moved the 
CLOCK_MONOTONIC to be the last condition.

On top of that, I also conditionalized the presence of `clock_gettime`, which 
isn't available before 10.12 on macosx and specific versions on other apple 
platforms. Check that by looking at the deployment target.


https://reviews.llvm.org/D27429

Files:
  src/chrono.cpp


Index: src/chrono.cpp
===
--- src/chrono.cpp
+++ src/chrono.cpp
@@ -12,6 +12,28 @@
 #include "system_error"  // __throw_system_error
 #include // clock_gettime, CLOCK_MONOTONIC and CLOCK_REALTIME
 
+#if (__APPLE__)
+#if defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)
+#if __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 101200
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif
+#elif defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__)
+#if __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ >= 10
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif
+#elif defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__)
+#if __ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__ >= 10
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif
+#elif defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__)
+#if __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ >= 3
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif
+#endif // __ENVIRONMENT_.*_VERSION_MIN_REQUIRED__
+#else
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif // __APPLE__
+
 #if defined(_LIBCPP_WIN32API)
 #define WIN32_LEAN_AND_MEAN
 #define VC_EXTRA_LEAN
@@ -70,16 +92,16 @@
static_cast<__int64>(ft.dwLowDateTime)};
   return time_point(duration_cast(d - nt_to_unix_epoch));
 #else
-#ifdef CLOCK_REALTIME
+#if defined(_LIBCXX_USE_CLOCK_GETTIME) && defined(CLOCK_REALTIME)
 struct timespec tp;
 if (0 != clock_gettime(CLOCK_REALTIME, ))
 __throw_system_error(errno, "clock_gettime(CLOCK_REALTIME) failed");
 return time_point(seconds(tp.tv_sec) + microseconds(tp.tv_nsec / 1000));
-#else  // !CLOCK_REALTIME
+#else
 timeval tv;
 gettimeofday(, 0);
 return time_point(seconds(tv.tv_sec) + microseconds(tv.tv_usec));
-#endif  // CLOCK_REALTIME
+#endif // _LIBCXX_USE_CLOCK_GETTIME && CLOCK_REALTIME
 #endif
 }
 
@@ -106,6 +128,18 @@
 
 #if defined(__APPLE__)
 
+// Darwin libc versions >= 1133 provide ns precision via CLOCK_UPTIME_RAW
+#if defined(_LIBCXX_USE_CLOCK_GETTIME) && defined(CLOCK_UPTIME_RAW)
+steady_clock::time_point
+steady_clock::now() _NOEXCEPT
+{
+struct timespec tp;
+if (0 != clock_gettime(CLOCK_UPTIME_RAW, ))
+__throw_system_error(errno, "clock_gettime(CLOCK_UPTIME_RAW) failed");
+return time_point(seconds(tp.tv_sec) + nanoseconds(tp.tv_nsec));
+}
+
+#else
 //   mach_absolute_time() * MachInfo.numer / MachInfo.denom is the number of
 //   nanoseconds since the computer booted up.  MachInfo.numer and 
MachInfo.denom
 //   are run time constants supplied by the OS.  This clock has no relationship
@@ -157,6 +191,7 @@
 static FP fp = init_steady_clock();
 return time_point(duration(fp()));
 }
+#endif // defined(_LIBCXX_USE_CLOCK_GETTIME) && defined(CLOCK_UPTIME_RAW)
 
 #elif defined(_LIBCPP_WIN32API)
 
@@ -175,6 +210,13 @@
 
 #elif defined(CLOCK_MONOTONIC)
 
+// On Apple platforms only CLOCK_UPTIME_RAW or mach_absolute_time are able to
+// time functions in the nanosecond range. Thus, they are the only acceptable
+// implementations of steady_clock.
+#ifdef __APPLE__
+#error "Never use CLOCK_MONOTONIC for steady_clock::now on Apple platforms"
+#endif
+
 steady_clock::time_point
 steady_clock::now() _NOEXCEPT
 {


Index: src/chrono.cpp
===
--- src/chrono.cpp
+++ src/chrono.cpp
@@ -12,6 +12,28 @@
 #include "system_error"  // __throw_system_error
 #include // clock_gettime, CLOCK_MONOTONIC and CLOCK_REALTIME
 
+#if (__APPLE__)
+#if defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)
+#if __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 101200
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif
+#elif defined(__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__)
+#if __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ >= 10
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif
+#elif defined(__ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__)
+#if __ENVIRONMENT_TV_OS_VERSION_MIN_REQUIRED__ >= 10
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif
+#elif defined(__ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__)
+#if __ENVIRONMENT_WATCH_OS_VERSION_MIN_REQUIRED__ >= 3
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif
+#endif // __ENVIRONMENT_.*_VERSION_MIN_REQUIRED__
+#else
+#define _LIBCXX_USE_CLOCK_GETTIME
+#endif // __APPLE__
+
 #if defined(_LIBCPP_WIN32API)
 #define WIN32_LEAN_AND_MEAN
 #define VC_EXTRA_LEAN
@@ -70,16 +92,16 

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping!


https://reviews.llvm.org/D26267



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

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi,

Thanks for working on this.




Comment at: include/clang/Basic/DiagnosticLexKinds.td:647
+  "top-level module '%0' in private module map, expected a submodule of '%1'">,
+  InGroup>;
 

It would be nice if we could also emit a FixIt to tell the user how should the 
modulemap be fixed.



Comment at: lib/Lex/HeaderSearch.cpp:208
+  // FooPrivate.framework.
+  if (!Module && SearchName.consume_back("Private")) {
+Module = lookupModule(ModuleName, SearchName);

Remove the curly braces here



Comment at: lib/Lex/HeaderSearch.cpp:211
+  }
+
+  return Module;

Remove this newline



Comment at: lib/Lex/HeaderSearch.cpp:216
+Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName) 
{
+
+  Module *Module = nullptr;

Remove this newline



Comment at: test/Modules/implicit-private-with-different-name.m:9
+
+// 
expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{expected
 a submodule}}
+

Can you check for the entire warning message?


https://reviews.llvm.org/D27852



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

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a reviewer: bruno.
bruno added inline comments.



Comment at: test/Modules/implicit-private-with-different-name.m:13
+// 
expected-warning@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{top-level
 module 'APrivate' in private module map, expected a submodule of 'A'}}
+// 
expected-note@Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap:1{{make
 'APrivate' a submodule of 'A' to ensure it can be found by name}}
+// CHECK: 
fix-it:"{{.*}}/Inputs/implicit-private-with-different-name/A.framework/Modules/module.private.modulemap":{1:18-1:26}:"A.Private"

I think we can enhance the usability a bit here, how about:

warning: top-level module 'APrivate' in private module map, expected a 
submodule of 'A'
note: make 'APrivate' a submodule of 'A' to ensure it can be found by name
  framework module APrivate {
   ^
A.Private




https://reviews.llvm.org/D27852



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

[PATCH] D27604: [Driver] Add compiler option to generate a reproducer

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping!


https://reviews.llvm.org/D27604



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

[PATCH] D27852: [modules] Handle modules with nonstandard names in module.private.modulemaps

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.

Right, I missed it, LGTM


https://reviews.llvm.org/D27852



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

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 82067.
bruno marked 3 inline comments as done.
bruno added a comment.

Uploaded new patch with another approach to fix the issue.


https://reviews.llvm.org/D26267

Files:
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/ModuleMap.h
  lib/Lex/HeaderSearch.cpp
  lib/Lex/ModuleMap.cpp
  lib/Lex/PPDirectives.cpp
  test/Modules/Inputs/import-textual/M/A/A.h
  test/Modules/Inputs/import-textual/M/B/B.h
  test/Modules/Inputs/import-textual/M/module.modulemap
  test/Modules/Inputs/import-textual/M/someheader.h
  test/Modules/Inputs/import-textual/M2/A/A.h
  test/Modules/Inputs/import-textual/M2/B/B.h
  test/Modules/Inputs/import-textual/M2/module.modulemap
  test/Modules/Inputs/import-textual/M2/someheader.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/cstddef
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/math.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/module.modulemap
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/stddef.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h
  test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
  test/Modules/builtin-import.mm
  test/Modules/import-textual-noguard.mm
  test/Modules/import-textual.mm

Index: test/Modules/import-textual.mm
===
--- /dev/null
+++ test/Modules/import-textual.mm
@@ -0,0 +1,10 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -fmodules -fimplicit-module-maps -I%S/Inputs/import-textual/M -fmodules-cache-path=%t -x objective-c++ -fmodules-local-submodule-visibility %s -verify
+
+// expected-no-diagnostics
+
+#include "A/A.h"
+#include "B/B.h"
+
+typedef aint xxx;
+typedef bint yyy;
Index: test/Modules/import-textual-noguard.mm
===
--- /dev/null
+++ test/Modules/import-textual-noguard.mm
@@ -0,0 +1,8 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -fmodules -fimplicit-module-maps -I%S/Inputs/import-textual/M2 -fmodules-cache-path=%t -x objective-c++ -fmodules-local-submodule-visibility %s -verify
+
+#include "A/A.h" // expected-error {{could not build module 'M'}}
+#include "B/B.h"
+
+typedef aint xxx;
+typedef bint yyy;
Index: test/Modules/builtin-import.mm
===
--- /dev/null
+++ test/Modules/builtin-import.mm
@@ -0,0 +1,12 @@
+// REQUIRES: system-darwin
+
+// RUN: rm -rf %t
+// RUN: %clang -cc1 -fsyntax-only -nostdinc++ -isysroot %S/Inputs/libc-libcxx/sysroot -isystem %S/Inputs/libc-libcxx/sysroot/usr/include/c++/v1 -std=c++11 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c++ -fmodules-local-submodule-visibility %s
+
+#include 
+#include 
+#include 
+
+typedef ptrdiff_t try1_ptrdiff_t;
+typedef my_ptrdiff_t try2_ptrdiff_t;
+
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
===
--- /dev/null
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_types.h
@@ -0,0 +1,6 @@
+#ifndef _SYS_TYPES_UMBRELLA
+#define _SYS_TYPES_UMBRELLA
+
+#include "_ptrdiff_t.h"
+
+#endif
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h
===
--- /dev/null
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/sys/_types/_ptrdiff_t.h
@@ -0,0 +1,4 @@
+#ifndef _PTRDIFF_T
+#define _PTRDIFF_T
+typedef int * ptrdiff_t;
+#endif /* _PTRDIFF_T */
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
===
--- test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/stddef.h
@@ -1 +1,6 @@
-// stddef.h
+#ifndef __STDDEF_H__
+#define __STDDEF_H__
+
+#include "sys/_types/_ptrdiff_t.h"
+
+#endif
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
===
--- test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
+++ test/Modules/Inputs/libc-libcxx/sysroot/usr/include/module.modulemap
@@ -5,4 +5,12 @@
   module stdint { header "stdint.h" export * }
   module stdio { header "stdio.h" export * }
   module util { header "util.h" export * }
+  module POSIX {
+module sys {
+  module types {
+umbrella header "sys/_types/_types.h"
+export *
+  }
+}
+  }
 }
Index: test/Modules/Inputs/libc-libcxx/sysroot/usr/include/c++/v1/type_traits
===
--- /dev/null
+++ 

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked 3 inline comments as done.
bruno added a comment.



> Do we need to perfectly preserve the functionality of `#pragma once` / 
> `#import`? That is, would it be acceptable to 
>  you if we spuriously enter files that have been imported in that way, while 
> building a module?

Unfortunately we can find ObjC headers that don't have include guards and only 
rely on #import's. Relaxing this condition could lead to more redefinition 
errors with current code base than we desire.

> Another possibility -- albeit slightly hacky -- would be to invent a 
> controlling macro if the header in question turns out to not have one (say, 
> form an identifier from the path by which the file was included and use that 
> as the macro name) and is #imported / uses #pragma once. Or we could try 
> simply rejecting textual headers that are #import'd / #pragma once'd and have 
> no controlling macro in a modules build.

Right, it's not that hacky - it actually performs the job of actually guarding 
as we do with includes, but with more context. However, it's not clear to me 
how to determine the suggested 'identifier from the path'.

I have a third suggestion: seems like we only care here about two cases (1) 
modular import's from the same header - this will only happens with builtins 
AFAIU and (2) textual headers that must be imported multiple times for 
different modules. We can solve (1) by only using the information on HFI and 
fix (2) by only re-trying to enter the #imports where we find a controlling 
macro for the header and that macro isn't reachable at that point. I wrote a 
new patch that implements this, let me know what are your thoughts. Note that I 
intentionally left non-modular includes out because AFAIK they should be part 
of the first module they are founded, so it doesn't make any sense to re-enter 
them anyway.




Comment at: include/clang/Lex/HeaderSearch.h:98-101
+  /// List of modules that import from this header. Since the number of modules
+  /// using the same FileEntry is usually small (2 or 3 at most, often related
+  /// to builtins) this should be enough. Choose a more appropriate data
+  /// structure in case the requirement changes.

rsmith wrote:
> I don't think this is true in general. For a textual header, there could be 
> hundreds or thousands of loaded modules that use it. If all header files in a 
> project start with
> 
>   #include "local_config.h"
> 
> ... which is configured to be a textual header for whatever reason and 
> contains a `#pragma once`, we would get into that situation for at least that 
> one header. While a vector might be OK (as the number of entries should at 
> least grow only linearly with the size of the entire codebase), 
> quadratic-time algorithms over it probably won't be. Perhaps a sorted vector 
> instead?
Right, I wasn't considering adding the textual headers at this point, this 
should have been filtered out by the ASTReader HFI merging. But handling the 
textual headers is necessary indeed. Hopefully my next patch fix it.



Comment at: lib/Lex/HeaderSearch.cpp:1116-1119
+//  - if there not associated module or the module already imports
+//from this header.
+//  - if any module associated with this header is visible.
+if (FileInfo.isImport) {

rsmith wrote:
> I think this would be clearer as:
> 
> > [...], ignore it, unless doing so will lead to us importing a module that 
> > contains the file but didn't actually include it (because we're still 
> > building the corresponding module), and we've not already made the file 
> > visible by importing some other module.
> 
> ... but I don't think that's actually right. If `M` is null (if the file is 
> only ever included as a textual header), we still need to check whether some 
> visible module has actually made it visible, and we should enter it if not.
Looks like what I actually wanted was to check if modules were supported at 
this point with `LangOpts.Modules`



Comment at: lib/Serialization/ASTReader.cpp:1691-1692
 HFI.isModuleHeader |= !(HeaderRole & ModuleMap::TextualHeader);
+if (HFI.isImport)
+  HFI.addModulesUsingHdr(Mod);
   }

rsmith wrote:
> Doing this here will do the wrong thing while building a module with local 
> submodule visibility enabled -- we need to know whether the visible module 
> set contains a module that entered the header, even for modules that we've 
> never serialized.
> 
> Also, this will not populate the vector for the cases where the header was 
> not listed in the module map, but was simply a non-moduler header that was 
> textually entered while building a module.
Ok


https://reviews.llvm.org/D26267



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

[PATCH] D27832: Add -plugin-opt=sample-profile for thinLTO build.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Driver/Tools.cpp:2211
+  if (Arg *A = Args.getLastArg(options::OPT_fprofile_sample_use_EQ)) {
+StringRef fname = A->getValue();
+if (!llvm::sys::fs::exists(fname))

"fname" -> "FName", "FileName", etc, or any name that starts with a capital 
letter


https://reviews.llvm.org/D27832



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

[PATCH] D30810: Preserve vec3 type.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

> I would like to see this patch committed. I see clear evidence of it 
> improving existing GPU targets in the master repo as well as outside of the 
> main tree implementations. Bruno, do you have any more concerns?

I believe the argument lacks numbers (or at least you have them but didn't 
mention). I didn't hear about performance results, or validation that this was 
actually tested for correctness. Small test cases prove a point, but can't be 
considered general.

OTOH, it seems like this is exactly why you want the flag, to hide any 
potential issues and play with it. I'm not opposed to adding the flag if 
there's a commitment to actually get the result and change the default to 
whatever seems to be better, does that seems reasonable?


https://reviews.llvm.org/D30810



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

[PATCH] D31134: [Serialization] Serialize DependentSizedExtVectorType

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM Alex, thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D31134



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

[PATCH] D25866: [Sema] Support implicit scalar to vector conversions

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: test/Sema/vector-gcc-compat.c:61
+  //match.
+  v2i64_r = v2i64_a == 1; // expected-warning {{incompatible vector types 
assigning to 'v2i64' (vector of 2 'long long' values) from 'long 
__attribute__((ext_vector_type(2)))' (vector of 2 'long' values)}}
+  v2i64_r = v2i64_a != 1; // expected-warning {{incompatible vector types 
assigning to 'v2i64' (vector of 2 'long long' values) from 'long 
__attribute__((ext_vector_type(2)))' (vector of 2 'long' values)}}

sdardis wrote:
> bruno wrote:
> > Can you double check where 'long __attribute__((ext_vector_type(2)))' comes 
> > from?
> > 
> > We have regressed in the past year in the way ext-vector interacts with 
> > non-ext-vectors, and I don't wanna make it worse until we actually have 
> > time to fix that; there's a lot of code out there relying on bitcasts 
> > between ext-vectors and non-ext-vectors to bridge between intrinsics 
> > headers and ext-vector code.
> Sema::CheckVectorCompareOperands calls Sema::GetSignedVectorType which only 
> returns ext-vector types. I presume that is now incorrect if we're producing 
> vectors from literal scalars in the non OpenCL case.
Nice catch, can you please submit these specific changes as a separated patch 
and mark it as a prerequisite of this one?


https://reviews.llvm.org/D25866



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

[PATCH] D29923: PPCallbacks::MacroUndefined, change signature and add test.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D29923



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

[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping!


https://reviews.llvm.org/D31269



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

[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.

When modules come from module map files explicitly specified by
-fmodule-map-file= arguments, allow those to override/shadow modules
with the same name that are found implicitly by header search. If such a
module is looked up by name (e.g. @import), we will always find the one
from -fmodule-map-file. If we try to use a shadowed module by including
one of its headers report an error.

This enables developers to force use of a specific copy of their module
to be used if there are multiple copies that would otherwise be visible,
for example if they develop modules that are installed in the default
search paths.

We're using this internally for a couple years now, it would be nice to have it 
upstreamed.

Patch originally by Ben Langmuir,
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20151116/143425.html

Based on cfe-dev discussion:
http://lists.llvm.org/pipermail/cfe-dev/2015-November/046164.html


https://reviews.llvm.org/D31269

Files:
  include/clang/Basic/DiagnosticCommonKinds.td
  include/clang/Basic/Module.h
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/ModuleMap.h
  lib/Basic/Module.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Lex/ModuleMap.cpp
  lib/Lex/PPDirectives.cpp
  test/Modules/Inputs/shadow/A1/A.h
  test/Modules/Inputs/shadow/A1/module.modulemap
  test/Modules/Inputs/shadow/A2/A.h
  test/Modules/Inputs/shadow/A2/module.modulemap
  test/Modules/shadow.m

Index: test/Modules/shadow.m
===
--- /dev/null
+++ test/Modules/shadow.m
@@ -0,0 +1,21 @@
+// RUN: rm -rf %t
+// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/shadow/A1 -I %S/Inputs/shadow/A2 %s -fsyntax-only 2>&1 | FileCheck %s -check-prefix=REDEFINITION
+// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fmodule-map-file=%S/Inputs/shadow/A1/module.modulemap -fmodule-map-file=%S/Inputs/shadow/A2/module.modulemap %s -fsyntax-only 2>&1 | FileCheck %s -check-prefix=REDEFINITION
+// REDEFINITION: error: redefinition of module 'A'
+// REDEFINITION: note: previously defined
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fmodule-map-file=%S/Inputs/shadow/A1/module.modulemap -I %S/Inputs/shadow %s -verify
+
+@import A1;
+@import A2;
+@import A;
+
+#import "A2/A.h" // expected-note {{implicitly imported}}
+// expected-error@A2/module.modulemap:1 {{import of shadowed module 'A'}}
+// expected-note@A1/module.modulemap:1 {{previous definition}}
+
+#if defined(A2_A_h)
+#error got the wrong definition of module A
+#elif !defined(A1_A_h)
+#error missing definition from A1
+#endif
Index: test/Modules/Inputs/shadow/A2/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/shadow/A2/module.modulemap
@@ -0,0 +1,5 @@
+module A {
+  header "A.h"
+}
+
+module A2 {}
Index: test/Modules/Inputs/shadow/A2/A.h
===
--- /dev/null
+++ test/Modules/Inputs/shadow/A2/A.h
@@ -0,0 +1 @@
+#define A2_A_h
Index: test/Modules/Inputs/shadow/A1/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/shadow/A1/module.modulemap
@@ -0,0 +1,5 @@
+module A {
+  header "A.h"
+}
+
+module A1 {}
Index: test/Modules/Inputs/shadow/A1/A.h
===
--- /dev/null
+++ test/Modules/Inputs/shadow/A1/A.h
@@ -0,0 +1 @@
+#define A1_A_h
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1869,13 +1869,17 @@
 if (!SuggestedModule.getModule()->isAvailable()) {
   Module::Requirement Requirement;
   Module::UnresolvedHeaderDirective MissingHeader;
+  Module *ShadowingModule = nullptr;
   Module *M = SuggestedModule.getModule();
   // Identify the cause.
   (void)M->isAvailable(getLangOpts(), getTargetInfo(), Requirement,
-   MissingHeader);
+   MissingHeader, ShadowingModule);
   if (MissingHeader.FileNameLoc.isValid()) {
 Diag(MissingHeader.FileNameLoc, diag::err_module_header_missing)
 << MissingHeader.IsUmbrella << MissingHeader.FileName;
+  } else if (ShadowingModule) {
+Diag(M->DefinitionLoc, diag::err_module_shadowed) << M->Name;
+Diag(ShadowingModule->DefinitionLoc, diag::note_previous_definition);
   } else {
 Diag(M->DefinitionLoc, diag::err_module_unavailable)
 << M->getFullModuleName() << Requirement.second << Requirement.first;
Index: lib/Lex/ModuleMap.cpp
===
--- lib/Lex/ModuleMap.cpp
+++ lib/Lex/ModuleMap.cpp
@@ -29,6 +29,7 @@
 #include 

[PATCH] D31378: [PCH] Attach instance's dependency collectors to PCH external AST sources.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.

Thanks for working on this! LGTM too


https://reviews.llvm.org/D31378



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

[PATCH] D31241: [Modules][PCH] Serialize #pragma pack

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

Thanks Alex. LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D31241



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

[PATCH] D30810: Preserve vec3 type.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



> As you can see, the type legalizer handle vec3 load/store properly. It does 
> not write 4th element. The vec3 load/store generates more instructions but it 
> has correct behavior. I am not 100% sure the vec3 --> vec4 load/store is 
> correct or not because no one has complained about it.  But if the vec3 --> 
> vec4 load/store is correct, llvm's type legalizer or somewhere on llvm's 
> codegen could follow the approach too to generate optimal code.

Thanks for the nice investigation/explanation on amdgcn.

> As a result, I think it would be good for clang to have both of features and 
> I would like to stick to the option "-fpresereve-vec3' to change the behavior 
> easily.

The motivation doesn't seem solid to me, who else is going to benefit from this 
flag? You also didn't explain why doing this transformation yourself (looking 
through the shuffle) on your downstream pass isn't enough for you. We generally 
try to avoid adding flags if not for a good reason.


https://reviews.llvm.org/D30810



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

[PATCH] D30810: Preserve vec3 type.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

> Thanks for your comment. We have a pass to undo the vec4 to vec3. I wondered 
> why clang generates the vec4 for vec3 load/store. As you can see the comment 
> on clang's code, they are generated for better performance. I had a questions 
> at this point. clang should consider vector load/store aligned by 4 
> regardless of target???

I believe the assumption is more practical: most part of upstream llvm targets 
only support vectors with even sized number of lanes. And in those cases you 
would have to expand to a 4x vector and leave the 4th element as undef anyway, 
so it was done in the front-end to get rid of it right away. Probably GPU 
targets do some special tricks here during legalization.

> llvm's codegen could handle vec3 according to targets' vector load/store with 
> their alignment. I agree the flag looks odd but I was concerned some llvm's 
> targets depend on the vec4 so I suggested to add the flag. If I missed 
> something, please let me know.

My guess here is that targets that do care are looking through the vector 
shuffle and customizing to whatever seems the best to them. If you wanna change 
the default behavior you need some data to show that your model solves a real 
issue and actually brings benefits; do you have any real examples on where that 
happens, or why GPU targets haven't yet tried to change this? Maybe other 
custom front-ends based on clang do? Finding the historical reason (if any) 
should be a good start point.


https://reviews.llvm.org/D30810



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

[PATCH] D25866: [Sema] Support implicit scalar to vector conversions

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:10024
+  // FIXME: The check for C++ here is for GCC compatibility. GCC rejects the
+  //usage of logical operators with vectors in C. This check could be
+  //notionally dropped.

Please mention in the comment the list of logical ops that this applies to: "!, 
&&, ||"



Comment at: test/Sema/vector-gcc-compat.c:101
+
+  v2i64_r = v2i64_a && 1; // expected-error {{invalid vector operand to binary 
expression ('v2i64' (vector of 2 'long long' values))}}
+  v2i64_r = v2i64_a || 1; // expected-error {{invalid vector operand to binary 
expression ('v2i64' (vector of 2 'long long' values))}}

Is this because of && and others only working in C++? If so, we need a better 
error message here, along the lines of "logical expression with vector only 
support in C++". If later on we decide to support it in non-C++, we then get 
rid of the warning.





https://reviews.llvm.org/D25866



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

[PATCH] D31667: [Sema] Extend GetSignedVectorType to deal with non ExtVector types

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Great! Thanks for improving this.




Comment at: include/clang/Sema/Sema.h:9287
bool AllowBothBool, bool AllowBoolConversion);
-  QualType GetSignedVectorType(QualType V);
+  QualType GetSignedVectorType(QualType V, bool WantExtVectorType);
   QualType CheckVectorCompareOperands(ExprResult , ExprResult ,

Can you change `WantExtVector`  to `UseExtVector`?



Comment at: lib/Sema/SemaExpr.cpp:9719
+// gets picked over long long.
+QualType Sema::GetSignedVectorType(QualType V, bool WantExtVector) {
   const VectorType *VTy = V->getAs();

Do we really need to pass `WantExtVector` here? Why relying on `V` being 
ext_vector or generic isn't enough to define `WantExtVector`? 



Comment at: lib/Sema/SemaExpr.cpp:9803
   // Return a signed type for the vector.
-  return GetSignedVectorType(vType);
+  return GetSignedVectorType(vType, isExtVectorType);
 }

So in `getLangOpts().OpenCL` mode we want to get a ext_vec even if `vType` 
isn't ext_vec? Would that ever happen?



Comment at: lib/Sema/SemaExpr.cpp:9821
+
+  return GetSignedVectorType(LHS.get()->getType(), isExtVectorType);
 }

Same question applies here



Comment at: lib/Sema/SemaExpr.cpp:11760
   // Vector logical not returns the signed variant of the operand type.
-  resultType = GetSignedVectorType(resultType);
+  resultType = GetSignedVectorType(resultType, true);
   break;

Use the idiom `...(resultType, true/*WantExtVector*/);`



Comment at: test/Sema/vector-ops.c:16
   // Comparison operators
-  v2ua = (v2ua==v2sa); // expected-warning{{incompatible vector types 
assigning to 'v2u' (vector of 2 'unsigned int' values) from 'int 
__attribute__((ext_vector_type(2)))' (vector of 2 'int' values)}}
+  v2ua = (v2ua==v2sa); // expected-warning{{incompatible vector types 
assigning to 'v2u' (vector of 2 'unsigned int' values) from 
'__attribute__((__vector_size__(2 * sizeof(int int' (vector of 2 'int' 
values)}}
   v2sa = (v2ua==v2sa);

Can you also add a test for the `CheckVectorLogicalOperands` case?


https://reviews.llvm.org/D31667



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

[PATCH] D25866: [Sema] Support implicit scalar to vector conversions

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:8032
+
+  return InvalidOperands(Loc, LHS, RHS);
+}

Double checking here: are there tests for the `InvalidOperands` case above?



Comment at: lib/Sema/SemaExpr.cpp:8163
+/// type without causing truncation of Scalar.
+static bool tryGCCVectorConvertAndSpalt(Sema , ExprResult *Scalar,
+ExprResult *Vector) {

`Spalt` -> `Splat`



Comment at: lib/Sema/SemaExpr.cpp:8188
+  //type and then perform the rest of the checks here. GCC as of
+  //pre-release 7.0 does not accept this though.
+  if (VectorEltTy->isIntegralType(S.Context) &&

Is this something that GCC is going to support at some point? Regardless of 
GCC, do we want this behavior?



Comment at: lib/Sema/SemaExpr.cpp:10019
   isa(vType->getAs()) || getLangOpts().OpenCL;
+  if ((!getLangOpts().CPlusPlus && !getLangOpts().OpenCL) && !isExtVectorType)
+return InvalidVectorOperands(Loc, LHS, RHS);

Why `!getLangOpts().CPlusPlus` is a requirement here?



Comment at: test/Sema/vector-g++-compat.cpp:1
+// RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -std=c++11
+

Can you rename this file to `vector-gcc-compat.cpp` instead?



Comment at: test/Sema/vector-g++-compat.cpp:155
+   unsigned long long d) { // expected-warning {{'long 
long' is incompatible with C++98}}
+
+  v4f32 v4f32_a = {0.4f, 0.4f, 0.4f, 0.4f};

Remove all these extra new lines after the function signature here, in the 
functions below and in the other added test file for c++.


https://reviews.llvm.org/D25866



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

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.

Allow ODR for ObjC/C in the sense that we won't keep more that one definition 
around (merge them). However, ensure the decl pass the structural compatibility 
check in C11 6.2.7/1, for that, reuse the structural equivalence checks used by 
the ASTImporter.

Few other considerations:

- Create error diagnostics for tag types mismatches and thread them into the 
structural equivalence checks.
- Note that by doing this we only support redefinition between types that are 
considered "compatible types" by C11.

This is mixed approach of the suggestions discussed in 
http://lists.llvm.org/pipermail/cfe-dev/2017-March/053257.html


https://reviews.llvm.org/D31778

Files:
  include/clang/AST/ASTStructuralEquivalence.h
  include/clang/Basic/DiagnosticASTKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/AST/ASTStructuralEquivalence.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseExpr.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/Modules/Inputs/F.framework/Headers/F.h
  test/Modules/Inputs/F.framework/Modules/module.modulemap
  test/Modules/Inputs/F.framework/Modules/module.private.modulemap
  test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
  test/Modules/elaborated-type-specifier-from-hidden-module.m
  test/Modules/redefinition-c-tagtypes.m

Index: test/Modules/redefinition-c-tagtypes.m
===
--- /dev/null
+++ test/Modules/redefinition-c-tagtypes.m
@@ -0,0 +1,48 @@
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -fsyntax-only %s -fmodules -fmodules-cache-path=%t.cache \
+// RUN:   -fimplicit-module-maps -F%S/Inputs -verify
+// RUN: %clang_cc1 -fsyntax-only %s -fmodules -fmodules-cache-path=%t.cache \
+// RUN:   -fimplicit-module-maps -F%S/Inputs -DCHANGE_TAGS -verify
+#include "F/F.h"
+
+#ifndef CHANGE_TAGS
+// expected-no-diagnostics
+#endif
+
+struct NS {
+  int a;
+#ifndef CHANGE_TAGS
+  int b;
+#else
+  int c; // expected-note {{field has name 'c' here}}
+  // expected-error@redefinition-c-tagtypes.m:12 {{type 'struct NS' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:3 {{field has name 'b' here}}
+#endif
+};
+
+enum NSE {
+  FST = 22,
+#ifndef CHANGE_TAGS
+  SND = 43,
+#else
+  SND = 44, // expected-note {{enumerator 'SND' with value 44 here}}
+  // expected-error@redefinition-c-tagtypes.m:23 {{type 'enum NSE' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:8 {{enumerator 'SND' with value 43 here}}
+#endif
+  TRD = 55
+};
+
+#define NS_ENUM(_type, _name) \
+  enum _name : _type _name;   \
+  enum _name : _type
+
+typedef NS_ENUM(int, NSMyEnum) {
+  MinX = 11,
+#ifndef CHANGE_TAGS
+  MinXOther = MinX,
+#else
+  MinXOther = TRD, // expected-note {{enumerator 'MinXOther' with value 55 here}}
+  // expected-error@redefinition-c-tagtypes.m:39 {{type 'enum NSMyEnum' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:18 {{enumerator 'MinXOther' with value 11 here}}
+#endif
+};
Index: test/Modules/elaborated-type-specifier-from-hidden-module.m
===
--- test/Modules/elaborated-type-specifier-from-hidden-module.m
+++ test/Modules/elaborated-type-specifier-from-hidden-module.m
@@ -4,12 +4,11 @@
 @import ElaboratedTypeStructs.Empty; // The structs are now hidden.
 struct S1 *x;
 struct S2 *y;
-// FIXME: compatible definition should not be an error.
-struct S2 { int x; }; // expected-error {{redefinition}}
+struct S2 { int x; };
 struct S3 *z;
 // Incompatible definition.
-struct S3 { float y; }; // expected-error {{redefinition}}
-// expected-note@elaborated-type-structs.h:* 2 {{previous definition is here}}
+struct S3 { float y; }; // expected-error {{has incompatible definitions}} // expected-note {{field has name}}
+// expected-note@Inputs/elaborated-type-structs.h:3 {{field has name}}
 
 @import ElaboratedTypeStructs.Structs;
 
Index: test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
@@ -0,0 +1,19 @@
+struct NS {
+  int a;
+  int b;
+};
+
+enum NSE {
+  FST = 22,
+  SND = 43,
+  TRD = 55
+};
+
+#define NS_ENUM(_type, _name) \
+  enum _name : _type _name;   \
+  enum _name : _type
+
+typedef NS_ENUM(int, NSMyEnum) {
+  MinX = 11,
+  MinXOther = MinX,
+};
Index: test/Modules/Inputs/F.framework/Modules/module.private.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/Modules/module.private.modulemap
@@ -0,0 +1,7 @@
+module F.Private [system] {
+  explicit module NS {
+  header "NS.h"
+  export *
+  }
+  export *
+}
Index: test/Modules/Inputs/F.framework/Modules/module.modulemap

[PATCH] D31781: [Modules] Allow local submodule visibility without c++

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.

Removing this restriction will make it handy to explore local submodule 
visibility without c++ being turned on. We're currently testing it out with 
C/ObjC.


https://reviews.llvm.org/D31781

Files:
  lib/Frontend/CompilerInvocation.cpp


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2113,12 +2113,6 @@
   Args.hasFlag(OPT_fdeclspec, OPT_fno_declspec,
(Opts.MicrosoftExt || Opts.Borland || Opts.CUDA));
 
-  // For now, we only support local submodule visibility in C++ (because we
-  // heavily depend on the ODR for merging redefinitions).
-  if (Opts.ModulesLocalVisibility && !Opts.CPlusPlus)
-Diags.Report(diag::err_drv_argument_not_allowed_with)
-<< "-fmodules-local-submodule-visibility" << "C";
-
   if (Arg *A = Args.getLastArg(OPT_faddress_space_map_mangling_EQ)) {
 switch (llvm::StringSwitch(A->getValue())
   .Case("target", LangOptions::ASMM_Target)


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2113,12 +2113,6 @@
   Args.hasFlag(OPT_fdeclspec, OPT_fno_declspec,
(Opts.MicrosoftExt || Opts.Borland || Opts.CUDA));
 
-  // For now, we only support local submodule visibility in C++ (because we
-  // heavily depend on the ODR for merging redefinitions).
-  if (Opts.ModulesLocalVisibility && !Opts.CPlusPlus)
-Diags.Report(diag::err_drv_argument_not_allowed_with)
-<< "-fmodules-local-submodule-visibility" << "C";
-
   if (Arg *A = Args.getLastArg(OPT_faddress_space_map_mangling_EQ)) {
 switch (llvm::StringSwitch(A->getValue())
   .Case("target", LangOptions::ASMM_Target)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25866: [Sema] Support implicit scalar to vector conversions

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:8188
+  //type and then perform the rest of the checks here. GCC as of
+  //pre-release 7.0 does not accept this though.
+  if (VectorEltTy->isIntegralType(S.Context) &&

sdardis wrote:
> bruno wrote:
> > Is this something that GCC is going to support at some point? Regardless of 
> > GCC, do we want this behavior?
> > Is this something that GCC is going to support at some point?
> 
> I've reread GCC's source for conversions of scalar to vectors types and it 
> appears that GCC does attempt to convert constant real expressions to 
> integers but it appears to only work for C++. It's a little odd.
> 
> > Regardless of GCC, do we want this behavior?
> 
> Given my oversight of the support of implicit safe conversions of floating 
> point constants to integer types, I believe we should do this in general. 
> I'll rework the comment to be more accurate and add the necessary conversion 
> checks.
Ok, improving the FIXME sounds good to me. We can add this extra functionality 
in a future patch.



Comment at: lib/Sema/SemaExpr.cpp:10019
   isa(vType->getAs()) || getLangOpts().OpenCL;
+  if ((!getLangOpts().CPlusPlus && !getLangOpts().OpenCL) && !isExtVectorType)
+return InvalidVectorOperands(Loc, LHS, RHS);

sdardis wrote:
> bruno wrote:
> > Why `!getLangOpts().CPlusPlus` is a requirement here?
> GCC only supports the logical operators &&, ||, ! when compiling C++.
> 
> It's noted near the bottom half of this page: 
> https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html
Ok. It would be nice to find out if there's any actual good reason for it, 
otherwise we might as well support it for non C++. But no need to block on 
that; just make sure to annotate these places with FIXMEs so we can work on it 
in the future. 


https://reviews.llvm.org/D25866



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

[PATCH] D31667: [Sema] Extend GetSignedVectorType to deal with non ExtVector types

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

Thanks Simon! LGTM


https://reviews.llvm.org/D31667



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

[PATCH] D31781: [Modules] Allow local submodule visibility without c++

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL300108: [Modules] Enable local submodule visibility for 
ObjC/C (authored by bruno).

Changed prior to commit:
  https://reviews.llvm.org/D31781?vs=94407=95034#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D31781

Files:
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/Modules/localsubmodulevis.m


Index: cfe/trunk/test/Modules/localsubmodulevis.m
===
--- cfe/trunk/test/Modules/localsubmodulevis.m
+++ cfe/trunk/test/Modules/localsubmodulevis.m
@@ -0,0 +1,8 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility 
-fimplicit-module-maps -fmodules-cache-path=%t.a -I %S/Inputs/preprocess 
-verify %s
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility 
-fimplicit-module-maps -fmodules-cache-path=%t.b -I %S/Inputs/preprocess -x c 
-verify -x c %s
+
+// expected-no-diagnostics
+
+#include "file.h"
+
Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -2117,12 +2117,6 @@
   Args.hasFlag(OPT_fdeclspec, OPT_fno_declspec,
(Opts.MicrosoftExt || Opts.Borland || Opts.CUDA));
 
-  // For now, we only support local submodule visibility in C++ (because we
-  // heavily depend on the ODR for merging redefinitions).
-  if (Opts.ModulesLocalVisibility && !Opts.CPlusPlus)
-Diags.Report(diag::err_drv_argument_not_allowed_with)
-<< "-fmodules-local-submodule-visibility" << "C";
-
   if (Arg *A = Args.getLastArg(OPT_faddress_space_map_mangling_EQ)) {
 switch (llvm::StringSwitch(A->getValue())
   .Case("target", LangOptions::ASMM_Target)


Index: cfe/trunk/test/Modules/localsubmodulevis.m
===
--- cfe/trunk/test/Modules/localsubmodulevis.m
+++ cfe/trunk/test/Modules/localsubmodulevis.m
@@ -0,0 +1,8 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -fimplicit-module-maps -fmodules-cache-path=%t.a -I %S/Inputs/preprocess -verify %s
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -fimplicit-module-maps -fmodules-cache-path=%t.b -I %S/Inputs/preprocess -x c -verify -x c %s
+
+// expected-no-diagnostics
+
+#include "file.h"
+
Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -2117,12 +2117,6 @@
   Args.hasFlag(OPT_fdeclspec, OPT_fno_declspec,
(Opts.MicrosoftExt || Opts.Borland || Opts.CUDA));
 
-  // For now, we only support local submodule visibility in C++ (because we
-  // heavily depend on the ODR for merging redefinitions).
-  if (Opts.ModulesLocalVisibility && !Opts.CPlusPlus)
-Diags.Report(diag::err_drv_argument_not_allowed_with)
-<< "-fmodules-local-submodule-visibility" << "C";
-
   if (Arg *A = Args.getLastArg(OPT_faddress_space_map_mangling_EQ)) {
 switch (llvm::StringSwitch(A->getValue())
   .Case("target", LangOptions::ASMM_Target)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27604: [Driver] Add compiler option to generate a reproducer

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL300109: [Driver] Add compiler option to generate a 
reproducer (authored by bruno).

Changed prior to commit:
  https://reviews.llvm.org/D27604?vs=80981=95035#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27604

Files:
  cfe/trunk/docs/UsersManual.rst
  cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
  cfe/trunk/include/clang/Driver/Driver.h
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/lib/Driver/Driver.cpp
  cfe/trunk/test/Driver/crash-report-crashfile.m
  cfe/trunk/tools/driver/driver.cpp

Index: cfe/trunk/lib/Driver/Driver.cpp
===
--- cfe/trunk/lib/Driver/Driver.cpp
+++ cfe/trunk/lib/Driver/Driver.cpp
@@ -91,7 +91,7 @@
   CCCPrintBindings(false), CCPrintHeaders(false), CCLogDiagnostics(false),
   CCGenDiagnostics(false), DefaultTargetTriple(DefaultTargetTriple),
   CCCGenericGCCName(""), CheckInputsExist(true), CCCUsePCH(true),
-  SuppressMissingInputWarning(false) {
+  GenReproducer(false), SuppressMissingInputWarning(false) {
 
   // Provide a sane fallback if no VFS is specified.
   if (!this->VFS)
@@ -620,6 +620,9 @@
 CCCGenericGCCName = A->getValue();
   CCCUsePCH =
   Args.hasFlag(options::OPT_ccc_pch_is_pch, options::OPT_ccc_pch_is_pth);
+  GenReproducer = Args.hasFlag(options::OPT_gen_reproducer,
+   options::OPT_fno_crash_diagnostics,
+   !!::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH"));
   // FIXME: DefaultTargetTriple is used by the target-prefixed calls to as/ld
   // and getToolChain is const.
   if (IsCLMode()) {
Index: cfe/trunk/tools/driver/driver.cpp
===
--- cfe/trunk/tools/driver/driver.cpp
+++ cfe/trunk/tools/driver/driver.cpp
@@ -460,8 +460,9 @@
 Res = TheDriver.ExecuteCompilation(*C, FailingCommands);
 
   // Force a crash to test the diagnostics.
-  if (::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH")) {
-Diags.Report(diag::err_drv_force_crash) << "FORCE_CLANG_DIAGNOSTICS_CRASH";
+  if (TheDriver.GenReproducer) {
+Diags.Report(diag::err_drv_force_crash)
+<< !::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH");
 
 // Pretend that every command failed.
 FailingCommands.clear();
Index: cfe/trunk/docs/UsersManual.rst
===
--- cfe/trunk/docs/UsersManual.rst
+++ cfe/trunk/docs/UsersManual.rst
@@ -562,6 +562,16 @@
 The -fno-crash-diagnostics flag can be helpful for speeding the process
 of generating a delta reduced test case.
 
+Clang is also capable of generating preprocessed source file(s) and associated
+run script(s) even without a crash. This is specially useful when trying to
+generate a reproducer for warnings or errors while using modules.
+
+.. option:: -gen-reproducer
+
+  Generates preprocessed source files, a reproducer script and if relevant, a
+  cache containing: built module pcm's and all headers needed to rebuilt the
+  same modules.
+
 .. _rpass:
 
 Options to Emit Optimization Reports
Index: cfe/trunk/include/clang/Driver/Driver.h
===
--- cfe/trunk/include/clang/Driver/Driver.h
+++ cfe/trunk/include/clang/Driver/Driver.h
@@ -216,6 +216,11 @@
   /// Use lazy precompiled headers for PCH support.
   unsigned CCCUsePCH : 1;
 
+  /// Force clang to emit reproducer for driver invocation. This is enabled
+  /// indirectly by setting FORCE_CLANG_DIAGNOSTICS_CRASH environment variable
+  /// or when using the -gen-reproducer driver flag.
+  unsigned GenReproducer : 1;
+
 private:
   /// Certain options suppress the 'no input files' warning.
   unsigned SuppressMissingInputWarning : 1;
Index: cfe/trunk/include/clang/Driver/Options.td
===
--- cfe/trunk/include/clang/Driver/Options.td
+++ cfe/trunk/include/clang/Driver/Options.td
@@ -265,6 +265,8 @@
 def arcmt_migrate_emit_arc_errors : Flag<["-"], "arcmt-migrate-emit-errors">,
   HelpText<"Emit ARC errors even if the migrator can fix them">,
   Flags<[CC1Option]>;
+def gen_reproducer: Flag<["-"], "gen-reproducer">, InternalDebugOpt,
+  HelpText<"Auto-generates preprocessed source files and a reproduction script">;
 
 def _migrate : Flag<["--"], "migrate">, Flags<[DriverOption]>,
   HelpText<"Run the migrator">;
@@ -701,7 +703,8 @@
 def fconstexpr_steps_EQ : Joined<["-"], "fconstexpr-steps=">, Group;
 def fconstexpr_backtrace_limit_EQ : Joined<["-"], "fconstexpr-backtrace-limit=">,
 Group;
-def fno_crash_diagnostics : Flag<["-"], "fno-crash-diagnostics">, Group, Flags<[NoArgumentUnused]>;
+def fno_crash_diagnostics : Flag<["-"], "fno-crash-diagnostics">, Group, Flags<[NoArgumentUnused]>,
+  HelpText<"Disable auto-generation of preprocessed source files and a script for 

[PATCH] D28670: [ObjC] Disallow vector parameters and return values in Objective-C methods on older X86 targets

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: lib/Sema/SemaDeclObjC.cpp:4309
+  const ObjCMethodDecl *Method) {
+  SourceLocation Loc;
+  QualType T;

Maybe add an assert for `Triple.getArch() == llvm::Triple::x86)` here?


Repository:
  rL LLVM

https://reviews.llvm.org/D28670



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

[PATCH] D28832: Improve redefinition errors pointing to the same header.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 95369.
bruno marked 2 inline comments as done.
bruno added a comment.

Updated the patch after Richard's comments:

Before
--

  In file included from x.c:2:
  Inputs4/C.h:3:5: error: redefinition of 'c'
  int c = 1;
  ^
  In module 'X' imported from x.c:1:
  Inputs4/C.h:3:5: note: previous definition is here
  int c = 1;
  ^
  1 error generated.

After
-

  In file included from x.c:2:
  Inputs4/C.h:3:5: error: redefinition of 'c'
  int c = 1;
  ^
  In module 'X' imported from x.c:1:
  Inputs4/B.h:3:10: note: 'Inputs4/C.h' included multiple times, additional 
include site in header from module 'X.B'
  #include "C.h"
   ^
  Inputs4/module.modulemap:6:10: note: X.B defined here
module B {
   ^
  x.c:2:10: note: 'Inputs4/C.h' included multiple times, additional include 
site here
  #include "C.h" // textual include
   ^
  1 error generated.


https://reviews.llvm.org/D28832

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaDecl.cpp
  test/Modules/Inputs/SameHeader/A.h
  test/Modules/Inputs/SameHeader/B.h
  test/Modules/Inputs/SameHeader/C.h
  test/Modules/Inputs/SameHeader/module.modulemap
  test/Modules/redefinition-same-header.m
  test/Sema/redefinition-same-header.c

Index: test/Sema/redefinition-same-header.c
===
--- /dev/null
+++ test/Sema/redefinition-same-header.c
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'int yyy = 42;' > %t/a.h
+// RUN: %clang_cc1 -fsyntax-only %s -I%t  -verify
+
+// expected-error@a.h:1 {{redefinition of 'yyy'}}
+// expected-note@a.h:1 {{unguarded header; consider using #ifdef guards or #pragma once}}
+// expected-note-re@redefinition-same-header.c:11 {{'{{.*}}/a.h' included multiple times, additional include site here}}
+// expected-note-re@redefinition-same-header.c:12 {{'{{.*}}/a.h' included multiple times, additional include site here}}
+
+#include "a.h"
+#include "a.h"
+
+int foo() { return yyy; }
Index: test/Modules/redefinition-same-header.m
===
--- /dev/null
+++ test/Modules/redefinition-same-header.m
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t.tmp
+// RUN: %clang_cc1 -fsyntax-only -I %S/Inputs/SameHeader -fmodules \
+// RUN:   -fimplicit-module-maps -fmodules-cache-path=%t.tmp %s -verify
+
+// expected-error@Inputs/SameHeader/C.h:3 {{redefinition of 'c'}}
+// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}}
+
+// expected-error@Inputs/SameHeader/C.h:5 {{redefinition of 'aaa'}}
+// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}}
+
+// expected-error@Inputs/SameHeader/C.h:9 {{redefinition of 'fd_set'}}
+// expected-note-re@Inputs/SameHeader/B.h:3 {{'{{.*}}/C.h' included multiple times, additional include site in header from module 'X.B'}}
+// expected-note@Inputs/SameHeader/module.modulemap:6 {{X.B defined here}}
+// expected-note-re@redefinition-same-header.m:20 {{'{{.*}}/C.h' included multiple times, additional include site here}}
+#include "A.h" // maps to a modular
+#include "C.h" // textual include
Index: test/Modules/Inputs/SameHeader/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/SameHeader/module.modulemap
@@ -0,0 +1,11 @@
+module X {
+  module A {
+header "A.h"
+export *
+  }
+  module B {
+header "B.h"
+export *
+  }
+  export *
+}
Index: test/Modules/Inputs/SameHeader/C.h
===
--- /dev/null
+++ test/Modules/Inputs/SameHeader/C.h
@@ -0,0 +1,12 @@
+#ifndef __C_h__
+#define __C_h__
+int c = 1;
+
+struct aaa {
+  int b;
+};
+
+typedef struct fd_set {
+  char c;
+};
+#endif
Index: test/Modules/Inputs/SameHeader/B.h
===
--- /dev/null
+++ test/Modules/Inputs/SameHeader/B.h
@@ -0,0 +1,4 @@
+#ifndef __B_h__
+#define __B_h__
+#include "C.h"
+#endif
Index: test/Modules/Inputs/SameHeader/A.h
===
--- /dev/null
+++ test/Modules/Inputs/SameHeader/A.h
@@ -0,0 +1,3 @@
+#ifndef __A_h__
+#define __A_h__
+#endif
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -1969,7 

[PATCH] D28832: Improve redefinition errors pointing to the same header.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

@v.g.vassilev your testcase is interesting but it's unrelated to this specific 
improvement. The error message it's triggered by (a) the decl name not being 
found because `kROOTD_OPEN` is hidden, (b) type transforms looks for an 
alternative (c) `Sema::diagnoseMissingImport` is called and produces the error 
you're seeing. I can fix this one too once we decide what we consider a good 
diagnostic here, let's iterate in http://bugs.llvm.org/show_bug.cgi?id=32670


https://reviews.llvm.org/D28832



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

[PATCH] D28832: Improve redefinition errors pointing to the same header.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked 5 inline comments as done.
bruno added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8708
+def note_redefinition_modules_same_file : Note<
+   "'%0' included multiple times, additional (likely non-modular) include 
site in module '%1'">;
+def note_redefinition_modules_same_file_modulemap : Note<

rsmith wrote:
> If we can't be sure whether or not the other include side was a modular 
> include, making a guess is probably not helpful. (In fact, I've seen this 
> issue show up a lot more where the header is a modular header in both 
> modules.)
After thinking more about this I agree: if used together with 
"-Wnon-modular-include-in-module" and similars, it should become more evident 
to the user what this is about.



Comment at: lib/Sema/SemaDecl.cpp:3747
+  // is confusing, try to get better diagnostics when modules is on.
+  auto OldModLoc = SrcMgr.getModuleImportLoc(Old);
+  if (!OldModLoc.first.isInvalid()) {

rsmith wrote:
> Rather than listing where the module was first imported (which could be 
> unrelated to the problem), it would make more sense to list the location at 
> which the previous declaration was made visible. You can get that by querying 
> the `VisibleModuleSet` for the owning module of the definition and every 
> other module in its merged definitions set (see 
> `Sema::hasVisibleMergedDefinition`).
I tried this, but AFAIU the Decls coming from non-modular headers are usually 
hidden, and since it has not been merged, which happens in the testcase because 
it's a var redefinition error, then we have no import location to get 
information from. Do you have a synthetic testcase in mind that I can use? I'm 
happy to follow up with that (here or in a next patch).



Comment at: lib/Sema/SemaDecl.cpp:3755-3759
+  if (!Mod->DefinitionLoc.isInvalid())
+Diag(Mod->DefinitionLoc,
+ diag::note_redefinition_modules_same_file_modulemap)
+<< SrcMgr.getFilename(SrcMgr.getSpellingLoc(Old)).str()
+<< Mod->getFullModuleName();

rsmith wrote:
> Is this ("add the header to the module map for some other module that 
> happened to include it first") really generally good advice? People have a 
> tendency to follow such guidance blindly.
Indeed, going to remove that.


https://reviews.llvm.org/D28832



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

[PATCH] D25383: [libcxx][modules] Add a submodule for math.h

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno abandoned this revision.
bruno added a comment.

This isn't needed anymore.


https://reviews.llvm.org/D25383



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

[PATCH] D27546: [ASTReader] Sort RawComments before merging

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno closed this revision.
bruno added a comment.

Done in r290134


https://reviews.llvm.org/D27546



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

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

@rsmith ping!


https://reviews.llvm.org/D31778



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

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked 6 inline comments as done.
bruno added inline comments.



Comment at: lib/Parse/ParseDecl.cpp:4236-4237
 
   Sema::SkipBodyInfo SkipBody;
+  Sema::CheckCompatTagInfo CheckCompatTag;
   if (!Name && TUK == Sema::TUK_Definition && Tok.is(tok::l_brace) &&

rsmith wrote:
> Do we really need both of these? The new stuff seems to be a natural 
> extension of `SkipBodyInfo`, to say "parse it, check it's the same as the 
> previous definition, then throw the new one away".
No specific reason. I'll augment `SkipBodyInfo` with two new fields then (a) a 
flag for the check and (b) the new tag.



Comment at: lib/Sema/SemaExpr.cpp:2198-2209
+  if (R.isAmbiguous()) {
+if (!IgnorePrevDef)
+  return ExprError();
+// We already know that there's a hidden decl included in the lookup,
+// ignore it and only use the first found (the local) one...
+auto RF = R.makeFilter();
+NamedDecl *ND = R.getRepresentativeDecl();

rsmith wrote:
> This is gross. In order to make this work, you'd need to propagate the 
> `IgnorePrevDef` flag through the entire expression parsing machinery.
> 
> Instead of this, how about deferring making the old definition visible until 
> after you complete parsing the new one and do the structural comparison?
Thanks for pointing it out, I totally missed this approach. Your suggestion 
works and I'll change the patch accordingly. However, `IgnorePrevDef` still 
needs to be threaded in `ParseEnumBody` and `ActOnEnumConstant` in order to 
prevent the latter to emit `err_redefinition_of_enumerator`-like diagnostics.


https://reviews.llvm.org/D31778



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

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 96080.
bruno marked 2 inline comments as done.
bruno added a comment.

Update the patch after Richard's suggestions


https://reviews.llvm.org/D31778

Files:
  include/clang/AST/ASTStructuralEquivalence.h
  include/clang/Basic/DiagnosticASTKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/AST/ASTStructuralEquivalence.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseExpr.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaType.cpp
  test/Modules/Inputs/F.framework/Headers/F.h
  test/Modules/Inputs/F.framework/Modules/module.modulemap
  test/Modules/Inputs/F.framework/Modules/module.private.modulemap
  test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
  test/Modules/elaborated-type-specifier-from-hidden-module.m
  test/Modules/redefinition-c-tagtypes.m

Index: test/Modules/redefinition-c-tagtypes.m
===
--- /dev/null
+++ test/Modules/redefinition-c-tagtypes.m
@@ -0,0 +1,48 @@
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -fsyntax-only %s -fmodules -fmodules-cache-path=%t.cache \
+// RUN:   -fimplicit-module-maps -F%S/Inputs -verify
+// RUN: %clang_cc1 -fsyntax-only %s -fmodules -fmodules-cache-path=%t.cache \
+// RUN:   -fimplicit-module-maps -F%S/Inputs -DCHANGE_TAGS -verify
+#include "F/F.h"
+
+#ifndef CHANGE_TAGS
+// expected-no-diagnostics
+#endif
+
+struct NS {
+  int a;
+#ifndef CHANGE_TAGS
+  int b;
+#else
+  int c; // expected-note {{field has name 'c' here}}
+  // expected-error@redefinition-c-tagtypes.m:12 {{type 'struct NS' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:3 {{field has name 'b' here}}
+#endif
+};
+
+enum NSE {
+  FST = 22,
+#ifndef CHANGE_TAGS
+  SND = 43,
+#else
+  SND = 44, // expected-note {{enumerator 'SND' with value 44 here}}
+  // expected-error@redefinition-c-tagtypes.m:23 {{type 'enum NSE' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:8 {{enumerator 'SND' with value 43 here}}
+#endif
+  TRD = 55
+};
+
+#define NS_ENUM(_type, _name) \
+  enum _name : _type _name;   \
+  enum _name : _type
+
+typedef NS_ENUM(int, NSMyEnum) {
+  MinX = 11,
+#ifndef CHANGE_TAGS
+  MinXOther = MinX,
+#else
+  MinXOther = TRD, // expected-note {{enumerator 'MinXOther' with value 55 here}}
+  // expected-error@redefinition-c-tagtypes.m:39 {{type 'enum NSMyEnum' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:18 {{enumerator 'MinXOther' with value 11 here}}
+#endif
+};
Index: test/Modules/elaborated-type-specifier-from-hidden-module.m
===
--- test/Modules/elaborated-type-specifier-from-hidden-module.m
+++ test/Modules/elaborated-type-specifier-from-hidden-module.m
@@ -4,12 +4,11 @@
 @import ElaboratedTypeStructs.Empty; // The structs are now hidden.
 struct S1 *x;
 struct S2 *y;
-// FIXME: compatible definition should not be an error.
-struct S2 { int x; }; // expected-error {{redefinition}}
+struct S2 { int x; };
 struct S3 *z;
 // Incompatible definition.
-struct S3 { float y; }; // expected-error {{redefinition}}
-// expected-note@elaborated-type-structs.h:* 2 {{previous definition is here}}
+struct S3 { float y; }; // expected-error {{has incompatible definitions}} // expected-note {{field has name}}
+// expected-note@Inputs/elaborated-type-structs.h:3 {{field has name}}
 
 @import ElaboratedTypeStructs.Structs;
 
Index: test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
@@ -0,0 +1,19 @@
+struct NS {
+  int a;
+  int b;
+};
+
+enum NSE {
+  FST = 22,
+  SND = 43,
+  TRD = 55
+};
+
+#define NS_ENUM(_type, _name) \
+  enum _name : _type _name;   \
+  enum _name : _type
+
+typedef NS_ENUM(int, NSMyEnum) {
+  MinX = 11,
+  MinXOther = MinX,
+};
Index: test/Modules/Inputs/F.framework/Modules/module.private.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/Modules/module.private.modulemap
@@ -0,0 +1,7 @@
+module F.Private [system] {
+  explicit module NS {
+  header "NS.h"
+  export *
+  }
+  export *
+}
Index: test/Modules/Inputs/F.framework/Modules/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/Modules/module.modulemap
@@ -0,0 +1,7 @@
+framework module F [extern_c] [system] {
+  umbrella header "F.h"
+  module * {
+  export *
+  }
+  export *
+}
Index: test/Modules/Inputs/F.framework/Headers/F.h
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/Headers/F.h
@@ -0,0 +1 @@
+// F.h
Index: lib/Sema/SemaType.cpp
===
--- 

[PATCH] D13117: [DarwinDriver] Use -lto_library to specify the path for libLTO.dylib

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno closed this revision.
bruno added a comment.

Done way back in r248932


Repository:
  rL LLVM

https://reviews.llvm.org/D13117



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

[PATCH] D27429: [Chrono][Darwin] On Darwin use CLOCK_UPTIME_RAW instead of CLOCK_MONOTONIC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno closed this revision.
bruno added a comment.

r291466 & r291517


https://reviews.llvm.org/D27429



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

[PATCH] D29001: [Modules] Fallback to building the module if a timeout occurs

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno closed this revision.
bruno added a comment.

Updating: already done in r298175


https://reviews.llvm.org/D29001



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

[PATCH] D30768: [PATCH][VFS] Ignore broken symlinks in the directory iterator.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Juergen,

Thanks for working on this.




Comment at: include/clang/Basic/VirtualFileSystem.h:164
 EC = Impl->increment();
-if (EC || !Impl->CurrentEntry.isStatusKnown())
+if (!Impl->CurrentEntry.isStatusKnown())
   Impl.reset(); // Normalize the end iterator to Impl == nullptr.

I would rather we don't drop checks for `EC`s - it's commonly assumed that 
whenever they are passed in we wanna check them for errors, can't you just skip 
the specific case you want to avoid?



Comment at: lib/Basic/VirtualFileSystem.cpp:1860
   directory_iterator I = FS->dir_begin(Path, EC);
-  if (!EC && I != directory_iterator()) {
+  if (I != directory_iterator()) {
 State = std::make_shared();

Same here.



Comment at: lib/Basic/VirtualFileSystem.cpp:1873
 vfs::directory_iterator I = FS->dir_begin(State->top()->getName(), EC);
-if (EC)
+if (EC && EC != std::errc::no_such_file_or_directory)
   return *this;

Can you add a comment explaining why you are doing it? I would prefer if we 
reset the `EC` state here than having the callers ignoring `EC` results.


https://reviews.llvm.org/D30768



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

[PATCH] D27810: FileManager: mark virtual file entries as valid entries

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Please attach a testcase!


https://reviews.llvm.org/D27810



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

[PATCH] D30183: Add -iframeworkwithsysroot compiler option

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

Hi Alex,

Thanks for taking a look a this. LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D30183



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

[PATCH] D30768: [PATCH][VFS] Ignore broken symlinks in the directory iterator.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Basic/VirtualFileSystem.cpp:1873
 vfs::directory_iterator I = FS->dir_begin(State->top()->getName(), EC);
-if (EC)
+if (EC && EC != std::errc::no_such_file_or_directory)
   return *this;

ributzka wrote:
> bruno wrote:
> > Can you add a comment explaining why you are doing it? I would prefer if we 
> > reset the `EC` state here than having the callers ignoring `EC` results.
> If we reset the EC here, then the caller won't know that there was an issue. 
> The idea is that we still want the caller to check EC. It should be up to the 
> caller to decide how to act on this particular error.
> 
> I guess since the caller has to check for the error anyway I could even 
> remove this check completely and not check EC at all here.
Right, this should be fine with the latest changes and per discussions offline. 
LGTM


https://reviews.llvm.org/D30768



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

[PATCH] D30915: Canonicalize the path provided by -fmodules-cache-path

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM




Comment at: lib/Frontend/CompilerInvocation.cpp:1434
+
+  // Canonicalize -fmodules-cache-path before storing it.
+  SmallString<128> P(Args.getLastArgValue(OPT_fmodules_cache_path));

I would suggest using `FileManager::makeAbsolutePath`, but at this point looks 
like we haven't set up a FileManager yet.


https://reviews.llvm.org/D30915



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

[PATCH] D30810: Preserve vec3 type.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a reviewer: bruno.
bruno added a comment.

Hi JinGu,

I just read the discussion on cfe-dev, some comments:

> My colleague and I are implementing a transformation pass between LLVM IR and 
> another IR and we want to keep the 3-component vector types in our target IR

Why can't you go back through the shuffle to find out that it comes from a vec3 
-> vec4 transformation? Adding a flag here just for that seems very odd.


https://reviews.llvm.org/D30810



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

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

Hi Pete, thanks for working on this!

LGTM with the minor comments below.




Comment at: include/clang/Lex/PPCallbacks.h:264
+  virtual void HasInclude(SourceLocation Loc, const FileEntry *File) {
+  }
   

clang-format this one!



Comment at: test/Frontend/dependency-gen-has-include.c:17
+#if __has_include("a/header.h")
+#endif

Can you also add a testcase for `__has_include_next`?


https://reviews.llvm.org/D30882



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

[PATCH] D30881: Track skipped files in dependency scanning

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Pete,




Comment at: lib/Frontend/DependencyFile.cpp:191
+   const Token ,
+   SrcMgr::CharacteristicKind FileType) override;
+

Is there any `FileSkipped` callback invocation that might trigger an unwanted 
file to be added as a dependency or it will only trigger for the symlink case?



Comment at: test/Frontend/dependency-gen-symlink.c:11
+// RUN: echo "#endif" >> %t.dir/a/header.h
+// RUN: ln %t.dir/a/header.h %t.dir/b/header.h
+

It seems that it works for hard links as well right? Is this intended or do you 
miss a `-s`?


https://reviews.llvm.org/D30881



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

[PATCH] D29117: SPARC: allow usage of floating-point registers in inline ASM

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi,

Thanks for working on this. Few questions:

- What happens with the validation if +soft-float is used?
- What about the 'e' mode, can you double check if the sparc backend support 
these instructions? If so it might be interesting to add it here.

Also, please attach patches with full context, it's easier to review.


https://reviews.llvm.org/D29117



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

[PATCH] D25866: [Sema] Support implicit scalar to vector conversions

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hey, really sorry for the delay here.




Comment at: lib/Sema/SemaExpr.cpp:8007
+static bool canConvertIntToOtherIntTy(Sema , ExprResult *Int,
+   QualType OtherIntTy) {
+  QualType IntTy = Int->get()->getType().getUnqualifiedType();

This doesn't look clang-formated. 



Comment at: lib/Sema/SemaExpr.cpp:8034
+NumBits > S.Context.getIntWidth(OtherIntTy))
+  return true;
+  } else {

Instead of the else here, you can:
   
  return (IntSigned != OtherIntSigned &&
  NumBits > S.Context.getIntWidth(OtherIntTy))
   }

   return (Order < 0);



Comment at: lib/Sema/SemaExpr.cpp:8092
+ExprResult *Vector) {
+  QualType ScalarTy = Scalar->get()->getType().getUnqualifiedType();
+  QualType VectorTy = Vector->get()->getType().getUnqualifiedType();

Can you please add an assertion to the beginning of this function to guarantee 
that the vector is never a ext-vector type? I wanna make sure we don't 
accidentally use this in the future for ext-vectors. 



Comment at: test/Sema/vector-gcc-compat.c:61
+  //match.
+  v2i64_r = v2i64_a == 1; // expected-warning {{incompatible vector types 
assigning to 'v2i64' (vector of 2 'long long' values) from 'long 
__attribute__((ext_vector_type(2)))' (vector of 2 'long' values)}}
+  v2i64_r = v2i64_a != 1; // expected-warning {{incompatible vector types 
assigning to 'v2i64' (vector of 2 'long long' values) from 'long 
__attribute__((ext_vector_type(2)))' (vector of 2 'long' values)}}

Can you double check where 'long __attribute__((ext_vector_type(2)))' comes 
from?

We have regressed in the past year in the way ext-vector interacts with 
non-ext-vectors, and I don't wanna make it worse until we actually have time to 
fix that; there's a lot of code out there relying on bitcasts between 
ext-vectors and non-ext-vectors to bridge between intrinsics headers and 
ext-vector code.


https://reviews.llvm.org/D25866



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

[PATCH] D28218: Small optimizations for SourceManager::getFileID()

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Daniel,

This seems pretty nice. Can you share how much performance improvements you got 
by this / how did you test it?


https://reviews.llvm.org/D28218



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

[PATCH] D28670: [ObjC] Disallow vector parameters and return values in Objective-C methods on older X86 targets

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Alex,




Comment at: lib/Sema/SemaDeclObjC.cpp:4312
+  for (const ParmVarDecl *P : Method->parameters()) {
+if (P->getType()->isVectorType()) {
+  Loc = P->getLocStart();

Assuming objc/c++ can pass/return these, the current check wouldn't be enough 
to cover x86_64 ABI cases where small (< 8 bytes) trivial classes containing 
vector types are directly passed / returned as vector types. You might want to 
check if that applies here.

See test/CodeGenCXX/x86_64-arguments-avx.cpp for examples (and x86_64 abi doc, 
item 3.2.3)



Comment at: test/SemaObjC/x86-method-vector-values.m:16
+
+typedef __attribute__((__ext_vector_type__(3))) float float3;
+

Can you also add to the testcase instances that use regular non-ext vector 
(`vector_type(...)`)?


Repository:
  rL LLVM

https://reviews.llvm.org/D28670



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

[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



> It seems to me that the problem here is that `DeclMustBeEmitted` is not safe 
> to call in the middle of deserialization if anything partially-deserialized 
> might be reachable from the declaration we're querying, and yet we're 
> currently calling it in that case.

`DeclMustBeEmitted` name seems misleading since we does more than just 
checking, it actually tries to emit stuff. If it's a local module, shouldn't be 
everything already available? Do we have non-deserialized items for a local 
module? Maybe I get the wrong idea of what local means in this context..

> I don't see how this patch actually addresses that problem, though: if you 
> build this testcase as a module instead of a PCH, won't it still fail in the 
> same way that it does now?

`D->getImportedOwningModule` calls `isFromASTFile`, which refers to PCH and 
modules, shouldn't it work for both then? I couldn't come up with a testcase 
that triggered it for modules though.

> I think we probably need to track all the declarations we deserialize in a 
> top-level deserialization,  and only check whether they're interesting when 
> we finish recursive deserialization (somewhere around 
> `ASTReader::FinishedDeserializing`). For the update record case, this will 
> need some care in order to retain the property that we only pass a 
> declaration to the consumer once, but I think we can make that work by 
> observing that it should generally be safe to call `DeclMustBeEmitted` on a 
> declaration that we didn't create in this deserialization step, and when 
> we're loading update records we know whether that's the case.

Right. This is a more involved fix and I don't fully understand all r276159 
consequences yet. OTOH, the current patch improves the situation and unblock 
some big projects to compile with clang ToT, for instance, Unreal Engine 4.


https://reviews.llvm.org/D29753



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

[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



> What do folks think?

IMO we should do it.


https://reviews.llvm.org/D29753



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

[PATCH] D29753: [PCH] Avoid early VarDecl emission attempt if no owning module avaiable

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL296656: [PCH] Avoid VarDecl emission attempt if no owning 
module avaiable (authored by bruno).

Changed prior to commit:
  https://reviews.llvm.org/D29753?vs=87774=90212#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29753

Files:
  cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
  cfe/trunk/test/PCH/empty-def-fwd-struct.h


Index: cfe/trunk/test/PCH/empty-def-fwd-struct.h
===
--- cfe/trunk/test/PCH/empty-def-fwd-struct.h
+++ cfe/trunk/test/PCH/empty-def-fwd-struct.h
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -emit-pch -x c++-header %s -std=c++14 -o %t.pch
+// RUN: %clang_cc1 -emit-llvm-only -x c++ /dev/null -std=c++14 -include-pch 
%t.pch -o %t.o
+struct FVector;
+struct FVector {};
+struct FBox {
+  FVector Min;
+  FBox(int);
+};
+namespace {
+FBox InvalidBoundingBox(0);
+}
+
Index: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
===
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
@@ -2530,8 +2530,8 @@
 
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() 
&&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 
   if (isa(D) || 


Index: cfe/trunk/test/PCH/empty-def-fwd-struct.h
===
--- cfe/trunk/test/PCH/empty-def-fwd-struct.h
+++ cfe/trunk/test/PCH/empty-def-fwd-struct.h
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -emit-pch -x c++-header %s -std=c++14 -o %t.pch
+// RUN: %clang_cc1 -emit-llvm-only -x c++ /dev/null -std=c++14 -include-pch %t.pch -o %t.o
+struct FVector;
+struct FVector {};
+struct FBox {
+  FVector Min;
+  FBox(int);
+};
+namespace {
+FBox InvalidBoundingBox(0);
+}
+
Index: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
===
--- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
+++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
@@ -2530,8 +2530,8 @@
 
   // An ImportDecl or VarDecl imported from a module will get emitted when
   // we import the relevant module.
-  if ((isa(D) || isa(D)) && Ctx.DeclMustBeEmitted(D) &&
-  D->getImportedOwningModule())
+  if ((isa(D) || isa(D)) && D->getImportedOwningModule() &&
+  Ctx.DeclMustBeEmitted(D))
 return false;
 
   if (isa(D) || 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.



Comment at: include/clang/AST/ASTStructuralEquivalence.h:52
 
+  /// \brief Whether warn or error on tag type mismatches.
+  bool ErrorOnTagTypeMismatch;

aprantl wrote:
> No need to use \brief in new code any more. We are compiling with the Doxygen 
> autobrief option.
Sure, updated D31777 to also remove the `\brief`s in the transition



Comment at: lib/Parse/ParseDecl.cpp:4316
+!Actions.hasStructuralCompatLayout(TagDecl, CheckCompatTag.NewDef)) {
+  DS.SetTypeSpecError(); // Bail out...
+  return;

aprantl wrote:
> I think the LLVM coding style wants this to be 
> ```
> {
> // Bail out.
> DS.SetTypeSpecError();
> return;
> }
> ```
> 
> or
> ```
> // Bail out.
> return DS.SetTypeSpecError();
> ```
OK



Comment at: lib/Sema/SemaDecl.cpp:13386
+  bool AllowODR = getLangOpts().CPlusPlus || getLangOpts().ObjC1 ||
+  getLangOpts().C11;
   NamedDecl *Hidden = nullptr;

aprantl wrote:
> Does `ObjC2` imply `ObjC1`?
Yep!


https://reviews.llvm.org/D31778



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

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 94471.
bruno added a comment.

Update patch on top of Adrian comments


https://reviews.llvm.org/D31778

Files:
  include/clang/AST/ASTStructuralEquivalence.h
  include/clang/Basic/DiagnosticASTKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/AST/ASTStructuralEquivalence.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseExpr.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/Modules/Inputs/F.framework/Headers/F.h
  test/Modules/Inputs/F.framework/Modules/module.modulemap
  test/Modules/Inputs/F.framework/Modules/module.private.modulemap
  test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
  test/Modules/elaborated-type-specifier-from-hidden-module.m
  test/Modules/redefinition-c-tagtypes.m

Index: test/Modules/redefinition-c-tagtypes.m
===
--- /dev/null
+++ test/Modules/redefinition-c-tagtypes.m
@@ -0,0 +1,48 @@
+// RUN: rm -rf %t.cache
+// RUN: %clang_cc1 -fsyntax-only %s -fmodules -fmodules-cache-path=%t.cache \
+// RUN:   -fimplicit-module-maps -F%S/Inputs -verify
+// RUN: %clang_cc1 -fsyntax-only %s -fmodules -fmodules-cache-path=%t.cache \
+// RUN:   -fimplicit-module-maps -F%S/Inputs -DCHANGE_TAGS -verify
+#include "F/F.h"
+
+#ifndef CHANGE_TAGS
+// expected-no-diagnostics
+#endif
+
+struct NS {
+  int a;
+#ifndef CHANGE_TAGS
+  int b;
+#else
+  int c; // expected-note {{field has name 'c' here}}
+  // expected-error@redefinition-c-tagtypes.m:12 {{type 'struct NS' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:3 {{field has name 'b' here}}
+#endif
+};
+
+enum NSE {
+  FST = 22,
+#ifndef CHANGE_TAGS
+  SND = 43,
+#else
+  SND = 44, // expected-note {{enumerator 'SND' with value 44 here}}
+  // expected-error@redefinition-c-tagtypes.m:23 {{type 'enum NSE' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:8 {{enumerator 'SND' with value 43 here}}
+#endif
+  TRD = 55
+};
+
+#define NS_ENUM(_type, _name) \
+  enum _name : _type _name;   \
+  enum _name : _type
+
+typedef NS_ENUM(int, NSMyEnum) {
+  MinX = 11,
+#ifndef CHANGE_TAGS
+  MinXOther = MinX,
+#else
+  MinXOther = TRD, // expected-note {{enumerator 'MinXOther' with value 55 here}}
+  // expected-error@redefinition-c-tagtypes.m:39 {{type 'enum NSMyEnum' has incompatible definitions}}
+  // expected-note@Inputs/F.framework/PrivateHeaders/NS.h:18 {{enumerator 'MinXOther' with value 11 here}}
+#endif
+};
Index: test/Modules/elaborated-type-specifier-from-hidden-module.m
===
--- test/Modules/elaborated-type-specifier-from-hidden-module.m
+++ test/Modules/elaborated-type-specifier-from-hidden-module.m
@@ -4,12 +4,11 @@
 @import ElaboratedTypeStructs.Empty; // The structs are now hidden.
 struct S1 *x;
 struct S2 *y;
-// FIXME: compatible definition should not be an error.
-struct S2 { int x; }; // expected-error {{redefinition}}
+struct S2 { int x; };
 struct S3 *z;
 // Incompatible definition.
-struct S3 { float y; }; // expected-error {{redefinition}}
-// expected-note@elaborated-type-structs.h:* 2 {{previous definition is here}}
+struct S3 { float y; }; // expected-error {{has incompatible definitions}} // expected-note {{field has name}}
+// expected-note@Inputs/elaborated-type-structs.h:3 {{field has name}}
 
 @import ElaboratedTypeStructs.Structs;
 
Index: test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/PrivateHeaders/NS.h
@@ -0,0 +1,19 @@
+struct NS {
+  int a;
+  int b;
+};
+
+enum NSE {
+  FST = 22,
+  SND = 43,
+  TRD = 55
+};
+
+#define NS_ENUM(_type, _name) \
+  enum _name : _type _name;   \
+  enum _name : _type
+
+typedef NS_ENUM(int, NSMyEnum) {
+  MinX = 11,
+  MinXOther = MinX,
+};
Index: test/Modules/Inputs/F.framework/Modules/module.private.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/Modules/module.private.modulemap
@@ -0,0 +1,7 @@
+module F.Private [system] {
+  explicit module NS {
+  header "NS.h"
+  export *
+  }
+  export *
+}
Index: test/Modules/Inputs/F.framework/Modules/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/Modules/module.modulemap
@@ -0,0 +1,7 @@
+framework module F [extern_c] [system] {
+  umbrella header "F.h"
+  module * {
+  export *
+  }
+  export *
+}
Index: test/Modules/Inputs/F.framework/Headers/F.h
===
--- /dev/null
+++ test/Modules/Inputs/F.framework/Headers/F.h
@@ -0,0 +1 @@
+// F.h
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ 

[PATCH] D27546: [ASTReader] Sort RawComments before merging

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

> Does this cause us to deserialize the SLocEntry for every FileID referenced 
> by a RawComment? That would seem pretty bad.

I don't recall because It's been a while, but I'm gonna take a look and get 
back to you. Thanks


https://reviews.llvm.org/D27546



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

[PATCH] D31778: [Modules] Implement ODR-like semantics for tag types in C/ObjC

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping!


https://reviews.llvm.org/D31778



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

[PATCH] D31269: [Modules] Allow modules specified by -fmodule-map-file to shadow implicitly found ones

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping!


https://reviews.llvm.org/D31269



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

[PATCH] D28832: Improve redefinition errors pointing to the same header.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Ping


https://reviews.llvm.org/D28832



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

[PATCH] D34588: Check for _MSC_VER before define _LIBCPP_MSVCRT

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



> Thinking more about this, on Windows, is there a strong reason to default to 
> a different libc by default on Windows?

I'm mostly concerned with `-fms-extensions` + darwin, which doesn't apply to 
this scenario, maybe someone else knows a better answer here.

> @bruno would reusing `-ffreestanding` work for you here?

I think forcing the use of `-ffreestanding` with Apple platforms just for this 
is too restrictive.

> Or is there something else that we can identify about the target environment 
> that can indicate that the MS CRT is unavailable?  I think that what is weird 
> to me about this is that this is not about compatibility with Visual Studio 
> but about the underlying libc.  It feels like it would be similar in spirit 
> to say that libc++ defaults to libSystem as the underlying libc on Linux.

Right, makes total sense. I'm assuming that `-fms-extensions` for other targets 
(Linux) will also rely in not using `_LIBCPP_MSVCRT`, however #ifdefing for 
other platforms here doens't seem to add much because they usually do not set 
`_MSC_VER` anyways. Additional ideas?


https://reviews.llvm.org/D34588



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

[PATCH] D34985: Do not read the file to determine its name.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

@v.g.vassilev will this also benefit from `ContentCache::getBuffer` 
improvements from https://reviews.llvm.org/D33275? I guess if `getBuffer` 
transparently handles pointing to right buffer we won't need this change?


Repository:
  rL LLVM

https://reviews.llvm.org/D34985



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

[PATCH] D33275: Keep into account if files were virtual.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

We're hitting a very similar problem to this one, which I think this patch will 
likely fix. I can't come up with a reproducible testcase either. If you can 
write one better yet, but I'm ok with this as is. LGTM.


https://reviews.llvm.org/D33275



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

[PATCH] D33275: Keep into account if files were virtual.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

That sounds even better. Thanks @v.g.vassilev


https://reviews.llvm.org/D33275



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

[PATCH] D34588: Check for _MSC_VER before define _LIBCPP_MSVCRT

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL308225: Check for _MSC_VER before defining _LIBCPP_MSVCRT 
(authored by bruno).

Changed prior to commit:
  https://reviews.llvm.org/D34588?vs=103975=106960#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D34588

Files:
  libcxx/trunk/include/__config


Index: libcxx/trunk/include/__config
===
--- libcxx/trunk/include/__config
+++ libcxx/trunk/include/__config
@@ -229,8 +229,9 @@
 #  define _LIBCPP_SHORT_WCHAR   1
 // Both MinGW and native MSVC provide a "MSVC"-like enviroment
 #  define _LIBCPP_MSVCRT_LIKE
-// If mingw not explicitly detected, assume using MS C runtime only.
-#  ifndef __MINGW32__
+// If mingw not explicitly detected, assume using MS C runtime only if
+// a MS compatibility version is specified.
+#  if defined(_MSC_VER) && !defined(__MINGW32__)
 #define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library
 #  endif
 #  if (defined(_M_AMD64) || defined(__x86_64__)) || (defined(_M_ARM) || 
defined(__arm__))


Index: libcxx/trunk/include/__config
===
--- libcxx/trunk/include/__config
+++ libcxx/trunk/include/__config
@@ -229,8 +229,9 @@
 #  define _LIBCPP_SHORT_WCHAR   1
 // Both MinGW and native MSVC provide a "MSVC"-like enviroment
 #  define _LIBCPP_MSVCRT_LIKE
-// If mingw not explicitly detected, assume using MS C runtime only.
-#  ifndef __MINGW32__
+// If mingw not explicitly detected, assume using MS C runtime only if
+// a MS compatibility version is specified.
+#  if defined(_MSC_VER) && !defined(__MINGW32__)
 #define _LIBCPP_MSVCRT // Using Microsoft's C Runtime library
 #  endif
 #  if (defined(_M_AMD64) || defined(__x86_64__)) || (defined(_M_ARM) || defined(__arm__))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34588: Check for _MSC_VER before define _LIBCPP_MSVCRT

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



> Could something like _WIN32 && _MSC_VER make this better?  That would allow 
> us to differentiate between the various environments.  It's kinda icky, but, 
> does make sense in a round about way.

The check in the patch was already under a `#if defined(_WIN32)`, having to 
repeat that again seemed not worth the effort!

> Another bad idea: -U_MSC_VER.  The undefine should come after the 
> preprocessor is initialized and should have the same effect as not having it 
> defined.

That's a bit non canonical :-)

In https://reviews.llvm.org/D34588#808076, @rnk wrote:

> > Some non-windows targets using MS extensions define _WIN32 for 
> > compatibility with Windows but do not have MSVC compatibility.
>
> So, these hypothetical targets have the Win32 API available, but they do not 
> use the MSVC CRT, correct?


Right.

Thanks for the review! Committed r308225.


Repository:
  rL LLVM

https://reviews.llvm.org/D34588



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

[PATCH] D34878: [ARM] Option for reading thread pointer from coprocessor register

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

If there's a precedence for a flag name there, it's nice to be compatible here. 
You don't necessarily need to use the same name as the backend.


https://reviews.llvm.org/D34878



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

[PATCH] D34030: Fix the postorder visting of the ClassTemplateSpecializationDecl nodes in the RecursiveASTVisitor.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

In https://reviews.llvm.org/D34030#819699, @MontyKutyi wrote:

> As I can see the `clang/test` contains a lot of different simple tests, but 
> for testing this I think it is not enough to run the clang with some 
> arguments on a specific input.  So I should create a new executable which 
> uses the postorder mode of the RecursiveASTVisitor.  Am I right or is there 
> another preferred way for doing this?


The place you're looking for here is in unittests. For example, see 
unittests/AST/PostOrderASTVisitor.cpp.


https://reviews.llvm.org/D34030



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