[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael marked 3 inline comments as done.
mikael added inline comments.



Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }

rjmccall wrote:
> The indentation here is messed up.
> 
> You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few 
> places.  That's currently correct, in the sense that the fast qualifiers are 
> currently defined to be the CVR qualifiers, but the point of the distinction 
> is that we might want to change that (and we have in fact explored that in 
> the past).  I think you should make `FunctionTypeBits` only claim to store 
> the fast qualifiers, which mostly just means updating a few field / accessor 
> names and changing things like the `getCVRQualifiers()` call right above this 
> to be `getFastQualifiers()`.
> 
> If there are places where it's convenient to assume that the CVR qualifiers 
> are exactly the fast qualifiers, it's okay to static_assert that, but you 
> should make sure to static_assert it in each place you assume it.
I'll change it, but I do have a question related to this:

Can we make any assumptions with the "fast qualifiers"? I'd like it if we can 
assume that they fit in 3 bits. Otherwise I think I also need an assertion here.



Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+  getOrCreateInstanceMethodType(
+  CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+  FPT, U),

rjmccall wrote:
> Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?
Your initial comment suggested to send in a record rather than a type. But I 
see now that it may make more sense to use the type directly instead. Will 
update it.



Comment at: lib/CodeGen/CGDeclCXX.cpp:32
+   (D.hasLocalStorage() && 
CGF.getContext().getLangOpts().OpenCLCPlusPlus)) &&
+  "VarDecl must have global or local (in the case of OpenCL) storage!");
   assert(!D.getType()->isReferenceType() &&

rjmccall wrote:
> What causes OpenCL C++ declarations to go down this path?
It goes there when we defined an object  in __local address space.  From the 
spec "The constructors of a objects in local memory should only be initialized 
once".  This is the same case as with C++ 11 static local variables.


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

https://reviews.llvm.org/D54862



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Personally I'm against this type of warning as it's likely anyone using 
`-mllvm` is actually intending to adjust certain behaviors across one or more 
passes with a lot of switches supported by it being intentionally hidden from 
` --help` output requiring explicitly specifying that hidden flags 
be shown. One real use case being a dozen of patches to my downstream 
LLVM/Clang fork, for example *very* experimental support for SEH on Itanium 
ABI. I feel like this is going to affect developers more than users as now 
additional flags will have to be passed to suppress the warnings generated from 
using flags to debug/diagnose passes by code authors themselves. I feel like 
using `-mllvm` already implies explicit understanding of that, and of the 
`cl::opt` semantics/purpose as well as the flags being generally out of public 
view unless one gets them from full help (which explicitly shows all registered 
flags, hidden or not), or from source code which is most likely to be the 
developers themselves.

For example, I routinely use the following with SEH (excuse some of the naming, 
this is just a downstream fork however):

  -mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm 
-wtfabi-opts=0x1EF77F

I like the ability to pass those via the driver seamlessly, other options being 
explicitly constructing a `clang -cc1` call myself, which is very verbose and 
the driver helps with that a huge amount or adding `#if 0` around them 
downstream (better than commenting out since it's unlikely to cause merge 
conflicts).

I'm mostly indifferent towards `-Xclang` however (since I very rarely used it, 
I think `-Xclang -fexternc-nounwind` is the only time I used it, so I don't 
really have a strong opinion regarding that diagnostic, I still think it's not 
a good change. (speaking of, I should probably make a diff to expose that to 
the frontend via driver as it was seemingly missed in compiler invocation 
argument building, from its `-f` prefix I'm guessing that was accidental but I 
haven't looked into it, which I definitely should)).

If I may suggest another option, is it possible to add a "maintainer mode" flag 
to the build system (ie. with CMake, `-DLLVM_MAINTAINER_MODE=ON`, and a similar 
style thing with GN) and guard the diagnostic emission with `#ifndef 
LLVM_MAINTAINER_MODE`. This would easily allow developers to experiement with 
LLVM downstream without needing explicit workarounds for supressing those 
warnings. I would be happy to write a patch for CMake based builds myself (not 
GN unfortunately, slightly rusty on it) if you feel that is a better 
compromise. This means that downstream developers, whether intending to 
upstream such changes or not, can pass this through and not worry about those 
warnings since this is an explicit "here be dragons" opt-in, as that is easy to 
add to a build system (this also ties in with the previous discussion of adding 
an unsupressable warning for a certain -Xclang flag with the intent of getting 
users to avoid it in releases and yet allow performance data collection, but to 
developers that is more of an annoyance). I feel like this would be the 
ultimate consent to "yes I really want this, I'm aware of what it does" and 
would also require full rebuilds to enable, which is easy if you're developing 
a pass, for example, since you would be rebuilding anyway (assuming in good 
faith that this is only enabled for development builds, by downstream forks or 
build system configurations and releases never have it set). In case of CMake a 
warning may be a good idea as well when that flag is enabled as well as clear 
updates to documentation to reflect the purpose of it.

Anyway that's my opinion/concern on the matter, I don't know if others share it 
or not and I'm not sure if there are glaring problems with the idea of a 
solution I proposed, but I think it's better than some downstream vendors 
excluding this patch altogether and various other (inconsistent) ways 
developers will get around it.

Thanks.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[Romanov Vlad via Phabricator via cfe-commits]

romanovvlad added a comment.

I'm getting the same IR for both examples you provided. And for both load of X 
value is aligned on 4 bytes wo patch and on 1 byte with.


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

https://reviews.llvm.org/D55262



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

[clang-tools-extra] r348467 - [clangd] Fix a typo in TUSchedulerTests

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Dec  6 00:55:24 2018
New Revision: 348467

URL: http://llvm.org/viewvc/llvm-project?rev=348467&view=rev
Log:
[clangd] Fix a typo in TUSchedulerTests

Reviewers: ilya-biryukov

Reviewed By: ilya-biryukov

Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, kadircet, 
cfe-commits

Differential Revision: https://reviews.llvm.org/D55312

Modified:
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=348467&r1=348466&r2=348467&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Dec  6 
00:55:24 2018
@@ -593,7 +593,7 @@ TEST_F(TUSchedulerTests, NoopOnEmptyChan
   ASSERT_FALSE(DoUpdate(SourceContents));
 
   // Update to a header should cause a rebuild, though.
-  Files[Header] = time_t(1);
+  Timestamps[Header] = time_t(1);
   ASSERT_TRUE(DoUpdate(SourceContents));
   ASSERT_FALSE(DoUpdate(SourceContents));
 


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

[PATCH] D55312: [clangd] Fix a typo in TUSchedulerTests

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348467: [clangd] Fix a typo in TUSchedulerTests (authored by 
hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55312

Files:
  clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp


Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -593,7 +593,7 @@
   ASSERT_FALSE(DoUpdate(SourceContents));
 
   // Update to a header should cause a rebuild, though.
-  Files[Header] = time_t(1);
+  Timestamps[Header] = time_t(1);
   ASSERT_TRUE(DoUpdate(SourceContents));
   ASSERT_FALSE(DoUpdate(SourceContents));
 


Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -593,7 +593,7 @@
   ASSERT_FALSE(DoUpdate(SourceContents));
 
   // Update to a header should cause a rebuild, though.
-  Files[Header] = time_t(1);
+  Timestamps[Header] = time_t(1);
   ASSERT_TRUE(DoUpdate(SourceContents));
   ASSERT_FALSE(DoUpdate(SourceContents));
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348469 - Make test resistant to line numbers changing

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Thu Dec  6 01:22:12 2018
New Revision: 348469

URL: http://llvm.org/viewvc/llvm-project?rev=348469&view=rev
Log:
Make test resistant to line numbers changing

Modified:
cfe/trunk/test/AST/dump.cpp

Modified: cfe/trunk/test/AST/dump.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/dump.cpp?rev=348469&r1=348468&r2=348469&view=diff
==
--- cfe/trunk/test/AST/dump.cpp (original)
+++ cfe/trunk/test/AST/dump.cpp Thu Dec  6 01:22:12 2018
@@ -43,7 +43,7 @@ struct S {
   }
 };
 
-// CHECK:  | `-OMPParallelForDirective {{.+}} {{|}}
+// CHECK:  | `-OMPParallelForDirective {{.+}} {{|}}
 // CHECK-NEXT: |   |-OMPDefaultClause {{.+}} 
 // CHECK-NEXT: |   |-OMPPrivateClause {{.+}} 
 // CHECK-NEXT: |   | `-DeclRefExpr {{.+}}  'int' lvalue 
OMPCapturedExpr {{.+}} 'a' 'int &'
@@ -53,19 +53,19 @@ struct S {
 // CHECK-NEXT: |   |-OMPScheduleClause {{.+}} 
 // CHECK-NEXT: |   | `-ImplicitCastExpr {{.+}}  'int' 

 // CHECK-NEXT: |   |   `-DeclRefExpr {{.+}}  'int' lvalue 
OMPCapturedExpr {{.+}} '.capture_expr.' 'int'
-// CHECK-NEXT: |   `-CapturedStmt {{.+}} 
+// CHECK-NEXT: |   `-CapturedStmt {{.+}} 
 // CHECK-NEXT: | |-CapturedDecl {{.+}} <> 
-// CHECK-NEXT: | | |-ForStmt {{.+}} 
-// CHECK:  | | | `-UnaryOperator {{.+}}  'int' 
lvalue prefix '++'
+// CHECK-NEXT: | | |-ForStmt {{.+}} 
+// CHECK:  | | | `-UnaryOperator {{.+}}  
'int' lvalue prefix '++'
 // CHECK-NEXT: | | |   `-DeclRefExpr {{.+}}  'int' lvalue 
OMPCapturedExpr {{.+}} 'a' 'int &'
 
 #pragma omp declare simd
 #pragma omp declare simd inbranch
 void foo();
 
-// CHECK:|-FunctionDecl {{.+}}  col:6 foo 'void ()'
-// CHECK-NEXT:   |-OMPDeclareSimdDeclAttr {{.+}}  Implicit 
BS_Inbranch
-// CHECK:`-OMPDeclareSimdDeclAttr {{.+}}  Implicit 
BS_Undefined
+// CHECK:|-FunctionDecl {{.+}}  col:6 foo 'void 
()'
+// CHECK-NEXT:   |-OMPDeclareSimdDeclAttr {{.+}}  
Implicit BS_Inbranch
+// CHECK:`-OMPDeclareSimdDeclAttr {{.+}}  
Implicit BS_Undefined
 
 #pragma omp declare target
 int bar() {
@@ -74,11 +74,11 @@ int bar() {
 }
 #pragma omp end declare target
 
-// CHECK:   `-FunctionDecl {{.+}}  line:71:5 bar 
'int ()'
-// CHECK-NEXT:  |-CompoundStmt {{.+}} 
-// CHECK-NEXT:  | |-DeclStmt {{.+}} 
+// CHECK:   `-FunctionDecl {{.+}}  
line:{{.+}}:5 bar 'int ()'
+// CHECK-NEXT:  |-CompoundStmt {{.+}} 
+// CHECK-NEXT:  | |-DeclStmt {{.+}} 
 // CHECK-NEXT:  | | `-VarDecl {{.+}}  col:7 used f 'int'
-// CHECK-NEXT:  | `-ReturnStmt {{.+}} 
+// CHECK-NEXT:  | `-ReturnStmt {{.+}} 
 // CHECK-NEXT:  |   `-ImplicitCastExpr {{.+}}  'int' 
 // CHECK-NEXT:  | `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'f' 'int'
 // CHECK-NEXT:  `-OMPDeclareTargetDeclAttr {{.+}} <> Implicit 
MT_To


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

[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 176936.
hokein marked 8 inline comments as done.
hokein added a comment.

Address comments.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -7,12 +7,13 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "Context.h"
 #include "Matchers.h"
 #include "TUScheduler.h"
 #include "TestFS.h"
-#include "llvm/ADT/ScopeExit.h"
 #include "gmock/gmock.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -23,6 +24,7 @@
 namespace {
 
 using ::testing::_;
+using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -30,6 +32,10 @@
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
 
+MATCHER_P2(TUState, State, ActionName, "") {
+  return arg.Action.S == State && arg.Action.Name == ActionName;
+}
+
 class TUSchedulerTests : public ::testing::Test {
 protected:
   ParseInputs getInputs(PathRef File, std::string Contents) {
@@ -658,6 +664,78 @@
   EXPECT_EQ(Counter.load(), 3);
 }
 
+TEST_F(TUSchedulerTests, TUStatus) {
+  class CaptureTUStatus : public DiagnosticsConsumer {
+  public:
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {}
+
+void onFileUpdated(PathRef File, const TUStatus &Status) override {
+  std::lock_guard Lock(Mutex);
+  AllStatus.push_back(Status);
+}
+
+std::vector AllStatus;
+
+  private:
+std::mutex Mutex;
+  } CaptureTUStatus;
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
+  Annotations Code("int m^ain () {}");
+
+  // We schedule the following tasks in the queue:
+  //   [Update] [GoToDefinition]
+  Server.addDocument(testPath("foo.cpp"), Code.code(), WantDiagnostics::Yes);
+  Server.findDefinitions(testPath("foo.cpp"), Code.point(),
+ [](Expected> Result) {
+   ASSERT_TRUE((bool)Result);
+ });
+
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  EXPECT_THAT(CaptureTUStatus.AllStatus,
+  ElementsAre(
+  // Statuses of "Update" action.
+  TUState(TUAction::Queued, "Update"),
+  TUState(TUAction::RunningAction, "Update"),
+  TUState(TUAction::BuildingPreamble, "Update"),
+  TUState(TUAction::BuildingFile, "Update"),
+
+  // Statuses of "Definitions" action
+  TUState(TUAction::Queued, "Definitions"),
+  TUState(TUAction::RunningAction, "Definitions"),
+  TUState(TUAction::Idle, /*No action*/ "")));
+}
+
+TEST_F(TUSchedulerTests, NoTUStatusEmittedForRemovedFile) {
+  class CaptureTUStatus : public DiagnosticsConsumer {
+  public:
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {}
+
+void onFileUpdated(PathRef File, const TUStatus &Status) override {
+  // Queued is emitted by the main thread, we don't block it.
+  if (Status.Action.S == TUAction::Queued)
+return;
+  // Block the worker thread until the document is removed.
+  ASSERT_TRUE(Status.Action.S == TUAction::RunningAction);
+  Removed.wait();
+}
+Notification Removed;
+  } CaptureTUStatus;
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
+
+  Server.addDocument(testPath("foo.cpp"), "int main() {}",
+ WantDiagnostics::Yes);
+  Server.removeDocument(testPath("foo.cpp"));
+  CaptureTUStatus.Removed.notify();
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for finishing";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -52,6 +52,36 @@
   unsigned MaxRetainedASTs = 3;
 };
 
+struct TUAction {
+  enum State {
+Queued,   // The TU is pending in the thread task queue to be built.
+RunningAction,// Starting running actions on the TU.
+BuildingPreamble, // The preamble of the TU is being built.
+BuildingFile, // The TU is being built. It is only emitted when building
+  // the AST for diagnostics in write action (update).
+Idle, // Indicates the worker thread is idle, and ready to run any upcoming
+   

r348471 - Add test for ObjC generics

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Thu Dec  6 01:23:59 2018
New Revision: 348471

URL: http://llvm.org/viewvc/llvm-project?rev=348471&view=rev
Log:
Add test for ObjC generics

Modified:
cfe/trunk/test/AST/ast-dump-decl.m

Modified: cfe/trunk/test/AST/ast-dump-decl.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-decl.m?rev=348471&r1=348470&r2=348471&view=diff
==
--- cfe/trunk/test/AST/ast-dump-decl.m (original)
+++ cfe/trunk/test/AST/ast-dump-decl.m Thu Dec  6 01:23:59 2018
@@ -81,6 +81,14 @@
 // CHECK-NEXT:   ObjCProtocol{{.*}} 'P'
 // CHECK-NEXT:   ObjCMethodDecl{{.*}} bar
 
+@interface TestGenericInterface : A {
+}
+@end
+// CHECK:  ObjCInterfaceDecl{{.*}} TestGenericInterface
+// CHECK-NEXT:   -ObjCTypeParamDecl
+// CHECK-NEXT:   -super ObjCInterface
+// CHECK-NEXT:   -ObjCProtocol
+
 @implementation TestObjCClass (TestObjCCategoryDecl)
 - (void) bar {
 }


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

r348470 - Extend OMP test

[Stephen Kelly via cfe-commits]

Author: steveire
Date: Thu Dec  6 01:23:53 2018
New Revision: 348470

URL: http://llvm.org/viewvc/llvm-project?rev=348470&view=rev
Log:
Extend OMP test

Modified:
cfe/trunk/test/AST/dump.cpp

Modified: cfe/trunk/test/AST/dump.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/dump.cpp?rev=348470&r1=348469&r2=348470&view=diff
==
--- cfe/trunk/test/AST/dump.cpp (original)
+++ cfe/trunk/test/AST/dump.cpp Thu Dec  6 01:23:53 2018
@@ -33,6 +33,13 @@ int ga, gb;
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
+// CHECK-NEXT: | |-BinaryOperator {{.+}}  'float' lvalue '='
+// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_priv' 'float'
+// CHECK-NEXT: | | `-BinaryOperator {{.+}}  'float' '+'
+// CHECK-NEXT: | |   |-ImplicitCastExpr {{.+}}  'float' 

+// CHECK-NEXT: | |   | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_orig' 'float'
+// CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'float' 

+// CHECK-NEXT: | | `-IntegerLiteral {{.+}}  'int' 15
 
 struct S {
   int a, b;


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

[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/TUScheduler.cpp:268
+  /// Status of the TU.
+  TUStatus Status; /* GUARDED_BY(DiagMu) */
 };

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > hokein wrote:
> > > > ilya-biryukov wrote:
> > > > > Is `Status` actually ever read from multiple threads?
> > > > > I assume it's only accessed on the worker thread, right? I think we 
> > > > > can leave out the locks around it and put beside other fields only 
> > > > > accessed by the worker thread, i.e. `DiagsWereReported`, etc.
> > > > > 
> > > > > PS Sorry go going back and forth, I didn't think about it in the 
> > > > > previous review iteration.
> > > > Unfortunately not, most statuses are emitted via worker thread, but we 
> > > > emit `Queued` status in the main thread...
> > > Hm, that sounds surprising.
> > > What if the rebuild is in progress on the worker thread and we emit the 
> > > queued status on the main thread at the same time? We might get weird 
> > > sequences of statuses, right? 
> > > E.g. `Queued → BuildingPreamble → [new update on the main thread] Queued 
> > > → [processing request on a worker thread] RunningAction('XRefs')`
> > > 
> > > The `Queued` request status between `BuildingPreamble` and 
> > > `RunningAction` looks **really** weird.
> > > Maybe we could avoid that by setting emitting the `Queued` status on the 
> > > worker thread whenever we're about to sleep on the debounce timeout?
> > This sounds fair enough, and can simplify the code.
> > 
> > > Maybe we could avoid that by setting emitting the Queued status on the 
> > > worker thread whenever we're about to sleep on the debounce timeout?
> > 
> > The `Queued` state is originally designed for the state that the worker 
> > thread is blocked by the max-concurrent-threads semaphore.
> > Emitting it when we're about to sleep on the debounce timeout doesn't seem 
> > a right place to me. I think a reasonable place is before requiring the 
> > `Barrier`.
> > 
> Ah, good point, waiting on a barrier can definitely take some time and is a 
> form of queuing that we might want to notify the clients about.
> It would be nice to notify only when the Barrier couldn't be acquired right 
> away though, this probably be achieved adding `try_lock()` on the semaphore 
> and only emitting a `Queued` notification if it fails. That way we'll avoid 
> spamming the clients with non-useful statuses.
> 
> Wrt to deciding on whether we should notify when waiting on a debounce timer 
> or on acquiring a semaphore... Why not both?
Sounds good.



Comment at: clangd/TUScheduler.cpp:422
+Details.ReuseAST = true;
+emitTUStatus({TUAction::BuildingFile, TaskName}, &Details);
 return;

ilya-biryukov wrote:
> Should we also emit this status if we reuse the AST, but still report the 
> diagnostics?
Yes, I missed that. And I think we also should emit a status if `buildAST` 
fails.



Comment at: clangd/TUScheduler.cpp:626
 {
+  emitTUStatus({TUAction::Queued, Req.Name});
   std::lock_guard BarrierLock(Barrier);

ilya-biryukov wrote:
> See the other suggestion about emitting this status only when locking the 
> barrier failed.
> This might be a good fit for a different patch, though, maybe add a FIXME 
> here?
Agree, added a fixme, will do it in a follow-up patch.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796



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

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

[John Paul Adrian Glaubitz via Phabricator via cfe-commits]

glaubitz added a comment.

I have applied this patch to the llvm-toolchain-7 package in Debian and did not 
see any regressions on x86_64 or 32-Bit PowerPC. Additionally, I have included 
the patches from https://reviews.llvm.org/D54409 and 
https://reviews.llvm.org/D54583 saw no regressions on x86_64 and 32-bit PowerPC.

All three patches will be part of the next upload of the llvm-toolchain-7 
package in Debian unstable which will be version 1:7.0.1~+rc2-9.


Repository:
  rC Clang

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

https://reviews.llvm.org/D49754



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

Re: r348471 - Add test for ObjC generics

[Roman Lebedev via cfe-commits]

Can you please write better commit messages, especially the first subject line?
E.g. this should have been "[AST][NFC] Add dump test for ObjC generics"

On Thu, Dec 6, 2018 at 12:26 PM Stephen Kelly via cfe-commits
 wrote:
>
> Author: steveire
> Date: Thu Dec  6 01:23:59 2018
> New Revision: 348471
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348471&view=rev
> Log:
> Add test for ObjC generics
>
> Modified:
> cfe/trunk/test/AST/ast-dump-decl.m
>
> Modified: cfe/trunk/test/AST/ast-dump-decl.m
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-decl.m?rev=348471&r1=348470&r2=348471&view=diff
> ==
> --- cfe/trunk/test/AST/ast-dump-decl.m (original)
> +++ cfe/trunk/test/AST/ast-dump-decl.m Thu Dec  6 01:23:59 2018
> @@ -81,6 +81,14 @@
>  // CHECK-NEXT:   ObjCProtocol{{.*}} 'P'
>  // CHECK-NEXT:   ObjCMethodDecl{{.*}} bar
>
> +@interface TestGenericInterface : A {
> +}
> +@end
> +// CHECK:  ObjCInterfaceDecl{{.*}} TestGenericInterface
> +// CHECK-NEXT:   -ObjCTypeParamDecl
> +// CHECK-NEXT:   -super ObjCInterface
> +// CHECK-NEXT:   -ObjCProtocol
> +
>  @implementation TestObjCClass (TestObjCCategoryDecl)
>  - (void) bar {
>  }
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348473 - Diagnose friend function template redefinitions.

[Serge Pavlov via cfe-commits]

Author: sepavloff
Date: Thu Dec  6 01:35:04 2018
New Revision: 348473

URL: http://llvm.org/viewvc/llvm-project?rev=348473&view=rev
Log:
Diagnose friend function template redefinitions.

Friend function template defined in a class template becomes available if
the enclosing class template is instantiated. Until the function template
is used, it does not have a body, but still is considered a definition for
the purpose of redeclaration checks.

This change modifies redefinition check so that it can find the friend
function template definitions in instantiated classes.

Differential Revision: http://reviews.llvm.org/D21508

Modified:
cfe/trunk/include/clang/AST/DeclBase.h
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
cfe/trunk/test/Modules/friend-definition.cpp
cfe/trunk/test/SemaCXX/friend-template-redecl.cpp
cfe/trunk/test/SemaCXX/friend2.cpp

Modified: cfe/trunk/include/clang/AST/DeclBase.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=348473&r1=348472&r2=348473&view=diff
==
--- cfe/trunk/include/clang/AST/DeclBase.h (original)
+++ cfe/trunk/include/clang/AST/DeclBase.h Thu Dec  6 01:35:04 2018
@@ -1065,11 +1065,11 @@ public:
 unsigned OldNS = IdentifierNamespace;
 assert((OldNS & (IDNS_Tag | IDNS_Ordinary |
  IDNS_TagFriend | IDNS_OrdinaryFriend |
- IDNS_LocalExtern)) &&
+ IDNS_LocalExtern | IDNS_NonMemberOperator)) &&
"namespace includes neither ordinary nor tag");
 assert(!(OldNS & ~(IDNS_Tag | IDNS_Ordinary | IDNS_Type |
IDNS_TagFriend | IDNS_OrdinaryFriend |
-   IDNS_LocalExtern)) &&
+   IDNS_LocalExtern | IDNS_NonMemberOperator)) &&
"namespace includes other than ordinary or tag");
 
 Decl *Prev = getPreviousDecl();
@@ -1082,7 +1082,8 @@ public:
 IdentifierNamespace |= IDNS_Tag | IDNS_Type;
 }
 
-if (OldNS & (IDNS_Ordinary | IDNS_OrdinaryFriend | IDNS_LocalExtern)) {
+if (OldNS & (IDNS_Ordinary | IDNS_OrdinaryFriend |
+ IDNS_LocalExtern | IDNS_NonMemberOperator)) {
   IdentifierNamespace |= IDNS_OrdinaryFriend;
   if (PerformFriendInjection ||
   (Prev && Prev->getIdentifierNamespace() & IDNS_Ordinary))

Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=348473&r1=348472&r2=348473&view=diff
==
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu Dec  6 01:35:04 2018
@@ -12737,6 +12737,29 @@ Sema::CheckForFunctionRedefinition(Funct
   }
 }
   }
+
+  if (!Definition)
+// Similar to friend functions a friend function template may be a
+// definition and do not have a body if it is instantiated in a class
+// template.
+if (FunctionTemplateDecl *FTD = FD->getDescribedFunctionTemplate()) {
+  for (auto I : FTD->redecls()) {
+auto D = cast(I);
+if (D != FTD) {
+  assert(!D->isThisDeclarationADefinition() &&
+ "More than one definition in redeclaration chain");
+  if (D->getFriendObjectKind() != Decl::FOK_None)
+if (FunctionTemplateDecl *FT =
+   D->getInstantiatedFromMemberTemplate()) 
{
+  if (FT->isThisDeclarationADefinition()) {
+Definition = D->getTemplatedDecl();
+break;
+  }
+}
+}
+  }
+}
+
   if (!Definition)
 return;
 

Modified: cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp?rev=348473&r1=348472&r2=348473&view=diff
==
--- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp Thu Dec  6 01:35:04 2018
@@ -1817,7 +1817,9 @@ Decl *TemplateDeclInstantiator::VisitFun
   // If the original function was part of a friend declaration,
   // inherit its namespace state and add it to the owner.
   if (isFriend) {
-PrincipalDecl->setObjectOfFriendDecl();
+Function->setObjectOfFriendDecl();
+if (FunctionTemplateDecl *FT = Function->getDescribedFunctionTemplate())
+  FT->setObjectOfFriendDecl();
 DC->makeDeclVisibleInContext(PrincipalDecl);
 
 bool QueuedInstantiation = false;

Modified: cfe/trunk/test/Modules/friend-definition.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/friend-definition.cpp?rev=348473&r1=348472&r2=348473&view=diff
==
--- cfe/trunk/test/Modules/friend-definition.cpp (orig

[PATCH] D21508: Diagnose friend function template redefinitions

[Serge Pavlov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348473: Diagnose friend function template redefinitions. 
(authored by sepavloff, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D21508?vs=176763&id=176938#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D21508

Files:
  cfe/trunk/include/clang/AST/DeclBase.h
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
  cfe/trunk/test/Modules/friend-definition.cpp
  cfe/trunk/test/SemaCXX/friend-template-redecl.cpp
  cfe/trunk/test/SemaCXX/friend2.cpp

Index: cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -1817,7 +1817,9 @@
   // If the original function was part of a friend declaration,
   // inherit its namespace state and add it to the owner.
   if (isFriend) {
-PrincipalDecl->setObjectOfFriendDecl();
+Function->setObjectOfFriendDecl();
+if (FunctionTemplateDecl *FT = Function->getDescribedFunctionTemplate())
+  FT->setObjectOfFriendDecl();
 DC->makeDeclVisibleInContext(PrincipalDecl);
 
 bool QueuedInstantiation = false;
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -12737,6 +12737,29 @@
   }
 }
   }
+
+  if (!Definition)
+// Similar to friend functions a friend function template may be a
+// definition and do not have a body if it is instantiated in a class
+// template.
+if (FunctionTemplateDecl *FTD = FD->getDescribedFunctionTemplate()) {
+  for (auto I : FTD->redecls()) {
+auto D = cast(I);
+if (D != FTD) {
+  assert(!D->isThisDeclarationADefinition() &&
+ "More than one definition in redeclaration chain");
+  if (D->getFriendObjectKind() != Decl::FOK_None)
+if (FunctionTemplateDecl *FT =
+   D->getInstantiatedFromMemberTemplate()) {
+  if (FT->isThisDeclarationADefinition()) {
+Definition = D->getTemplatedDecl();
+break;
+  }
+}
+}
+  }
+}
+
   if (!Definition)
 return;
 
Index: cfe/trunk/include/clang/AST/DeclBase.h
===
--- cfe/trunk/include/clang/AST/DeclBase.h
+++ cfe/trunk/include/clang/AST/DeclBase.h
@@ -1065,11 +1065,11 @@
 unsigned OldNS = IdentifierNamespace;
 assert((OldNS & (IDNS_Tag | IDNS_Ordinary |
  IDNS_TagFriend | IDNS_OrdinaryFriend |
- IDNS_LocalExtern)) &&
+ IDNS_LocalExtern | IDNS_NonMemberOperator)) &&
"namespace includes neither ordinary nor tag");
 assert(!(OldNS & ~(IDNS_Tag | IDNS_Ordinary | IDNS_Type |
IDNS_TagFriend | IDNS_OrdinaryFriend |
-   IDNS_LocalExtern)) &&
+   IDNS_LocalExtern | IDNS_NonMemberOperator)) &&
"namespace includes other than ordinary or tag");
 
 Decl *Prev = getPreviousDecl();
@@ -1082,7 +1082,8 @@
 IdentifierNamespace |= IDNS_Tag | IDNS_Type;
 }
 
-if (OldNS & (IDNS_Ordinary | IDNS_OrdinaryFriend | IDNS_LocalExtern)) {
+if (OldNS & (IDNS_Ordinary | IDNS_OrdinaryFriend |
+ IDNS_LocalExtern | IDNS_NonMemberOperator)) {
   IdentifierNamespace |= IDNS_OrdinaryFriend;
   if (PerformFriendInjection ||
   (Prev && Prev->getIdentifierNamespace() & IDNS_Ordinary))
Index: cfe/trunk/test/SemaCXX/friend-template-redecl.cpp
===
--- cfe/trunk/test/SemaCXX/friend-template-redecl.cpp
+++ cfe/trunk/test/SemaCXX/friend-template-redecl.cpp
@@ -18,16 +18,3 @@
   foo(x);
   bar(x);
 }
-
-namespace PR39742 {
-template
-struct wrapper {
-  template
-  friend void friend_function_template() {}
-};
-
-wrapper x;
-// FIXME: We should really error here because of the redefinition of
-// friend_function_template.
-wrapper y;
-}
Index: cfe/trunk/test/SemaCXX/friend2.cpp
===
--- cfe/trunk/test/SemaCXX/friend2.cpp
+++ cfe/trunk/test/SemaCXX/friend2.cpp
@@ -129,6 +129,83 @@
 void func_22() {} // expected-error{{redefinition of 'func_22'}}
 
 
+// Case of template friend functions.
+
+template void func_31(T *x);
+template
+struct C31a {
+  template friend void func_31(T *x) {}
+};
+template
+struct C31b {
+  template friend void func_31(T *x) {}
+};
+
+
+template inline void func_32(T *x) {}
+template
+struct C32a {
+  template friend void func_32(T *x) {}
+};
+template
+struct C32b {

[clang-tools-extra] r348475 - [clangd] C++ API for emitting file status.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Dec  6 01:41:04 2018
New Revision: 348475

URL: http://llvm.org/viewvc/llvm-project?rev=348475&view=rev
Log:
[clangd] C++ API for emitting file status.

Introduce clangd C++ API to emit the current status of file.

Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=348475&r1=348474&r2=348475&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Thu Dec  6 01:41:04 2018
@@ -86,6 +86,10 @@ struct UpdateIndexCallbacks : public Par
 DiagConsumer.onDiagnosticsReady(File, std::move(Diags));
   }
 
+  void onFileUpdated(PathRef File, const TUStatus &Status) override {
+DiagConsumer.onFileUpdated(File, Status);
+  }
+
 private:
   FileIndex *FIndex;
   DiagnosticsConsumer &DiagConsumer;
@@ -144,6 +148,10 @@ ClangdServer::ClangdServer(const GlobalC
 
 void ClangdServer::addDocument(PathRef File, StringRef Contents,
WantDiagnostics WantDiags) {
+  // FIXME: some build systems like Bazel will take time to preparing
+  // environment to build the file, it would be nice if we could emit a
+  // "PreparingBuild" status to inform users, it is non-trivial given the
+  // current implementation.
   WorkScheduler.update(File,
ParseInputs{getCompileCommand(File),
FSProvider.getFileSystem(), Contents.str()},

Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=348475&r1=348474&r2=348475&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Thu Dec  6 01:41:04 2018
@@ -37,6 +37,7 @@ class PCHContainerOperations;
 
 namespace clangd {
 
+// FIXME: find a better name.
 class DiagnosticsConsumer {
 public:
   virtual ~DiagnosticsConsumer() = default;
@@ -44,6 +45,8 @@ public:
   /// Called by ClangdServer when \p Diagnostics for \p File are ready.
   virtual void onDiagnosticsReady(PathRef File,
   std::vector Diagnostics) = 0;
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 };
 
 /// Manages a collection of source files and derived data (ASTs, indexes),

Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=348475&r1=348474&r2=348475&view=diff
==
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Thu Dec  6 01:41:04 2018
@@ -205,6 +205,10 @@ private:
   /// Adds a new task to the end of the request queue.
   void startTask(StringRef Name, unique_function Task,
  Optional UpdateType);
+  /// Updates the TUStatus and emits it. Only called in the worker thread.
+  void emitTUStatus(TUAction FAction,
+const TUStatus::BuildDetails *Detail = nullptr);
+
   /// Determines the next action to perform.
   /// All actions that should never run are discarded.
   /// Returns a deadline for the next action. If it's expired, run now.
@@ -234,6 +238,8 @@ private:
   ParsingCallbacks &Callbacks;
   /// Helper class required to build the ASTs.
   const std::shared_ptr PCHs;
+  /// Only accessed by the worker thread.
+  TUStatus Status;
 
   Semaphore &Barrier;
   /// Inputs, corresponding to the current state of AST.
@@ -251,7 +257,9 @@ private:
   bool Done;/* GUARDED_BY(Mutex) */
   std::deque Requests; /* GUARDED_BY(Mutex) */
   mutable std::condition_variable RequestsCV;
-  /// Guards a critical section for running the diagnostics callbacks. 
+  // FIXME: rename it to better fix the current usage, we also use it to guard
+  // emitting TUStatus.
+  /// Guards a critical section for running the diagnostics callbacks.
   std::mutex DiagsMu;
   // Used to prevent remove document + leading to out-of-order diagnostics:
   // The lifetime of the old/new ASTWorkers will overlap, but their handles
@@ -326,8 +334,9 @@ ASTWorker::ASTWorker(PathRef FileName, T
  bool StorePreamblesInMemory, ParsingCallbacks &Callbacks)
 : IdleASTs(LRUCache), RunSync(RunSync), UpdateDebounce(UpdateDebounce),
   FileName(FileName), StorePreambleInMemory(StorePreamblesInMemory

[PATCH] D55358: [ASTImporter] Fix import of NestedNameSpecifierLoc.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, martong, dkrupp.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.

Import type location in case of TypeSpec and TypeSpecWithTemplate.
Without this fix the imported NespedNameSpecifierLoc will have an
invalid begin location.


Repository:
  rC Clang

https://reviews.llvm.org/D55358

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1179,6 +1179,26 @@
   has(fieldDecl(hasType(dependentSizedArrayType(;
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportBeginLocOfDeclRefExpr) {
+  Decl *FromTU = getTuDecl(
+  "class A { public: static int X; }; void f() { (void)A::X; }", Lang_CXX);
+  auto From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(From);
+  ASSERT_TRUE(
+  cast(cast(From->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+  FunctionDecl *To = Import(From, Lang_CXX);
+  ASSERT_TRUE(To);
+  ASSERT_TRUE(
+  cast(cast(To->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+}
+
 TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX);
   auto From =
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8017,7 +8017,8 @@
 case NestedNameSpecifier::TypeSpec:
 case NestedNameSpecifier::TypeSpecWithTemplate: {
   TypeSourceInfo *TSI = getToContext().getTrivialTypeSourceInfo(
-QualType(Spec->getAsType(), 0));
+QualType(Spec->getAsType(), 0),
+Import(NNS.getTypeLoc().getBeginLoc()));
   Builder.Extend(getToContext(),
  Import(NNS.getLocalBeginLoc()),
  TSI->getTypeLoc(),


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1179,6 +1179,26 @@
   has(fieldDecl(hasType(dependentSizedArrayType(;
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportBeginLocOfDeclRefExpr) {
+  Decl *FromTU = getTuDecl(
+  "class A { public: static int X; }; void f() { (void)A::X; }", Lang_CXX);
+  auto From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(From);
+  ASSERT_TRUE(
+  cast(cast(From->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+  FunctionDecl *To = Import(From, Lang_CXX);
+  ASSERT_TRUE(To);
+  ASSERT_TRUE(
+  cast(cast(To->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+}
+
 TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX);
   auto From =
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -8017,7 +8017,8 @@
 case NestedNameSpecifier::TypeSpec:
 case NestedNameSpecifier::TypeSpecWithTemplate: {
   TypeSourceInfo *TSI = getToContext().getTrivialTypeSourceInfo(
-QualType(Spec->getAsType(), 0));
+QualType(Spec->getAsType(), 0),
+Import(NNS.getTypeLoc().getBeginLoc()));
   Builder.Extend(getToContext(),
  Import(NNS.getLocalBeginLoc()),
  TSI->getTypeLoc(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55331: [CodeComplete] Fix assertion failure

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D55331



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

[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Forgot to associate this patch to the actual commit, committed in rL348475 
.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796



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

[clang-tools-extra] r348478 - [clangd] Update the test code

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Dec  6 02:22:48 2018
New Revision: 348478

URL: http://llvm.org/viewvc/llvm-project?rev=348478&view=rev
Log:
[clangd] Update the test code

I forgot to update it in the last round of code review.

Modified:
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=348478&r1=348477&r2=348478&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Dec  6 
02:22:48 2018
@@ -716,11 +716,7 @@ TEST_F(TUSchedulerTests, NoTUStatusEmitt
 std::vector Diagnostics) override {}
 
 void onFileUpdated(PathRef File, const TUStatus &Status) override {
-  // Queued is emitted by the main thread, we don't block it.
-  if (Status.Action.S == TUAction::Queued)
-return;
   // Block the worker thread until the document is removed.
-  ASSERT_TRUE(Status.Action.S == TUAction::RunningAction);
   Removed.wait();
 }
 Notification Removed;


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

[PATCH] D53866: [Preamble] Stop generating preamble for circular #includes

[Nikolai Kosjar via Phabricator via cfe-commits]

nik marked 2 inline comments as done and an inline comment as not done.
nik added inline comments.



Comment at: include/clang/Lex/Preprocessor.h:391
   } PreambleConditionalStack;
+  bool PreambleGenerationFailed = false;
 

ilya-biryukov wrote:
> nik wrote:
> > ilya-biryukov wrote:
> > > There's a mechanism to handle preamble with errors, see 
> > > `PreprocessorOpts::AllowPCHWithCompilerErrors`.
> > > Maybe rely on it and avoid adding a different one?
> > I'm not sure how to make use of AllowPCHWithCompilerErrors. It's clearly 
> > meant as an input/readonly option - do you suggest setting that one to 
> > "false" in case we detect the cyclic include (and later checking for 
> > it...)? That feels a bit hacky. Have you meant something else?
> We emit an error, so the preamble will **not** be emitted. 
> Unless the users specify `AllowPCHWithCompilerErrors`, in which case they've 
> signed up for this anyway.
> 
> I propose to **not** add the new flag at all. Would that work?
As stated further above, no.

That's because for the libclang/c-index-test case, 
AllowPCHWithCompilerErrors=true - see clang_parseTranslationUnit_Impl. As such, 
the preamble will be generated/emitted as the second early return in 
PCHGenerator::HandleTranslationUnit is never hit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53866



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

[PATCH] D53866: [Preamble] Stop generating preamble for circular #includes

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 176941.
nik added a comment.

Added a dedicated diagnostic.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53866

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  include/clang/Lex/Preprocessor.h
  include/clang/Serialization/ASTWriter.h
  lib/Frontend/PrecompiledPreamble.cpp
  lib/Lex/PPDirectives.cpp
  test/Index/preamble-cyclic-include.cpp

Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,9 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-annotate-tokens=%s:4:1:8:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+#ifndef A_H
+#define A_H
+#  include "preamble-cyclic-include.cpp"
+int bar();
+#endif
+
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -1933,6 +1933,20 @@
 return;
   }
 
+  // Check whether it makes sense to continue preamble generation. We can't
+  // generate a consistent preamble with regard to the conditional stack if the
+  // main file is included again as due to the preamble bounds some directives
+  // (e.g. #endif of a header guard) will never be seen. Since this will lead to
+  // confusing errors, abort the preamble generation.
+  if (File && PreambleConditionalStack.isRecording() &&
+  SourceMgr.translateFile(File) == SourceMgr.getMainFileID()) {
+PreambleGenerationFailed = true;
+// Generate a fatal error to skip further processing.
+Diag(FilenameTok.getLocation(),
+ diag::err_pp_including_mainfile_for_preamble);
+return;
+  }
+
   // Should we enter the source file? Set to false if either the source file is
   // known to have no effect beyond its effect on module visibility -- that is,
   // if it's got an include guard that is already defined or is a modular header
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -170,6 +170,9 @@
   }
 
   void HandleTranslationUnit(ASTContext &Ctx) override {
+if (getPreprocessor().preambleGenerationFailed())
+  return;
+
 PCHGenerator::HandleTranslationUnit(Ctx);
 if (!hasEmittedPCH())
   return;
Index: include/clang/Serialization/ASTWriter.h
===
--- include/clang/Serialization/ASTWriter.h
+++ include/clang/Serialization/ASTWriter.h
@@ -979,6 +979,7 @@
   ASTWriter &getWriter() { return Writer; }
   const ASTWriter &getWriter() const { return Writer; }
   SmallVectorImpl &getPCH() const { return Buffer->Data; }
+  const Preprocessor &getPreprocessor() const { return PP; }
 
 public:
   PCHGenerator(const Preprocessor &PP, StringRef OutputFile, StringRef isysroot,
Index: include/clang/Lex/Preprocessor.h
===
--- include/clang/Lex/Preprocessor.h
+++ include/clang/Lex/Preprocessor.h
@@ -388,6 +388,7 @@
 SmallVector ConditionalStack;
 State ConditionalStackState = Off;
   } PreambleConditionalStack;
+  bool PreambleGenerationFailed = false;
 
   /// The current top of the stack that we're lexing from if
   /// not expanding a macro and we are lexing directly from source code.
@@ -2159,6 +2160,10 @@
   Module *M,
   SourceLocation MLoc);
 
+  bool preambleGenerationFailed() const {
+return PreambleGenerationFailed;
+  }
+
   bool isRecordingPreamble() const {
 return PreambleConditionalStack.isRecording();
   }
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -428,6 +428,8 @@
 : Error<"'%0' file not found, did you mean '%1'?">;
 def err_pp_error_opening_file : Error<
   "error opening file '%0': %1">, DefaultFatal;
+def err_pp_including_mainfile_for_preamble : Error<
+  "main file cannot be included recursively for preamble">, DefaultFatal;
 def err_pp_empty_filename : Error<"empty filename">;
 def err_pp_include_too_deep : Error<"#include nested too deeply">;
 def err_pp_expects_filename : Error<"expected \"FILENAME\" or ">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55359: [clangd] Avoid emitting Queued status when we are able to acquire the Barrier.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric, 
javed.absar.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55359

Files:
  clangd/TUScheduler.cpp
  clangd/Threading.cpp
  clangd/Threading.h
  unittests/clangd/TUSchedulerTests.cpp


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -698,13 +698,11 @@
   EXPECT_THAT(CaptureTUStatus.AllStatus,
   ElementsAre(
   // Statuses of "Update" action.
-  TUState(TUAction::Queued, "Update"),
   TUState(TUAction::RunningAction, "Update"),
   TUState(TUAction::BuildingPreamble, "Update"),
   TUState(TUAction::BuildingFile, "Update"),
 
   // Statuses of "Definitions" action
-  TUState(TUAction::Queued, "Definitions"),
   TUState(TUAction::RunningAction, "Definitions"),
   TUState(TUAction::Idle, /*No action*/ "")));
 }
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -42,6 +42,7 @@
 public:
   Semaphore(std::size_t MaxLocks);
 
+  bool try_lock();
   void lock();
   void unlock();
 
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -28,6 +28,15 @@
 
 Semaphore::Semaphore(std::size_t MaxLocks) : FreeSlots(MaxLocks) {}
 
+bool Semaphore::try_lock() {
+  std::unique_lock Lock(Mutex);
+  if (FreeSlots > 0) {
+--FreeSlots;
+return true;
+  }
+  return false;
+}
+
 void Semaphore::lock() {
   trace::Span Span("WaitForFreeSemaphoreSlot");
   // trace::Span can also acquire locks in ctor and dtor, we make sure it
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -634,13 +634,22 @@
 } // unlock Mutex
 
 {
-  // FIXME: only emit this status when the Barrier couldn't be acquired.
-  emitTUStatus({TUAction::Queued, Req.Name});
-  std::lock_guard BarrierLock(Barrier);
-  WithContext Guard(std::move(Req.Ctx));
-  trace::Span Tracer(Req.Name);
-  emitTUStatus({TUAction::RunningAction, Req.Name});
-  Req.Action();
+  auto ExecuteAction = [&]() {
+WithContext Guard(std::move(Req.Ctx));
+trace::Span Tracer(Req.Name);
+emitTUStatus({TUAction::RunningAction, Req.Name});
+Req.Action();
+  };
+  std::unique_lock Lock(Barrier, std::try_to_lock);
+  if (Lock.owns_lock()) {
+ExecuteAction();
+  } else {
+emitTUStatus({TUAction::Queued, Req.Name});
+{
+  std::lock_guard BarrierLock(Barrier);
+  ExecuteAction();
+}
+  }
 }
 
 bool IsEmpty = false;


Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -698,13 +698,11 @@
   EXPECT_THAT(CaptureTUStatus.AllStatus,
   ElementsAre(
   // Statuses of "Update" action.
-  TUState(TUAction::Queued, "Update"),
   TUState(TUAction::RunningAction, "Update"),
   TUState(TUAction::BuildingPreamble, "Update"),
   TUState(TUAction::BuildingFile, "Update"),
 
   // Statuses of "Definitions" action
-  TUState(TUAction::Queued, "Definitions"),
   TUState(TUAction::RunningAction, "Definitions"),
   TUState(TUAction::Idle, /*No action*/ "")));
 }
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -42,6 +42,7 @@
 public:
   Semaphore(std::size_t MaxLocks);
 
+  bool try_lock();
   void lock();
   void unlock();
 
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -28,6 +28,15 @@
 
 Semaphore::Semaphore(std::size_t MaxLocks) : FreeSlots(MaxLocks) {}
 
+bool Semaphore::try_lock() {
+  std::unique_lock Lock(Mutex);
+  if (FreeSlots > 0) {
+--FreeSlots;
+return true;
+  }
+  return false;
+}
+
 void Semaphore::lock() {
   trace::Span Span("WaitForFreeSemaphoreSlot");
   // trace::Span can also acquire locks in ctor and dtor, we make sure it
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -634,13 +634,22 @@
 } // unlock Mutex
 
 {
-  // FIXME: only emit this status when the Barrier couldn't be acquired.
-   

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael updated this revision to Diff 176943.
mikael added a comment.

- Added FIXME's.
- Fixed the latest comments.


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

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/CanonicalType.h
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  include/clang/Sema/Sema.h
  lib/AST/ASTContext.cpp
  lib/AST/ASTDumper.cpp
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/AST/Type.cpp
  lib/AST/TypePrinter.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGValue.h
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplate.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiate.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8370,7 +8370,7 @@
   const Decl *D = cxcursor::getCursorDecl(C);
   const CXXMethodDecl *Method =
   D ? dyn_cast_or_null(D->getAsFunction()) : nullptr;
-  return (Method && (Method->getTypeQualifiers() & Qualifiers::Const)) ? 1 : 0;
+  return (Method && Method->getTypeQualifiers().hasConst()) ? 1 : 0;
 }
 
 unsigned clang_CXXMethod_isDefaulted(CXCursor C) {
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  C(const C &c) { v = c.v; }
+  C &operator=(const C &c) {
+v = c.v;
+return *this;
+  }
+  int get() { return v; }
+};
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  C c1(c);
+  C c2;
+  c2 = c1;
+}
+
+// CHECK-LABEL: @__cxx_global_var_init()
+// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+
+// Test that the address space is __generic for the constructor
+// CHECK-LABEL: @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %this)
+// CHECK: entry:
+// CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
+// CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   call void @_ZNU3AS41CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   ret void
+
+// CHECK-LABEL: @_Z12test__globalv()
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: %call = call i32 @_ZNU3AS41C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %0 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK: call void @_ZNU3AS41CC1ERU3AS4KS_(%class.C addrspace(4)* %0, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK:   %1 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %1) #4
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:   %2 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:   %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   %call1 = call dereferenceable(4) %class.C addrspace(4)* @_ZNU3AS41CaSERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) %2)
+
+#define TEST(AS) \
+  __kernel void test##AS() { \
+AS C c;  \
+int i = c.get(); \
+C c1(c); \
+C c2;\
+c2 = c1; \
+  }
+
+TEST(__local)
+
+// CHECK-LABEL: _Z11test__localv
+// CHECK: @__cxa_guard_acquire
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael marked an inline comment as done and an inline comment as not done.
mikael added inline comments.



Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }

mikael wrote:
> rjmccall wrote:
> > The indentation here is messed up.
> > 
> > You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few 
> > places.  That's currently correct, in the sense that the fast qualifiers 
> > are currently defined to be the CVR qualifiers, but the point of the 
> > distinction is that we might want to change that (and we have in fact 
> > explored that in the past).  I think you should make `FunctionTypeBits` 
> > only claim to store the fast qualifiers, which mostly just means updating a 
> > few field / accessor names and changing things like the 
> > `getCVRQualifiers()` call right above this to be `getFastQualifiers()`.
> > 
> > If there are places where it's convenient to assume that the CVR qualifiers 
> > are exactly the fast qualifiers, it's okay to static_assert that, but you 
> > should make sure to static_assert it in each place you assume it.
> I'll change it, but I do have a question related to this:
> 
> Can we make any assumptions with the "fast qualifiers"? I'd like it if we can 
> assume that they fit in 3 bits. Otherwise I think I also need an assertion 
> here.
I solved it like this in the end:

* I defined the FastTypeQuals to get the size of Qualifiers::FastWidth to make 
it dependent on possible changes to the fast flags.
* Then I put a static_assertion to guard the hastConst(), hasVolatile() and 
hasRestrict() methods.


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

https://reviews.llvm.org/D54862



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

[PATCH] D55363: [clangd] Expose FileStatus in LSP.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric, 
javed.absar.

Add an LSP extension "textDocument/fileStatus" to emit file-status information.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55363

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  test/clangd/filestatus.test

Index: test/clangd/filestatus.test
===
--- /dev/null
+++ test/clangd/filestatus.test
@@ -0,0 +1,14 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"int x; int y = x;"}}}
+#  CHECK:  "method": "textDocument/fileStatus",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"details": [],
+# CHECK-NEXT:"state": "BuildingPreamble",
+# CHECK-NEXT:"uri": "{{.*}}/main.cpp"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -63,6 +63,8 @@
   // actions.
   };
   TUAction(State S, llvm::StringRef Name) : S(S), Name(Name){};
+  /// Returns a human readable string representation.
+  std::string toString() const;
   State S;
   /// The name of the action currently running, e.g. Update, GoToDef, Hover.
   /// Empty if we are in the idle state.
@@ -77,6 +79,8 @@
 /// Indicates whether we reused the prebuilt AST.
 bool ReuseAST = false;
   };
+  /// Serialize this to an LSP file status item.
+  FileStatus render(PathRef File) const;
 
   TUAction Action;
   BuildDetails Details;
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -739,6 +739,40 @@
   return HardwareConcurrency;
 }
 
+std::string TUAction::toString() const {
+  std::string Result;
+  raw_string_ostream OS(Result);
+  switch (S) {
+  case TUAction::Queued:
+OS << "Queued";
+break;
+  case TUAction::RunningAction:
+OS << "RunningAction"
+   << "(" << Name << ")";
+break;
+  case TUAction::BuildingPreamble:
+OS << "BuildingPreamble";
+break;
+  case TUAction::BuildingFile:
+OS << "BuildingFile";
+break;
+  default:
+break;
+  }
+  return OS.str();
+}
+
+FileStatus TUStatus::render(PathRef File) const {
+  FileStatus FStatus;
+  FStatus.uri = URIForFile::canonicalize(File, /*TUPath=*/File);
+  FStatus.state = Action.toString();
+  if (Details.BuildFailed)
+FStatus.details.push_back({MessageType::Error, "failed to build the file"});
+  if (Details.ReuseAST)
+FStatus.details.push_back({MessageType::Info, "reused the AST"});
+  return FStatus;
+}
+
 struct TUScheduler::FileData {
   /// Latest inputs, passed to TUScheduler::update().
   std::string Contents;
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -973,6 +973,33 @@
 };
 bool fromJSON(const llvm::json::Value &, ReferenceParams &);
 
+enum class MessageType {
+  Error = 1,
+  Warning = 2,
+  Info = 3,
+  Log = 4,
+};
+struct ShowMessageParams {
+  /// The message type.
+  MessageType type;
+  /// The actual message.
+  std::string message;
+};
+llvm::json::Value toJSON(const ShowMessageParams &);
+
+/// Clangd extension: indicates the current state of the file in clangd,
+/// sent from server via the `textDocument/fileStatus` notification.
+struct FileStatus {
+  /// The text document's URI.
+  URIForFile uri;
+  /// The human-readable string presents the current state of the file, can be
+  /// shown in the UI (e.g. status bar).
+  std::string state;
+  /// Details of the state that are worth sufacing to users.
+  std::vector details;
+};
+llvm::json::Value toJSON(const FileStatus &FStatus);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -716,6 +716,20 @@
   };
 }
 
+llvm::json::Value toJSON(const ShowMessageParams &S) {
+  return json::Object{
+  {"type", static_cast(S.type}, {"message", S.message},
+  };
+}
+
+llvm::json::Value toJSON(const FileStatus &FStatus) {
+  return json::Object{
+  {"uri", FStatus.uri},
+  {"state", FStatus.state},
+  {"details", FStatus.details},
+  };
+}
+
 raw_ostream &operator<<(raw_ostream &O, const DocumentHighlight &V) {
   O << V.range;
   if (V.kind == DocumentHighlightKind::Read)
Index: clangd/ClangdLSPServer.h
===

Re: r348469 - Make test resistant to line numbers changing

[Aaron Ballman via cfe-commits]

On Thu, Dec 6, 2018 at 4:25 AM Stephen Kelly via cfe-commits
 wrote:
>
> Author: steveire
> Date: Thu Dec  6 01:22:12 2018
> New Revision: 348469
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348469&view=rev
> Log:
> Make test resistant to line numbers changing

I would prefer to see the line numbers be tested rather than entirely
ignored (it's still important information we want to make sure is
correct). Can you use [[@LINE + N]] to instead make the line numbers
relative?

~Aaron

>
> Modified:
> cfe/trunk/test/AST/dump.cpp
>
> Modified: cfe/trunk/test/AST/dump.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/dump.cpp?rev=348469&r1=348468&r2=348469&view=diff
> ==
> --- cfe/trunk/test/AST/dump.cpp (original)
> +++ cfe/trunk/test/AST/dump.cpp Thu Dec  6 01:22:12 2018
> @@ -43,7 +43,7 @@ struct S {
>}
>  };
>
> -// CHECK:  | `-OMPParallelForDirective {{.+}} {{ col:80>|}}
> +// CHECK:  | `-OMPParallelForDirective {{.+}} {{ col:80>|}}
>  // CHECK-NEXT: |   |-OMPDefaultClause {{.+}} 
>  // CHECK-NEXT: |   |-OMPPrivateClause {{.+}} 
>  // CHECK-NEXT: |   | `-DeclRefExpr {{.+}}  'int' lvalue 
> OMPCapturedExpr {{.+}} 'a' 'int &'
> @@ -53,19 +53,19 @@ struct S {
>  // CHECK-NEXT: |   |-OMPScheduleClause {{.+}} 
>  // CHECK-NEXT: |   | `-ImplicitCastExpr {{.+}}  'int' 
> 
>  // CHECK-NEXT: |   |   `-DeclRefExpr {{.+}}  'int' lvalue 
> OMPCapturedExpr {{.+}} '.capture_expr.' 'int'
> -// CHECK-NEXT: |   `-CapturedStmt {{.+}} 
> +// CHECK-NEXT: |   `-CapturedStmt {{.+}} 
>  // CHECK-NEXT: | |-CapturedDecl {{.+}} <>  sloc>
> -// CHECK-NEXT: | | |-ForStmt {{.+}} 
> -// CHECK:  | | | `-UnaryOperator {{.+}}  'int' 
> lvalue prefix '++'
> +// CHECK-NEXT: | | |-ForStmt {{.+}} 
> +// CHECK:  | | | `-UnaryOperator {{.+}}  
> 'int' lvalue prefix '++'
>  // CHECK-NEXT: | | |   `-DeclRefExpr {{.+}}  'int' lvalue 
> OMPCapturedExpr {{.+}} 'a' 'int &'
>
>  #pragma omp declare simd
>  #pragma omp declare simd inbranch
>  void foo();
>
> -// CHECK:|-FunctionDecl {{.+}}  col:6 foo 'void 
> ()'
> -// CHECK-NEXT:   |-OMPDeclareSimdDeclAttr {{.+}}  
> Implicit BS_Inbranch
> -// CHECK:`-OMPDeclareSimdDeclAttr {{.+}}  
> Implicit BS_Undefined
> +// CHECK:|-FunctionDecl {{.+}}  col:6 foo 
> 'void ()'
> +// CHECK-NEXT:   |-OMPDeclareSimdDeclAttr {{.+}}  
> Implicit BS_Inbranch
> +// CHECK:`-OMPDeclareSimdDeclAttr {{.+}}  
> Implicit BS_Undefined
>
>  #pragma omp declare target
>  int bar() {
> @@ -74,11 +74,11 @@ int bar() {
>  }
>  #pragma omp end declare target
>
> -// CHECK:   `-FunctionDecl {{.+}}  line:71:5 bar 
> 'int ()'
> -// CHECK-NEXT:  |-CompoundStmt {{.+}} 
> -// CHECK-NEXT:  | |-DeclStmt {{.+}} 
> +// CHECK:   `-FunctionDecl {{.+}}  
> line:{{.+}}:5 bar 'int ()'
> +// CHECK-NEXT:  |-CompoundStmt {{.+}} 
> +// CHECK-NEXT:  | |-DeclStmt {{.+}} 
>  // CHECK-NEXT:  | | `-VarDecl {{.+}}  col:7 used f 'int'
> -// CHECK-NEXT:  | `-ReturnStmt {{.+}} 
> +// CHECK-NEXT:  | `-ReturnStmt {{.+}} 
>  // CHECK-NEXT:  |   `-ImplicitCastExpr {{.+}}  'int' 
>  // CHECK-NEXT:  | `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
> 'f' 'int'
>  // CHECK-NEXT:  `-OMPDeclareTargetDeclAttr {{.+}} <> Implicit 
> MT_To
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r348471 - Add test for ObjC generics

[Aaron Ballman via cfe-commits]

On Thu, Dec 6, 2018 at 4:26 AM Stephen Kelly via cfe-commits
 wrote:
>
> Author: steveire
> Date: Thu Dec  6 01:23:59 2018
> New Revision: 348471
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348471&view=rev
> Log:
> Add test for ObjC generics
>
> Modified:
> cfe/trunk/test/AST/ast-dump-decl.m
>
> Modified: cfe/trunk/test/AST/ast-dump-decl.m
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-decl.m?rev=348471&r1=348470&r2=348471&view=diff
> ==
> --- cfe/trunk/test/AST/ast-dump-decl.m (original)
> +++ cfe/trunk/test/AST/ast-dump-decl.m Thu Dec  6 01:23:59 2018
> @@ -81,6 +81,14 @@
>  // CHECK-NEXT:   ObjCProtocol{{.*}} 'P'
>  // CHECK-NEXT:   ObjCMethodDecl{{.*}} bar
>
> +@interface TestGenericInterface : A {
> +}
> +@end
> +// CHECK:  ObjCInterfaceDecl{{.*}} TestGenericInterface
> +// CHECK-NEXT:   -ObjCTypeParamDecl
> +// CHECK-NEXT:   -super ObjCInterface
> +// CHECK-NEXT:   -ObjCProtocol

It would be useful to test the line and column, identifier, and type
information as well as the basic AST nodes.

~Aaron

> +
>  @implementation TestObjCClass (TestObjCCategoryDecl)
>  - (void) bar {
>  }
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54565: Introduce `-Wc++14-compat-ctad` as a subgroup of `-Wc++14-compat`

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone updated this revision to Diff 176955.
Quuxplusone added a comment.

Rename `-Wc++14-compat-ctad` to just `-Wctad`, per @rsmith's comment to "move 
this out of `-Wc++14-compat`."

@rsmith ping?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54565

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  test/SemaCXX/cxx14-compat-ctad.cpp


Index: test/SemaCXX/cxx14-compat-ctad.cpp
===
--- /dev/null
+++ test/SemaCXX/cxx14-compat-ctad.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -Wctad -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++2a -Wctad -verify %s
+
+template struct X {};
+X x; // expected-warning {{class template argument deduction is incompatible 
with C++ standards before C++17; for compatibility, use explicit type name 
'X'}}
+
+template class> struct Y {};
+Y yx; // ok, not class template argument deduction
+
+template void f(T t) {
+  X x = t; // expected-warning {{class template argument deduction is 
incompatible with C++ standards before C++17}}
+}
+
+template void g(T t) {
+  typename T::X x = t; // expected-warning {{class template argument deduction 
is incompatible with C++ standards before C++17; for compatibility, use 
explicit type name 'typename A::X' (aka 'A::X')}}
+}
+
+struct A { template struct X { X(T); }; };
+void h(A a) { g(a); } // expected-note {{in instantiation of function template 
specialization 'g'}}
+
+template struct V { V(const T&) {} };
+
+V(int) -> V;  // ok, deduction guide is not a use of class template 
argument deduction
+
+void f2() { V v('a'); } // expected-warning {{class template argument 
deduction is incompatible with C++ standards before C++17; for compatibility, 
use explicit type name 'V'}}
+void g2() { V v(0); } // expected-warning {{class template argument deduction 
is incompatible with C++ standards before C++17; for compatibility, use 
explicit type name 'V'}}
+
+void h2() {
+auto lam = [](){};
+V v(lam); // expected-warning {{class template argument deduction is 
incompatible with C++ standards before C++17}}
+}
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2159,7 +2159,7 @@
 def warn_cxx14_compat_class_template_argument_deduction : Warning<
   "class template argument deduction is incompatible with C++ standards "
   "before C++17%select{|; for compatibility, use explicit type name %1}0">,
-  InGroup, DefaultIgnore;
+  InGroup, DefaultIgnore;
 
 // C++14 deduced return types
 def err_auto_fn_deduction_failure : Error<
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -175,6 +175,7 @@
 
 def InvalidIOSDeploymentTarget : DiagGroup<"invalid-ios-deployment-target">;
 
+def CTAD : DiagGroup<"ctad">;
 def CXX17CompatMangling : DiagGroup<"c++17-compat-mangling">;
 def : DiagGroup<"c++1z-compat-mangling", [CXX17CompatMangling]>;
 // Name of this warning in GCC.
@@ -185,7 +186,8 @@
 def CXXPre14CompatPedantic : DiagGroup<"c++98-c++11-compat-pedantic",
[CXXPre14Compat,
 CXXPre14CompatBinaryLiteral]>;
-def CXXPre17Compat : DiagGroup<"c++98-c++11-c++14-compat">;
+def CXXPre17Compat : DiagGroup<"c++98-c++11-c++14-compat",
+   [CTAD]>;
 def CXXPre17CompatPedantic : DiagGroup<"c++98-c++11-c++14-compat-pedantic",
[CXXPre17Compat]>;
 def CXXPre2aCompat : DiagGroup<"c++98-c++11-c++14-c++17-compat">;


Index: test/SemaCXX/cxx14-compat-ctad.cpp
===
--- /dev/null
+++ test/SemaCXX/cxx14-compat-ctad.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -Wctad -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++2a -Wctad -verify %s
+
+template struct X {};
+X x; // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17; for compatibility, use explicit type name 'X'}}
+
+template class> struct Y {};
+Y yx; // ok, not class template argument deduction
+
+template void f(T t) {
+  X x = t; // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17}}
+}
+
+template void g(T t) {
+  typename T::X x = t; // expected-warning {{class template argument deduction is incompatible with C++ standards before C++17; for compatibility, use explicit type name 'typename A::X' (aka 'A::X')}}
+}
+
+struct A { template struct X { X(T); }; };
+void h(A a) { g(a); } // expected-note {{in instantiation of function template specialization 'g'}

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus planned changes to this revision.
Szelethus added a comment.

Let's try that again then! :)


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

https://reviews.llvm.org/D51866



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

[PATCH] D55337: NFC: Move dumpDeclRef to NodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/TextNodeDumper.h:28
const comments::FullComment *> {
+  TextTreeStructure &TreeStructure;
   raw_ostream &OS;

This makes me a bit wary because you create a node dumper in the same 
situations you make a tree structure object, but now there's a strict ordering 
between the two object creations. If you're doing this construction local to a 
function, you wind up with a dangling reference unless you're careful (which is 
unfortunate, but not the end of the world). If you're doing this construction 
as part of a constructor's initializer list, you now have to properly order the 
member declarations within the class and that is also unfortunate. Given that 
those are the two common scenarios for how I envision constructing an ast dump 
of some kind, I worry about the fragility. e.g.,
```
unique_ptr createASTDumper(...) {
  TextTreeStructure TreeStructure;
  TextNodeDumper NodeDumper(TreeStructure); // Oops, dangling reference
  return make_unique(TreeStructure, NodeDumper, ...);
}

// vs

struct MySuperAwesomeASTDumper : ... {
  MySuperAwesomeASTDumper() : TreeStructure(...), NodeDumper(TreeStructure, 
...) {}
private:
  TextTreeStructure TreeStructure; // This order is now SUPER important
  TextNodeDumper NodeDumper;
};
```
There's a part of me that wonders if a better approach is to have this object 
passed to the `dumpFoo()` calls as a reference parameter. This way, the caller 
is still responsible for creating an object, but the creation order between the 
tree and the node dumper isn't as fragile.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55337



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

[PATCH] D55212: Handle alloc_size attribute on function pointers

[Alexander Richardson via Phabricator via cfe-commits]

arichardson marked an inline comment as done.
arichardson added inline comments.



Comment at: include/clang/Basic/Attr.td:1072
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
+  let Subjects = SubjectList<[HasFunctionProto]>;

aaron.ballman wrote:
> arichardson wrote:
> > aaron.ballman wrote:
> > > arichardson wrote:
> > > > aaron.ballman wrote:
> > > > > Does GCC support writing `alloc_size` on function pointers?
> > > > Yes it does and it seems to be used by some projects. I first 
> > > > discovered this while compiling libxml2: 
> > > > https://github.com/GNOME/libxml2/blob/35e83488505d501864826125cfe6a7950d6cba78/include/libxml/xmlmemory.h#L66
> > > Parsed and ignored is different than supported. For instance, I can't 
> > > seem to get GCC to produce different behavior here: 
> > > https://godbolt.org/z/MI5k_m
> > > 
> > > Am I missing something?
> > Ah yes, it does seem like it is ignored. I assumed it would work for GCC 
> > since it handles, e.g. the format attribute on function pointers.
> > 
> > However, I would like it if we could support it on function pointers even 
> > if GCC just ignores is.
> > However, I would like it if we could support it on function pointers even 
> > if GCC just ignores is.
> 
> We can, but the question is: under what spelling(s)? I think we can support 
> this under `__attribute__((alloc_size(N)))` without issue. If GCC is looking 
> to implement this same behavior, then I think we can also support it under 
> `[[gnu::alloc_size(N)]]`.
> 
> If GCC doesn't have plans to add this functionality any time soon, we'd be 
> stepping on their implementation namespace by implementing this functionality 
> under the gnu vendor namespace and users would then be confused as to whose 
> bug it is. In that case, I'd recommend we add `[[clang::alloc_size(N)]]` that 
> behaves identically to `[[gnu::alloc_size(N)]]' except that it can be applied 
> to function pointer types, and not change the behavior of 
> `[[gnu::alloc_size(N)]]`. This can be implemented as a single semantic 
> attribute by looking at the semantic spelling of the attribute and issuing a 
> compatibility warning if you see `[[gnu::alloc_size(N)]]` being applied to a 
> function pointer type, and can even recommend a fix-it to switch to 
> `[[clang::alloc_size(N)]]` in that case. (See uses of `getSemanticSpelling()` 
> elsewhere in the project for examples of how to test which spelling a given 
> sematic attribute was spelled with.)
It seems like GCC will also implement this behaviour so using 
`[[gnu::alloc_size()]]` (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88372#c1) 
should be fine.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55212



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

[PATCH] D55338: NFC: Move VisitStmt code to dumpStmt

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55338



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

[PATCH] D55190: Move dump of individual comment nodes to NodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55190



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

[PATCH] D55339: NFC: Move VisitExpr code to dumpStmt

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from a small nit.




Comment at: lib/AST/ASTDumper.cpp:1698
 
+if (auto *E = dyn_cast(S)) {
+  NodeDumper.dumpType(E->getType());

`const auto *`


Repository:
  rC Clang

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

https://reviews.llvm.org/D55339



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

[PATCH] D55346: [clang-tidy] check for using declaration scope and qualification

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Could you send each check in a separate patch, please? This makes the review 
much more straightforward and I'm sure it will be faster too.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55346



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

[PATCH] D55346: [clang-tidy] check for using declaration scope and qualification

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/abseil/AbseilTidyModule.cpp:25
 #include "StrCatAppendCheck.h"
 

Please upload the patch with full context. See 
https://llvm.org/docs/Phabricator.html


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55346



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

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D54945#1318343 , @MarinaKalashina 
wrote:

> @alexfh Thanks a lot for your patience and help. I've made another revision, 
> now with the diff made by 'git show HEAD -U99' to have the full context 
> availlable.


I don't know how this happened, but the diff is still almost empty. This is 
what I get when I click on the "Download raw diff":

  Index: docs/clang-tidy/index.rst
  ===
  --- docs/clang-tidy/index.rst
  +++ docs/clang-tidy/index.rst
  @@ -403,7 +403,7 @@
   
   // Silent only the specified diagnostics for the next line
   // NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
  -Foo(bool param);
  +Foo(bool param); 
 };
   
   The formal syntax of ``NOLINT``/``NOLINTNEXTLINE`` is the following:

It's definitely not a common problem, since there are other patches in review, 
which are displayed just fine.


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

https://reviews.llvm.org/D54945



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

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D54945#1321301 , @alexfh wrote:

> In D54945#1318343 , @MarinaKalashina 
> wrote:
>
> > @alexfh Thanks a lot for your patience and help. I've made another 
> > revision, now with the diff made by 'git show HEAD -U99' to have the 
> > full context availlable.
>
>
> I don't know how this happened, but the diff is still almost empty.


A correction: it's now empty (and it was broken in a different way last time).

Could you verify that you're correctly following one of the processed described 
in the documentation (https://llvm.org/docs/Phabricator.html)? It looks like 
you're uploading only one commit from the whole branch.


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

https://reviews.llvm.org/D54945



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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 176962.
nik marked an inline comment as done.
nik added a comment.

Addressed comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D41005

Files:
  include/clang/Frontend/ASTUnit.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/PrecompiledPreamble.cpp
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -53,7 +53,10 @@
   FileSystemOptions FSOpts;
 
 public:
-  void SetUp() override {
+  void SetUp() override { ResetVFS(); }
+  void TearDown() override {}
+
+  void ResetVFS() {
 VFS = new ReadCountingInMemoryFileSystem();
 // We need the working directory to be set to something absolute,
 // otherwise it ends up being inadvertently set to the current
@@ -64,9 +67,6 @@
 VFS->setCurrentWorkingDirectory("//./");
   }
 
-  void TearDown() override {
-  }
-
   void AddFile(const std::string &Filename, const std::string &Contents) {
 ::time_t now;
 ::time(&now);
@@ -124,6 +124,72 @@
   }
 };
 
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which does not exist on
+  // disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Reparse and check that the preamble was reused.
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which does not exist on
+  // disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Create the unsaved file also on disk and check that preamble was reused.
+  AddFile(Header1, "#define ZERO 0\n");
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+   ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+  std::string Header1 = "//./foo/header1.h";
+  std::string MainName = "//./main.cpp";
+  std::string MainFileContent = R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp";
+  AddFile(MainName, MainFileContent);
+  AddFile(Header1, "#define ZERO 0\n");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which exists on disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+
+  // Remove the unsaved file from disk and check that the preamble was reused.
+  ResetVFS();
+  AddFile(MainName, MainFileContent);
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounterForTests(), 1U);
+}
+
 TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
   std::string Header1 = "//./header1.h";
   std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -28,6 +28,7 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include 
@@ -449,20 +450,33 @@
 Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
   }
 
+  llvm::StringMap OverridenFileBuffers;
   for (const auto &RB : PreprocessorOpts.RemappedFileBuffers) {
-llvm::vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
-  return false;
-
-OverriddenFiles[Status.getUniqueID()] =
+const PrecompiledPreamble::PreambleFileHash PreambleHash =
 PreambleFileHash::createForMemoryBuffer(RB.second);
+llvm::vfs::Status Status;
+if (moveOnNoError(VFS->status(RB.first), Status)) {
+  OverriddenFiles[Status.getUniqueID()] = PreambleHash;
+} else {
+  OverridenFileBuffers[RB.first] = PreambleHash;
+}
   }
 
   // Check whether anything has changed.
   for (const auto &F : FilesInPreamble) {
+auto OverridenFileBuffer = OverridenFileBuffers.fin

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Nikolai Kosjar via Phabricator via cfe-commits]

nik marked 4 inline comments as done.
nik added inline comments.



Comment at: include/clang/Frontend/ASTUnit.h:581
 
+  unsigned getPreambleCounter() const { return PreambleCounter; }
+

ilya-biryukov wrote:
> NIT: `getPreambleCounterForTests()`? This is clearly an internal detail, 
> would try giving it a name that discourages using it outside the testing code.
Done.

Side note: I hardly see something like this used in clang, but I agree that 
it's good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D41005



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

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Maybe try to use Arcanist for uploading diffs?
The web interface can be confusing at times, Arcanist is more reliable (LLVM 
docs 

 have pointers to the Arcanist documentation to get started)


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

https://reviews.llvm.org/D54945



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

[PATCH] D55340: NFC: Move dump of individual Stmts to TextNodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from some small nits.




Comment at: lib/AST/TextNodeDumper.cpp:685
+void TextNodeDumper::VisitObjCSelectorExpr(const ObjCSelectorExpr *Node) {
+
+  OS << " ";

Extra whitespace that wasn't there before.



Comment at: lib/AST/TextNodeDumper.cpp:691
+void TextNodeDumper::VisitObjCProtocolExpr(const ObjCProtocolExpr *Node) {
+
+  OS << ' ' << *Node->getProtocol();

Extra whitespace that wasn't there before.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55340



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

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[Alexander Kornienko via Phabricator via cfe-commits]

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

In D54401#1320772 , @Szelethus wrote:

> In D54401#1318282 , @alexfh wrote:
>
> > In D54401#1315354 , @NoQ wrote:
> >
> > > The code looks good, but i vaguely remember that we might accidentally 
> > > break clang-tidy integration that uses this API to enable/disable 
> > > specific checkers via `-checks=-analyzer-...`.
> > >
> > > *summons @alexfh*
> >
> >
> > It's hard to tell without trying. Could you build and test 
> > clang-tools-extra with this patch? There should be a test for the relevant 
> > functionality in clang-tidy.
>
>
> It compiles without problem, and `check-all` doesn't produce a single 
> `clang-tools-extra` failure. Yay!


Then I have no concerns.


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

https://reviews.llvm.org/D54401



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

[clang-tools-extra] r348490 - [clangd] Remove the test that sometimes deadlocks

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Thu Dec  6 07:14:11 2018
New Revision: 348490

URL: http://llvm.org/viewvc/llvm-project?rev=348490&view=rev
Log:
[clangd] Remove the test that sometimes deadlocks

Will figure out how to properly rewrite it and recommit.

Modified:
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp

Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=348490&r1=348489&r2=348490&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Dec  6 
07:14:11 2018
@@ -709,29 +709,6 @@ TEST_F(TUSchedulerTests, TUStatus) {
   TUState(TUAction::Idle, /*No action*/ "")));
 }
 
-TEST_F(TUSchedulerTests, NoTUStatusEmittedForRemovedFile) {
-  class CaptureTUStatus : public DiagnosticsConsumer {
-  public:
-void onDiagnosticsReady(PathRef File,
-std::vector Diagnostics) override {}
-
-void onFileUpdated(PathRef File, const TUStatus &Status) override {
-  // Block the worker thread until the document is removed.
-  Removed.wait();
-}
-Notification Removed;
-  } CaptureTUStatus;
-  MockFSProvider FS;
-  MockCompilationDatabase CDB;
-  ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
-
-  Server.addDocument(testPath("foo.cpp"), "int main() {}",
- WantDiagnostics::Yes);
-  Server.removeDocument(testPath("foo.cpp"));
-  CaptureTUStatus.Removed.notify();
-  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for finishing";
-}
-
 } // namespace
 } // namespace clangd
 } // namespace clang


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

[PATCH] D55371: Fix thunks returning memptrs via sret by emitting also scalar return values directly in sret slot (PR39901)

[Hans Wennborg via Phabricator via cfe-commits]

hans edited subscribers, added: cfe-commits; removed: llvm-commits.
hans added a comment.

-llvm-commits
+cfe-commits


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

https://reviews.llvm.org/D55371



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

r348492 - [OPENMP][NVPTX] Fix globalization of the mapped array sections.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Thu Dec  6 07:35:13 2018
New Revision: 348492

URL: http://llvm.org/viewvc/llvm-project?rev=348492&view=rev
Log:
[OPENMP][NVPTX] Fix globalization of the mapped array sections.

If the array section is based on pointer and this sections is mapped in
target region + then it is used in the inner parallel region, it also
must be globalized as the pointer itself is passed by value, not by
reference.

Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
cfe/trunk/test/OpenMP/nvptx_target_codegen.cpp

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp?rev=348492&r1=348491&r2=348492&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp Thu Dec  6 07:35:13 2018
@@ -326,9 +326,11 @@ class CheckVarsEscapingDeclContext final
   const auto *Attr = FD->getAttr();
   if (!Attr)
 return;
-  if (!isOpenMPPrivate(
-  static_cast(Attr->getCaptureKind())) ||
-  Attr->getCaptureKind() == OMPC_map)
+  if (((Attr->getCaptureKind() != OMPC_map) &&
+   !isOpenMPPrivate(
+   static_cast(Attr->getCaptureKind( ||
+  ((Attr->getCaptureKind() == OMPC_map) &&
+   !FD->getType()->isAnyPointerType()))
 return;
 }
 if (!FD->getType()->isReferenceType()) {

Modified: cfe/trunk/test/OpenMP/nvptx_target_codegen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/nvptx_target_codegen.cpp?rev=348492&r1=348491&r2=348492&view=diff
==
--- cfe/trunk/test/OpenMP/nvptx_target_codegen.cpp (original)
+++ cfe/trunk/test/OpenMP/nvptx_target_codegen.cpp Thu Dec  6 07:35:13 2018
@@ -8,15 +8,18 @@
 #ifndef HEADER
 #define HEADER
 
-// Check that the execution mode of all 6 target regions is set to Generic 
Mode.
+// Check that the execution mode of all 7 target regions is set to Generic 
Mode.
 // CHECK-DAG: [[NONSPMD:@.+]] = private unnamed_addr constant %struct.ident_t 
{ i32 0, i32 2, i32 0, i32 0, i8* getelementptr inbounds
 // CHECK-DAG: [[UNKNOWN:@.+]] = private unnamed_addr constant %struct.ident_t 
{ i32 0, i32 2, i32 2, i32 0, i8* getelementptr inbounds
-// CHECK-DAG: {{@__omp_offloading_.+l105}}_exec_mode = weak constant i8 1
-// CHECK-DAG: {{@__omp_offloading_.+l182}}_exec_mode = weak constant i8 1
-// CHECK-DAG: {{@__omp_offloading_.+l292}}_exec_mode = weak constant i8 1
-// CHECK-DAG: {{@__omp_offloading_.+l330}}_exec_mode = weak constant i8 1
-// CHECK-DAG: {{@__omp_offloading_.+l348}}_exec_mode = weak constant i8 1
-// CHECK-DAG: {{@__omp_offloading_.+l313}}_exec_mode = weak constant i8 1
+// CHECK-DAG: {{@__omp_offloading_.+l59}}_exec_mode = weak constant i8 1
+// CHECK-DAG: {{@__omp_offloading_.+l137}}_exec_mode = weak constant i8 1
+// CHECK-DAG: {{@__omp_offloading_.+l214}}_exec_mode = weak constant i8 1
+// CHECK-DAG: {{@__omp_offloading_.+l324}}_exec_mode = weak constant i8 1
+// CHECK-DAG: {{@__omp_offloading_.+l362}}_exec_mode = weak constant i8 1
+// CHECK-DAG: {{@__omp_offloading_.+l380}}_exec_mode = weak constant i8 1
+// CHECK-DAG: {{@__omp_offloading_.+l345}}_exec_mode = weak constant i8 1
+// CHECK-DAG: [[MAP_TY:%.+]] = type { [{{8|4}} x i8] }
+// CHECK-DAG: [[GLOB_TY:%.+]] = type { i32* }
 
 __thread int id;
 
@@ -29,6 +32,35 @@ struct TT{
   tx &operator[](int i) { return X; }
 };
 
+// CHECK: define weak void 
@__omp_offloading_{{.+}}_{{.+}}targetBar{{.+}}_l59(i32* [[PTR1:%.+]], i32** 
dereferenceable{{.*}} [[PTR2_REF:%.+]])
+// CHECK: store i32* [[PTR1]], i32** [[PTR1_ADDR:%.+]],
+// CHECK: store i32** [[PTR2_REF]], i32*** [[PTR2_REF_PTR:%.+]],
+// CHECK: [[PTR2_REF:%.+]] = load i32**, i32*** [[PTR2_REF_PTR]],
+// CHECK: call void @__kmpc_kernel_init(
+// CHECK: call void @__kmpc_get_team_static_memory(i8* addrspacecast (i8 
addrspace(3)* getelementptr inbounds ([[MAP_TY]], [[MAP_TY]] addrspace(3)* 
@{{.+}}, i32 0, i32 0, i32 0) to i8*), i{{64|32}} %{{.+}}, i16 %{{.+}}, i8** 
addrspacecast (i8* addrspace(3)* [[BUF_PTR:@.+]] to i8**))
+// CHECK: [[BUF:%.+]] = load i8*, i8* addrspace(3)* [[BUF_PTR]],
+// CHECK: [[BUF_OFFS:%.+]] = getelementptr inbounds i8, i8* [[BUF]], 
i{{[0-9]+}} 0
+// CHECK: [[BUF:%.+]] = bitcast i8* [[BUF_OFFS]] to [[GLOB_TY]]*
+// CHECK: [[PTR1:%.+]] = load i32*, i32** [[PTR1_ADDR]],
+// CHECK: [[PTR1_GLOB_REF:%.+]] = getelementptr inbounds [[GLOB_TY]], 
[[GLOB_TY]]* [[BUF]], i32 0, i32 0
+// CHECK: store i32* [[PTR1]], i32** [[PTR1_GLOB_REF]],
+// CHECK: call void @__kmpc_begin_sharing_variables(i8*** 
[[ARG_PTRS_REF:%.+]], i{{64|32}} 2)
+// CHECK: [[ARG_PTRS:%.+]] = load i8**, i8*** [[ARG_PTRS_REF]],
+// CHECK: [[ARG_PTR1:%.+]] = getelementptr inbounds i8*, i8** [[ARG_PTRS]], 
i{{[0-9]+}

[PATCH] D55280: [CTU] Make loadExternalAST return with non nullptr on success

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Hi Aleksey,

The first version was indeed not correct. But, I think we handle that case with 
the latest update (https://reviews.llvm.org/differential/diff/176779/).  In 
`loadExternalAST()` we have this right before the return:

  if (!Unit)
  return llvm::make_error(
  index_error_code::failed_to_get_external_ast);
  return Unit;

So we will never return with a nullptr value in the `Expected`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55280



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

[PATCH] D55212: Handle alloc_size attribute on function pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think all that's missing are some C++ tests and some nits.




Comment at: include/clang/Basic/Attr.td:1072
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
+  let Subjects = SubjectList<[HasFunctionProto]>;

arichardson wrote:
> aaron.ballman wrote:
> > arichardson wrote:
> > > aaron.ballman wrote:
> > > > arichardson wrote:
> > > > > aaron.ballman wrote:
> > > > > > Does GCC support writing `alloc_size` on function pointers?
> > > > > Yes it does and it seems to be used by some projects. I first 
> > > > > discovered this while compiling libxml2: 
> > > > > https://github.com/GNOME/libxml2/blob/35e83488505d501864826125cfe6a7950d6cba78/include/libxml/xmlmemory.h#L66
> > > > Parsed and ignored is different than supported. For instance, I can't 
> > > > seem to get GCC to produce different behavior here: 
> > > > https://godbolt.org/z/MI5k_m
> > > > 
> > > > Am I missing something?
> > > Ah yes, it does seem like it is ignored. I assumed it would work for GCC 
> > > since it handles, e.g. the format attribute on function pointers.
> > > 
> > > However, I would like it if we could support it on function pointers even 
> > > if GCC just ignores is.
> > > However, I would like it if we could support it on function pointers even 
> > > if GCC just ignores is.
> > 
> > We can, but the question is: under what spelling(s)? I think we can support 
> > this under `__attribute__((alloc_size(N)))` without issue. If GCC is 
> > looking to implement this same behavior, then I think we can also support 
> > it under `[[gnu::alloc_size(N)]]`.
> > 
> > If GCC doesn't have plans to add this functionality any time soon, we'd be 
> > stepping on their implementation namespace by implementing this 
> > functionality under the gnu vendor namespace and users would then be 
> > confused as to whose bug it is. In that case, I'd recommend we add 
> > `[[clang::alloc_size(N)]]` that behaves identically to 
> > `[[gnu::alloc_size(N)]]' except that it can be applied to function pointer 
> > types, and not change the behavior of `[[gnu::alloc_size(N)]]`. This can be 
> > implemented as a single semantic attribute by looking at the semantic 
> > spelling of the attribute and issuing a compatibility warning if you see 
> > `[[gnu::alloc_size(N)]]` being applied to a function pointer type, and can 
> > even recommend a fix-it to switch to `[[clang::alloc_size(N)]]` in that 
> > case. (See uses of `getSemanticSpelling()` elsewhere in the project for 
> > examples of how to test which spelling a given sematic attribute was 
> > spelled with.)
> It seems like GCC will also implement this behaviour so using 
> `[[gnu::alloc_size()]]` 
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88372#c1) should be fine.
Fantastic, thank you for coordinating this! I think using the `GCC` spelling is 
the right way to go then.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55212



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

[PATCH] D49890: Clang-Tidy Export Problem

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

In D49890#1200061 , @TheAhmad wrote:

> In D49890#1182556 , @alexfh wrote:
>
> > Could you describe the specific problem you're solving and provide an 
> > example? As mentioned by others, a test would be very welcome as well.
>
>
> Sorry for so much delay,  @alexfh. I didn't see your comment. I will describe 
> in detail:
>  I wanted to do a source to source transformation on `MPlayer-1.3.0` source 
> code. The transformation may require modification of many files and possibly 
> repeated modifications in the headers files included in multiple `.c` files. 
> Therefore, the changes should be serialized for each translation unit and 
> stored in a `YAML` file. At the end, `clang-apply-replacements` will be 
> called and transform the entire source code.
>  The problem is that `clang-tidy` expects a limited format for the 
> compilation database. This is the format typically used when the build system 
> generating the compilation database is `CMAKE`.  But `MPlayer` uses 
> `Makefile`. Therefore, I had to use an external database generator, `Bear`.  
> In this case, the contents of the `YAML` files are OK. But it is not what is 
> expected by `clang-tidy`.  `clang-tidy` requires every file path to be 
> absolute, even the header files.
>  The problem (i.e., using relative paths) only arises when the fixes are 
> `exported`. Not when they are applied `in-place`. I reused some of the code 
> for the in-place case and did some modifications to it. The code is OK, at 
> least for my case with `MPlayer`. A small change is still needed to support 
> `merge conflicts` which can be brought from the `in-place fix` stuff. It 
> seems that at the end the commanlities of the two cases should be put in a 
> function. Then this function can be called from both places (i.e., the 
> `in-place fix` and the `export fix`).
>  I am new to `Clang` and do not know what is needed for tests. I am looking 
> forward to your reply.
>  Regards.


Sorry for missing your reply (vacation, travels - and a lot of mail got buried 
under other mail). Feel free to ping regularly, if you don't get a response.

The solution you propose seems reasonable, but 1. we need a test, 2. I would 
like to better understand where relative paths are coming from (`directory`? 
include search paths in the compilation command?). Could you upload an example 
of a problematic compilation database?




Comment at: ClangTidy.cpp:614
+  vfs::FileSystem &FileSystem = *Files->getVirtualFileSystem();
+  auto InitialWorkingDir = FileSystem.getCurrentWorkingDirectory();
+  if (!InitialWorkingDir)

TheAhmad wrote:
> JonasToth wrote:
> > TheAhmad wrote:
> > > Eugene.Zelenko wrote:
> > > > Type is not obvious, so please don't use auto.
> > > Hi, Eugene. Why line 352 uses auto?
> > He means line 615 (`InitialWorkingDir`). The type of the variable can not 
> > be deduced from reading the code.
> > 
> > The rule is, to write the type once. E.g. `llvm::make_unique(args)` 
> > makes it clear, that the type is `MyType`, so you can use `auto` for the 
> > variable.
> > This is not the case for `InitialWorkingDir`.
> Right. I agree. So `line 352` should not use `auto` either.
Agree. Line 352 should use explicit type.



Comment at: ClangTidy.cpp:612
+
+  auto Files = make_unique(FileSystemOptions());
+  vfs::FileSystem &FileSystem = *Files->getVirtualFileSystem();

Should we get the file manager from the SourceManager as in handleErrors?



Comment at: ClangTidy.cpp:632
+  AbsoluteError.Fix.clear();
+  SingleErrors.insert(std::make_pair(ErrorAbsoluteFilePath, 
AbsoluteError));
+}

I'd use `try_emplace` instead.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D49890



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

[PATCH] D55374: [clangd] Show FileStatus in vscode-clangd.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric.

The file status will be shown in the status bar.
Depends on D55363 .


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55374

Files:
  clangd/clients/clangd-vscode/src/extension.ts


Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -18,6 +18,37 @@
  void, void>('textDocument/switchSourceHeader');
 }
 
+class FileStatus {
+private statuses = new Map();
+private readonly statusBarItem =
+vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left, 10);
+
+onFileUpdated(fileStatus: any) {
+  const filePath = vscode.Uri.parse(fileStatus.uri);
+  this.statuses.set(filePath.fsPath, fileStatus);
+  this.updateStatus();
+}
+
+updateStatus() {
+const path = vscode.window.activeTextEditor.document.fileName;
+const status = this.statuses.get(path);
+if (!status) {
+  this.statusBarItem.hide();
+  return;
+}
+this.statusBarItem.text = `clangd: ` + status.state;
+// FIXME: find a way to show the details in a nicer way.
+if (status.details.length > 0)
+this.statusBarItem.tooltip = status.details.map(
+(detail: any) => detail.message).join('; ');
+this.statusBarItem.show()
+}
+
+dispose() {
+this.statusBarItem.dispose();
+}
+}
+
 /**
  *  this method is called when your extension is activate
  *  your extension is activated the very first time the command is executed
@@ -80,4 +111,13 @@
 vscode.Uri.parse(sourceUri));
 vscode.window.showTextDocument(doc);
   }));
+  const status = new FileStatus();
+  context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(() => {
+  status.updateStatus();
+  }))
+  clangdClient.onReady().then(() => {
+  clangdClient.onNotification('textDocument/fileStatus', (fileStatus) => {
+  status.onFileUpdated(fileStatus)
+  });
+  })
 }


Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -18,6 +18,37 @@
  void, void>('textDocument/switchSourceHeader');
 }
 
+class FileStatus {
+private statuses = new Map();
+private readonly statusBarItem =
+vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left, 10);
+
+onFileUpdated(fileStatus: any) {
+  const filePath = vscode.Uri.parse(fileStatus.uri);
+  this.statuses.set(filePath.fsPath, fileStatus);
+  this.updateStatus();
+}
+
+updateStatus() {
+const path = vscode.window.activeTextEditor.document.fileName;
+const status = this.statuses.get(path);
+if (!status) {
+  this.statusBarItem.hide();
+  return;
+}
+this.statusBarItem.text = `clangd: ` + status.state;
+// FIXME: find a way to show the details in a nicer way.
+if (status.details.length > 0)
+this.statusBarItem.tooltip = status.details.map(
+(detail: any) => detail.message).join('; ');
+this.statusBarItem.show()
+}
+
+dispose() {
+this.statusBarItem.dispose();
+}
+}
+
 /**
  *  this method is called when your extension is activate
  *  your extension is activated the very first time the command is executed
@@ -80,4 +111,13 @@
 vscode.Uri.parse(sourceUri));
 vscode.window.showTextDocument(doc);
   }));
+  const status = new FileStatus();
+  context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(() => {
+  status.updateStatus();
+  }))
+  clangdClient.onReady().then(() => {
+  clangdClient.onNotification('textDocument/fileStatus', (fileStatus) => {
+  status.onFileUpdated(fileStatus)
+  });
+  })
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55363: [clangd] Expose FileStatus in LSP.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 176983.
hokein added a comment.

Update.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55363

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  test/clangd/filestatus.test

Index: test/clangd/filestatus.test
===
--- /dev/null
+++ test/clangd/filestatus.test
@@ -0,0 +1,14 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"int x; int y = x;"}}}
+#  CHECK:  "method": "textDocument/fileStatus",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:"details": [],
+# CHECK-NEXT:"state": "BuildingPreamble",
+# CHECK-NEXT:"uri": "{{.*}}/main.cpp"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -63,6 +63,8 @@
   // actions.
   };
   TUAction(State S, llvm::StringRef Name) : S(S), Name(Name){};
+  /// Returns a human readable string representation.
+  std::string toString() const;
   State S;
   /// The name of the action currently running, e.g. Update, GoToDef, Hover.
   /// Empty if we are in the idle state.
@@ -77,6 +79,8 @@
 /// Indicates whether we reused the prebuilt AST.
 bool ReuseAST = false;
   };
+  /// Serialize this to an LSP file status item.
+  FileStatus render(PathRef File) const;
 
   TUAction Action;
   BuildDetails Details;
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -739,6 +739,41 @@
   return HardwareConcurrency;
 }
 
+std::string TUAction::toString() const {
+  std::string Result;
+  raw_string_ostream OS(Result);
+  switch (S) {
+  case TUAction::Queued:
+OS << "Queued";
+break;
+  case TUAction::RunningAction:
+OS << "RunningAction"
+   << "(" << Name << ")";
+break;
+  case TUAction::BuildingPreamble:
+OS << "BuildingPreamble";
+break;
+  case TUAction::BuildingFile:
+OS << "BuildingFile";
+break;
+  case TUAction::Idle:
+OS << "Idle";
+break;
+  }
+  return OS.str();
+}
+
+FileStatus TUStatus::render(PathRef File) const {
+  FileStatus FStatus;
+  FStatus.uri = URIForFile::canonicalize(File, /*TUPath=*/File);
+  FStatus.state = Action.toString();
+  if (Details.BuildFailed)
+FStatus.details.push_back({MessageType::Error, "failed to build the file"});
+  if (Details.ReuseAST)
+FStatus.details.push_back({MessageType::Info, "reused the AST"});
+  return FStatus;
+}
+
 struct TUScheduler::FileData {
   /// Latest inputs, passed to TUScheduler::update().
   std::string Contents;
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -973,6 +973,33 @@
 };
 bool fromJSON(const llvm::json::Value &, ReferenceParams &);
 
+enum class MessageType {
+  Error = 1,
+  Warning = 2,
+  Info = 3,
+  Log = 4,
+};
+struct ShowMessageParams {
+  /// The message type.
+  MessageType type;
+  /// The actual message.
+  std::string message;
+};
+llvm::json::Value toJSON(const ShowMessageParams &);
+
+/// Clangd extension: indicates the current state of the file in clangd,
+/// sent from server via the `textDocument/fileStatus` notification.
+struct FileStatus {
+  /// The text document's URI.
+  URIForFile uri;
+  /// The human-readable string presents the current state of the file, can be
+  /// shown in the UI (e.g. status bar).
+  std::string state;
+  /// Details of the state that are worth sufacing to users.
+  std::vector details;
+};
+llvm::json::Value toJSON(const FileStatus &FStatus);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -716,6 +716,20 @@
   };
 }
 
+llvm::json::Value toJSON(const ShowMessageParams &S) {
+  return json::Object{
+  {"type", static_cast(S.type)}, {"message", S.message},
+  };
+}
+
+llvm::json::Value toJSON(const FileStatus &FStatus) {
+  return json::Object{
+  {"uri", FStatus.uri},
+  {"state", FStatus.state},
+  {"details", FStatus.details},
+  };
+}
+
 raw_ostream &operator<<(raw_ostream &O, const DocumentHighlight &V) {
   O << V.range;
   if (V.kind == DocumentHighlightKind::Read)
Index: clangd/ClangdLSPServer.h

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.
Herald added a subscriber: arphaman.

In D48116#1144732 , @ilya-biryukov 
wrote:

> Have you considered doing the same filtering in ASTUnit's 
> `StoredDiagnosticConsumer`? It should not be more difficult and allows to 
> avoid changing the clang's diagnostic interfaces. That's what we do in clangd.


Will check.

> I wonder if you want to handle notes and remarks in a special manner? They 
> can be seen as part of the original diagnostic, rather than the separate 
> diagnostic. E.g. showing a note in the main file, but not showing the 
> diagnostic from the headers file that this note comes from, might be 
> confusing to the users.

Looks like this case is handled fine.

WIthout the new option, a note is printed as expected/always (shortened output):

  $ bin/c-index-test -test-load-source function 
ignore-warnings-from-headers.cpp -Wunused-parameter
  ignore-warnings-from-headers.h:1:12: warning: unused parameter 
'unusedInHeader' [-Wunused-parameter]
  ignore-warnings-from-headers.cpp:1:10: note: in file included from 
ignore-warnings-from-headers.cpp:1:
  ignore-warnings-from-headers.cpp:3:12: warning: unused parameter 
'unusedInMainFile' [-Wunused-parameter]

With the new option, note is not printed and thus no confusion should arise:

  $ CINDEXTEST_IGNORE_NONERRORS_FROM_INCLUDED_FILES=1 bin/c-index-test 
-test-load-source function ignore-warnings-from-headers.cpp -Wunused-parameter
  ignore-warnings-from-headers.cpp:3:12: warning: unused parameter 
'unusedInMainFile' [-Wunused-parameter]

I will add "// CHECK-NOT: note: in file included from" to the test.

> Maybe also add tests for diagnostics in the main file with notes/remarks in 
> the header files and vice versa?

"Vice versa" is covered as shown above. If the diagnostic is in main file and a 
note of that one refers to the header, then the note should be shown/included. 
This case seems also fine - I've added a test for this.

In D48116#1144838 , @yvvan wrote:

> But this one misses a way to set this flag for everything except libclang. We 
> can probably change that (Nikolai is in vacation till the end of summer so 
> it's probably my part now) and add some tests outside of Index for it 
> (probably Frontend)


What's the use case of this?


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D55289: [analyzer] MoveChecker Pt.5: Improve invalidation policies.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:528
 ArrayRef ExplicitRegions,
 ArrayRef Regions, const LocationContext *LCtx,
 const CallEvent *Call) const {

NoQ wrote:
> Szelethus wrote:
> > This isn't specific to this revision, but I find the parameter name 
> > `Regions` way too vague. Maybe `ImplicitRegions`?
> How about these?
Awesome, thanks! :)



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:540-542
+// Explicit regions are the regions passed into the call directly, but
+// not all of them end up being invalidated. The ones that do appear in
+// the Regions array as well.

NoQ wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > Really? Then I guess this needs to be updated in 
> > > `CheckerDocumentation.cpp`:
> > > 
> > > ```
> > >   /// \param ExplicitRegions The regions explicitly requested for 
> > > invalidation.
> > >   ///For a function call, this would be the arguments. For a 
> > > bind, this
> > >   ///would be the region being bound to.
> > > ```
> > > 
> > > To me, this clearly indicates that the elements of `ExplicitRegions` will 
> > > be invalidated. Does "requested for" really just mean "requested for 
> > > potential"? Since this happens //before// any invalidation, it could 
> > > easily be interpreted as "invalidated after the end of the callback".
> > The callback fires after invalidation - cf. 
> > `ProgramState::invalidateRegionsImpl`. Note that the `if (Eng)` condition 
> > is always true, because how the heck were we supposed to run path-sensitive 
> > analysis without `ExprEngine`.
> > 
> > The terminology is really weird here because we the word "invalidation" has 
> > too many meanings. Essentially, `ExplicitRegions` are regions that were 
> > specifically requested for invalidation, but it is up to the `CallEvent` / 
> > `ProgramState` / `StoreManager` (depending on which `invalidateRegions()` 
> > was called) to decide what invalidation means in this case by filling in 
> > the `RegionAndSymbolInvalidationTraits` map.
> > 
> > For regions that represent values of const pointers/references directly 
> > passed into the function, `CallEvent` decides to set the 
> > `TK_PreserveContents` trait, which says "invalidate the region but keep its 
> > contents intact". It would still perform other forms of invalidation on 
> > that region, say, pointer escape: if there are pointer values written down 
> > somewhere within that region, checkers need to stop tracking them.
> > 
> > Now, the callback never said that it has something to do with 
> > "invalidation". Instead, it is about "region changes", which means changes 
> > of the region's contents in the Store. This doesn't necessarily happen due 
> > to invalidation; it may also happen while evaluating a simple assignment in 
> > the program, as we see in the newly added `testUpdateField()` test. And, as 
> > we've seen above, not every "invalidation" changes contents of the region.
> > 
> > Similarly, `TK_SuppressEscape` would suppress the `checkPointerEscape()` 
> > callback for the region, but will not suppress `checkRegionChanges()` for 
> > that (non-explicit) region.
> > 
> > Therefore we end up in a weird situation when some regions were requested 
> > to be invalidated but never were actually invalidated in terms of the 
> > Store. It kinda  makes sense, but i hate it anyway.  The 
> > `checkRegionChanges()` callback is hard to understand and hard to use and 
> > too general and has too much stuff stuffed into it. `checkPointerEscape` is 
> > an easier-to-use fork of it, but it doesn't cover the use case of checkers 
> > that track objects via regions. I suspect that the right solution here is 
> > to start tracking objects as "symbols", i.e., assign a unique opaque ID to 
> > every state through which every object in the program goes (regardless of 
> > which object it is) and use that as keys in the map. That should remove the 
> > stress of dealing with invalidation from C++ checkers that need to track 
> > objects opaquely. The problem won't magically disappear, as we still need 
> > to identify when exactly does the state change, but an additional level of 
> > indirection (Region -> ID -> CheckerState instead of just Region -> 
> > CheckerState) allows us to decompose it into smaller parts and de-duplicate 
> > some of this work.
> > 
> > But regardless of that, it is still weird that `ExplicitRegions` is not a 
> > sub-set of `Regions`. We need to either document it or fix it, and for some 
> > reason i prefer the latter. In particular, the only checker that actually 
> > actively acts upon `ExplicitRegions` when they're const is 
> > `RetainCountChecker`, but in fact people don't ever use `const` in stuff 
> > that it checks, it just isn't idiomatic.
> Another funny thing about `RegionAndSymbolInvalidationTraits`  i

[PATCH] D51554: [CUDA][OPENMP][NVPTX]Improve logic of the debug info support.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked 4 inline comments as done.
ABataev added inline comments.



Comment at: lib/Driver/ToolChains/Cuda.cpp:282-285
 enum DebugInfoKind {
-  NoDebug,   /// No debug info.
-  LineTableOnly, /// Line tables only.
-  FullDebug  /// Full debug info.
+  NoDebug, /// No debug info.
+  DebugDirectivesOnly, /// Line tables only.
+  FullDebug,   /// Full debug info.

echristo wrote:
> This enum doesn't appear to be complete? Either way can you make it match the 
> other and document what each thing means a bit more?
No, it is complete, but probably has some wrong names. I reworked it. Actually, 
this enum is intended to track the debug info emitted for the device. It may be 
disabled, debug directives only or same debug info as for the host.



Comment at: lib/Driver/ToolChains/Cuda.cpp:289
 
 static DebugInfoKind mustEmitDebugInfo(const ArgList &Args) {
+  const Arg *A = Args.getLastArg(options::OPT_O_Group);

echristo wrote:
> Please document this routine in prose.
Added description.



Comment at: lib/Driver/ToolChains/Cuda.cpp:706-708
+void CudaToolChain::adjustDebugInfoKind(
+codegenoptions::DebugInfoKind &DebugInfoKind, const ArgList &Args) const {
+  switch (mustEmitDebugInfo(Args)) {

echristo wrote:
> Is this really doing anything?
Yes, actually it does. Currently, when we need to emit the code for the device, 
we use the same debug info level just like for the host. But in some 
situations, we need to disable it or emit only debug directives for the device, 
while keeping the original debug info for the host. This function allows us to 
change the debug info level for the device and force clang to emit required 
debug info data during codegen for the NVPTX devices.


Repository:
  rC Clang

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

https://reviews.llvm.org/D51554



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

[PATCH] D51554: [CUDA][OPENMP][NVPTX]Improve logic of the debug info support.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev updated this revision to Diff 176993.
ABataev marked an inline comment as done.
ABataev added a comment.

Reworked according to the latest comments from Eric.


Repository:
  rC Clang

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

https://reviews.llvm.org/D51554

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Cuda.cpp
  lib/Driver/ToolChains/Cuda.h
  test/Driver/cuda-dwarf-2.cu
  test/Driver/openmp-offload-gpu.c

Index: test/Driver/openmp-offload-gpu.c
===
--- test/Driver/openmp-offload-gpu.c
+++ test/Driver/openmp-offload-gpu.c
@@ -190,29 +190,35 @@
 // CHK-BCLIB-WARN: No library 'libomptarget-nvptx-sm_20.bc' found in the default clang lib directory or in LIBRARY_PATH. Expect degraded performance due to no inlining of runtime functions on target devices.
 
 /// Check that debug info is emitted in dwarf-2
-// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -g -O0 --no-cuda-noopt-device-debug 2>&1 \
-// RUN:   | FileCheck -check-prefix=NO_DEBUG %s
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -g -O1 --no-cuda-noopt-device-debug 2>&1 \
+// RUN:   | FileCheck -check-prefix=DEBUG_DIRECTIVES %s
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -g -O3 2>&1 \
-// RUN:   | FileCheck -check-prefix=NO_DEBUG %s
+// RUN:   | FileCheck -check-prefix=DEBUG_DIRECTIVES %s
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -g -O3 --no-cuda-noopt-device-debug 2>&1 \
-// RUN:   | FileCheck -check-prefix=NO_DEBUG %s
+// RUN:   | FileCheck -check-prefix=DEBUG_DIRECTIVES %s
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -g0 2>&1 \
 // RUN:   | FileCheck -check-prefix=NO_DEBUG %s
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -ggdb0 -O3 --cuda-noopt-device-debug 2>&1 \
 // RUN:   | FileCheck -check-prefix=NO_DEBUG %s
-// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -gline-tables-only 2>&1 \
-// RUN:   | FileCheck -check-prefix=NO_DEBUG -check-prefix=LINE_TABLE %s
-// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -ggdb1 -O2 --cuda-noopt-device-debug 2>&1 \
-// RUN:   | FileCheck -check-prefix=NO_DEBUG -check-prefix=LINE_TABLE %s
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -gline-directives-only 2>&1 \
+// RUN:   | FileCheck -check-prefix=DEBUG_DIRECTIVES %s
 
-// LINE_TABLE-NOT: warning: debug
+// DEBUG_DIRECTIVES-NOT: warning: debug
 // NO_DEBUG-NOT: warning: debug
+// NO_DEBUG: "-fopenmp-is-device"
+// NO_DEBUG-NOT: "-debug-info-kind=
 // NO_DEBUG: ptxas
-// LINE_TABLE: "-lineinfo"
+// DEBUG_DIRECTIVES: "-triple" "nvptx64-nvidia-cuda"
+// DEBUG_DIRECTIVES-SAME: "-debug-info-kind=line-directives-only"
+// DEBUG_DIRECTIVES-SAME: "-fopenmp-is-device"
+// DEBUG_DIRECTIVES: ptxas
+// DEBUG_DIRECTIVES: "-lineinfo"
 // NO_DEBUG-NOT: "-g"
 // NO_DEBUG: nvlink
 // NO_DEBUG-NOT: "-g"
 
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -g -O0 --no-cuda-noopt-device-debug 2>&1 \
+// RUN:   | FileCheck -check-prefix=HAS_DEBUG %s
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -g 2>&1 \
 // RUN:   | FileCheck -check-prefix=HAS_DEBUG %s
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -g -O0 --cuda-noopt-device-debug 2>&1 \
@@ -227,9 +233,14 @@
 // RUN:   | FileCheck -check-prefix=HAS_DEBUG %s
 // RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -ggdb3 -O2 --cuda-noopt-device-debug 2>&1 \
 // RUN:   | FileCheck -check-prefix=HAS_DEBUG %s
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -gline-tables-only 2>&1 \
+// RUN:   | FileCheck -check-prefix=HAS_DEBUG %s
+// RUN:   %clang -### -no-canonical-prefixes -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -Xopenmp-target -march=sm_60 %s -ggdb1 -O2 --cuda-noopt-device-debug 2>&1 \
+// RUN:   | FileCheck -check-prefix=HAS_DEBUG %s
 
 // HAS_DEBUG-NOT: warning: debug
 // HAS_DEBUG: "-triple" "nvptx64-nvidia-cuda"
+// HAS_DEB

[PATCH] D55270: [Sema] Further improvements to to static_assert diagnostics.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/PCH/cxx-static_assert.cpp:17
 
-// expected-error@12 {{static_assert failed "N is not 2!"}}
+// expected-error@12 {{static_assert failed due to requirement '1 == 2' "N is 
not 2!"}}
 T<1> t1; // expected-note {{in instantiation of template class 'T<1>' 
requested here}}

I'm not certain how I feel about now printing the failure condition when 
there's an explicit message provided. From what I understand, a fair amount of 
code in the wild does `static_assert(some_condition, "some_condition")` because 
of older language modes where the message was not optional. I worry we're going 
to start seeing a lot of diagnostics like: `static_assert failed due to 
requirement '1 == 2' "N == 2"`, which seems a bit low-quality. See 
`DFAPacketizer::DFAPacketizer()` in LLVM as an example of something similar.

Given that the user entered a message, do we still want to show the 
requirement? Do we feel the same way if the requirement is fairly large?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55270



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321046 , @kristina wrote:

> Personally I'm against this type of warning as it's likely anyone using 
> `-mllvm` is actually intending to adjust certain behaviors across one or more 
> passes with a lot of switches supported by it being intentionally hidden from 
> ` --help` output requiring explicitly specifying that hidden flags 
> be shown.


There is a cost to having people encode these flags into their build systems -- 
it can then cause issues if we ever change these internal flags. I do not think 
any Clang maintainer intends to support these as stable APIs, unlike most of 
the driver command-line. But, neither -Xclang nor -mllvm obviously scream out 
"don't use this!", and so people may then add them to their buildsystems 
without causing reviewers to say "Wait...really? Are you sure that's a good 
idea?".

That's why I think a warning is useful -- it'll discourage people from using 
them when they haven't properly understand the consequences, but does not 
prevent them from doing so, when they actually do.

> For example, I routinely use the following with SEH (excuse some of the 
> naming, this is just a downstream fork however):
>  `-mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm 
> -wtfabi-opts=0x1EF77F`

If you already are passing that, do you see a problem with instead passing
 `-mllvm -target-enable-seh=true -mllvm -force-msvc-seh=true -mllvm 
-wtfabi-opts=0x1EF77F -Wno-experimental-driver-option`
?


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

https://reviews.llvm.org/D55150



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

[PATCH] D55269: [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu

[Artem Belevich via Phabricator via cfe-commits]

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

LGTM.


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

https://reviews.llvm.org/D55269



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

r348504 - [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu

[Joel E. Denny via cfe-commits]

Author: jdenny
Date: Thu Dec  6 09:46:17 2018
New Revision: 348504

URL: http://llvm.org/viewvc/llvm-project?rev=348504&view=rev
Log:
[CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu

This just extends D40453 (r319317) to Ubuntu.

Reviewed By: Hahnfeld, tra

Differential Revision: https://reviews.llvm.org/D55269

Modified:
cfe/trunk/lib/Driver/ToolChains/Cuda.cpp

Modified: cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Cuda.cpp?rev=348504&r1=348503&r2=348504&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Cuda.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Cuda.cpp Thu Dec  6 09:46:17 2018
@@ -114,7 +114,7 @@ CudaInstallationDetector::CudaInstallati
 for (const char *Ver : Versions)
   Candidates.emplace_back(D.SysRoot + "/usr/local/cuda-" + Ver);
 
-if (Distro(D.getVFS()).IsDebian())
+if (Distro(D.getVFS()).IsDebian() || Distro(D.getVFS()).IsUbuntu())
   // Special case for Debian to have nvidia-cuda-toolkit work
   // out of the box. More info on http://bugs.debian.org/882505
   Candidates.emplace_back(D.SysRoot + "/usr/lib/cuda");


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

[PATCH] D55337: NFC: Move dumpDeclRef to NodeDumper

[Stephen Kelly via Phabricator via cfe-commits]

steveire marked an inline comment as done.
steveire added inline comments.



Comment at: include/clang/AST/TextNodeDumper.h:28
const comments::FullComment *> {
+  TextTreeStructure &TreeStructure;
   raw_ostream &OS;

aaron.ballman wrote:
> This makes me a bit wary because you create a node dumper in the same 
> situations you make a tree structure object, but now there's a strict 
> ordering between the two object creations. If you're doing this construction 
> local to a function, you wind up with a dangling reference unless you're 
> careful (which is unfortunate, but not the end of the world). If you're doing 
> this construction as part of a constructor's initializer list, you now have 
> to properly order the member declarations within the class and that is also 
> unfortunate. Given that those are the two common scenarios for how I envision 
> constructing an ast dump of some kind, I worry about the fragility. e.g.,
> ```
> unique_ptr createASTDumper(...) {
>   TextTreeStructure TreeStructure;
>   TextNodeDumper NodeDumper(TreeStructure); // Oops, dangling reference
>   return make_unique(TreeStructure, NodeDumper, ...);
> }
> 
> // vs
> 
> struct MySuperAwesomeASTDumper : ... {
>   MySuperAwesomeASTDumper() : TreeStructure(...), NodeDumper(TreeStructure, 
> ...) {}
> private:
>   TextTreeStructure TreeStructure; // This order is now SUPER important
>   TextNodeDumper NodeDumper;
> };
> ```
> There's a part of me that wonders if a better approach is to have this object 
> passed to the `dumpFoo()` calls as a reference parameter. This way, the 
> caller is still responsible for creating an object, but the creation order 
> between the tree and the node dumper isn't as fragile.
In your first snippet there is a dangling reference because the author of 
`MySuperAwesomeASTDumper` decided to make the members references. If the 
members are references, code like your first snippet will cause dangling 
references and nothing can prevent that. Adding `TreeStructure&` to Visit 
methods as you suggested does not prevent it.

The only solution is make the `MySuperAwesomeASTDumper` not use member 
references (ie your second snippet). The order is then in fact not problematic 
because "taking a reference to an uninitialized object is legal".


Repository:
  rC Clang

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

https://reviews.llvm.org/D55337



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

[PATCH] D55269: [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348504: [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu 
(authored by jdenny, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55269?vs=176916&id=176999#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55269

Files:
  cfe/trunk/lib/Driver/ToolChains/Cuda.cpp


Index: cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
@@ -114,7 +114,7 @@
 for (const char *Ver : Versions)
   Candidates.emplace_back(D.SysRoot + "/usr/local/cuda-" + Ver);
 
-if (Distro(D.getVFS()).IsDebian())
+if (Distro(D.getVFS()).IsDebian() || Distro(D.getVFS()).IsUbuntu())
   // Special case for Debian to have nvidia-cuda-toolkit work
   // out of the box. More info on http://bugs.debian.org/882505
   Candidates.emplace_back(D.SysRoot + "/usr/lib/cuda");


Index: cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Cuda.cpp
@@ -114,7 +114,7 @@
 for (const char *Ver : Versions)
   Candidates.emplace_back(D.SysRoot + "/usr/local/cuda-" + Ver);
 
-if (Distro(D.getVFS()).IsDebian())
+if (Distro(D.getVFS()).IsDebian() || Distro(D.getVFS()).IsUbuntu())
   // Special case for Debian to have nvidia-cuda-toolkit work
   // out of the box. More info on http://bugs.debian.org/882505
   Candidates.emplace_back(D.SysRoot + "/usr/lib/cuda");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Hey Alexey,

Here is what I found so far:
The new assertion is this:

  Assertion failed: (ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD)), 
function ImportDeclContext

ToRD looks like this in code:

  typedef struct __sFILE {
 // several fields here ...
  } FILE;

And the AST:

  CXXRecordDecl 0x7f87ce97d488 
 col:16 struct __sFILE definition
  |-DefinitionData pass_in_registers aggregate standard_layout 
trivially_copyable pod trivial literal
  | |-DefaultConstructor exists trivial needs_implicit
  | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial needs_implicit
  |-CXXRecordDecl 0x7f87d1886000  col:16 implicit struct __sFILE
  | `-
  `-

I dug deeper and this is because `ToRD` is not equal with 
`ToD->getDeclContext()`.
However, if I load the fields of `ToRD` and then dump it seems to be 
structurally equivalent with `ToD->getDeclContext()`.
Both `ToRD` and `ToD->getDeclContext()` are part of a Decl chain with one 
element, this is true for `FromRD` and `D->getDeclContext()`. So I suspect some 
sort of lookup problem here: first we import the declContext during the import 
of a field, then when we do `auto ImportedDC = import(cast(FromDC));` 
where we fail to find the first imported DeclContext. I suspect that this is 
because typedef hides the real struct, I mean `__sFILE` seems to be not 
find-able by normal C lookup (ie. it is hidden).

Here is one workaround (I did not try it yet):
We could lazy initialize `ToRD` inside the for loop to be equal to the first 
'ToD`'s decl context.

  RecordDecl *ToRD = nullptr;
  for (auto *D : FromRD->decls()) {
if (isa(D) || isa(D)) {
  Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
  if (!ToRD)
  ToRD = ToD->getDeclContext();
  ToRD->removeDecl(ToD);
}
  }

I am afraid I can continue with the debug only next week.


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100



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

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177002.
Szelethus marked 32 inline comments as done.
Szelethus added a reviewer: MTC.
Szelethus added a comment.

Addressing inline comments.


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

https://reviews.llvm.org/D54823

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/malloc-annotations.c
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
Index: test/Analysis/malloc-annotations.c
===
--- test/Analysis/malloc-annotations.c
+++ test/Analysis/malloc-annotations.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc -analyzer-store=region -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize,unix.Malloc \
+// RUN:   -analyzer-config unix.Malloc:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,28 +87,59 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily getAllocationFamily(
+const MemFunctionInfoTy &MemFunctionInfo, CheckerContext &C, const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D55262#1321048 , @romanovvlad wrote:

> I'm getting the same IR for both examples you provided. And for both load of 
> X value is aligned on 4 bytes wo patch and on 1 byte with.


That's odd, because the natural type alignment for `A` ought to be 1, since 
it's a packed type.  What does the AST look like?


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

https://reviews.llvm.org/D55262



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

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

NoQ wrote:
> Szelethus wrote:
> > MTC wrote:
> > > Szelethus wrote:
> > > > MTC wrote:
> > > > > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > > > > `MallocChecker` that hold a bunch of memory function identifiers and 
> > > > > provide corresponding helper APIs. And we should pick a proper time 
> > > > > to initialize it.
> > > > > 
> > > > > I want to known why you call `initIdentifierInfo()` in 
> > > > > `checkPostStmt(const CallExpr *E,)`, this callback is called after 
> > > > > `checkPreCall(const CallEvent &Call, )` in which we need 
> > > > > `MemFunctionInfo`.
> > > > Well, I'd like to know too! I think lazy initializing this struct 
> > > > creates more problems that it solves, but I wanted to draw a line 
> > > > somewhere in terms of how invasive I'd like to make this patch.
> > > How about put the initialization stuff into constructor? Let the 
> > > constructor do the initialization that it should do, let `register*()` do 
> > > its registration, like setting `setOptimism` and so on.
> > Interestingly, `MemFunctionInfo` is not always initialized, so a 
> > performance argument can be made here. I specifically checked whether 
> > there's any point in doing this hackery, and the answer is... well, some. 
> > I'll probably touch on these in a followup patch.
> Lazy initialization of identifiers is kinda perceived as a fairly worthless 
> optimization. Hence `CallDescription`.
> 
> Also it cannot be done during registration because the AST is not constructed 
> yet at this point. Well, probably it can be done anyway because the empty 
> identifier table is already there in the `ASTContext` and we can always 
> eagerly add a few items into it, but it still sounds scary.
I would personally prefer to risk initializing these during registration, but 
if we're this many comments into this discussion, then I believe it deserves a 
separate patch.

I'll leave a `TODO:` in the code.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
 

Szelethus wrote:
> MTC wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > I'm not sure `hasNonTrivialConstructorCall()` is a good name although 
> > > > you explained it in details in the comments below. And the comments for 
> > > > this function is difficult for me to understand, which is maybe my 
> > > > problem. 
> > > > 
> > > > And I also think this function doesn't do as much as described in the 
> > > > comments, it is just `true if the invoked constructor in \p NE has a 
> > > > parameter - pointer or reference to a record`, right?
> > > I admit that I copy-pasted this from the source I provided below, and I 
> > > overlooked it before posting it here. I //very// much prefer what you 
> > > proposed :)
> > The comments you provided is from the summary of the patch [[ 
> > https://reviews.llvm.org/D4025 | D4025 ]], ayartsev has not updated the 
> > summary information in time to adapt to his patch, so I think it is 
> > appropriate to extract the summary information when he submitted this 
> > patch, see [[ 
> > https://github.com/llvm-mirror/clang/commit/2e61d2893934f7b91ca9e2f889a4e4cc9939b05c#diff-ff9160d4628cb9b6186559837c2c8668
> >  | [Analyzer] fix for PR19102 ]]. What do you think?
> Wow, nice detective work there :D Sounds good to me.
Umm, yea, I really couldn't come up with a better name. Hopefully the comments 
both here and at the call site help on this.


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

https://reviews.llvm.org/D54823



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[Romanov Vlad via Phabricator via cfe-commits]

romanovvlad added a comment.

Yes it's 1, but after addrspacecast alignment becomes not struct A but 
alignment of the pointer


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

https://reviews.llvm.org/D55262



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

[PATCH] D55269: [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D55269#1320382 , @tra wrote:

> >> Let's start with fixing OpenMP's cmake files. Once it no longer insists on 
> >> specifying --cuda-path=/usr, and isUbuntu is in place, what is the 
> >> remaining failure that you see?
> > 
> > I disagree here: It's not OpenMP but CMake that's detecting the wrong path 
> > here. This is the place to fix it once and for all (and possibly get the 
> > shim package right in that process).
>
> It's somewhat orthogonal, IMO. I agree that it's not OpenMP's problem. Cmake 
> will fix it at some point in future, but, presumably, we want OpenMP 
> buildable *now*. Adding a temporary workaround for something that Cmake can't 
> handle now would make building OpenMP with clang somewhat easier which was 
> the end goal of this patch. In the end it's up to OpenMP maintainers what 
> they would want to do.


I don't know when I'll explore the cmake problem further.  If someone else 
decides to, please cc/subscribe me.

Thanks for the reviews.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55269



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

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > Szelethus wrote:
> > > > > MTC wrote:
> > > > > > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > > > > > `MallocChecker` that hold a bunch of memory function identifiers 
> > > > > > and provide corresponding helper APIs. And we should pick a proper 
> > > > > > time to initialize it.
> > > > > > 
> > > > > > I want to known why you call `initIdentifierInfo()` in 
> > > > > > `checkPostStmt(const CallExpr *E,)`, this callback is called after 
> > > > > > `checkPreCall(const CallEvent &Call, )` in which we need 
> > > > > > `MemFunctionInfo`.
> > > > > Well, I'd like to know too! I think lazy initializing this struct 
> > > > > creates more problems that it solves, but I wanted to draw a line 
> > > > > somewhere in terms of how invasive I'd like to make this patch.
> > > > How about put the initialization stuff into constructor? Let the 
> > > > constructor do the initialization that it should do, let `register*()` 
> > > > do its registration, like setting `setOptimism` and so on.
> > > Interestingly, `MemFunctionInfo` is not always initialized, so a 
> > > performance argument can be made here. I specifically checked whether 
> > > there's any point in doing this hackery, and the answer is... well, some. 
> > > I'll probably touch on these in a followup patch.
> > Lazy initialization of identifiers is kinda perceived as a fairly worthless 
> > optimization. Hence `CallDescription`.
> > 
> > Also it cannot be done during registration because the AST is not 
> > constructed yet at this point. Well, probably it can be done anyway because 
> > the empty identifier table is already there in the `ASTContext` and we can 
> > always eagerly add a few items into it, but it still sounds scary.
> I would personally prefer to risk initializing these during registration, but 
> if we're this many comments into this discussion, then I believe it deserves 
> a separate patch.
> 
> I'll leave a `TODO:` in the code.
Or just go with `CallDescription`.


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

https://reviews.llvm.org/D54823



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[John McCall via Phabricator via cfe-commits]

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

LGTM, but you can probably undo one of my requests. :)




Comment at: lib/AST/Type.cpp:2950
+   FunctionTypeBits.HasExtQuals = 0;
+  }
 }

mikael wrote:
> mikael wrote:
> > rjmccall wrote:
> > > The indentation here is messed up.
> > > 
> > > You seem to be mixing up "fast qualifiers" with "CVR qualifiers" in a few 
> > > places.  That's currently correct, in the sense that the fast qualifiers 
> > > are currently defined to be the CVR qualifiers, but the point of the 
> > > distinction is that we might want to change that (and we have in fact 
> > > explored that in the past).  I think you should make `FunctionTypeBits` 
> > > only claim to store the fast qualifiers, which mostly just means updating 
> > > a few field / accessor names and changing things like the 
> > > `getCVRQualifiers()` call right above this to be `getFastQualifiers()`.
> > > 
> > > If there are places where it's convenient to assume that the CVR 
> > > qualifiers are exactly the fast qualifiers, it's okay to static_assert 
> > > that, but you should make sure to static_assert it in each place you 
> > > assume it.
> > I'll change it, but I do have a question related to this:
> > 
> > Can we make any assumptions with the "fast qualifiers"? I'd like it if we 
> > can assume that they fit in 3 bits. Otherwise I think I also need an 
> > assertion here.
> I solved it like this in the end:
> 
> * I defined the FastTypeQuals to get the size of Qualifiers::FastWidth to 
> make it dependent on possible changes to the fast flags.
> * Then I put a static_assertion to guard the hastConst(), hasVolatile() and 
> hasRestrict() methods.
Great, looks good.



Comment at: lib/CodeGen/CGDebugInfo.cpp:2593
+  getOrCreateInstanceMethodType(
+  CXXMethodDecl::getThisType(FPT, Ty->getMostRecentCXXRecordDecl()),
+  FPT, U),

mikael wrote:
> rjmccall wrote:
> > Why is this `getMostRecentCXXRecordDecl` instead of `getClass`?
> Your initial comment suggested to send in a record rather than a type. But I 
> see now that it may make more sense to use the type directly instead. Will 
> update it.
Oh, I actually didn't even think about the fact that the `getClass())` returns 
a `Type*` instead of a `CXXRecordDecl`.  Using the record decl is probably 
better, sorry.


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

https://reviews.llvm.org/D54862



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D55262#1321641 , @romanovvlad wrote:

> Yes it's 1, but after addrspacecast alignment becomes not struct A but 
> alignment of the pointer


Oh!  Yes, that's even more broken than I thought.




Comment at: lib/CodeGen/CGExpr.cpp:4272
 *this, LV.getPointer(), E->getSubExpr()->getType().getAddressSpace(),
 DestTy.getAddressSpace(), ConvertType(DestTy));
+return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),

This call to `getAddressSpace()` doesn't look right: we just constructed 
`DestTy` as an unqualified pointer type.  It needs to be 
`E->getType().getAddressSpace()`.


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

https://reviews.llvm.org/D55262



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

[PATCH] D55371: Fix thunks returning memptrs via sret by emitting also scalar return values directly in sret slot (PR39901)

[John McCall via Phabricator via cfe-commits]

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

That seems reasonable.


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

https://reviews.llvm.org/D55371



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

[PATCH] D42682: [clang-tidy] Add io-functions-misused checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/IoFunctionsCheck.cpp:32
+has(cxxMemberCallExpr(
+on(hasType(namedDecl(hasAnyName("istream",
+callee(cxxMethodDecl(hasName("get")).bind("DeclOfGet"),

`::std::istream`



Comment at: clang-tidy/bugprone/IoFunctionsCheck.cpp:47-49
+  "consider to cast the return value of %0 from type integer to type char, 
"
+  "possible loss of precision if an error has occurred or the end "
+  "of file has been reached");

This diagnostic confuses me, so perhaps you can explain the situation you want 
to diagnose a bit further.

FIO34-C is about situations where `sizeof(char) == sizeof(int)` and the call 
returns EOF/WEOF. In that case, there's no way to distinguish between an 
EOF/error and a valid character. Suggesting to explicitly cast to a character 
type doesn't add any safety to the code -- the user needs to insert calls to 
`feof()` or `ferror()` instead (to make it portable, anyway) and should store 
the character value in a sufficiently large integer type before doing the 
comparison against EOF.

Are you trying to catch a different kind of bug?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D42682



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[Romanov Vlad via Phabricator via cfe-commits]

romanovvlad added a comment.

Backtrace: F7656050: stack 


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

https://reviews.llvm.org/D55262



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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Sorry.  Can you post the result of passing `-ast-dump` instead of `-emit-llvm` 
to the compiler?


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

https://reviews.llvm.org/D55262



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

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 177005.
kadircet added a comment.

- Fix a few problems that come up in the field test.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55224

Files:
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/index/Background.cpp
  clangd/index/Background.h
  clangd/index/BackgroundIndexStorage.cpp
  clangd/index/SymbolCollector.cpp
  test/clangd/background-index.test
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -7,6 +7,7 @@
 
 using testing::_;
 using testing::AllOf;
+using testing::Contains;
 using testing::Not;
 using testing::UnorderedElementsAre;
 
@@ -125,7 +126,7 @@
FileURI("unittest:///root/B.cc")}));
 }
 
-TEST_F(BackgroundIndexTest, ShardStorageWriteTest) {
+TEST_F(BackgroundIndexTest, ShardStorageTest) {
   MockFSProvider FS;
   FS.Files[testPath("root/A.h")] = R"cpp(
   void common();
@@ -154,6 +155,16 @@
   EXPECT_EQ(CacheHits, 0U);
   EXPECT_EQ(Storage.size(), 2U);
 
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+  EXPECT_EQ(Storage.size(), 2U);
+
   auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
   EXPECT_NE(ShardHeader, nullptr);
   EXPECT_THAT(
@@ -221,5 +232,73 @@
   EmptyIncludeNode());
 }
 
+TEST_F(BackgroundIndexTest, ShardStorageLoad) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+
+  // Change header.
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  class A_CCnew {};
+  )cpp";
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardHeader->Symbols, Contains(Named("A_CCnew")));
+
+  // Change source.
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }\nvoid f_b() {}";
+  {
+CacheHits = 0;
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+  EXPECT_EQ(CacheHits, 2U); // Check both A.cc and A.h loaded from cache.
+
+  // Check if the new symbol has arrived.
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_NE(ShardHeader, nullptr);
+  EXPECT_THAT(*ShardSource->Symbols,
+  Contains(AllOf(Named("f_b"), Declared(), Defined(;
+}
+
 } // namespace clangd
 } // namespace clang
Index: test/clangd/background-index.test
===
--- test/clangd/background-index.test
+++ test/clangd/background-index.test
@@ -16,6 +16,5 @@
 # RUN: ls %t/.clangd-index/foo.cpp.*.idx
 
 # Test the index is read from disk: delete code and restart clangd.
-# FIXME: This test currently fails as we don't read the index yet.
 # RUN: rm %t/foo.cpp
-# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | not FileCheck %t/definition.jsonrpc
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/Symbo

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D55262#1321682 , @rjmccall wrote:

> Sorry.  Can you post the result of passing `-ast-dump` instead of 
> `-emit-llvm` to the compiler?


Actually, nevermind, I don't need that.  If you can fix the address-space issue 
I pointed out, I think this is ready to go.


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

https://reviews.llvm.org/D55262



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

[PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added a reviewer: rnk.

The flag -fdebug-compilation-dir is useful to make generated .o files 
independent of the path of the build directory, without making the compile 
command-line dependent on the path of the build directory, like 
-fdebug-prefix-map requires. This change makes it so that the driver can 
forward the flag to -cc1as, like it already can for -cc1. We might want to 
consider making -fdebug-compilation-dir a driver flag in a follow-up.

(Since -fdebug-compilation-dir defaults to PWD, it's already possible to get 
this effect by setting PWD, but explicit compiler flags are better than env 
vars, because e.g. ninja tracks command lines and reruns commands that change.)

Somewhat related to PR14625.


https://reviews.llvm.org/D55377

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/integrated-as.s


Index: test/Driver/integrated-as.s
===
--- test/Driver/integrated-as.s
+++ test/Driver/integrated-as.s
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 
2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s 
-Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler 
-fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck 
--check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2152,6 +2152,9 @@
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;


Index: test/Driver/integrated-as.s
===
--- test/Driver/integrated-as.s
+++ test/Driver/integrated-as.s
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler -fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck --check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2152,6 +2152,9 @@
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

> There is a cost to having people encode these flags into their build systems 
> -- it can then cause issues if we ever change these internal flags. I do not 
> think any Clang maintainer intends to support these as stable APIs, unlike 
> most of the driver command-line. But, neither -Xclang nor -mllvm obviously 
> scream out "don't use this!", and so people may then add them to their 
> buildsystems without causing reviewers to say "Wait...really? Are you sure 
> that's a good idea?".

This is why I proposed a compromise, allow this warning to be disabled 
completely for people actively using those flags, which are pretty much 
exclusively toolchain developers (well basically what I proposed, since it's 
not clear what counts as a build made by someone who is working and debugging a 
pass, being fully aware of those flags, using the subset of them specific to 
that pass/feature, I would say assertion+dump enabled builds are closest, but 
having an explicit build flag for it would be nicer). It's more unified than 
everyone either adding workarounds into build systems or disabling it 
completely (by just removing it).

Alternatively just let people shoot themselves in the foot, a documentation 
change regarding the dangers of the flag should suffice. Besides, I don't think 
this really ever surfaced as an issue, until Chandler's idea with regards to an 
unsupressable warning for performance tests for 7.x.x (which is a very specific 
and narrow edge case to allow people to collect performance data). Outside of 
that very specific case, have we really had many issues with consumers 
accidentally setting weird flags that they would have to discover in the first 
place. I don't have a strong opinion on `-Xclang` but `-mllvm ` is 
verbose enough to use as is. Maybe it should be made a lot more obvious in one 
way or another but a warning by default seems like it's taking rather drastic 
preemptive measures against a non-issue (do correct me if I'm wrong here).

I do agree that the documentation should definitely scream that it's not stable 
and it's intended for maintainers since it's a convinient interface (the 
`-mllvm` one) for passing flags through the driver all the way to things like 
the MC layer, where needed, and yes these flags can be removed without notice, 
but again, I don't think it's our responsibility to protect users from using 
*intentionally* hidden flags aside from documenting the reason for why they're 
intentionally hidden in a more obvious way, I think the fact that they are 
intentionally not shown in standard LLVM help and yet are available contradicts 
the idea behind this patch, the whole purpose of the flag is to directly 
interact with specific internal parts of LLVM which in itself is not really 
intended to be a stable interface.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 177007.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Delete redundant comment


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55315

Files:
  clangd/Threading.cpp
  clangd/Threading.h
  clangd/index/Background.cpp
  clangd/index/Background.h

Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -13,6 +13,7 @@
 #include "Context.h"
 #include "FSProvider.h"
 #include "GlobalCompilationDatabase.h"
+#include "Threading.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
@@ -110,7 +111,7 @@
   // queue management
   using Task = std::function;
   void run(); // Main loop executed by Thread. Runs tasks from Queue.
-  void enqueueTask(Task T);
+  void enqueueTask(Task T, ThreadPriority Prioirty);
   void enqueueLocked(tooling::CompileCommand Cmd,
  BackgroundIndexStorage *IndexStorage);
   std::mutex QueueMu;
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -22,6 +22,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SHA1.h"
@@ -104,11 +105,6 @@
   assert(this->IndexStorageFactory && "Storage factory can not be null!");
   while (ThreadPoolSize--) {
 ThreadPool.emplace_back([this] { run(); });
-// Set priority to low, since background indexing is a long running task we
-// do not want to eat up cpu when there are any other high priority threads.
-// FIXME: In the future we might want a more general way of handling this to
-// support tasks with various priorities.
-setThreadPriority(ThreadPool.back(), ThreadPriority::Low);
   }
 }
 
@@ -160,44 +156,61 @@
 }
 
 void BackgroundIndex::enqueue(const std::vector &ChangedFiles) {
-  enqueueTask([this, ChangedFiles] {
-trace::Span Tracer("BackgroundIndexEnqueue");
-// We're doing this asynchronously, because we'll read shards here too.
-// FIXME: read shards here too.
-
-log("Enqueueing {0} commands for indexing", ChangedFiles.size());
-SPAN_ATTACH(Tracer, "files", int64_t(ChangedFiles.size()));
-
-// We shuffle the files because processing them in a random order should
-// quickly give us good coverage of headers in the project.
-std::vector Permutation(ChangedFiles.size());
-std::iota(Permutation.begin(), Permutation.end(), 0);
-std::mt19937 Generator(std::random_device{}());
-std::shuffle(Permutation.begin(), Permutation.end(), Generator);
-
-for (const unsigned I : Permutation)
-  enqueue(ChangedFiles[I]);
-  });
+  enqueueTask(
+  [this, ChangedFiles] {
+trace::Span Tracer("BackgroundIndexEnqueue");
+// We're doing this asynchronously, because we'll read shards here too.
+// FIXME: read shards here too.
+
+log("Enqueueing {0} commands for indexing", ChangedFiles.size());
+SPAN_ATTACH(Tracer, "files", int64_t(ChangedFiles.size()));
+
+// We shuffle the files because processing them in a random order should
+// quickly give us good coverage of headers in the project.
+std::vector Permutation(ChangedFiles.size());
+std::iota(Permutation.begin(), Permutation.end(), 0);
+std::mt19937 Generator(std::random_device{}());
+std::shuffle(Permutation.begin(), Permutation.end(), Generator);
+
+for (const unsigned I : Permutation)
+  enqueue(ChangedFiles[I]);
+  },
+  ThreadPriority::Normal);
 }
 
 void BackgroundIndex::enqueue(const std::string &File) {
   ProjectInfo Project;
   if (auto Cmd = CDB.getCompileCommand(File, &Project)) {
 auto *Storage = IndexStorageFactory(Project.SourceRoot);
+// Set priority to low, since background indexing is a long running
+// task we do not want to eat up cpu when there are any other high
+// priority threads.
 enqueueTask(Bind(
-[this, File, Storage](tooling::CompileCommand Cmd) {
-  Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
-  if (auto Error = index(std::move(Cmd), Storage))
-log("Indexing {0} failed: {1}", File, std::move(Error));
-},
-std::move(*Cmd)));
+[this, File, Storage](tooling::CompileCommand Cmd) {
+  Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
+  if (auto Error = index(std::move(Cmd), Storage))
+log("Indexing {0} failed: {1}", File, std::move(Error));
+},
+std::move(*Cmd)),
+Thr

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/index/Background.cpp:202
 std::lock_guard Lock(QueueMu);
-Queue.push_back(std::move(T));
+if (Priority == ThreadPriority::Low) {
+  Queue.push_back(Bind(

ilya-biryukov wrote:
> Since we might be interested in scheduling higher-priority tasks first anyway 
> (not in this patch, but still), maybe store a pair of `(Task, Priority)` in 
> the queue and call `setCurrentThreadPriority` when actually running the task?
I believe we can introduce that later on whenever we have more than 2 
priorities, currently we push to front for important tasks and back for the low 
priority ones. Do you think it is not enough?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55315



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

r348513 - Reapply "Avoid emitting redundant or unusable directories in DIFile metadata entries.""

[Adrian Prantl via cfe-commits]

Author: adrian
Date: Thu Dec  6 10:44:50 2018
New Revision: 348513

URL: http://llvm.org/viewvc/llvm-project?rev=348513&view=rev
Log:
Reapply "Avoid emitting redundant or unusable directories in DIFile metadata 
entries.""

This reverts commit r348280 and reapplies D55085 without modifications.

Original commit message:

Avoid emitting redundant or unusable directories in DIFile metadata entries.

As discussed on llvm-dev recently, Clang currently emits redundant
directories in DIFile entries, such as

  .file  1 "/Volumes/Data/llvm" 
"/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"

This patch looks at any common prefix between the compilation
directory and the (absolute) file path and strips the redundant
part. More importantly it leaves the compilation directory empty if
the two paths have no common prefix.

After this patch the above entry is (assuming a compilation dir of 
"/Volumes/Data/llvm/_build"):

  .file 1 "/Volumes/Data/llvm" 
"tools/clang/test/CodeGen/debug-info-abspath.c"

When building the FileCheck binary with debug info, this patch makes
the build artifacts ~1kb smaller.

Differential Revision: https://reviews.llvm.org/D55085

Added:
cfe/trunk/test/CodeGen/debug-info-abspath.c
Modified:
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/lib/CodeGen/CodeGenAction.cpp
cfe/trunk/test/CodeGen/debug-prefix-map.c
cfe/trunk/test/Modules/module-debuginfo-prefix.m

Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348513&r1=348512&r2=348513&view=diff
==
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Thu Dec  6 10:44:50 2018
@@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
   SourceManager &SM = CGM.getContext().getSourceManager();
   auto *Scope = cast(LexicalBlockStack.back());
   PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
-
-  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
+  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
 return;
 
   if (auto *LBF = dyn_cast(Scope)) {
@@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
   SourceManager &SM = CGM.getContext().getSourceManager();
   PresumedLoc PLoc = SM.getPresumedLoc(Loc);
 
-  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
+  StringRef FileName = PLoc.getFilename();
+  if (PLoc.isInvalid() || FileName.empty())
 // If the location is not valid then use main input file.
 return getOrCreateMainFile();
 
   // Cache the results.
-  const char *fname = PLoc.getFilename();
-  auto It = DIFileCache.find(fname);
+  auto It = DIFileCache.find(FileName.data());
 
   if (It != DIFileCache.end()) {
 // Verify that the information still exists.
@@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
   if (CSKind)
 CSInfo.emplace(*CSKind, Checksum);
 
-  llvm::DIFile *F = DBuilder.createFile(
-  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()), 
CSInfo,
-  getSource(SM, SM.getFileID(Loc)));
+  StringRef Dir;
+  StringRef File;
+  std::string RemappedFile = remapDIPath(FileName);
+  std::string CurDir = remapDIPath(getCurrentDirname());
+  SmallString<128> DirBuf;
+  SmallString<128> FileBuf;
+  if (llvm::sys::path::is_absolute(RemappedFile)) {
+// Strip the common prefix (if it is more than just "/") from current
+// directory and FileName for a more space-efficient encoding.
+auto FileIt = llvm::sys::path::begin(RemappedFile);
+auto FileE = llvm::sys::path::end(RemappedFile);
+auto CurDirIt = llvm::sys::path::begin(CurDir);
+auto CurDirE = llvm::sys::path::end(CurDir);
+for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt, ++FileIt)
+  llvm::sys::path::append(DirBuf, *CurDirIt);
+if (std::distance(llvm::sys::path::begin(CurDir), CurDirIt) == 1) {
+  // The common prefix only the root; stripping it would cause
+  // LLVM diagnostic locations to be more confusing.
+  Dir = {};
+  File = RemappedFile;
+} else {
+  for (; FileIt != FileE; ++FileIt)
+llvm::sys::path::append(FileBuf, *FileIt);
+  Dir = DirBuf;
+  File = FileBuf;
+}
+  } else {
+Dir = CurDir;
+File = RemappedFile;
+  }
+  llvm::DIFile *F =
+  DBuilder.createFile(File, Dir, CSInfo,
+  getSource(SM, SM.getFileID(Loc)));
 
-  DIFileCache[fname].reset(F);
+  DIFileCache[FileName.data()].reset(F);
   return F;
 }
 

Modified: cfe/trunk/lib/CodeGen/CodeGenAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenAction.cpp?rev=348513&r1=348512&r2=348513&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenAction.cpp (original)
+++ cfe/trunk/lib/CodeGen/Cod

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321705 , @kristina wrote:

> > There is a cost to having people encode these flags into their build 
> > systems -- it can then cause issues if we ever change these internal flags. 
> > I do not think any Clang maintainer intends to support these as stable 
> > APIs, unlike most of the driver command-line. But, neither -Xclang nor 
> > -mllvm obviously scream out "don't use this!", and so people may then add 
> > them to their buildsystems without causing reviewers to say "Wait...really? 
> > Are you sure that's a good idea?".
>
> This is why I proposed a compromise, allow this warning to be disabled 
> completely for people actively using those flags, which are pretty much 
> exclusively toolchain developers (well basically what I proposed, since it's 
> not clear what counts as a build made by someone who is working and debugging 
> a pass, being fully aware of those flags, using the subset of them specific 
> to that pass/feature, I would say assertion+dump enabled builds are closest, 
> but having an explicit build flag for it would be nicer). It's more unified 
> than everyone either adding workarounds into build systems or disabling it 
> completely (by just removing it).


I mean, I'm not much opposed to adding that -- just that any new build-time 
options is always a minor shame. But you didn't respond to the other part of my 
message -- is adding `-Wno-experimental-driver-option` to your compile-flags 
not already a completely sufficient solution for your use-case?

> Besides, I don't think this really ever surfaced as an issue, until 
> Chandler's idea with regards to an unsupressable warning for performance 
> tests for 7.x.x

The primary impetus for me was actually the discovery that Boost's build system 
was using "-Xclang -include-pth" a few weeks back.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/UsersManual.rst:3137
   -W Enable the specified warning
-  -XclangPass  to the clang compiler
+  -XclangPass  to the clang cc1 frontend. For 
experimental use only.
 

One downside to this is that I believe there are still situations where -Xclang 
may be required, such as when loading plugins. This could be mildly annoying 
even without it impacting -Werror because some CI tools do warning counts, it 
chats at users, etc.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

I don't have an opinion on this patch (if you force me to have one, I'm weakly 
in favor), but I agree with the general sentiment. When I told people to not 
use mllvm and Xclang before, they told me "but if I'm not supposed to use them, 
why are they advertised in --help"?

  thakis@thakis:~/src/llvm-build$ bin/clang --help | grep Xclang
-XclangPass  to the clang compiler
  thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
-mllvm   Additional arguments to forward to LLVM's option 
processing

Which to me is a valid point! Maybe we should remove them from --help or say 
"internal only, don't usually use" there?


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

https://reviews.llvm.org/D55150



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

[PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

[Hans Wennborg via Phabricator via cfe-commits]

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

lgtm


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

https://reviews.llvm.org/D55377



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

r348515 - Allow forwarding -fdebug-compilation-dir to cc1as

[Nico Weber via cfe-commits]

Author: nico
Date: Thu Dec  6 10:50:39 2018
New Revision: 348515

URL: http://llvm.org/viewvc/llvm-project?rev=348515&view=rev
Log:
Allow forwarding -fdebug-compilation-dir to cc1as

The flag -fdebug-compilation-dir is useful to make generated .o files
independent of the path of the build directory, without making the compile
command-line dependent on the path of the build directory, like
-fdebug-prefix-map requires. This change makes it so that the driver can
forward the flag to -cc1as, like it already can for -cc1. We might want to
consider making -fdebug-compilation-dir a driver flag in a follow-up.

(Since -fdebug-compilation-dir defaults to PWD, it's already possible to get
this effect by setting PWD, but explicit compiler flags are better than env
vars, because e.g. ninja tracks command lines and reruns commands that change.)

Somewhat related to PR14625.

Differential Revision: https://reviews.llvm.org/D55377

Modified:
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/integrated-as.s

Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=348515&r1=348514&r2=348515&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Thu Dec  6 10:50:39 2018
@@ -2152,6 +2152,9 @@ static void CollectArgsForIntegratedAsse
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;

Modified: cfe/trunk/test/Driver/integrated-as.s
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/integrated-as.s?rev=348515&r1=348514&r2=348515&view=diff
==
--- cfe/trunk/test/Driver/integrated-as.s (original)
+++ cfe/trunk/test/Driver/integrated-as.s Thu Dec  6 10:50:39 2018
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 
2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s 
-Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler 
-fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck 
--check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."


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

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

[Romanov Vlad via Phabricator via cfe-commits]

romanovvlad updated this revision to Diff 177012.

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

https://reviews.llvm.org/D55262

Files:
  lib/CodeGen/CGExpr.cpp
  test/CodeGenOpenCLCXX/address-space-deduction2.cl


Index: test/CodeGenOpenCLCXX/address-space-deduction2.cl
===
--- test/CodeGenOpenCLCXX/address-space-deduction2.cl
+++ test/CodeGenOpenCLCXX/address-space-deduction2.cl
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm 
-o - | FileCheck %s
+
+class P {
+public:
+  P(const P &Rhs) = default;
+
+  long A;
+  long B;
+};
+
+void foo(__global P *GPtr) {
+// CHECK: call void @llvm.memcpy{{.*}}, {{.*}}, i32 16
+  P Val = GPtr[0];
+}
+
+struct __attribute__((packed)) A { int X; };
+int test(__global A *GPtr) {
+// CHECK: {{.*}} = load i32, {{.*}}, align 1
+  return static_cast<__generic A &>(*GPtr).X;
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -4269,8 +4269,9 @@
 QualType DestTy = getContext().getPointerType(E->getType());
 llvm::Value *V = getTargetHooks().performAddrSpaceCast(
 *this, LV.getPointer(), E->getSubExpr()->getType().getAddressSpace(),
-DestTy.getAddressSpace(), ConvertType(DestTy));
-return MakeNaturalAlignPointeeAddrLValue(V, DestTy);
+E->getType().getAddressSpace(), ConvertType(DestTy));
+return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),
+  E->getType(), LV.getBaseInfo(), LV.getTBAAInfo());
   }
   case CK_ObjCObjectLValueCast: {
 LValue LV = EmitLValue(E->getSubExpr());


Index: test/CodeGenOpenCLCXX/address-space-deduction2.cl
===
--- test/CodeGenOpenCLCXX/address-space-deduction2.cl
+++ test/CodeGenOpenCLCXX/address-space-deduction2.cl
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s
+
+class P {
+public:
+  P(const P &Rhs) = default;
+
+  long A;
+  long B;
+};
+
+void foo(__global P *GPtr) {
+// CHECK: call void @llvm.memcpy{{.*}}, {{.*}}, i32 16
+  P Val = GPtr[0];
+}
+
+struct __attribute__((packed)) A { int X; };
+int test(__global A *GPtr) {
+// CHECK: {{.*}} = load i32, {{.*}}, align 1
+  return static_cast<__generic A &>(*GPtr).X;
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -4269,8 +4269,9 @@
 QualType DestTy = getContext().getPointerType(E->getType());
 llvm::Value *V = getTargetHooks().performAddrSpaceCast(
 *this, LV.getPointer(), E->getSubExpr()->getType().getAddressSpace(),
-DestTy.getAddressSpace(), ConvertType(DestTy));
-return MakeNaturalAlignPointeeAddrLValue(V, DestTy);
+E->getType().getAddressSpace(), ConvertType(DestTy));
+return MakeAddrLValue(Address(V, LV.getAddress().getAlignment()),
+  E->getType(), LV.getBaseInfo(), LV.getTBAAInfo());
   }
   case CK_ObjCObjectLValueCast: {
 LValue LV = EmitLValue(E->getSubExpr());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Oh hey, the patch does that already! Ignore me, then :-) That part is probably 
less controversial, maybe you want to land that while the warning part is being 
discussed?


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

I don't really see the point of this and think it will just be an inconvenience 
to llvm developers.

Another use case we have for using these in a build system is for the builtin 
library shipped with the compiler


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

https://reviews.llvm.org/D55150



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

[PATCH] D55377: Allow forwarding -fdebug-compilation-dir to cc1as

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348515: Allow forwarding -fdebug-compilation-dir to cc1as 
(authored by nico, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55377?vs=177004&id=177015#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55377

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/test/Driver/integrated-as.s


Index: cfe/trunk/test/Driver/integrated-as.s
===
--- cfe/trunk/test/Driver/integrated-as.s
+++ cfe/trunk/test/Driver/integrated-as.s
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 
2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s 
-Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler 
-fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck 
--check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -2152,6 +2152,9 @@
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;


Index: cfe/trunk/test/Driver/integrated-as.s
===
--- cfe/trunk/test/Driver/integrated-as.s
+++ cfe/trunk/test/Driver/integrated-as.s
@@ -50,3 +50,9 @@
 
 // RUN: %clang -### -target x86_64--- -x assembler -c -fPIC -integrated-as %s 2>&1 | FileCheck --check-prefix=PIC %s
 // PIC: "-mrelocation-model" "pic"
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Wa,-fdebug-compilation-dir,. 2>&1 | FileCheck --check-prefix=WA_DEBUGDIR %s
+// WA_DEBUGDIR: "-fdebug-compilation-dir" "."
+
+// RUN: %clang -### -target x86_64--- -c -integrated-as %s -Xassembler -fdebug-compilation-dir -Xassembler . 2>&1 | FileCheck --check-prefix=XA_DEBUGDIR %s
+// XA_DEBUGDIR: "-fdebug-compilation-dir" "."
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -2152,6 +2152,9 @@
   }
   CmdArgs.push_back(Value.data());
   TakeNextArg = true;
+  } else if (Value == "-fdebug-compilation-dir") {
+CmdArgs.push_back("-fdebug-compilation-dir");
+TakeNextArg = true;
   } else {
 D.Diag(diag::err_drv_unsupported_option_argument)
 << A->getOption().getName() << Value;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'm in favor of at least the documentation changes, though I'd like to see them 
use stronger language.  I'm fairly in favor of the warning itself since its 
easy enough to disable (with the -Wno flag), so I don't terribly understand 
those against it.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added reviewers: dcoughlin, NoQ.
george.karpenkov requested changes to this revision.
george.karpenkov added a comment.

Using `-Xclang` is the only way to pass options to the static analyzer, I don't 
think we should warn on it.


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

https://reviews.llvm.org/D55150



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

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Thanks! I'll commit in a couple days to give some breathing room for anyone 
who'd object.


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

https://reviews.llvm.org/D54401



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

[PATCH] D52956: Support `-fno-visibility-inlines-hidden`

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Is this change blocked on something?


Repository:
  rC Clang

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

https://reviews.llvm.org/D52956



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Alex Lorenz via Phabricator via cfe-commits]

arphaman requested changes to this revision.
arphaman added a comment.

Swift uses `-Xclang` to pass in build settings to its own build and to pass in 
custom options through its Clang importer that we intentionally don't want to 
expose to Clang's users. We don't want to warn for those uses for sure.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55378: [compiler-rt] [test] Add missing cmake include for building libFuzzer alone

[Michał Górny via Phabricator via cfe-commits]

mgorny created this revision.
mgorny added reviewers: phosek, george.karpenkov.
Herald added subscribers: Sanitizers, llvm-commits, dberris.

Include CompilerRTCompile in fuzzer tests explicitly.  Otherwise, when
building only libFuzzer, CMake fails due to:

  CMake Error at cmake/Modules/AddCompilerRT.cmake:395 (sanitizer_test_compile):
Unknown CMake command "sanitizer_test_compile".
  Call Stack (most recent call first):
lib/fuzzer/tests/CMakeLists.txt:53 (generate_compiler_rt_tests)


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D55378

Files:
  lib/fuzzer/tests/CMakeLists.txt


Index: lib/fuzzer/tests/CMakeLists.txt
===
--- lib/fuzzer/tests/CMakeLists.txt
+++ lib/fuzzer/tests/CMakeLists.txt
@@ -1,3 +1,5 @@
+include(CompilerRTCompile)
+
 set(LIBFUZZER_UNITTEST_CFLAGS
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}


Index: lib/fuzzer/tests/CMakeLists.txt
===
--- lib/fuzzer/tests/CMakeLists.txt
+++ lib/fuzzer/tests/CMakeLists.txt
@@ -1,3 +1,5 @@
+include(CompilerRTCompile)
+
 set(LIBFUZZER_UNITTEST_CFLAGS
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

In D55150#1321734 , @thakis wrote:

> I don't have an opinion on this patch (if you force me to have one, I'm 
> weakly in favor), but I agree with the general sentiment. When I told people 
> to not use mllvm and Xclang before, they told me "but if I'm not supposed to 
> use them, why are they advertised in --help"?
>
>   thakis@thakis:~/src/llvm-build$ bin/clang --help | grep Xclang
> -XclangPass  to the clang compiler
>   thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
> -mllvm   Additional arguments to forward to LLVM's option 
> processing
>
>
> Which to me is a valid point! Maybe we should remove them from --help or say 
> "internal only, don't usually use" there?


I think the documentation for `-mllvm` could definitely emphasize the aspect 
that these flags are not a part of any public or stable interface and are used 
to change internal behavior of LLVM components a lot more. Definitely in favor 
of doing that.

In D55150#1321724 , @jyknight wrote:

> In D55150#1321705 , @kristina wrote:
>
> > > There is a cost to having people encode these flags into their build 
> > > systems -- it can then cause issues if we ever change these internal 
> > > flags. I do not think any Clang maintainer intends to support these as 
> > > stable APIs, unlike most of the driver command-line. But, neither -Xclang 
> > > nor -mllvm obviously scream out "don't use this!", and so people may then 
> > > add them to their buildsystems without causing reviewers to say 
> > > "Wait...really? Are you sure that's a good idea?".
> >
> > This is why I proposed a compromise, allow this warning to be disabled 
> > completely for people actively using those flags, which are pretty much 
> > exclusively toolchain developers (well basically what I proposed, since 
> > it's not clear what counts as a build made by someone who is working and 
> > debugging a pass, being fully aware of those flags, using the subset of 
> > them specific to that pass/feature, I would say assertion+dump enabled 
> > builds are closest, but having an explicit build flag for it would be 
> > nicer). It's more unified than everyone either adding workarounds into 
> > build systems or disabling it completely (by just removing it).
>
>
> I mean, I'm not much opposed to adding that -- just that any new build-time 
> options is always a minor shame. But you didn't respond to the other part of 
> my message -- is adding `-Wno-experimental-driver-option` to your 
> compile-flags not already a completely sufficient solution for your use-case?
>
> > Besides, I don't think this really ever surfaced as an issue, until 
> > Chandler's idea with regards to an unsupressable warning for performance 
> > tests for 7.x.x
>
> The primary impetus for me was actually the discovery that Boost's build 
> system was using "-Xclang -include-pth" a few weeks back.


It would be an inconvenience to developers and a lot of patches grow from 
downstream, given that some people may want to disable the flag for 
development, it would basically mean that some may end up with varying 
workarounds as opposed to a uniform one. I don't think that happening would be 
of benefit to anyone. And yes I could add an extra flag for that configuration 
in the build system, in my case, but I don't think I'll be the only one doing 
that so again, this would just cause more fragmentation. I don't see that as a 
good thing.


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D55150#1321759 , @george.karpenkov 
wrote:

> Using `-Xclang` is the only way to pass options to the static analyzer, I 
> don't think we should warn on it.


Well,, that seems unfortunate if we have the only supported interface for the 
static analyzer be an internal interface. Perhaps it can be given a different 
option? Even discounting this change, I that seems like it would be appropriate.

In D55150#1321771 , @arphaman wrote:

> Swift uses `-Xclang` to pass in build settings to its own build and to pass 
> in custom options through its Clang importer that we intentionally don't want 
> to expose to Clang's users. We don't want to warn for those uses for sure.


I'm not sure if I understand correctly, so I'll try to reiterate: Swift calls 
clang internally, and when it does so, intentionally uses options that are not 
generally intended for others to use. Of course we shouldn't emit warnings in 
that case.

Is that a correct understanding? If so, doesn't it just make sense for that 
constructed command-line to disable the warning? And, isn't it good that it 
will warn, otherwise, in order to discourage people from using those flags that 
are intentionally-not-exposed?


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I'm not sure that putting a warning that can be disabled really helps here; 
anyone who needs the option will just disable the warning anyway, and then 
users adding additional options somewhere else in the build system will miss 
the warning.

Instead, it would probably be better to rename Xclang and mllvm to something 
that makes it clear the user is doing something unsupported. Maybe 
"--unstable-llvm-option" and "--unstable-clang-option" or something like that.  
(This will lead to some breakage, but the breakage is roughly equivalent for 
anyone using -Werror.)


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

https://reviews.llvm.org/D55150



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

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> Well,, that seems unfortunate if we have the only supported interface for the 
> static analyzer be an internal interface. Perhaps it can be given a different 
> option? Even discounting this change, I that seems like it would be 
> appropriate.

Officially there is, it's called `-Xanalyzer`, but in practice it does the same 
as `-Xclang`, and users use the two interchangeably.


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

https://reviews.llvm.org/D55150



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