[PATCH] D61804: Support building shared and static libraries simultaneously

[Wink Saville via Phabricator via cfe-commits]

winksaville added a comment.

Sorry, the two previous comments were meant for D61909 
 and I've moved them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61804



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

[PATCH] D61909: Add Clang shared library with C++ exports

[Wink Saville via Phabricator via cfe-commits]

winksaville added a comment.

Questions:

- Should we only build `libclang_shared.so` if `LLVM_BUILD_LLVM_DYLIB` is ON?
- Should we use link clang-9 to libclang_shared.so when `LLVM_LINK_LLVM_DYLIB` 
is ON?
- Or maybe we should define `BUILD_CLANG_DYLIB` and `LINK_CLANG_DYLIB` or ... ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909



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

[PATCH] D61909: Add Clang shared library with C++ exports

[Wink Saville via Phabricator via cfe-commits]

winksaville added a comment.

I did some quick testing.

I used cmake and ninja to build `llvm` and enabled `clang;lld;compiler-rt` 
subprojects:

  $ cd build-beanz-clang-shlib-2-add-debug-BUILD-LINK_DYNLIB-ON
  $ cmake ../llvm -G Ninja '-DLLVM_ENABLE_PROJECTS=clang;lld;compiler-rt' 
-DCMAKE_INSTALL_PREFIX=/home/wink/local-beanz-clang-shlib-2-add-debug-BUILD-LINK_DYNLIB-ON
 -DCMAKE_BUILD_TYPE=Release -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON
  $ ninja

I then used `bin/clang++` to create `bin/clang-shared` and verified it ran and 
looked at the sizes; `bin/clang-shared` is 187K while `bin/clang-9` is 45M

  $ bin/clang++  -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 
-Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
-Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough 
-Wno-maybe-uninitialized -Wno-class-memaccess -Wno-noexcept-type 
-Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections 
-fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 
-DNDEBUG  -Wl,-allow-shlib-undefined   -Wl,--export-dynamic  
-Wl,-rpath-link,/home/wink/prgs/llvm/llvm-project/build-beanz-clang-shlib-2-add-debug-BUILD-LINK_DYNLIB-ON/./lib
  -Wl,-O3 tools/clang/tools/driver/CMakeFiles/clang.dir/driver.cpp.o 
tools/clang/tools/driver/CMakeFiles/clang.dir/cc1_main.cpp.o 
tools/clang/tools/driver/CMakeFiles/clang.dir/cc1as_main.cpp.o 
tools/clang/tools/driver/CMakeFiles/clang.dir/cc1gen_reproducer_main.cpp.o  -o 
bin/clang-shared  -Wl,-rpath,"\$ORIGIN/../lib" -lpthread lib/libclang_shared.so 
lib/libLLVM-9svn.so 
  $ ./bin/clang-shared --version
  clang version 9.0.0 (g...@github.com:winksaville/llvm-project 
2e41f82e327c9aace5b1367995357a787be77105)
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: 
/home/wink/prgs/llvm/llvm-project/build-beanz-clang-shlib-2-add-debug-BUILD-LINK_DYNLIB-ON/./bin
  $ ls -hal bin/clang-9 bin/clang-shared
  -rwxr-xr-x 1 wink users  45M May 14 17:57 bin/clang-9
  -rwxr-xr-x 1 wink users 187K May 14 22:15 bin/clang-shared

And I'd previously built `clang-9` with LLVM_BUILD_LLVM_DYLIB=OFF and it is 
114M:

  $ ls -hal ../build-beanz-clang-shlib-2-add-debug-DYNLIB-OFF/bin/clang-9
  -rwxr-xr-x 1 wink users 114M May 14 11:31 
../build-beanz-clang-shlib-2-add-debug-DYNLIB-OFF/bin/clang-9

I then compilef hi.c with `clang-shared` and ran the result:

  $ cat ../hi.c
  #include 
  
  int main(void) {
printf("hi\n");
return 0;
  }
  $ ./bin/clang-shared ../hi.c -o hi
  $ ./hi
  hi

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909



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

[PATCH] D61804: Support building shared and static libraries simultaneously

[Wink Saville via Phabricator via cfe-commits]

winksaville added a comment.

Questions:

- Should we only build `libclang_shared.so` if `LLVM_BUILD_LLVM_DYLIB` is ON?
- Should we use link clang-9 to libclang_shared.so when `LLVM_LINK_LLVM_DYLIB` 
is ON?
- Or maybe we should define `BUILD_CLANG_DYLIB` and `LINK_CLANG_DYLIB` or ... ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61804



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

[PATCH] D61804: Support building shared and static libraries simultaneously

[Wink Saville via Phabricator via cfe-commits]

winksaville added a comment.

I did some quick testing.

I used cmake and ninja to build `llvm` and enabled `clang;lld;compiler-rt` 
subprojects:

  $ cd build-beanz-clang-shlib-2-add-debug-BUILD-LINK_DYNLIB-ON
  $ cmake ../llvm -G Ninja '-DLLVM_ENABLE_PROJECTS=clang;lld;compiler-rt' 
-DCMAKE_INSTALL_PREFIX=/home/wink/local-beanz-clang-shlib-2-add-debug-BUILD-LINK_DYNLIB-ON
 -DCMAKE_BUILD_TYPE=Release -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON
  $ ninja

I then used `bin/clang++` to create `bin/clang-shared` and verified it ran and 
looked at the sizes; `bin/clang-shared` is 187K while `bin/clang-9` is 45M

  $ bin/clang++  -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 
-Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
-Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough 
-Wno-maybe-uninitialized -Wno-class-memaccess -Wno-noexcept-type 
-Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections 
-fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 
-DNDEBUG  -Wl,-allow-shlib-undefined   -Wl,--export-dynamic  
-Wl,-rpath-link,/home/wink/prgs/llvm/llvm-project/build-beanz-clang-shlib-2-add-debug-BUILD-LINK_DYNLIB-ON/./lib
  -Wl,-O3 tools/clang/tools/driver/CMakeFiles/clang.dir/driver.cpp.o 
tools/clang/tools/driver/CMakeFiles/clang.dir/cc1_main.cpp.o 
tools/clang/tools/driver/CMakeFiles/clang.dir/cc1as_main.cpp.o 
tools/clang/tools/driver/CMakeFiles/clang.dir/cc1gen_reproducer_main.cpp.o  -o 
bin/clang-shared  -Wl,-rpath,"\$ORIGIN/../lib" -lpthread lib/libclang_shared.so 
lib/libLLVM-9svn.so 
  $ ./bin/clang-shared --version
  clang version 9.0.0 (g...@github.com:winksaville/llvm-project 
2e41f82e327c9aace5b1367995357a787be77105)
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: 
/home/wink/prgs/llvm/llvm-project/build-beanz-clang-shlib-2-add-debug-BUILD-LINK_DYNLIB-ON/./bin
  $ ls -hal bin/clang-9 bin/clang-shared
  -rwxr-xr-x 1 wink users  45M May 14 17:57 bin/clang-9
  -rwxr-xr-x 1 wink users 187K May 14 22:15 bin/clang-shared

And I'd previously built `clang-9` with LLVM_BUILD_LLVM_DYLIB=OFF and it is 
114M:

  $ ls -hal ../build-beanz-clang-shlib-2-add-debug-DYNLIB-OFF/bin/clang-9
  -rwxr-xr-x 1 wink users 114M May 14 11:31 
../build-beanz-clang-shlib-2-add-debug-DYNLIB-OFF/bin/clang-9

I then compilef hi.c with `clang-shared` and ran the result:

  $ cat ../hi.c
  #include 
  
  int main(void) {
printf("hi\n");
return 0;
  }
  $ ./bin/clang-shared ../hi.c -o hi
  $ ./hi
  hi

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61804



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

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:346
+  std::string CheckName = CTContext->getCheckName(Info.getID());
+  if (!CheckName.empty() && WarningAsErrorFilter->contains(CheckName)) {
+Level = DiagnosticsEngine::Error;

ilya-biryukov wrote:
> Why can't we call `CTContext->treatAsError()` here?
Good point, that makes this much easier. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61841



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

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 199552.
nridge marked 2 inline comments as done.
nridge added a comment.

Address review comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61841

Files:
  clang-tools-extra/clangd/ClangdUnit.cpp


Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -304,6 +304,21 @@
 this->CTContext = CTContext;
   }
 
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const clang::Diagnostic ) override {
+DiagnosticsEngine::Level Level = DiagLevel;
+
+// Check for warning-as-error.
+if (CTContext && DiagLevel == DiagnosticsEngine::Warning) {
+  std::string CheckName = CTContext->getCheckName(Info.getID());
+  if (!CheckName.empty() && CTContext->treatAsError(CheckName)) {
+Level = DiagnosticsEngine::Error;
+  }
+}
+
+StoreDiags::HandleDiagnostic(Level, Info);
+  }
+
 private:
   tidy::ClangTidyContext *CTContext = nullptr;
 };


Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -304,6 +304,21 @@
 this->CTContext = CTContext;
   }
 
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const clang::Diagnostic ) override {
+DiagnosticsEngine::Level Level = DiagLevel;
+
+// Check for warning-as-error.
+if (CTContext && DiagLevel == DiagnosticsEngine::Warning) {
+  std::string CheckName = CTContext->getCheckName(Info.getID());
+  if (!CheckName.empty() && CTContext->treatAsError(CheckName)) {
+Level = DiagnosticsEngine::Error;
+  }
+}
+
+StoreDiags::HandleDiagnostic(Level, Info);
+  }
+
 private:
   tidy::ClangTidyContext *CTContext = nullptr;
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:243
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:

sammccall wrote:
> Thanks, this is much cleaner.
> 
> I think we don't need the subclass at all though - just a filter function 
> here, and call setFilter after setting up clang-tidy?
I rely on the derived class more in the follow-up patch D61841 where I override 
`HandleDiagnostic()`. Should I still remove it from this patch?



Comment at: clang-tools-extra/clangd/Diagnostics.cpp:309
 
   if (!LangOpts || !Info.hasSourceManager()) {
 IgnoreDiagnostics::log(DiagLevel, Info);

sammccall wrote:
> do we need to set LastErrorWasIgnored here (in the case that it's not a note)?
> 
> I guess it's pretty unlikely that e.g. a problem with the command-line will 
> get a note that has a source location...
If that does happen, then it looks like storing the note in that case is a 
pre-existing behaviour, which this patch just preserves.



Comment at: clang-tools-extra/clangd/Diagnostics.h:132
   llvm::Optional LastDiag;
+  bool LastErrorWasIgnored = false;
 };

sammccall wrote:
> I think we're talking about last non-note diagnostic, right? Warnings count 
> here I think.
> 
> At the risk of being verbose, maybe `LastPrimaryDiagnosticWasIgnored`?
I copied the name from `ClangTidyDiagnosticConsumer::LastErrorWasIgnored`, but 
sure, I can rename this as suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60953



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

[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 199551.
nridge marked 9 inline comments as done.
nridge added a comment.

Address review comments, except for the derived class one, about which I have a 
question


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60953

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -767,6 +767,24 @@
  "a type specifier for all declarations"),
   WithNote(Diag(Header.range(), "error occurred here");
 }
+
+TEST(ClangTidy, SuppressionComment) {
+  Annotations Main(R"cpp(
+int main() {
+  int i = 3;
+  double d = 8 / i;  // NOLINT
+  // NOLINTNEXTLINE
+  double e = 8 / i;
+  double f = [[8]] / i;
+}
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  UnorderedElementsAre(Diag(
+  Main.range(), "result of integer division used in a floating "
+"point context; possible loss of precision")));
+}
 } // namespace
 
 } // namespace clangd
Index: clang-tools-extra/clangd/Diagnostics.h
===
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -112,6 +112,9 @@
 /// Convert from clang diagnostic level to LSP severity.
 int getSeverity(DiagnosticsEngine::Level L);
 
+/// Check if a diagnostic is inside the main file.
+bool isInsideMainFile(const clang::Diagnostic );
+
 /// StoreDiags collects the diagnostics that can later be reported by
 /// clangd. It groups all notes for a diagnostic into a single Diag
 /// and filters out diagnostics that don't mention the main file (i.e. neither
@@ -128,17 +131,25 @@
 
   using DiagFixer = std::function(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
+  using DiagFilter =
+  std::function;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
+  /// If set, ignore diagnostics for which \p SuppressionFilter returns true.
+  void suppressDiagnostics(DiagFilter SuppressionFilter) {
+this->SuppressionFilter = SuppressionFilter;
+  }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
+  DiagFilter SuppressionFilter = nullptr;
   std::vector Output;
   llvm::Optional LangOpts;
   llvm::Optional LastDiag;
   llvm::DenseSet IncludeLinesWithErrors;
+  bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 } // namespace clangd
Index: clang-tools-extra/clangd/Diagnostics.cpp
===
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -146,6 +146,8 @@
   return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc));
 }
 
+} // namespace
+
 bool isInsideMainFile(const clang::Diagnostic ) {
   if (!D.hasSourceManager())
 return false;
@@ -153,6 +155,8 @@
   return isInsideMainFile(D.getLocation(), D.getSourceManager());
 }
 
+namespace {
+
 bool isNote(DiagnosticsEngine::Level L) {
   return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
 }
@@ -514,6 +518,12 @@
 // Handle the new main diagnostic.
 flushLastDiag();
 
+if (SuppressionFilter && SuppressionFilter(DiagLevel, Info)) {
+  LastPrimaryDiagnosticWasSuppressed = true;
+  return;
+}
+LastPrimaryDiagnosticWasSuppressed = false;
+
 LastDiag = Diag();
 LastDiag->ID = Info.getID();
 FillDiagBase(*LastDiag);
@@ -528,6 +538,13 @@
 }
   } else {
 // Handle a note to an existing diagnostic.
+
+// If a diagnostic was suppressed due to the suppression filter,
+// also suppress notes associated with it.
+if (LastPrimaryDiagnosticWasSuppressed) {
+  return;
+}
+
 if (!LastDiag) {
   assert(false && "Adding a note without main diagnostic");
   IgnoreDiagnostics::log(DiagLevel, Info);
Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -113,12 +113,12 @@
   }
 
   void EndOfMainFile() {
-for (const auto& Entry : MainFileMacros)
+for (const auto  : MainFileMacros)
   

[PATCH] D61923: [GWP-ASan] Mutex implementation [2].

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D61923#1502323 , @hctim wrote:

> In D61923#1502245 , @jfb wrote:
>
> > Seems a shame to duplicate mutex again... Why can't use use the STL's 
> > version again? It doesn't allocate.
>
>
> We can't use `std::mutex` as we must be C-compatible


Can you clarify what you mean by this? I don't understand.

> (and can't make the strong guarantee that `std::mutex` is header-only), see 
> https://reviews.llvm.org/D60593#inline-537276.

Have you asked on libcxx-dev? Is there interest in the libc++ community for a 
stand-alone base?

> We also are trying to stay independent of `pthread` for 
> platform-independentness.

The code I see is clearly not platform independent. Please clarify your 
reasoning. I can understand if you don't want pthread for a valid reason, but 
"platform-independence" doesn't make sense here.

> We also can't use a `sanitizer_common` variant as we don't want to pull in 
> the entirety `sanitizer_common` as a dependency.

What's the restriction that makes it unacceptable?

> Basically, the roadmap is to create a shared mutex library for 
> `sanitizer_common`, `gwp_asan` and `scudo` to all use.

I'm asking all these question because they should be in the commit message, and 
would hopefully have surfaced from the RFC about GWP / Scudo. If those weren't 
in the RFC that's fine, they should still be in the commit message and you'll 
want to update the RFC with "while reviewing code we realized *stuff*".

In D61923#1502337 , @eugenis wrote:

> I think the idea is that implementing our own spinlock is not much harder 
> than having 3 platform-specific implementations (posix, window, fuchsia).
>  Also, scudo_standalone is doing the same (  @cryptoad, why? ).


Your concerns are backwards. Implementation is a one-time thing, maintenance is 
an ongoing concern and it way overshadows implementation concerns. In 
particular, a variety of multicore code that does its own locking is much 
harder to tune at the system-wide level compared to a single thing that's tuned 
for each application. I'm not saying a separate mutex doesn't make sense, what 
I'm saying is that experience shows your above reasoning to be generally 
invalid. I'm totally willing to believe that there's a very good reason to have 
your own mutex here, but you gotta explain it.




Comment at: compiler-rt/lib/gwp_asan/definitions.h:14
+# undef ALWAYS_INLINE
+#endif // defined(ALWAYS_INLINE)
+#define ALWAYS_INLINE inline __attribute__((always_inline))

The undef above doesn't seem like a good idea. You should prefix the macro with 
`GWP_` if you're concerned about name clashes.



Comment at: compiler-rt/lib/gwp_asan/mutex.h:42
+  __atomic_is_lock_free(sizeof(Locked), /*Typical Alignment*/ 0),
+  "gwp_asan::Mutex::Locked is not lock-free on this architecture.");
+};

You want `__atomic_always_lock_free`, it'll always be a `constexpr` (whereas 
the other can be a libcall).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61923



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

[PATCH] D57858: [analyzer] Add a new frontend flag to display all checker options

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

http://lists.llvm.org/pipermail/cfe-dev/2019-May/062298.html


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

https://reviews.llvm.org/D57858



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

r360739 - [analyzer] MIGChecker: Fix redundant semicolon.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Tue May 14 18:36:41 2019
New Revision: 360739

URL: http://llvm.org/viewvc/llvm-project?rev=360739=rev
Log:
[analyzer] MIGChecker: Fix redundant semicolon.

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=360739=360738=360739=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Tue May 14 18:36:41 
2019
@@ -112,7 +112,7 @@ public:
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
 // A set of parameters for which the check is suppressed because
 // reference counting is being performed.
-REGISTER_SET_WITH_PROGRAMSTATE(RefCountedParameters, const ParmVarDecl *);
+REGISTER_SET_WITH_PROGRAMSTATE(RefCountedParameters, const ParmVarDecl *)
 
 static const ParmVarDecl *getOriginParam(SVal V, CheckerContext ,
  bool IncludeBaseRegions = false) {


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

[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

[Mitch Phillips via Phabricator via cfe-commits]

hctim added a comment.

In D60593#1502266 , @jfb wrote:

> Hello I'm here to bikeshed, and this is a terrible name for an LLVM project. 
> Or super appropriate because LLVM itself is a terrible name? In any case, 
> backronym it to something else, or rename it. Thanks!


Also see `GwpAsan.rst`:

> It informally is a recursive acronym, "**G**WP-ASan **W**ill **P**rovide 
> **A**llocation **SAN**ity".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593



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

Re: r360403 - Change -gz and -Wa, --compress-debug-sections to use gABI compression (SHF_COMPRESSED)

[Fāng-ruì Sòng via cfe-commits]

It's fine :) The problem may be that users with older pre-2.26 binutils may
have older GNU as as well. Their GNU as may not recognize
--compress-debug-sections=zlib.

On Wed, May 15, 2019 at 3:39 AM Eric Christopher  wrote:

> Hi Ray,
>
> I've temporarily reverted this here:
>
> echristo@jhereg ~/s/llvm-project> git llvm push
> Pushing 1 commit:
>   fda79815a33 Temporarily revert "Change -gz and
> -Wa,--compress-debug-sections to use gABI compression
> (SHF_COMPRESSED)"
> Sendingcfe/trunk/docs/ReleaseNotes.rst
> Sendingcfe/trunk/lib/Frontend/CompilerInvocation.cpp
> Sendingcfe/trunk/tools/driver/cc1as_main.cpp
> Transmitting file data ...done
> Committing transaction...
> Committed revision 360703.
> Committed fda79815a33 to svn.
>
> From my message:
>
> This affects users of older (pre 2.26) binutils in such a way that
> they can't necessarily
> work around it as it doesn't support the compress option on the
> command line. Reverting
> to unblock them and we can revisit whether to make this change now
> or fix how we want
> to express the option.
>
> I'm not sure what we want to do here, but wanted to unblock people in
> the meantime. What do you think?
>
> Thanks!
>
> -eric
>
>
> On Thu, May 9, 2019 at 7:05 PM Fangrui Song via cfe-commits
>  wrote:
> >
> > Author: maskray
> > Date: Thu May  9 19:08:21 2019
> > New Revision: 360403
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=360403=rev
> > Log:
> > Change -gz and -Wa,--compress-debug-sections to use gABI compression
> (SHF_COMPRESSED)
> >
> > Since July 15, 2015 (binutils-gdb commit
> > 19a7fe52ae3d0971e67a134bcb1648899e21ae1c, included in 2.26), gas
> > --compress-debug-sections=zlib (gcc -gz) means zlib-gabi:
> > SHF_COMPRESSED. Before that it meant zlib-gnu (.zdebug).
> >
> > clang's -gz was introduced in rC306115 (Jun 2017) to indicate zlib-gnu.
> It
> > is 2019 now and it is not unreasonable to assume users of the new
> > feature to have new linkers (ld.bfd/gold >= 2.26, lld >= rLLD273661).
> >
> > Change clang's default accordingly to improve standard conformance.
> > zlib-gnu becomes out of fashion and gets poorer toolchain support.
> > Its mangled names confuse tools and are more likely to cause problems.
> >
> > Reviewed By: compnerd
> >
> > Differential Revision: https://reviews.llvm.org/D61689
> >
> > Modified:
> > cfe/trunk/docs/ReleaseNotes.rst
> > cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> > cfe/trunk/tools/driver/cc1as_main.cpp
> >
> > Modified: cfe/trunk/docs/ReleaseNotes.rst
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=360403=360402=360403=diff
> >
> ==
> > --- cfe/trunk/docs/ReleaseNotes.rst (original)
> > +++ cfe/trunk/docs/ReleaseNotes.rst Thu May  9 19:08:21 2019
> > @@ -77,7 +77,10 @@ Modified Compiler Flags
> >
> >  - `clang -dumpversion` now returns the version of Clang itself.
> >
> > -- ...
> > +- On ELF, ``-gz`` now defaults to ``-gz=zlib``. It produces
> ``SHF_COMPRESSED``
> > +  style compression of debug information. GNU binutils 2.26 or newer,
> or lld is
> > +  required to link produced object files. Use ``-gz=zlib-gnu`` to get
> the old
> > +  behavior.
> >
> >  New Pragmas in Clang
> >  
> >
> > Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=360403=360402=360403=diff
> >
> ==
> > --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
> > +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Thu May  9 19:08:21
> 2019
> > @@ -1052,8 +1052,7 @@ static bool ParseCodeGenArgs(CodeGenOpti
> >if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
> >   OPT_compress_debug_sections_EQ)) {
> >  if (A->getOption().getID() == OPT_compress_debug_sections) {
> > -  // TODO: be more clever about the compression type auto-detection
> > -  Opts.setCompressDebugSections(llvm::DebugCompressionType::GNU);
> > +  Opts.setCompressDebugSections(llvm::DebugCompressionType::Z);
> >  } else {
> >auto DCT =
> llvm::StringSwitch(A->getValue())
> >   .Case("none", llvm::DebugCompressionType::None)
> >
> > Modified: cfe/trunk/tools/driver/cc1as_main.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/driver/cc1as_main.cpp?rev=360403=360402=360403=diff
> >
> ==
> > --- cfe/trunk/tools/driver/cc1as_main.cpp (original)
> > +++ cfe/trunk/tools/driver/cc1as_main.cpp Thu May  9 19:08:21 2019
> > @@ -221,8 +221,7 @@ bool AssemblerInvocation::CreateFromArgs
> >if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
> >   

[PATCH] D61925: [analyzer] MIGChecker: Add support for os_ref_retain().

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC360737: [analyzer] MIGChecker: Add support for 
os_ref_retain(). (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61925?vs=199537=199543#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D61925

Files:
  lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  test/Analysis/mig.mm

Index: test/Analysis/mig.mm
===
--- test/Analysis/mig.mm
+++ test/Analysis/mig.mm
@@ -19,11 +19,24 @@
 typedef unsigned mach_port_t;
 typedef uint32_t UInt32;
 
+struct os_refcnt {};
+typedef struct os_refcnt os_refcnt_t;
+
+struct thread {
+  os_refcnt_t ref_count;
+};
+typedef struct thread *thread_t;
+
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 kern_return_t mach_vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 void mig_deallocate(vm_address_t, vm_size_t);
 kern_return_t mach_port_deallocate(ipc_space_t, mach_port_name_t);
 void ipc_port_release(ipc_port_t);
+void thread_deallocate(thread_t);
+
+static void os_ref_retain(struct os_refcnt *rc);
+
+#define thread_reference_internal(thread) os_ref_retain(&(thread)->ref_count);
 
 #define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
 
@@ -237,3 +250,10 @@
// expected-note@-1{{MIG callback fails with error after deallocating argument value}}
   }
 };
+
+MIG_SERVER_ROUTINE
+kern_return_t test_os_ref_retain(thread_t thread) {
+  thread_reference_internal(thread);
+  thread_deallocate(thread);
+  return KERN_ERROR; // no-warning
+}
Index: lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -84,6 +84,8 @@
 #undef CALL
   };
 
+  CallDescription OsRefRetain{"os_ref_retain", 1};
+
   void checkReturnAux(const ReturnStmt *RS, CheckerContext ) const;
 
 public:
@@ -105,10 +107,17 @@
 };
 } // end anonymous namespace
 
+// A flag that says that the programmer has called a MIG destructor
+// for at least one parameter.
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
-
-static const ParmVarDecl *getOriginParam(SVal V, CheckerContext ) {
-  SymbolRef Sym = V.getAsSymbol();
+// A set of parameters for which the check is suppressed because
+// reference counting is being performed.
+REGISTER_SET_WITH_PROGRAMSTATE(RefCountedParameters, const ParmVarDecl *);
+
+static const ParmVarDecl *getOriginParam(SVal V, CheckerContext ,
+ bool IncludeBaseRegions = false) {
+  // TODO: We should most likely always include base regions here.
+  SymbolRef Sym = V.getAsSymbol(IncludeBaseRegions);
   if (!Sym)
 return nullptr;
 
@@ -168,6 +177,19 @@
 }
 
 void MIGChecker::checkPostCall(const CallEvent , CheckerContext ) const {
+  if (Call.isCalled(OsRefRetain)) {
+// If the code is doing reference counting over the parameter,
+// it opens up an opportunity for safely calling a destructor function.
+// TODO: We should still check for over-releases.
+if (const ParmVarDecl *PVD =
+getOriginParam(Call.getArgSVal(0), C, /*IncludeBaseRegions=*/true)) {
+  // We never need to clean up the program state because these are
+  // top-level parameters anyway, so they're always live.
+  C.addTransition(C.getState()->add(PVD));
+}
+return;
+  }
+
   if (!isInMIGCall(C))
 return;
 
@@ -178,10 +200,11 @@
   if (I == Deallocators.end())
 return;
 
+  ProgramStateRef State = C.getState();
   unsigned ArgIdx = I->second;
   SVal Arg = Call.getArgSVal(ArgIdx);
   const ParmVarDecl *PVD = getOriginParam(Arg, C);
-  if (!PVD)
+  if (!PVD || State->contains(PVD))
 return;
 
   const NoteTag *T = C.getNoteTag([this, PVD](BugReport ) -> std::string {
@@ -193,7 +216,7 @@
<< "\' is deallocated";
 return OS.str();
   });
-  C.addTransition(C.getState()->set(true), T);
+  C.addTransition(State->set(true), T);
 }
 
 // Returns true if V can potentially represent a "successful" kern_return_t.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61923: [GWP-ASan] Mutex implementation [2].

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I think the idea is that implementing our own spinlock is not much harder than 
having 3 platform-specific implementations (posix, window, fuchsia).
Also, scudo_standalone is doing the same (  @cryptoad, why? ).

As Mitch mentioned, we should move the implementation into a common directory. 
Why not do this now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61923



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

r360737 - [analyzer] MIGChecker: Add support for os_ref_retain().

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Tue May 14 18:19:19 2019
New Revision: 360737

URL: http://llvm.org/viewvc/llvm-project?rev=360737=rev
Log:
[analyzer] MIGChecker: Add support for os_ref_retain().

Suppress MIG checker false positives that occur when the programmer increments
the reference count before calling a MIG destructor, and the MIG destructor
literally boils down to decrementing the reference count.

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
cfe/trunk/test/Analysis/mig.mm

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=360737=360736=360737=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Tue May 14 18:19:19 
2019
@@ -84,6 +84,8 @@ class MIGChecker : public Checkeradd(PVD));
+}
+return;
+  }
+
   if (!isInMIGCall(C))
 return;
 
@@ -178,10 +200,11 @@ void MIGChecker::checkPostCall(const Cal
   if (I == Deallocators.end())
 return;
 
+  ProgramStateRef State = C.getState();
   unsigned ArgIdx = I->second;
   SVal Arg = Call.getArgSVal(ArgIdx);
   const ParmVarDecl *PVD = getOriginParam(Arg, C);
-  if (!PVD)
+  if (!PVD || State->contains(PVD))
 return;
 
   const NoteTag *T = C.getNoteTag([this, PVD](BugReport ) -> std::string {
@@ -193,7 +216,7 @@ void MIGChecker::checkPostCall(const Cal
<< "\' is deallocated";
 return OS.str();
   });
-  C.addTransition(C.getState()->set(true), T);
+  C.addTransition(State->set(true), T);
 }
 
 // Returns true if V can potentially represent a "successful" kern_return_t.

Modified: cfe/trunk/test/Analysis/mig.mm
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/mig.mm?rev=360737=360736=360737=diff
==
--- cfe/trunk/test/Analysis/mig.mm (original)
+++ cfe/trunk/test/Analysis/mig.mm Tue May 14 18:19:19 2019
@@ -19,11 +19,24 @@ typedef struct ipc_port *ipc_port_t;
 typedef unsigned mach_port_t;
 typedef uint32_t UInt32;
 
+struct os_refcnt {};
+typedef struct os_refcnt os_refcnt_t;
+
+struct thread {
+  os_refcnt_t ref_count;
+};
+typedef struct thread *thread_t;
+
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 kern_return_t mach_vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 void mig_deallocate(vm_address_t, vm_size_t);
 kern_return_t mach_port_deallocate(ipc_space_t, mach_port_name_t);
 void ipc_port_release(ipc_port_t);
+void thread_deallocate(thread_t);
+
+static void os_ref_retain(struct os_refcnt *rc);
+
+#define thread_reference_internal(thread) os_ref_retain(&(thread)->ref_count);
 
 #define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
 
@@ -237,3 +250,10 @@ class MyClient: public IOUserClient {
// expected-note@-1{{MIG callback fails with error 
after deallocating argument value}}
   }
 };
+
+MIG_SERVER_ROUTINE
+kern_return_t test_os_ref_retain(thread_t thread) {
+  thread_reference_internal(thread);
+  thread_deallocate(thread);
+  return KERN_ERROR; // no-warning
+}


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

[PATCH] D61923: [GWP-ASan] Mutex implementation [2].

[Mitch Phillips via Phabricator via cfe-commits]

hctim added a comment.

In D61923#1502245 , @jfb wrote:

> Seems a shame to duplicate mutex again... Why can't use use the STL's version 
> again? It doesn't allocate.


We can't use `std::mutex` as we must be C-compatible (and can't make the strong 
guarantee that `std::mutex` is header-only), see 
https://reviews.llvm.org/D60593#inline-537276.
We also are trying to stay independent of `pthread` for 
platform-independentness.
We also can't use a `sanitizer_common` variant as we don't want to pull in the 
entirety `sanitizer_common` as a dependency.

Basically, the roadmap is to create a shared mutex library for 
`sanitizer_common`, `gwp_asan` and `scudo` to all use.

In D61923#1502245 , @jfb wrote:

> Tests?


Added some unit tests.




Comment at: compiler-rt/lib/gwp_asan/mutex.h:32
+private:
+  void yieldProcessor(uint8_t Count);
+  void yieldPlatform();

jfb wrote:
> `uint8_t` seems easy to inadvertently truncate. If you want to restrict size, 
> make it a template and refuse to compile above a certain size. It's always 
> `10` anyways.
Seeming as this is an entirely-internal number, I've made it a global instead.



Comment at: compiler-rt/lib/gwp_asan/mutex.h:81
+else
+  yieldPlatform();
+

jfb wrote:
> This won't compile if `__unix__` isn't defined.
Should be guarded by `if (COMPILER_RT_HAS_GWP_ASAN)` at 
`lib/gwp_asan/CMakeLists.txt:20`, but have added explicit error condition here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61923



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

[PATCH] D61923: [GWP-ASan] Mutex implementation [2].

[Mitch Phillips via Phabricator via cfe-commits]

hctim updated this revision to Diff 199541.
hctim marked 10 inline comments as done.
hctim added a comment.
Herald added subscribers: cfe-commits, srhines.
Herald added a reviewer: jfb.
Herald added a project: clang.

- Working copy of mutex.
- >>! In D61923#1502245 , @jfb wrote:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61923

Files:
  clang/runtime/CMakeLists.txt
  compiler-rt/lib/gwp_asan/CMakeLists.txt
  compiler-rt/lib/gwp_asan/definitions.h
  compiler-rt/lib/gwp_asan/mutex.h
  compiler-rt/lib/gwp_asan/tests/CMakeLists.txt
  compiler-rt/lib/gwp_asan/tests/driver.cpp
  compiler-rt/lib/gwp_asan/tests/mutex_test.cpp
  compiler-rt/test/gwp_asan/CMakeLists.txt
  compiler-rt/test/gwp_asan/dummy_test.cc
  compiler-rt/test/gwp_asan/lit.cfg
  compiler-rt/test/gwp_asan/lit.site.cfg.in
  compiler-rt/test/gwp_asan/unit/lit.site.cfg.in

Index: compiler-rt/test/gwp_asan/unit/lit.site.cfg.in
===
--- /dev/null
+++ compiler-rt/test/gwp_asan/unit/lit.site.cfg.in
@@ -0,0 +1,9 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+config.name = "GwpAsan-Unittest"
+# Load common config for all compiler-rt unit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/unittests/lit.common.unit.configured")
+
+config.test_exec_root = os.path.join("@COMPILER_RT_BINARY_DIR@",
+ "lib", "gwp_asan", "tests")
+config.test_source_root = config.test_exec_root
Index: compiler-rt/test/gwp_asan/lit.site.cfg.in
===
--- /dev/null
+++ compiler-rt/test/gwp_asan/lit.site.cfg.in
@@ -0,0 +1,11 @@
+@LIT_SITE_CFG_IN_HEADER@
+
+config.name_suffix = "@GWP_ASAN_TEST_CONFIG_SUFFIX@"
+config.target_arch = "@GWP_ASAN_TEST_TARGET_ARCH@"
+config.target_cflags = "@GWP_ASAN_TEST_TARGET_CFLAGS@"
+
+# Load common config for all compiler-rt lit tests.
+lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
+
+# Load tool-specific config that would do the real work.
+lit_config.load_config(config, "@GWP_ASAN_LIT_SOURCE_DIR@/lit.cfg")
Index: compiler-rt/test/gwp_asan/lit.cfg
===
--- /dev/null
+++ compiler-rt/test/gwp_asan/lit.cfg
@@ -0,0 +1,31 @@
+# -*- Python -*-
+
+import os
+
+# Setup config name.
+config.name = 'GWP-ASan' + config.name_suffix
+
+# Setup source root.
+config.test_source_root = os.path.dirname(__file__)
+
+# Test suffixes.
+config.suffixes = ['.c', '.cc', '.cpp', '.test']
+
+# C & CXX flags.
+c_flags = ([config.target_cflags])
+
+# Android doesn't want -lrt.
+if not config.android:
+  c_flags += ["-lrt"]
+
+cxx_flags = (c_flags + config.cxx_mode_flags + ["-std=c++11"])
+
+def build_invocation(compile_flags):
+  return " " + " ".join([config.clang] + compile_flags) + " "
+
+# Add substitutions.
+config.substitutions.append(("%clang ", build_invocation(c_flags)))
+
+# GWP-ASan tests are currently supported on Linux only.
+if config.host_os not in ['Linux']:
+   config.unsupported = True
Index: compiler-rt/test/gwp_asan/dummy_test.cc
===
--- /dev/null
+++ compiler-rt/test/gwp_asan/dummy_test.cc
@@ -0,0 +1,4 @@
+// Exists to simply stop warnings about lit not discovering any tests here.
+// RUN: %clang %s
+
+int main() { return 0; }
Index: compiler-rt/test/gwp_asan/CMakeLists.txt
===
--- compiler-rt/test/gwp_asan/CMakeLists.txt
+++ compiler-rt/test/gwp_asan/CMakeLists.txt
@@ -0,0 +1,45 @@
+set(GWP_ASAN_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+set(GWP_ASAN_LIT_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
+
+set(GWP_ASAN_TESTSUITES)
+
+set(GWP_ASAN_UNITTEST_DEPS)
+set(GWP_ASAN_TEST_DEPS
+  ${SANITIZER_COMMON_LIT_TEST_DEPS}
+  gwp_asan)
+
+if (COMPILER_RT_INCLUDE_TESTS)
+  list(APPEND GWP_ASAN_TEST_DEPS GwpAsanUnitTests)
+  configure_lit_site_cfg(
+${CMAKE_CURRENT_SOURCE_DIR}/unit/lit.site.cfg.in
+${CMAKE_CURRENT_BINARY_DIR}/unit/lit.site.cfg)
+  add_lit_testsuite(check-gwp_asan-unit "Running GWP-ASan unit tests"
+${CMAKE_CURRENT_BINARY_DIR}/unit
+DEPENDS ${GWP_ASAN_TEST_DEPS})
+  set_target_properties(check-gwp_asan-unit PROPERTIES FOLDER
+"Compiler-RT Tests")
+list(APPEND GWP_ASAN_TEST_DEPS check-gwp_asan-unit)
+endif()
+
+configure_lit_site_cfg(
+  ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in
+  ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg
+  )
+
+foreach(arch ${GWP_ASAN_SUPPORTED_ARCH})
+  set(GWP_ASAN_TEST_TARGET_ARCH ${arch})
+  string(TOLOWER "-${arch}" GWP_ASAN_TEST_CONFIG_SUFFIX)
+  get_test_cc_for_arch(${arch} GWP_ASAN_TEST_TARGET_CC GWP_ASAN_TEST_TARGET_CFLAGS)
+  string(TOUPPER ${arch} ARCH_UPPER_CASE)
+  set(CONFIG_NAME ${ARCH_UPPER_CASE}${OS_NAME}Config)
+
+  configure_lit_site_cfg(
+

[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Ha, I think it matches LLVM perfectly :)
G, of course, stands for "Galaxy".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593



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

[PATCH] D61925: [analyzer] MIGChecker: Add support for os_ref_retain().

[Devin Coughlin via Phabricator via cfe-commits]

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

LGTM. Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D61925



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

[PATCH] D61925: [analyzer] MIGChecker: Add support for os_ref_retain().

[Artem Dergachev via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:118-119
+static const ParmVarDecl *getOriginParam(SVal V, CheckerContext ,
+ bool IncludeBaseRegions = false) {
+  // TODO: We should most likely always include base regions here.
+  SymbolRef Sym = V.getAsSymbol(IncludeBaseRegions);

This is probably a source of false negatives; i'll investigate it separately.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61925



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

[PATCH] D61925: [analyzer] MIGChecker: Add support for os_ref_retain().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added a reviewer: dcoughlin.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.

Suppress MIG checker false positives that occur when the programmer increments 
the reference count before calling a MIG destructor, and the MIG destructor 
literally boils down to decrementing the reference count.


Repository:
  rC Clang

https://reviews.llvm.org/D61925

Files:
  clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
  clang/test/Analysis/mig.mm

Index: clang/test/Analysis/mig.mm
===
--- clang/test/Analysis/mig.mm
+++ clang/test/Analysis/mig.mm
@@ -19,11 +19,24 @@
 typedef unsigned mach_port_t;
 typedef uint32_t UInt32;
 
+struct os_refcnt {};
+typedef struct os_refcnt os_refcnt_t;
+
+struct thread {
+  os_refcnt_t ref_count;
+};
+typedef struct thread *thread_t;
+
 kern_return_t vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 kern_return_t mach_vm_deallocate(mach_port_name_t, vm_address_t, vm_size_t);
 void mig_deallocate(vm_address_t, vm_size_t);
 kern_return_t mach_port_deallocate(ipc_space_t, mach_port_name_t);
 void ipc_port_release(ipc_port_t);
+void thread_deallocate(thread_t);
+
+static void os_ref_retain(struct os_refcnt *rc);
+
+#define thread_reference_internal(thread) os_ref_retain(&(thread)->ref_count);
 
 #define MIG_SERVER_ROUTINE __attribute__((mig_server_routine))
 
@@ -237,3 +250,10 @@
// expected-note@-1{{MIG callback fails with error after deallocating argument value}}
   }
 };
+
+MIG_SERVER_ROUTINE
+kern_return_t test_os_ref_retain(thread_t thread) {
+  thread_reference_internal(thread);
+  thread_deallocate(thread);
+  return KERN_ERROR; // no-warning
+}
Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
@@ -84,6 +84,8 @@
 #undef CALL
   };
 
+  CallDescription OsRefRetain{"os_ref_retain", 1};
+
   void checkReturnAux(const ReturnStmt *RS, CheckerContext ) const;
 
 public:
@@ -105,10 +107,17 @@
 };
 } // end anonymous namespace
 
+// A flag that says that the programmer has called a MIG destructor
+// for at least one parameter.
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool)
-
-static const ParmVarDecl *getOriginParam(SVal V, CheckerContext ) {
-  SymbolRef Sym = V.getAsSymbol();
+// A set of parameters for which the check is suppressed because
+// reference counting is being performed.
+REGISTER_SET_WITH_PROGRAMSTATE(RefCountedParameters, const ParmVarDecl *);
+
+static const ParmVarDecl *getOriginParam(SVal V, CheckerContext ,
+ bool IncludeBaseRegions = false) {
+  // TODO: We should most likely always include base regions here.
+  SymbolRef Sym = V.getAsSymbol(IncludeBaseRegions);
   if (!Sym)
 return nullptr;
 
@@ -168,6 +177,19 @@
 }
 
 void MIGChecker::checkPostCall(const CallEvent , CheckerContext ) const {
+  if (Call.isCalled(OsRefRetain)) {
+// If the code is doing reference counting over the parameter,
+// it opens up an opportunity for safely calling a destructor function.
+// TODO: We should still check for over-releases.
+if (const ParmVarDecl *PVD =
+getOriginParam(Call.getArgSVal(0), C, /*IncludeBaseRegions=*/true)) {
+  // We never need to clean up the program state because these are
+  // top-level parameters anyway, so they're always live.
+  C.addTransition(C.getState()->add(PVD));
+}
+return;
+  }
+
   if (!isInMIGCall(C))
 return;
 
@@ -178,10 +200,11 @@
   if (I == Deallocators.end())
 return;
 
+  ProgramStateRef State = C.getState();
   unsigned ArgIdx = I->second;
   SVal Arg = Call.getArgSVal(ArgIdx);
   const ParmVarDecl *PVD = getOriginParam(Arg, C);
-  if (!PVD)
+  if (!PVD || State->contains(PVD))
 return;
 
   const NoteTag *T = C.getNoteTag([this, PVD](BugReport ) -> std::string {
@@ -193,7 +216,7 @@
<< "\' is deallocated";
 return OS.str();
   });
-  C.addTransition(C.getState()->set(true), T);
+  C.addTransition(State->set(true), T);
 }
 
 // Returns true if V can potentially represent a "successful" kern_return_t.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D60593#1498622 , @hctim wrote:

> In D60593#1495428 , @gribozavr wrote:
>
> > What does GWP stand for?
>
>
> Google Wide Profiling 
> .
>
> We use the name GWP-ASan, even though it's "technically neither GWP nor 
> ASan", but because it describes well what the outcome is (sampled allocations 
> with memory detection capabilities). It also mirrors the name used for the 
> identical mechanism implemented in Chromium 
> .


Hello I'm here to bikeshed, and this is a terrible name for an LLVM project. Or 
super appropriate because LLVM itself is a terrible name? In any case, 
backronym it to something else, or rename it. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593



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

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 199534.
NoQ marked an inline comment as done.
NoQ added a comment.

> What about "the most derived class"

Sounds great!

> It would be cool to say "by A" or something more simple and precise.

We can't do that in all cases (we don't necessarily see the constructor of the 
most derived class) but in some cases we might be able to do it (when we're 
sure that we actually see it). Added a TODO.


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

https://reviews.llvm.org/D61817

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/diagnostics/initializer.cpp

Index: clang/test/Analysis/diagnostics/initializer.cpp
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/initializer.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core -analyzer-output=text \
+// RUN:   -verify %s
+
+namespace note_on_skipped_vbases {
+struct A {
+  int x;
+  A() : x(0) {} // expected-note{{The value 0 is assigned to 'c.x'}}
+  A(int x) : x(x) {}
+};
+
+struct B : virtual A {
+  int y;
+  // This note appears only once, when this constructor is called from C.
+  // When this constructor is called from D, this note is still correct but
+  // it doesn't appear because it's pruned out because it's irrelevant to the
+  // bug report.
+  B(): // expected-note{{Virtual base initialization skipped because it has already been handled by the most derived class}}
+A(1),
+y(1 / x) // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+  {}
+};
+
+struct C : B {
+  C(): // expected-note{{Calling default constructor for 'A'}}
+   // expected-note@-1{{Returning from default constructor for 'A'}}
+B() // expected-note{{Calling default constructor for 'B'}}
+  {}
+};
+
+void test_note() {
+  C c; // expected-note{{Calling default constructor for 'C'}}
+}
+
+struct D: B {
+  D() : A(1), B() {}
+};
+
+void test_prunability() {
+  D d;
+  1 / 0; // expected-warning{{Division by zero}}
+ // expected-note@-1{{Division by zero}}
+}
+} // namespace note_on_skipped_vbases
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -724,7 +724,15 @@
   const Stmt* S = nullptr;
   if (Optional BE = P.getAs()) {
 const CFGBlock *BSrc = BE->getSrc();
-S = BSrc->getTerminatorCondition();
+if (BSrc->getTerminator().isVirtualBasesBranch()) {
+  // TODO: VirtualBaseBranches should also appear for destructors.
+  // In this case we should put the diagnostic at the end of decl.
+  return PathDiagnosticLocation::createBegin(
+  P.getLocationContext()->getDecl(), SMng);
+
+} else {
+  S = BSrc->getTerminatorCondition();
+}
   } else if (Optional SP = P.getAs()) {
 S = SP->getStmt();
 if (P.getAs())
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -216,6 +216,25 @@
LC->getDecl(),
LC->getCFG()->getNumBlockIDs());
 
+  // Display a prunable path note to the user if it's a virtual bases branch
+  // and we're taking the path that skips virtual base constructors.
+  if (L.getSrc()->getTerminator().isVirtualBasesBranch() &&
+  L.getDst() == *L.getSrc()->succ_begin()) {
+ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(
+[](BugReporterContext &, BugReport &) -> std::string {
+  // TODO: Just call out the name of the most derived class
+  // when we know it.
+  return "Virtual base initialization skipped because "
+ "it has already been handled by the most derived class";
+}, /*IsPrunable=*/true));
+// Perform the transition.
+ExplodedNodeSet Dst;
+NodeBuilder Bldr(Pred, Dst, BuilderCtx);
+Pred = Bldr.generateNode(P, Pred->getState(), Pred);
+if (!Pred)
+  return;
+  }
+
   // Check if we are entering the EXIT block.
   if (Blk == &(L.getLocationContext()->getCFG()->getExit())) {
 assert(L.getLocationContext()->getCFG()->getExit().empty() &&
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ 

[PATCH] D61816: [CFG] [analyzer] pr41300: Add a branch to skip virtual base initializers when they are handled by the superclass.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 199532.
NoQ added a comment.

Rebase.


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

https://reviews.llvm.org/D61816

Files:
  clang/include/clang/Analysis/AnalysisDeclContext.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/lib/Analysis/AnalysisDeclContext.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/initializer.cpp
  clang/test/Analysis/initializers-cfg-output.cpp

Index: clang/test/Analysis/initializers-cfg-output.cpp
===
--- clang/test/Analysis/initializers-cfg-output.cpp
+++ clang/test/Analysis/initializers-cfg-output.cpp
@@ -30,21 +30,25 @@
 class B : public virtual A {
 public:
   // CHECK:   B()
-  // CHECK:[B2 (ENTRY)]
-  // CHECK-NEXT: Succs (1): B1
+  // CHECK:[B3 (ENTRY)]
+  // CHECK-NEXT: Succs (1): B2
   // CHECK:[B1]
   // WARNINGS-NEXT: 1:  (CXXConstructExpr, class A)
   // ANALYZER-NEXT: 1:  (CXXConstructExpr, A() (Base initializer), class A)
   // CHECK-NEXT: 2: A([B1.1]) (Base initializer)
   // CHECK-NEXT: Preds (1): B2
   // CHECK-NEXT: Succs (1): B0
+  // CHECK:[B2]
+  // CHECK-NEXT: T: (See if superclass ctor has already initialized vbases)
+  // CHECK-NEXT: Preds (1): B3
+  // CHECK-NEXT: Succs (2): B0 B1
   // CHECK:[B0 (EXIT)]
-  // CHECK-NEXT: Preds (1): B1
+  // CHECK-NEXT: Preds (2): B1 B2
   B() {}
 
   // CHECK:   B(int i)
-  // CHECK:[B2 (ENTRY)]
-  // CHECK-NEXT: Succs (1): B1
+  // CHECK:[B3 (ENTRY)]
+  // CHECK-NEXT: Succs (1): B2
   // CHECK:[B1]
   // CHECK-NEXT: 1: i
   // CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
@@ -53,29 +57,37 @@
   // CHECK-NEXT: 4: A([B1.3]) (Base initializer)
   // CHECK-NEXT: Preds (1): B2
   // CHECK-NEXT: Succs (1): B0
+  // CHECK:[B2]
+  // CHECK-NEXT: T: (See if superclass ctor has already initialized vbases)
+  // CHECK-NEXT: Preds (1): B3
+  // CHECK-NEXT: Succs (2): B0 B1
   // CHECK:[B0 (EXIT)]
-  // CHECK-NEXT: Preds (1): B1
+  // CHECK-NEXT: Preds (2): B1 B2
   B(int i) : A(i) {}
 };
 
 class C : public virtual A {
 public:
   // CHECK:   C()
-  // CHECK:[B2 (ENTRY)]
-  // CHECK-NEXT: Succs (1): B1
+  // CHECK:[B3 (ENTRY)]
+  // CHECK-NEXT: Succs (1): B2
   // CHECK:[B1]
   // WARNINGS-NEXT: 1:  (CXXConstructExpr, class A)
   // ANALYZER-NEXT: 1:  (CXXConstructExpr, A() (Base initializer), class A)
   // CHECK-NEXT: 2: A([B1.1]) (Base initializer)
   // CHECK-NEXT: Preds (1): B2
   // CHECK-NEXT: Succs (1): B0
+  // CHECK:[B2]
+  // CHECK-NEXT: T: (See if superclass ctor has already initialized vbases)
+  // CHECK-NEXT: Preds (1): B3
+  // CHECK-NEXT: Succs (2): B0 B1
   // CHECK:[B0 (EXIT)]
-  // CHECK-NEXT: Preds (1): B1
+  // CHECK-NEXT: Preds (2): B1 B2
   C() {}
 
   // CHECK:   C(int i)
-  // CHECK:[B2 (ENTRY)]
-  // CHECK-NEXT: Succs (1): B1
+  // CHECK:[B3 (ENTRY)]
+  // CHECK-NEXT: Succs (1): B2
   // CHECK:[B1]
   // CHECK-NEXT: 1: i
   // CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
@@ -84,8 +96,12 @@
   // CHECK-NEXT: 4: A([B1.3]) (Base initializer)
   // CHECK-NEXT: Preds (1): B2
   // CHECK-NEXT: Succs (1): B0
+  // CHECK:[B2]
+  // CHECK-NEXT: T: (See if superclass ctor has already initialized vbases)
+  // CHECK-NEXT: Preds (1): B3
+  // CHECK-NEXT: Succs (2): B0 B1
   // CHECK:[B0 (EXIT)]
-  // CHECK-NEXT: Preds (1): B1
+  // CHECK-NEXT: Preds (2): B1 B2
   C(int i) : A(i) {}
 };
 
@@ -98,31 +114,38 @@
 };
 
 // CHECK:   TestOrder::TestOrder()
-// CHECK:[B2 (ENTRY)]
-// CHECK-NEXT: Succs (1): B1
+// CHECK:[B4 (ENTRY)]
+// CHECK-NEXT: Succs (1): B3
 // CHECK:[B1]
+// WARNINGS-NEXT: 1:  (CXXConstructExpr, class C)
+// ANALYZER-NEXT: 1:  (CXXConstructExpr, C() (Base initializer), class C)
+// CHECK-NEXT: 2: C([B1.1]) (Base initializer)
+// WARNINGS-NEXT: 3:  (CXXConstructExpr, class B)
+// ANALYZER-NEXT: 3:  (CXXConstructExpr, B() (Base initializer), class B)
+// CHECK-NEXT: 4: B([B1.3]) (Base initializer)
+// WARNINGS-NEXT: 5:  (CXXConstructExpr, class A)
+// ANALYZER-NEXT: 5:  (CXXConstructExpr, A() (Base initializer), class A)
+// CHECK-NEXT: 6: A([B1.5]) (Base initializer)
+// CHECK-NEXT: 7: /*implicit*/(int)0
+// CHECK-NEXT: 8: i([B1.7]) (Member initializer)
+// CHECK-NEXT: 9: this
+// CHECK-NEXT:10: [B1.9]->i
+// CHECK-NEXT:11: r([B1.10]) (Member initializer)
+// WARNINGS-NEXT:12:  

[PATCH] D61814: [CFG] NFC: Remove implicit conversion from CFGTerminator to Stmt *, make it a variant class instead.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 199531.
NoQ marked 2 inline comments as done.
NoQ added a comment.

Fxd.


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

https://reviews.llvm.org/D61814

Files:
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/Analysis/ProgramPoint.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/CFGStmtMap.cpp
  clang/lib/Analysis/Consumed.cpp
  clang/lib/Analysis/LiveVariables.cpp
  clang/lib/Analysis/ProgramPoint.cpp
  clang/lib/Analysis/ReachableCode.cpp
  clang/lib/Analysis/ThreadSafety.cpp
  clang/lib/Analysis/UninitializedValues.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -794,7 +794,7 @@
   if (auto SP = P.getAs())
 return SP->getStmt();
   if (auto BE = P.getAs())
-return BE->getSrc()->getTerminator();
+return BE->getSrc()->getTerminatorStmt();
   if (auto CE = P.getAs())
 return CE->getCallExpr();
   if (auto CEE = P.getAs())
Index: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -234,7 +234,7 @@
 
 ProgramPoint P = N->getLocation();
 if (Optional BE = P.getAs())
-  S = BE->getBlock()->getTerminator();
+  S = BE->getBlock()->getTerminatorStmt();
 
 if (S == LoopStmt)
   return false;
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1861,7 +1861,7 @@
   // other constraints) then consider completely unrolling it.
   if(AMgr.options.ShouldUnrollLoops) {
 unsigned maxBlockVisitOnPath = AMgr.options.maxBlockVisitOnPath;
-const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminator();
+const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminatorStmt();
 if (Term) {
   ProgramStateRef NewState = updateLoopStack(Term, AMgr.getASTContext(),
  Pred, maxBlockVisitOnPath);
@@ -1882,7 +1882,7 @@
   unsigned int BlockCount = nodeBuilder.getContext().blockCount();
   if (BlockCount == AMgr.options.maxBlockVisitOnPath - 1 &&
   AMgr.options.ShouldWidenLoops) {
-const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminator();
+const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminatorStmt();
 if (!(Term &&
   (isa(Term) || isa(Term) || isa(Term
   return;
@@ -2007,8 +2007,8 @@
   if (!BO || !BO->isLogicalOp())
 return Condition;
 
-  assert(!B->getTerminator().isTemporaryDtorsBranch() &&
- "Temporary destructor branches handled by processBindTemporary.");
+  assert(B->getTerminator().isStmtBranch() &&
+ "Other kinds of branches are handled separately!");
 
   // For logical operations, we still have the case where some branches
   // use the traditional "merge" approach and others sink the branch
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -275,14 +275,14 @@
 }
 
 void CoreEngine::HandleBlockExit(const CFGBlock * B, ExplodedNode *Pred) {
-  if (const Stmt *Term = B->getTerminator()) {
+  if (const Stmt *Term = B->getTerminatorStmt()) {
 switch (Term->getStmtClass()) {
   default:
 llvm_unreachable("Analysis for this terminator not implemented.");
 
   case Stmt::CXXBindTemporaryExprClass:
 HandleCleanupTemporaryBranch(
-cast(B->getTerminator().getStmt()), B, Pred);
+cast(Term), B, Pred);
 return;
 
   // Model static initializers.
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1494,7 +1494,7 @@
 return nullptr;
 
   CFGStmtMap *Map = CurLC->getAnalysisDeclContext()->getCFGStmtMap();
-  CurTerminatorStmt = Map->getBlock(CurStmt)->getTerminator();
+  CurTerminatorStmt = Map->getBlock(CurStmt)->getTerminatorStmt();
 } else {
   return nullptr;
 }
@@ -1566,7 +1566,7 @@
   

[PATCH] D60974: Clang IFSO driver action.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

How about some cases for:

- global variable which is `static` and `extern`'ed
- global variable which is `static` defined in a function which is `static`
- global variable which is `static` defined in a function which is *not* 
`inline`
- global variable which is `static` which is `static` `inline` (GNU inline 
semantics)
- global variable which is `static` which is `static` `inline` (C99 inline 
semantics)
- virtual functions
- class vtables
- tests for hidden class deriving from hidden class
- tests for hidden class deriving from public class
- tests for public class deriving from public class
- tests for public class deriving from hidden class
- `constexpr` cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

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

rsmith added a comment.

In D36357#1501999 , @Rakete wrote:

> In D36357#1501961 , @rsmith wrote:
>
> > In D36357#1500949 , @Rakete 
> > wrote:
> >
> > > How should I do this? Do I just skip to the next `}`, or also take into 
> > > account  any additional scopes? Also does this mean that I skip and then 
> > > revert, because that seems pretty expensive?
> >
> >
> > It would be a little expensive, yes, but we'd only be doing it on codepaths 
> > where we're producing an error -- for an ill-formed program, it's OK to 
> > take more time in order to produce a better diagnostic. Skipping to the 
> > next `}` won't work, because `SkipUntil` will skip over pairs of 
> > brace-balanced tokens (so you'll skip past the `}` you're looking for), but 
> > skipping until the next `{` and then skipping to the `}` after it should 
> > work.
>
>
> Hmm wouldn't this interact badly with `{}` in initializers?
>
>   [] {};
>   [](int = {0}) {};
>


Ouch. The first of these two seems like it won't work, since `SkipUntil` has no 
idea the angles are brackets. =( Getting this right is ... really hairy. Eg:

  []{} // end of lambda here?
  >(){} // or here?

(The answer depends on whether `a` is a template.) And the same problem shows 
up in the //trailing-return-type// and the //requires-clause//.

How about this: `SkipUntil` an `{` or `<`. If you find a `<` first, don't 
attach a `FixItHint` to the diagnostic since we can't easily tell where to 
suggest putting the `)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357



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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

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



Comment at: test/std/containers/map_allocator_requirement_test_templates.h:236
 const ValueTp v(42, 1);
-cc->expect();
+cc->expect();
 It ret = c.insert(c.end(), std::move(v));

EricWF wrote:
> I really think the current behavior libc++ has here is correct.
> 
> 
File a DR to retroactively make libc++'s behavior standard then? :)


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

https://reviews.llvm.org/D61364



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete updated this revision to Diff 199528.
Rakete marked an inline comment as done.
Rakete added a comment.

Nevermind, seems to be working fine even with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++2a %s
 
 enum E { e };
 
@@ -43,31 +44,57 @@
 int a4[1] = {[] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'const int *'}}
 int a5[3] = { []{return 0;}() };
 int a6[1] = {[this] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'C *'}}
-int a7[1] = {[d(0)] { return d; } ()}; // expected-warning{{extension}}
-int a8[1] = {[d = 0] { return d; } ()}; // expected-warning{{extension}}
+int a7[1] = {[d(0)] { return d; } ()};
+int a8[1] = {[d = 0] { return d; } ()};
+int a10[1] = {[id(0)] { return id; } ()};
+#if __cplusplus <= 201103L
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+#endif
 int a9[1] = {[d = 0] = 1}; // expected-error{{is not an integral constant expression}}
-int a10[1] = {[id(0)] { return id; } ()}; // expected-warning{{extension}}
+#if __cplusplus >= 201402L
+// expected-note@-2{{constant expression cannot modify an object that is visible outside that expression}}
+#endif
 int a11[1] = {[id(0)] = 1};
   }
 
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#if __cplusplus > 201703L
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#endif
   }
 
   // We support init-captures in C++11 as an extension.
   int z;
   void init_capture() {
-[n(0)] () mutable -> int { return ++n; }; // expected-warning{{extension}}
-[n{0}] { return; }; // expected-warning{{extension}}
-[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}} expected-warning{{extension}}
-[n = {0}] { return; }; // expected-error {{}} expected-warning{{extension}}
-[a([ = z]{})](){}; // expected-warning 2{{extension}}
+[n(0)] () mutable -> int { return ++n; };
+[n{0}] { return; };
+[a([ = z]{})](){};
+[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}}
+[n = {0}] { return; }; // expected-error {{}}
+#if __cplusplus <= 201103L
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+#endif
 
 int x = 4;
-auto y = [ = x, x = x + 1]() -> int { // expected-warning 2{{extension}}
+auto y = [ = x, x = x + 1]() -> int {
+#if __cplusplus <= 201103L
+  // expected-warning@-2{{extension}}
+  // expected-warning@-3{{extension}}
+#endif
   r += 2;
   return x + 2;
 } ();
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda 

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: test/std/containers/map_allocator_requirement_test_templates.h:236
 const ValueTp v(42, 1);
-cc->expect();
+cc->expect();
 It ret = c.insert(c.end(), std::move(v));

I really think the current behavior libc++ has here is correct.




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

https://reviews.llvm.org/D61364



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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added a comment.

In D61364#1502200 , @EricWF wrote:

> From Billy and my last discussion, I think we came to the agreement that it's 
> not clear exactly what the "standard behavior" is.


No, I don't think so. I think there was agreement that the behavior in the 
standard may not have been what was intended by the original move semantics 
proposals, but I don't think there's any doubt about what the standard 
currently requires.


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

https://reviews.llvm.org/D61364



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D61634#1502138 , @hfinkel wrote:

> In D61634#1502043 , @efriedma wrote:
>
> > > I have a related patch that turns -fno-builtin* options into module flags
> >
> > Do you have any opinion on representing -fno-builtin using a module flag 
> > vs. a function attribute in IR?  It seems generally more flexible and 
> > easier to reason about a function attribute from my perspective.  But I 
> > might be missing something about the semantics of -fno-builtin that would 
> > make that representation awkward.  Or I guess it might just be more work to 
> > implement, given we have some IPO passes that use TargetLibraryInfo?
>
>
> I think that a function attribute would be better. We generally use these 
> flags only in the context of certain translation units, and when we use LTO, 
> it would be sad if we had to take the most-conservative settings across the 
> entire application. When we insert new function call to a standard library, 
> we always do it in the context of some function. We'd probably need to block 
> inlining in some cases, but that's better than a global conservative setting.


Using function level attributes instead of module flags does provide finer 
grained control and avoids the conservativeness when merging IR for LTO. The 
downsides I see, mostly just in terms of the engineering effort to get this to 
work, are:

- need to prevent inlining with different attributes
- currently the TargetLibraryInfo is constructed on a per-module basis. 
Presumably it would instead need to be created per Function - this one in 
particular seems like it would require fairly extensive changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

From Billy and my last discussion, I think we came to the agreement that it's 
not clear exactly what the "standard behavior" is.


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

https://reviews.llvm.org/D61364



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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added a comment.

In D61364#1502172 , @ldionne wrote:

> Do you know *why* the tests are failing with libc++? I see this overload for 
> `insert` and it seems like it should be a better match?


No, I haven't investigated. I avoid looking at libc++ product code too much for 
now because I want to avoid any appearance or reality of licensing conflicts... 
for now  ;) .

In D61364#1502172 , @ldionne wrote:

> I think we should just fix libc++ to do the right thing (according to the 
> standard).


That's what I'd recommend :)

In D61364#1502172 , @ldionne wrote:

> Separately, I'm not sure it's worth changing the Standard to ensure that the 
> `T const&` overload is selected -- I don't see a huge benefit here.


Would be a code size win if both overloads are called in the same program.


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

https://reviews.llvm.org/D61364



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

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added a comment.

In D61366#1502170 , @Quuxplusone wrote:

> you are indeed allowed to //move// the //elements//


And indeed we *must* do that :).


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

https://reviews.llvm.org/D61366



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

r360720 - Fix bots by adding target triple to test.

[Leonard Chan via cfe-commits]

Author: leonardchan
Date: Tue May 14 15:37:34 2019
New Revision: 360720

URL: http://llvm.org/viewvc/llvm-project?rev=360720=rev
Log:
Fix bots by adding target triple to test.

Modified:
cfe/trunk/test/CodeGen/hwasan-new-pm.c

Modified: cfe/trunk/test/CodeGen/hwasan-new-pm.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/hwasan-new-pm.c?rev=360720=360719=360720=diff
==
--- cfe/trunk/test/CodeGen/hwasan-new-pm.c (original)
+++ cfe/trunk/test/CodeGen/hwasan-new-pm.c Tue May 14 15:37:34 2019
@@ -2,19 +2,19 @@
 // We run them under different optimizations and LTOs to ensure the IR is still
 // being instrumented properly.
 
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto=thin %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=hwaddress %s | FileCheck %s --check-prefixes=CHECK,HWASAN
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,HWASAN
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto=thin %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=hwaddress -flto=thin %s | FileCheck 
%s --check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=hwaddress -flto=thin %s | FileCheck 
%s
 
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress -flto=thin %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress %s | FileCheck %s --check-prefixes=CHECK,KHWASAN
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress -flto=thin %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto %s | 
FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto=thin %s | 
FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto %s | 
FileCheck %s --check-prefixes=CHECK,KHWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto=thin %s | 
FileCheck %s
 
 int foo(int *a) { return *a; }
 


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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

> When the insert(P&&) is called, it delegates to emplace, which only gets 
> Cpp17EmplaceConstructible from the supplied parameters, not 
> Cpp17EmplaceConstructible from add_const_t of the parameters. Thus, libcxx's 
> test is asserting a condition the standard explicitly does not allow at this 
> time. [...]

Agreed.

> Unfortunately with the tests fixed to assert the correct allocator::construct 
> call, libc++ will fail these tests. I'm looking for guidance on how libc++ 
> maintainers would like to handle this.

Do you know *why* the tests are failing with libc++? I see this overload for 
`insert` and it seems like it should be a better match?

  template ::value>::type>
  _LIBCPP_INLINE_VISIBILITY
  pair insert(_Pp&& __p);

I think we should just fix libc++ to do the right thing (according to the 
standard).

Separately, I'm not sure it's worth changing the Standard to ensure that the `T 
const&` overload is selected -- I don't see a huge benefit here.


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

https://reviews.llvm.org/D61364



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

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

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

Quuxplusone added a comment.

>> Are you not allowed to move the containers elements in this case?
> 
> Correct. The allocator is not POCMA and not equal, so it's functionally the 
> same as doing `assign(make_move_iterator(begin()), 
> make_move_iterator(end()))`.

In case the clarification helps some reader: When the allocator is 
not-POCMA-and-not-equal, then you are forbidden to //pilfer// the //pointer// 
from the source container, but you are indeed allowed to //move// the 
//elements//, and that's what Billy is describing. You could finish by having 
the source container destroy all its elements (those elements being now in a 
moved-from state) and become empty, but that's less efficient than keeping the 
moved-from elements around.

Unless of course you know that the element type is //trivially relocatable//... 
;)


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

https://reviews.llvm.org/D61366



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D61634#1502043 , @efriedma wrote:

> > I have a related patch that turns -fno-builtin* options into module flags
>
> Do you have any opinion on representing -fno-builtin using a module flag vs. 
> a function attribute in IR?  It seems generally more flexible and easier to 
> reason about a function attribute from my perspective.  But I might be 
> missing something about the semantics of -fno-builtin that would make that 
> representation awkward.  Or I guess it might just be more work to implement, 
> given we have some IPO passes that use TargetLibraryInfo?


I think that a function attribute would be better. We generally use these flags 
only in the context of certain translation units, and when we use LTO, it would 
be sad if we had to take the most-conservative settings across the entire 
application. When we insert new function call to a standard library, we always 
do it in the context of some function. We'd probably need to block inlining in 
some cases, but that's better than a global conservative setting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D61350: [clang-tidy] New check calling out uses of +new in Objective-C code

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore accepted this revision.
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp:54
+CheckFactories.registerCheck(
+"google-objc-avoid-nsobject-new");
 CheckFactories.registerCheck(

It is a little odd that "google-objc-avoid-nsobject-new" warns on `+new` even 
for classes that are not derived from `NSObject`. In a sense, "nsobject-new" is 
being used as an identifier for any class method named "new". Interestingly, 
renaming to the more direct  "google-objc-avoid-new-class-method" is even more 
misleading. I have yet to think of a name that I think would actually be better.

While I think the current name is potentially misleading, I think I am okay 
with this name for the following reasons:
(1) I believe this will only be misleading in marginal scenarios. I believe 
that it will be exceedingly rare for a class method named `new` to exist in a 
class hierarchy where the root class is not `NSObject`.
(2) We can always amend the check to restrict it to `NSObject`. In the 
unexpected circumstance where someone does provide a legitimate use case for a 
class method named `new` in a class hierarchy where the root class is not 
`NSObject`, we can simply amend the check to restrict its enforcement to class 
hierarchies where the root class is `NSObject`.


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

https://reviews.llvm.org/D61350



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

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> If we agree that this is a good way forward, there also appears to be 
> +neonfp/-neonfp additions happening in handleTargetFeatures that should 
> probably be happening in initFeatureMap instead?

neonfp isn't passed as a feature in the first place; there's a separate API 
setFPMath which is used for that.  We translate it into a target feature for 
the sake of the backend.  So I'm not sure what you're proposing.

--

What happens if someone specifies __attribute__((target("soft-float-abi"))) or 
something like that?  (There's an API isValidFeatureName to validate feature 
names, but we currently don't implement it.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D61750



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

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal added a comment.

In D61366#1502073 , @EricWF wrote:

> I'm not sure I agree with your design decision, but this patch LGTM.


I wouldn't object to a standards change to make this the case; though it is 
suboptimal to destroy all the elements needlessly.

> Are you not allowed to move the containers elements in this case?

Correct. The allocator is not POCMA and not equal, so it's functionally the 
same as doing assign(make_move_iterator(begin()), make_move_iterator(end())).

Billy3


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

https://reviews.llvm.org/D61366



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

[PATCH] D61365: [libcxx] [test] Suppress float->int narrowing warning in vector range-construction test.

[Eric Fiselier via Phabricator via cfe-commits]

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

This LGTM after correctly wrapping it as Casey mentionse.


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

https://reviews.llvm.org/D61365



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

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

[Eric Fiselier via Phabricator via cfe-commits]

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

I'm not sure I agree with your design decision, but this patch LGTM.

Are you not allowed to move the containers elements in this case?


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

https://reviews.llvm.org/D61366



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

[PATCH] D61845: [builtin] Fixed definitions of builtins that rely on the int/long long type is 32/64 bits

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: test/CodeGen/builtins.cpp:5
+// RUN: %clang_cc1 -std=c++11 -triple powerpc-pc-linux -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple arm-linux-gnueabi -verify %s
+

You don't need quite so many targets on this list.  There are essentially three 
interesting kinds of targets: targets where int is 16 bits (like avr), targets 
where int is 32 bits and long is 64 bits (like x86_64 Linux), and targets where 
int and long are both 32 bits (like i686 Linux).

You might need to pass -ffreestanding to avoid including /usr/include/stdint.h 
on non-Linux systems.



Comment at: test/CodeGen/builtins.cpp:21
+
+uint16_t bswap16; // expected-note{{previous definition is here}}
+decltype(__builtin_bswap16(0)) bswap16 = 42; // 
expected-error-re{{redefinition of 'bswap16'{{$

If you write "extern uint16_t bswap16;", there won't be any error message when 
the types match.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61845



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

[PATCH] D57858: [analyzer] Add a new frontend flag to display all checker options

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D57858#1501065 , @dkrupp wrote:

> These are the alpha checkers that we are testing in Ericsson:


Hmm, if you don't mind i'll take this to cfe-dev, as it's an interesting topic 
:)


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

https://reviews.llvm.org/D57858



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

[PATCH] D61709: [NewPM] Port HWASan and Kernel HWASan

[Leonard Chan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
leonardchan marked 2 inline comments as done.
Closed by commit rC360707: [NewPM] Port HWASan and Kernel HWASan (authored by 
leonardchan, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61709?vs=198889=199511#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D61709

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/hwasan-new-pm.c

Index: test/CodeGen/hwasan-new-pm.c
===
--- test/CodeGen/hwasan-new-pm.c
+++ test/CodeGen/hwasan-new-pm.c
@@ -0,0 +1,34 @@
+// Test that HWASan and KHWASan runs with the new pass manager.
+// We run them under different optimizations and LTOs to ensure the IR is still
+// being instrumented properly.
+
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=hwaddress %s | FileCheck %s --check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=hwaddress -flto=thin %s | FileCheck %s --check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=hwaddress %s | FileCheck %s --check-prefixes=CHECK,HWASAN
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,HWASAN
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=hwaddress -flto=thin %s | FileCheck %s
+
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress %s | FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto=thin %s | FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress %s | FileCheck %s --check-prefixes=CHECK,KHWASAN
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,KHWASAN
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto=thin %s | FileCheck %s
+
+int foo(int *a) { return *a; }
+
+// All the cases above mark the function with sanitize_hwaddress.
+// CHECK-DAG: sanitize_hwaddress
+
+// Both sanitizers produce %hwasan.shadow without both thinlto and optimizations.
+// HWASAN-DAG: %hwasan.shadow
+// KHWASAN-DAG: %hwasan.shadow
+
+// Both sanitizers produce __hwasan_tls without both thinlto and optimizations.
+// HWASAN-DAG: __hwasan_tls
+// KHWASAN-DAG: __hwasan_tls
+
+// For unoptimized cases, both sanitizers produce different load functions.
+// HWASAN-NOOPT-DAG: __hwasan_loadN
+// KHWASAN-NOOPT-DAG: __hwasan_loadN_noabort
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -57,6 +57,7 @@
 #include "llvm/Transforms/Instrumentation/AddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
+#include "llvm/Transforms/Instrumentation/HWAddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/InstrProfiling.h"
 #include "llvm/Transforms/Instrumentation/MemorySanitizer.h"
 #include "llvm/Transforms/Instrumentation/ThreadSanitizer.h"
@@ -265,12 +266,13 @@
   static_cast(Builder);
   const CodeGenOptions  = BuilderWrapper.getCGOpts();
   bool Recover = CGOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
-  PM.add(createHWAddressSanitizerPass(/*CompileKernel*/ false, Recover));
+  PM.add(
+  createHWAddressSanitizerLegacyPassPass(/*CompileKernel*/ false, Recover));
 }
 
 static void addKernelHWAddressSanitizerPasses(const PassManagerBuilder ,
 legacy::PassManagerBase ) {
-  PM.add(createHWAddressSanitizerPass(
+  PM.add(createHWAddressSanitizerLegacyPassPass(
   /*CompileKernel*/ true, /*Recover*/ true));
 }
 
@@ -962,6 +964,17 @@
   if (LangOpts.Sanitize.has(SanitizerKind::Thread)) {
 MPM.addPass(createModuleToFunctionPassAdaptor(ThreadSanitizerPass()));
   }
+
+  if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
+bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
+MPM.addPass(createModuleToFunctionPassAdaptor(
+HWAddressSanitizerPass(/*CompileKernel=*/false, Recover)));
+  }
+
+  if 

[PATCH] D61709: [NewPM] Port HWASan and Kernel HWASan

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:179
 
-  bool runOnFunction(Function ) override;
-  bool doInitialization(Module ) override;
+  bool instrumentFunction(Function );
+  void initializeWithModule(Module );

philip.pfaffe wrote:
> There are some naming clashes here. In the other sanitizers these functions 
> are called `sanitizeFunction` and `initializeModule`.
Renamed


Repository:
  rC Clang

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

https://reviews.llvm.org/D61709



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> I have a related patch that turns -fno-builtin* options into module flags

Do you have any opinion on representing -fno-builtin using a module flag vs. a 
function attribute in IR?  It seems generally more flexible and easier to 
reason about a function attribute from my perspective.  But I might be missing 
something about the semantics of -fno-builtin that would make that 
representation awkward.  Or I guess it might just be more work to implement, 
given we have some IPO passes that use TargetLibraryInfo?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

r360707 - [NewPM] Port HWASan and Kernel HWASan

[Leonard Chan via cfe-commits]

Author: leonardchan
Date: Tue May 14 14:17:21 2019
New Revision: 360707

URL: http://llvm.org/viewvc/llvm-project?rev=360707=rev
Log:
[NewPM] Port HWASan and Kernel HWASan

Port hardware assisted address sanitizer to new PM following the same 
guidelines as msan and tsan.

Changes:
- Separate HWAddressSanitizer into a pass class and a sanitizer class.
- Create new PM wrapper pass for the sanitizer class.
- Use the getOrINsert pattern for some module level initialization declarations.
- Also enable kernel-kwasan in new PM
- Update llvm tests and add clang test.

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

Added:
cfe/trunk/test/CodeGen/hwasan-new-pm.c
Modified:
cfe/trunk/lib/CodeGen/BackendUtil.cpp

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=360707=360706=360707=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Tue May 14 14:17:21 2019
@@ -57,6 +57,7 @@
 #include "llvm/Transforms/Instrumentation/AddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
+#include "llvm/Transforms/Instrumentation/HWAddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/InstrProfiling.h"
 #include "llvm/Transforms/Instrumentation/MemorySanitizer.h"
 #include "llvm/Transforms/Instrumentation/ThreadSanitizer.h"
@@ -265,12 +266,13 @@ static void addHWAddressSanitizerPasses(
   static_cast(Builder);
   const CodeGenOptions  = BuilderWrapper.getCGOpts();
   bool Recover = CGOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
-  PM.add(createHWAddressSanitizerPass(/*CompileKernel*/ false, Recover));
+  PM.add(
+  createHWAddressSanitizerLegacyPassPass(/*CompileKernel*/ false, 
Recover));
 }
 
 static void addKernelHWAddressSanitizerPasses(const PassManagerBuilder 
,
 legacy::PassManagerBase ) {
-  PM.add(createHWAddressSanitizerPass(
+  PM.add(createHWAddressSanitizerLegacyPassPass(
   /*CompileKernel*/ true, /*Recover*/ true));
 }
 
@@ -962,6 +964,17 @@ static void addSanitizersAtO0(ModulePass
   if (LangOpts.Sanitize.has(SanitizerKind::Thread)) {
 MPM.addPass(createModuleToFunctionPassAdaptor(ThreadSanitizerPass()));
   }
+
+  if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
+bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
+MPM.addPass(createModuleToFunctionPassAdaptor(
+HWAddressSanitizerPass(/*CompileKernel=*/false, Recover)));
+  }
+
+  if (LangOpts.Sanitize.has(SanitizerKind::KernelHWAddress)) {
+MPM.addPass(createModuleToFunctionPassAdaptor(
+HWAddressSanitizerPass(/*CompileKernel=*/true, /*Recover=*/true)));
+  }
 }
 
 /// A clean version of `EmitAssembly` that uses the new pass manager.
@@ -1145,6 +1158,23 @@ void EmitAssemblyHelper::EmitAssemblyWit
   UseOdrIndicator));
 });
   }
+  if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
+bool Recover =
+CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
+PB.registerOptimizerLastEPCallback(
+[Recover](FunctionPassManager ,
+  PassBuilder::OptimizationLevel Level) {
+  FPM.addPass(HWAddressSanitizerPass(
+  /*CompileKernel=*/false, Recover));
+});
+  }
+  if (LangOpts.Sanitize.has(SanitizerKind::KernelHWAddress)) {
+PB.registerOptimizerLastEPCallback(
+[](FunctionPassManager , PassBuilder::OptimizationLevel Level) 
{
+  FPM.addPass(HWAddressSanitizerPass(
+  /*CompileKernel=*/true, /*Recover=*/true));
+});
+  }
   if (Optional Options = getGCOVOptions(CodeGenOpts))
 PB.registerPipelineStartEPCallback([Options](ModulePassManager ) {
   MPM.addPass(GCOVProfilerPass(*Options));

Added: cfe/trunk/test/CodeGen/hwasan-new-pm.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/hwasan-new-pm.c?rev=360707=auto
==
--- cfe/trunk/test/CodeGen/hwasan-new-pm.c (added)
+++ cfe/trunk/test/CodeGen/hwasan-new-pm.c Tue May 14 14:17:21 2019
@@ -0,0 +1,34 @@
+// Test that HWASan and KHWASan runs with the new pass manager.
+// We run them under different optimizations and LTOs to ensure the IR is still
+// being instrumented properly.
+
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz updated this revision to Diff 199508.
beanz added a comment.

Changed to lowercase 'c' to match other clang libraries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909

Files:
  clang/cmake/modules/AddClang.cmake
  clang/tools/CMakeLists.txt
  clang/tools/clang-shlib/CMakeLists.txt
  clang/tools/clang-shlib/clang-shlib.cpp


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} 
${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module , TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

tejohnson wrote:
> tejohnson wrote:
> > steven_wu wrote:
> > > Should this be done not just for LTOBackend but for regular compilation 
> > > as well? LegacyCodegenerator and llc can all be benefit from a matching 
> > > TargetLibraryInfo?
> > Yeah, probably. I think the best way to have this utilized everywhere is to 
> > move the below code into the TargetLibraryInfoImpl itself - by having it 
> > also take the Module as a parameter). Probably as a required parameter, to 
> > ensure it is used consistently. WDYT? 
> I meant, "into the TargetLibraryInfoImpl constructor"
SGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@winksaville whether or not PIC is required for shared libraries varies by 
platform. These days LLVM defaults to -fPIC, and I'm not sure we actually 
support running LLVM without -fPIC on systems that require shared libraries to 
be PIC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete marked 2 inline comments as done.
Rakete added a comment.

In D36357#1501961 , @rsmith wrote:

> In D36357#1500949 , @Rakete 
> wrote:
>
> > How should I do this? Do I just skip to the next `}`, or also take into 
> > account  any additional scopes? Also does this mean that I skip and then 
> > revert, because that seems pretty expensive?
>
>
> It would be a little expensive, yes, but we'd only be doing it on codepaths 
> where we're producing an error -- for an ill-formed program, it's OK to take 
> more time in order to produce a better diagnostic. Skipping to the next `}` 
> won't work, because `SkipUntil` will skip over pairs of brace-balanced tokens 
> (so you'll skip past the `}` you're looking for), but skipping until the next 
> `{` and then skipping to the `}` after it should work.


Hmm wouldn't this interact badly with `{}` in initializers?

  [] {};
  [](int = {0}) = {};






Comment at: clang/lib/Parse/ParseExprCXX.cpp:2996
+   GetLookAheadToken(4).is(tok::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis

rsmith wrote:
> rsmith wrote:
> > `RightBracketLock` -> `RSquareLoc`
> > 
> > (Our convention is to use `Loc` for "location" and to avoid the word 
> > "bracket" because it means different things in different English dialects 
> > -- usually `[]` in US English and usually `()` in UK English.)
> `Lock` -> `Loc`. There's no `k` in "location" =)
No idea how that k managed to sneak in :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357



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

RE: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways

[via cfe-commits]

> -Original Message-
> From: David Blaikie [mailto:dblai...@gmail.com]
> Sent: Tuesday, May 14, 2019 1:47 PM
> To: Robinson, Paul
> Cc: cfe-commits
> Subject: Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we
> don't need two ways
> 
> Fair enough - since they're already there I don't feel super serious
> about converging on one (though I probably wouldn't've been in favor
> of adding a second spelling at the point it was proposed).

UNSUPPORTED came first, in 2014, followed by the rare REQUIRES-ANY
in 2016, followed by boolean expression syntax in 2017.  UNSUPPORTED
is particularly popular in the libcxx suite (over 2000 tests) so I
doubt there's enough motivation to remove it.  REQUIRES-ANY could be 
tossed though, 17 occurrences across clang and compiler-rt.

I do see one test in clang that uses UNSUPPORTED as a FileCheck prefix,
I'll make a note to change that next time I need a break from real work.
--paulr

> 
> On Tue, May 14, 2019 at 8:03 AM  wrote:
> >
> > There's no practical difference.  I view it as a matter of documentation
> of intent, see my commit log comment for r360603.
> >
> > Arguably we could eliminate UNSUPPORTED and move all the expressions
> into REQUIRES (appropriately negated), but I'm not sure that's a net win
> for readability.
> >
> > --paulr
> >
> >
> >
> > From: David Blaikie [mailto:dblai...@gmail.com]
> > Sent: Monday, May 13, 2019 10:48 AM
> > To: Robinson, Paul
> > Cc: cfe-commits
> > Subject: Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because
> we don't need two ways
> >
> >
> >
> > What's the practical difference between "UNSUPPORTED: foo" and
> "REQUIRES: !foo"? (I see some of the fixes you've made go one way, some
> the other way)
> >
> >
> >
> > On Fri, May 10, 2019 at 11:30 AM Paul Robinson via cfe-commits  comm...@lists.llvm.org> wrote:
> >
> > Author: probinson
> > Date: Fri May 10 11:32:53 2019
> > New Revision: 360452
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=360452=rev
> > Log:
> > Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways
> > to say the same thing.
> >
> > Modified:
> > cfe/trunk/test/Driver/nozlibcompress.c
> >
> > Modified: cfe/trunk/test/Driver/nozlibcompress.c
> > URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/Driver/nozlibcompress.c?rev=360452=360451=360
> 452=diff
> >
> ==
> 
> > --- cfe/trunk/test/Driver/nozlibcompress.c (original)
> > +++ cfe/trunk/test/Driver/nozlibcompress.c Fri May 10 11:32:53 2019
> > @@ -1,4 +1,4 @@
> > -// REQUIRES: nozlib
> > +// REQUIRES: !zlib
> >
> >  // RUN: %clang -### -fintegrated-as -gz -c %s 2>&1 | FileCheck %s -
> check-prefix CHECK-WARN
> >  // RUN: %clang -### -fintegrated-as -gz=none -c %s 2>&1 | FileCheck -
> allow-empty -check-prefix CHECK-NOWARN %s
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61814: [CFG] NFC: Remove implicit conversion from CFGTerminator to Stmt *, make it a variant class instead.

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

Szelethus accepted this revision.
Szelethus added a comment.

LGTM!




Comment at: clang/include/clang/Analysis/CFG.h:514
+  CFGTerminator() { assert(!isValid()); }
+  CFGTerminator(Stmt *S, Kind K = StmtBranch) : Data(S, K) {}
 

Can we add something to check that the integer part of `Data` is actually large 
enough to store an object of type `Kind`, even if we add more values to the 
enum?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61814



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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[Teresa Johnson via Phabricator via cfe-commits]

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



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module , TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

tejohnson wrote:
> steven_wu wrote:
> > Should this be done not just for LTOBackend but for regular compilation as 
> > well? LegacyCodegenerator and llc can all be benefit from a matching 
> > TargetLibraryInfo?
> Yeah, probably. I think the best way to have this utilized everywhere is to 
> move the below code into the TargetLibraryInfoImpl itself - by having it also 
> take the Module as a parameter). Probably as a required parameter, to ensure 
> it is used consistently. WDYT? 
I meant, "into the TargetLibraryInfoImpl constructor"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[Teresa Johnson via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/svml-calls.ll:16
+
+define void @sin_f64(double* nocapture %varray) {
+; CHECK-LABEL: @sin_f64(

steven_wu wrote:
> Personally, I think codegen tests like this will be cleaner to keep in LLVM. 
> Clang tests just test the IRGen of the module flag and LLVM tests check that 
> those flags are respected and module flag merge is respected.
Ok. I originally was doing it here to ensure that ThinLTO distributed backends 
(which use clang) are fixed. But now that the approach no longer involves 
passing down additional info via that path into LTO, I don't think we need to 
test this explicitly but rather just as an LLVM LTO test. Will move.



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module , TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

steven_wu wrote:
> Should this be done not just for LTOBackend but for regular compilation as 
> well? LegacyCodegenerator and llc can all be benefit from a matching 
> TargetLibraryInfo?
Yeah, probably. I think the best way to have this utilized everywhere is to 
move the below code into the TargetLibraryInfoImpl itself - by having it also 
take the Module as a parameter). Probably as a required parameter, to ensure it 
is used consistently. WDYT? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

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

rsmith added a comment.

In D36357#1500949 , @Rakete wrote:

> How should I do this? Do I just skip to the next `}`, or also take into 
> account  any additional scopes? Also does this mean that I skip and then 
> revert, because that seems pretty expensive?


It would be a little expensive, yes, but we'd only be doing it on codepaths 
where we're producing an error -- for an ill-formed program, it's OK to take 
more time in order to produce a better diagnostic. Skipping to the next `}` 
won't work, because `SkipUntil` will skip over pairs of brace-balanced tokens 
(so you'll skip past the `}` you're looking for), but skipping until the next 
`{` and then skipping to the `}` after it should work.




Comment at: clang/lib/Parse/ParseExprCXX.cpp:2996
+   GetLookAheadToken(4).is(tok::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis

rsmith wrote:
> `RightBracketLock` -> `RSquareLoc`
> 
> (Our convention is to use `Loc` for "location" and to avoid the word 
> "bracket" because it means different things in different English dialects -- 
> usually `[]` in US English and usually `()` in UK English.)
`Lock` -> `Loc`. There's no `k` in "location" =)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357



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

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

[Csaba Dabis via Phabricator via cfe-commits]

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

What about "the most derived class" or "a superclass" instead of "the 
superclass"? (https://en.cppreference.com/w/cpp/language/derived_class) 
May the sentence is a little bit too long. It would be cool to say "by `A`" or 
something more simple and precise.




Comment at: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:223
+// But only if we're actually skipping the virtual constructors.
+if (L.getDst() == *L.getSrc()->succ_begin()) {
+  ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(

I think it is better to reduce the number of `if`s and merge the related 
comment as it emphasizes there is //only one thing// happen.


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

https://reviews.llvm.org/D61817



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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev created this revision.
anton-afanasyev added a reviewer: thakis.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Add output to `llvm::errs()` when `-ftime-trace` option is enabled,
add regression test checking this option works as expected.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61914

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/test/Support/check-time-trace.cxx


Index: llvm/test/Support/check-time-trace.cxx
===
--- /dev/null
+++ llvm/test/Support/check-time-trace.cxx
@@ -0,0 +1,11 @@
+// RUN: clang++ -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat | FileCheck %s
+
+// CHECK: "args":{"name":"clang"}
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're


Index: llvm/test/Support/check-time-trace.cxx
===
--- /dev/null
+++ llvm/test/Support/check-time-trace.cxx
@@ -0,0 +1,11 @@
+// RUN: clang++ -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat | FileCheck %s
+
+// CHECK: "args":{"name":"clang"}
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

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

rsmith added a comment.

In D59885#1501774 , @ilya-biryukov 
wrote:

> The suggested approach looks promising. The difference seems to be within the 
> noise levels on my machine:


:) Awesome!

> I guess it would make more sense to do the following before landing this:
> 
> - flip the flag, i.e. instead of reporting whether the token is "new" (the 
> common case), report whether it's "cached", i.e. coming from `CachedTokens` 
> or from a non-macro-expansion token stream.
> - set this flag only inside `CachingLex` and `TokenLexer`.
> 
>   The revision also removes `IsNewToken` from `CachingLex` and sets the flag 
> in the token instead. Any other suggestions?

Notionally, I think we should separate out "produces new tokens" from "is macro 
expansion" in `TokenLexer`. Please take a quick look at the various callers of 
`EnterTokenStream` (particularly the ones inside `Lex`; I'm not too worried 
about the ones in `Parse`) and see which (if any) of them actually intend to 
inject new "real" phase-4-of-translation tokens. If there are any (and maybe 
even if there aren't?), please add a flag on `EnterTokenStream` to specify 
whether the `TokenLexer` believes it's creating new tokens or replaying tokens 
that have been seen before.

We'll need to decide what to do about the annotation tokens that we create when 
we enter / leave / import modules (`Preprocessor::EnterAnnotationToken`). In 
some sense, those are "new" tokens -- and that's how the parser will interpret 
them -- but they didn't come from the original source file. I think either 
answer could be OK there, but I'm inclined to treat them as "new" because that 
gives more information to the token consumer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59885



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

r360705 - Fix ASTMerge/namespace/test.cpp after r360701

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Tue May 14 13:01:03 2019
New Revision: 360705

URL: http://llvm.org/viewvc/llvm-project?rev=360705=rev
Log:
Fix ASTMerge/namespace/test.cpp after r360701

Modified:
cfe/trunk/test/ASTMerge/namespace/test.cpp

Modified: cfe/trunk/test/ASTMerge/namespace/test.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ASTMerge/namespace/test.cpp?rev=360705=360704=360705=diff
==
--- cfe/trunk/test/ASTMerge/namespace/test.cpp (original)
+++ cfe/trunk/test/ASTMerge/namespace/test.cpp Tue May 14 13:01:03 2019
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast %S/Inputs/namespace1.cpp
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast %S/Inputs/namespace2.cpp
-// RUN: not %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only %s 2>&1 | FileCheck %s
 
 static_assert(TestAliasName::z == 4);
 static_assert(ContainsInline::z == 10);


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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Torbjörn Klatt via Phabricator via cfe-commits]

torbjoernk updated this revision to Diff 199496.
torbjoernk added a comment.

minor rewording


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

https://reviews.llvm.org/D61827

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp

Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -776,17 +776,20 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
@@ -991,18 +994,26 @@
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = []() { int R = I; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -369,7 +369,7 @@
 // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
@@ -431,19 +431,22 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -258,6 +258,8 @@
   value:   'some value'
   ...
 
+.. _clang-tidy-nolint:
+
 Suppressing Undesired Diagnostics
 =
 
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst

[PATCH] D61281: [clang-format] Fixed self assignment

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 199494.
xbolva00 added a comment.

removed line


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

https://reviews.llvm.org/D61281

Files:
  lib/Format/FormatTokenLexer.cpp


Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -246,7 +246,6 @@
   StringRef(Identifier->TokenText.begin(),
 Question->TokenText.end() - Identifier->TokenText.begin());
   Identifier->ColumnWidth += Question->ColumnWidth;
-  Identifier->Type = Identifier->Type;
   Tokens.erase(Tokens.end() - 1);
   return true;
 }


Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -246,7 +246,6 @@
   StringRef(Identifier->TokenText.begin(),
 Question->TokenText.end() - Identifier->TokenText.begin());
   Identifier->ColumnWidth += Question->ColumnWidth;
-  Identifier->Type = Identifier->Type;
   Tokens.erase(Tokens.end() - 1);
   return true;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61790: [C++20] add consteval specifier

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

rsmith added a comment.

Thanks for working on this!

Comments on the general approach:

- We should only evaluate each immediate invocation once (this will become 
essential once we start supporting reflection -- and particularly operations 
that mutate the AST -- inside `consteval` functions).
- Each time an immediate invocation is formed, you should create a 
`ConstantExpr` AST node wrapping that call to reflect that it is a constant.
- You should change `ConstantExpr` to store an `APValue` representing the 
evaluated value of the expression.

Most of the above should be done as a separate preparatory change; we should be 
able to remove a lot of the existing ad-hoc caching of evaluated values (eg, 
for enumerators and the initializers of variables) at the same time.

Please also split this into smaller pieces. If you split off a patch to just 
add the keyword, parsing, and AST representation for the `consteval` specifier 
(but treat it identically to `constexpr` for constant evaluation purposes), 
that'd be a good first patch for this feature.




Comment at: clang/include/clang/AST/Decl.h:2102
   /// Whether this is a (C++11) constexpr function or constexpr constructor.
   bool isConstexpr() const { return FunctionDeclBits.IsConstexpr; }
   void setConstexpr(bool IC) { FunctionDeclBits.IsConstexpr = IC; }

Please rename this to `isConstexprSpecified()` (compare this to how we model 
`inline`, which has similar behavior: it can be explicit, or can be implied by 
other properties of the function).



Comment at: clang/include/clang/AST/Decl.h:2109
+
+  bool isConstexprOrConsteval() const { return isConstexpr() || isConsteval(); 
}
+

Both the `constexpr` and `consteval` specifiers make a function a "constexpr 
function", so I think this should be called simply `isConstexpr`; callers that 
want to distinguish `constexpr` from `consteval` can use `isConsteval()` (or we 
can add a `getConstexprKind()` or similar function).



Comment at: clang/include/clang/AST/DeclCXX.h:2115
+QualType T, TypeSourceInfo *TInfo, StorageClass SC,
+bool isInline, bool isConstexpr, bool isConsteval,
+SourceLocation EndLocation)

Instead of the three bool flags in a row here (which will make call sites hard 
to read), consider using an enum, perhaps:

```
enum class ConstexprSpecifierKind { None, Constexpr, Consteval };
```



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2297-2300
+def err_consteval_cannot_be_constant_eval : Error<
+  "call to %0 cannot be constant evaluated">;
+def note_argument_n_cannot_be_constant_eval : Note<
+  "argument %0 cannot be constant evaluated">;

It would be useful in these diagnostics to explain why the call is required to 
be constant evaluated; please also use the standard terminology "constant 
expression" here. (Eg, "call to consteval function %0 is not a constant 
expression")



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2308-2314
 def err_constexpr_tag : Error<
   "%select{class|struct|interface|union|enum}0 cannot be marked constexpr">;
-def err_constexpr_dtor : Error<"destructor cannot be marked constexpr">;
+def err_constexpr_dtor : Error<"destructor cannot be marked %0">;
+def err_invalid_consteval_decl_kind : Error<
+  "operator %select{new|delete|new[]|delete[]}0 cannot be marked consteval">;
+def err_take_adress_of_consteval_decl : Error<
+  "taking address of a %0">;

In these diagnostics (including the existing one for tags), we should say 
"declared constexpr" not "marked constexpr".



Comment at: clang/include/clang/Basic/TokenKinds.def:390-391
 
+// C++ consteval proposal
+CXX2A_KEYWORD(consteval , 0)
+

This and `char8_t` are both adopted C++2a features now (they'e not just 
proposals any more); please change the comment to just "C++2a keywords".



Comment at: clang/include/clang/Sema/DeclSpec.h:404
   SourceLocation FS_forceinlineLoc;
-  SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc;
+  SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc, ConstevalLoc;
   SourceLocation TQ_pipeLoc;

I think it would be preferable to track only one location here (for the 
`constexpr` / `consteval` specifier) and store a constexpr specifier kind 
instead of `Constexpr_specified`, like we do for the other kinds of specifier 
for which we allow only one of a set of keywords.



Comment at: clang/include/clang/Sema/Sema.h:985-986
+///   cases in a switch statement).
+/// - "immediate function context" in C++2a terms, a call to a function
+///   marked as consteval
 ConstantEvaluated,

An "immediate function context" is also "potentially evaluated"; I think what 
we want to say here is something like "The current 

[PATCH] D61689: Change -gz and -Wa,--compress-debug-sections to use gABI compression (SHF_COMPRESSED)

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht added a comment.

So, while I think this is an //entirely// reasonable assumption in most cases, 
it's also not one that provides any kind of workaround for the few cases where 
it's not universally true.

- As mentioned in the patch, this effectively changes the default from 
`-gz=zlib-gnu` to `-gz=zlib`. Anyone with an older toolchain that wants the old 
behavior can set `-gz=zlib-gnu`. Seems OK so far.
- However, passing the `-gz` flag //also// implies sending 
`--compress-debug-sections=XXX` to the assembler, which -- if someone is using 
`-no-integrated-as` has an older toolchain -- is not a supported option (at 
least the variant that takes a value).

In other works, for the few users that have an older toolchain, this provides 
literally no workaround. Not setting the flag causes failures with an older 
linker, and setting the flag causes failures with the assembler.

(echristo reverted this in rL360703 , I 
wanted to add some context)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61689



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

Re: r360403 - Change -gz and -Wa, --compress-debug-sections to use gABI compression (SHF_COMPRESSED)

[Eric Christopher via cfe-commits]

Hi Ray,

I've temporarily reverted this here:

echristo@jhereg ~/s/llvm-project> git llvm push
Pushing 1 commit:
  fda79815a33 Temporarily revert "Change -gz and
-Wa,--compress-debug-sections to use gABI compression
(SHF_COMPRESSED)"
Sendingcfe/trunk/docs/ReleaseNotes.rst
Sendingcfe/trunk/lib/Frontend/CompilerInvocation.cpp
Sendingcfe/trunk/tools/driver/cc1as_main.cpp
Transmitting file data ...done
Committing transaction...
Committed revision 360703.
Committed fda79815a33 to svn.

From my message:

This affects users of older (pre 2.26) binutils in such a way that
they can't necessarily
work around it as it doesn't support the compress option on the
command line. Reverting
to unblock them and we can revisit whether to make this change now
or fix how we want
to express the option.

I'm not sure what we want to do here, but wanted to unblock people in
the meantime. What do you think?

Thanks!

-eric


On Thu, May 9, 2019 at 7:05 PM Fangrui Song via cfe-commits
 wrote:
>
> Author: maskray
> Date: Thu May  9 19:08:21 2019
> New Revision: 360403
>
> URL: http://llvm.org/viewvc/llvm-project?rev=360403=rev
> Log:
> Change -gz and -Wa,--compress-debug-sections to use gABI compression 
> (SHF_COMPRESSED)
>
> Since July 15, 2015 (binutils-gdb commit
> 19a7fe52ae3d0971e67a134bcb1648899e21ae1c, included in 2.26), gas
> --compress-debug-sections=zlib (gcc -gz) means zlib-gabi:
> SHF_COMPRESSED. Before that it meant zlib-gnu (.zdebug).
>
> clang's -gz was introduced in rC306115 (Jun 2017) to indicate zlib-gnu. It
> is 2019 now and it is not unreasonable to assume users of the new
> feature to have new linkers (ld.bfd/gold >= 2.26, lld >= rLLD273661).
>
> Change clang's default accordingly to improve standard conformance.
> zlib-gnu becomes out of fashion and gets poorer toolchain support.
> Its mangled names confuse tools and are more likely to cause problems.
>
> Reviewed By: compnerd
>
> Differential Revision: https://reviews.llvm.org/D61689
>
> Modified:
> cfe/trunk/docs/ReleaseNotes.rst
> cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> cfe/trunk/tools/driver/cc1as_main.cpp
>
> Modified: cfe/trunk/docs/ReleaseNotes.rst
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=360403=360402=360403=diff
> ==
> --- cfe/trunk/docs/ReleaseNotes.rst (original)
> +++ cfe/trunk/docs/ReleaseNotes.rst Thu May  9 19:08:21 2019
> @@ -77,7 +77,10 @@ Modified Compiler Flags
>
>  - `clang -dumpversion` now returns the version of Clang itself.
>
> -- ...
> +- On ELF, ``-gz`` now defaults to ``-gz=zlib``. It produces 
> ``SHF_COMPRESSED``
> +  style compression of debug information. GNU binutils 2.26 or newer, or lld 
> is
> +  required to link produced object files. Use ``-gz=zlib-gnu`` to get the old
> +  behavior.
>
>  New Pragmas in Clang
>  
>
> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=360403=360402=360403=diff
> ==
> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Thu May  9 19:08:21 2019
> @@ -1052,8 +1052,7 @@ static bool ParseCodeGenArgs(CodeGenOpti
>if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
>   OPT_compress_debug_sections_EQ)) {
>  if (A->getOption().getID() == OPT_compress_debug_sections) {
> -  // TODO: be more clever about the compression type auto-detection
> -  Opts.setCompressDebugSections(llvm::DebugCompressionType::GNU);
> +  Opts.setCompressDebugSections(llvm::DebugCompressionType::Z);
>  } else {
>auto DCT = 
> llvm::StringSwitch(A->getValue())
>   .Case("none", llvm::DebugCompressionType::None)
>
> Modified: cfe/trunk/tools/driver/cc1as_main.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/driver/cc1as_main.cpp?rev=360403=360402=360403=diff
> ==
> --- cfe/trunk/tools/driver/cc1as_main.cpp (original)
> +++ cfe/trunk/tools/driver/cc1as_main.cpp Thu May  9 19:08:21 2019
> @@ -221,8 +221,7 @@ bool AssemblerInvocation::CreateFromArgs
>if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
>   OPT_compress_debug_sections_EQ)) {
>  if (A->getOption().getID() == OPT_compress_debug_sections) {
> -  // TODO: be more clever about the compression type auto-detection
> -  Opts.CompressDebugSections = llvm::DebugCompressionType::GNU;
> +  Opts.CompressDebugSections = llvm::DebugCompressionType::Z;
>  } else {
>Opts.CompressDebugSections =
>llvm::StringSwitch(A->getValue())
>
>
> 

r360703 - Temporarily revert "Change -gz and -Wa, --compress-debug-sections to use gABI compression (SHF_COMPRESSED)"

[Eric Christopher via cfe-commits]

Author: echristo
Date: Tue May 14 12:40:42 2019
New Revision: 360703

URL: http://llvm.org/viewvc/llvm-project?rev=360703=rev
Log:
Temporarily revert "Change -gz and -Wa,--compress-debug-sections to use gABI 
compression (SHF_COMPRESSED)"

This affects users of older (pre 2.26) binutils in such a way that they can't 
necessarily
work around it as it doesn't support the compress option on the command line. 
Reverting
to unblock them and we can revisit whether to make this change now or fix how 
we want
to express the option.

This reverts commit bdb21337e6e1732c9895966449c33c408336d295/r360403.

Modified:
cfe/trunk/docs/ReleaseNotes.rst
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/tools/driver/cc1as_main.cpp

Modified: cfe/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=360703=360702=360703=diff
==
--- cfe/trunk/docs/ReleaseNotes.rst (original)
+++ cfe/trunk/docs/ReleaseNotes.rst Tue May 14 12:40:42 2019
@@ -77,10 +77,7 @@ Modified Compiler Flags
 
 - `clang -dumpversion` now returns the version of Clang itself.
 
-- On ELF, ``-gz`` now defaults to ``-gz=zlib``. It produces ``SHF_COMPRESSED``
-  style compression of debug information. GNU binutils 2.26 or newer, or lld is
-  required to link produced object files. Use ``-gz=zlib-gnu`` to get the old
-  behavior.
+- ...
 
 New Pragmas in Clang
 

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=360703=360702=360703=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Tue May 14 12:40:42 2019
@@ -1052,7 +1052,8 @@ static bool ParseCodeGenArgs(CodeGenOpti
   if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
  OPT_compress_debug_sections_EQ)) {
 if (A->getOption().getID() == OPT_compress_debug_sections) {
-  Opts.setCompressDebugSections(llvm::DebugCompressionType::Z);
+  // TODO: be more clever about the compression type auto-detection
+  Opts.setCompressDebugSections(llvm::DebugCompressionType::GNU);
 } else {
   auto DCT = llvm::StringSwitch(A->getValue())
  .Case("none", llvm::DebugCompressionType::None)

Modified: cfe/trunk/tools/driver/cc1as_main.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/driver/cc1as_main.cpp?rev=360703=360702=360703=diff
==
--- cfe/trunk/tools/driver/cc1as_main.cpp (original)
+++ cfe/trunk/tools/driver/cc1as_main.cpp Tue May 14 12:40:42 2019
@@ -221,7 +221,8 @@ bool AssemblerInvocation::CreateFromArgs
   if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
  OPT_compress_debug_sections_EQ)) {
 if (A->getOption().getID() == OPT_compress_debug_sections) {
-  Opts.CompressDebugSections = llvm::DebugCompressionType::Z;
+  // TODO: be more clever about the compression type auto-detection
+  Opts.CompressDebugSections = llvm::DebugCompressionType::GNU;
 } else {
   Opts.CompressDebugSections =
   llvm::StringSwitch(A->getValue())


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

[PATCH] D61281: [clang-format] Fixed self assignment

[MyDeveloperDay via Phabricator via cfe-commits]

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

please simply remove line 249


Repository:
  rC Clang

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

https://reviews.llvm.org/D61281



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

I'll be happy to commit for you, but will give it till tomorrow just to make 
sure no one else has any additional comments.

Thanks again!




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst:262
+not been used on code with a compatibility requirements of OpenMP prior to
+version 5. It is **intentional** that this check does not make any attempt to
+not issue diagnostics on OpenMP for loops.

Could you reword this last line to remove the double negative?  Perhaps 
something like:

```
This check makes no attempt to exclude incorrect diagnostics for OpenMP loops 
prior to OpenMP 5.
```


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

https://reviews.llvm.org/D61827



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

r360701 - Update ASTMerge FileCheck test expectations

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Tue May 14 12:02:39 2019
New Revision: 360701

URL: http://llvm.org/viewvc/llvm-project?rev=360701=rev
Log:
Update ASTMerge FileCheck test expectations

I belive many of these diagnostics changed from errors to warnings in
r357394. I've simply mechanically updated the tests, but whoever owns
this code should probably audit for unintented behavior changes. I
wasn't able to find a flag to make these warnings errors again.

Modified:
cfe/trunk/test/ASTMerge/category/test.m
cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp
cfe/trunk/test/ASTMerge/class-template/test.cpp
cfe/trunk/test/ASTMerge/enum/test.c
cfe/trunk/test/ASTMerge/function/test.c
cfe/trunk/test/ASTMerge/interface/test.m
cfe/trunk/test/ASTMerge/namespace/test.cpp
cfe/trunk/test/ASTMerge/property/test.m
cfe/trunk/test/ASTMerge/struct/test.c
cfe/trunk/test/ASTMerge/typedef/test.c
cfe/trunk/test/ASTMerge/var/test.c

Modified: cfe/trunk/test/ASTMerge/category/test.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ASTMerge/category/test.m?rev=360701=360700=360701=diff
==
--- cfe/trunk/test/ASTMerge/category/test.m (original)
+++ cfe/trunk/test/ASTMerge/category/test.m Tue May 14 12:02:39 2019
@@ -1,13 +1,11 @@
-// FIXME: Errors are now warnings.
-// XFAIL: *
 // RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/category1.m
 // RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/category2.m
-// RUN: not %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only 
%s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 
2>&1 | FileCheck %s
 
-// CHECK: category2.m:18:1: error: instance method 'method2' has incompatible 
result types in different translation units ('float' vs. 'int')
+// CHECK: category2.m:18:1: warning: instance method 'method2' has 
incompatible result types in different translation units ('float' vs. 'int')
 // CHECK: category1.m:16:1: note: instance method 'method2' also declared here
-// CHECK: category2.m:26:1: error: instance method 'method3' has incompatible 
result types in different translation units ('float' vs. 'int')
+// CHECK: category2.m:26:1: warning: instance method 'method3' has 
incompatible result types in different translation units ('float' vs. 'int')
 // CHECK: category1.m:24:1: note: instance method 'method3' also declared here
-// CHECK: category2.m:48:1: error: instance method 'blah' has incompatible 
result types in different translation units ('int' vs. 'float')
+// CHECK: category2.m:48:1: warning: instance method 'blah' has incompatible 
result types in different translation units ('int' vs. 'float')
 // CHECK: category1.m:46:1: note: instance method 'blah' also declared here
-// CHECK: 3 errors generated.
+// CHECK: 3 warnings generated.

Modified: cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp?rev=360701=360700=360701=diff
==
--- cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp (original)
+++ cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp Tue May 14 
12:02:39 2019
@@ -1,8 +1,8 @@
-// FIXME: Errors are now warnings.
+// FIXME: Crashes after r357394
 // XFAIL: *
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast 
%S/Inputs/class-template-partial-spec1.cpp
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast 
%S/Inputs/class-template-partial-spec2.cpp
-// RUN: not %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only %s 2>&1 | FileCheck %s
 
 static_assert(sizeof(**SingleSource.member) == sizeof(**SingleDest.member));
 static_assert(sizeof(SecondDoubleSource.member) == 
sizeof(SecondDoubleDest.member));
@@ -11,17 +11,17 @@ static_assert(sizeof(Z0Source.member) ==
 static_assert(sizeof(Dst::Z0Dst.member) == sizeof(double));
 static_assert(sizeof(One::Child1>::member) == sizeof(double));
 
-// CHECK: class-template-partial-spec2.cpp:21:32: error: external variable 
'X1' declared with incompatible types in different translation units 
('TwoOptionTemplate' vs. 'TwoOptionTemplate')
+// CHECK: class-template-partial-spec2.cpp:21:32: warning: external variable 
'X1' declared with incompatible types in different translation units 
('TwoOptionTemplate' vs. 'TwoOptionTemplate')
 // CHECK: class-template-partial-spec1.cpp:21:31: note: declared here with 
type 'TwoOptionTemplate'
 
-// CHECK: class-template-partial-spec2.cpp:24:29: error: external variable 
'X4' declared with incompatible types in different translation units 
('TwoOptionTemplate' vs. 'TwoOptionTemplate')
+// CHECK: class-template-partial-spec2.cpp:24:29: warning: external variable 
'X4' declared with 

Re: r359960 - Reduce amount of work ODR hashing does.

[David Blaikie via cfe-commits]

On Tue, May 7, 2019 at 7:40 PM Richard Trieu  wrote:
>
>
> From: David Blaikie 
> Date: Mon, May 6, 2019 at 4:39 PM
> To: Richard Trieu
> Cc: cfe-commits
>
>> On Mon, May 6, 2019 at 4:24 PM Richard Trieu  wrote:
>> >
>> > There was no cycle for this crash.
>>
>> Oh, yeah, didn't mean to imply there were - but that a system designed
>> to prevent cycles might also be used/help prevent redundant work like
>> this.
>>
>> > What happened is that an exponential runtime is reduced to a linear 
>> > runtime.  Without this revision, ODR hashing would have worked if the 
>> > machine had enough memory and the user waited long enough.
>> >
>> > void foo(int a, int b) {}
>> > When computing the ODR hash for function foo, it will visit the type int 
>> > twice, once per parameter.  In general, re-visiting types shouldn't be a 
>> > problem, and in most cases, should be pretty fast.
>>
>> It does mean some potentially problematic worst-case situations where
>> non-trivial types are mentioned more than once (eg: if, instead of
>> int, it was a complex struct type - it wouldn't cycle, but it would do
>> all that work twice (or many more times if it appears in more places
>> in the entity being hashed)
>
>
> See below in the answer to DWARF.  ODRHash did have a system, it worked for a 
> while until it didn't, and was since removed.
>>
>>
>> > class S {
>> >   void bar(S* s);
>> > };
>> > There's actually two ways to visit the Decl behind S, 
>> > ODR::AddCXXRecordDecl and ODR::AddDecl.  When computing the ODR hash of S, 
>> > ODR::AddCXXRecordDecl is used for a deep dive into the AST of S.  When 
>> > reaching S another way, (via FunctionDecl bar, parameter s, PointerType 
>> > S*, RecordType S), then the CXXRecordDecl gets processed through 
>> > ODR::AddDecl, which only processes enough information to identify S, but 
>> > not any of its deeper details.  This allows self-reference without 
>> > introducing cycles.
>>
>> Ah, OK - specifically to break the cycle.
>>
>> So the ODR hash of the function "void f(S*)" doesn't hash the
>> implementation of S, (it uses AddDecl, not AddCXXRecordDecl)? But if
>> it were "void f(S)" it would hash S? What about a member function that
>> takes a parameter by value? ("struct S { void bar(S); }")
>
>
> The three functions AddCXXRecordDecl, AddFunctionDecl, and AddEnumDecl are 
> the entry points from outside to use the ODRHash and nothing inside ODRHash 
> will call these functions.  That means hashing "class S {};"  
> AddCXXRecordDecl is called with S.  Every other example, "void f(S)", "void 
> bar(S);", etc will be called into AddDecl.  The next question is probably, 
> how do you know if two functions "void f(S)" in two files refer to same class 
> S?  The answer is, ODRHash doesn't know and doesn't care.  But when Clang 
> imports both "void f(S)" functions, it will also import both S classes.  
> Since Clang checks, ODRHash doesn't need to.

Clang checks this by entering ODRHash separately (via AddCXXRecordDecl).

So it's sort of an inductive proof based on the assumptions of the ODR
- >"void f(S)" is ODR compliant if S is ODR compliant<, etc.

That makes sense, though still does mean potentially a lot of
redundant work in cases like this bug, but never circular work -
because anonymous types can't form cycles (because they have no name
to do that).

I think it still /might/ be worth using a type stack/hash so there's
never redundant work, even if it's non-circular work.

>>
>>
>> > I think it would be possible to add some checks in debug mode to catch 
>> > cycles.  I'm not sure it can detect redundant work as the function foo 
>> > example above shows that visiting the same types over multiple times is 
>> > expected.
>>
>> Both for efficiency and to avoid these cycles, it might be worthwhile
>> to consider a different way to resolve this issue
>>
>> The reason these ideas come to my mind is that DWARF has a type hash
>> that works in a different way to avoid cycles and redundant work.
>>
>> http://dwarfstd.org/doc/DWARF5.pdf - 7.32, Type Signature Computation.
>> It works by assigning every type a number when it's first encountered
>> (even before its contents are hashed), and if it's ever encountered
>> again, hash the number again rather than going back into hashing the
>> implementation.
>>
> Originally, ODR hashing did have a system similar to what DWARF had.  
> Relevant portions of 7.32 are 1, 4.a, and 4.b.  Basically, maintain a list of 
> Type's, when first visiting a Type, add it to the list and process it, and if 
> the Type is ever seen again, use the index number instead reprocessing.  
> Worked well, and then the AST had a small change in it where now we needed 
> two different Type's to hash to the same thing.  
> https://reviews.llvm.org/rL335853 ripped this out.  It's possible to replace 
> it, but it needs to be better than what we currently have.

Ah - I think I understand/see. The uniqueness of Type*s is based on
the profile hashing and it varies in 

r360699 - Restore test files accidentally deleted in r354839

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Tue May 14 11:51:07 2019
New Revision: 360699

URL: http://llvm.org/viewvc/llvm-project?rev=360699=rev
Log:
Restore test files accidentally deleted in r354839

I think there must be a bug in git-llvm causing parent directories to be
deleted when the diff deletes files in a subdirectory. Perhaps it is
Windows-only.

There has been a behavior change, so some of these tests now fail. I
have marked them XFAIL and will fix them in a follow-up to separate the
changes.

Added:
cfe/trunk/test/ASTMerge/anonymous-fields/
cfe/trunk/test/ASTMerge/anonymous-fields/Inputs/
cfe/trunk/test/ASTMerge/anonymous-fields/Inputs/anonymous-fields1.cpp
cfe/trunk/test/ASTMerge/anonymous-fields/Inputs/anonymous-fields2.cpp
cfe/trunk/test/ASTMerge/anonymous-fields/test.cpp
cfe/trunk/test/ASTMerge/asm/
cfe/trunk/test/ASTMerge/asm/Inputs/
cfe/trunk/test/ASTMerge/asm/Inputs/asm-function.cpp
cfe/trunk/test/ASTMerge/asm/test.cpp
cfe/trunk/test/ASTMerge/category/
cfe/trunk/test/ASTMerge/category/Inputs/
cfe/trunk/test/ASTMerge/category/Inputs/category1.m
cfe/trunk/test/ASTMerge/category/Inputs/category2.m
cfe/trunk/test/ASTMerge/category/test.m
cfe/trunk/test/ASTMerge/class/
cfe/trunk/test/ASTMerge/class-template/
cfe/trunk/test/ASTMerge/class-template-partial-spec/
cfe/trunk/test/ASTMerge/class-template-partial-spec/Inputs/

cfe/trunk/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec1.cpp

cfe/trunk/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec2.cpp
cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp
cfe/trunk/test/ASTMerge/class-template/Inputs/
cfe/trunk/test/ASTMerge/class-template/Inputs/class-template1.cpp
cfe/trunk/test/ASTMerge/class-template/Inputs/class-template2.cpp
cfe/trunk/test/ASTMerge/class-template/test.cpp
cfe/trunk/test/ASTMerge/class/Inputs/
cfe/trunk/test/ASTMerge/class/Inputs/class1.cpp
cfe/trunk/test/ASTMerge/class/Inputs/class2.cpp
cfe/trunk/test/ASTMerge/class/test.cpp
cfe/trunk/test/ASTMerge/class2/
cfe/trunk/test/ASTMerge/class2/Inputs/
cfe/trunk/test/ASTMerge/class2/Inputs/class3.cpp
cfe/trunk/test/ASTMerge/class2/test.cpp
cfe/trunk/test/ASTMerge/codegen-body/
cfe/trunk/test/ASTMerge/codegen-body/Inputs/
cfe/trunk/test/ASTMerge/codegen-body/Inputs/body1.c
cfe/trunk/test/ASTMerge/codegen-body/Inputs/body2.c
cfe/trunk/test/ASTMerge/codegen-body/test.c
cfe/trunk/test/ASTMerge/codegen-exprs/
cfe/trunk/test/ASTMerge/codegen-exprs/Inputs/
cfe/trunk/test/ASTMerge/codegen-exprs/Inputs/exprs1.c
cfe/trunk/test/ASTMerge/codegen-exprs/Inputs/exprs2.c
cfe/trunk/test/ASTMerge/codegen-exprs/test.c
cfe/trunk/test/ASTMerge/enum/
cfe/trunk/test/ASTMerge/enum/Inputs/
cfe/trunk/test/ASTMerge/enum/Inputs/enum1.c
cfe/trunk/test/ASTMerge/enum/Inputs/enum2.c
cfe/trunk/test/ASTMerge/enum/test.c
cfe/trunk/test/ASTMerge/exprs/
cfe/trunk/test/ASTMerge/exprs-cpp/
cfe/trunk/test/ASTMerge/exprs-cpp/Inputs/
cfe/trunk/test/ASTMerge/exprs-cpp/Inputs/exprs3.cpp
cfe/trunk/test/ASTMerge/exprs-cpp/test.cpp
cfe/trunk/test/ASTMerge/exprs/Inputs/
cfe/trunk/test/ASTMerge/exprs/Inputs/exprs1.c
cfe/trunk/test/ASTMerge/exprs/Inputs/exprs2.c
cfe/trunk/test/ASTMerge/exprs/test.c
cfe/trunk/test/ASTMerge/function/
cfe/trunk/test/ASTMerge/function-cpp/
cfe/trunk/test/ASTMerge/function-cpp/Inputs/
cfe/trunk/test/ASTMerge/function-cpp/Inputs/function-1.cpp
cfe/trunk/test/ASTMerge/function-cpp/test.cpp
cfe/trunk/test/ASTMerge/function/Inputs/
cfe/trunk/test/ASTMerge/function/Inputs/function1.c
cfe/trunk/test/ASTMerge/function/Inputs/function2.c
cfe/trunk/test/ASTMerge/function/test.c
cfe/trunk/test/ASTMerge/inheritance/
cfe/trunk/test/ASTMerge/inheritance/Inputs/
cfe/trunk/test/ASTMerge/inheritance/Inputs/inheritance-base.cpp
cfe/trunk/test/ASTMerge/inheritance/test.cpp
cfe/trunk/test/ASTMerge/init-ctors/
cfe/trunk/test/ASTMerge/init-ctors/Inputs/
cfe/trunk/test/ASTMerge/init-ctors/Inputs/init-ctors-classes.cpp
cfe/trunk/test/ASTMerge/init-ctors/test.cpp
cfe/trunk/test/ASTMerge/injected-class-name-decl/
cfe/trunk/test/ASTMerge/injected-class-name-decl/Inputs/
cfe/trunk/test/ASTMerge/injected-class-name-decl/Inputs/inject1.cpp
cfe/trunk/test/ASTMerge/injected-class-name-decl/Inputs/inject2.cpp
cfe/trunk/test/ASTMerge/injected-class-name-decl/test.cpp
cfe/trunk/test/ASTMerge/interface/
cfe/trunk/test/ASTMerge/interface/Inputs/
cfe/trunk/test/ASTMerge/interface/Inputs/interface1.m
cfe/trunk/test/ASTMerge/interface/Inputs/interface2.m
cfe/trunk/test/ASTMerge/interface/test.m
cfe/trunk/test/ASTMerge/macro/
cfe/trunk/test/ASTMerge/macro/Inputs/
cfe/trunk/test/ASTMerge/macro/Inputs/macro.modulemap

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Remove the now redundant 'public:' spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59885

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Lex/Token.h
  clang/include/clang/Lex/TokenLexer.h
  clang/lib/Lex/PPCaching.cpp
  clang/lib/Lex/Preprocessor.cpp

Index: clang/lib/Lex/Preprocessor.cpp
===
--- clang/lib/Lex/Preprocessor.cpp
+++ clang/lib/Lex/Preprocessor.cpp
@@ -635,8 +635,7 @@
   CurTokenLexer->Lex(Tok);
   break;
 case CLK_CachingLexer:
-  bool IsNewToken;
-  CachingLex(Tok, IsNewToken);
+  CachingLex(Tok);
   break;
 case CLK_LexAfterModuleImport:
   LexAfterModuleImport(Tok);
@@ -880,22 +879,27 @@
   ++LexLevel;
 
   // We loop here until a lex function returns a token; this avoids recursion.
+  // FIXME: IsNewToken should be set inside lexers.
   bool ReturnedToken;
-  bool IsNewToken = true;
   do {
 switch (CurLexerKind) {
 case CLK_Lexer:
   ReturnedToken = CurLexer->Lex(Result);
+  Result.setFlag(Token::IsNewToken);
   break;
-case CLK_TokenLexer:
+case CLK_TokenLexer: {
+  bool IsNewToken = CurTokenLexer->isMacroExpansion();
   ReturnedToken = CurTokenLexer->Lex(Result);
+  Result.setFlagValue(Token::IsNewToken, IsNewToken);
   break;
+}
 case CLK_CachingLexer:
-  CachingLex(Result, IsNewToken);
+  CachingLex(Result);
   ReturnedToken = true;
   break;
 case CLK_LexAfterModuleImport:
   ReturnedToken = LexAfterModuleImport(Result);
+  Result.setFlag(Token::IsNewToken);
   break;
 }
   } while (!ReturnedToken);
@@ -911,7 +915,8 @@
 
   // Update ImportSeqState to track our position within a C++20 import-seq
   // if this token is being produced as a result of phase 4 of translation.
-  if (getLangOpts().CPlusPlusModules && LexLevel == 1 && IsNewToken) {
+  if (getLangOpts().CPlusPlusModules && LexLevel == 1 &&
+  Result.getFlag(Token::IsNewToken)) {
 switch (Result.getKind()) {
 case tok::l_paren: case tok::l_square: case tok::l_brace:
   ImportSeqState.handleOpenBracket();
@@ -952,6 +957,8 @@
 
   LastTokenWasAt = Result.is(tok::at);
   --LexLevel;
+  if (OnToken && LexLevel == 0 && Result.getFlag(Token::IsNewToken))
+OnToken(Result);
 }
 
 /// Lex a header-name token (including one formed from header-name-tokens if
Index: clang/lib/Lex/PPCaching.cpp
===
--- clang/lib/Lex/PPCaching.cpp
+++ clang/lib/Lex/PPCaching.cpp
@@ -45,7 +45,7 @@
   recomputeCurLexerKind();
 }
 
-void Preprocessor::CachingLex(Token , bool ) {
+void Preprocessor::CachingLex(Token ) {
   if (!InCachingLexMode())
 return;
 
@@ -55,7 +55,8 @@
 
   if (CachedLexPos < CachedTokens.size()) {
 Result = CachedTokens[CachedLexPos++];
-IsNewToken = false;
+// FIXME: do this flag when writing to CachedTokens.
+Result.clearFlag(Token::IsNewToken);
 return;
   }
 
Index: clang/include/clang/Lex/TokenLexer.h
===
--- clang/include/clang/Lex/TokenLexer.h
+++ clang/include/clang/Lex/TokenLexer.h
@@ -147,6 +147,10 @@
   /// preprocessor directive.
   bool isParsingPreprocessorDirective() const;
 
+  /// Returns true iff the TokenLexer is expanding a macro and not replaying a
+  /// stream of tokens.
+  bool isMacroExpansion() const { return Macro != nullptr; }
+
 private:
   void destroy();
 
Index: clang/include/clang/Lex/Token.h
===
--- clang/include/clang/Lex/Token.h
+++ clang/include/clang/Lex/Token.h
@@ -70,20 +70,22 @@
 public:
   // Various flags set per token:
   enum TokenFlags {
-StartOfLine   = 0x01,  // At start of line or only after whitespace
-   // (considering the line after macro expansion).
-LeadingSpace  = 0x02,  // Whitespace exists before this token (considering
-   // whitespace after macro expansion).
-DisableExpand = 0x04,  // This identifier may never be macro expanded.
-NeedsCleaning = 0x08,  // Contained an escaped newline or trigraph.
+StartOfLine = 0x01,   // At start of line or only after whitespace
+  // (considering the line after macro expansion).
+LeadingSpace = 0x02,  // Whitespace exists before this token (considering
+  // whitespace after macro expansion).
+DisableExpand = 0x04, // This identifier may never be macro expanded.
+NeedsCleaning = 0x08, // Contained an escaped newline or trigraph.
 LeadingEmptyMacro = 0x10, // Empty macro exists before this token.
-HasUDSuffix = 0x20,// This string or character literal has a ud-suffix.
-HasUCN = 0x40,

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The suggested approach looks promising. The difference seems to be within the 
noise levels on my machine:

  Before the change (baseline upstream revision):
Time (mean ± σ): 159.1 ms ±   8.7 ms[User: 138.3 ms, System: 20.6 
ms]
Range (min … max):   150.4 ms … 196.2 ms100 runs
  
  After (no callback specified):
Time (mean ± σ): 160.4 ms ±   7.6 ms[User: 138.5 ms, System: 21.7 
ms]
Range (min … max):   151.0 ms … 191.5 ms100 runs

I'm sending out a prototype I used for measures for review. The flag is 
currently set by the preprocessor, but I guess it would make more sense to do 
the following before landing this:

- flip the flag, i.e. instead of reporting whether the token is "new" (the 
common case), report whether it's "cached", i.e. coming from `CachedTokens` or 
from a non-macro-expansion token stream.
- set this flag only inside `CachingLex` and `TokenLexer`.

The revision also removes `IsNewToken` from `CachingLex` and sets the flag in 
the token instead.
Any other suggestions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59885



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

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Use a flag inside clang::Token instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59885

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Lex/Token.h
  clang/include/clang/Lex/TokenLexer.h
  clang/lib/Lex/PPCaching.cpp
  clang/lib/Lex/Preprocessor.cpp

Index: clang/lib/Lex/Preprocessor.cpp
===
--- clang/lib/Lex/Preprocessor.cpp
+++ clang/lib/Lex/Preprocessor.cpp
@@ -635,8 +635,7 @@
   CurTokenLexer->Lex(Tok);
   break;
 case CLK_CachingLexer:
-  bool IsNewToken;
-  CachingLex(Tok, IsNewToken);
+  CachingLex(Tok);
   break;
 case CLK_LexAfterModuleImport:
   LexAfterModuleImport(Tok);
@@ -880,22 +879,27 @@
   ++LexLevel;
 
   // We loop here until a lex function returns a token; this avoids recursion.
+  // FIXME: IsNewToken should be set inside lexers.
   bool ReturnedToken;
-  bool IsNewToken = true;
   do {
 switch (CurLexerKind) {
 case CLK_Lexer:
   ReturnedToken = CurLexer->Lex(Result);
+  Result.setFlag(Token::IsNewToken);
   break;
-case CLK_TokenLexer:
+case CLK_TokenLexer: {
+  bool IsNewToken = CurTokenLexer->isMacroExpansion();
   ReturnedToken = CurTokenLexer->Lex(Result);
+  Result.setFlagValue(Token::IsNewToken, IsNewToken);
   break;
+}
 case CLK_CachingLexer:
-  CachingLex(Result, IsNewToken);
+  CachingLex(Result);
   ReturnedToken = true;
   break;
 case CLK_LexAfterModuleImport:
   ReturnedToken = LexAfterModuleImport(Result);
+  Result.setFlag(Token::IsNewToken);
   break;
 }
   } while (!ReturnedToken);
@@ -911,7 +915,8 @@
 
   // Update ImportSeqState to track our position within a C++20 import-seq
   // if this token is being produced as a result of phase 4 of translation.
-  if (getLangOpts().CPlusPlusModules && LexLevel == 1 && IsNewToken) {
+  if (getLangOpts().CPlusPlusModules && LexLevel == 1 &&
+  Result.getFlag(Token::IsNewToken)) {
 switch (Result.getKind()) {
 case tok::l_paren: case tok::l_square: case tok::l_brace:
   ImportSeqState.handleOpenBracket();
@@ -952,6 +957,8 @@
 
   LastTokenWasAt = Result.is(tok::at);
   --LexLevel;
+  if (OnToken && LexLevel == 0 && Result.getFlag(Token::IsNewToken))
+OnToken(Result);
 }
 
 /// Lex a header-name token (including one formed from header-name-tokens if
Index: clang/lib/Lex/PPCaching.cpp
===
--- clang/lib/Lex/PPCaching.cpp
+++ clang/lib/Lex/PPCaching.cpp
@@ -45,7 +45,7 @@
   recomputeCurLexerKind();
 }
 
-void Preprocessor::CachingLex(Token , bool ) {
+void Preprocessor::CachingLex(Token ) {
   if (!InCachingLexMode())
 return;
 
@@ -55,7 +55,8 @@
 
   if (CachedLexPos < CachedTokens.size()) {
 Result = CachedTokens[CachedLexPos++];
-IsNewToken = false;
+// FIXME: do this flag when writing to CachedTokens.
+Result.clearFlag(Token::IsNewToken);
 return;
   }
 
Index: clang/include/clang/Lex/TokenLexer.h
===
--- clang/include/clang/Lex/TokenLexer.h
+++ clang/include/clang/Lex/TokenLexer.h
@@ -147,6 +147,10 @@
   /// preprocessor directive.
   bool isParsingPreprocessorDirective() const;
 
+  /// Returns true iff the TokenLexer is expanding a macro and not replaying a
+  /// stream of tokens.
+  bool isMacroExpansion() const { return Macro != nullptr; }
+
 private:
   void destroy();
 
Index: clang/include/clang/Lex/Token.h
===
--- clang/include/clang/Lex/Token.h
+++ clang/include/clang/Lex/Token.h
@@ -70,20 +70,22 @@
 public:
   // Various flags set per token:
   enum TokenFlags {
-StartOfLine   = 0x01,  // At start of line or only after whitespace
-   // (considering the line after macro expansion).
-LeadingSpace  = 0x02,  // Whitespace exists before this token (considering
-   // whitespace after macro expansion).
-DisableExpand = 0x04,  // This identifier may never be macro expanded.
-NeedsCleaning = 0x08,  // Contained an escaped newline or trigraph.
+StartOfLine = 0x01,   // At start of line or only after whitespace
+  // (considering the line after macro expansion).
+LeadingSpace = 0x02,  // Whitespace exists before this token (considering
+  // whitespace after macro expansion).
+DisableExpand = 0x04, // This identifier may never be macro expanded.
+NeedsCleaning = 0x08, // Contained an escaped newline or trigraph.
 LeadingEmptyMacro = 0x10, // Empty macro exists before this token.
-HasUDSuffix = 0x20,// This string or character literal has a ud-suffix.
-HasUCN = 0x40,  

[PATCH] D61874: [clang-tidy] Fix invalid fixit for readability-static-accessed-through-instance (bug 40544)

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360698: [clang-tidy] Fix invalid fixit for 
readability-static-accessed-through-instance… (authored by mgehre, committed by 
).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61874?vs=199333=199484#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61874

Files:
  
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
  
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp


Index: 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
===
--- 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
+++ 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
@@ -67,6 +67,7 @@
   const ASTContext *AstContext = Result.Context;
   PrintingPolicy PrintingPolicyWithSupressedTag(AstContext->getLangOpts());
   PrintingPolicyWithSupressedTag.SuppressTagKeyword = true;
+  PrintingPolicyWithSupressedTag.SuppressUnwrittenScope = true;
   std::string BaseTypeName =
   BaseType.getAsString(PrintingPolicyWithSupressedTag);
 
Index: 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -220,3 +220,31 @@
   qp->y = 10; // OK, the overloaded operator might have side-effects.
   qp->K = 10; //
 }
+
+namespace {
+  struct Anonymous {
+static int I;
+  };
+}
+
+void use_anonymous() {
+  Anonymous Anon;
+  Anon.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Anonymous::I;{{$}}
+}
+
+namespace Outer {
+  inline namespace Inline {
+struct S {
+  static int I;
+};
+  }
+}
+
+void use_inline() {
+  Outer::S V;
+  V.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Outer::S::I;{{$}}
+}


Index: clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
@@ -67,6 +67,7 @@
   const ASTContext *AstContext = Result.Context;
   PrintingPolicy PrintingPolicyWithSupressedTag(AstContext->getLangOpts());
   PrintingPolicyWithSupressedTag.SuppressTagKeyword = true;
+  PrintingPolicyWithSupressedTag.SuppressUnwrittenScope = true;
   std::string BaseTypeName =
   BaseType.getAsString(PrintingPolicyWithSupressedTag);
 
Index: clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -220,3 +220,31 @@
   qp->y = 10; // OK, the overloaded operator might have side-effects.
   qp->K = 10; //
 }
+
+namespace {
+  struct Anonymous {
+static int I;
+  };
+}
+
+void use_anonymous() {
+  Anonymous Anon;
+  Anon.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Anonymous::I;{{$}}
+}
+
+namespace Outer {
+  inline namespace Inline {
+struct S {
+  static int I;
+};
+  }
+}
+
+void use_inline() {
+  Outer::S V;
+  V.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Outer::S::I;{{$}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r360698 - [clang-tidy] Fix invalid fixit for readability-static-accessed-through-instance (bug 40544)

[Matthias Gehre via cfe-commits]

Author: mgehre
Date: Tue May 14 11:23:10 2019
New Revision: 360698

URL: http://llvm.org/viewvc/llvm-project?rev=360698=rev
Log:
[clang-tidy] Fix invalid fixit for readability-static-accessed-through-instance 
(bug 40544)

Summary:
Fixed https://bugs.llvm.org/show_bug.cgi?id=40544
Before, we would generate a fixit like `(anonymous namespace)::Foo::fun();` for
the added test case.

Reviewers: aaron.ballman, alexfh, xazax.hun

Subscribers: rnkovacs, cfe-commits

Tags: #clang, #clang-tools-extra

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

Modified:

clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp?rev=360698=360697=360698=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
 (original)
+++ 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
 Tue May 14 11:23:10 2019
@@ -67,6 +67,7 @@ void StaticAccessedThroughInstanceCheck:
   const ASTContext *AstContext = Result.Context;
   PrintingPolicy PrintingPolicyWithSupressedTag(AstContext->getLangOpts());
   PrintingPolicyWithSupressedTag.SuppressTagKeyword = true;
+  PrintingPolicyWithSupressedTag.SuppressUnwrittenScope = true;
   std::string BaseTypeName =
   BaseType.getAsString(PrintingPolicyWithSupressedTag);
 

Modified: 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp?rev=360698=360697=360698=diff
==
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
 (original)
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
 Tue May 14 11:23:10 2019
@@ -220,3 +220,31 @@ int func(Qptr qp) {
   qp->y = 10; // OK, the overloaded operator might have side-effects.
   qp->K = 10; //
 }
+
+namespace {
+  struct Anonymous {
+static int I;
+  };
+}
+
+void use_anonymous() {
+  Anonymous Anon;
+  Anon.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Anonymous::I;{{$}}
+}
+
+namespace Outer {
+  inline namespace Inline {
+struct S {
+  static int I;
+};
+  }
+}
+
+void use_inline() {
+  Outer::S V;
+  V.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Outer::S::I;{{$}}
+}


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

[PATCH] D61909: Add Clang shared library with C++ exports

[Wink Saville via Phabricator via cfe-commits]

winksaville added a comment.

You mention that you're using OBJECT libraries so objects aren't built 
mutliples times
and in my current tests the number of steps increased by only 3, it went from 
4353 to 4356,
when using this patch, which is great!

What I see in my testing of the patch is that cmake finds that the compilers 
support position independent code and use `-fPIC` as default:

  -- LLVM default target triple: x86_64-unknown-linux-gnu
  -- Performing Test C_SUPPORTS_FPIC
  -- Performing Test C_SUPPORTS_FPIC - Success
  -- Performing Test CXX_SUPPORTS_FPIC
  -- Performing Test CXX_SUPPORTS_FPIC - Success
  -- Building with -fPIC

And then, as expected, I see the compile command lines have `-fPIC`:

  [1/4356] /usr/bin/c++  -DGTEST_HAS_RTTI=0 ... -fPIC ...

Its my understanding that when creating shared libraries position independent 
code generation is required.
Is there a problem if in some scenario the OBJECT libraries were not compiled 
with `-fPIC`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Torbjörn Klatt via Phabricator via cfe-commits]

torbjoernk updated this revision to Diff 199483.
torbjoernk edited the summary of this revision.
torbjoernk added a comment.
Herald added a subscriber: arphaman.

Addressed Roman Lebedev's comment regarding reference to `NOLINT` in the docs.

As I don't have commit rights, I'd be grateful someone else could commit this 
patch once it's fully accepted.


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

https://reviews.llvm.org/D61827

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp

Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -776,17 +776,20 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
@@ -991,18 +994,26 @@
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = []() { int R = I; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -369,7 +369,7 @@
 // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
@@ -431,19 +431,22 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst

[PATCH] D61909: Add Clang shared library with C++ exports

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

Thanks for working on this, I have wanted something like this for a while.

It would also be nice to have a CLANG_LINK_CLANG_DYLIB option like we have for 
llvm, but this can be a follow on patch, and I would be happy to help with this 
if needed.




Comment at: clang/tools/clang-shlib/CMakeLists.txt:8
+
+add_clang_library(Clang_shared
+  SHARED

Is this capital 'C' to distinguish it from the other libclang.so ?  I would 
prfer to use lowercase 'c' here instead, so it fit the naming convention for 
the other libraries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909



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

Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways

[David Blaikie via cfe-commits]

Fair enough - since they're already there I don't feel super serious
about converging on one (though I probably wouldn't've been in favor
of adding a second spelling at the point it was proposed).

On Tue, May 14, 2019 at 8:03 AM  wrote:
>
> There's no practical difference.  I view it as a matter of documentation of 
> intent, see my commit log comment for r360603.
>
> Arguably we could eliminate UNSUPPORTED and move all the expressions into 
> REQUIRES (appropriately negated), but I'm not sure that's a net win for 
> readability.
>
> --paulr
>
>
>
> From: David Blaikie [mailto:dblai...@gmail.com]
> Sent: Monday, May 13, 2019 10:48 AM
> To: Robinson, Paul
> Cc: cfe-commits
> Subject: Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we 
> don't need two ways
>
>
>
> What's the practical difference between "UNSUPPORTED: foo" and "REQUIRES: 
> !foo"? (I see some of the fixes you've made go one way, some the other way)
>
>
>
> On Fri, May 10, 2019 at 11:30 AM Paul Robinson via cfe-commits 
>  wrote:
>
> Author: probinson
> Date: Fri May 10 11:32:53 2019
> New Revision: 360452
>
> URL: http://llvm.org/viewvc/llvm-project?rev=360452=rev
> Log:
> Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways
> to say the same thing.
>
> Modified:
> cfe/trunk/test/Driver/nozlibcompress.c
>
> Modified: cfe/trunk/test/Driver/nozlibcompress.c
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/nozlibcompress.c?rev=360452=360451=360452=diff
> ==
> --- cfe/trunk/test/Driver/nozlibcompress.c (original)
> +++ cfe/trunk/test/Driver/nozlibcompress.c Fri May 10 11:32:53 2019
> @@ -1,4 +1,4 @@
> -// REQUIRES: nozlib
> +// REQUIRES: !zlib
>
>  // RUN: %clang -### -fintegrated-as -gz -c %s 2>&1 | FileCheck %s 
> -check-prefix CHECK-WARN
>  // RUN: %clang -### -fintegrated-as -gz=none -c %s 2>&1 | FileCheck 
> -allow-empty -check-prefix CHECK-NOWARN %s
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 199476.
ymandel marked 18 inline comments as done.
ymandel added a comment.
Herald added a subscriber: jfb.

responded to comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -116,7 +116,8 @@
 };
   }
 
-  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+  template 
+  void testRule(R Rule, StringRef Input, StringRef Expected) {
 Transformer T(std::move(Rule), consumer());
 T.registerMatchers();
 compareSnippets(Expected, rewrite(Input));
@@ -147,7 +148,7 @@
  .bind(StringExpr)),
   callee(cxxMethodDecl(hasName("c_str")),
   change("REPLACED"));
-  R.Explanation = text("Use size() method directly on string.");
+  R.Cases[0].Explanation = text("Use size() method directly on string.");
   return R;
 }
 
@@ -375,6 +376,92 @@
Input, Expected);
 }
 
+TEST_F(TransformerTest, OrderedRuleUnrelated) {
+  StringRef Flag = "flag";
+  RewriteRule FlagRule = makeRule(
+  cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
+hasName("proto::ProtoCommandLineFlag"
+   .bind(Flag)),
+unless(callee(cxxMethodDecl(hasName("GetProto"),
+  change(Flag, "PROTO"));
+
+  std::string Input = R"cc(
+proto::ProtoCommandLineFlag flag;
+int x = flag.foo();
+int y = flag.GetProto().foo();
+int f(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+proto::ProtoCommandLineFlag flag;
+int x = PROTO.foo();
+int y = flag.GetProto().foo();
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected);
+}
+
+// Version of ruleStrlenSizeAny that inserts a method with a different name than
+// ruleStrlenSize, so we can tell their effect apart.
+RewriteRule ruleStrlenSizeDistinct() {
+  StringRef S;
+  return makeRule(
+  callExpr(callee(functionDecl(hasName("strlen"))),
+   hasArgument(0, cxxMemberCallExpr(
+  on(expr().bind(S)),
+  callee(cxxMethodDecl(hasName("c_str")),
+  change("DISTINCT"));
+}
+
+TEST_F(TransformerTest, OrderedRuleRelated) {
+  std::string Input = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return strlen(s.c_str()); }
+}  // namespace foo
+int g(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return DISTINCT; }
+}  // namespace foo
+int g(string s) { return REPLACED; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input,
+   Expected);
+}
+
+// Change the order of the rules to get a different result.
+TEST_F(TransformerTest, OrderedRuleRelatedSwapped) {
+  std::string Input = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return strlen(s.c_str()); }
+}  // namespace foo
+int g(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return DISTINCT; }
+}  // namespace foo
+int g(string s) { return DISTINCT; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input,
+   Expected);
+}
+
 //
 // Negative tests (where we expect no transformation to occur).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -28,6 +28,7 @@
 using namespace tooling;
 
 using ast_matchers::MatchFinder;
+using ast_matchers::internal::DynTypedMatcher;
 using ast_type_traits::ASTNodeKind;
 using ast_type_traits::DynTypedNode;
 using llvm::Error;
@@ -144,9 +145,9 @@
   llvm_unreachable("Unexpected case in NodePart type.");
 }
 
-Expected>
-tooling::translateEdits(const MatchResult ,
-llvm::ArrayRef Edits) {
+Expected>
+tooling::detail::translateEdits(const MatchResult ,
+llvm::ArrayRef Edits) {
   SmallVector Transformations;
   auto  = Result.Nodes.getMap();
   for (const auto  : Edits) {
@@ -171,18 +172,113 @@
  

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > > All of the rules must use the same kind of matcher
> > > 
> > > Could you elaborate why we want this restriction?
> > > Why they can't be of different kinds? 
> > > 
> > > It feels ok to have transformations to completely unrelated nodes and 
> > > still apply the first one.
> > > 
> > Agreed. The problem is purely from the implementation perspective. Since 
> > anyOf() enforces this restriction, I need to either
> > 1. pass it on to the user
> > 2. infer the base kind of each matcher and place them in the appropriate 
> > bucket.
> > 
> > I figured for a first pass, we'd go with 1. if you disagree, I can add the 
> > logic to bucket them and thereby support arbitrary combinations. FWIW, I 
> > think it's a desirable feature, so its not wasted work. its just a matter 
> > of splitting up the work.
> Ah, ok, so this is the restriction of `anyOf`.
indeed -- and all the other variadoc operations.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > I wonder if there a better way to construct an `anyOf` matcher that can 
> > > tell which case matched...
> > > It looks to be the reason for a more complicated interface on the 
> > > transformer side, but I'm not sure there's a way out of it.
> > > 
> > > Any ideas?
> > I don't quite follow. Which interface complication are you referring to?  
> > FWIW, the reason the code here doesn't just use the anyOf() matches is that 
> > it doesn't take a vector -- it only has a variadic form.
> E.g. the restriction that the matchers should have a common kind seems to 
> come from the fact that we need to later find out which case matched.
> 
> It this a limitation of the AST matchers design? E.g. I can't match both a 
> type and an expression and bind different subnodes in each submatcher, right?
> E.g. the restriction that the matchers should have a common kind seems to 
> come from the fact that we need to later find out which case matched.
>
> It this a limitation of the AST matchers design? E.g. I can't match both a 
> type and an expression and bind different subnodes in each submatcher, right?

Yes, as far as I can tell, but I don't think its connected to the binding -- 
every `DynTypedMatcher` needs to report what kind it supports.  IIRC, this is 
connected to the desire to specialize matchers to types to provide some static 
checking for matchers.  Since there is no universal/root AST type, there's no 
root kind, and we're stuck w/ this restriction.  If we were willing for the 
`Matcher` class to diverge from the AST, we could add a "universal" node kind, 
but that's another discussion...

In practice, this is mitigated in matchers by forcing the user to call a 
different `addMatcher` for each kind. But, that won't work for us since we want 
to (ultimately) support rules of different (base) kinds.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335



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

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Done with first round.




Comment at: lib/AST/ASTImporter.cpp:1643
+if (!ImportedOrErr) {
+  // For RecordDecls, failed import of a field will break the layout of the
+  // structure. Handle it as an error.

What cases are failed import of a field ok? Is that because we will get the 
field later on by another path?



Comment at: lib/AST/ASTImporter.cpp:1655
+  // Reorder declarations in RecordDecls because they may have another
+  // order. Keeping field order is vitable because it determines structure
+  // layout.

vitable?



Comment at: lib/AST/ASTImporter.cpp:1663
+
+
+  // NOTE: Here and below, we cannot call field_begin() method and its callers

Maybe a comment here explaining the purpose of the loops below. IIUC removing 
fields since they may have been imported in the wrong order and then adding 
them back in what should be the correct order now.



Comment at: lib/AST/ASTImporter.cpp:1674
+  Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+  assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+  ToRD->removeDecl(ToD);

Are we sure `ToD` is never a `nullptr` b/c you are unconditionally derefenecing 
it here.



Comment at: lib/AST/ASTImporter.cpp:1679
+
+  if (!ToRD->hasExternalLexicalStorage())
+assert(ToRD->field_empty());

Can you add a comment explaining why if the Decl has ExternalLexicalStorage the 
fields might not be empty even though we just removed them?


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100



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

Re: r360637 - PR41817: Fix regression in r359260 that caused the MS compatibility

[Richard Smith via cfe-commits]

On Tue, 14 May 2019, 03:09 Hans Wennborg via cfe-commits, <
cfe-commits@lists.llvm.org> wrote:

> Actually, we didn't notice r359260 in Chromium, however this one
> (r360637) caused Clang to start asserting during our builds
> (https://bugs.chromium.org/p/chromium/issues/detail?id=962840), here's
> a reduced test case:
>
> --
> extern int a;
> static int *use1 = 
> int **use2 = 
> static int a = 0;
> --
>
> $ clang.bad -cc1 -triple i386-pc-windows-msvc19.11.0 -emit-obj
> -fms-extensions /tmp/a.cc
> clang.bad: /work/llvm.monorepo/clang/lib/AST/Decl.cpp:1494:
> clang::LinkageInfo clang::LinkageComputer::getLVForDecl(const
> clang::NamedDecl*, clang::LVComputationKind): Assertion
> `D->getCachedLinkage() == LV.getLinkage()' failed.
>
>
> I've reverted this one in r360657 in the meantime.
>

Yep, I'm not at all surprised. Perhaps we should stop claiming to support
this extension, given that it fundamentally violates assumptions made by
clang (that linkage doesn't change after the first declaration). Presumably
we instead used to miscompile the example you give above (and after the
revert, miscompile it again now)?

From: Richard Smith via cfe-commits 
> Date: Tue, May 14, 2019 at 2:24 AM
> To: 
>
> > Author: rsmith
> > Date: Mon May 13 17:27:16 2019
> > New Revision: 360637
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=360637=rev
> > Log:
> > PR41817: Fix regression in r359260 that caused the MS compatibility
> > extension allowing a "static" declaration to follow an "extern"
> > declaration to stop working.
> >
> > Added:
> > cfe/trunk/test/CodeGen/ms-compat-extern-static.c
> > Modified:
> > cfe/trunk/lib/AST/Decl.cpp
> >
> > Modified: cfe/trunk/lib/AST/Decl.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=360637=360636=360637=diff
> >
> ==
> > --- cfe/trunk/lib/AST/Decl.cpp (original)
> > +++ cfe/trunk/lib/AST/Decl.cpp Mon May 13 17:27:16 2019
> > @@ -613,12 +613,41 @@ static LinkageInfo getExternalLinkageFor
> >  static StorageClass getStorageClass(const Decl *D) {
> >if (auto *TD = dyn_cast(D))
> >  D = TD->getTemplatedDecl();
> > -  if (D) {
> > -if (auto *VD = dyn_cast(D))
> > -  return VD->getStorageClass();
> > -if (auto *FD = dyn_cast(D))
> > -  return FD->getStorageClass();
> > +  if (!D)
> > +return SC_None;
> > +
> > +  if (auto *VD = dyn_cast(D)) {
> > +// Generally, the storage class is determined by the first
> declaration.
> > +auto SC = VD->getCanonicalDecl()->getStorageClass();
> > +
> > +// ... except that MSVC permits an 'extern' declaration to be
> redeclared
> > +// 'static' as an extension.
> > +if (SC == SC_Extern) {
> > +  for (auto *Redecl : VD->redecls()) {
> > +if (Redecl->getStorageClass() == SC_Static)
> > +  return SC_Static;
> > +if (Redecl->getStorageClass() != SC_Extern &&
> > +!Redecl->isLocalExternDecl() &&
> !Redecl->getFriendObjectKind())
> > +  break;
> > +  }
> > +}
> > +return SC;
> >}
> > +
> > +  if (auto *FD = dyn_cast(D)) {
> > +auto SC = FD->getCanonicalDecl()->getStorageClass();
> > +if (SC == SC_Extern) {
> > +  for (auto *Redecl : FD->redecls()) {
> > +if (Redecl->getStorageClass() == SC_Static)
> > +  return SC_Static;
> > +if (Redecl->getStorageClass() != SC_Extern &&
> > +!Redecl->isLocalExternDecl() &&
> !Redecl->getFriendObjectKind())
> > +  break;
> > +  }
> > +}
> > +return SC;
> > +  }
> > +
> >return SC_None;
> >  }
> >
> > @@ -634,7 +663,7 @@ LinkageComputer::getLVForNamespaceScopeD
> >//   A name having namespace scope (3.3.6) has internal linkage if it
> >//   is the name of
> >
> > -  if (getStorageClass(D->getCanonicalDecl()) == SC_Static) {
> > +  if (getStorageClass(D) == SC_Static) {
> >  // - a variable, variable template, function, or function template
> >  //   that is explicitly declared static; or
> >  // (This bullet corresponds to C99 6.2.2p3.)
> >
> > Added: cfe/trunk/test/CodeGen/ms-compat-extern-static.c
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-compat-extern-static.c?rev=360637=auto
> >
> ==
> > --- cfe/trunk/test/CodeGen/ms-compat-extern-static.c (added)
> > +++ cfe/trunk/test/CodeGen/ms-compat-extern-static.c Mon May 13 17:27:16
> 2019
> > @@ -0,0 +1,11 @@
> > +// RUN: %clang_cc1 -emit-llvm %s -o - -fms-extensions -triple
> x86_64-windows | FileCheck %s
> > +
> > +// CHECK: @n = internal global i32 1
> > +extern int n;
> > +static int n = 1;
> > +int *use = 
> > +
> > +// CHECK: define internal void @f(
> > +extern void f();
> > +static void f() {}
> > +void g() { return f(); }
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > 

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

Thanks for doing this. I think module flag is a good idea. Some comments inline.




Comment at: clang/test/CodeGen/svml-calls.ll:16
+
+define void @sin_f64(double* nocapture %varray) {
+; CHECK-LABEL: @sin_f64(

Personally, I think codegen tests like this will be cleaner to keep in LLVM. 
Clang tests just test the IRGen of the module flag and LLVM tests check that 
those flags are respected and module flag merge is respected.



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module , TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

Should this be done not just for LTOBackend but for regular compilation as 
well? LegacyCodegenerator and llc can all be benefit from a matching 
TargetLibraryInfo?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Thank you guys for the review!

In D61438#1501457 , @aprantl wrote:

> Alright, thanks for taking the time to walk me through the thought process!





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The implementation LG, thanks! Just a few NITs and comments about the public 
interface and the comments




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ymandel wrote:
> ilya-biryukov wrote:
> > > All of the rules must use the same kind of matcher
> > 
> > Could you elaborate why we want this restriction?
> > Why they can't be of different kinds? 
> > 
> > It feels ok to have transformations to completely unrelated nodes and still 
> > apply the first one.
> > 
> Agreed. The problem is purely from the implementation perspective. Since 
> anyOf() enforces this restriction, I need to either
> 1. pass it on to the user
> 2. infer the base kind of each matcher and place them in the appropriate 
> bucket.
> 
> I figured for a first pass, we'd go with 1. if you disagree, I can add the 
> logic to bucket them and thereby support arbitrary combinations. FWIW, I 
> think it's a desirable feature, so its not wasted work. its just a matter of 
> splitting up the work.
Ah, ok, so this is the restriction of `anyOf`.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef Rules);
+

ymandel wrote:
> ilya-biryukov wrote:
> > Any ideas for a better name? `pickFirst`  or `applyFirst`?
> Yes, those are both better. Went w/ `applyFirst`.  Also considered 
> `applyFirstMatch` but not sure that buys much clarity
`applyFirst` looks good, thanks!



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c

ymandel wrote:
> ilya-biryukov wrote:
> > Could they be made private? If not, why?
> I tried to clarify this. But, I'm also fine splitting these three into a 
> separate header file or moving them to the bottom of this one.  They're only 
> exposed at all because TransformerTidy lives in a different directory and 
> namespace.  WDYT?
I see. Maybe additionally put them into `namespace detail`?
A good way to make sure that any use is a red herring. (Otherwise clients would 
need to read the code to realize it's an internal function.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:195
 
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node.

NIT: Maybe shorten a bit? Something like

```
Applies the first rule whose pattern matches, other rules are ignored
```


The current version has a bit too many details, so it's hard to grasp on a 
first read.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:215
+//
+// Ordered rules allow us to specify each independently:
+// ```

NIT: s/Ordered rules allow us/applyFirst allows to... ?

With a new name for the function, "ordered" rules can be confusing



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:231
+// ```
+// More generally, anywhere you'd use anyOf(m1.bind("m1"), m2.bind("m2")) and
+// then dispatch on those tags in your code to decide what to do, we'll lift

Maybe start with this? It's a great analogy. Something like this at the start 
of the comment would be great:
```
`applyFirst` is similar to `anyOf` matcher with a replace action attached to 
each of its cases...
```



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:239
+  for (auto  : Rules) {
+R.Cases.append(Rule.Cases.begin(), Rule.Cases.end());
+  }

NIT: remove redundant `{}`



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:262
+
+// Finds the rule that was "selected" -- that is, whose matcher triggered the
+// `MatchResult`.

NIT:s/Finds the rule/Finds the case
Would help avoid potential confusion...



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ymandel wrote:
> ilya-biryukov wrote:
> > I wonder if there a better way to construct an `anyOf` matcher that can 
> > tell which case matched...
> > It looks to be the reason for a more complicated interface on the 
> > transformer side, but I'm not sure there's a way out of it.
> > 
> > Any ideas?
> I don't quite follow. Which interface 

[PATCH] D61790: [C++20] add consteval specifier

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

"complex" in this context is the C99 `_Complex`, which is supported in C++ as 
an extension, using the same syntax as C. You can just declare a variable with 
type `_Complex float`.  If you need to manipulate the real and imaginary parts 
of a variable, you can use the gcc extension `__real__`/`__imag__`, although I 
don't think you need to.


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

https://reviews.llvm.org/D61790



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 9 inline comments as done and an inline comment as not done.
ymandel added a comment.

Thanks for the review. PTAL.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ilya-biryukov wrote:
> > All of the rules must use the same kind of matcher
> 
> Could you elaborate why we want this restriction?
> Why they can't be of different kinds? 
> 
> It feels ok to have transformations to completely unrelated nodes and still 
> apply the first one.
> 
Agreed. The problem is purely from the implementation perspective. Since 
anyOf() enforces this restriction, I need to either
1. pass it on to the user
2. infer the base kind of each matcher and place them in the appropriate bucket.

I figured for a first pass, we'd go with 1. if you disagree, I can add the 
logic to bucket them and thereby support arbitrary combinations. FWIW, I think 
it's a desirable feature, so its not wasted work. its just a matter of 
splitting up the work.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:224
+//
+// RewriteRule R = makeOrderedRule({makeRule(two_calls, two_calls_action)},
+// {makeRule(left_call, left_call_action)},

ilya-biryukov wrote:
> Why would we need braces around each call? Aren't they a rewrite rule in 
> themselves?
typo. thanks!



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef Rules);
+

ilya-biryukov wrote:
> Any ideas for a better name? `pickFirst`  or `applyFirst`?
Yes, those are both better. Went w/ `applyFirst`.  Also considered 
`applyFirstMatch` but not sure that buys much clarity



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c

ilya-biryukov wrote:
> Could they be made private? If not, why?
I tried to clarify this. But, I'm also fine splitting these three into a 
separate header file or moving them to the bottom of this one.  They're only 
exposed at all because TransformerTidy lives in a different directory and 
namespace.  WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335



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

[PATCH] D61804: Support building shared and static libraries simultaneously

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

@winksaville that was my bad. I left off the new files in the commit because I 
forgot `git-diff` doesn't grab untracked files...

I've uploaded the patch as a D61909 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61804



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 199465.
ymandel edited the summary of this revision.
ymandel added a comment.

Response to comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -116,7 +116,8 @@
 };
   }
 
-  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+  template 
+  void testRule(R Rule, StringRef Input, StringRef Expected) {
 Transformer T(std::move(Rule), consumer());
 T.registerMatchers();
 compareSnippets(Expected, rewrite(Input));
@@ -147,7 +148,7 @@
  .bind(StringExpr)),
   callee(cxxMethodDecl(hasName("c_str")),
   change("REPLACED"));
-  R.Explanation = text("Use size() method directly on string.");
+  R.Cases[0].Explanation = text("Use size() method directly on string.");
   return R;
 }
 
@@ -375,6 +376,92 @@
Input, Expected);
 }
 
+TEST_F(TransformerTest, OrderedRuleUnrelated) {
+  StringRef Flag = "flag";
+  RewriteRule FlagRule = makeRule(
+  cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
+hasName("proto::ProtoCommandLineFlag"
+   .bind(Flag)),
+unless(callee(cxxMethodDecl(hasName("GetProto"),
+  change(Flag, "PROTO"));
+
+  std::string Input = R"cc(
+proto::ProtoCommandLineFlag flag;
+int x = flag.foo();
+int y = flag.GetProto().foo();
+int f(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+proto::ProtoCommandLineFlag flag;
+int x = PROTO.foo();
+int y = flag.GetProto().foo();
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected);
+}
+
+// Version of ruleStrlenSizeAny that inserts a method with a different name than
+// ruleStrlenSize, so we can tell their effect apart.
+RewriteRule ruleStrlenSizeDistinct() {
+  StringRef S;
+  return makeRule(
+  callExpr(callee(functionDecl(hasName("strlen"))),
+   hasArgument(0, cxxMemberCallExpr(
+  on(expr().bind(S)),
+  callee(cxxMethodDecl(hasName("c_str")),
+  change("DISTINCT"));
+}
+
+TEST_F(TransformerTest, OrderedRuleRelated) {
+  std::string Input = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return strlen(s.c_str()); }
+}  // namespace foo
+int g(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return DISTINCT; }
+}  // namespace foo
+int g(string s) { return REPLACED; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input,
+   Expected);
+}
+
+// Change the order of the rules to get a different result.
+TEST_F(TransformerTest, OrderedRuleRelatedSwapped) {
+  std::string Input = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return strlen(s.c_str()); }
+}  // namespace foo
+int g(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return DISTINCT; }
+}  // namespace foo
+int g(string s) { return DISTINCT; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input,
+   Expected);
+}
+
 //
 // Negative tests (where we expect no transformation to occur).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -28,6 +28,7 @@
 using namespace tooling;
 
 using ast_matchers::MatchFinder;
+using ast_matchers::internal::DynTypedMatcher;
 using ast_type_traits::ASTNodeKind;
 using ast_type_traits::DynTypedNode;
 using llvm::Error;
@@ -171,18 +172,113 @@
   return Transformations;
 }
 
-RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M,
+RewriteRule tooling::makeRule(DynTypedMatcher M,
   SmallVector Edits) {
+  return RewriteRule{
+  {RewriteRule::Case{std::move(M), std::move(Edits), nullptr}}};
+}
+
+// Determines whether A is a base type of B in the class hierarchy, including
+// the implicit relationship of Type and QualType.

[PATCH] D61909: Add Clang shared library with C++ exports

[Chris Bieneman via Phabricator via cfe-commits]

beanz created this revision.
beanz added reviewers: tstellar, winksaville.
Herald added a subscriber: mgorny.
Herald added a project: clang.

This patch adds a libClang_shared library on *nix systems which exports the 
entire C++ API. In order to support this on Windows we should really refactor 
llvm-shlib and share code between the two.

This also uses a slightly different method for generating the shared library, 
which I should back-port to llvm-shlib. Instead of linking the static archives 
and passing linker flags to force loading the whole libraries, this patch 
creates object libraries for every library (which has no cost in the build 
system), and link the object libraries.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61909

Files:
  clang/cmake/modules/AddClang.cmake
  clang/tools/CMakeLists.txt
  clang/tools/clang-shlib/CMakeLists.txt
  clang/tools/clang-shlib/clang-shlib.cpp


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(Clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} 
${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(Clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

[Adrian Prantl via Phabricator via cfe-commits]

aprantl accepted this revision.
aprantl added a comment.

Alright, thanks for taking the time to walk me through the thought process!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added a comment.

Agreed on all the comments -- just one question:




Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ilya-biryukov wrote:
> I wonder if there a better way to construct an `anyOf` matcher that can tell 
> which case matched...
> It looks to be the reason for a more complicated interface on the transformer 
> side, but I'm not sure there's a way out of it.
> 
> Any ideas?
I don't quite follow. Which interface complication are you referring to?  FWIW, 
the reason the code here doesn't just use the anyOf() matches is that it 
doesn't take a vector -- it only has a variadic form.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335



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

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@aaron.ballman, would be a good reviewer for this? I'm happy to look at the 
transformer bits, but I'm definitely the wrong one to asses it from the  
`clang-tidy` perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



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