[PATCH] D56936: Fix handling of overriden methods during ASTImport

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

balazske added inline comments.



Comment at: lib/AST/ASTImporter.cpp:2950
 
+ExpectedStmt ASTNodeImporter::ImportFunctionDeclBody( FunctionDecl *FromFD, 
FunctionDecl *ToFD ) {
+if (Stmt *FromBody = FromFD->getBody()) {

It would be better to return `Error` instead of `Expected`. The return value 
(Stmt*) is not used and it may be nullptr if the `FromFD` does not have a body.



Comment at: lib/AST/ASTImporter.cpp:2959
+}
+
 ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {

Code formatting is not OK in this function (and other places). (Indentation and 
spaces around "(" ")".)



Comment at: unittests/AST/ASTImporterTest.cpp:2311
+  auto DFP =
+  cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), 
unless(isDefinition()) );
+

This line is too long. clang-format can be used to format the code 
automatically (or git-clang-format).


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

https://reviews.llvm.org/D56936



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think the code that comes to mind is mostly like in 
`GetTypeSourceInfoForDeclarator`:

  LangAS AS =
  (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx);

It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if 
it's not OpenCL++, but there shouldn't be a reason why the rest of the code in 
that block won't work for regular C++.

In fact, in most regular C++ this would be an issue, since Default typically 
_is_ the 'generic' address space there.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Sam McCall via cfe-commits]

Thanks for all the digging!
I can reproduce locally with your last example and ulimit -n 1000.

So it sounds like:
 - this *is* actually the interaction/behavior I expected from FileManager
(we call first with openFile=false then with openFile=true, and the file is
opened the second time)
 - the problem manifests because the file is opened but never closed, so we
leak file descriptors, but closing the file would'nt be the best fix - we
shouldn't open it if we don't need its content
 - I would call this a bug in the caller of file manager (in the stack we
saw where getFileAndSuggestModule calls getFile), because it calls with
openFile=true, but doesn't use the file contents nor ensure the file is
closed. This was previously masked by the bug (fixed in my patch) where
openFile=true could result in not opening the file.
 - nevertheless if I can't find a fairly safe fix soon then we'll need to
revert this patch.

On Wed, Jan 23, 2019 at 10:27 PM Nico Weber  wrote:

> For completeness, in case the construction isn't clear, here's a full
> repro of the symptom.
>
> $ cat pch.cc
> $ cat pch.h
> #include "foo.h"
> $ rm bar/foo.h
> $ for i in {1..300}; do echo '#include "foo'$i'.h"' >> bar/foo.h; done
> $ for i in {1..300}; do touch bar/foo$i.h; done
> $ cat client.cc
> #include "bar/foo.h"
> $ for i in {1..300}; do echo '#include "bar/foo'$i'.h"' >> client.cc; done
>
> With today's trunk, on macOS:
>
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
> /FIpch.h pch.cc  /Ibar
> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
> /FIpch.h client.cc /Ibar
> client.cc(253,10):  fatal error: 'bar/foo252.h' file not found
> #include "bar/foo252.h"
>  ^~
> 1 error generated.
>
> This works fine with this patch undone.
>
> On Wed, Jan 23, 2019 at 2:55 PM Nico Weber  wrote:
>
>> This might finally be a repro!
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
>> foo.h foo2.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
>> -bash: type: foo.h: not found
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
>> foo.h foo2.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
>> #include "foo2.h"
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
>> #include "foo.h"
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
>> #include "bar/foo.h"
>> #include "bar/foo2.h"
>>
>> #include "foo3.h"
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>> /FIpch.h pch.cc  /Ibar
>> stat pch.cc 1
>> opened pch.cc
>> closing pch.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> closing ./pch.h
>> stat ./foo.h 1
>> stat bar/foo.h 1
>> opened bar/foo.h
>> closing bar/foo.h
>> stat bar/foo2.h 1
>> opened bar/foo2.h
>> closing bar/foo2.h
>> stat pch.cc 1
>> opened pch.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
>> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>> closing /Users/thakis/src/llvm-mono-2/pch.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>> /FIpch.h client.cc
>> stat client.cc 1
>> opened client.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
>> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>> closing client.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> stat ./bar/foo.h 1
>> opened ./bar/foo.h
>> closing ./bar/foo.h
>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 1
>> opened /Users/thakis/src/llvm-mono-2/bar/foo2.h
>> closing /Users/thakis/src/llvm-mono-2/bar/foo2.h
>> stat ./bar/foo2.h 1
>> opened ./bar/foo2.h
>> stat ./foo3.h 1
>> opened ./foo3.h
>> closing ./foo3.h
>>
>>
>> Note how ./bar/foo2.h is opened in the 4th-to-last line but never
>> closed.  For comparision, with your patch reverted:
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>> /FIpch.h pch.cc  /Ibar
>> stat pch.cc 1
>> opened pch.cc
>> closing pch.cc
>> stat ./pch.h 1
>> opened ./pch.h
>> closing ./pch.h
>> stat ./foo.h 1
>> stat bar/foo.h 1
>> opened bar/foo.h
>> closing bar/foo.h
>> stat bar/foo2.h 1
>> opened bar/foo2.h
>> closing bar/foo2.h
>> stat pch.cc 1
>> opened pch.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
>> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>> closing /Users/thakis/src/llvm-mono-2/pch.cc
>> stat ./pch.h 1
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>> /FIpch.h client.cc
>> stat client.cc 1
>> opened client.cc
>> stat pch.pch 1
>> opened pch.pch
>> closing pch.pch
>> stat /Use

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:32
+: ClangTidyCheck(Name, Context),
+  OverrideMacro(Options.get("OverrideMacro", "override")),
+  FinalMacro(Options.get("FinalMacro", "final")) {}

alexfh wrote:
> I'd suggest to default to an empty string and use `override` as a fallback 
> right in the code where the diagnostic is generated.
So I tried this and and met with some issues with the unit tests where it 
seemed to think "override" was a macro, I also found myself just simply always 
setting OverrideMacro/Final Macro to "override" and "final" anyway.. I've 
changed this round a little to only check for the macro when the OverrideMacro 
isn't override. This seems to resolve the problem, let me know if it still 
feels wrong.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:42-46
+  // If we use ``override``, we require at least C++11. Use a
+  // macro with pre c++11 compilers by using OverrideMacro option.
+  if ((OverrideMacro == "override" && !getLangOpts().CPlusPlus11) ||
+  !getLangOpts().CPlusPlus)
+return;

alexfh wrote:
> I think, this should be left as is, because
> 1. clang-tidy understands C++11 and it makes sense to run it in C++11 mode to 
> get more information out of compiler (e.g. then OVERRIDE macro will actually 
> be defined to `override` and clang-tidy wouldn't have to jump through the 
> hoops to detect that a method is already declared `override`).
> 2. I've seen folks who just `#define override` in pre-C++11 mode to make the 
> code compile.
I agree with this..I don't need to run clang-tidy in cxx98 mode..reverting to 
the original check



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:97
+
+  if (!doesOverrideMacroExist(Context, OverrideMacro))
+return;

alexfh wrote:
> The utility of the function is questionable. I'd drop it and replace the call 
> with `if (!OverrideMacro.empty() && 
> !Context.Idents.get(OverrideMacro).hasMacroDefinition()) ...`.
removed the function, but did change the condition here to be 
OverrideMacro!="override" to overcome issue in the unit test when not 
overriding the macro it would sometimes return here and not perform the fix-it, 
I wondered if  check_clang_tidy.py -fix somehow ran the clang tidy check() 
function twice, once for the message and once for the fix.. but I didn't dig in 
any further.



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:121-131
+Message = "prefer using '" + OverrideMacro + "' or (rarely) '" +
+  FinalMacro + "' instead of 'virtual'";
   } else if (KeywordCount == 0) {
-Message = "annotate this function with 'override' or (rarely) 'final'";
+Message = "annotate this function with '" + OverrideMacro +
+  "' or (rarely) '" + FinalMacro + "'";
   } else {
 StringRef Redundant =

alexfh wrote:
> How about using diagnostic arguments instead of string concatenation 
> (`diag(Loc, "prefer using %1 or %2") << Macro1 << Macro2;`)?
switched to using %0 and %1 hope that is correct, that 0 vs 1 stumped me for a 
bit, but I looked at other checks doing the same thing



Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:133
+StringRef Correct =
+HasFinal ? "'" + FinalMacro + "'" : "'" + OverrideMacro + "'";
 

JonasToth wrote:
> Dangling? These values seem to be temporary and `StringRef` would bind to the 
> temporary, not?
> For the concatenation `llvm::Twine` would be better as well, same in the 
> other places.
removed the need for so may concatenations, I hope by using %0 and %1



Comment at: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp:95
+  // CHECK-FIXES: {{^}}  MustUseResultObject k() OVERRIDE;
+};

alexfh wrote:
> Please add a test where the OVERRIDE macro is already present. Same for the 
> FINAL macro.
added a couple more..let me know if its sufficient.. changed the name of the 
test to remove cxx98 too


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

https://reviews.llvm.org/D57087



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

[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 183268.
MyDeveloperDay marked 12 inline comments as done.
MyDeveloperDay added a comment.

Addressing review comments,

- reduce changes causing excessive string concats and problems with temporaries
- simplify cxx version and macro checks


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

https://reviews.llvm.org/D57087

Files:
  clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tidy/modernize/UseOverrideCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/modernize-use-override.rst
  test/clang-tidy/modernize-use-override-with-macro.cpp
  test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp

Index: test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++11
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-FIXES: {{^}}  virtual ~SimpleCases();
+
+  void a();
+  // CHECK-FIXES: {{^}}  void a();
+
+  virtual void b();
+  // CHECK-FIXES: {{^}}  virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-with-macro.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-override-with-macro.cpp
@@ -0,0 +1,69 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++11
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct Base {
+  virtual ~Base() {}
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void e() = 0;
+  virtual void f2() const = 0;
+  virtual void g() = 0;
+  virtual void j() const;
+  virtual void k() = 0;
+  virtual void l() const;
+};
+
+struct SimpleCases : public Base {
+public:
+  virtual ~SimpleCases();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~SimpleCases() OVERRIDE;
+
+  void a();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'OVERRIDE' or (rarely) 'FINAL' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() OVERRIDE;
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void b() OVERRIDE;
+
+  virtual void c();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void c() OVERRIDE;
+
+  virtual void e() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void e() OVERRIDE = 0;
+
+  virtual void f2() const = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void f2() const OVERRIDE = 0;
+
+  virtual void g() ABSTRACT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void g() OVERRIDE ABSTRACT;
+
+  virtual void j() const;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+  // CHECK-FIXES: {{^}}  void j() const OVERRIDE;
+
+  virtual void k() OVERRIDE;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'OVERRIDE' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void k() OVERRIDE;
+
+  virtual void l() const OVERRIDE;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'OVERRIDE' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void l() const OVERRIDE;
+};
Index: docs/clang-tidy/checks/modernize-use-override.rst
===
--- docs/clang-tidy/checks/modernize-use-override.rst
+++ docs/clang-tidy/checks/modernize-use-override.rst
@@ -3,5 +3,37 @@
 modernize-use-override
 ==
 
+Adds ``override`` (introduced in C++11) to overridden virtual functions and
+removes ``virtual`` from those functions as it is not required.
 
-Use C++11's ``override`` and remove ``virtual`` where applicable.
+virtual on non base class implementations was used to help indiciate to the user
+that a function was virtual. C++ compilers 

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:35
+  std::vector FunctionsThatShouldNotThrowVec =
+  utils::options::parseStringList(RawFunctionsThatShouldNotThrow);
   FunctionsThatShouldNotThrow.insert(FunctionsThatShouldNotThrowVec.begin(),

JonasToth wrote:
> lebedev.ri wrote:
> > Does it also accept ',' delimitter?
> > If not, i'm not sure how good of an idea it is to break existing configs..
> No it doesn't. It is a breaking change but at the same time it is 
> inconsistent and duplicated functionality.
> Should be mentioned in release notes, that for sure.
The thing is, if said list was set (as in user-specified to non-empty), it will 
be impossible
to use that same .clang-tidy config with clang-tidy <=8 and trunk.
I'm personally not affected, but if i was, i wouldn't think this is a great 
idea..

Can you at least make the breakage less crippling by also renaming the config 
field?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100



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

[PATCH] D56836: [mips] Include whole lpthread when using both -pthread and -static

[Aleksandar Beserminji via Phabricator via cfe-commits]

abeserminji abandoned this revision.
abeserminji added a comment.

Agreed on this solution D52878 .


Repository:
  rC Clang

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

https://reviews.llvm.org/D56836



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

[PATCH] D57093: [CodeComplete] [clangd] Fix crash on ValueDecl with a null type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 183273.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

- Rename the test
- clang-format


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

https://reviews.llvm.org/D57093

Files:
  clang-tools-extra/clangd/ExpectedTypes.cpp
  clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/test/CodeCompletion/crash-null-type.cpp


Index: clang/test/CodeCompletion/crash-null-type.cpp
===
--- /dev/null
+++ clang/test/CodeCompletion/crash-null-type.cpp
@@ -0,0 +1,8 @@
+void test() {
+  for (auto [loopVar] : y) { // y has to be unresolved
+loopVa
+  }
+}
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:3:11 %s -o - \
+// RUN:| FileCheck %s
+// CHECK: COMPLETION: loopVar
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -680,7 +680,8 @@
 T = Property->getType();
   else if (const auto *Value = dyn_cast(ND))
 T = Value->getType();
-  else
+
+  if (T.isNull())
 return QualType();
 
   // Dig through references, function pointers, and block pointers to
Index: clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -2319,6 +2319,17 @@
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
 }
 
+TEST(CompletionTest, WorksWithNullType) {
+  auto R = completions(R"cpp(
+int main() {
+  for (auto [loopVar] : y ) { // y has to be unresolved.
+int z = loopV^;
+  }
+}
+  )cpp");
+  EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ExpectedTypes.cpp
===
--- clang-tools-extra/clangd/ExpectedTypes.cpp
+++ clang-tools-extra/clangd/ExpectedTypes.cpp
@@ -35,8 +35,10 @@
 typeOfCompletion(const CodeCompletionResult &R) {
   auto *VD = dyn_cast_or_null(R.Declaration);
   if (!VD)
-return None; // We handle only variables and functions below.
+return llvm::None; // We handle only variables and functions below.
   auto T = VD->getType();
+  if (T.isNull())
+return llvm::None;
   if (auto FuncT = T->getAs()) {
 // Functions are a special case. They are completed as 'foo()' and we want
 // to match their return type rather than the function type itself.


Index: clang/test/CodeCompletion/crash-null-type.cpp
===
--- /dev/null
+++ clang/test/CodeCompletion/crash-null-type.cpp
@@ -0,0 +1,8 @@
+void test() {
+  for (auto [loopVar] : y) { // y has to be unresolved
+loopVa
+  }
+}
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:3:11 %s -o - \
+// RUN:| FileCheck %s
+// CHECK: COMPLETION: loopVar
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -680,7 +680,8 @@
 T = Property->getType();
   else if (const auto *Value = dyn_cast(ND))
 T = Value->getType();
-  else
+
+  if (T.isNull())
 return QualType();
 
   // Dig through references, function pointers, and block pointers to
Index: clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp
@@ -2319,6 +2319,17 @@
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
 }
 
+TEST(CompletionTest, WorksWithNullType) {
+  auto R = completions(R"cpp(
+int main() {
+  for (auto [loopVar] : y ) { // y has to be unresolved.
+int z = loopV^;
+  }
+}
+  )cpp");
+  EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ExpectedTypes.cpp
===
--- clang-tools-extra/clangd/ExpectedTypes.cpp
+++ clang-tools-extra/clangd/ExpectedTypes.cpp
@@ -35,8 +35,10 @@
 typeOfCompletion(const CodeCompletionResult &R) {
   auto *VD = dyn_cast_or_null(R.Declaration);
   if (!VD)
-return None; // We handle only variables and functions below.
+return llvm::None; // We handle only variables and functions below.
   auto T = VD->getType();
+  if (T.isNull())
+return llvm::None;
   if (auto FuncT = T->getAs()) {
 // Functions are a special case. They are completed as 'foo()' and we want
 // t

r352039 - Fix python3 compability issue in clang binding

[Serge Guelton via cfe-commits]

Author: serge_sans_paille
Date: Thu Jan 24 02:34:44 2019
New Revision: 352039

URL: http://llvm.org/viewvc/llvm-project?rev=352039&view=rev
Log:
Fix python3 compability issue in clang binding

The file contents could be of str type. Should use byte length instead
of str length, otherwise utf-8 encoded files may not get properly parsed
for completion.

Source issue: https://github.com/ncm2/ncm2-pyclang#2

Commited on behalf of `roxma'

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

Modified:
cfe/trunk/bindings/python/clang/cindex.py

Modified: cfe/trunk/bindings/python/clang/cindex.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/bindings/python/clang/cindex.py?rev=352039&r1=352038&r2=352039&view=diff
==
--- cfe/trunk/bindings/python/clang/cindex.py (original)
+++ cfe/trunk/bindings/python/clang/cindex.py Thu Jan 24 02:34:44 2019
@@ -2814,9 +2814,9 @@ class TranslationUnit(ClangObject):
 for i, (name, contents) in enumerate(unsaved_files):
 if hasattr(contents, "read"):
 contents = contents.read()
-
+contents = b(contents)
 unsaved_array[i].name = b(fspath(name))
-unsaved_array[i].contents = b(contents)
+unsaved_array[i].contents = contents
 unsaved_array[i].length = len(contents)
 
 ptr = conf.lib.clang_parseTranslationUnit(index,
@@ -2993,17 +2993,13 @@ class TranslationUnit(ClangObject):
 unsaved_files_array = 0
 if len(unsaved_files):
 unsaved_files_array = (_CXUnsavedFile * len(unsaved_files))()
-for i,(name,value) in enumerate(unsaved_files):
-if not isinstance(value, str):
-# FIXME: It would be great to support an efficient version
-# of this, one day.
-value = value.read()
-print(value)
-if not isinstance(value, str):
-raise TypeError('Unexpected unsaved file contents.')
-unsaved_files_array[i].name = fspath(name)
-unsaved_files_array[i].contents = value
-unsaved_files_array[i].length = len(value)
+for i,(name,contents) in enumerate(unsaved_files):
+if hasattr(contents, "read"):
+contents = contents.read()
+contents = b(contents)
+unsaved_files_array[i].name = b(fspath(name))
+unsaved_files_array[i].contents = contents
+unsaved_files_array[i].length = len(contents)
 ptr = conf.lib.clang_reparseTranslationUnit(self, len(unsaved_files),
 unsaved_files_array, options)
 
@@ -3057,17 +3053,13 @@ class TranslationUnit(ClangObject):
 unsaved_files_array = 0
 if len(unsaved_files):
 unsaved_files_array = (_CXUnsavedFile * len(unsaved_files))()
-for i,(name,value) in enumerate(unsaved_files):
-if not isinstance(value, str):
-# FIXME: It would be great to support an efficient version
-# of this, one day.
-value = value.read()
-print(value)
-if not isinstance(value, str):
-raise TypeError('Unexpected unsaved file contents.')
+for i,(name,contents) in enumerate(unsaved_files):
+if hasattr(contents, "read"):
+contents = contents.read()
+contents = b(contents)
 unsaved_files_array[i].name = b(fspath(name))
-unsaved_files_array[i].contents = b(value)
-unsaved_files_array[i].length = len(value)
+unsaved_files_array[i].contents = contents
+unsaved_files_array[i].length = len(contents)
 ptr = conf.lib.clang_codeCompleteAt(self, fspath(path), line, column,
 unsaved_files_array, len(unsaved_files), options)
 if ptr:


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

[PATCH] D56429: fix python3 compability issue

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352039: Fix python3 compability issue in clang binding 
(authored by serge_sans_paille, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56429?vs=180620&id=183276#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56429

Files:
  cfe/trunk/bindings/python/clang/cindex.py


Index: cfe/trunk/bindings/python/clang/cindex.py
===
--- cfe/trunk/bindings/python/clang/cindex.py
+++ cfe/trunk/bindings/python/clang/cindex.py
@@ -2814,9 +2814,9 @@
 for i, (name, contents) in enumerate(unsaved_files):
 if hasattr(contents, "read"):
 contents = contents.read()
-
+contents = b(contents)
 unsaved_array[i].name = b(fspath(name))
-unsaved_array[i].contents = b(contents)
+unsaved_array[i].contents = contents
 unsaved_array[i].length = len(contents)
 
 ptr = conf.lib.clang_parseTranslationUnit(index,
@@ -2993,17 +2993,13 @@
 unsaved_files_array = 0
 if len(unsaved_files):
 unsaved_files_array = (_CXUnsavedFile * len(unsaved_files))()
-for i,(name,value) in enumerate(unsaved_files):
-if not isinstance(value, str):
-# FIXME: It would be great to support an efficient version
-# of this, one day.
-value = value.read()
-print(value)
-if not isinstance(value, str):
-raise TypeError('Unexpected unsaved file contents.')
-unsaved_files_array[i].name = fspath(name)
-unsaved_files_array[i].contents = value
-unsaved_files_array[i].length = len(value)
+for i,(name,contents) in enumerate(unsaved_files):
+if hasattr(contents, "read"):
+contents = contents.read()
+contents = b(contents)
+unsaved_files_array[i].name = b(fspath(name))
+unsaved_files_array[i].contents = contents
+unsaved_files_array[i].length = len(contents)
 ptr = conf.lib.clang_reparseTranslationUnit(self, len(unsaved_files),
 unsaved_files_array, options)
 
@@ -3057,17 +3053,13 @@
 unsaved_files_array = 0
 if len(unsaved_files):
 unsaved_files_array = (_CXUnsavedFile * len(unsaved_files))()
-for i,(name,value) in enumerate(unsaved_files):
-if not isinstance(value, str):
-# FIXME: It would be great to support an efficient version
-# of this, one day.
-value = value.read()
-print(value)
-if not isinstance(value, str):
-raise TypeError('Unexpected unsaved file contents.')
+for i,(name,contents) in enumerate(unsaved_files):
+if hasattr(contents, "read"):
+contents = contents.read()
+contents = b(contents)
 unsaved_files_array[i].name = b(fspath(name))
-unsaved_files_array[i].contents = b(value)
-unsaved_files_array[i].length = len(value)
+unsaved_files_array[i].contents = contents
+unsaved_files_array[i].length = len(contents)
 ptr = conf.lib.clang_codeCompleteAt(self, fspath(path), line, column,
 unsaved_files_array, len(unsaved_files), options)
 if ptr:


Index: cfe/trunk/bindings/python/clang/cindex.py
===
--- cfe/trunk/bindings/python/clang/cindex.py
+++ cfe/trunk/bindings/python/clang/cindex.py
@@ -2814,9 +2814,9 @@
 for i, (name, contents) in enumerate(unsaved_files):
 if hasattr(contents, "read"):
 contents = contents.read()
-
+contents = b(contents)
 unsaved_array[i].name = b(fspath(name))
-unsaved_array[i].contents = b(contents)
+unsaved_array[i].contents = contents
 unsaved_array[i].length = len(contents)
 
 ptr = conf.lib.clang_parseTranslationUnit(index,
@@ -2993,17 +2993,13 @@
 unsaved_files_array = 0
 if len(unsaved_files):
 unsaved_files_array = (_CXUnsavedFile * len(unsaved_files))()
-for i,(name,value) in enumerate(unsaved_files):
-if not isinstance(value, str):
-# FIXME: It would be great to support an efficient version
-# of this, one day.
-value = value.read()
-print(value)
-if not isinstance(value, str):
-

[PATCH] D56429: fix python3 compability issue

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

Thanks @roxma for the patch!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56429



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

[clang-tools-extra] r352040 - [CodeComplete] [clangd] Fix crash on ValueDecl with a null type

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Thu Jan 24 02:41:43 2019
New Revision: 352040

URL: http://llvm.org/viewvc/llvm-project?rev=352040&view=rev
Log:
[CodeComplete] [clangd] Fix crash on ValueDecl with a null type

Reviewers: kadircet

Reviewed By: kadircet

Subscribers: ioeric, MaskRay, jkorous, arphaman, cfe-commits

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

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

Modified: clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ExpectedTypes.cpp?rev=352040&r1=352039&r2=352040&view=diff
==
--- clang-tools-extra/trunk/clangd/ExpectedTypes.cpp (original)
+++ clang-tools-extra/trunk/clangd/ExpectedTypes.cpp Thu Jan 24 02:41:43 2019
@@ -35,8 +35,10 @@ static llvm::Optional
 typeOfCompletion(const CodeCompletionResult &R) {
   auto *VD = dyn_cast_or_null(R.Declaration);
   if (!VD)
-return None; // We handle only variables and functions below.
+return llvm::None; // We handle only variables and functions below.
   auto T = VD->getType();
+  if (T.isNull())
+return llvm::None;
   if (auto FuncT = T->getAs()) {
 // Functions are a special case. They are completed as 'foo()' and we want
 // to match their return type rather than the function type itself.

Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=352040&r1=352039&r2=352040&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Thu Jan 24 
02:41:43 2019
@@ -2319,6 +2319,17 @@ TEST(CompletionTest, ObjectiveCMethodTwo
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
 }
 
+TEST(CompletionTest, WorksWithNullType) {
+  auto R = completions(R"cpp(
+int main() {
+  for (auto [loopVar] : y ) { // y has to be unresolved.
+int z = loopV^;
+  }
+}
+  )cpp");
+  EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


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

r352040 - [CodeComplete] [clangd] Fix crash on ValueDecl with a null type

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Thu Jan 24 02:41:43 2019
New Revision: 352040

URL: http://llvm.org/viewvc/llvm-project?rev=352040&view=rev
Log:
[CodeComplete] [clangd] Fix crash on ValueDecl with a null type

Reviewers: kadircet

Reviewed By: kadircet

Subscribers: ioeric, MaskRay, jkorous, arphaman, cfe-commits

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

Added:
cfe/trunk/test/CodeCompletion/crash-null-type.cpp
Modified:
cfe/trunk/lib/Sema/SemaCodeComplete.cpp

Modified: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCodeComplete.cpp?rev=352040&r1=352039&r2=352040&view=diff
==
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp (original)
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp Thu Jan 24 02:41:43 2019
@@ -680,7 +680,8 @@ QualType clang::getDeclUsageType(ASTCont
 T = Property->getType();
   else if (const auto *Value = dyn_cast(ND))
 T = Value->getType();
-  else
+
+  if (T.isNull())
 return QualType();
 
   // Dig through references, function pointers, and block pointers to

Added: cfe/trunk/test/CodeCompletion/crash-null-type.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeCompletion/crash-null-type.cpp?rev=352040&view=auto
==
--- cfe/trunk/test/CodeCompletion/crash-null-type.cpp (added)
+++ cfe/trunk/test/CodeCompletion/crash-null-type.cpp Thu Jan 24 02:41:43 2019
@@ -0,0 +1,8 @@
+void test() {
+  for (auto [loopVar] : y) { // y has to be unresolved
+loopVa
+  }
+}
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:3:11 %s -o - \
+// RUN:| FileCheck %s
+// CHECK: COMPLETION: loopVar


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

[PATCH] D57093: [CodeComplete] [clangd] Fix crash on ValueDecl with a null type

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp:2322
 
+TEST(CompletionTest, CrashOnNullType) {
+  auto Results = completions(R"cpp(

kadircet wrote:
> nit: WorksWithNullType ? it doesn't seem like crashing
Indeed, I've been using this naming convention for ages, but I can see why it 
might be confusing.


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

https://reviews.llvm.org/D57093



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

[PATCH] D57093: [CodeComplete] [clangd] Fix crash on ValueDecl with a null type

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352040: [CodeComplete] [clangd] Fix crash on ValueDecl with 
a null type (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57093?vs=183273&id=183278#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57093

Files:
  cfe/trunk/lib/Sema/SemaCodeComplete.cpp
  cfe/trunk/test/CodeCompletion/crash-null-type.cpp
  clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp


Index: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
===
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp
@@ -680,7 +680,8 @@
 T = Property->getType();
   else if (const auto *Value = dyn_cast(ND))
 T = Value->getType();
-  else
+
+  if (T.isNull())
 return QualType();
 
   // Dig through references, function pointers, and block pointers to
Index: cfe/trunk/test/CodeCompletion/crash-null-type.cpp
===
--- cfe/trunk/test/CodeCompletion/crash-null-type.cpp
+++ cfe/trunk/test/CodeCompletion/crash-null-type.cpp
@@ -0,0 +1,8 @@
+void test() {
+  for (auto [loopVar] : y) { // y has to be unresolved
+loopVa
+  }
+}
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:3:11 %s -o - \
+// RUN:| FileCheck %s
+// CHECK: COMPLETION: loopVar
Index: clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
===
--- clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
+++ clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
@@ -35,8 +35,10 @@
 typeOfCompletion(const CodeCompletionResult &R) {
   auto *VD = dyn_cast_or_null(R.Declaration);
   if (!VD)
-return None; // We handle only variables and functions below.
+return llvm::None; // We handle only variables and functions below.
   auto T = VD->getType();
+  if (T.isNull())
+return llvm::None;
   if (auto FuncT = T->getAs()) {
 // Functions are a special case. They are completed as 'foo()' and we want
 // to match their return type rather than the function type itself.
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -2319,6 +2319,17 @@
   EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
 }
 
+TEST(CompletionTest, WorksWithNullType) {
+  auto R = completions(R"cpp(
+int main() {
+  for (auto [loopVar] : y ) { // y has to be unresolved.
+int z = loopV^;
+  }
+}
+  )cpp");
+  EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
===
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp
@@ -680,7 +680,8 @@
 T = Property->getType();
   else if (const auto *Value = dyn_cast(ND))
 T = Value->getType();
-  else
+
+  if (T.isNull())
 return QualType();
 
   // Dig through references, function pointers, and block pointers to
Index: cfe/trunk/test/CodeCompletion/crash-null-type.cpp
===
--- cfe/trunk/test/CodeCompletion/crash-null-type.cpp
+++ cfe/trunk/test/CodeCompletion/crash-null-type.cpp
@@ -0,0 +1,8 @@
+void test() {
+  for (auto [loopVar] : y) { // y has to be unresolved
+loopVa
+  }
+}
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:3:11 %s -o - \
+// RUN:| FileCheck %s
+// CHECK: COMPLETION: loopVar
Index: clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
===
--- clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
+++ clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
@@ -35,8 +35,10 @@
 typeOfCompletion(const CodeCompletionResult &R) {
   auto *VD = dyn_cast_or_null(R.Declaration);
   if (!VD)
-return None; // We handle only variables and functions below.
+return llvm::None; // We handle only variables and functions below.
   auto T = VD->getType();
+  if (T.isNull())
+return llvm::None;
   if (auto FuncT = T->getAs()) {
 // Functions are a special case. They are completed as 'foo()' and we want
 // to match their return type rather than the function type itself.
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittes

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Jonas Toth via Phabricator via cfe-commits]

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



Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:35
+  std::vector FunctionsThatShouldNotThrowVec =
+  utils::options::parseStringList(RawFunctionsThatShouldNotThrow);
   FunctionsThatShouldNotThrow.insert(FunctionsThatShouldNotThrowVec.begin(),

lebedev.ri wrote:
> JonasToth wrote:
> > lebedev.ri wrote:
> > > Does it also accept ',' delimitter?
> > > If not, i'm not sure how good of an idea it is to break existing configs..
> > No it doesn't. It is a breaking change but at the same time it is 
> > inconsistent and duplicated functionality.
> > Should be mentioned in release notes, that for sure.
> The thing is, if said list was set (as in user-specified to non-empty), it 
> will be impossible
> to use that same .clang-tidy config with clang-tidy <=8 and trunk.
> I'm personally not affected, but if i was, i wouldn't think this is a great 
> idea..
> 
> Can you at least make the breakage less crippling by also renaming the config 
> field?
Oh thats true, i didnt consider the config files. Maybe I just leave it as is.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100



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

r352042 - Test commit: fix typo.

[Pierre Gousseau via cfe-commits]

Author: pgousseau
Date: Thu Jan 24 03:44:24 2019
New Revision: 352042

URL: http://llvm.org/viewvc/llvm-project?rev=352042&view=rev
Log:
Test commit: fix typo.

Modified:
cfe/trunk/lib/Driver/SanitizerArgs.cpp

Modified: cfe/trunk/lib/Driver/SanitizerArgs.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/SanitizerArgs.cpp?rev=352042&r1=352041&r2=352042&view=diff
==
--- cfe/trunk/lib/Driver/SanitizerArgs.cpp (original)
+++ cfe/trunk/lib/Driver/SanitizerArgs.cpp Thu Jan 24 03:44:24 2019
@@ -251,7 +251,7 @@ SanitizerArgs::SanitizerArgs(const ToolC
   if (RemoveObjectSizeAtO0) {
 AllRemove |= SanitizerKind::ObjectSize;
 
-// The user explicitly enabled the object size sanitizer. Warn that
+// The user explicitly enabled the object size sanitizer. Warn
 // that this does nothing at -O0.
 if (Add & SanitizerKind::ObjectSize)
   D.Diag(diag::warn_drv_object_size_disabled_O0)


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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Sam McCall via cfe-commits]

So from my reading starting at getFileAndSuggestModule, changing to
openFile=false is correct there unless some code is relying on that side
effect.
All the tests pass, too.

The most likely regression is where we previously did open+fstat we'll now
do stat+open+fstat.

I tried running three commands using your repro, counting stat/open/fstat
by instrumenting Unix/Path.inc.
I used three versions of the code: old = before my patch, mid = with
r347205, new = with r347205 and openFile=false.

Build PCH:
  stat: 331 -> 331 -> 635
  open: 316 -> 314 -> 316
  fstat: 311 -> 311 -> 8

Use PCH:
 stat: 322 -> 322 -> 924
 open: 910 -> 609 -> 308
 fstat: 607 -> 607 -> 5

No PCH:
 stat: 16 -> 16 -> 617
 open: 606 -> 606 -> 306
 fstat: 604 -> 604 -> 3

Summary:
 - r347205 was neutral for stats and fstats. It caused fewer files to be
opened when building a PCH (though failed to close the ones it did). It
caused file handles to leak when using a PCH.
 - openFile=false reduces the number of files opened when compiling (with
or without PCH). It also changes most fstats to stats, which is a loss.
 - compared to the original state, having both patches seems like a small
loss when building a PCH, and a small win when using one or compiling
without one.

So I'm tempted to check in the openFile=false.
It seems more internally consistent (passing openFile=true leaves the file
open), and avoids breaking clangd.
It seems uninvasive, though as we've seen side-effects are hard to predict.
Performance seems likely to be not-worse (vs reverting r347205).

On Thu, Jan 24, 2019 at 10:49 AM Sam McCall  wrote:

> Thanks for all the digging!
> I can reproduce locally with your last example and ulimit -n 1000.
>
> So it sounds like:
>  - this *is* actually the interaction/behavior I expected from FileManager
> (we call first with openFile=false then with openFile=true, and the file is
> opened the second time)
>  - the problem manifests because the file is opened but never closed, so
> we leak file descriptors, but closing the file would'nt be the best fix -
> we shouldn't open it if we don't need its content
>  - I would call this a bug in the caller of file manager (in the stack we
> saw where getFileAndSuggestModule calls getFile), because it calls with
> openFile=true, but doesn't use the file contents nor ensure the file is
> closed. This was previously masked by the bug (fixed in my patch) where
> openFile=true could result in not opening the file.
>  - nevertheless if I can't find a fairly safe fix soon then we'll need to
> revert this patch.
>
> On Wed, Jan 23, 2019 at 10:27 PM Nico Weber  wrote:
>
>> For completeness, in case the construction isn't clear, here's a full
>> repro of the symptom.
>>
>> $ cat pch.cc
>> $ cat pch.h
>> #include "foo.h"
>> $ rm bar/foo.h
>> $ for i in {1..300}; do echo '#include "foo'$i'.h"' >> bar/foo.h; done
>> $ for i in {1..300}; do touch bar/foo$i.h; done
>> $ cat client.cc
>> #include "bar/foo.h"
>> $ for i in {1..300}; do echo '#include "bar/foo'$i'.h"' >> client.cc; done
>>
>> With today's trunk, on macOS:
>>
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>> /FIpch.h pch.cc  /Ibar
>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>> /FIpch.h client.cc /Ibar
>> client.cc(253,10):  fatal error: 'bar/foo252.h' file not found
>> #include "bar/foo252.h"
>>  ^~
>> 1 error generated.
>>
>> This works fine with this patch undone.
>>
>> On Wed, Jan 23, 2019 at 2:55 PM Nico Weber  wrote:
>>
>>> This might finally be a repro!
>>>
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
>>> foo.h foo2.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
>>> -bash: type: foo.h: not found
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
>>> foo.h foo2.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
>>> #include "foo2.h"
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
>>> #include "foo.h"
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.cc
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat client.cc
>>> #include "bar/foo.h"
>>> #include "bar/foo2.h"
>>>
>>> #include "foo3.h"
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>>> /FIpch.h pch.cc  /Ibar
>>> stat pch.cc 1
>>> opened pch.cc
>>> closing pch.cc
>>> stat ./pch.h 1
>>> opened ./pch.h
>>> closing ./pch.h
>>> stat ./foo.h 1
>>> stat bar/foo.h 1
>>> opened bar/foo.h
>>> closing bar/foo.h
>>> stat bar/foo2.h 1
>>> opened bar/foo2.h
>>> closing bar/foo2.h
>>> stat pch.cc 1
>>> opened pch.cc
>>> stat pch.pch 1
>>> opened pch.pch
>>> closing pch.pch
>>> stat /Users/thakis/src/llvm-mono-2/bar/foo2.h 0
>>> stat /Users/thakis/src/llvm-mono-2/bar/foo.h 0
>>> stat /Users/thakis/src/llvm-mono-2/pch.h 0
>>> stat /Users/thakis/src/llvm-mono-2/pch.cc 0
>>> closing /Users/thakis/src/llvm-mono-2/pch.cc
>>> stat ./pch.h 1
>>> opened ./pch.h
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>>> /FIpc

[PATCH] D57100: [clang-tidy] refactor bugprone-exception-escape analysis into class

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:35
+  std::vector FunctionsThatShouldNotThrowVec =
+  utils::options::parseStringList(RawFunctionsThatShouldNotThrow);
   FunctionsThatShouldNotThrow.insert(FunctionsThatShouldNotThrowVec.begin(),

JonasToth wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > lebedev.ri wrote:
> > > > Does it also accept ',' delimitter?
> > > > If not, i'm not sure how good of an idea it is to break existing 
> > > > configs..
> > > No it doesn't. It is a breaking change but at the same time it is 
> > > inconsistent and duplicated functionality.
> > > Should be mentioned in release notes, that for sure.
> > The thing is, if said list was set (as in user-specified to non-empty), it 
> > will be impossible
> > to use that same .clang-tidy config with clang-tidy <=8 and trunk.
> > I'm personally not affected, but if i was, i wouldn't think this is a great 
> > idea..
> > 
> > Can you at least make the breakage less crippling by also renaming the 
> > config field?
> Oh thats true, i didnt consider the config files. Maybe I just leave it as is.
I understand the cleanup and the inconsistency and duplicated functionality,
but maybe regardless of the fix, it should be addressed in a separate review?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57100



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

[PATCH] D57108: [clang-tidy] diagnose possibiltiy to add 'noexcept' in modernize-use-noexcept

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Hmm, i think this diff is incorrect. Doesn't it also include the changes from 
D57100  ?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57108



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

Re: [clang-tools-extra] r352040 - [CodeComplete] [clangd] Fix crash on ValueDecl with a null type

[Ilya Biryukov via cfe-commits]

+Hans Wennborg , could you please merge this fix into
the release branch?

On Thu, Jan 24, 2019 at 1:41 PM Ilya Biryukov via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: ibiryukov
> Date: Thu Jan 24 02:41:43 2019
> New Revision: 352040
>
> URL: http://llvm.org/viewvc/llvm-project?rev=352040&view=rev
> Log:
> [CodeComplete] [clangd] Fix crash on ValueDecl with a null type
>
> Reviewers: kadircet
>
> Reviewed By: kadircet
>
> Subscribers: ioeric, MaskRay, jkorous, arphaman, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D57093
>
> Modified:
> clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
> clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
>
> Modified: clang-tools-extra/trunk/clangd/ExpectedTypes.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ExpectedTypes.cpp?rev=352040&r1=352039&r2=352040&view=diff
>
> ==
> --- clang-tools-extra/trunk/clangd/ExpectedTypes.cpp (original)
> +++ clang-tools-extra/trunk/clangd/ExpectedTypes.cpp Thu Jan 24 02:41:43
> 2019
> @@ -35,8 +35,10 @@ static llvm::Optional
>  typeOfCompletion(const CodeCompletionResult &R) {
>auto *VD = dyn_cast_or_null(R.Declaration);
>if (!VD)
> -return None; // We handle only variables and functions below.
> +return llvm::None; // We handle only variables and functions below.
>auto T = VD->getType();
> +  if (T.isNull())
> +return llvm::None;
>if (auto FuncT = T->getAs()) {
>  // Functions are a special case. They are completed as 'foo()' and we
> want
>  // to match their return type rather than the function type itself.
>
> Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=352040&r1=352039&r2=352040&view=diff
>
> ==
> --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
> (original)
> +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Thu Jan
> 24 02:41:43 2019
> @@ -2319,6 +2319,17 @@ TEST(CompletionTest, ObjectiveCMethodTwo
>EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}")));
>  }
>
> +TEST(CompletionTest, WorksWithNullType) {
> +  auto R = completions(R"cpp(
> +int main() {
> +  for (auto [loopVar] : y ) { // y has to be unresolved.
> +int z = loopV^;
> +  }
> +}
> +  )cpp");
> +  EXPECT_THAT(R.Completions, ElementsAre(Named("loopVar")));
> +}
> +
>  } // namespace
>  } // namespace clangd
>  } // namespace clang
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>


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

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 183291.
sammccall added a comment.

Actually include the relevant code change :-\


Repository:
  rC Clang

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

https://reviews.llvm.org/D57150

Files:
  lib/Lex/HeaderSearch.cpp
  test/PCH/leakfiles


Index: test/PCH/leakfiles
===
--- /dev/null
+++ test/PCH/leakfiles
@@ -0,0 +1,29 @@
+// Test that compiling using a PCH doesn't leak file descriptors.
+// https://bugs.chromium.org/p/chromium/issues/detail?id=924225
+
+// This test requires bash loops and ulimit.
+// REQUIRES: shell
+// UNSUPPORTED: win32
+//
+// Set up source files. lib/lib.h includes lots of lib*.h files in that dir.
+// client.c includes lib/lib.h, and also the individual files directly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: mkdir lib
+// RUN: for i in {1..300}; do touch lib/lib$i.h; done
+// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done
+// RUN: echo "#include \"lib/lib.h\"" > client.c
+// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done
+//
+// We want to verify that we don't hold all the files open at the same time.
+// This is important e.g. on mac, which has a low default FD limit.
+// RUN: ulimit -n 100
+//
+// Test without PCH.
+// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c
+//
+// Test with PCH.
+// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c
+// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only
Index: lib/Lex/HeaderSearch.cpp
===
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -310,7 +310,7 @@
 ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
-  const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/false);
   if (!File)
 return nullptr;
 


Index: test/PCH/leakfiles
===
--- /dev/null
+++ test/PCH/leakfiles
@@ -0,0 +1,29 @@
+// Test that compiling using a PCH doesn't leak file descriptors.
+// https://bugs.chromium.org/p/chromium/issues/detail?id=924225
+
+// This test requires bash loops and ulimit.
+// REQUIRES: shell
+// UNSUPPORTED: win32
+//
+// Set up source files. lib/lib.h includes lots of lib*.h files in that dir.
+// client.c includes lib/lib.h, and also the individual files directly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: mkdir lib
+// RUN: for i in {1..300}; do touch lib/lib$i.h; done
+// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done
+// RUN: echo "#include \"lib/lib.h\"" > client.c
+// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done
+//
+// We want to verify that we don't hold all the files open at the same time.
+// This is important e.g. on mac, which has a low default FD limit.
+// RUN: ulimit -n 100
+//
+// Test without PCH.
+// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c
+//
+// Test with PCH.
+// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c
+// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only
Index: lib/Lex/HeaderSearch.cpp
===
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -310,7 +310,7 @@
 ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
-  const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/false);
   if (!File)
 return nullptr;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: hans.
sammccall added a comment.

@hans: I think we're going to need either this fix or a revert of rL347205 
 + dependents cherrypicked for 8.0.
I don't know what the constraints/goals are for releases, let me know if you 
have an opinion which way we should go, (e.g. how important that trunk+release 
go the same way).


Repository:
  rC Clang

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

https://reviews.llvm.org/D57150



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

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: thakis, ilya-biryukov, bkramer.
Herald added a subscriber: cfe-commits.

We don't immediately need the file to be open, and this code does not ensure
that it is closed. On at least some code paths (using headers from PCHs) the
file is never closed and we accumulate open file descriptors.

Prior to r347205, the file was not actually opened (despite shouldOpen=true)
if the FileEntry already existed. In the PCH case, the entry existed (due to
a previous call with shouldOpen=false) and so we got away with passing true.

r347205 "fixed" that behavior of FileManager: if the file was previously statted
but not opened, and then FileManager::get(shouldOpen=true) is called, then the
file is opened.
Subsequently we observe spurious "file not found" errors in projects that use
large PCHs on platforms with low open-file limits:

  https://bugs.chromium.org/p/chromium/issues/detail?id=924225

The fix here has non-obvious effects on the sequence of FS operations, due to
the subtle pattern of caches and side-effects in FileManager's usage in clang.
The main observed effects (from instrumenting FileSystem) are:

- It converts most open()+fstat() pairs for headers into a stat()+open() pair. 
This should be a performance loss: stat() is more expensive.
- It avoids open()s of some files where we turn out not to need the contents. 
This should be a performance win: stat() is cheaper, and open() usually 
requires an additional close().

At least in simple tests without PCHs, I observe the same total for
fstat+stat+open, e.g. fstat -51, stat +73, open -22.


Repository:
  rC Clang

https://reviews.llvm.org/D57150

Files:
  test/PCH/leakfiles


Index: test/PCH/leakfiles
===
--- /dev/null
+++ test/PCH/leakfiles
@@ -0,0 +1,29 @@
+// Test that compiling using a PCH doesn't leak file descriptors.
+// https://bugs.chromium.org/p/chromium/issues/detail?id=924225
+
+// This test requires bash loops and ulimit.
+// REQUIRES: shell
+// UNSUPPORTED: win32
+//
+// Set up source files. lib/lib.h includes lots of lib*.h files in that dir.
+// client.c includes lib/lib.h, and also the individual files directly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: mkdir lib
+// RUN: for i in {1..300}; do touch lib/lib$i.h; done
+// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done
+// RUN: echo "#include \"lib/lib.h\"" > client.c
+// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done
+//
+// We want to verify that we don't hold all the files open at the same time.
+// This is important e.g. on mac, which has a low default FD limit.
+// RUN: ulimit -n 100
+//
+// Test without PCH.
+// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c
+//
+// Test with PCH.
+// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c
+// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only


Index: test/PCH/leakfiles
===
--- /dev/null
+++ test/PCH/leakfiles
@@ -0,0 +1,29 @@
+// Test that compiling using a PCH doesn't leak file descriptors.
+// https://bugs.chromium.org/p/chromium/issues/detail?id=924225
+
+// This test requires bash loops and ulimit.
+// REQUIRES: shell
+// UNSUPPORTED: win32
+//
+// Set up source files. lib/lib.h includes lots of lib*.h files in that dir.
+// client.c includes lib/lib.h, and also the individual files directly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: mkdir lib
+// RUN: for i in {1..300}; do touch lib/lib$i.h; done
+// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done
+// RUN: echo "#include \"lib/lib.h\"" > client.c
+// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done
+//
+// We want to verify that we don't hold all the files open at the same time.
+// This is important e.g. on mac, which has a low default FD limit.
+// RUN: ulimit -n 100
+//
+// Test without PCH.
+// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c
+//
+// Test with PCH.
+// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c
+// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r352047 - [Sema] Don't crash when recovering from a misspelled pseudo destructor call to an incomplete type.

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Thu Jan 24 05:52:47 2019
New Revision: 352047

URL: http://llvm.org/viewvc/llvm-project?rev=352047&view=rev
Log:
[Sema] Don't crash when recovering from a misspelled pseudo destructor call to 
an incomplete type.

When attempting to correct a misspelled pseudo destructor call as in:

struct Foo;
void foo(Foo *p) {
  p.~Foo();
}

a call is made in canRecoverDotPseudoDestructorCallsOnPointerObjects
to LookupDestructor without checking that the record has a definition.

This causes an assertion later in LookupSpecialMember which assumes that
the record has a definition.

Patch By Roman Zhikharevich!

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

Reviewed By: riccibruno


Modified:
cfe/trunk/lib/Sema/SemaExprCXX.cpp
cfe/trunk/test/SemaCXX/incomplete-call.cpp

Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=352047&r1=352046&r2=352047&view=diff
==
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Thu Jan 24 05:52:47 2019
@@ -6854,8 +6854,9 @@ canRecoverDotPseudoDestructorCallsOnPoin
QualType DestructedType) {
   // If this is a record type, check if its destructor is callable.
   if (auto *RD = DestructedType->getAsCXXRecordDecl()) {
-if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
-  return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
+if (RD->hasDefinition())
+  if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
+return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
 return false;
   }
 

Modified: cfe/trunk/test/SemaCXX/incomplete-call.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/incomplete-call.cpp?rev=352047&r1=352046&r2=352047&view=diff
==
--- cfe/trunk/test/SemaCXX/incomplete-call.cpp (original)
+++ cfe/trunk/test/SemaCXX/incomplete-call.cpp Thu Jan 24 05:52:47 2019
@@ -48,6 +48,10 @@ void test_incomplete_object_call(C& c) {
   c(); // expected-error{{incomplete type in call to object of type}}
 }
 
+void test_incomplete_object_dtor(C *p) {
+  p.~C(); // expected-error{{member reference type 'C *' is a pointer; did you 
mean to use '->'?}}
+}
+
 namespace pr18542 {
   struct X {
 int count;


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

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/Expr.h:5103
+using reference = AssociationTy;
+using pointer = AssociationTy;
+AssociationIteratorTy() = default;

aaron.ballman wrote:
> Carrying over the conversation from D57098:
> 
> >> @aaron.ballman Cute, but I suspect this may come back to bite us at some 
> >> point. For instance, if someone thinks they're working with a real 
> >> pointer, they're likely to expect pointer arithmetic to work when it won't 
> >> (at least they'll get compile errors though).
> > @riccibruno Hmm, but pointer is just the return type of operator-> no ? Is 
> > it actually required to behave like a pointer ? The only requirement I can 
> > find is that It->x must be equivalent to (*It).x, which is true here.
> 
> I double-checked and you're right, this is not a requirement of the STL.
> 
> > Also looking at the requirements for forward iterators I think that this 
> > iterator should actually be downgraded to an input iterator, because of the 
> > requirement that reference = T&.
> 
> My concern is that with the less functional iterators, common algorithms get 
> more expensive. For instance, `std::distance()`, `std::advance()` become more 
> expensive without random access, and things like `std::prev()` become 
> impossible.
> 
> It seems like the view this needs to provide is a read-only access to the 
> `AssociationTy` objects (because we need to construct them on the fly), but 
> not the data contained within them. If the only thing you can get back from 
> the iterator is a const pointer/reference/value type, then we could store a 
> local "current association" object in the iterator and return a 
> pointer/reference to that. WDYT?
I am worried about lifetime issues with this approach. Returning a 
reference/pointer to an `Association` object stored in the iterator means that 
the reference/pointer will dangle as soon as the iterator goes out of scope. 
This is potentially surprising since the "container" (that is the 
`GenericSelectionExpr`) here will still be in scope. Returning a value is safer 
in this aspect.

I believe it should be possible to make the iterator a random access iterator, 
at least
if we are willing to ignore some requirements:

1.) For forward iterators and up, we must have `reference = T&` or `const T&`.
2.) For forward iterators and up, `It1 == It2` if and only if `*It1` and `*It2` 
are bound to the same object.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:5151
 
+  if (FromTypeCanon.getQualifiers().hasAddressSpace()) {
+Qualifiers QualsImplicitParamType = ImplicitParamType.getQualifiers();

I tested this patch with some kludges to let it parse our address space 
qualifiers, and hit a problem here; just because the FromType isn't qualified 
doesn't mean that the object conversion is okay. A conversion of an object 
`Type` to `__X Type` might not be legal if there is no conversion from 'no 
address space' to '__X' address space.

The example was a class with an AS-qualified overload, and when resolving for 
calling the overloaded method on an object `Type *`, it hit an ambiguous 
resolution since it considered the AS-qualified method to be a legal candidate.

It feels like this might be rather specific to how a language/target wants to 
use address spaces, though.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

[PATCH] D56723: [CodeComplete] Propagate preferred types through parser in more cases

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 183297.
ilya-biryukov added a comment.

- Remove enterUnknown(), keep token location instead.
- Inline update() and remove it


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

https://reviews.llvm.org/D56723

Files:
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -339,4 +339,103 @@
   EXPECT_THAT(collectPreferredTypes(Code), Each("NULL TYPE"));
 }
 
+TEST(PreferredTypeTest, Members) {
+  StringRef Code = R"cpp(
+struct vector {
+  int *begin();
+  vector clone();
+};
+
+void test(int *a) {
+  a = ^vector().^clone().^begin();
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("int *"));
+}
+
+TEST(PreferredTypeTest, Conditions) {
+  StringRef Code = R"cpp(
+struct vector {
+  bool empty();
+};
+
+void test() {
+  if (^vector().^empty()) {}
+  while (^vector().^empty()) {}
+  for (; ^vector().^empty();) {}
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("_Bool"));
+}
+
+TEST(PreferredTypeTest, InitAndAssignment) {
+  StringRef Code = R"cpp(
+struct vector {
+  int* begin();
+};
+
+void test() {
+  const int* x = ^vector().^begin();
+  x = ^vector().^begin();
+
+  if (const int* y = ^vector().^begin()) {}
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("const int *"));
+}
+
+TEST(PreferredTypeTest, UnaryExprs) {
+  StringRef Code = R"cpp(
+void test(long long a) {
+  a = +^a;
+  a = -^a
+  a = ++^a;
+  a = --^a;
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("long long"));
+
+  Code = R"cpp(
+void test(int a, int *ptr) {
+  !^a;
+  !^ptr;
+  !!!^a;
+
+  a = !^a;
+  a = !^ptr;
+  a = !!!^a;
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("_Bool"));
+
+  Code = R"cpp(
+void test(int a) {
+  const int* x = &^a;
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("const int"));
+
+  Code = R"cpp(
+void test(int *a) {
+  int x = *^a;
+  int &r = *^a;
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("int *"));
+
+  Code = R"cpp(
+void test(int a) {
+  *^a;
+  &^a;
+}
+
+  )cpp";
+}
+
+TEST(PreferredTypeTest, ParenExpr) {
+  StringRef Code = R"cpp(
+const int *i = ^(^(^(^10)));
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("const int *"));
+}
 } // namespace
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -347,6 +347,210 @@
 };
 } // namespace
 
+PreferredTypeBuilder::RestoreRAII
+PreferredTypeBuilder::enterReturn(Sema &S, SourceLocation Tok) {
+  RestoreRAII R(*this);
+  if (isa(S.CurContext)) {
+if (sema::BlockScopeInfo *BSI = S.getCurBlock()) {
+  Type = BSI->ReturnType;
+  ExpectedLoc = Tok;
+}
+return R;
+  }
+  if (const auto *Function = dyn_cast(S.CurContext)) {
+Type = Function->getReturnType();
+ExpectedLoc = Tok;
+return R;
+  }
+  if (const auto *Method = dyn_cast(S.CurContext)) {
+Type = Method->getReturnType();
+ExpectedLoc = Tok;
+return R;
+  }
+  return R;
+}
+
+PreferredTypeBuilder::RestoreRAII
+PreferredTypeBuilder::enterVariableInit(SourceLocation Tok, Decl *D) {
+  RestoreRAII R(*this);
+  auto *VD = llvm::dyn_cast_or_null(D);
+  Type = VD ? VD->getType() : QualType();
+  ExpectedLoc = Tok;
+  return R;
+}
+
+PreferredTypeBuilder::RestoreRAII
+PreferredTypeBuilder::enterParenExpr(SourceLocation Tok,
+ SourceLocation LParLoc) {
+  RestoreRAII R(*this);
+  // expected type for parenthesized expression does not change.
+  if (ExpectedLoc == LParLoc)
+ExpectedLoc = Tok;
+  return R;
+}
+
+static QualType getPreferredTypeOfBinaryRHS(Sema &S, Expr *LHS,
+tok::TokenKind Op) {
+  if (!LHS)
+return QualType();
+
+  QualType LHSType = LHS->getType();
+  if (LHSType->isPointerType()) {
+if (Op == tok::plus || Op == tok::plusequal || Op == tok::minusequal)
+  return S.getASTContext().getPointerDiffType();
+// Pointer difference is more common than subtracting an int from a pointer.
+if (Op == tok::minus)
+  return LHSType;
+  }
+
+  switch (Op) {
+  // No way to infer the type of RHS from LHS.
+  case tok::comma:
+return QualType();
+  // Prefer the type of the left o

[PATCH] D57111: Make Clang not crash on calls to destructors on incomplete pointer types

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352047: [Sema] Don't crash when recovering from a 
misspelled pseudo destructor call to… (authored by brunoricci, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57111?vs=183144&id=183292#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57111

Files:
  cfe/trunk/lib/Sema/SemaExprCXX.cpp
  cfe/trunk/test/SemaCXX/incomplete-call.cpp


Index: cfe/trunk/test/SemaCXX/incomplete-call.cpp
===
--- cfe/trunk/test/SemaCXX/incomplete-call.cpp
+++ cfe/trunk/test/SemaCXX/incomplete-call.cpp
@@ -48,6 +48,10 @@
   c(); // expected-error{{incomplete type in call to object of type}}
 }
 
+void test_incomplete_object_dtor(C *p) {
+  p.~C(); // expected-error{{member reference type 'C *' is a pointer; did you 
mean to use '->'?}}
+}
+
 namespace pr18542 {
   struct X {
 int count;
Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -6854,8 +6854,9 @@
QualType DestructedType) {
   // If this is a record type, check if its destructor is callable.
   if (auto *RD = DestructedType->getAsCXXRecordDecl()) {
-if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
-  return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
+if (RD->hasDefinition())
+  if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
+return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
 return false;
   }
 


Index: cfe/trunk/test/SemaCXX/incomplete-call.cpp
===
--- cfe/trunk/test/SemaCXX/incomplete-call.cpp
+++ cfe/trunk/test/SemaCXX/incomplete-call.cpp
@@ -48,6 +48,10 @@
   c(); // expected-error{{incomplete type in call to object of type}}
 }
 
+void test_incomplete_object_dtor(C *p) {
+  p.~C(); // expected-error{{member reference type 'C *' is a pointer; did you mean to use '->'?}}
+}
+
 namespace pr18542 {
   struct X {
 int count;
Index: cfe/trunk/lib/Sema/SemaExprCXX.cpp
===
--- cfe/trunk/lib/Sema/SemaExprCXX.cpp
+++ cfe/trunk/lib/Sema/SemaExprCXX.cpp
@@ -6854,8 +6854,9 @@
QualType DestructedType) {
   // If this is a record type, check if its destructor is callable.
   if (auto *RD = DestructedType->getAsCXXRecordDecl()) {
-if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
-  return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
+if (RD->hasDefinition())
+  if (CXXDestructorDecl *D = SemaRef.LookupDestructor(RD))
+return SemaRef.CanUseDecl(D, /*TreatUnavailableAsInvalid=*/false);
 return false;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56723: [CodeComplete] Propagate preferred types through parser in more cases

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added inline comments.



Comment at: lib/Sema/SemaCodeComplete.cpp:373
+}
+Type = QualType();
+  });

kadircet wrote:
> nit: maybe move this into top and get rid of the return statements inside 
> `if`s(also by changing them to `else if`s)
FWIW I find ifs with return to be more readable than a bunch of if+else 
statements. The reason is that return clearly states function is done, while 
trailing `else` suggests it will go further.

But this particular function is short, so it's not a strong argument, can 
change it if you insist.



Comment at: lib/Sema/SemaCodeComplete.cpp:380
+  return update([&]() {
+auto *VD = llvm::dyn_cast_or_null(D);
+Type = VD ? VD->getType() : QualType();

kadircet wrote:
> Is it really possible for D to be null ?
Probably shouldn't, but I can't be certain, I'm too lazy to inspect all code 
paths in the parser, so I'd simply rely on the types used there and assume this 
could happen.

I'll try adding an assertion and finding the example when it happens, though.


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

https://reviews.llvm.org/D56723



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

[clang-tools-extra] r352049 - [clangd] Clean the cache of file statuses on vscode-clangd when clangd crashes.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Jan 24 06:25:53 2019
New Revision: 352049

URL: http://llvm.org/viewvc/llvm-project?rev=352049&view=rev
Log:
[clangd] Clean the cache of file statuses on vscode-clangd when clangd crashes.

Summary:
Clear the cached file statuses, otherwise we will leave some garbage texts on
the status bar when clangd crashes.

Reviewers: ilya-biryukov

Subscribers: ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits

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

Modified:
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts

Modified: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts?rev=352049&r1=352048&r2=352049&view=diff
==
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts 
(original)
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts Thu 
Jan 24 06:25:53 2019
@@ -40,6 +40,11 @@ class FileStatus {
 this.statusBarItem.show();
 }
 
+clear() {
+this.statuses.clear();
+this.statusBarItem.hide();
+}
+
 dispose() {
 this.statusBarItem.dispose();
 }
@@ -112,9 +117,16 @@ export function activate(context: vscode
 context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(() => 
{
 status.updateStatus();
 }));
-clangdClient.onReady().then(() => {
-clangdClient.onNotification(
-'textDocument/clangd.fileStatus',
-(fileStatus) => { status.onFileUpdated(fileStatus); });
-})
+clangdClient.onDidChangeState(
+({ newState }) => {
+if (newState == vscodelc.State.Running) {
+// clangd starts or restarts after crash.
+clangdClient.onNotification(
+'textDocument/clangd.fileStatus',
+(fileStatus) => { status.onFileUpdated(fileStatus); });
+} else if (newState == vscodelc.State.Stopped) {
+// Clear all cached statuses when clangd crashes.
+status.clear();
+}
+})
 }


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

[PATCH] D56540: [clangd] Clean the cache of file statuses on vscode-clangd when clangd crashes.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE352049: [clangd] Clean the cache of file statuses on 
vscode-clangd when clangd crashes. (authored by hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56540?vs=181035&id=183298#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56540

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
@@ -40,6 +40,11 @@
 this.statusBarItem.show();
 }
 
+clear() {
+this.statuses.clear();
+this.statusBarItem.hide();
+}
+
 dispose() {
 this.statusBarItem.dispose();
 }
@@ -112,9 +117,16 @@
 context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(() => 
{
 status.updateStatus();
 }));
-clangdClient.onReady().then(() => {
-clangdClient.onNotification(
-'textDocument/clangd.fileStatus',
-(fileStatus) => { status.onFileUpdated(fileStatus); });
-})
+clangdClient.onDidChangeState(
+({ newState }) => {
+if (newState == vscodelc.State.Running) {
+// clangd starts or restarts after crash.
+clangdClient.onNotification(
+'textDocument/clangd.fileStatus',
+(fileStatus) => { status.onFileUpdated(fileStatus); });
+} else if (newState == vscodelc.State.Stopped) {
+// Clear all cached statuses when clangd crashes.
+status.clear();
+}
+})
 }


Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -40,6 +40,11 @@
 this.statusBarItem.show();
 }
 
+clear() {
+this.statuses.clear();
+this.statusBarItem.hide();
+}
+
 dispose() {
 this.statusBarItem.dispose();
 }
@@ -112,9 +117,16 @@
 context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(() => {
 status.updateStatus();
 }));
-clangdClient.onReady().then(() => {
-clangdClient.onNotification(
-'textDocument/clangd.fileStatus',
-(fileStatus) => { status.onFileUpdated(fileStatus); });
-})
+clangdClient.onDidChangeState(
+({ newState }) => {
+if (newState == vscodelc.State.Running) {
+// clangd starts or restarts after crash.
+clangdClient.onNotification(
+'textDocument/clangd.fileStatus',
+(fileStatus) => { status.onFileUpdated(fileStatus); });
+} else if (newState == vscodelc.State.Stopped) {
+// Clear all cached statuses when clangd crashes.
+status.clear();
+}
+})
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Nico Weber via cfe-commits]

Can you measure on macOS and Windows too, and maybe with a larger program?
The open+fstat (instead of stat+open) optimization was I think done with
benchmarking on macOS.

I can see why you say you think the new behavior is more consistent, but
it's basically changing what information goes into the getFile() cache key.
Previously, openFile didn't go in there, and from that perspective saying
"the cache keeps the openFile passed the first time you call this for a
file" makes sense to me too.

I'm happy with either before your patch or with getFileAndSuggestModule()
passing openFile=false if perf is comparable (which I expect it to be, to
be honest).

Since the openFile=false change might be a bit risky, maybe we should
revert on 8.0 and do openFile=false only on trunk.

On Thu, Jan 24, 2019 at 6:51 AM Sam McCall  wrote:

> So from my reading starting at getFileAndSuggestModule, changing to
> openFile=false is correct there unless some code is relying on that side
> effect.
> All the tests pass, too.
>
> The most likely regression is where we previously did open+fstat we'll now
> do stat+open+fstat.
>
> I tried running three commands using your repro, counting stat/open/fstat
> by instrumenting Unix/Path.inc.
> I used three versions of the code: old = before my patch, mid = with
> r347205, new = with r347205 and openFile=false.
>
> Build PCH:
>   stat: 331 -> 331 -> 635
>   open: 316 -> 314 -> 316
>   fstat: 311 -> 311 -> 8
>
> Use PCH:
>  stat: 322 -> 322 -> 924
>  open: 910 -> 609 -> 308
>  fstat: 607 -> 607 -> 5
>
> No PCH:
>  stat: 16 -> 16 -> 617
>  open: 606 -> 606 -> 306
>  fstat: 604 -> 604 -> 3
>
> Summary:
>  - r347205 was neutral for stats and fstats. It caused fewer files to be
> opened when building a PCH (though failed to close the ones it did). It
> caused file handles to leak when using a PCH.
>  - openFile=false reduces the number of files opened when compiling (with
> or without PCH). It also changes most fstats to stats, which is a loss.
>  - compared to the original state, having both patches seems like a small
> loss when building a PCH, and a small win when using one or compiling
> without one.
>
> So I'm tempted to check in the openFile=false.
> It seems more internally consistent (passing openFile=true leaves the file
> open), and avoids breaking clangd.
> It seems uninvasive, though as we've seen side-effects are hard to
> predict. Performance seems likely to be not-worse (vs reverting r347205).
>
> On Thu, Jan 24, 2019 at 10:49 AM Sam McCall  wrote:
>
>> Thanks for all the digging!
>> I can reproduce locally with your last example and ulimit -n 1000.
>>
>> So it sounds like:
>>  - this *is* actually the interaction/behavior I expected from
>> FileManager (we call first with openFile=false then with openFile=true, and
>> the file is opened the second time)
>>  - the problem manifests because the file is opened but never closed, so
>> we leak file descriptors, but closing the file would'nt be the best fix -
>> we shouldn't open it if we don't need its content
>>  - I would call this a bug in the caller of file manager (in the stack we
>> saw where getFileAndSuggestModule calls getFile), because it calls with
>> openFile=true, but doesn't use the file contents nor ensure the file is
>> closed. This was previously masked by the bug (fixed in my patch) where
>> openFile=true could result in not opening the file.
>>  - nevertheless if I can't find a fairly safe fix soon then we'll need to
>> revert this patch.
>>
>> On Wed, Jan 23, 2019 at 10:27 PM Nico Weber  wrote:
>>
>>> For completeness, in case the construction isn't clear, here's a full
>>> repro of the symptom.
>>>
>>> $ cat pch.cc
>>> $ cat pch.h
>>> #include "foo.h"
>>> $ rm bar/foo.h
>>> $ for i in {1..300}; do echo '#include "foo'$i'.h"' >> bar/foo.h; done
>>> $ for i in {1..300}; do touch bar/foo$i.h; done
>>> $ cat client.cc
>>> #include "bar/foo.h"
>>> $ for i in {1..300}; do echo '#include "bar/foo'$i'.h"' >> client.cc;
>>> done
>>>
>>> With today's trunk, on macOS:
>>>
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Ycpch.h
>>> /FIpch.h pch.cc  /Ibar
>>> Nicos-MacBook-Pro:llvm-mono-2 thakis$ out/gn/bin/clang-cl /c /Yupch.h
>>> /FIpch.h client.cc /Ibar
>>> client.cc(253,10):  fatal error: 'bar/foo252.h' file not found
>>> #include "bar/foo252.h"
>>>  ^~
>>> 1 error generated.
>>>
>>> This works fine with this patch undone.
>>>
>>> On Wed, Jan 23, 2019 at 2:55 PM Nico Weber  wrote:
>>>
 This might finally be a repro!

 Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
 foo.h foo2.h
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ type foo.h
 -bash: type: foo.h: not found
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ ls bar
 foo.h foo2.h
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo.h
 #include "foo2.h"
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat bar/foo2.h
 Nicos-MacBook-Pro:llvm-mono-2 thakis$ cat pch.h
 #include "foo.h"
 Nicos-MacBook-Pro:llvm-mono

[PATCH] D56540: [clangd] Clean the cache of file statuses on vscode-clangd when clangd crashes.

[Haojian Wu via Phabricator via cfe-commits]

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



Comment at: clangd/clients/clangd-vscode/src/extension.ts:127
+(fileStatus) => { status.onFileUpdated(fileStatus); });
+} else if (newState == vscodelc.State.Stopped) {
+// Clear all cached statuses when clangd crashes.

ilya-biryukov wrote:
> This looks scary, if we end up doing that in too many places, the code would 
> become completely unreadable.
> 
> Have no idea how to make it better, but a very general observation is that 
> we'd be better off with dropping all the state we have when clangd crashes 
> and starting from scratch, rather than trying to keep old components and 
> bring them into a sensible state when crashes happen.
> 
> No need to do anything right now, just something to keep in mind for the 
> future.
Acked.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56540



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

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

[Nico Weber via Phabricator via cfe-commits]

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

Thanks for the fix!

Nice test too. Let's land this and see if anything breaks :-)

On the 8.0, I'd probably recommend reverting your change instead, since this 
feels like a subtle change that might have unintended consequences somewhere.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57150



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

[PATCH] D56723: [CodeComplete] Propagate preferred types through parser in more cases

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D56723#1366885 , @rsmith wrote:

> `ConsumeToken` is a fairly hot function; if you can avoid changes there 
> that'd be preferable.


Done, there are no `enterUnknown` calls anymore and to avoid updating on each 
call to `ConsumeToken` we now store a location of the token we computed the 
expected type for.
The resulting code looks ok to my taste, let me know what you think.

Initially I thought this approach would allow to get rid of the RAII object to 
restore the type, but I don't see a good alternative to it even with the new 
approach.


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

https://reviews.llvm.org/D56723



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

[PATCH] D56723: [CodeComplete] Propagate preferred types through parser in more cases

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 183303.
ilya-biryukov added a comment.

- Improve a comment


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

https://reviews.llvm.org/D56723

Files:
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/unittests/Sema/CodeCompleteTest.cpp

Index: clang/unittests/Sema/CodeCompleteTest.cpp
===
--- clang/unittests/Sema/CodeCompleteTest.cpp
+++ clang/unittests/Sema/CodeCompleteTest.cpp
@@ -339,4 +339,103 @@
   EXPECT_THAT(collectPreferredTypes(Code), Each("NULL TYPE"));
 }
 
+TEST(PreferredTypeTest, Members) {
+  StringRef Code = R"cpp(
+struct vector {
+  int *begin();
+  vector clone();
+};
+
+void test(int *a) {
+  a = ^vector().^clone().^begin();
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("int *"));
+}
+
+TEST(PreferredTypeTest, Conditions) {
+  StringRef Code = R"cpp(
+struct vector {
+  bool empty();
+};
+
+void test() {
+  if (^vector().^empty()) {}
+  while (^vector().^empty()) {}
+  for (; ^vector().^empty();) {}
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("_Bool"));
+}
+
+TEST(PreferredTypeTest, InitAndAssignment) {
+  StringRef Code = R"cpp(
+struct vector {
+  int* begin();
+};
+
+void test() {
+  const int* x = ^vector().^begin();
+  x = ^vector().^begin();
+
+  if (const int* y = ^vector().^begin()) {}
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("const int *"));
+}
+
+TEST(PreferredTypeTest, UnaryExprs) {
+  StringRef Code = R"cpp(
+void test(long long a) {
+  a = +^a;
+  a = -^a
+  a = ++^a;
+  a = --^a;
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("long long"));
+
+  Code = R"cpp(
+void test(int a, int *ptr) {
+  !^a;
+  !^ptr;
+  !!!^a;
+
+  a = !^a;
+  a = !^ptr;
+  a = !!!^a;
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("_Bool"));
+
+  Code = R"cpp(
+void test(int a) {
+  const int* x = &^a;
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("const int"));
+
+  Code = R"cpp(
+void test(int *a) {
+  int x = *^a;
+  int &r = *^a;
+}
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("int *"));
+
+  Code = R"cpp(
+void test(int a) {
+  *^a;
+  &^a;
+}
+
+  )cpp";
+}
+
+TEST(PreferredTypeTest, ParenExpr) {
+  StringRef Code = R"cpp(
+const int *i = ^(^(^(^10)));
+  )cpp";
+  EXPECT_THAT(collectPreferredTypes(Code), Each("const int *"));
+}
 } // namespace
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -347,6 +347,210 @@
 };
 } // namespace
 
+PreferredTypeBuilder::RestoreRAII
+PreferredTypeBuilder::enterReturn(Sema &S, SourceLocation Tok) {
+  RestoreRAII R(*this);
+  if (isa(S.CurContext)) {
+if (sema::BlockScopeInfo *BSI = S.getCurBlock()) {
+  Type = BSI->ReturnType;
+  ExpectedLoc = Tok;
+}
+return R;
+  }
+  if (const auto *Function = dyn_cast(S.CurContext)) {
+Type = Function->getReturnType();
+ExpectedLoc = Tok;
+return R;
+  }
+  if (const auto *Method = dyn_cast(S.CurContext)) {
+Type = Method->getReturnType();
+ExpectedLoc = Tok;
+return R;
+  }
+  return R;
+}
+
+PreferredTypeBuilder::RestoreRAII
+PreferredTypeBuilder::enterVariableInit(SourceLocation Tok, Decl *D) {
+  RestoreRAII R(*this);
+  auto *VD = llvm::dyn_cast_or_null(D);
+  Type = VD ? VD->getType() : QualType();
+  ExpectedLoc = Tok;
+  return R;
+}
+
+PreferredTypeBuilder::RestoreRAII
+PreferredTypeBuilder::enterParenExpr(SourceLocation Tok,
+ SourceLocation LParLoc) {
+  RestoreRAII R(*this);
+  // expected type for parenthesized expression does not change.
+  if (ExpectedLoc == LParLoc)
+ExpectedLoc = Tok;
+  return R;
+}
+
+static QualType getPreferredTypeOfBinaryRHS(Sema &S, Expr *LHS,
+tok::TokenKind Op) {
+  if (!LHS)
+return QualType();
+
+  QualType LHSType = LHS->getType();
+  if (LHSType->isPointerType()) {
+if (Op == tok::plus || Op == tok::plusequal || Op == tok::minusequal)
+  return S.getASTContext().getPointerDiffType();
+// Pointer difference is more common than subtracting an int from a pointer.
+if (Op == tok::minus)
+  return LHSType;
+  }
+
+  switch (Op) {
+  // No way to infer the type of RHS from LHS.
+  case tok::comma:
+return QualType();
+  // Prefer the type of the left operand for all of these.
+  // Arithmetic operations.
+  case tok:

[PATCH] D57155: [WebAssembly] Add a __wasi__ target macro

[Dan Gohman via Phabricator via cfe-commits]

sunfish created this revision.
sunfish added reviewers: sbc100, dschuff, aheejin.
Herald added subscribers: llvm-commits, jgravelle-google.
sunfish edited the summary of this revision.

This adds a `__wasi__` macro for the wasi OS, similar to `__linux__` etc. for 
other OS's.


Repository:
  rL LLVM

https://reviews.llvm.org/D57155

Files:
  lib/Basic/Targets.cpp
  lib/Basic/Targets/OSTargets.h
  test/Preprocessor/init.c

Index: lib/Basic/Targets.cpp
===
--- lib/Basic/Targets.cpp
+++ lib/Basic/Targets.cpp
@@ -569,19 +569,27 @@
 Triple.getVendor() != llvm::Triple::UnknownVendor ||
 !Triple.isOSBinFormatWasm())
   return nullptr;
-if (Triple.getOS() != llvm::Triple::UnknownOS &&
-Triple.getOS() != llvm::Triple::WASI)
-  return nullptr;
-return new WebAssemblyOSTargetInfo(Triple, Opts);
+switch (Triple.getOS()) {
+  case llvm::Triple::WASI:
+return new WASITargetInfo(Triple, Opts);
+  case llvm::Triple::UnknownOS:
+return new WebAssemblyOSTargetInfo(Triple, Opts);
+  default:
+return nullptr;
+}
   case llvm::Triple::wasm64:
 if (Triple.getSubArch() != llvm::Triple::NoSubArch ||
 Triple.getVendor() != llvm::Triple::UnknownVendor ||
 !Triple.isOSBinFormatWasm())
   return nullptr;
-if (Triple.getOS() != llvm::Triple::UnknownOS &&
-Triple.getOS() != llvm::Triple::WASI)
-  return nullptr;
-return new WebAssemblyOSTargetInfo(Triple, Opts);
+switch (Triple.getOS()) {
+  case llvm::Triple::WASI:
+return new WASITargetInfo(Triple, Opts);
+  case llvm::Triple::UnknownOS:
+return new WebAssemblyOSTargetInfo(Triple, Opts);
+  default:
+return nullptr;
+}
 
   case llvm::Triple::renderscript32:
 return new LinuxTargetInfo(Triple, Opts);
Index: test/Preprocessor/init.c
===
--- test/Preprocessor/init.c
+++ test/Preprocessor/init.c
@@ -9111,9 +9111,15 @@
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=wasm32-unknown-unknown \
 // RUN:   < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix=WEBASSEMBLY32 %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=wasm32-unknown-wasi \
+// RUN:   < /dev/null \
+// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY32,WEBASSEMBLY-WASI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=wasm64-unknown-unknown \
 // RUN:   < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix=WEBASSEMBLY64 %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=wasm64-unknown-wasi \
+// RUN:   < /dev/null \
+// RUN:   | FileCheck -match-full-lines -check-prefixes=WEBASSEMBLY,WEBASSEMBLY64,WEBASSEMBLY-WASI %s
 //
 // WEBASSEMBLY32:#define _ILP32 1
 // WEBASSEMBLY32-NOT:#define _LP64
@@ -9428,6 +9431,7 @@
 // WEBASSEMBLY-NEXT:#define __clang_patchlevel__ {{.*}}
 // WEBASSEMBLY-NEXT:#define __clang_version__ "{{.*}}"
 // WEBASSEMBLY-NEXT:#define __llvm__ 1
+// WEBASSEMBLY-WASI-NEXT:#define __wasi__ 1
 // WEBASSEMBLY-NOT:#define __wasm_simd128__
 // WEBASSEMBLY-NOT:#define __wasm_simd256__
 // WEBASSEMBLY-NOT:#define __wasm_simd512__
Index: lib/Basic/Targets/OSTargets.h
===
--- lib/Basic/Targets/OSTargets.h
+++ lib/Basic/Targets/OSTargets.h
@@ -764,7 +764,7 @@
 class LLVM_LIBRARY_VISIBILITY WebAssemblyOSTargetInfo
 : public OSTargetInfo {
   void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
-MacroBuilder &Builder) const final {
+MacroBuilder &Builder) const {
 // A common platform macro.
 if (Opts.POSIXThreads)
   Builder.defineMacro("_REENTRANT");
@@ -782,6 +782,21 @@
   }
 };
 
+// WASI target
+template 
+class LLVM_LIBRARY_VISIBILITY WASITargetInfo
+: public WebAssemblyOSTargetInfo {
+  void getOSDefines(const LangOptions &Opts, const llvm::Triple &Triple,
+MacroBuilder &Builder) const final {
+WebAssemblyOSTargetInfo::getOSDefines(Opts, Triple, Builder);
+Builder.defineMacro("__wasi__");
+  }
+
+public:
+  explicit WASITargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
+  : WebAssemblyOSTargetInfo(Triple, Opts) {}
+};
+
 } // namespace targets
 } // namespace clang
 #endif // LLVM_CLANG_LIB_BASIC_TARGETS_OSTARGETS_H
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r352050 - [ASTImporter] Fix inequality of functions with different attributes

[Gabor Marton via cfe-commits]

Author: martong
Date: Thu Jan 24 06:47:44 2019
New Revision: 352050

URL: http://llvm.org/viewvc/llvm-project?rev=352050&view=rev
Log:
[ASTImporter] Fix inequality of functions with different attributes

Summary:
FunctionType::ExtInfo holds such properties of a function which are needed
mostly for code gen. We should not compare these bits when checking for
structural equivalency.
Checking ExtInfo caused false ODR errors during CTU analysis (of tmux).

Reviewers: a_sidorin, a.sidorin, shafik

Subscribers: rnkovacs, dkrupp, Szelethus, cfe-commits

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

Modified:
cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp

Modified: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp?rev=352050&r1=352049&r2=352050&view=diff
==
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp (original)
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp Thu Jan 24 06:47:44 2019
@@ -296,6 +296,32 @@ static bool IsArrayStructurallyEquivalen
   return true;
 }
 
+/// Determine structural equivalence based on the ExtInfo of functions. This
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions bits but must not compare some other bits.
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ FunctionType::ExtInfo EI1,
+ FunctionType::ExtInfo EI2) {
+  // Compatible functions must have compatible calling conventions.
+  if (EI1.getCC() != EI2.getCC())
+return false;
+
+  // Regparm is part of the calling convention.
+  if (EI1.getHasRegParm() != EI2.getHasRegParm())
+return false;
+  if (EI1.getRegParm() != EI2.getRegParm())
+return false;
+
+  if (EI1.getProducesResult() != EI2.getProducesResult())
+return false;
+  if (EI1.getNoCallerSavedRegs() != EI2.getNoCallerSavedRegs())
+return false;
+  if (EI1.getNoCfCheck() != EI2.getNoCfCheck())
+return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two types.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
  QualType T1, QualType T2) {
@@ -539,7 +565,8 @@ static bool IsStructurallyEquivalent(Str
 if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
   Function2->getReturnType()))
   return false;
-if (Function1->getExtInfo() != Function2->getExtInfo())
+if (!IsStructurallyEquivalent(Context, Function1->getExtInfo(),
+  Function2->getExtInfo()))
   return false;
 break;
   }

Modified: cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp?rev=352050&r1=352049&r2=352050&view=diff
==
--- cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp (original)
+++ cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp Thu Jan 24 06:47:44 
2019
@@ -370,6 +370,31 @@ TEST_F(StructuralEquivalenceFunctionTest
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceFunctionTest, FunctionsWithDifferentNoreturnAttr) {
+  auto t = makeNamedDecls(
+  "__attribute__((noreturn)) void foo();",
+  "  void foo();",
+  Lang_C);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentCallingConventions) {
+  auto t = makeNamedDecls(
+  "__attribute__((fastcall)) void foo();",
+  "__attribute__((ms_abi))   void foo();",
+  Lang_C);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceFunctionTest, FunctionsWithDifferentSavedRegsAttr) 
{
+  auto t = makeNamedDecls(
+  "__attribute__((no_caller_saved_registers)) void foo();",
+  "   void foo();",
+  Lang_C);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
 };
 


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

[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352050: [ASTImporter] Fix inequality of functions with 
different attributes (authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53699?vs=178060&id=183307#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D53699

Files:
  lib/AST/ASTStructuralEquivalence.cpp
  unittests/AST/StructuralEquivalenceTest.cpp


Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -296,6 +296,32 @@
   return true;
 }
 
+/// Determine structural equivalence based on the ExtInfo of functions. This
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions bits but must not compare some other bits.
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ FunctionType::ExtInfo EI1,
+ FunctionType::ExtInfo EI2) {
+  // Compatible functions must have compatible calling conventions.
+  if (EI1.getCC() != EI2.getCC())
+return false;
+
+  // Regparm is part of the calling convention.
+  if (EI1.getHasRegParm() != EI2.getHasRegParm())
+return false;
+  if (EI1.getRegParm() != EI2.getRegParm())
+return false;
+
+  if (EI1.getProducesResult() != EI2.getProducesResult())
+return false;
+  if (EI1.getNoCallerSavedRegs() != EI2.getNoCallerSavedRegs())
+return false;
+  if (EI1.getNoCfCheck() != EI2.getNoCfCheck())
+return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two types.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
  QualType T1, QualType T2) {
@@ -539,7 +565,8 @@
 if (!IsStructurallyEquivalent(Context, Function1->getReturnType(),
   Function2->getReturnType()))
   return false;
-if (Function1->getExtInfo() != Function2->getExtInfo())
+if (!IsStructurallyEquivalent(Context, Function1->getExtInfo(),
+  Function2->getExtInfo()))
   return false;
 break;
   }
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -370,6 +370,31 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceFunctionTest, FunctionsWithDifferentNoreturnAttr) {
+  auto t = makeNamedDecls(
+  "__attribute__((noreturn)) void foo();",
+  "  void foo();",
+  Lang_C);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceFunctionTest,
+FunctionsWithDifferentCallingConventions) {
+  auto t = makeNamedDecls(
+  "__attribute__((fastcall)) void foo();",
+  "__attribute__((ms_abi))   void foo();",
+  Lang_C);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceFunctionTest, FunctionsWithDifferentSavedRegsAttr) 
{
+  auto t = makeNamedDecls(
+  "__attribute__((no_caller_saved_registers)) void foo();",
+  "   void foo();",
+  Lang_C);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
 };
 


Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -296,6 +296,32 @@
   return true;
 }
 
+/// Determine structural equivalence based on the ExtInfo of functions. This
+/// is inspired by ASTContext::mergeFunctionTypes(), we compare calling
+/// conventions bits but must not compare some other bits.
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ FunctionType::ExtInfo EI1,
+ FunctionType::ExtInfo EI2) {
+  // Compatible functions must have compatible calling conventions.
+  if (EI1.getCC() != EI2.getCC())
+return false;
+
+  // Regparm is part of the calling convention.
+  if (EI1.getHasRegParm() != EI2.getHasRegParm())
+return false;
+  if (EI1.getRegParm() != EI2.getRegParm())
+return false;
+
+  if (EI1.getProducesResult() != EI2.getProducesResult())
+return false;
+  if (EI1.getNoCallerSavedRegs() != EI2.getNoCallerSavedRegs())
+return false;
+  if (EI1.getNoCfCheck() != EI2.getNoCfCheck())
+return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two types.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
  QualType T1, QualType T2) {
@@ -539,7 +565,8 @@
 if (!IsStructurallyE

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

[Benjamin Kramer via Phabricator via cfe-commits]

bkramer accepted this revision.
bkramer added inline comments.



Comment at: lib/Lex/HeaderSearch.cpp:313
   // check whether we'll have a suggestion for a module.
-  const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
+  const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/false);
   if (!File)

This deserves a comment that we don't open it because we might not need it. If 
we would use it the file would be closed after reading the contents.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57150



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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D55850#1369008 , @ebevhan wrote:

> I think the code that comes to mind is mostly like in 
> `GetTypeSourceInfoForDeclarator`:
>
>   LangAS AS =
>   (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx);
>   
>
> It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if 
> it's not OpenCL++, but there shouldn't be a reason why the rest of the code 
> in that block won't work for regular C++.
>
> In fact, in most regular C++ this would be an issue, since Default typically 
> _is_ the 'generic' address space there.


Yes, `opencl_generic` is something quite specific to OpenCL I feel. I am not 
sure we can generalize it to C++.  I would quite like to understand this 
better. However, I think we could potentially implement `opencl_generic` using 
`Default` if only earlier OpenCL implementations wouldn't use `Default` for 
`opencl_private`. And because everything was built on top of this assumption 
it's now a big and a scary refactoring change to make. However, I think we can 
align different language modes better if `opencl_generic` could be implemented 
as `Default`. I might look into this at some point.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55850



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

r352054 - [CPU-Dispatch] Make pentium_iii_no_xmm_regs and pentium_iii alias.

[Erich Keane via cfe-commits]

Author: erichkeane
Date: Thu Jan 24 07:28:57 2019
New Revision: 352054

URL: http://llvm.org/viewvc/llvm-project?rev=352054&view=rev
Log:
[CPU-Dispatch] Make pentium_iii_no_xmm_regs and pentium_iii alias.

I discovered that in ICC (where this list comes from), that the two
pentium_iii versions were actually identical despite the two different
names (despite them implying a difference). Because of this, they ended
up having identical manglings, which obviously caused problems when used
together.

This patch makes pentium_iii_no_xmm_regs an alias for pentium_iii so
that it can still be used, but has the same meaning as ICC. However, we
still prohibit using the two together which is different (albeit better)
behavior.

Change-Id: I4f3c9a47e48490c81525c8a3d23ed4201921b288

Modified:
cfe/trunk/include/clang/Basic/X86Target.def
cfe/trunk/test/Sema/attr-cpuspecific.c

Modified: cfe/trunk/include/clang/Basic/X86Target.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/X86Target.def?rev=352054&r1=352053&r2=352054&view=diff
==
--- cfe/trunk/include/clang/Basic/X86Target.def (original)
+++ cfe/trunk/include/clang/Basic/X86Target.def Thu Jan 24 07:28:57 2019
@@ -300,7 +300,7 @@ CPU_SPECIFIC("pentium_pro", 'C', "+cmov"
 CPU_SPECIFIC("pentium_mmx", 'D', "+mmx")
 CPU_SPECIFIC("pentium_ii", 'E', "+cmov,+mmx")
 CPU_SPECIFIC("pentium_iii", 'H', "+cmov,+mmx,+sse")
-CPU_SPECIFIC("pentium_iii_no_xmm_regs", 'H',"+cmov,+sse")
+CPU_SPECIFIC_ALIAS("pentium_iii_no_xmm_regs", "pentium_iii")
 CPU_SPECIFIC("pentium_4", 'J', "+cmov,+mmx,+sse,+sse2")
 CPU_SPECIFIC("pentium_m", 'K', "+cmov,+mmx,+sse,+sse2")
 CPU_SPECIFIC("pentium_4_sse3", 'L', "+cmov,+mmx,+sse,+sse2,+sse3")

Modified: cfe/trunk/test/Sema/attr-cpuspecific.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/attr-cpuspecific.c?rev=352054&r1=352053&r2=352054&view=diff
==
--- cfe/trunk/test/Sema/attr-cpuspecific.c (original)
+++ cfe/trunk/test/Sema/attr-cpuspecific.c Thu Jan 24 07:28:57 2019
@@ -112,3 +112,6 @@ int __attribute__((cpu_specific(pentium_
 int use3(void) {
   return called_invalid_value();
 }
+
+// expected-warning@+1 {{CPU list contains duplicate entries; attribute 
ignored}}
+int __attribute__((cpu_dispatch(pentium_iii, pentium_iii_no_xmm_regs))) 
dupe_p3(void);


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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked 2 inline comments as done.
riccibruno added inline comments.



Comment at: include/clang/AST/Expr.h:5095
 
-  SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; }
-  SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }

steveire wrote:
> Why remove the `LLVM_READONLY` from this class instead of from everywhere in 
> the file it is not needed? (in a separate commit, obviously).
`LLVM_READONLY` is a macro for `__attribute__((pure))` (when supported). It is 
useful in some cases to give a hint to the optimizer about the behavior of a 
function. However in this case the definition of the function is visible in all 
translation units using this function. Therefore the optimizer can very well 
see on its own what this function is doing.

It would be perfectly possible to inspect all uses of `LLVM_READONLY` and only 
keep what is needed, but this is such a small issue that nobody bothered to do 
it until now.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: include/clang/AST/Expr.h:5095
 
-  SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; }
-  SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }

riccibruno wrote:
> steveire wrote:
> > Why remove the `LLVM_READONLY` from this class instead of from everywhere 
> > in the file it is not needed? (in a separate commit, obviously).
> `LLVM_READONLY` is a macro for `__attribute__((pure))` (when supported). It 
> is useful in some cases to give a hint to the optimizer about the behavior of 
> a function. However in this case the definition of the function is visible in 
> all translation units using this function. Therefore the optimizer can very 
> well see on its own what this function is doing.
> 
> It would be perfectly possible to inspect all uses of `LLVM_READONLY` and 
> only keep what is needed, but this is such a small issue that nobody bothered 
> to do it until now.
My point was again about commit hygiene.

You could make a commit changing this without further review, leaving this 
review cleaner. You could do the same with the moved friend decl. Commits which 
only do one thing are easier to review.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Bruno Ricci via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/Expr.h:5095
 
-  SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; }
-  SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }

steveire wrote:
> riccibruno wrote:
> > steveire wrote:
> > > Why remove the `LLVM_READONLY` from this class instead of from everywhere 
> > > in the file it is not needed? (in a separate commit, obviously).
> > `LLVM_READONLY` is a macro for `__attribute__((pure))` (when supported). It 
> > is useful in some cases to give a hint to the optimizer about the behavior 
> > of a function. However in this case the definition of the function is 
> > visible in all translation units using this function. Therefore the 
> > optimizer can very well see on its own what this function is doing.
> > 
> > It would be perfectly possible to inspect all uses of `LLVM_READONLY` and 
> > only keep what is needed, but this is such a small issue that nobody 
> > bothered to do it until now.
> My point was again about commit hygiene.
> 
> You could make a commit changing this without further review, leaving this 
> review cleaner. You could do the same with the moved friend decl. Commits 
> which only do one thing are easier to review.
Ah I see. Yes I can do this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 183314.
riccibruno added a comment.

Made `GenericSelectionExpr::AssociationIterator` a random access iterator.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106

Files:
  include/clang/AST/Expr.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/AST/StmtDataCollectors.td
  lib/AST/ASTDumper.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/StmtProfile.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaPseudoObject.cpp
  lib/Sema/TreeTransform.h

Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -9071,10 +9071,10 @@
 
   SmallVector AssocExprs;
   SmallVector AssocTypes;
-  for (unsigned i = 0; i != E->getNumAssocs(); ++i) {
-TypeSourceInfo *TS = E->getAssocTypeSourceInfo(i);
-if (TS) {
-  TypeSourceInfo *AssocType = getDerived().TransformType(TS);
+  for (GenericSelectionExpr::Association Assoc : E->associations()) {
+TypeSourceInfo *TSI = Assoc.getTypeSourceInfo();
+if (TSI) {
+  TypeSourceInfo *AssocType = getDerived().TransformType(TSI);
   if (!AssocType)
 return ExprError();
   AssocTypes.push_back(AssocType);
@@ -9082,7 +9082,8 @@
   AssocTypes.push_back(nullptr);
 }
 
-ExprResult AssocExpr = getDerived().TransformExpr(E->getAssocExpr(i));
+ExprResult AssocExpr =
+getDerived().TransformExpr(Assoc.getAssociationExpr());
 if (AssocExpr.isInvalid())
   return ExprError();
 AssocExprs.push_back(AssocExpr.get());
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -140,19 +140,22 @@
 unsigned resultIndex = gse->getResultIndex();
 unsigned numAssocs = gse->getNumAssocs();
 
-SmallVector assocs(numAssocs);
-SmallVector assocTypes(numAssocs);
-
-for (unsigned i = 0; i != numAssocs; ++i) {
-  Expr *assoc = gse->getAssocExpr(i);
-  if (i == resultIndex) assoc = rebuild(assoc);
-  assocs[i] = assoc;
-  assocTypes[i] = gse->getAssocTypeSourceInfo(i);
+SmallVector assocExprs;
+SmallVector assocTypes;
+assocExprs.reserve(numAssocs);
+assocTypes.reserve(numAssocs);
+
+for (GenericSelectionExpr::Association assoc : gse->associations()) {
+  Expr *assocExpr = assoc.getAssociationExpr();
+  if (assoc.isSelected())
+assocExpr = rebuild(assocExpr);
+  assocExprs.push_back(assocExpr);
+  assocTypes.push_back(assoc.getTypeSourceInfo());
 }
 
 return GenericSelectionExpr::Create(
 S.Context, gse->getGenericLoc(), gse->getControllingExpr(),
-assocTypes, assocs, gse->getDefaultLoc(), gse->getRParenLoc(),
+assocTypes, assocExprs, gse->getDefaultLoc(), gse->getRParenLoc(),
 gse->containsUnexpandedParameterPack(), resultIndex);
   }
 
Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -4332,14 +4332,16 @@
 assert(!gse->isResultDependent());
 
 unsigned n = gse->getNumAssocs();
-SmallVector subExprs(n);
-SmallVector subTypes(n);
-for (unsigned i = 0; i != n; ++i) {
-  subTypes[i] = gse->getAssocTypeSourceInfo(i);
-  Expr *sub = gse->getAssocExpr(i);
-  if (i == gse->getResultIndex())
+SmallVector subExprs;
+SmallVector subTypes;
+subExprs.reserve(n);
+subTypes.reserve(n);
+for (GenericSelectionExpr::Association assoc : gse->associations()) {
+  subTypes.push_back(assoc.getTypeSourceInfo());
+  Expr *sub = assoc.getAssociationExpr();
+  if (assoc.isSelected())
 sub = stripARCUnbridgedCast(sub);
-  subExprs[i] = sub;
+  subExprs.push_back(sub);
 }
 
 return GenericSelectionExpr::Create(
Index: lib/AST/StmtProfile.cpp
===
--- lib/AST/StmtProfile.cpp
+++ lib/AST/StmtProfile.cpp
@@ -1259,13 +1259,13 @@
 
 void StmtProfiler::VisitGenericSelectionExpr(const GenericSelectionExpr *S) {
   VisitExpr(S);
-  for (unsigned i = 0; i != S->getNumAssocs(); ++i) {
-QualType T = S->getAssocType(i);
+  for (GenericSelectionExpr::ConstAssociation Assoc : S->associations()) {
+QualType T = Assoc.getType();
 if (T.isNull())
   ID.AddPointer(nullptr);
 else
   VisitType(T);
-VisitExpr(S->getAssocExpr(i));
+VisitExpr(Assoc.getAssociationExpr());
   }
 }
 
Index: lib/AST/StmtPrinter.cpp
===
--- lib/AST/StmtPrinter.cpp
+++ lib/AST/StmtPrinter.cpp
@@ -1261,15 +1261,15 @@
 void StmtPrinter::VisitGenericSelectionExpr(GenericSelectionExpr *Node) {
   OS << "_Generic(";
  

r352055 - Fix failing buildbots

[Gabor Marton via cfe-commits]

Author: martong
Date: Thu Jan 24 07:42:20 2019
New Revision: 352055

URL: http://llvm.org/viewvc/llvm-project?rev=352055&view=rev
Log:
Fix failing buildbots

Related commit which caused the buildbots to fail:
rL352050

Modified:
cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp

Modified: cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp?rev=352055&r1=352054&r2=352055&view=diff
==
--- cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp (original)
+++ cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp Thu Jan 24 07:42:20 
2019
@@ -378,14 +378,17 @@ TEST_F(StructuralEquivalenceFunctionTest
   EXPECT_TRUE(testStructuralMatch(t));
 }
 
+// These calling conventions may not be available on certain platforms.
+#if defined(__x86_64__) && defined(__linux__)
 TEST_F(StructuralEquivalenceFunctionTest,
 FunctionsWithDifferentCallingConventions) {
   auto t = makeNamedDecls(
-  "__attribute__((fastcall)) void foo();",
+  "__attribute__((preserve_all)) void foo();",
   "__attribute__((ms_abi))   void foo();",
   Lang_C);
   EXPECT_FALSE(testStructuralMatch(t));
 }
+#endif
 
 TEST_F(StructuralEquivalenceFunctionTest, FunctionsWithDifferentSavedRegsAttr) 
{
   auto t = makeNamedDecls(


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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Sam McCall via cfe-commits]

Sure. Op counts and linux benchmarks are easy, I could try to get access to
mac/windows too...

Building clang's HeaderSearch.cpp (without PCH) yields:
master: fstat = 341, open = 1039, stat = 211
openFile=false: fstat = 9 (-332), open = 379 (-660), stat = 1203 (+992)
revert: fstat = 341 (+0), open = 1039 (+0), stat = 211 (+0)
(this is on linux, but that shouldn't matter to the counts)

Timings on my workstation (-fsyntax-only, disk caches flushed, warmed up by
running clang with no args):
master: 2.063
openFile=false patch: 1.953
revert: 1.946
(stdev is around 0.07)
So openFile=false a regression of 6% or so on linux, I don't think it's
worth testing anything else :-\

Best plan I have now is:
 - revert r347205, disable tests that this breaks
 - cherrypick the revert to 8.0 branch
 - reapply r347205 + a version of this patch where the openFile value is
plumbed through, with false for the PCH case.
I'll send the revert out shortly. It should probably get reviewed since it
won't be clean.
The final patch is going to be ugly, but there's time to come up with a
better idea.

On Thu, Jan 24, 2019 at 3:26 PM Nico Weber  wrote:

> Can you measure on macOS and Windows too, and maybe with a larger program?
> The open+fstat (instead of stat+open) optimization was I think done with
> benchmarking on macOS.
>
> I can see why you say you think the new behavior is more consistent, but
> it's basically changing what information goes into the getFile() cache key.
> Previously, openFile didn't go in there, and from that perspective saying
> "the cache keeps the openFile passed the first time you call this for a
> file" makes sense to me too.
>
> I'm happy with either before your patch or with getFileAndSuggestModule()
> passing openFile=false if perf is comparable (which I expect it to be, to
> be honest).
>
> Since the openFile=false change might be a bit risky, maybe we should
> revert on 8.0 and do openFile=false only on trunk.
>
> On Thu, Jan 24, 2019 at 6:51 AM Sam McCall  wrote:
>
>> So from my reading starting at getFileAndSuggestModule, changing to
>> openFile=false is correct there unless some code is relying on that side
>> effect.
>> All the tests pass, too.
>>
>> The most likely regression is where we previously did open+fstat we'll
>> now do stat+open+fstat.
>>
>> I tried running three commands using your repro, counting stat/open/fstat
>> by instrumenting Unix/Path.inc.
>> I used three versions of the code: old = before my patch, mid = with
>> r347205, new = with r347205 and openFile=false.
>>
>> Build PCH:
>>   stat: 331 -> 331 -> 635
>>   open: 316 -> 314 -> 316
>>   fstat: 311 -> 311 -> 8
>>
>> Use PCH:
>>  stat: 322 -> 322 -> 924
>>  open: 910 -> 609 -> 308
>>  fstat: 607 -> 607 -> 5
>>
>> No PCH:
>>  stat: 16 -> 16 -> 617
>>  open: 606 -> 606 -> 306
>>  fstat: 604 -> 604 -> 3
>>
>> Summary:
>>  - r347205 was neutral for stats and fstats. It caused fewer files to be
>> opened when building a PCH (though failed to close the ones it did). It
>> caused file handles to leak when using a PCH.
>>  - openFile=false reduces the number of files opened when compiling (with
>> or without PCH). It also changes most fstats to stats, which is a loss.
>>  - compared to the original state, having both patches seems like a small
>> loss when building a PCH, and a small win when using one or compiling
>> without one.
>>
>> So I'm tempted to check in the openFile=false.
>> It seems more internally consistent (passing openFile=true leaves the
>> file open), and avoids breaking clangd.
>> It seems uninvasive, though as we've seen side-effects are hard to
>> predict. Performance seems likely to be not-worse (vs reverting r347205).
>>
>> On Thu, Jan 24, 2019 at 10:49 AM Sam McCall  wrote:
>>
>>> Thanks for all the digging!
>>> I can reproduce locally with your last example and ulimit -n 1000.
>>>
>>> So it sounds like:
>>>  - this *is* actually the interaction/behavior I expected from
>>> FileManager (we call first with openFile=false then with openFile=true, and
>>> the file is opened the second time)
>>>  - the problem manifests because the file is opened but never closed, so
>>> we leak file descriptors, but closing the file would'nt be the best fix -
>>> we shouldn't open it if we don't need its content
>>>  - I would call this a bug in the caller of file manager (in the stack
>>> we saw where getFileAndSuggestModule calls getFile), because it calls with
>>> openFile=true, but doesn't use the file contents nor ensure the file is
>>> closed. This was previously masked by the bug (fixed in my patch) where
>>> openFile=true could result in not opening the file.
>>>  - nevertheless if I can't find a fairly safe fix soon then we'll need
>>> to revert this patch.
>>>
>>> On Wed, Jan 23, 2019 at 10:27 PM Nico Weber  wrote:
>>>
 For completeness, in case the construction isn't clear, here's a full
 repro of the symptom.

 $ cat pch.cc
 $ cat pch.h
 #include "foo.h"
 $ rm bar/foo.h
 $ for i in {1..30

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sigh, this regresses perf by 6%ish in my testing (building HeaderSearch.cpp 
with -fsyntax-only and a cold disk cache).
I think we'll need a more targeted fix: probably passing openFile=false only in 
the using-PCH case.

Plan for now is to revert the original change, live with the things that are 
broken, cherrypick the revert to the llvm8 branch, then revisit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57150



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

[PATCH] D57160: [WebAssembly] Add an import_module function attribute

[Dan Gohman via Phabricator via cfe-commits]

sunfish created this revision.
sunfish added reviewers: sbc100, dschuff, aheejin.
Herald added subscribers: llvm-commits, jgravelle-google.

This adds a C/C++ attribute which corresponds to the LLVM IR wasm-import-module 
attribute. It allows code to specify an explicit import module.


Repository:
  rL LLVM

https://reviews.llvm.org/D57160

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/wasm-import-module.c

Index: test/CodeGen/wasm-import-module.c
===
--- test/CodeGen/wasm-import-module.c
+++ test/CodeGen/wasm-import-module.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -triple wasm32-unknown-unknown-wasm -emit-llvm -o - %s | FileCheck %s
+
+void __attribute__((import_module("bar"))) foo(void);
+
+void call(void) {
+  foo();
+}
+
+// CHECK: declare void @foo() [[A:#[0-9]+]]
+
+// CHECK: attributes [[A]] = {{{.*}} "wasm-import-module"="bar" {{.*}}}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -5734,7 +5734,29 @@
   handleSimpleAttribute(S, D, AL);
 }
 
+static void handleWebAssemblyImportModuleAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (!isFunctionOrMethod(D)) {
+S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+<< "'import_module'" << ExpectedFunction;
+return;
+  }
 
+  auto *FD = cast(D);
+  if (FD->isThisDeclarationADefinition()) {
+S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
+return;
+  }
+
+  StringRef Str;
+  SourceLocation ArgLoc;
+  if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &ArgLoc))
+return;
+
+  FD->addAttr(::new (S.Context) WebAssemblyImportModuleAttr(
+  AL.getRange(), S.Context, Str,
+  AL.getAttributeSpellingListIndex()));
+}
+
 static void handleRISCVInterruptAttr(Sema &S, Decl *D,
  const ParsedAttr &AL) {
   // Warn about repeated attributes.
@@ -6487,6 +6509,9 @@
   case ParsedAttr::AT_AVRSignal:
 handleAVRSignalAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_WebAssemblyImportModule:
+handleWebAssemblyImportModuleAttr(S, D, AL);
+break;
   case ParsedAttr::AT_IBAction:
 handleSimpleAttribute(S, D, AL);
 break;
Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -760,6 +760,16 @@
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule &CGM) const override {
+TargetCodeGenInfo::setTargetAttributes(D, GV, CGM);
+if (const auto *FD = dyn_cast_or_null(D)) {
+  if (const auto *Attr = FD->getAttr()) {
+llvm::Function *Fn = cast(GV);
+llvm::AttrBuilder B;
+B.addAttribute("wasm-import-module", Attr->getImportModuleName());
+Fn->addAttributes(llvm::AttributeList::FunctionIndex, B);
+  }
+}
+
 if (auto *FD = dyn_cast_or_null(D)) {
   llvm::Function *Fn = cast(GV);
   if (!FD->doesThisDeclarationHaveABody() && !FD->hasPrototype())
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -3651,7 +3651,23 @@
 For more information see
 `gcc documentation `_
 or `msvc documentation `_.
-}];
+}]; }
+
+def WebAssemblyImportModuleDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+Clang supports the ``__attribute__((import_module()))`` 
+attribute for the WebAssembly target. This attribute may be attached to a
+function declaration, where it modifies how the symbol is to be imported
+within the WebAssembly linking environment.
+
+WebAssembly imports use a two-level namespace scheme, consisting of a module
+name, which typically identifies a module from which to import, and a field
+name, which typically identifies a field from that module to import. By
+default, module names for C/C++ symbols are assigned automatically by the
+linker. This attribute can be used to override the default behavior, and
+reuqest a specific module name be used instead.
+  }];
 }
 
 def ArtificialDocs : Documentation {
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -331,6 +331,7 @@
 def TargetRISCV : TargetArch<["riscv32", "riscv64"]>;
 def TargetX86 : TargetArch<["x86"]>;
 def TargetAnyX86 : TargetArch<["x86", "x86_64"]>;
+def TargetWebAssembly : TargetArch<["wasm32", "wasm64"]>;
 def TargetWindows : TargetArch<["x86", "x86_64", "arm", "thumb", "aarch64"]> {
   let OSes

[PATCH] D57162: [DEBUG_INFO][NVPTX] Generate correct data about variable address class.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev created this revision.
ABataev added reviewers: echristo, probinson.
Herald added subscribers: aprantl, jholewinski.

Added ability to generate correct debug info data about the variable
address class. Currently, for all the locals and globals the default
values are used, ADDR_local_space(6) for locals and ADDR_global_space(5)
for globals. The values are taken from the table in

  
https://docs.nvidia.com/cuda/archive/10.0/ptx-writers-guide-to-interoperability/index.html#cuda-specific-dwarf.
  We need to emit correct data for address classes of, at least, shared
  and constant globals. Currently, all these variables are treated by
  the cuda-gdb debugger as the variables in the global address space
  and, thus, it require manual data type casting.


Repository:
  rC Clang

https://reviews.llvm.org/D57162

Files:
  lib/Basic/Targets/NVPTX.h
  lib/CodeGen/CGDebugInfo.cpp
  test/CodeGenCUDA/debug-info-address-class.cu


Index: test/CodeGenCUDA/debug-info-address-class.cu
===
--- /dev/null
+++ test/CodeGenCUDA/debug-info-address-class.cu
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -fcuda-is-device -triple 
nvptx-unknown-unknown -debug-info-kind=limited -dwarf-version=2 
-debugger-tuning=gdb | FileCheck %s
+
+#include "Inputs/cuda.h"
+
+// CHECK-DAG: ![[FILEVAR0:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FileVar0", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: false, isDefinition: true)
+// CHECK-DAG: !DIGlobalVariableExpression(var: ![[FILEVAR0]], expr: 
!DIExpression())
+__device__ int FileVar0;
+// CHECK-DAG: ![[FILEVAR1:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FileVar1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: false, isDefinition: true)
+// CHECK-DAG: !DIGlobalVariableExpression(var: ![[FILEVAR1]], expr: 
!DIExpression(DW_OP_constu, 8, DW_OP_swap, DW_OP_xderef))
+__device__ __shared__ int FileVar1;
+// CHECK-DAG: ![[FILEVAR2:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FileVar2", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: false, isDefinition: true)
+// CHECK-DAG: !DIGlobalVariableExpression(var: ![[FILEVAR2]], expr: 
!DIExpression(DW_OP_constu, 4, DW_OP_swap, DW_OP_xderef))
+__device__ __constant__ int FileVar2;
+
+__device__ void kernel1(
+// CHECK-DAG: ![[ARG:[0-9]+]] = !DILocalVariable(name: "Arg", arg: 
{{[0-9]+}}, scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}})
+// CHECK-DAG: call void @llvm.dbg.declare(metadata i32* {{.*}}, metadata 
![[ARG]], metadata !DIExpression()), !dbg !{{[0-9]+}}
+int Arg) {
+// CHECK-DAG: ![[FUNCVAR0:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FuncVar0", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: true, isDefinition: true)
+// CHECK-DAG: !DIGlobalVariableExpression(var: ![[FUNCVAR0]], expr: 
!DIExpression(DW_OP_constu, 8, DW_OP_swap, DW_OP_xderef))
+  __shared__ int FuncVar0;
+  // CHECK-DAG: ![[FUNCVAR1:[0-9]+]] = !DILocalVariable(name: "FuncVar1", 
scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
+  // CHECK-DAG: call void @llvm.dbg.declare(metadata i32* {{.*}}, metadata 
![[FUNCVAR1]], metadata !DIExpression()), !dbg !{{[0-9]+}}
+  int FuncVar1;
+}
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -4232,6 +4232,14 @@
 SmallVector Expr;
 unsigned AddressSpace =
 CGM.getContext().getTargetAddressSpace(D->getType());
+if (CGM.getLangOpts().CUDA && CGM.getLangOpts().CUDAIsDevice) {
+  if (D->hasAttr())
+AddressSpace =
+CGM.getContext().getTargetAddressSpace(LangAS::cuda_shared);
+  else if (D->hasAttr())
+AddressSpace =
+CGM.getContext().getTargetAddressSpace(LangAS::cuda_constant);
+}
 AppendAddressSpaceXDeref(AddressSpace, Expr);
 
 GVE = DBuilder.createGlobalVariableExpression(
Index: lib/Basic/Targets/NVPTX.h
===
--- lib/Basic/Targets/NVPTX.h
+++ lib/Basic/Targets/NVPTX.h
@@ -35,6 +35,16 @@
 3, // cuda_shared
 };
 
+/// The DWARF address class. Taken from
+/// 
https://docs.nvidia.com/cuda/archive/10.0/ptx-writers-guide-to-interoperability/index.html#cuda-specific-dwarf
+static const int NVPTXDWARFAddrSpaceMap[] = {
+-1, // Default, opencl_private or opencl_generic - not defined
+5,  // opencl_global
+-1,
+8,  // opencl_local or cuda_shared
+4,  // opencl_constant or cuda_constant
+};
+
 class LLVM_LIBRARY_VISIBILITY NVPTXTargetInfo : public TargetInfo {
   static const char *const GCCRegNames[];
   static const Builtin::Info BuiltinInfo[];
@@ -124,6 +134,20 @@
 Opts.support("cl_khr_local_int32_extended_atomics");
   }
 
+  /// \returns If a target req

r352060 - Fix failing buildbots

[Gabor Marton via cfe-commits]

Author: martong
Date: Thu Jan 24 08:27:21 2019
New Revision: 352060

URL: http://llvm.org/viewvc/llvm-project?rev=352060&view=rev
Log:
Fix failing buildbots

Fix remaining unittest errors caused by
__attribute__((no_caller_saved_registers))
Related commit which caused the buildbots to fail:
rL352050

Modified:
cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp

Modified: cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp?rev=352060&r1=352059&r2=352060&view=diff
==
--- cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp (original)
+++ cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp Thu Jan 24 08:27:21 
2019
@@ -378,7 +378,7 @@ TEST_F(StructuralEquivalenceFunctionTest
   EXPECT_TRUE(testStructuralMatch(t));
 }
 
-// These calling conventions may not be available on certain platforms.
+// These attributes may not be available on certain platforms.
 #if defined(__x86_64__) && defined(__linux__)
 TEST_F(StructuralEquivalenceFunctionTest,
 FunctionsWithDifferentCallingConventions) {
@@ -388,7 +388,6 @@ TEST_F(StructuralEquivalenceFunctionTest
   Lang_C);
   EXPECT_FALSE(testStructuralMatch(t));
 }
-#endif
 
 TEST_F(StructuralEquivalenceFunctionTest, FunctionsWithDifferentSavedRegsAttr) 
{
   auto t = makeNamedDecls(
@@ -397,6 +396,7 @@ TEST_F(StructuralEquivalenceFunctionTest
   Lang_C);
   EXPECT_FALSE(testStructuralMatch(t));
 }
+#endif
 
 struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
 };


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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

In D56554#1357385 , @mgorny wrote:

> @ruiu, what do you think? The current logic forces some precedence, i.e. if 
> you pass `-z nognustack`, further `-z {no,}execstack` are ignored. I suppose 
> we could just force passing `-z nognustack` as last option clang-wise.


Are you asking about making the flag tri-state? If so, I think it logically 
makes sense and feels more natural to represent it with two boolean flags. As 
you said, the last one should always takes precedence over the others among the 
three flags.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D57001: [libunwind] Don't define unw_fpreg_t to uint64_t for __ARM_DWARF_EH__

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

`double` should be safe for ARM DWARF EH, though, technically, `long double` is 
more appropriate of a type definition (the FPU state that is saved should be 
the widest floating point type).  That would be `long double` (aka FP80 on x86, 
FP128 on AArch64/PPC, FP64 elsewhere).  This happens to work because ARM uses 
FP64 irrespective of DWARF or EHABI.


Repository:
  rUNW libunwind

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

https://reviews.llvm.org/D57001



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

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: dblaikie, mclow.lists.
aaron.ballman added subscribers: dblaikie, mclow.lists.
aaron.ballman added a comment.

Adding some additional reviewers to help tease out the iterator design 
questions.




Comment at: include/clang/AST/Expr.h:5103
+using reference = AssociationTy;
+using pointer = AssociationTy;
+AssociationIteratorTy() = default;

riccibruno wrote:
> aaron.ballman wrote:
> > Carrying over the conversation from D57098:
> > 
> > >> @aaron.ballman Cute, but I suspect this may come back to bite us at some 
> > >> point. For instance, if someone thinks they're working with a real 
> > >> pointer, they're likely to expect pointer arithmetic to work when it 
> > >> won't (at least they'll get compile errors though).
> > > @riccibruno Hmm, but pointer is just the return type of operator-> no ? 
> > > Is it actually required to behave like a pointer ? The only requirement I 
> > > can find is that It->x must be equivalent to (*It).x, which is true here.
> > 
> > I double-checked and you're right, this is not a requirement of the STL.
> > 
> > > Also looking at the requirements for forward iterators I think that this 
> > > iterator should actually be downgraded to an input iterator, because of 
> > > the requirement that reference = T&.
> > 
> > My concern is that with the less functional iterators, common algorithms 
> > get more expensive. For instance, `std::distance()`, `std::advance()` 
> > become more expensive without random access, and things like `std::prev()` 
> > become impossible.
> > 
> > It seems like the view this needs to provide is a read-only access to the 
> > `AssociationTy` objects (because we need to construct them on the fly), but 
> > not the data contained within them. If the only thing you can get back from 
> > the iterator is a const pointer/reference/value type, then we could store a 
> > local "current association" object in the iterator and return a 
> > pointer/reference to that. WDYT?
> I am worried about lifetime issues with this approach. Returning a 
> reference/pointer to an `Association` object stored in the iterator means 
> that the reference/pointer will dangle as soon as the iterator goes out of 
> scope. This is potentially surprising since the "container" (that is the 
> `GenericSelectionExpr`) here will still be in scope. Returning a value is 
> safer in this aspect.
> 
> I believe it should be possible to make the iterator a random access 
> iterator, at least
> if we are willing to ignore some requirements:
> 
> 1.) For forward iterators and up, we must have `reference = T&` or `const T&`.
> 2.) For forward iterators and up, `It1 == It2` if and only if `*It1` and 
> `*It2` are bound to the same object.
> I am worried about lifetime issues with this approach. Returning a 
> reference/pointer to an Association object stored in the iterator means that 
> the reference/pointer will dangle as soon as the iterator goes out of scope. 
> This is potentially surprising since the "container" (that is the 
> GenericSelectionExpr) here will still be in scope. Returning a value is safer 
> in this aspect.

That's valid.

> I believe it should be possible to make the iterator a random access 
> iterator, at least if we are willing to ignore some requirements:
>
> 1.) For forward iterators and up, we must have reference = T& or const T&.
> 2.) For forward iterators and up, It1 == It2 if and only if *It1 and *It2 are 
> bound to the same object.

Yes, but then passing these iterators to STL algorithms will have UB because we 
claim to meet the requirements for random access iteration but don't actually 
meet the requirements. I am not certain what problems might arise from 
violating these requirements.

@dblaikie, @mclow.lists: do you have opinions on whether it's okay to not meet 
these requirements or suggestions on what we could do differently with the 
design?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106



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

[PATCH] D50563: Fixed frontend clang tests in windows read-only container

[Justice Adams via Phabricator via cfe-commits]

justice_adams added a comment.

@stella.stamenova thanks for the review. I don't have commit access, would you 
mind committing this for me?


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

https://reviews.llvm.org/D50563



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

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: bkramer, thakis.
Herald added subscribers: cfe-commits, kadircet, ioeric, ilya-biryukov.

r347205 fixed a bug in FileManager: first calling
getFile(shouldOpen=false) and then getFile(shouldOpen=true) results in
the file not being open.

Unfortunately, some code was (inadvertently?) relying on this bug: when
building with a PCH, the file entries are obtained first by passing
shouldOpen=false, and then later shouldOpen=true, without any intention
of reading them. After r347205, they do get unneccesarily opened.
Aside from extra operations, this means they need to be closed. Normally
files are closed when their contents are read. As these files are never
read, they stay open until clang exits. On platforms with a low
open-files limit (e.g. Mac), this can lead to spurious file-not-found
errors when building large projects with PCH enabled, e.g.

  https://bugs.chromium.org/p/chromium/issues/detail?id=924225

Fixing the callsites to pass shouldOpen=false when the file won't be
read is not quite trivial (that info isn't available at the direct
callsite), and passing shouldOpen=false is a performance regression (it
results in open+fstat pairs being replaced by stat+open).

So an ideal fix is going to be a little risky and we need some fix soon
(especially for the llvm 8 branch).
The problem addressed by r347205 is rare and has only been observed in
clangd. It was present in llvm-7, so we can live with it for now.


Repository:
  rC Clang

https://reviews.llvm.org/D57165

Files:
  include/clang/Basic/FileManager.h
  lib/Basic/FileManager.cpp
  test/PCH/leakfiles
  unittests/Basic/FileManagerTest.cpp

Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -221,33 +221,6 @@
   EXPECT_EQ(nullptr, file);
 }
 
-// When calling getFile(OpenFile=false); getFile(OpenFile=true) the file is
-// opened for the second call.
-TEST_F(FileManagerTest, getFileDefersOpen) {
-  llvm::IntrusiveRefCntPtr FS(
-  new llvm::vfs::InMemoryFileSystem());
-  FS->addFile("/tmp/test", 0, llvm::MemoryBuffer::getMemBufferCopy("test"));
-  FS->addFile("/tmp/testv", 0, llvm::MemoryBuffer::getMemBufferCopy("testv"));
-  FileManager manager(options, FS);
-
-  const FileEntry *file = manager.getFile("/tmp/test", /*OpenFile=*/false);
-  ASSERT_TRUE(file != nullptr);
-  ASSERT_TRUE(file->isValid());
-  // "real path name" reveals whether the file was actually opened.
-  EXPECT_FALSE(file->isOpenForTests());
-
-  file = manager.getFile("/tmp/test", /*OpenFile=*/true);
-  ASSERT_TRUE(file != nullptr);
-  ASSERT_TRUE(file->isValid());
-  EXPECT_TRUE(file->isOpenForTests());
-
-  // However we should never try to open a file previously opened as virtual.
-  ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
-  ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
-  file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_FALSE(file->isOpenForTests());
-}
-
 // The following tests apply to Unix-like system only.
 
 #ifndef _WIN32
Index: test/PCH/leakfiles
===
--- /dev/null
+++ test/PCH/leakfiles
@@ -0,0 +1,29 @@
+// Test that compiling using a PCH doesn't leak file descriptors.
+// https://bugs.chromium.org/p/chromium/issues/detail?id=924225
+
+// This test requires bash loops and ulimit.
+// REQUIRES: shell
+// UNSUPPORTED: win32
+//
+// Set up source files. lib/lib.h includes lots of lib*.h files in that dir.
+// client.c includes lib/lib.h, and also the individual files directly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: mkdir lib
+// RUN: for i in {1..300}; do touch lib/lib$i.h; done
+// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done
+// RUN: echo "#include \"lib/lib.h\"" > client.c
+// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done
+//
+// We want to verify that we don't hold all the files open at the same time.
+// This is important e.g. on mac, which has a low default FD limit.
+// RUN: ulimit -n 100
+//
+// Test without PCH.
+// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c
+//
+// Test with PCH.
+// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c
+// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -188,21 +188,15 @@
   *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
 
   // See if there is already an entry in the map.
-  if (NamedFileEnt.second) {
-if (NamedFileEnt.second == NON_EXISTENT_FILE)
-  return nullptr;
-// Entry exists: return it *unless* it wasn't opened and open is requested.
-if (!(NamedFileEnt.second->DeferredOpen && openFile))
-  return NamedFileEn

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

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

mgorny added a comment.

Just to be clear, are you suggesting that I add `-z gnustack` as well?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

No, I'm suggesting you add execstack, noexecstack and nognustack as a tri-state 
-z flag. Does this sound good?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

[PATCH] D54654: Correct CreateAlignmentAssumption to check sign and power-of-2 (Clang Part)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri abandoned this revision.
lebedev.ri added a comment.

As disscussed in D54653  
(https://reviews.llvm.org/D54653#1318964),
this differential will be simply unneeded, that review will address the issue 
too, so abandoning.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54654



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

[PATCH] D56554: [ELF] Add '-z nognustack' opt to suppress emitting PT_GNU_STACK

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

mgorny added a comment.

In D56554#1369592 , @ruiu wrote:

> No, I'm suggesting you add execstack, noexecstack and nognustack as a 
> tri-state -z flag. Does this sound good?


Do you mean that of those three options, the last one specified should take 
precedence?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56554



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

r352069 - Revert "[Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls"

[Julian Lettner via cfe-commits]

Author: yln
Date: Thu Jan 24 10:04:21 2019
New Revision: 352069

URL: http://llvm.org/viewvc/llvm-project?rev=352069&view=rev
Log:
Revert "[Sanitizers] UBSan unreachable incompatible with ASan in the presence 
of `noreturn` calls"

This reverts commit cea84ab93aeb079a358ab1c8aeba6d9140ef8b47.

Modified:
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/test/CodeGenCXX/ubsan-unreachable.cpp

Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=352069&r1=352068&r2=352069&view=diff
==
--- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCall.cpp Thu Jan 24 10:04:21 2019
@@ -4401,16 +4401,12 @@ RValue CodeGenFunction::EmitCall(const C
 if (UnusedReturnSizePtr)
   PopCleanupBlock();
 
-// Replace the noreturn attribute to better diagnose unreachable UB.
+// Strip away the noreturn attribute to better diagnose unreachable UB.
 if (SanOpts.has(SanitizerKind::Unreachable)) {
-  // Also remove from function since CS.hasFnAttr(..) also checks 
attributes
-  // of the called function.
   if (auto *F = CS.getCalledFunction())
 F->removeFnAttr(llvm::Attribute::NoReturn);
   CS.removeAttribute(llvm::AttributeList::FunctionIndex,
  llvm::Attribute::NoReturn);
-  CS.addAttribute(llvm::AttributeList::FunctionIndex,
-  llvm::Attribute::ExpectNoReturn);
 }
 
 EmitUnreachable(Loc);

Modified: cfe/trunk/test/CodeGenCXX/ubsan-unreachable.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/ubsan-unreachable.cpp?rev=352069&r1=352068&r2=352069&view=diff
==
--- cfe/trunk/test/CodeGenCXX/ubsan-unreachable.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/ubsan-unreachable.cpp Thu Jan 24 10:04:21 2019
@@ -2,35 +2,38 @@
 
 extern void __attribute__((noreturn)) abort();
 
-// CHECK-LABEL: define void @_Z14calls_noreturnv()
+// CHECK-LABEL: define void @_Z14calls_noreturnv
 void calls_noreturn() {
-  // CHECK: call void @_Z5abortv() [[CALL_SITE_ATTR:#[0-9]+]]
   abort();
 
+  // Check that there are no attributes on the call site.
+  // CHECK-NOT: call void @_Z5abortv{{.*}}#
+
   // CHECK: __ubsan_handle_builtin_unreachable
   // CHECK: unreachable
 }
 
 struct A {
-  // CHECK: declare void @_Z5abortv() [[EXTERN_FN_ATTR:#[0-9]+]]
+  // CHECK: declare void @_Z5abortv{{.*}} [[ABORT_ATTR:#[0-9]+]]
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call1Ev
   void call1() {
-// CHECK: call void @_ZN1A16does_not_return2Ev({{.*}}) [[CALL_SITE_ATTR]]
+// CHECK-NOT: call void @_ZN1A16does_not_return2Ev{{.*}}#
 does_not_return2();
 
 // CHECK: __ubsan_handle_builtin_unreachable
 // CHECK: unreachable
   }
 
-  // Test static members. Checks are below after `struct A` scope ends.
+  // Test static members.
   static void __attribute__((noreturn)) does_not_return1() {
+// CHECK-NOT: call void @_Z5abortv{{.*}}#
 abort();
   }
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call2Ev
   void call2() {
-// CHECK: call void @_ZN1A16does_not_return1Ev() [[CALL_SITE_ATTR]]
+// CHECK-NOT: call void @_ZN1A16does_not_return1Ev{{.*}}#
 does_not_return1();
 
 // CHECK: __ubsan_handle_builtin_unreachable
@@ -43,18 +46,18 @@ struct A {
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call3Ev
   void call3() {
 MemFn MF = &A::does_not_return2;
-// CHECK: call void %{{[0-9]+\(.*}}) [[CALL_SITE_ATTR]]
 (this->*MF)();
 
+// CHECK-NOT: call void %{{.*}}#
 // CHECK: __ubsan_handle_builtin_unreachable
 // CHECK: unreachable
   }
 
   // Test regular members.
   // CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return2Ev({{.*}})
-  // CHECK-SAME: [[USER_FN_ATTR:#[0-9]+]]
+  // CHECK-SAME: [[DOES_NOT_RETURN_ATTR:#[0-9]+]]
   void __attribute__((noreturn)) does_not_return2() {
-// CHECK: call void @_Z5abortv() [[CALL_SITE_ATTR]]
+// CHECK-NOT: call void @_Z5abortv(){{.*}}#
 abort();
 
 // CHECK: call void @__ubsan_handle_builtin_unreachable
@@ -65,9 +68,7 @@ struct A {
   }
 };
 
-// CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return1Ev()
-// CHECK-SAME: [[USER_FN_ATTR]]
-// CHECK: call void @_Z5abortv() [[CALL_SITE_ATTR]]
+// CHECK: define linkonce_odr void @_ZN1A16does_not_return1Ev() 
[[DOES_NOT_RETURN_ATTR]]
 
 void force_irgen() {
   A a;
@@ -76,9 +77,5 @@ void force_irgen() {
   a.call3();
 }
 
-// 1) 'noreturn' should be removed from functions and call sites
-// 2) 'expect_noreturn' added to call sites
-// CHECK-LABEL: attributes
-// CHECK: [[USER_FN_ATTR]] = { {{.*[^noreturn].*}} }
-// CHECK: [[EXTERN_FN_ATTR]] = { {{.*[^noreturn].*}} }
-// CHECK: [[CALL_SITE_ATTR]] = { expect_noreturn }
+// CHECK-NOT: [[ABORT_ATTR]] = {{[^}]+}}noreturn
+// CHECK-NOT: [[DOES_NOT_RETURN_ATTR]] = {{[^}]+}}noreturn


___

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

[Nico Weber via Phabricator via cfe-commits]

thakis added inline comments.



Comment at: lib/Basic/FileManager.cpp:295
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

Should this call fillRealPathName() (which was added after your change)?


Repository:
  rC Clang

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

https://reviews.llvm.org/D57165



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln reopened this revision.
yln added a comment.

In D56624#1368966 , @lebedev.ri wrote:

> Please revert this.
>  First, this wasn't reviewed.
>  Second, the lists weren't subscribed.


I apologize for this. It was not my intention to land the revision without 
formal acceptance.

commit-lists: 
I prepared this patch via the monorepo and did not select a repository in 
Phabricator because the changes span multiple repos. This means that I have to 
manually ensure that the correct lists are subscribed in the Phabricator web 
interface, correct?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

[Nico Weber via Phabricator via cfe-commits]

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

lgtm, but lebtm with call to  fillRealPathName()


Repository:
  rC Clang

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

https://reviews.llvm.org/D57165



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D56624#1369626 , @yln wrote:

> In D56624#1368966 , @lebedev.ri 
> wrote:
>
> > Please revert this.
> >  First, this wasn't reviewed.
> >  Second, the lists weren't subscribed.
>
>
> I apologize for this. It was not my intention to land the revision without 
> formal acceptance.
>
> commit-lists: 
>  I prepared this patch via the monorepo and did not select a repository in 
> Phabricator because the changes span multiple repos.


I guess rL  would be a safe default to 
make.

> This means that I have to manually ensure that the correct lists are 
> subscribed in the Phabricator web interface, correct?

Looks like the rules to subscribe the lists based on a content of the patch are 
still not added, so i'd say yes.

That being said, the normal practice in such situations is to open a new review.

Also, i suspect this should be split up into **at least** two parts - the new 
LLVM IR `expect_noreturn` attribute, and the rest.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

What are the advantages of a generalized expect_noreturn attribute, vs. a 
narrower attribute or intrinsic? The expect_noreturn semantics do not provide 
strong guarantees, and are not really orthogonal from the pre-existing cold 
attribute. In particular, expect_noreturn doesn't even seem strong enough to 
allow ASan to unpoison its stack.

Apologies if discussion has shifted elsewhere -- I'd be happy to chime in on 
the new review.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D57076: [ObjC generics] Fix applying `__kindof` to the type parameter.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 2 inline comments as done.
vsapsai added inline comments.



Comment at: clang/lib/AST/Type.cpp:1293
+
+const auto *newAttrType = dyn_cast(newType.getTypePtr());
+if (newAttrType->getAttrKind() != attr::ObjCKindOf)

doug.gregor wrote:
> Either this should be a `cast` or the next if should check 
> for !newAttrType. I suspect the latter is safer.
You are right. Thanks for the catch. Currently it shouldn't happen, as far as I 
can tell, so no test for the case when AttributedType is transformed to 
something else.


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

https://reviews.llvm.org/D57076



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

[PATCH] D56326: [OpenMP 5.0] Parsing/sema support for "omp declare mapper" directive

[Lingda Li via Phabricator via cfe-commits]

lildmh added inline comments.



Comment at: lib/AST/DeclOpenMP.cpp:164
+  if (NumClauses) {
+Clauses = (OMPClause **)C.Allocate(sizeof(OMPClause *) * NumClauses);
+setClauses(CL);

ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > lildmh wrote:
> > > > ABataev wrote:
> > > > > lildmh wrote:
> > > > > > ABataev wrote:
> > > > > > > No, bad idea. Use tail allocation for the clauses. Check the 
> > > > > > > implementation of `OMPRequiresDecl`
> > > > > > I think it is possible to use TrailingObjects for clause storage 
> > > > > > when the number of clauses are known before creating the directive 
> > > > > > (e.g., for OMPRequiresDecl and OMPExecutableDirective). 
> > > > > > 
> > > > > > The reason that I had to create OMPDeclareMapperDecl before parsing 
> > > > > > map clauses, is the mapper variable (AA in the example below) needs 
> > > > > > to be declared within OMPDeclareMapperDecl, because the following 
> > > > > > map clauses will use it.
> > > > > > 
> > > > > > ```
> > > > > > #pragma omp declare mapper(struct S AA) map(AA.field1)
> > > > > > ```
> > > > > > 
> > > > > > A possible way to get around this is to count the number of map 
> > > > > > clauses before hand. But this solution is not trivial since the 
> > > > > > normal method for parsing map clauses cannot be used (e.g., it does 
> > > > > > not know AA when parsing map(AA.field1)). A customized and complex 
> > > > > > (because it needs to handle all possible situations) parsing method 
> > > > > > needs to be created, just for counting clause number. I think it's 
> > > > > > not worthy to do this compared with allocating map clause space 
> > > > > > later.
> > > > > > 
> > > > > > I checked the code for OMPDeclareReductionDecl that you wrote. It 
> > > > > > also has to be created before parsing the combiner and initializer. 
> > > > > > It does not have a variable number of clauses though.
> > > > > > 
> > > > > > Any suggestions?
> > > > > Instead, you can introduce special DeclContext-based declaration and 
> > > > > keep the reference to this declaration inside of the 
> > > > > `OMPDeclareMapperDecl`.
> > > > Hi Alexey,
> > > > 
> > > > Thanks a lot for your quick response! I don't think I understand your 
> > > > idea. Can you establish more on that?
> > > > 
> > > > In my current implementation, OMPDeclareMapperDecl is used as the 
> > > > DeclConext of the variable AA in the above example, and it already 
> > > > includes the reference to AA's declaration.
> > > > 
> > > > My problem is, I need to create OMPDeclareMapperDecl before parsing map 
> > > > clauses. But before parsing map clauses, I don't know the number of 
> > > > clauses. Using TrailingObject requires to know how many clauses there 
> > > > are when creating OMPDeclareMapperDecl. So I couldn't use 
> > > > TrailingObject.
> > > > 
> > > > My current solution is to create OMPDeclareMapperDecl before parsing 
> > > > map clauses, and to create the clause storage after parsing finishes.
> > > What I meant, that you don't need to use `OMPDeclareMapperDecl` for this, 
> > > instead you can add another (very simple) special declaration based on 
> > > `DeclContext` to use it as the parent declaration for the variable. In 
> > > the `OMPDeclareMapperDecl` you can keep the reference to this special 
> > > declaration.
> > Thanks for your response! Please let me know if my understanding below is 
> > correct:
> > 
> > `OMPDeclareMapperDecl` no longer inherits from `DeclContext`. Instead, we 
> > create something like `OMPDeclareMapperDeclContext` which inherits from 
> > `DeclContext`, and `OMPDeclareMapperDecl` keeps a pointer that points to 
> > this `OMPDeclareMapperDeclContext`.  AA and map clauses are parsed within 
> > `OMPDeclareMapperDeclContext`.
> > 
> > This sounds a bit more complex, but if you believe it's better, I can 
> > change the code. Please share your thoughts.
> Yes, something like this.
Hi Alexey,

Sorry for the late response. I was working on something else last week.

When I tried to modify the code based on your suggestions, I found out that 
`DeclContext` is only meant to be used for a `Decl` (please see the comments 
before `class DeclContext {...}` in include/clang/AST/DeclBase.h).

It means, if I create a `OMPDeclareMapperDeclContext ` which is a `DeclContext 
` but not a `Decl`, the code cannot work correctly. Therefore 
`OMPDeclareMapperDeclContext` must be a `Decl` itself. If I do it this way, a 
lot of useless information (all inherited from `Decl`) will exist within 
`OMPDeclareMapperDeclContext`, which is very inefficient.

An alternative way is to have something called `OMPDeclareMapperClauses` that 
inherits from `TrailingObject` to store clause information, and 
`OMPDeclareMapperDecl` keeps a pointer that points to 
`OMPDeclareMapperClauses`. But I don't think this is better than just having a 
`OMPClause **Clauses`, which is my current implementation.

What do you think?


CHANGES 

[PATCH] D57076: [ObjC generics] Fix applying `__kindof` to the type parameter.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 183338.
vsapsai added a comment.

- Address review comment: don't crash when AttributedType is transformed to 
another type class.


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

https://reviews.llvm.org/D57076

Files:
  clang/lib/AST/Type.cpp
  clang/test/SemaObjC/kindof.m

Index: clang/test/SemaObjC/kindof.m
===
--- clang/test/SemaObjC/kindof.m
+++ clang/test/SemaObjC/kindof.m
@@ -384,9 +384,17 @@
 }
 @end
 
+// ---
+// __kindof on type parameters
+// ---
+
 @interface NSGeneric : NSObject
 - (void)test:(__kindof ObjectType)T; // expected-note{{passing argument to parameter 'T' here}}
 - (void)mapUsingBlock:(id (^)(__kindof ObjectType))block;
+@property (copy) ObjectType object;
+@property (copy) __kindof ObjectType kindof_object;
+
+@property (copy) __kindof ObjectType _Nonnull nonnull_kindof_object;
 @end
 @implementation NSGeneric
 - (void)test:(id)T {
@@ -395,6 +403,11 @@
 }
 @end
 
+@interface NSDefaultGeneric : NSObject
+@property (copy) ObjectType object;
+@property (copy) __kindof ObjectType kindof_object;
+@end
+
 void testGeneric(NSGeneric *generic) {
   NSObject *NSObject_obj;
   // Assign from NSObject_obj to __kindof NSString*.
@@ -403,6 +416,45 @@
   [generic test:NSString_str];
 }
 
+void testGenericAssignment() {
+  NSMutableString *NSMutableString_str;
+  NSNumber *NSNumber_obj;
+
+  NSGeneric *generic;
+  NSMutableString_str = generic.object; // expected-warning{{incompatible pointer types}}
+  NSNumber_obj = generic.object; // expected-warning{{incompatible pointer types}}
+  NSMutableString_str = generic.kindof_object;
+  NSNumber_obj = generic.kindof_object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof NSString *'}}
+
+  NSGeneric<__kindof NSString*> *kindof_generic;
+  NSMutableString_str = kindof_generic.object;
+  NSNumber_obj = kindof_generic.object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof NSString *'}}
+  NSMutableString_str = kindof_generic.kindof_object;
+  NSNumber_obj = kindof_generic.kindof_object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof __kindof NSString *'}}
+
+  NSDefaultGeneric *default_generic;
+  NSMutableString_str = default_generic.object;
+  NSNumber_obj = default_generic.object; // expected-warning{{incompatible pointer types}}
+  NSMutableString_str = default_generic.kindof_object;
+  NSNumber_obj = default_generic.kindof_object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof __kindof NSString *'}}
+
+  typedef NSString *Typedef_NSString;
+  NSGeneric *typedef_generic;
+  NSMutableString_str = typedef_generic.object; // expected-warning{{incompatible pointer types}}
+  NSNumber_obj = typedef_generic.object; // expected-warning{{incompatible pointer types}}
+  NSMutableString_str = typedef_generic.kindof_object;
+  NSNumber_obj = typedef_generic.kindof_object; // expected-warning{{incompatible pointer types assigning to 'NSNumber *' from '__kindof Typedef_NSString'}}
+}
+
+void testKindofNonObjectType() {
+  typedef void (^BlockType)(int);
+  NSGeneric *generic;
+}
+
+void testKindofNullability(NSGeneric *generic) {
+  generic.nonnull_kindof_object = 0; // expected-warning{{null passed to a callee that requires a non-null argument}}
+}
+
 // Check that clang doesn't crash when a type parameter is illegal.
 @interface Array1 : NSObject
 @end
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -1284,6 +1284,42 @@
 
 return BaseType::VisitObjCObjectType(objcObjectType);
   }
+
+  QualType VisitAttributedType(const AttributedType *attrType) {
+QualType newType = BaseType::VisitAttributedType(attrType);
+if (newType.isNull())
+  return {};
+
+const auto *newAttrType = dyn_cast(newType.getTypePtr());
+if (!newAttrType || newAttrType->getAttrKind() != attr::ObjCKindOf)
+  return newType;
+
+// Find out if it's an Objective-C object or object pointer type;
+QualType newEquivType = newAttrType->getEquivalentType();
+const ObjCObjectPointerType *ptrType =
+newEquivType->getAs();
+const ObjCObjectType *objType = ptrType
+? ptrType->getObjectType()
+: newEquivType->getAs();
+if (!objType)
+  return newType;
+
+// Rebuild the "equivalent" type, which pushes __kindof down into
+// the object type.
+newEquivType = Ctx.getObjCObjectType(
+objType->getBaseType(), objType->getTypeArgsAsWritten(),
+objType->getProtocols(),
+// There is no need to apply kindof on an un

r352079 - [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Thu Jan 24 10:55:24 2019
New Revision: 352079

URL: http://llvm.org/viewvc/llvm-project?rev=352079&view=rev
Log:
[FileManager] Revert r347205 to avoid PCH file-descriptor leak.

Summary:
r347205 fixed a bug in FileManager: first calling
getFile(shouldOpen=false) and then getFile(shouldOpen=true) results in
the file not being open.

Unfortunately, some code was (inadvertently?) relying on this bug: when
building with a PCH, the file entries are obtained first by passing
shouldOpen=false, and then later shouldOpen=true, without any intention
of reading them. After r347205, they do get unneccesarily opened.
Aside from extra operations, this means they need to be closed. Normally
files are closed when their contents are read. As these files are never
read, they stay open until clang exits. On platforms with a low
open-files limit (e.g. Mac), this can lead to spurious file-not-found
errors when building large projects with PCH enabled, e.g.
  https://bugs.chromium.org/p/chromium/issues/detail?id=924225

Fixing the callsites to pass shouldOpen=false when the file won't be
read is not quite trivial (that info isn't available at the direct
callsite), and passing shouldOpen=false is a performance regression (it
results in open+fstat pairs being replaced by stat+open).

So an ideal fix is going to be a little risky and we need some fix soon
(especially for the llvm 8 branch).
The problem addressed by r347205 is rare and has only been observed in
clangd. It was present in llvm-7, so we can live with it for now.

Reviewers: bkramer, thakis

Subscribers: ilya-biryukov, ioeric, kadircet, cfe-commits

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

Added:
cfe/trunk/test/PCH/leakfiles
Modified:
cfe/trunk/include/clang/Basic/FileManager.h
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/unittests/Basic/FileManagerTest.cpp

Modified: cfe/trunk/include/clang/Basic/FileManager.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=352079&r1=352078&r2=352079&view=diff
==
--- cfe/trunk/include/clang/Basic/FileManager.h (original)
+++ cfe/trunk/include/clang/Basic/FileManager.h Thu Jan 24 10:55:24 2019
@@ -69,15 +69,14 @@ class FileEntry {
   bool IsNamedPipe;
   bool InPCH;
   bool IsValid;   // Is this \c FileEntry initialized and valid?
-  bool DeferredOpen;  // Created by getFile(OpenFile=0); may open 
later.
 
   /// The open file, if it is owned by the \p FileEntry.
   mutable std::unique_ptr File;
 
 public:
   FileEntry()
-  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false),
-DeferredOpen(false) {}
+  : UniqueID(0, 0), IsNamedPipe(false), InPCH(false), IsValid(false)
+  {}
 
   FileEntry(const FileEntry &) = delete;
   FileEntry &operator=(const FileEntry &) = delete;

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=352079&r1=352078&r2=352079&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Thu Jan 24 10:55:24 2019
@@ -188,21 +188,15 @@ const FileEntry *FileManager::getFile(St
   *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
 
   // See if there is already an entry in the map.
-  if (NamedFileEnt.second) {
-if (NamedFileEnt.second == NON_EXISTENT_FILE)
-  return nullptr;
-// Entry exists: return it *unless* it wasn't opened and open is requested.
-if (!(NamedFileEnt.second->DeferredOpen && openFile))
-  return NamedFileEnt.second;
-// We previously stat()ed the file, but didn't open it: do that below.
-// FIXME: the below does other redundant work too (stats the dir and file).
-  } else {
-// By default, initialize it to invalid.
-NamedFileEnt.second = NON_EXISTENT_FILE;
-  }
+  if (NamedFileEnt.second)
+return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
+: NamedFileEnt.second;
 
   ++NumFileCacheMisses;
 
+  // By default, initialize it to invalid.
+  NamedFileEnt.second = NON_EXISTENT_FILE;
+
   // Get the null-terminated file name as stored as the key of the
   // SeenFileEntries map.
   StringRef InterndFileName = NamedFileEnt.first();
@@ -240,7 +234,6 @@ const FileEntry *FileManager::getFile(St
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Data.UniqueID];
-  UFE.DeferredOpen = !openFile;
 
   NamedFileEnt.second = &UFE;
 
@@ -257,15 +250,6 @@ const FileEntry *FileManager::getFile(St
 InterndFileName = NamedFileEnt.first().data();
   }
 
-  // If we opened the file for the first time, record the resulting info.
-  // Do this even if the cache ent

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 183340.
sammccall marked an inline comment as done.
sammccall added a comment.

avoid revert of fillRealPathName() (thanks Nico!)


Repository:
  rC Clang

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

https://reviews.llvm.org/D57165

Files:
  include/clang/Basic/FileManager.h
  lib/Basic/FileManager.cpp
  test/PCH/leakfiles
  unittests/Basic/FileManagerTest.cpp

Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -221,33 +221,6 @@
   EXPECT_EQ(nullptr, file);
 }
 
-// When calling getFile(OpenFile=false); getFile(OpenFile=true) the file is
-// opened for the second call.
-TEST_F(FileManagerTest, getFileDefersOpen) {
-  llvm::IntrusiveRefCntPtr FS(
-  new llvm::vfs::InMemoryFileSystem());
-  FS->addFile("/tmp/test", 0, llvm::MemoryBuffer::getMemBufferCopy("test"));
-  FS->addFile("/tmp/testv", 0, llvm::MemoryBuffer::getMemBufferCopy("testv"));
-  FileManager manager(options, FS);
-
-  const FileEntry *file = manager.getFile("/tmp/test", /*OpenFile=*/false);
-  ASSERT_TRUE(file != nullptr);
-  ASSERT_TRUE(file->isValid());
-  // "real path name" reveals whether the file was actually opened.
-  EXPECT_FALSE(file->isOpenForTests());
-
-  file = manager.getFile("/tmp/test", /*OpenFile=*/true);
-  ASSERT_TRUE(file != nullptr);
-  ASSERT_TRUE(file->isValid());
-  EXPECT_TRUE(file->isOpenForTests());
-
-  // However we should never try to open a file previously opened as virtual.
-  ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
-  ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
-  file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_FALSE(file->isOpenForTests());
-}
-
 // The following tests apply to Unix-like system only.
 
 #ifndef _WIN32
Index: test/PCH/leakfiles
===
--- /dev/null
+++ test/PCH/leakfiles
@@ -0,0 +1,29 @@
+// Test that compiling using a PCH doesn't leak file descriptors.
+// https://bugs.chromium.org/p/chromium/issues/detail?id=924225
+//
+// This test requires bash loops and ulimit.
+// REQUIRES: shell
+// UNSUPPORTED: win32
+//
+// Set up source files. lib/lib.h includes lots of lib*.h files in that dir.
+// client.c includes lib/lib.h, and also the individual files directly.
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: mkdir lib
+// RUN: for i in {1..300}; do touch lib/lib$i.h; done
+// RUN: for i in {1..300}; do echo "#include \"lib$i.h\"" >> lib/lib.h; done
+// RUN: echo "#include \"lib/lib.h\"" > client.c
+// RUN: for i in {1..300}; do echo "#include \"lib/lib$i.h\"" >> client.c; done
+//
+// We want to verify that we don't hold all the files open at the same time.
+// This is important e.g. on mac, which has a low default FD limit.
+// RUN: ulimit -n 100
+//
+// Test without PCH.
+// RUN: %clang_cc1 -fsyntax-only -Ilib/ client.c
+//
+// Test with PCH.
+// RUN: %clang_cc1 -emit-pch -o pch -Ilib/ client.c
+// RUN: %clang_cc1 -include-pch pch -Ilib/ client.c -fsyntax-only
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -188,21 +188,15 @@
   *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
 
   // See if there is already an entry in the map.
-  if (NamedFileEnt.second) {
-if (NamedFileEnt.second == NON_EXISTENT_FILE)
-  return nullptr;
-// Entry exists: return it *unless* it wasn't opened and open is requested.
-if (!(NamedFileEnt.second->DeferredOpen && openFile))
-  return NamedFileEnt.second;
-// We previously stat()ed the file, but didn't open it: do that below.
-// FIXME: the below does other redundant work too (stats the dir and file).
-  } else {
-// By default, initialize it to invalid.
-NamedFileEnt.second = NON_EXISTENT_FILE;
-  }
+  if (NamedFileEnt.second)
+return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
+: NamedFileEnt.second;
 
   ++NumFileCacheMisses;
 
+  // By default, initialize it to invalid.
+  NamedFileEnt.second = NON_EXISTENT_FILE;
+
   // Get the null-terminated file name as stored as the key of the
   // SeenFileEntries map.
   StringRef InterndFileName = NamedFileEnt.first();
@@ -240,7 +234,6 @@
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Data.UniqueID];
-  UFE.DeferredOpen = !openFile;
 
   NamedFileEnt.second = &UFE;
 
@@ -257,15 +250,6 @@
 InterndFileName = NamedFileEnt.first().data();
   }
 
-  // If we opened the file for the first time, record the resulting info.
-  // Do this even if the cache entry was valid, maybe we didn't previously open.
-  if (F && !UFE.Fil

[PATCH] D57165: [FileManager] Revert r347205 to avoid PCH file-descriptor leak.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352079: [FileManager] Revert r347205 to avoid PCH 
file-descriptor leak. (authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57165?vs=183340&id=183347#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57165

Files:
  include/clang/Basic/FileManager.h
  lib/Basic/FileManager.cpp
  test/PCH/leakfiles
  unittests/Basic/FileManagerTest.cpp

Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -188,21 +188,15 @@
   *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
 
   // See if there is already an entry in the map.
-  if (NamedFileEnt.second) {
-if (NamedFileEnt.second == NON_EXISTENT_FILE)
-  return nullptr;
-// Entry exists: return it *unless* it wasn't opened and open is requested.
-if (!(NamedFileEnt.second->DeferredOpen && openFile))
-  return NamedFileEnt.second;
-// We previously stat()ed the file, but didn't open it: do that below.
-// FIXME: the below does other redundant work too (stats the dir and file).
-  } else {
-// By default, initialize it to invalid.
-NamedFileEnt.second = NON_EXISTENT_FILE;
-  }
+  if (NamedFileEnt.second)
+return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
+: NamedFileEnt.second;
 
   ++NumFileCacheMisses;
 
+  // By default, initialize it to invalid.
+  NamedFileEnt.second = NON_EXISTENT_FILE;
+
   // Get the null-terminated file name as stored as the key of the
   // SeenFileEntries map.
   StringRef InterndFileName = NamedFileEnt.first();
@@ -240,7 +234,6 @@
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Data.UniqueID];
-  UFE.DeferredOpen = !openFile;
 
   NamedFileEnt.second = &UFE;
 
@@ -257,15 +250,6 @@
 InterndFileName = NamedFileEnt.first().data();
   }
 
-  // If we opened the file for the first time, record the resulting info.
-  // Do this even if the cache entry was valid, maybe we didn't previously open.
-  if (F && !UFE.File) {
-if (auto PathName = F->getName())
-  fillRealPathName(&UFE, *PathName);
-UFE.File = std::move(F);
-assert(!UFE.DeferredOpen && "we just opened it!");
-  }
-
   if (UFE.isValid()) { // Already have an entry with this inode, return it.
 
 // FIXME: this hack ensures that if we look up a file by a virtual path in
@@ -296,9 +280,13 @@
   UFE.UniqueID = Data.UniqueID;
   UFE.IsNamedPipe = Data.IsNamedPipe;
   UFE.InPCH = Data.InPCH;
+  UFE.File = std::move(F);
   UFE.IsValid = true;
-  // Note File and DeferredOpen were initialized above.
 
+  if (UFE.File) {
+if (auto PathName = UFE.File->getName())
+  fillRealPathName(&UFE, *PathName);
+  }
   return &UFE;
 }
 
@@ -370,7 +358,6 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
-  UFE->DeferredOpen = false;
   return UFE;
 }
 
Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -221,33 +221,6 @@
   EXPECT_EQ(nullptr, file);
 }
 
-// When calling getFile(OpenFile=false); getFile(OpenFile=true) the file is
-// opened for the second call.
-TEST_F(FileManagerTest, getFileDefersOpen) {
-  llvm::IntrusiveRefCntPtr FS(
-  new llvm::vfs::InMemoryFileSystem());
-  FS->addFile("/tmp/test", 0, llvm::MemoryBuffer::getMemBufferCopy("test"));
-  FS->addFile("/tmp/testv", 0, llvm::MemoryBuffer::getMemBufferCopy("testv"));
-  FileManager manager(options, FS);
-
-  const FileEntry *file = manager.getFile("/tmp/test", /*OpenFile=*/false);
-  ASSERT_TRUE(file != nullptr);
-  ASSERT_TRUE(file->isValid());
-  // "real path name" reveals whether the file was actually opened.
-  EXPECT_FALSE(file->isOpenForTests());
-
-  file = manager.getFile("/tmp/test", /*OpenFile=*/true);
-  ASSERT_TRUE(file != nullptr);
-  ASSERT_TRUE(file->isValid());
-  EXPECT_TRUE(file->isOpenForTests());
-
-  // However we should never try to open a file previously opened as virtual.
-  ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
-  ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
-  file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_FALSE(file->isOpenForTests());
-}
-
 // The following tests apply to Unix-like system only.
 
 #ifndef _WIN32
Index: include/clang/Basic/FileManager.h
===
--- include/clang/Basic/FileManager.h
+++ include/clang/Basic/FileManager.h
@@ -69,15 +69,14 @@
   bool IsNamedPipe;
   bool InPCH;
   bool IsValid;   // Is this \c FileE

[PATCH] D53199: Fix the behavior of clang's -w flag.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Ping.


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

https://reviews.llvm.org/D53199



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

[PATCH] D57175: [NFC][clang] Test updates for CreateAlignmentAssumption() changes in D54653

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.

Depends on D54653 .


Repository:
  rC Clang

https://reviews.llvm.org/D57175

Files:
  test/CodeGen/alloc-align-attr.c
  
test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp


Index: 
test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
===
--- 
test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
+++ 
test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
@@ -30,9 +30,7 @@
   // CHECK-NEXT:%[[X_RELOADED:.*]] = load i8**, i8*** 
%[[X_ADDR]], align 8
   // CHECK-NEXT:%[[ALIGNMENT_RELOADED:.*]] = load i64, 
i64* %[[ALIGNMENT_ADDR]], align 8
   // CHECK-NEXT:%[[X_RETURNED:.*]] = call i8** 
@[[PASSTHROUGH]](i8** %[[X_RELOADED]], i64 %[[ALIGNMENT_RELOADED]])
-  // CHECK-NEXT:%[[ISPOSITIVE:.*]] = icmp sgt i64 
%[[ALIGNMENT_RELOADED]], 0
-  // CHECK-NEXT:%[[POSITIVEMASK:.*]] = sub i64 
%[[ALIGNMENT_RELOADED]], 1
-  // CHECK-NEXT:%[[MASK:.*]] = select i1 
%[[ISPOSITIVE]], i64 %[[POSITIVEMASK]], i64 0
+  // CHECK-NEXT:%[[MASK:.*]] = sub i64 
%[[ALIGNMENT_RELOADED]], 1
   // CHECK-NEXT:%[[PTRINT:.*]] = ptrtoint i8** 
%[[X_RETURNED]] to i64
   // CHECK-NEXT:%[[MASKEDPTR:.*]] = and i64 
%[[PTRINT]], %[[MASK]]
   // CHECK-NEXT:%[[MASKCOND:.*]] = icmp eq i64 
%[[MASKEDPTR]], 0
Index: test/CodeGen/alloc-align-attr.c
===
--- test/CodeGen/alloc-align-attr.c
+++ test/CodeGen/alloc-align-attr.c
@@ -6,11 +6,9 @@
 __INT32_TYPE__ test1(__INT32_TYPE__ a) {
 // CHECK: define i32 @test1
   return *m1(a);
-// CHECK: call i32* @m1(i32 [[PARAM1:%[^\)]+]]) 
-// CHECK: [[ALIGNCAST1:%.+]] = sext i32 [[PARAM1]] to i64
-// CHECK: [[ISPOS1:%.+]] = icmp sgt i64 [[ALIGNCAST1]], 0
-// CHECK: [[POSMASK1:%.+]] = sub i64 [[ALIGNCAST1]], 1
-// CHECK: [[MASK1:%.+]] = select i1 [[ISPOS1]], i64 [[POSMASK1]], i64 0
+// CHECK: call i32* @m1(i32 [[PARAM1:%[^\)]+]])
+// CHECK: [[ALIGNCAST1:%.+]] = zext i32 [[PARAM1]] to i64
+// CHECK: [[MASK1:%.+]] = sub i64 [[ALIGNCAST1]], 1
 // CHECK: [[PTRINT1:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR1:%.+]] = and i64 [[PTRINT1]], [[MASK1]]
 // CHECK: [[MASKCOND1:%.+]] = icmp eq i64 [[MASKEDPTR1]], 0
@@ -22,10 +20,8 @@
   return *m1(a);
 // CHECK: [[CONV2:%.+]] = trunc i64 %{{.+}} to i32
 // CHECK: call i32* @m1(i32 [[CONV2]])
-// CHECK: [[ALIGNCAST2:%.+]] = sext i32 [[CONV2]] to i64
-// CHECK: [[ISPOS2:%.+]] = icmp sgt i64 [[ALIGNCAST2]], 0
-// CHECK: [[POSMASK2:%.+]] = sub i64 [[ALIGNCAST2]], 1
-// CHECK: [[MASK2:%.+]] = select i1 [[ISPOS2]], i64 [[POSMASK2]], i64 0
+// CHECK: [[ALIGNCAST2:%.+]] = zext i32 [[CONV2]] to i64
+// CHECK: [[MASK2:%.+]] = sub i64 [[ALIGNCAST2]], 1
 // CHECK: [[PTRINT2:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR2:%.+]] = and i64 [[PTRINT2]], [[MASK2]]
 // CHECK: [[MASKCOND2:%.+]] = icmp eq i64 [[MASKEDPTR2]], 0
@@ -39,9 +35,7 @@
   return *m2(a);
 // CHECK: [[CONV3:%.+]] = sext i32 %{{.+}} to i64
 // CHECK: call i32* @m2(i64 [[CONV3]])
-// CHECK: [[ISPOS3:%.+]] = icmp sgt i64 [[CONV3]], 0
-// CHECK: [[POSMASK3:%.+]] = sub i64 [[CONV3]], 1
-// CHECK: [[MASK3:%.+]] = select i1 [[ISPOS3]], i64 [[POSMASK3]], i64 0
+// CHECK: [[MASK3:%.+]] = sub i64 [[CONV3]], 1
 // CHECK: [[PTRINT3:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR3:%.+]] = and i64 [[PTRINT3]], [[MASK3]]
 // CHECK: [[MASKCOND3:%.+]] = icmp eq i64 [[MASKEDPTR3]], 0
@@ -52,10 +46,8 @@
 __INT32_TYPE__ test4(__SIZE_TYPE__ a) {
 // CHECK: define i32 @test4
   return *m2(a);
-// CHECK: call i32* @m2(i64 [[PARAM4:%[^\)]+]]) 
-// CHECK: [[ISPOS4:%.+]] = icmp sgt i64 [[PARAM4]], 0
-// CHECK: [[POSMASK4:%.+]] = sub i64 [[PARAM4]], 1
-// CHECK: [[MASK4:%.+]] = select i1 [[ISPOS4]], i64 [[POSMASK4]], i64 0
+// CHECK: call i32* @m2(i64 [[PARAM4:%[^\)]+]])
+// CHECK: [[MASK4:%.+]] = sub i64 [[PARAM4]], 1
 // CHECK: [[PTRINT4:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR4:%.+]] = and i64 [[PTRINT4]], [[MASK4]]
 // CHECK: [[MASKCOND4:%.+]] = icmp eq i64 [[MASKEDPTR4]], 0
@@ -72,11 +64,9 @@
 // CHECK: define i32 @test5
   struct Empty e;
   return *m3(e, a);
-// CHECK: call i32* @m3(i64 %{{.*}}, i64 %{{.*}}) 
+// CHECK: call i32* @m3(i64 %{{.*}}, i64 %{{.*}})
 // CHECK: [[ALIGNCAST5:%.+]] = trunc i128 %{{.*}} to i64
-// CHECK: [[ISPOS5:%.+]] = icmp sgt i64 [[ALIGNCAST5]], 0
-// CHECK: [[POSMASK5:%.+]] = sub i64 [[ALIGNCAST5]], 1
-// CHECK: [[MASK5:%.+]] = select i1 [[ISPOS5]], i64 [[POSMASK5]], i64 0
+// CHECK: [[MASK5:%.+]] = sub i64 [[ALIGNCAST5]], 1
 // CHECK: [[PTRINT5:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR5:%.+]] = and i64 [[PTRINT5]], [[MASK5]]
 // CHECK: [[MASKCOND5:%.+]] = icmp eq i64 [[MASKEDPT

[PATCH] D57150: [HeaderSearch] don't immediately request that headers are opened in getFileAndSuggestModule().

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D57150#1369423 , @sammccall wrote:

> Plan for now is to revert the original change, live with the things that are 
> broken, cherrypick the revert to the llvm8 branch, then revisit.


Sgtm for 8.0. I see the revert landed in r352079, so I'll merge that once it's 
baked in trunk a little bit.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57150



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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 183349.
riccibruno marked 6 inline comments as done.
riccibruno added a comment.

Cleanup the patch by factoring out the NFC changes.

@steveire This should be much clearer now.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h
  lib/AST/Expr.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaPseudoObject.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp

Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -968,18 +968,24 @@
 
 void ASTStmtWriter::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
+
   Record.push_back(E->getNumAssocs());
   Record.push_back(E->ResultIndex);
-
-  Record.AddStmt(E->getControllingExpr());
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-Record.AddTypeSourceInfo(E->getAssocTypeSourceInfo(I));
-Record.AddStmt(E->getAssocExpr(I));
-  }
-
   Record.AddSourceLocation(E->getGenericLoc());
   Record.AddSourceLocation(E->getDefaultLoc());
   Record.AddSourceLocation(E->getRParenLoc());
+
+  Stmt **Stmts = E->getTrailingObjects();
+  // Add 1 to account for the controlling expression which is the first
+  // expression in the trailing array of Stmt *. This is not needed for
+  // the trailing array of TypeSourceInfo *.
+  for (unsigned I = 0, N = E->getNumAssocs() + 1; I < N; ++I)
+Record.AddStmt(Stmts[I]);
+
+  TypeSourceInfo **TSIs = E->getTrailingObjects();
+  for (unsigned I = 0, N = E->getNumAssocs(); I < N; ++I)
+Record.AddTypeSourceInfo(TSIs[I]);
+
   Code = serialization::EXPR_GENERIC_SELECTION;
 }
 
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -1022,23 +1022,24 @@
 
 void ASTStmtReader::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
-  E->NumAssocs = Record.readInt();
-  E->ResultIndex = Record.readInt();
-
-  E->AssocTypes = new (Record.getContext()) TypeSourceInfo *[E->NumAssocs];
-  E->SubExprs = new (Record.getContext())
-  Stmt *[GenericSelectionExpr::AssocExprStartIndex + E->NumAssocs];
-
-  E->SubExprs[GenericSelectionExpr::ControllingIndex] = Record.readSubExpr();
-  for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-E->AssocTypes[I] = GetTypeSourceInfo();
-E->SubExprs[GenericSelectionExpr::AssocExprStartIndex + I] =
-Record.readSubExpr();
-  }
 
-  E->GenericLoc = ReadSourceLocation();
+  unsigned NumAssocs = Record.readInt();
+  assert(NumAssocs == E->getNumAssocs() && "Wrong NumAssocs!");
+  E->ResultIndex = Record.readInt();
+  E->GenericSelectionExprBits.GenericLoc = ReadSourceLocation();
   E->DefaultLoc = ReadSourceLocation();
   E->RParenLoc = ReadSourceLocation();
+
+  Stmt **Stmts = E->getTrailingObjects();
+  // Add 1 to account for the controlling expression which is the first
+  // expression in the trailing array of Stmt *. This is not needed for
+  // the trailing array of TypeSourceInfo *.
+  for (unsigned I = 0, N = NumAssocs + 1; I < N; ++I)
+Stmts[I] = Record.readSubExpr();
+
+  TypeSourceInfo **TSIs = E->getTrailingObjects();
+  for (unsigned I = 0, N = NumAssocs; I < N; ++I)
+TSIs[I] = GetTypeSourceInfo();
 }
 
 void ASTStmtReader::VisitPseudoObjectExpr(PseudoObjectExpr *E) {
@@ -2677,7 +2678,9 @@
   break;
 
 case EXPR_GENERIC_SELECTION:
-  S = new (Context) GenericSelectionExpr(Empty);
+  S = GenericSelectionExpr::CreateEmpty(
+  Context,
+  /*NumAssocs=*/Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_OBJC_STRING_LITERAL:
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -150,15 +150,10 @@
   assocTypes[i] = gse->getAssocTypeSourceInfo(i);
 }
 
-return new (S.Context) GenericSelectionExpr(S.Context,
-gse->getGenericLoc(),
-gse->getControllingExpr(),
-assocTypes,
-assocs,
-gse->getDefaultLoc(),
-gse->getRParenLoc(),
-  gse->containsUnexpandedParameterPack(),
-resultIndex);
+return GenericSelectionExpr::Create(
+S.Context, gse->getGenericLoc(), gse->getControllingExpr(),
+assocTy

[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 183352.
riccibruno added a comment.

Rebased on D57104 . No other changes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57106

Files:
  include/clang/AST/Expr.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/AST/StmtDataCollectors.td
  lib/AST/ASTDumper.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/StmtProfile.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaPseudoObject.cpp
  lib/Sema/TreeTransform.h

Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -9071,10 +9071,10 @@
 
   SmallVector AssocExprs;
   SmallVector AssocTypes;
-  for (unsigned i = 0; i != E->getNumAssocs(); ++i) {
-TypeSourceInfo *TS = E->getAssocTypeSourceInfo(i);
-if (TS) {
-  TypeSourceInfo *AssocType = getDerived().TransformType(TS);
+  for (GenericSelectionExpr::Association Assoc : E->associations()) {
+TypeSourceInfo *TSI = Assoc.getTypeSourceInfo();
+if (TSI) {
+  TypeSourceInfo *AssocType = getDerived().TransformType(TSI);
   if (!AssocType)
 return ExprError();
   AssocTypes.push_back(AssocType);
@@ -9082,7 +9082,8 @@
   AssocTypes.push_back(nullptr);
 }
 
-ExprResult AssocExpr = getDerived().TransformExpr(E->getAssocExpr(i));
+ExprResult AssocExpr =
+getDerived().TransformExpr(Assoc.getAssociationExpr());
 if (AssocExpr.isInvalid())
   return ExprError();
 AssocExprs.push_back(AssocExpr.get());
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -140,19 +140,22 @@
 unsigned resultIndex = gse->getResultIndex();
 unsigned numAssocs = gse->getNumAssocs();
 
-SmallVector assocs(numAssocs);
-SmallVector assocTypes(numAssocs);
-
-for (unsigned i = 0; i != numAssocs; ++i) {
-  Expr *assoc = gse->getAssocExpr(i);
-  if (i == resultIndex) assoc = rebuild(assoc);
-  assocs[i] = assoc;
-  assocTypes[i] = gse->getAssocTypeSourceInfo(i);
+SmallVector assocExprs;
+SmallVector assocTypes;
+assocExprs.reserve(numAssocs);
+assocTypes.reserve(numAssocs);
+
+for (GenericSelectionExpr::Association assoc : gse->associations()) {
+  Expr *assocExpr = assoc.getAssociationExpr();
+  if (assoc.isSelected())
+assocExpr = rebuild(assocExpr);
+  assocExprs.push_back(assocExpr);
+  assocTypes.push_back(assoc.getTypeSourceInfo());
 }
 
 return GenericSelectionExpr::Create(
 S.Context, gse->getGenericLoc(), gse->getControllingExpr(),
-assocTypes, assocs, gse->getDefaultLoc(), gse->getRParenLoc(),
+assocTypes, assocExprs, gse->getDefaultLoc(), gse->getRParenLoc(),
 gse->containsUnexpandedParameterPack(), resultIndex);
   }
 
Index: lib/Sema/SemaExprObjC.cpp
===
--- lib/Sema/SemaExprObjC.cpp
+++ lib/Sema/SemaExprObjC.cpp
@@ -4332,14 +4332,16 @@
 assert(!gse->isResultDependent());
 
 unsigned n = gse->getNumAssocs();
-SmallVector subExprs(n);
-SmallVector subTypes(n);
-for (unsigned i = 0; i != n; ++i) {
-  subTypes[i] = gse->getAssocTypeSourceInfo(i);
-  Expr *sub = gse->getAssocExpr(i);
-  if (i == gse->getResultIndex())
+SmallVector subExprs;
+SmallVector subTypes;
+subExprs.reserve(n);
+subTypes.reserve(n);
+for (GenericSelectionExpr::Association assoc : gse->associations()) {
+  subTypes.push_back(assoc.getTypeSourceInfo());
+  Expr *sub = assoc.getAssociationExpr();
+  if (assoc.isSelected())
 sub = stripARCUnbridgedCast(sub);
-  subExprs[i] = sub;
+  subExprs.push_back(sub);
 }
 
 return GenericSelectionExpr::Create(
Index: lib/AST/StmtProfile.cpp
===
--- lib/AST/StmtProfile.cpp
+++ lib/AST/StmtProfile.cpp
@@ -1259,13 +1259,13 @@
 
 void StmtProfiler::VisitGenericSelectionExpr(const GenericSelectionExpr *S) {
   VisitExpr(S);
-  for (unsigned i = 0; i != S->getNumAssocs(); ++i) {
-QualType T = S->getAssocType(i);
+  for (GenericSelectionExpr::ConstAssociation Assoc : S->associations()) {
+QualType T = Assoc.getType();
 if (T.isNull())
   ID.AddPointer(nullptr);
 else
   VisitType(T);
-VisitExpr(S->getAssocExpr(i));
+VisitExpr(Assoc.getAssociationExpr());
   }
 }
 
Index: lib/AST/StmtPrinter.cpp
===
--- lib/AST/StmtPrinter.cpp
+++ lib/AST/StmtPrinter.cpp
@@ -1261,15 +1261,15 @@
 void StmtPrinter::VisitGenericSelectionExpr(GenericSelectionExpr *Node) {
   OS << "_Generic(";
   Pri

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

@lebedev.ri
Thanks for the clarifications!
I will split this up into multiple patches once we settled on a design.

In D56624#1369635 , @vsk wrote:

> What are the advantages of a generalized expect_noreturn attribute, vs. a 
> narrower attribute or intrinsic? The expect_noreturn semantics do not provide 
> strong guarantees, and are not really orthogonal from the pre-existing cold 
> attribute.


@eugenis Do you want to chime in here?
I think they convey different meanings even if their treatment by the optimizer 
is similar. The `cold` attribute says nothing about whether or not a function 
is expected to return.

> In particular, expect_noreturn doesn't even seem strong enough to allow ASan 
> to unpoison its stack.

I am not sure I understand this part. Can you elaborate?

For context:

  ``cold``
This attribute indicates that this function is rarely called. When
computing edge weights, basic blocks post-dominated by a cold
function call are also considered to be cold; and, thus, given low
weight.
  ``noreturn``
This function attribute indicates that the function never returns
normally. This produces undefined behavior at runtime if the
function ever does dynamically return.
  ``expect_noreturn``
This function attribute indicates that the function is unlikely to return
normally, but that it is still allowed to do so. This is useful in cases
for which ``noreturn`` is too strong a guarantee.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

Re: r347205 - [FileManager] getFile(open=true) after getFile(open=false) should open the file.

[Sam McCall via cfe-commits]

Reverted as r352079, with a test derived from this thread.
https://bugs.llvm.org/show_bug.cgi?id=40448 to cherrypick to branch.
I'll work on relanding a fixed version soon.

Thanks again for investigating, and sorry for the trouble.

On Thu, Jan 24, 2019 at 5:05 PM Sam McCall  wrote:

> Sure. Op counts and linux benchmarks are easy, I could try to get access
> to mac/windows too...
>
> Building clang's HeaderSearch.cpp (without PCH) yields:
> master: fstat = 341, open = 1039, stat = 211
> openFile=false: fstat = 9 (-332), open = 379 (-660), stat = 1203 (+992)
> revert: fstat = 341 (+0), open = 1039 (+0), stat = 211 (+0)
> (this is on linux, but that shouldn't matter to the counts)
>
> Timings on my workstation (-fsyntax-only, disk caches flushed, warmed up
> by running clang with no args):
> master: 2.063
> openFile=false patch: 1.953
> revert: 1.946
> (stdev is around 0.07)
> So openFile=false a regression of 6% or so on linux, I don't think it's
> worth testing anything else :-\
>
> Best plan I have now is:
>  - revert r347205, disable tests that this breaks
>  - cherrypick the revert to 8.0 branch
>  - reapply r347205 + a version of this patch where the openFile value is
> plumbed through, with false for the PCH case.
> I'll send the revert out shortly. It should probably get reviewed since it
> won't be clean.
> The final patch is going to be ugly, but there's time to come up with a
> better idea.
>
> On Thu, Jan 24, 2019 at 3:26 PM Nico Weber  wrote:
>
>> Can you measure on macOS and Windows too, and maybe with a larger
>> program? The open+fstat (instead of stat+open) optimization was I think
>> done with benchmarking on macOS.
>>
>> I can see why you say you think the new behavior is more consistent, but
>> it's basically changing what information goes into the getFile() cache key.
>> Previously, openFile didn't go in there, and from that perspective saying
>> "the cache keeps the openFile passed the first time you call this for a
>> file" makes sense to me too.
>>
>> I'm happy with either before your patch or with getFileAndSuggestModule()
>> passing openFile=false if perf is comparable (which I expect it to be, to
>> be honest).
>>
>> Since the openFile=false change might be a bit risky, maybe we should
>> revert on 8.0 and do openFile=false only on trunk.
>>
>> On Thu, Jan 24, 2019 at 6:51 AM Sam McCall  wrote:
>>
>>> So from my reading starting at getFileAndSuggestModule, changing to
>>> openFile=false is correct there unless some code is relying on that side
>>> effect.
>>> All the tests pass, too.
>>>
>>> The most likely regression is where we previously did open+fstat we'll
>>> now do stat+open+fstat.
>>>
>>> I tried running three commands using your repro, counting
>>> stat/open/fstat by instrumenting Unix/Path.inc.
>>> I used three versions of the code: old = before my patch, mid = with
>>> r347205, new = with r347205 and openFile=false.
>>>
>>> Build PCH:
>>>   stat: 331 -> 331 -> 635
>>>   open: 316 -> 314 -> 316
>>>   fstat: 311 -> 311 -> 8
>>>
>>> Use PCH:
>>>  stat: 322 -> 322 -> 924
>>>  open: 910 -> 609 -> 308
>>>  fstat: 607 -> 607 -> 5
>>>
>>> No PCH:
>>>  stat: 16 -> 16 -> 617
>>>  open: 606 -> 606 -> 306
>>>  fstat: 604 -> 604 -> 3
>>>
>>> Summary:
>>>  - r347205 was neutral for stats and fstats. It caused fewer files to be
>>> opened when building a PCH (though failed to close the ones it did). It
>>> caused file handles to leak when using a PCH.
>>>  - openFile=false reduces the number of files opened when compiling
>>> (with or without PCH). It also changes most fstats to stats, which is a
>>> loss.
>>>  - compared to the original state, having both patches seems like a
>>> small loss when building a PCH, and a small win when using one or compiling
>>> without one.
>>>
>>> So I'm tempted to check in the openFile=false.
>>> It seems more internally consistent (passing openFile=true leaves the
>>> file open), and avoids breaking clangd.
>>> It seems uninvasive, though as we've seen side-effects are hard to
>>> predict. Performance seems likely to be not-worse (vs reverting r347205).
>>>
>>> On Thu, Jan 24, 2019 at 10:49 AM Sam McCall 
>>> wrote:
>>>
 Thanks for all the digging!
 I can reproduce locally with your last example and ulimit -n 1000.

 So it sounds like:
  - this *is* actually the interaction/behavior I expected from
 FileManager (we call first with openFile=false then with openFile=true, and
 the file is opened the second time)
  - the problem manifests because the file is opened but never closed,
 so we leak file descriptors, but closing the file would'nt be the best fix
 - we shouldn't open it if we don't need its content
  - I would call this a bug in the caller of file manager (in the stack
 we saw where getFileAndSuggestModule calls getFile), because it calls with
 openFile=true, but doesn't use the file contents nor ensure the file is
 closed. This was previously masked by the bug (fixed in my patch) where

r352084 - Add a priority field to availability attributes to prioritize explicit

[Alex Lorenz via cfe-commits]

Author: arphaman
Date: Thu Jan 24 11:14:39 2019
New Revision: 352084

URL: http://llvm.org/viewvc/llvm-project?rev=352084&view=rev
Log:
Add a priority field to availability attributes to prioritize explicit
attributes from declaration over attributes from '#pragma clang attribute'

Before this commit users had an issue when using #pragma clang attribute with
availability attributes:

The explicit attribute that's specified next to the declaration is not
guaranteed to be preferred over the attribute specified in the pragma.

This commit fixes this by introducing a priority field to the availability
attribute to control how they're merged. Attributes with higher priority are
applied over attributes with lower priority for the same platform. The
implicitly inferred attributes are given the lower priority. This ensures that:

- explicit attributes are preferred over all other attributes.
- implicitly inferred attributes that are inferred from an explicit attribute
  are discarded if there's an explicit attribute or an attribute specified
  using a #pragma for the same platform.
- implicitly inferred attributes that are inferred from an attribute in the
  #pragma are not used if there's an explicit, explicit #pragma, or an
  implicit attribute inferred from an explicit attribute for the declaration.

This is the resulting ranking:

`platform availability > platform availability from pragma > inferred 
availability > inferred availability from pragma`

rdar://46390243

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

Added:
cfe/trunk/test/SemaObjC/attr-availability-priority.m
Modified:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/include/clang/Sema/ParsedAttr.h
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaAttr.cpp
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp

Modified: cfe/trunk/include/clang/Basic/Attr.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=352084&r1=352083&r2=352084&view=diff
==
--- cfe/trunk/include/clang/Basic/Attr.td (original)
+++ cfe/trunk/include/clang/Basic/Attr.td Thu Jan 24 11:14:39 2019
@@ -717,7 +717,8 @@ def Availability : InheritableAttr {
   let Args = [IdentifierArgument<"platform">, VersionArgument<"introduced">,
   VersionArgument<"deprecated">, VersionArgument<"obsoleted">,
   BoolArgument<"unavailable">, StringArgument<"message">,
-  BoolArgument<"strict">, StringArgument<"replacement">];
+  BoolArgument<"strict">, StringArgument<"replacement">,
+  IntArgument<"priority">];
   let AdditionalMembers =
 [{static llvm::StringRef getPrettyPlatformName(llvm::StringRef Platform) {
 return llvm::StringSwitch(Platform)

Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=352084&r1=352083&r2=352084&view=diff
==
--- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
+++ cfe/trunk/include/clang/Basic/AttrDocs.td Thu Jan 24 11:14:39 2019
@@ -1152,11 +1152,14 @@ replacement=\ *string-literal*
   the deprecated declaration with the new declaration specified.
 
 Multiple availability attributes can be placed on a declaration, which may
-correspond to different platforms.  Only the availability attribute with the
-platform corresponding to the target platform will be used; any others will be
-ignored.  If no availability attribute specifies availability for the current
-target platform, the availability attributes are ignored.  Supported platforms
-are:
+correspond to different platforms. For most platforms, the availability
+attribute with the platform corresponding to the target platform will be used;
+any others will be ignored. However, the availability for ``watchOS`` and
+``tvOS`` can be implicitly inferred from an ``iOS`` availability attribute.
+Any explicit availability attributes for those platforms are still prefered 
over
+the implicitly inferred availability attributes. If no availability attribute
+specifies availability for the current target platform, the availability
+attributes are ignored. Supported platforms are:
 
 ``ios``
   Apple's iOS operating system.  The minimum deployment target is specified by
@@ -1229,6 +1232,63 @@ Starting with the macOS 10.12 SDK, the `
   - (id)otherMethod API_AVAILABLE(macos(10.11), ios(11.0));
   @end
 
+Availability attributes can also be applied using a ``#pragma clang 
attribute``.
+Any explicit availability attribute whose platform corresponds to the target
+platform is applied to a declaration regardless of the availability attributes
+specified in the pragma. For example, in the code below,
+``hasExplicitAvailabilityAttribute`` will use the ``macOS`` availability
+attribute that is sp

[PATCH] D56892: Add a priority field to availability attributes to prioritize explicit attributes from declaration over attributes from '#pragma clang attribute'

[Alex Lorenz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352084: Add a priority field to availability attributes to 
prioritize explicit (authored by arphaman, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56892?vs=182608&id=183353#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56892

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Sema/ParsedAttr.h
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaAttr.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/SemaObjC/attr-availability-priority.m

Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -2459,18 +2459,38 @@
 AMK_ProtocolImplementation,
   };
 
+  /// Describes the kind of priority given to an availability attribute.
+  ///
+  /// The sum of priorities deteremines the final priority of the attribute.
+  /// The final priority determines how the attribute will be merged.
+  /// An attribute with a lower priority will always remove higher priority
+  /// attributes for the specified platform when it is being applied. An
+  /// attribute with a higher priority will not be applied if the declaration
+  /// already has an availability attribute with a lower priority for the
+  /// specified platform. The final prirority values are not expected to match
+  /// the values in this enumeration, but instead should be treated as a plain
+  /// integer value. This enumeration just names the priority weights that are
+  /// used to calculate that final vaue.
+  enum AvailabilityPriority : int {
+/// The availability attribute was specified explicitly next to the
+/// declaration.
+AP_Explicit = 0,
+
+/// The availability attribute was applied using '#pragma clang attribute'.
+AP_PragmaClangAttribute = 1,
+
+/// The availability attribute for a specific platform was inferred from
+/// an availability attribute for another platform.
+AP_InferredFromOtherPlatform = 2
+  };
+
   /// Attribute merging methods. Return true if a new attribute was added.
-  AvailabilityAttr *mergeAvailabilityAttr(NamedDecl *D, SourceRange Range,
-  IdentifierInfo *Platform,
-  bool Implicit,
-  VersionTuple Introduced,
-  VersionTuple Deprecated,
-  VersionTuple Obsoleted,
-  bool IsUnavailable,
-  StringRef Message,
-  bool IsStrict, StringRef Replacement,
-  AvailabilityMergeKind AMK,
-  unsigned AttrSpellingListIndex);
+  AvailabilityAttr *mergeAvailabilityAttr(
+  NamedDecl *D, SourceRange Range, IdentifierInfo *Platform, bool Implicit,
+  VersionTuple Introduced, VersionTuple Deprecated, VersionTuple Obsoleted,
+  bool IsUnavailable, StringRef Message, bool IsStrict,
+  StringRef Replacement, AvailabilityMergeKind AMK, int Priority,
+  unsigned AttrSpellingListIndex);
   TypeVisibilityAttr *mergeTypeVisibilityAttr(Decl *D, SourceRange Range,
TypeVisibilityAttr::VisibilityType Vis,
   unsigned AttrSpellingListIndex);
Index: cfe/trunk/include/clang/Sema/ParsedAttr.h
===
--- cfe/trunk/include/clang/Sema/ParsedAttr.h
+++ cfe/trunk/include/clang/Sema/ParsedAttr.h
@@ -207,6 +207,9 @@
   /// A cached value.
   mutable unsigned ProcessingCache : 8;
 
+  /// True if the attribute is specified using '#pragma clang attribute'.
+  mutable unsigned IsPragmaClangAttribute : 1;
+
   /// The location of the 'unavailable' keyword in an
   /// availability attribute.
   SourceLocation UnavailableLoc;
@@ -238,7 +241,8 @@
 ScopeLoc(scopeLoc), EllipsisLoc(ellipsisLoc), NumArgs(numArgs),
 SyntaxUsed(syntaxUsed), Invalid(false), UsedAsTypeAttr(false),
 IsAvailability(false), IsTypeTagForDatatype(false), IsProperty(false),
-HasParsedType(false), HasProcessingCache(false) {
+HasParsedType(false), HasProcessingCache(false),
+IsPragmaClangAttribute(false) {
 if (numArgs) memcpy(getArgsBuffer(), args, numArgs * sizeof(ArgsUnion));
 AttrKind = getKind(getName(), getScopeName(), syntaxUsed);
   }
@@ -255,8 +259,8 @@
 ScopeLoc(scopeLoc), NumArgs(1), SyntaxUsed(syntaxUsed), Invalid(false),
 UsedAsTypeAttr(false), IsAvail

[PATCH] D57175: [NFC][clang] Test updates for CreateAlignmentAssumption() changes in D54653

[Roman Lebedev via Phabricator via cfe-commits]

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

Self-accepting since D54653  got reviewed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57175



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

[PATCH] D50452: [WIP] clangd XPC adapter

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Abandonned in favor of https://reviews.llvm.org/D54428


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D50452



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

[PATCH] D56624: [Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D56624#1369767 , @yln wrote:

> In D56624#1369635 , @vsk wrote:
>
> > What are the advantages of a generalized expect_noreturn attribute, vs. a 
> > narrower attribute or intrinsic? The expect_noreturn semantics do not 
> > provide strong guarantees, and are not really orthogonal from the 
> > pre-existing cold attribute.
>
>
> @eugenis Do you want to chime in here?
>  I think they convey different meanings even if their treatment by the 
> optimizer is similar. The `cold` attribute says nothing about whether or not 
> a function is expected to return.


That's my point: it doesn't need to, because it's orthogonal. It's just a hint 
that a call is cold and could be profitable to split/reorder. Features of llvm 
IR generally try to be orthogonal to reduce complexity in the optimizer.

>> In particular, expect_noreturn doesn't even seem strong enough to allow ASan 
>> to unpoison its stack.
> 
> I am not sure I understand this part. Can you elaborate?

Because "expect_noreturn" calls are allowed to return, the compiler must behave 
as they could. In particular, this means that unpoisoning the stack before 
expect_noreturn calls (given the current semantics) is premature.

Put another way, a frontend author may (understandably, but mistakenly!) attach 
expect_noreturn to calls which they expect to be cold. That would regress ASan 
coverage.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624



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

[PATCH] D14686: Protect against overloaded comma in random_shuffle and improve tests

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists closed this revision.
mclow.lists added a comment.
Herald added a subscriber: llvm-commits.

(Finally) committed this as revision 352087.
I cut out most of the random_shuffle_rand.pass.cpp test, because it relied on 
C++11 features, and didn't work for C++03.
If you want to re-submit the test as a separate patch, I promise to review it 
in a more timely manner.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D14686



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

[clang-tools-extra] r352088 - [clang-tidy] Rename the absl duration helper functions; NFC

[Hyrum Wright via cfe-commits]

Author: hwright
Date: Thu Jan 24 11:23:50 2019
New Revision: 352088

URL: http://llvm.org/viewvc/llvm-project?rev=352088&view=rev
Log:
[clang-tidy] Rename the absl duration helper functions; NFC

Modified:
clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationConversionCastCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.h
clang-tools-extra/trunk/clang-tidy/abseil/DurationSubtractionCheck.cpp

Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp?rev=352088&r1=352087&r2=352088&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationComparisonCheck.cpp Thu 
Jan 24 11:23:50 2019
@@ -34,7 +34,7 @@ void DurationComparisonCheck::registerMa
 void DurationComparisonCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Binop = Result.Nodes.getNodeAs("binop");
 
-  llvm::Optional Scale = getScaleForInverse(
+  llvm::Optional Scale = getScaleForDurationInverse(
   Result.Nodes.getNodeAs("function_decl")->getName());
   if (!Scale)
 return;

Modified: 
clang-tools-extra/trunk/clang-tidy/abseil/DurationConversionCastCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationConversionCastCheck.cpp?rev=352088&r1=352087&r2=352088&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationConversionCastCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationConversionCastCheck.cpp 
Thu Jan 24 11:23:50 2019
@@ -44,14 +44,15 @@ void DurationConversionCastCheck::check(
   const auto *Arg = Result.Nodes.getNodeAs("arg");
   StringRef ConversionFuncName = FuncDecl->getName();
 
-  llvm::Optional Scale = getScaleForInverse(ConversionFuncName);
+  llvm::Optional Scale =
+  getScaleForDurationInverse(ConversionFuncName);
   if (!Scale)
 return;
 
   // Casting a double to an integer.
   if (MatchedCast->getTypeAsWritten()->isIntegerType() &&
   ConversionFuncName.contains("Double")) {
-llvm::StringRef NewFuncName = getInverseForScale(*Scale).second;
+llvm::StringRef NewFuncName = getDurationInverseForScale(*Scale).second;
 
 diag(MatchedCast->getBeginLoc(),
  "duration should be converted directly to an integer rather than "
@@ -66,7 +67,7 @@ void DurationConversionCastCheck::check(
   // Casting an integer to a double.
   if (MatchedCast->getTypeAsWritten()->isRealFloatingType() &&
   ConversionFuncName.contains("Int64")) {
-llvm::StringRef NewFuncName = getInverseForScale(*Scale).first;
+llvm::StringRef NewFuncName = getDurationInverseForScale(*Scale).first;
 
 diag(MatchedCast->getBeginLoc(), "duration should be converted directly to 
"
  "a floating-piont number rather than "

Modified: 
clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp?rev=352088&r1=352087&r2=352088&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/abseil/DurationFactoryScaleCheck.cpp Thu 
Jan 24 11:23:50 2019
@@ -209,7 +209,7 @@ void DurationFactoryScaleCheck::check(co
   diag(Call->getBeginLoc(), "internal duration scaling can be removed")
   << FixItHint::CreateReplacement(
  Call->getSourceRange(),
- (llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
+ (llvm::Twine(getDurationFactoryForScale(*NewScale)) + "(" +
   tooling::fixit::getText(*Remainder, *Result.Context) + ")")
  .str());
 }
@@ -222,7 +222,7 @@ void DurationFactoryScaleCheck::check(co
 diag(Call->getBeginLoc(), "internal duration scaling can be removed")
 << FixItHint::CreateReplacement(
Call->getSourceRange(),
-   (llvm::Twine(getFactoryForScale(*NewScale)) + "(" +
+   (llvm::Twine(getDurationFactoryForScale(*NewScale)) + "(" +
 tooling::fixit::getText(*Remainder, *Result.Context) + ")")
.str());
   }

Modified: clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationRewriter.cpp?rev=352088&r1=352087&r2=352088&view=d

[PATCH] D57155: [WebAssembly] Add a __wasi__ target macro

[Derek Schuff via Phabricator via cfe-commits]

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

LGTM; don't forget to include all the context in the future


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57155



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

[PATCH] D57175: [NFC][clang] Test updates for CreateAlignmentAssumption() changes in D54653

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL352090: [NFC][clang] Test updates for 
CreateAlignmentAssumption() changes in D54653 (authored by lebedevri, committed 
by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57175?vs=183350&id=183356#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57175

Files:
  cfe/trunk/test/CodeGen/alloc-align-attr.c
  
cfe/trunk/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp


Index: 
cfe/trunk/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
===
--- 
cfe/trunk/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
+++ 
cfe/trunk/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
@@ -30,9 +30,7 @@
   // CHECK-NEXT:%[[X_RELOADED:.*]] = load i8**, i8*** 
%[[X_ADDR]], align 8
   // CHECK-NEXT:%[[ALIGNMENT_RELOADED:.*]] = load i64, 
i64* %[[ALIGNMENT_ADDR]], align 8
   // CHECK-NEXT:%[[X_RETURNED:.*]] = call i8** 
@[[PASSTHROUGH]](i8** %[[X_RELOADED]], i64 %[[ALIGNMENT_RELOADED]])
-  // CHECK-NEXT:%[[ISPOSITIVE:.*]] = icmp sgt i64 
%[[ALIGNMENT_RELOADED]], 0
-  // CHECK-NEXT:%[[POSITIVEMASK:.*]] = sub i64 
%[[ALIGNMENT_RELOADED]], 1
-  // CHECK-NEXT:%[[MASK:.*]] = select i1 
%[[ISPOSITIVE]], i64 %[[POSITIVEMASK]], i64 0
+  // CHECK-NEXT:%[[MASK:.*]] = sub i64 
%[[ALIGNMENT_RELOADED]], 1
   // CHECK-NEXT:%[[PTRINT:.*]] = ptrtoint i8** 
%[[X_RETURNED]] to i64
   // CHECK-NEXT:%[[MASKEDPTR:.*]] = and i64 
%[[PTRINT]], %[[MASK]]
   // CHECK-NEXT:%[[MASKCOND:.*]] = icmp eq i64 
%[[MASKEDPTR]], 0
Index: cfe/trunk/test/CodeGen/alloc-align-attr.c
===
--- cfe/trunk/test/CodeGen/alloc-align-attr.c
+++ cfe/trunk/test/CodeGen/alloc-align-attr.c
@@ -6,11 +6,9 @@
 __INT32_TYPE__ test1(__INT32_TYPE__ a) {
 // CHECK: define i32 @test1
   return *m1(a);
-// CHECK: call i32* @m1(i32 [[PARAM1:%[^\)]+]]) 
-// CHECK: [[ALIGNCAST1:%.+]] = sext i32 [[PARAM1]] to i64
-// CHECK: [[ISPOS1:%.+]] = icmp sgt i64 [[ALIGNCAST1]], 0
-// CHECK: [[POSMASK1:%.+]] = sub i64 [[ALIGNCAST1]], 1
-// CHECK: [[MASK1:%.+]] = select i1 [[ISPOS1]], i64 [[POSMASK1]], i64 0
+// CHECK: call i32* @m1(i32 [[PARAM1:%[^\)]+]])
+// CHECK: [[ALIGNCAST1:%.+]] = zext i32 [[PARAM1]] to i64
+// CHECK: [[MASK1:%.+]] = sub i64 [[ALIGNCAST1]], 1
 // CHECK: [[PTRINT1:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR1:%.+]] = and i64 [[PTRINT1]], [[MASK1]]
 // CHECK: [[MASKCOND1:%.+]] = icmp eq i64 [[MASKEDPTR1]], 0
@@ -22,10 +20,8 @@
   return *m1(a);
 // CHECK: [[CONV2:%.+]] = trunc i64 %{{.+}} to i32
 // CHECK: call i32* @m1(i32 [[CONV2]])
-// CHECK: [[ALIGNCAST2:%.+]] = sext i32 [[CONV2]] to i64
-// CHECK: [[ISPOS2:%.+]] = icmp sgt i64 [[ALIGNCAST2]], 0
-// CHECK: [[POSMASK2:%.+]] = sub i64 [[ALIGNCAST2]], 1
-// CHECK: [[MASK2:%.+]] = select i1 [[ISPOS2]], i64 [[POSMASK2]], i64 0
+// CHECK: [[ALIGNCAST2:%.+]] = zext i32 [[CONV2]] to i64
+// CHECK: [[MASK2:%.+]] = sub i64 [[ALIGNCAST2]], 1
 // CHECK: [[PTRINT2:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR2:%.+]] = and i64 [[PTRINT2]], [[MASK2]]
 // CHECK: [[MASKCOND2:%.+]] = icmp eq i64 [[MASKEDPTR2]], 0
@@ -39,9 +35,7 @@
   return *m2(a);
 // CHECK: [[CONV3:%.+]] = sext i32 %{{.+}} to i64
 // CHECK: call i32* @m2(i64 [[CONV3]])
-// CHECK: [[ISPOS3:%.+]] = icmp sgt i64 [[CONV3]], 0
-// CHECK: [[POSMASK3:%.+]] = sub i64 [[CONV3]], 1
-// CHECK: [[MASK3:%.+]] = select i1 [[ISPOS3]], i64 [[POSMASK3]], i64 0
+// CHECK: [[MASK3:%.+]] = sub i64 [[CONV3]], 1
 // CHECK: [[PTRINT3:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR3:%.+]] = and i64 [[PTRINT3]], [[MASK3]]
 // CHECK: [[MASKCOND3:%.+]] = icmp eq i64 [[MASKEDPTR3]], 0
@@ -52,10 +46,8 @@
 __INT32_TYPE__ test4(__SIZE_TYPE__ a) {
 // CHECK: define i32 @test4
   return *m2(a);
-// CHECK: call i32* @m2(i64 [[PARAM4:%[^\)]+]]) 
-// CHECK: [[ISPOS4:%.+]] = icmp sgt i64 [[PARAM4]], 0
-// CHECK: [[POSMASK4:%.+]] = sub i64 [[PARAM4]], 1
-// CHECK: [[MASK4:%.+]] = select i1 [[ISPOS4]], i64 [[POSMASK4]], i64 0
+// CHECK: call i32* @m2(i64 [[PARAM4:%[^\)]+]])
+// CHECK: [[MASK4:%.+]] = sub i64 [[PARAM4]], 1
 // CHECK: [[PTRINT4:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR4:%.+]] = and i64 [[PTRINT4]], [[MASK4]]
 // CHECK: [[MASKCOND4:%.+]] = icmp eq i64 [[MASKEDPTR4]], 0
@@ -72,11 +64,9 @@
 // CHECK: define i32 @test5
   struct Empty e;
   return *m3(e, a);
-// CHECK: call i32* @m3(i64 %{{.*}}, i64 %{{.*}}) 
+// CHECK: call i32* @m3(i64 %{{.*}}, i64 %{{.*}})
 // CHECK: [[ALIGNCAST5:%.+]] = trunc i128 %{{.*}} to

r352090 - [NFC][clang] Test updates for CreateAlignmentAssumption() changes in D54653

[Roman Lebedev via cfe-commits]

Author: lebedevri
Date: Thu Jan 24 11:32:49 2019
New Revision: 352090

URL: http://llvm.org/viewvc/llvm-project?rev=352090&view=rev
Log:
[NFC][clang] Test updates for CreateAlignmentAssumption() changes in D54653

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

Modified:
cfe/trunk/test/CodeGen/alloc-align-attr.c

cfe/trunk/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp

Modified: cfe/trunk/test/CodeGen/alloc-align-attr.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/alloc-align-attr.c?rev=352090&r1=352089&r2=352090&view=diff
==
--- cfe/trunk/test/CodeGen/alloc-align-attr.c (original)
+++ cfe/trunk/test/CodeGen/alloc-align-attr.c Thu Jan 24 11:32:49 2019
@@ -6,11 +6,9 @@ __INT32_TYPE__*m1(__INT32_TYPE__ i) __at
 __INT32_TYPE__ test1(__INT32_TYPE__ a) {
 // CHECK: define i32 @test1
   return *m1(a);
-// CHECK: call i32* @m1(i32 [[PARAM1:%[^\)]+]]) 
-// CHECK: [[ALIGNCAST1:%.+]] = sext i32 [[PARAM1]] to i64
-// CHECK: [[ISPOS1:%.+]] = icmp sgt i64 [[ALIGNCAST1]], 0
-// CHECK: [[POSMASK1:%.+]] = sub i64 [[ALIGNCAST1]], 1
-// CHECK: [[MASK1:%.+]] = select i1 [[ISPOS1]], i64 [[POSMASK1]], i64 0
+// CHECK: call i32* @m1(i32 [[PARAM1:%[^\)]+]])
+// CHECK: [[ALIGNCAST1:%.+]] = zext i32 [[PARAM1]] to i64
+// CHECK: [[MASK1:%.+]] = sub i64 [[ALIGNCAST1]], 1
 // CHECK: [[PTRINT1:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR1:%.+]] = and i64 [[PTRINT1]], [[MASK1]]
 // CHECK: [[MASKCOND1:%.+]] = icmp eq i64 [[MASKEDPTR1]], 0
@@ -22,10 +20,8 @@ __INT32_TYPE__ test2(__SIZE_TYPE__ a) {
   return *m1(a);
 // CHECK: [[CONV2:%.+]] = trunc i64 %{{.+}} to i32
 // CHECK: call i32* @m1(i32 [[CONV2]])
-// CHECK: [[ALIGNCAST2:%.+]] = sext i32 [[CONV2]] to i64
-// CHECK: [[ISPOS2:%.+]] = icmp sgt i64 [[ALIGNCAST2]], 0
-// CHECK: [[POSMASK2:%.+]] = sub i64 [[ALIGNCAST2]], 1
-// CHECK: [[MASK2:%.+]] = select i1 [[ISPOS2]], i64 [[POSMASK2]], i64 0
+// CHECK: [[ALIGNCAST2:%.+]] = zext i32 [[CONV2]] to i64
+// CHECK: [[MASK2:%.+]] = sub i64 [[ALIGNCAST2]], 1
 // CHECK: [[PTRINT2:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR2:%.+]] = and i64 [[PTRINT2]], [[MASK2]]
 // CHECK: [[MASKCOND2:%.+]] = icmp eq i64 [[MASKEDPTR2]], 0
@@ -39,9 +35,7 @@ __INT32_TYPE__ test3(__INT32_TYPE__ a) {
   return *m2(a);
 // CHECK: [[CONV3:%.+]] = sext i32 %{{.+}} to i64
 // CHECK: call i32* @m2(i64 [[CONV3]])
-// CHECK: [[ISPOS3:%.+]] = icmp sgt i64 [[CONV3]], 0
-// CHECK: [[POSMASK3:%.+]] = sub i64 [[CONV3]], 1
-// CHECK: [[MASK3:%.+]] = select i1 [[ISPOS3]], i64 [[POSMASK3]], i64 0
+// CHECK: [[MASK3:%.+]] = sub i64 [[CONV3]], 1
 // CHECK: [[PTRINT3:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR3:%.+]] = and i64 [[PTRINT3]], [[MASK3]]
 // CHECK: [[MASKCOND3:%.+]] = icmp eq i64 [[MASKEDPTR3]], 0
@@ -52,10 +46,8 @@ __INT32_TYPE__ test3(__INT32_TYPE__ a) {
 __INT32_TYPE__ test4(__SIZE_TYPE__ a) {
 // CHECK: define i32 @test4
   return *m2(a);
-// CHECK: call i32* @m2(i64 [[PARAM4:%[^\)]+]]) 
-// CHECK: [[ISPOS4:%.+]] = icmp sgt i64 [[PARAM4]], 0
-// CHECK: [[POSMASK4:%.+]] = sub i64 [[PARAM4]], 1
-// CHECK: [[MASK4:%.+]] = select i1 [[ISPOS4]], i64 [[POSMASK4]], i64 0
+// CHECK: call i32* @m2(i64 [[PARAM4:%[^\)]+]])
+// CHECK: [[MASK4:%.+]] = sub i64 [[PARAM4]], 1
 // CHECK: [[PTRINT4:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR4:%.+]] = and i64 [[PTRINT4]], [[MASK4]]
 // CHECK: [[MASKCOND4:%.+]] = icmp eq i64 [[MASKEDPTR4]], 0
@@ -72,11 +64,9 @@ __INT32_TYPE__ test5(__int128_t a) {
 // CHECK: define i32 @test5
   struct Empty e;
   return *m3(e, a);
-// CHECK: call i32* @m3(i64 %{{.*}}, i64 %{{.*}}) 
+// CHECK: call i32* @m3(i64 %{{.*}}, i64 %{{.*}})
 // CHECK: [[ALIGNCAST5:%.+]] = trunc i128 %{{.*}} to i64
-// CHECK: [[ISPOS5:%.+]] = icmp sgt i64 [[ALIGNCAST5]], 0
-// CHECK: [[POSMASK5:%.+]] = sub i64 [[ALIGNCAST5]], 1
-// CHECK: [[MASK5:%.+]] = select i1 [[ISPOS5]], i64 [[POSMASK5]], i64 0
+// CHECK: [[MASK5:%.+]] = sub i64 [[ALIGNCAST5]], 1
 // CHECK: [[PTRINT5:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR5:%.+]] = and i64 [[PTRINT5]], [[MASK5]]
 // CHECK: [[MASKCOND5:%.+]] = icmp eq i64 [[MASKEDPTR5]], 0
@@ -88,11 +78,9 @@ __INT32_TYPE__ test6(__int128_t a) {
 // CHECK: define i32 @test6
   struct MultiArgs e;
   return *m4(e, a);
-// CHECK: call i32* @m4(i64 %{{.*}}, i64 %{{.*}}, i64 %{{.*}}) 
+// CHECK: call i32* @m4(i64 %{{.*}}, i64 %{{.*}}, i64 %{{.*}})
 // CHECK: [[ALIGNCAST6:%.+]] = trunc i128 %{{.*}} to i64
-// CHECK: [[ISPOS6:%.+]] = icmp sgt i64 [[ALIGNCAST6]], 0
-// CHECK: [[POSMASK6:%.+]] = sub i64 [[ALIGNCAST6]], 1
-// CHECK: [[MASK6:%.+]] = select i1 [[ISPOS6]], i64 [[POSMASK6]], i64 0
+// CHECK: [[MASK6:%.+]] = sub i64 [[ALIGNCAST6]], 1
 // CHECK: [[PTRINT6:%.+]] = ptrtoint
 // CHECK: [[MASKEDPTR6:%.+]] = and i64 [[PTRINT6]], [[MASK6]]
 // CHECK: [[MASKCOND6:%.+]] = icmp eq i64 [[MASKEDPTR6]], 0

Modified: 
cfe/trunk/test/CodeGen/catch-alignment-assumption-attribute-alloc_align-on-function-variable.cpp
URL: 
http://llvm.or

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Thanks, but I don't see the factored out changes in the repo. Are you going to 
commit those first?

Also, please factor out the introduction and use of `::Create`. It seems to be 
unrelated to the trailing objects change.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

LLVM buildmaster will be restarted tonight

[Galina Kistanova via cfe-commits]

 Hello everyone,

LLVM buildmaster will be updated and restarted after 6PM Pacific time today.

Thanks

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

[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D57104#1369864 , @steveire wrote:

> Thanks, but I don't see the factored out changes in the repo. Are you going 
> to commit those first?
>
> Also, please factor out the introduction and use of `::Create`. It seems to 
> be unrelated to the trailing objects change.


I was planning to yes. I don't agree that the introduction of `Create` is 
unrelated. It is very much part of the changes
since the introduction of the trailing objects implies the need to do a 
placement new, which implies the need to
use a `Create` function. Also see pretty much every other patch which 
introduced trailing objects for the
statement/expression nodes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104



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

r352099 - [WebAssembly] Factor commonality between wasm32 and wasm64 in test/Preprocessor/init.c

[Dan Gohman via cfe-commits]

Author: djg
Date: Thu Jan 24 12:31:11 2019
New Revision: 352099

URL: http://llvm.org/viewvc/llvm-project?rev=352099&view=rev
Log:
[WebAssembly] Factor commonality between wasm32 and wasm64 in 
test/Preprocessor/init.c

Use the -check-prefixes= feature to merge most of the WEBASSEMBLY32 and
WEBASSEMBLY64 test checks into a shared WEBASSEMBLY test check.

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

Modified:
cfe/trunk/test/Preprocessor/init.c

Modified: cfe/trunk/test/Preprocessor/init.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preprocessor/init.c?rev=352099&r1=352098&r2=352099&view=diff
==
--- cfe/trunk/test/Preprocessor/init.c (original)
+++ cfe/trunk/test/Preprocessor/init.c Thu Jan 24 12:31:11 2019
@@ -9110,667 +9110,376 @@
 //
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=wasm32-unknown-unknown \
 // RUN:   < /dev/null \
-// RUN:   | FileCheck -match-full-lines -check-prefix=WEBASSEMBLY32 %s
+// RUN:   | FileCheck -match-full-lines 
-check-prefixes=WEBASSEMBLY,WEBASSEMBLY32 %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=wasm64-unknown-unknown \
+// RUN:   < /dev/null \
+// RUN:   | FileCheck -match-full-lines 
-check-prefixes=WEBASSEMBLY,WEBASSEMBLY64 %s
 //
 // WEBASSEMBLY32:#define _ILP32 1
 // WEBASSEMBLY32-NOT:#define _LP64
-// WEBASSEMBLY32-NEXT:#define __ATOMIC_ACQUIRE 2
-// WEBASSEMBLY32-NEXT:#define __ATOMIC_ACQ_REL 4
-// WEBASSEMBLY32-NEXT:#define __ATOMIC_CONSUME 1
-// WEBASSEMBLY32-NEXT:#define __ATOMIC_RELAXED 0
-// WEBASSEMBLY32-NEXT:#define __ATOMIC_RELEASE 3
-// WEBASSEMBLY32-NEXT:#define __ATOMIC_SEQ_CST 5
-// WEBASSEMBLY32-NEXT:#define __BIGGEST_ALIGNMENT__ 16
-// WEBASSEMBLY32-NEXT:#define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
-// WEBASSEMBLY32-NEXT:#define __CHAR16_TYPE__ unsigned short
-// WEBASSEMBLY32-NEXT:#define __CHAR32_TYPE__ unsigned int
-// WEBASSEMBLY32-NEXT:#define __CHAR_BIT__ 8
-// WEBASSEMBLY32-NOT:#define __CHAR_UNSIGNED__
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_BOOL_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_CHAR16_T_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_CHAR32_T_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_CHAR_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_INT_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_LLONG_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_LONG_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_POINTER_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_SHORT_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CLANG_ATOMIC_WCHAR_T_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __CONSTANT_CFSTRINGS__ 1
-// WEBASSEMBLY32-NEXT:#define __DBL_DECIMAL_DIG__ 17
-// WEBASSEMBLY32-NEXT:#define __DBL_DENORM_MIN__ 4.9406564584124654e-324
-// WEBASSEMBLY32-NEXT:#define __DBL_DIG__ 15
-// WEBASSEMBLY32-NEXT:#define __DBL_EPSILON__ 2.2204460492503131e-16
-// WEBASSEMBLY32-NEXT:#define __DBL_HAS_DENORM__ 1
-// WEBASSEMBLY32-NEXT:#define __DBL_HAS_INFINITY__ 1
-// WEBASSEMBLY32-NEXT:#define __DBL_HAS_QUIET_NAN__ 1
-// WEBASSEMBLY32-NEXT:#define __DBL_MANT_DIG__ 53
-// WEBASSEMBLY32-NEXT:#define __DBL_MAX_10_EXP__ 308
-// WEBASSEMBLY32-NEXT:#define __DBL_MAX_EXP__ 1024
-// WEBASSEMBLY32-NEXT:#define __DBL_MAX__ 1.7976931348623157e+308
-// WEBASSEMBLY32-NEXT:#define __DBL_MIN_10_EXP__ (-307)
-// WEBASSEMBLY32-NEXT:#define __DBL_MIN_EXP__ (-1021)
-// WEBASSEMBLY32-NEXT:#define __DBL_MIN__ 2.2250738585072014e-308
-// WEBASSEMBLY32-NEXT:#define __DECIMAL_DIG__ __LDBL_DECIMAL_DIG__
-// WEBASSEMBLY32-NOT:#define __ELF__
-// WEBASSEMBLY32-NEXT:#define __FINITE_MATH_ONLY__ 0
-// WEBASSEMBLY32:#define __FLT_DECIMAL_DIG__ 9
-// WEBASSEMBLY32-NEXT:#define __FLT_DENORM_MIN__ 1.40129846e-45F
-// WEBASSEMBLY32-NEXT:#define __FLT_DIG__ 6
-// WEBASSEMBLY32-NEXT:#define __FLT_EPSILON__ 1.19209290e-7F
-// WEBASSEMBLY32-NEXT:#define __FLT_EVAL_METHOD__ 0
-// WEBASSEMBLY32-NEXT:#define __FLT_HAS_DENORM__ 1
-// WEBASSEMBLY32-NEXT:#define __FLT_HAS_INFINITY__ 1
-// WEBASSEMBLY32-NEXT:#define __FLT_HAS_QUIET_NAN__ 1
-// WEBASSEMBLY32-NEXT:#define __FLT_MANT_DIG__ 24
-// WEBASSEMBLY32-NEXT:#define __FLT_MAX_10_EXP__ 38
-// WEBASSEMBLY32-NEXT:#define __FLT_MAX_EXP__ 128
-// WEBASSEMBLY32-NEXT:#define __FLT_MAX__ 3.40282347e+38F
-// WEBASSEMBLY32-NEXT:#define __FLT_MIN_10_EXP__ (-37)
-// WEBASSEMBLY32-NEXT:#define __FLT_MIN_EXP__ (-125)
-// WEBASSEMBLY32-NEXT:#define __FLT_MIN__ 1.17549435e-38F
-// WEBASSEMBLY32-NEXT:#define __FLT_RADIX__ 2
-// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_BOOL_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_CHAR16_T_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_CHAR32_T_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_CHAR_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_INT_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_LLONG_LOCK_FREE 2
-// WEBASSEMBLY32-NEXT:#define __GCC_ATOMIC_LONG_LOCK_FREE 2
-// WEBASSEMBLY32-

r352100 - [WebAssembly] Support __float128

[Dan Gohman via cfe-commits]

Author: djg
Date: Thu Jan 24 12:33:28 2019
New Revision: 352100

URL: http://llvm.org/viewvc/llvm-project?rev=352100&view=rev
Log:
[WebAssembly] Support __float128

This enables support for the "__float128" keyword.

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

Modified:
cfe/trunk/lib/Basic/Targets/OSTargets.h
cfe/trunk/test/Preprocessor/init.c

Modified: cfe/trunk/lib/Basic/Targets/OSTargets.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/OSTargets.h?rev=352100&r1=352099&r2=352100&view=diff
==
--- cfe/trunk/lib/Basic/Targets/OSTargets.h (original)
+++ cfe/trunk/lib/Basic/Targets/OSTargets.h Thu Jan 24 12:33:28 2019
@@ -771,6 +771,8 @@ class LLVM_LIBRARY_VISIBILITY WebAssembl
 // Follow g++ convention and predefine _GNU_SOURCE for C++.
 if (Opts.CPlusPlus)
   Builder.defineMacro("_GNU_SOURCE");
+// Indicate that we have __float128.
+Builder.defineMacro("__FLOAT128__");
   }
 
 public:
@@ -779,6 +781,7 @@ public:
   : OSTargetInfo(Triple, Opts) {
 this->MCountName = "__mcount";
 this->TheCXXABI.set(TargetCXXABI::WebAssembly);
+this->HasFloat128 = true;
   }
 };
 

Modified: cfe/trunk/test/Preprocessor/init.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preprocessor/init.c?rev=352100&r1=352099&r2=352100&view=diff
==
--- cfe/trunk/test/Preprocessor/init.c (original)
+++ cfe/trunk/test/Preprocessor/init.c Thu Jan 24 12:33:28 2019
@@ -9159,6 +9159,7 @@
 // WEBASSEMBLY-NEXT:#define __DECIMAL_DIG__ __LDBL_DECIMAL_DIG__
 // WEBASSEMBLY-NOT:#define __ELF__
 // WEBASSEMBLY-NEXT:#define __FINITE_MATH_ONLY__ 0
+// WEBASSEMBLY-NEXT:#define __FLOAT128__ 1
 // WEBASSEMBLY-NEXT:#define __FLT16_DECIMAL_DIG__ 5
 // WEBASSEMBLY-NEXT:#define __FLT16_DENORM_MIN__ 5.9604644775390625e-8F16
 // WEBASSEMBLY-NEXT:#define __FLT16_DIG__ 3


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

[PATCH] D57154: [WebAssembly] Support __float128

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC352100: [WebAssembly] Support __float128 (authored by djg, 
committed by ).
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57154?vs=183302&id=183369#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57154

Files:
  lib/Basic/Targets/OSTargets.h
  test/Preprocessor/init.c


Index: test/Preprocessor/init.c
===
--- test/Preprocessor/init.c
+++ test/Preprocessor/init.c
@@ -9159,6 +9159,7 @@
 // WEBASSEMBLY-NEXT:#define __DECIMAL_DIG__ __LDBL_DECIMAL_DIG__
 // WEBASSEMBLY-NOT:#define __ELF__
 // WEBASSEMBLY-NEXT:#define __FINITE_MATH_ONLY__ 0
+// WEBASSEMBLY-NEXT:#define __FLOAT128__ 1
 // WEBASSEMBLY-NEXT:#define __FLT16_DECIMAL_DIG__ 5
 // WEBASSEMBLY-NEXT:#define __FLT16_DENORM_MIN__ 5.9604644775390625e-8F16
 // WEBASSEMBLY-NEXT:#define __FLT16_DIG__ 3
Index: lib/Basic/Targets/OSTargets.h
===
--- lib/Basic/Targets/OSTargets.h
+++ lib/Basic/Targets/OSTargets.h
@@ -771,6 +771,8 @@
 // Follow g++ convention and predefine _GNU_SOURCE for C++.
 if (Opts.CPlusPlus)
   Builder.defineMacro("_GNU_SOURCE");
+// Indicate that we have __float128.
+Builder.defineMacro("__FLOAT128__");
   }
 
 public:
@@ -779,6 +781,7 @@
   : OSTargetInfo(Triple, Opts) {
 this->MCountName = "__mcount";
 this->TheCXXABI.set(TargetCXXABI::WebAssembly);
+this->HasFloat128 = true;
   }
 };
 


Index: test/Preprocessor/init.c
===
--- test/Preprocessor/init.c
+++ test/Preprocessor/init.c
@@ -9159,6 +9159,7 @@
 // WEBASSEMBLY-NEXT:#define __DECIMAL_DIG__ __LDBL_DECIMAL_DIG__
 // WEBASSEMBLY-NOT:#define __ELF__
 // WEBASSEMBLY-NEXT:#define __FINITE_MATH_ONLY__ 0
+// WEBASSEMBLY-NEXT:#define __FLOAT128__ 1
 // WEBASSEMBLY-NEXT:#define __FLT16_DECIMAL_DIG__ 5
 // WEBASSEMBLY-NEXT:#define __FLT16_DENORM_MIN__ 5.9604644775390625e-8F16
 // WEBASSEMBLY-NEXT:#define __FLT16_DIG__ 3
Index: lib/Basic/Targets/OSTargets.h
===
--- lib/Basic/Targets/OSTargets.h
+++ lib/Basic/Targets/OSTargets.h
@@ -771,6 +771,8 @@
 // Follow g++ convention and predefine _GNU_SOURCE for C++.
 if (Opts.CPlusPlus)
   Builder.defineMacro("_GNU_SOURCE");
+// Indicate that we have __float128.
+Builder.defineMacro("__FLOAT128__");
   }
 
 public:
@@ -779,6 +781,7 @@
   : OSTargetInfo(Triple, Opts) {
 this->MCountName = "__mcount";
 this->TheCXXABI.set(TargetCXXABI::WebAssembly);
+this->HasFloat128 = true;
   }
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56561: [Preprocessor] For missing file in framework add note about framework location.

[Jan Korous via Phabricator via cfe-commits]

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

LGTM with nits about doxygen annotations.




Comment at: clang/include/clang/Lex/DirectoryLookup.h:175
+  /// \param [out] IsFrameworkFound For a framework directory set to true if
+  /// specified '.framework' directory is found.
+  ///

We might make this easier to understand by explaining where/how the 
'.framework' directory is specified.



Comment at: clang/include/clang/Lex/HeaderSearch.h:396
+  /// \param IsFrameworkFound If non-null, will be set to true for a framework
+  /// include and if corresponding '.framework' directory was found. Doesn't
+  /// guarantee the requested file is found.

We might try to explain what exactly is meant by corresponding .framework 
directory.


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

https://reviews.llvm.org/D56561



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

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D56411#1365745 , @rjmccall wrote:

> In D56411#1365727 , @yaxunl wrote:
>
> > In D56411#1360010 , @rjmccall 
> > wrote:
> >
> > > I think the diagnostic should come during instantiation when you find an 
> > > evaluated use of a host function within a device function.
> >
> >
> > It seems the body of function template is checked only during parsing of 
> > the definition of the template itself. When a function
> >  template is instantiated, the body of the instantiated function is not 
> > checked again.
>
>
> No, that's not correct.  However, it's checked somewhat differently, and it's 
> possible that the existing diagnostic is not set up to fire along all common 
> paths.  Try moving the diagnostic to `MarkFunctionReferenced`, and note that 
> `OdrUse` will be `false` in all the unevaluated contexts.


I got regression in the folowing test when checking CheckCUDACall in 
MarkFunctionReferenced:

  typedef struct {
template  void *foo() { return 0; }
  
void foo() {
  foo<0>();
}
  } A;

Basically clang does not allow getting linkage of foo<0> before 
ActOnTypedefDeclarator, quoting SemaDecl.cpp line 4171

  // If we've already computed linkage for the anonymous tag, then
  // adding a typedef name for the anonymous decl can change that
  // linkage, which might be a serious problem.  Diagnose this as
  // unsupported and ignore the typedef name.  TODO: we should
  // pursue this as a language defect and establish a formal rule
  // for how to handle it.
  if (TagFromDeclSpec->hasLinkageBeenComputed()) {
Diag(NewTD->getLocation(), diag::err_typedef_changes_linkage);

However, CheckCUDACall needs to call GetGVALinkageForFunction on the callee to 
know if it will be emitted,
which causes the linkage of the anonymous struct to be cached and triggers 
err_typedef_changes_linkage.


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

https://reviews.llvm.org/D56411



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

[PATCH] D57057: [clangd] Log clang-tidy configuration, NFC

[Jan Korous via Phabricator via cfe-commits]

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

LGTM


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57057



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