[PATCH] D159363: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd03a7f15f019: [clangd] SIGSEGV at clangd: DiagnosticConsumer 
Is Used After Free (authored by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159363

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


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@
   Stats ? TimedFS : StatCacheFS, 
std::make_shared(),
   StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@
   Stats ? TimedFS : StatCacheFS, std::make_shared(),
   StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159363: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D159363#4636581 , @kadircet wrote:

> thanks, the fix LGTM as well.
>
> but i wonder how this surfaces, to make sure we're taking necessary 
> precautions in the future. we definitely have a dangling reference, which 
> isn't great. but it's surprising that we access diags consumer during 
> indexing.
> I assume it's about the modules setup you're running clangd in. Do you have 
> any stack traces that shows the execution path? my assumption is, this 
> triggers when clangd ends up deserializing some symbols from a module. If 
> these end up being important diagnostics, we might want to figure out how to 
> emit diagnostics from these stages as well.

Yes, you are right. The diags consumer is triggered when it tries to read an 
implicit module that has some incompatibilities with the preamble headers.

The typical stack trace is below (that is LLVM-12 specific)

  clang::DiagnosticIDs::ProcessDiag(clang::DiagnosticsEngine&)
  clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool)
  clang::DiagnosticBuilder::~DiagnosticBuilder()
  clang::ASTReader::diagnoseOdrViolations()
  clang::ASTReader::FinishedDeserializing()
  clang::DeclContext::LoadLexicalDeclsFromExternalStorage()
  clang::DeclContext::decls_begin()
  clang::index::IndexingContext::indexDeclContext(clang::DeclContext const*)
  clang::declvisitor::Base::Visit(clang::Decl const*)
  clang::index::IndexingContext::indexDecl(clang::Decl const*)
  clang::index::IndexingContext::indexDeclContext(clang::DeclContext const*)
  clang::declvisitor::Base::Visit(clang::Decl const*)
  clang::index::IndexingContext::indexDecl(clang::Decl const*)
  clang::index::IndexingContext::indexDeclContext(clang::DeclContext const*)
  clang::declvisitor::Base::Visit(clang::Decl const*)
  clang::index::IndexingContext::indexDecl(clang::Decl const*)
  clang::index::indexTopLevelDecls(clang::ASTContext&, clang::Preprocessor&, 
llvm::ArrayRef, clang::index::IndexDataConsumer&, 
clang::index::IndexingOptions)
  clang::clangd::(anonymous namespace)::indexSymbols(clang::ASTContext&, 
std::shared_ptr, llvm::ArrayRef, 
clang::clangd::MainFileMacros const*, clang::clangd::CanonicalIncludes const&, 
bool, llvm::StringRef, bool)
  clang::clangd::indexHeaderSymbols(llvm::StringRef, clang::ASTContext&, 
std::shared_ptr, clang::clangd::CanonicalIncludes const&)
  clang::clangd::FileIndex::updatePreamble(llvm::StringRef, llvm::StringRef, 
clang::ASTContext&, std::shared_ptr, 
clang::clangd::CanonicalIncludes const&)
  void
  void
  threadFuncAsync(void*)
  start_thread


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159363

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

[PATCH] D159363: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:709
   Ctx->setStatCache(Result->StatCache);
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed

sammccall wrote:
> I think this should go up next to PreambleDiagsEngine.reset() etc, as it's 
> basically the same thing: we're trying to avoid any race between the async 
> work done by the callback and the cleanup at the end of the function.
> 
> Also I think it's slightly clearer for us to consistently be taking the 
> diagnostics from PreambleDiagnostics *after* it's detached from the compiler.
I applied the change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159363

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

[PATCH] D159363: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 555441.
ivanmurashko marked an inline comment as done.
ivanmurashko added a comment.

@sammccall's comment was addressed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159363

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


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@
   Stats ? TimedFS : StatCacheFS, 
std::make_shared(),
   StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -665,6 +665,10 @@
   Stats ? TimedFS : StatCacheFS, std::make_shared(),
   StoreInMemory, /*StoragePath=*/"", CapturedInfo);
   PreambleTimer.stopTimer();
+
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  PreambleDiagsEngine->setClient(new IgnoringDiagConsumer, true);
   // Reset references to ref-counted-ptrs before executing the callbacks, to
   // prevent resetting them concurrently.
   PreambleDiagsEngine.reset();
@@ -706,6 +710,7 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D159363: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:610-619
   StoreDiags PreambleDiagnostics;
   PreambleDiagnostics.setDiagCallback(
   [](const clang::Diagnostic , clangd::Diag ) {
 llvm::for_each(ASTListeners,
[&](const auto ) { L->sawDiagnostic(D, Diag); });
   });
   llvm::IntrusiveRefCntPtr PreambleDiagsEngine =

Alternative fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159363

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

[PATCH] D159363: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: kadircet, sammccall.
ivanmurashko added projects: clang, clang-tools-extra.
Herald added a subscriber: arphaman.
Herald added a project: All.
ivanmurashko requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.

This is a follow-up patch for D148088 . The 
dynamic symbol index (`FileIndex::updatePreamble`) may run in a separate 
thread, and the `DiagnosticConsumer` that is set up in `buildPreamble` might go 
out of scope before it is used. This could result in a SIGSEGV when attempting 
to call any method of the `DiagnosticConsumer` class.

The function `buildPreamble` sets up the `DiagnosticConsumer` as follows:

  ... buildPreamble(...) {
  ...
StoreDiags PreambleDiagnostics;
...
llvm::IntrusiveRefCntPtr PreambleDiagsEngine =
  CompilerInstance::createDiagnostics((),
  ,
  /*ShouldOwnClient=*/false);
...
// The call might use the diagnostic consumer in a separate thread
PreambleCallback(...)
...
  }

`PreambleDiagnostics` might be out of scope for `buildPreamble` function when 
we call it inside `PreambleCallback` in a separate thread.

The Fix
The fix involves replacing the client (DiagnosticConsumer) with an 
`IgnoringDiagConsumer` instance, which will print messages to the clangd log.

Alternatively, we can replace `PreambleDiagnostics` with an object that is 
owned by `DiagnosticsEngine`.

Note
There is no corresponding LIT/GTest for this issue, since there is a specific 
race condition that is difficult to reproduce within a test framework.

Test Plan:

  ninja check-clangd


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159363

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


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -706,6 +706,11 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  Ctx->getASTContext().getDiagnostics().setClient(new IgnoringDiagConsumer,
+  true);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;


Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -706,6 +706,11 @@
   // While extending the life of FileMgr and VFS, StatCache should also be
   // extended.
   Ctx->setStatCache(Result->StatCache);
+  // We have to setup DiagnosticConsumer that will be alife
+  // while preamble callback is executed
+  Ctx->getASTContext().getDiagnostics().setClient(new IgnoringDiagConsumer,
+  true);
+
   PreambleCallback(std::move(*Ctx), Result->Pragmas);
 }
 return Result;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko abandoned this revision.
ivanmurashko added a comment.

The diff is not required after https://reviews.llvm.org/D145302 deployed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D145302#4492595 , @mstorsjo wrote:

> In D145302#4491973 , @glandium 
> wrote:
>
>> This broke building with `-DLLVM_LINK_LLVM_DYLIB=ON`:
>
> I also ran into this; I pushed a fix in 
> a20d57e83441a69fa2bab86593b18cc0402095d2 
> .

Thank for the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG56ac9d46a7c1: [clangd] Add library for clangd main function 
(authored by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,19 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+namespace clang {
+namespace clangd {
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -710,8 +711,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +718,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1037,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,6 +1,13 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdToolMain.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
+  )
+
+add_clang_tool(clangd
+  ClangdToolMain.cpp
   $
   )
 
@@ -13,7 +20,7 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
-clang_target_link_libraries(clangd
+clang_target_link_libraries(clangdMain
   PRIVATE
   clangAST
   clangBasic
@@ -25,14 +32,14 @@
   clangToolingCore
   clangToolingRefactoring
   clangToolingSyntax
-  )
-
-target_link_libraries(clangd
-  PRIVATE
   clangTidy
-
   clangDaemon
   clangdRemoteIndex
   clangdSupport
   ${CLANGD_XPC_LIBS}
   )
+
+clang_target_link_libraries(clangd
+  PRIVATE
+  clangdMain
+  )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 539119.
ivanmurashko added a comment.

rebase before the final push


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,19 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+namespace clang {
+namespace clangd {
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -710,8 +711,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +718,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1037,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,6 +1,13 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdToolMain.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
+  )
+
+add_clang_tool(clangd
+  ClangdToolMain.cpp
   $
   )
 
@@ -13,7 +20,7 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
-clang_target_link_libraries(clangd
+clang_target_link_libraries(clangdMain
   PRIVATE
   clangAST
   clangBasic
@@ -25,14 +32,14 @@
   clangToolingCore
   clangToolingRefactoring
   clangToolingSyntax
-  )
-
-target_link_libraries(clangd
-  PRIVATE
   clangTidy
-
   clangDaemon
   clangdRemoteIndex
   clangdSupport
   ${CLANGD_XPC_LIBS}
   )
+
+clang_target_link_libraries(clangd
+  PRIVATE
+  clangdMain
+  )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D145302#4441334 , @kadircet wrote:

> thanks LG and seems to be working in a couple build configurations I tried. 
> but there might still be breakages in different configs, so please be on the 
> watchout after landing this for breakages in https://lab.llvm.org/.

@kadircet , could you look at the final version?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang-tools-extra/clangd/tool/CMakeLists.txt:37
+
+target_link_libraries(clangdMain
+  PRIVATE

kadircet wrote:
> you can merge this with the previous rule
fixed



Comment at: clang-tools-extra/clangd/tool/CMakeLists.txt:61
 
 target_link_libraries(clangd
   PRIVATE

kadircet wrote:
> you can get rid of this rule too
fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko marked an inline comment as not done.
ivanmurashko added inline comments.



Comment at: clang-tools-extra/clangd/tool/CMakeLists.txt:11
+  ClangdToolMain.cpp
   $
   )

kadircet wrote:
> we should move this into `clangdMain` target now
Unfortunately it did not work and broke the clangd unit tests.

Note: all other comments were fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 536577.
ivanmurashko added a comment.

Fixed clangd unit tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,19 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+namespace clang {
+namespace clangd {
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -710,8 +711,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +718,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1037,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,6 +1,13 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdToolMain.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
+  )
+
+add_clang_tool(clangd
+  ClangdToolMain.cpp
   $
   )
 
@@ -13,7 +20,7 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
-clang_target_link_libraries(clangd
+clang_target_link_libraries(clangdMain
   PRIVATE
   clangAST
   clangBasic
@@ -25,14 +32,14 @@
   clangToolingCore
   clangToolingRefactoring
   clangToolingSyntax
-  )
-
-target_link_libraries(clangd
-  PRIVATE
   clangTidy
-
   clangDaemon
   clangdRemoteIndex
   clangdSupport
   ${CLANGD_XPC_LIBS}
   )
+
+clang_target_link_libraries(clangd
+  PRIVATE
+  clangdMain
+  )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 535144.
ivanmurashko added a comment.

small fixes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,19 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+namespace clang {
+namespace clangd {
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -710,8 +711,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +718,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1037,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,9 +1,16 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdToolMain.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
   $
   )
 
+add_clang_tool(clangd
+  ClangdToolMain.cpp
+  )
+
 set(LLVM_LINK_COMPONENTS
   support
   )
@@ -13,7 +20,7 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
-clang_target_link_libraries(clangd
+clang_target_link_libraries(clangdMain
   PRIVATE
   clangAST
   clangBasic
@@ -25,14 +32,14 @@
   clangToolingCore
   clangToolingRefactoring
   clangToolingSyntax
-  )
-
-target_link_libraries(clangd
-  PRIVATE
   clangTidy
-
   clangDaemon
   clangdRemoteIndex
   clangdSupport
   ${CLANGD_XPC_LIBS}
   )
+
+clang_target_link_libraries(clangd
+  PRIVATE
+  clangdMain
+  )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 535138.
ivanmurashko added a comment.

Rebase + apply suggestions from @kadircet


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,19 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+namespace clang {
+namespace clangd {
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -710,8 +711,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +718,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1037,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,6 +1,13 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdToolMain.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
+  )
+
+add_clang_tool(clangd
+  ClangdToolMain.cpp
   $
   )
 
@@ -13,7 +20,7 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
-clang_target_link_libraries(clangd
+clang_target_link_libraries(clangdMain
   PRIVATE
   clangAST
   clangBasic
@@ -25,14 +32,14 @@
   clangToolingCore
   clangToolingRefactoring
   clangToolingSyntax
-  )
-
-target_link_libraries(clangd
-  PRIVATE
   clangTidy
-
   clangDaemon
   clangdRemoteIndex
   clangdSupport
   ${CLANGD_XPC_LIBS}
   )
+
+clang_target_link_libraries(clangd
+  PRIVATE
+  clangdMain
+  )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Ivan Murashko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa8ad413f0d18: [RFC][clangd] Move preamble index out of 
document open critical path (authored by kuganv, committed by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang/include/clang/Frontend/CompilerInstance.h

Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -233,6 +234,8 @@
 return *Invocation;
   }
 
+  std::shared_ptr getInvocationPtr() { return Invocation; }
+
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr Value);
 
@@ -338,6 +341,11 @@
 return *Diagnostics;
   }
 
+  IntrusiveRefCntPtr getDiagnosticsPtr() const {
+assert(Diagnostics && "Compiler instance has no diagnostics!");
+return Diagnostics;
+  }
+
   /// setDiagnostics - Replace the current diagnostics engine.
   void setDiagnostics(DiagnosticsEngine *Value);
 
@@ -373,6 +381,11 @@
 return *Target;
   }
 
+  IntrusiveRefCntPtr getTargetPtr() const {
+assert(Target && "Compiler instance has no target!");
+return Target;
+  }
+
   /// Replace the current Target.
   void setTarget(TargetInfo *Value);
 
@@ -406,6 +419,11 @@
 return *FileMgr;
   }
 
+  IntrusiveRefCntPtr getFileManagerPtr() const {
+assert(FileMgr && "Compiler instance has no file manager!");
+return FileMgr;
+  }
+
   void resetAndLeakFileManager() {
 llvm::BuryPointer(FileMgr.get());
 FileMgr.resetWithoutRelease();
@@ -426,6 +444,11 @@
 return *SourceMgr;
   }
 
+  IntrusiveRefCntPtr getSourceManagerPtr() const {
+assert(SourceMgr && "Compiler instance has no source manager!");
+return SourceMgr;
+  }
+
   void resetAndLeakSourceManager() {
 llvm::BuryPointer(SourceMgr.get());
 SourceMgr.resetWithoutRelease();
@@ -466,6 +489,11 @@
 return *Context;
   }
 
+  IntrusiveRefCntPtr getASTContextPtr() const {
+assert(Context && "Compiler instance has no AST context!");
+return Context;
+  }
+
   void resetAndLeakASTContext() {
 llvm::BuryPointer(Context.get());
 Context.resetWithoutRelease();
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- clang-tools-extra/clangd/unittests/TestWorkspace.cpp
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -21,11 +21,14 @@
   continue;
 TU.Code = Input.second.Code;
 TU.Filename = Input.first().str();
-TU.preamble([&](ASTContext , Preprocessor ,
-const CanonicalIncludes ) {
-  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
-CanonIncludes);
-});
+TU.preamble(
+[&](CapturedASTCtx ASTCtx,
+const std::shared_ptr CanonIncludes) {
+  auto  = ASTCtx.getASTContext();
+  auto  = ASTCtx.getPreprocessor();
+  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
+*CanonIncludes);
+});
 ParsedAST MainAST = TU.build();
 Index->updateMain(testPath(Input.first()), MainAST);
   }
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1129,9 +1129,9 @@
   public:
 BlockPreambleThread(llvm::StringRef BlockVersion, Notification )
 : BlockVersion(BlockVersion), N(N) {}
-void onPreambleAST(PathRef Path, llvm::StringRef Version,
-   const CompilerInvocation &, ASTContext ,
-   Preprocessor &, const CanonicalIncludes &) override {
+void
+onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+  const std::shared_ptr) override {
   if 

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf8536fb11e3d: [clang][HeaderSearch] Fix implicit module when 
using header maps (authored by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/implicit-module-header-maps.cpp


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,54 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't used, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,54 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't used, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 525570.
ivanmurashko added a comment.

removed header install, only lib install has been left after the change

CC: @kadircet


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,19 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+namespace clang {
+namespace clangd {
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -710,8 +711,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +718,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1037,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,6 +1,13 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdMain.cpp ClangdToolMain.cpp Check.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
+  )
+
+add_clang_tool(clangd
+  ClangdToolMain.cpp
   $
   )
 
@@ -13,8 +20,32 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
+clang_target_link_libraries(clangdMain
+  PRIVATE
+  clangAST
+  clangBasic
+  clangFormat
+  clangFrontend
+  clangLex
+  clangSema
+  clangTooling
+  clangToolingCore
+  clangToolingRefactoring
+  clangToolingSyntax
+  )
+
+target_link_libraries(clangdMain
+  PRIVATE
+  clangTidy
+  clangDaemon
+  clangdRemoteIndex
+  clangdSupport
+  ${CLANGD_XPC_LIBS}
+  )
+
 clang_target_link_libraries(clangd
   PRIVATE
+  clangdMain
   clangAST
   clangBasic
   clangFormat
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 525558.
ivanmurashko added a comment.

rebased to the latest LLVM main


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,19 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+namespace clang {
+namespace clangd {
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -710,8 +711,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +718,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1037,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,6 +1,13 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdMain.cpp ClangdToolMain.cpp Check.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
+  )
+
+add_clang_tool(clangd
+  ClangdToolMain.cpp
   $
   )
 
@@ -13,8 +20,32 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
+clang_target_link_libraries(clangdMain
+  PRIVATE
+  clangAST
+  clangBasic
+  clangFormat
+  clangFrontend
+  clangLex
+  clangSema
+  clangTooling
+  clangToolingCore
+  clangToolingRefactoring
+  clangToolingSyntax
+  )
+
+target_link_libraries(clangdMain
+  PRIVATE
+  clangTidy
+  clangDaemon
+  clangdRemoteIndex
+  clangdSupport
+  ${CLANGD_XPC_LIBS}
+  )
+
 clang_target_link_libraries(clangd
   PRIVATE
+  clangdMain
   clangAST
   clangBasic
   clangFormat
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -219,3 +219,17 @@
 
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(FILES
+${CMAKE_CURRENT_SOURCE_DIR}/tool/ClangdMain.h
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/clangd"
+COMPONENT clangd-headers)
+  add_custom_target(clangd-headers)
+  set_target_properties(clangd-headers PROPERTIES FOLDER "Misc")
+  if(NOT LLVM_ENABLE_IDE)
+add_llvm_install_targets(install-clangd-headers
+ DEPENDS clangd-headers
+ COMPONENT clangd-headers)
+  endif()

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 525557.
ivanmurashko added a comment.

rebase to latest LLVM main branch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/implicit-module-header-maps.cpp


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,54 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't used, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,54 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't used, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto 

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

> Still looking at issues and not sure whether these are blockers or not.

Friendly reminder:
@benlangmuir, do you need any assistance from my side with it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

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

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 521580.
ivanmurashko added a comment.

typo fixed + rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/implicit-module-header-maps.cpp


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,54 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't used, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,54 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't used, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+ 

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 520117.
ivanmurashko added a comment.

windows marked as non supported


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/implicit-module-header-maps.cpp


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,54 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,54 @@
+// UNSUPPORTED: system-windows
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto 

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 520096.
ivanmurashko added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/implicit-module-header-maps.cpp


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" -e "s||/|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" -e "s||/|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> 

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 520069.
ivanmurashko added a comment.

small fix at the lit-test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/implicit-module-header-maps.cpp


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" -e "s||/|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" -e "s||/|g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> 

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 520062.
ivanmurashko added a comment.

`split-file` was used for lit-test simplification (see @ChuanqiXu comment)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/implicit-module-header-maps.cpp


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" -e "s##/#g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules 
-fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap 
-fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.


Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write a.hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+//
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" -e "s##/#g" b.hmap.json > hmap.json
+// RUN: %hmaptool write hmap.json hmap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=module.modulemap -fsyntax-only -I hmap -fmodules-cache-path=%t test.cpp
+
+//--- After/Mapping.h
+#ifdef FOO
+#error foo
+#endif
+
+//--- a.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
+
+//--- b.hmap.json
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
+
+//--- module.modulemap
+module a {
+  header "After/Mapping.h"
+}
+
+
+//--- test.cpp
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto 

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

Friendly ping

@arphaman, @jansvoboda11, I have made the patch buildable on all platforms and 
have all tests passed. There was also a small fix (temp path for modules 
artefact) at the test that could fix its run on some platforms. Could you look 
at it? Does it have any issues on your side?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

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

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D148088#4302269 , @ilya-biryukov 
wrote:

> 



> Could you elaborate a bit more on what is being cached with modules and how 
> this patch would affect it?

I hope that I can provide some info here. We are going to use implicit modules 
to cache preamble i.e. the corresponding modulemap file will contain headers 
from the preamble. The idea is to provide faster goto-definition functionality 
that requires AST be built. The modules can be loaded lazily and therefore it 
can give performance boost up >10x depending on the source file. The problem 
here is the preamble indexing as soon as it mostly force the module load and 
kills the performance wins i.e. it will be ~20% instead of 10x. The possible 
solution is to move the indexing into a separate thread with possible delay for 
some functionality. Any other ideas will be also interesting here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148088

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

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 517323.
ivanmurashko added a comment.

Windows constrains added


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/Inputs/implicit-module-header-maps/a.h
  clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
  clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
  clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
  clang/test/Modules/implicit-module-header-maps.cpp

Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,35 @@
+// REQUIRES: shell
+// REQUIRES: case-insensitive-filesystem
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write %S/Inputs/implicit-module-header-maps/a.hmap.json %t/hmap
+//
+// RUN: mkdir -p %t/After
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.h %t/After/Mapping.h
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.module.modulemap %t/module.modulemap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=%t/module.modulemap -fsyntax-only %s -I %t/hmap -fmodules-cache-path=%t
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" %S/Inputs/implicit-module-header-maps/b.hmap.json > %t/hmap.json
+// RUN: %hmaptool write %t/hmap.json %t/hmap
+//
+// RUN: mkdir -p %t/After
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.h %t/After/Mapping.h
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.module.modulemap %t/module.modulemap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=%t/module.modulemap -fsyntax-only %s -I %t/hmap -fmodules-cache-path=%t
+
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
@@ -0,0 +1,6 @@
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
@@ -0,0 +1,3 @@
+module a {
+  header "After/Mapping.h"
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
@@ -0,0 +1,7 @@
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.h
@@ -0,0 +1,3 @@
+#ifdef FOO
+#error foo
+#endif
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 517222.
ivanmurashko added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/Inputs/implicit-module-header-maps/a.h
  clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
  clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
  clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
  clang/test/Modules/implicit-module-header-maps.cpp

Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,32 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write %S/Inputs/implicit-module-header-maps/a.hmap.json %t/hmap
+//
+// RUN: mkdir -p %t/After
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.h %t/After/Mapping.h
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.module.modulemap %t/module.modulemap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=%t/module.modulemap -fsyntax-only %s -I %t/hmap -fmodules-cache-path=%t
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" %S/Inputs/implicit-module-header-maps/b.hmap.json > %t/hmap.json
+// RUN: %hmaptool write %t/hmap.json %t/hmap
+//
+// RUN: mkdir -p %t/After
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.h %t/After/Mapping.h
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.module.modulemap %t/module.modulemap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=%t/module.modulemap -fsyntax-only %s -I %t/hmap -fmodules-cache-path=%t
+
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
@@ -0,0 +1,6 @@
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
@@ -0,0 +1,3 @@
+module a {
+  header "After/Mapping.h"
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
@@ -0,0 +1,7 @@
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.h
@@ -0,0 +1,3 @@
+#ifdef FOO
+#error foo
+#endif
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 517159.
ivanmurashko added a comment.

Commandeer the diff from @andrewjcg and made some chnages at the code (get it 
compatible with latest clang source code) and at the tests (move modules 
artefacts to temp folder to make the test execution more stable).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

Files:
  clang/lib/Lex/HeaderSearch.cpp
  clang/test/Modules/Inputs/implicit-module-header-maps/a.h
  clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
  clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
  clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
  clang/test/Modules/implicit-module-header-maps.cpp

Index: clang/test/Modules/implicit-module-header-maps.cpp
===
--- /dev/null
+++ clang/test/Modules/implicit-module-header-maps.cpp
@@ -0,0 +1,32 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+//
+// RUN: %hmaptool write %S/Inputs/implicit-module-header-maps/a.hmap.json %t/hmap
+//
+// RUN: mkdir -p %t/After
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.h %t/After/Mapping.h
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.module.modulemap %t/module.modulemap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=%t/module.modulemap -fsyntax-only %s -I %t/hmap -fmodules-cache-path=%t
+//
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+//
+// RUN: sed -e "s|OUTPUTS_DIR|%t|g" %S/Inputs/implicit-module-header-maps/b.hmap.json > %t/hmap.json
+// RUN: %hmaptool write %t/hmap.json %t/hmap
+//
+// RUN: mkdir -p %t/After
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.h %t/After/Mapping.h
+// RUN: cp %S/Inputs/implicit-module-header-maps/a.module.modulemap %t/module.modulemap
+//
+// RUN: %clang -Rmodule-build -fmodules -fimplicit-modules -fimplicit-module-maps -fmodule-map-file=%t/module.modulemap -fsyntax-only %s -I %t/hmap -fmodules-cache-path=%t
+
+#define FOO
+// This include will fail if:
+// 1) modules are't used, as the `FOO` define will propagate into the included
+//header and trip a `#error`, or
+// 2) header maps aren't usesd, as the header name doesn't exist and relies on
+//the header map to remap it to the real header.
+#include "Before/Mapping.h"
Index: clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/b.hmap.json
@@ -0,0 +1,6 @@
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "OUTPUTS_DIR/After/Mapping.h"
+}
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.module.modulemap
@@ -0,0 +1,3 @@
+module a {
+  header "After/Mapping.h"
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.hmap.json
@@ -0,0 +1,7 @@
+{
+  "mappings" :
+{
+ "Before/Mapping.h" : "After/Mapping.h",
+ "After/Mapping.h" : "After/Mapping.h"
+}
+}
Index: clang/test/Modules/Inputs/implicit-module-header-maps/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-module-header-maps/a.h
@@ -0,0 +1,3 @@
+#ifdef FOO
+#error foo
+#endif
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -491,7 +491,8 @@
 
   IsInHeaderMap = true;
 
-  auto FixupSearchPath = [&]() {
+  auto FixupSearchPathAndFindUsableModule =
+  [&](auto File) -> OptionalFileEntryRef {
 if (SearchPath) {
   StringRef SearchPathRef(getName());
   SearchPath->clear();
@@ -501,6 +502,12 @@
   RelativePath->clear();
   RelativePath->append(Filename.begin(), Filename.end());
 }
+if (!HS.findUsableModuleForHeader(
+(), File.getFileEntry().getDir(),
+RequestingModule, SuggestedModule, isSystemHeaderDirectory())) {
+  return std::nullopt;
+}
+return File;
   };
 
   // Check if the headermap maps the filename to a framework include
@@ -513,8 +520,7 @@
   }
 
   if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest, OpenFile)) {
-FixupSearchPath();
-return *Res;
+return FixupSearchPathAndFindUsableModule(*Res);
   }
 
   // Header maps need to be marked as used whenever the filename matches.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

@arphaman, @andrewjcg, what's the status of the diff?
The functionality is important for me and I want to get it landed.

BTW: @andrewjcg I can commander the diff if you don't mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103930

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

> Can this be solved at the build-system level, without changing the source 
> code in ways that aren't otherwise useful?
> For example, with a `CLANG_TIDY_EXTRA_CHECKS` cmake variable that adds more 
> deps? This seems like it could also work for the `clang-tidy` binary.

As it was mentioned at D145228 , we use LLVM 
as a set of libraries and headers that allow us to create custom components. 
The build system we use for the component is different from CMake (it's buck). 
Thus, unfortunately, the CMake customization does not work for us.

> IIUC we want to link some extra targets into the `clangd` binary in order to 
> provide extra clang-tidy checks, but *don't* want to customize the binary 
> further.
> (The latter can be useful, but this interface isn't sufficient for it).

I agree that the current interface does not provide any way to customize the 
binary. My primary reason was the following: I tried to keep the changes as 
minimal as possible to get an ability to build the clangd outside LLVM repo. I 
assumed that additional customization API can be added later as soon as it is 
be required.

BTW: I added the copy of the single header file (API) instead of copying all 
internal headers. That will allow to abandon D145228 
 and add future external API at the select 
set of headers in future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

> If we consider the bare minimum with the only goal to build outside LLVM 
> source tree then we don’t need to copy all internal headers.  At the case the 
> D145302  can be modified and we can end up 
> with the only one header that is required to copy:  
> clang-tools-extra/clangd/tool/ClangdMain.h.

I just updated D145302  with the changes. The 
D145228  can be abandoned if the changes are 
OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 507029.
ivanmurashko added a comment.

The change addresses comments from D145228 . 
The only one header is copied as a part of the installation. As result the diff 
contains the minimal changes required to integrated clangd into custom build 
systems (different from CMake).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,19 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+namespace clang {
+namespace clangd {
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -710,8 +711,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +718,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1037,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,6 +1,13 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdMain.cpp ClangdToolMain.cpp Check.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
+  )
+
+add_clang_tool(clangd
+  ClangdToolMain.cpp
   $
   )
 
@@ -13,8 +20,32 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
+clang_target_link_libraries(clangdMain
+  PRIVATE
+  clangAST
+  clangBasic
+  clangFormat
+  clangFrontend
+  clangLex
+  clangSema
+  clangTooling
+  clangToolingCore
+  clangToolingRefactoring
+  clangToolingSyntax
+  )
+
+target_link_libraries(clangdMain
+  PRIVATE
+  clangTidy
+  clangDaemon
+  clangdRemoteIndex
+  clangdSupport
+  ${CLANGD_XPC_LIBS}
+  )
+
 clang_target_link_libraries(clangd
   PRIVATE
+  clangdMain
   clangAST
   clangBasic
   clangFormat
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -218,3 +218,17 @@
 
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(FILES
+${CMAKE_CURRENT_SOURCE_DIR}/tool/ClangdMain.h
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/clangd"
+COMPONENT clangd-headers)
+  add_custom_target(clangd-headers)
+  

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D145228#4174829 , @sammccall wrote:

> I think this is a bit abstract though. Concretely, what API do you need here? 
> e.g. which headers do you want to include, to what end?

If we consider the bare minimum with the only goal to build outside LLVM source 
tree then we don’t need to copy all internal headers.  At the case the D145302 
 can be modified and we can end up with the 
only one header that is required to copy:  
clang-tools-extra/clangd/tool/ClangdMain.h.

For further customization one might require additional headers to be installed. 
That can be done once it’s required or via one diff that install all possible 
headers.

What do you think about it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D145228#4174543 , @kadircet wrote:

> 

I understand concerns about maintenance cost for the change. But I dare to ask 
why you think it is so high? Perhaps there are different expectations from the 
feature. I’m not asking to support any API stability or anything like this. 
IMHO, the maintenance cost for the feature should be on people who would like 
to use it. Clangd can only export headers and libraries as they are with one 
addition to allow main function overriding. Users of the feature should be 
prepared that it might change significantly at any point even within release. 
There is smaller but similar problem with other many other LLVM APIs, that are 
significantly changed especially between releases. I hope this expectation 
should make the maintenance cost for the change smaller and we are happy to 
support them if it is needed.

There are some additional context about the change. We have a separate build 
system (not CMake) for in-house apps. The LVVM is considered as a third-party 
with corresponding libs and headers. The approach allows us to create in-house 
clang tools such as lint and refactoring tools compiling from the same sources 
against multiple clang versions and variants. The approach is supported by LLVM 
modular structure that gives an ability to combine different pieces to create 
powerful customized in-house apps. Use forks of LLVM is one of the obvious way 
to use the feature, but it has high maintenance cost and it makes harder to 
contribute back bug fixes and features. Because we have to pay this cost anyway 
we can support this feature in upstream for everybody and make LLVM even more 
re-usable. If we need to support it in another build system, we can do it as 
well (up to the degree we can in LLVM).

What do you think about it? Have I missed something important?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145228

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

[PATCH] D145302: [clangd] Add library for clangd main function

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: sammccall, kadircet, ilya-biryukov.
Herald added a subscriber: arphaman.
Herald added a project: All.
ivanmurashko requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang-tools-extra.

The diff adds a library for clangd main function. That change allows to create 
custom builds for clangd outside the main LLVM repo. The main reason for such 
builds is an ability to use custom clang-tidy modules (created outside LLVM 
repo).

Note: The possibility for the custom clang-tidy modules was added recently at 
https://reviews.llvm.org/D73300. The header installation for clangd was also 
added as a separate diff: https://reviews.llvm.org/D145228

Test Plan:

  ninja clangd

also check that necessary libs are installed aka

  ninja install
  ...
  ls /lib/libclangdMain.a


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145302

Files:
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/tool/ClangdMain.h
  clang-tools-extra/clangd/tool/ClangdToolMain.cpp

Index: clang-tools-extra/clangd/tool/ClangdToolMain.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdToolMain.cpp
@@ -0,0 +1,13 @@
+//===--- ClangdToolMain.cpp - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangdMain.h"
+
+int main(int argc, char **argv) {
+  return clang::clangd::clangdMain(argc, argv);
+}
Index: clang-tools-extra/clangd/tool/ClangdMain.h
===
--- /dev/null
+++ clang-tools-extra/clangd/tool/ClangdMain.h
@@ -0,0 +1,27 @@
+//===--- ClangdMain.h - clangd main function ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
+
+#include "ClangdLSPServer.h"
+
+namespace clang {
+namespace clangd {
+// Check functon that simulates opening a file
+// (determining compile command, parsing, indexing)
+// and then running features at many locations.
+bool check(llvm::StringRef File, const ThreadsafeFS ,
+   const ClangdLSPServer::Options );
+
+// clangd main function (clangd server loop)
+int clangdMain(int argc, char *argv[]);
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TOOL_CLANGDMAIN_H
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//
 
+#include "ClangdMain.h"
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "Compiler.h"
@@ -60,10 +61,6 @@
 namespace clang {
 namespace clangd {
 
-// Implemented in Check.cpp.
-bool check(const llvm::StringRef File, const ThreadsafeFS ,
-   const ClangdLSPServer::Options );
-
 namespace {
 
 using llvm::cl::cat;
@@ -710,8 +707,6 @@
   }
 };
 } // namespace
-} // namespace clangd
-} // namespace clang
 
 enum class ErrorResultCode : int {
   NoShutdownRequest = 1,
@@ -719,10 +714,7 @@
   CheckFailed = 3
 };
 
-int main(int argc, char *argv[]) {
-  using namespace clang;
-  using namespace clang::clangd;
-
+int clangdMain(int argc, char *argv[]) {
   llvm::InitializeAllTargetInfos();
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::sys::AddSignalHandler(
@@ -1041,3 +1033,6 @@
 
   return ExitCode;
 }
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -1,6 +1,13 @@
-add_clang_tool(clangd
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES ClangdMain.cpp ClangdToolMain.cpp Check.cpp)
+
+add_clang_library(clangdMain
   ClangdMain.cpp
   Check.cpp
+  )
+
+add_clang_tool(clangd
+  ClangdToolMain.cpp
   $
   )
 
@@ -13,8 +20,32 @@
   list(APPEND CLANGD_XPC_LIBS "clangdXpcJsonConversions" "clangdXpcTransport")
 endif()
 
+clang_target_link_libraries(clangdMain
+  PRIVATE
+  clangAST
+  

[PATCH] D145228: [clangd] Add clangd headers to install targets

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: sammccall, alexfh, smeenai, aaron.ballman.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
ivanmurashko requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

The install target for clang distributes the clangd static libs but missing 
corresponding headers. The diff adds necessary headers. That opens a 
possibility to create custom clangd builds outside LLVM repo.

Test Plan:

  ninja install-clangd-headers

see the headers installed at the install folder


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145228

Files:
  clang-tools-extra/clangd/CMakeLists.txt


Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -218,3 +218,26 @@
 
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(DIRECTORY .
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/clangd"
+COMPONENT clangd-headers
+FILES_MATCHING
+PATTERN "*.h"
+)
+  install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
+COMPONENT clangd-headers
+FILES_MATCHING
+PATTERN "CMakeFiles" EXCLUDE
+PATTERN "*.inc"
+)
+  add_custom_target(clangd-headers)
+  set_target_properties(clangd-headers PROPERTIES FOLDER "Misc")
+  if(NOT LLVM_ENABLE_IDE)
+add_llvm_install_targets(install-clangd-headers
+ DEPENDS clangd-headers
+ COMPONENT clangd-headers)
+  endif()
+endif()


Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -218,3 +218,26 @@
 
 add_subdirectory(index/remote)
 add_subdirectory(index/dex/dexp)
+
+if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
+  install(DIRECTORY .
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/clangd"
+COMPONENT clangd-headers
+FILES_MATCHING
+PATTERN "*.h"
+)
+  install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
+COMPONENT clangd-headers
+FILES_MATCHING
+PATTERN "CMakeFiles" EXCLUDE
+PATTERN "*.inc"
+)
+  add_custom_target(clangd-headers)
+  set_target_properties(clangd-headers PROPERTIES FOLDER "Misc")
+  if(NOT LLVM_ENABLE_IDE)
+add_llvm_install_targets(install-clangd-headers
+ DEPENDS clangd-headers
+ COMPONENT clangd-headers)
+  endif()
+endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141950: [NFC] Use find_last_of when seraching for code in getRawCommentForDeclNoCacheImpl

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd604c57164ec: [NFC] Use find_last_of when seraching for code 
in… (authored by kuganv, committed by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141950

Files:
  clang/lib/AST/ASTContext.cpp


Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -349,7 +349,7 @@
 
   // There should be no other declarations or preprocessor directives between
   // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  if (Text.find_last_of(";{}#@") != StringRef::npos)
 return nullptr;
 
   return CommentBeforeDecl;


Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -349,7 +349,7 @@
 
   // There should be no other declarations or preprocessor directives between
   // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  if (Text.find_last_of(";{}#@") != StringRef::npos)
 return nullptr;
 
   return CommentBeforeDecl;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133436: Ground work for cuda-related checks in clang-tidy

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang-tools-extra/test/lit.cfg.py:19
 # suffixes: A list of file extensions to treat as test files.
-config.suffixes = ['.c', '.cpp', '.hpp', '.m', '.mm', '.cu', '.ll', '.cl', 
'.s',
+config.suffixes = ['.c', '.cpp', '.cu', '.hpp', '.m', '.mm', '.cu', '.ll', 
'.cl', '.s',
   '.modularize', '.module-map-checker', '.test']

`.cu` is specified several times there. It looks like the change is not 
necessary at the line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133436

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

[PATCH] D131675: [clang] SIGSEGV fix at clang::ASTContext::getRawCommentForDeclNoCacheImpl

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG557e32e002ed: [clang] SIGSEGV fix at 
clang::ASTContext::getRawCommentForDeclNoCacheImpl (authored by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131675

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/AST/ast-crash-doc.cpp


Index: clang/test/AST/ast-crash-doc.cpp
===
--- /dev/null
+++ clang/test/AST/ast-crash-doc.cpp
@@ -0,0 +1,30 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -emit-module -x c++ -fmodules -I %t/Inputs -fmodule-name=aa 
%t/Inputs/module.modulemap -o %t/aa.pcm
+// RUN: rm %t/Inputs/b.h
+// RUN: not %clang_cc1 -x c++ -Wdocumentation -ast-dump-all -fmodules -I 
%t/Inputs -fmodule-file=%t/aa.pcm %t/test.cpp | FileCheck %s
+
+//--- Inputs/module.modulemap
+module aa {
+header "a.h"
+header "b.h"
+}
+
+//--- Inputs/a.h
+// empty file
+
+//--- Inputs/b.h
+/// test foo @return
+int foo();
+
+
+//--- test.cpp
+#include "a.h"
+
+/// test comment at the primary file
+
+int a = foo();
+
+
+// CHECK: TranslationUnitDecl
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -298,6 +298,9 @@
 return nullptr;
 
   const FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
+  if (!File.isValid()) {
+return nullptr;
+  }
   const auto CommentsInThisFile = Comments.getCommentsInFile(File);
   if (!CommentsInThisFile || CommentsInThisFile->empty())
 return nullptr;


Index: clang/test/AST/ast-crash-doc.cpp
===
--- /dev/null
+++ clang/test/AST/ast-crash-doc.cpp
@@ -0,0 +1,30 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -emit-module -x c++ -fmodules -I %t/Inputs -fmodule-name=aa %t/Inputs/module.modulemap -o %t/aa.pcm
+// RUN: rm %t/Inputs/b.h
+// RUN: not %clang_cc1 -x c++ -Wdocumentation -ast-dump-all -fmodules -I %t/Inputs -fmodule-file=%t/aa.pcm %t/test.cpp | FileCheck %s
+
+//--- Inputs/module.modulemap
+module aa {
+header "a.h"
+header "b.h"
+}
+
+//--- Inputs/a.h
+// empty file
+
+//--- Inputs/b.h
+/// test foo @return
+int foo();
+
+
+//--- test.cpp
+#include "a.h"
+
+/// test comment at the primary file
+
+int a = foo();
+
+
+// CHECK: TranslationUnitDecl
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -298,6 +298,9 @@
 return nullptr;
 
   const FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
+  if (!File.isValid()) {
+return nullptr;
+  }
   const auto CommentsInThisFile = Comments.getCommentsInFile(File);
   if (!CommentsInThisFile || CommentsInThisFile->empty())
 return nullptr;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131675: [clang] SIGSEGV fix at clang::ASTContext::getRawCommentForDeclNoCacheImpl

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: sammccall, aaron.ballman.
Herald added subscribers: usaxena95, kadircet.
Herald added a project: All.
ivanmurashko requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.

The `File` might point to an invalid `FileID` at the case of broken AST. That 
leads to clang/clangd crash while processing comments. Relevant part of the 
crash is below

   #4 0x7f1d7fbf95bc std::_Rb_tree, std::_Select1st>, std::less, 
std::allocator>>::_M_lower_bound(std::_Rb_tree_node> const*, std::_Rb_tree_node_base const*, 
unsigned int const&) const /usr/include/c++/8/bits/stl_tree.h:1911:2
   #5 0x7f1d7fbf95bc std::_Rb_tree, std::_Select1st>, std::less, 
std::allocator>>::lower_bound(unsigned int const&) const 
/usr/include/c++/8/bits/stl_tree.h:1214:56
   #6 0x7f1d7fbf95bc std::map, std::allocator>>::lower_bound(unsigned int const&) const 
/usr/include/c++/8/bits/stl_map.h:1264:3
  6
   #7 0x7f1d7fbf95bc 
clang::ASTContext::getRawCommentForDeclNoCacheImpl(clang::Decl const*, 
clang::SourceLocation, std::map, std::allocator>> const&) const 
/home/ivanmurashko/local/llvm-project/clang/lib/AST/ASTContext.cpp:226:57

The corresponding lit test that reproduces that crash was also added


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131675

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/AST/ast-crash-doc.cpp


Index: clang/test/AST/ast-crash-doc.cpp
===
--- /dev/null
+++ clang/test/AST/ast-crash-doc.cpp
@@ -0,0 +1,30 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -emit-module -x c++ -fmodules -I %t/Inputs -fmodule-name=aa 
%t/Inputs/module.modulemap -o %t/aa.pcm
+// RUN: rm %t/Inputs/b.h
+// RUN: not %clang_cc1 -x c++ -Wdocumentation -ast-dump-all -fmodules -I 
%t/Inputs -fmodule-file=%t/aa.pcm %t/test.cpp | FileCheck %s
+
+//--- Inputs/module.modulemap
+module aa {
+header "a.h"
+header "b.h"
+}
+
+//--- Inputs/a.h
+// empty file
+
+//--- Inputs/b.h
+/// test foo @return
+int foo();
+
+
+//--- test.cpp
+#include "a.h"
+
+/// test comment at the primary file
+
+int a = foo();
+
+
+// CHECK: TranslationUnitDecl
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -298,6 +298,9 @@
 return nullptr;
 
   const FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
+  if (!File.isValid()) {
+return nullptr;
+  }
   const auto CommentsInThisFile = Comments.getCommentsInFile(File);
   if (!CommentsInThisFile || CommentsInThisFile->empty())
 return nullptr;


Index: clang/test/AST/ast-crash-doc.cpp
===
--- /dev/null
+++ clang/test/AST/ast-crash-doc.cpp
@@ -0,0 +1,30 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -emit-module -x c++ -fmodules -I %t/Inputs -fmodule-name=aa %t/Inputs/module.modulemap -o %t/aa.pcm
+// RUN: rm %t/Inputs/b.h
+// RUN: not %clang_cc1 -x c++ -Wdocumentation -ast-dump-all -fmodules -I %t/Inputs -fmodule-file=%t/aa.pcm %t/test.cpp | FileCheck %s
+
+//--- Inputs/module.modulemap
+module aa {
+header "a.h"
+header "b.h"
+}
+
+//--- Inputs/a.h
+// empty file
+
+//--- Inputs/b.h
+/// test foo @return
+int foo();
+
+
+//--- test.cpp
+#include "a.h"
+
+/// test comment at the primary file
+
+int a = foo();
+
+
+// CHECK: TranslationUnitDecl
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -298,6 +298,9 @@
 return nullptr;
 
   const FileID File = SourceMgr.getDecomposedLoc(DeclLoc).first;
+  if (!File.isValid()) {
+return nullptr;
+  }
   const auto CommentsInThisFile = Comments.getCommentsInFile(File);
   if (!CommentsInThisFile || CommentsInThisFile->empty())
 return nullptr;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130847: [clang] SourceManager: fix at SourceManager::getFileIDLoaded for the case of invalid SLockEntry

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe78c80e3d622: [clang] SourceManager: fix at 
SourceManager::getFileIDLoaded for the case of… (authored by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

Files:
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -877,9 +877,12 @@
 I = (-LastID - 2) + 1;
 
   unsigned NumProbes;
+  bool Invalid = false;
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(I, );
+if (Invalid)
+  return FileID(); // invalid entry.
 if (E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
@@ -897,8 +900,8 @@
   while (true) {
 ++NumProbes;
 unsigned MiddleIndex = (LessIndex - GreaterIndex) / 2 + GreaterIndex;
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex);
-if (E.getOffset() == 0)
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex, );
+if (Invalid)
   return FileID(); // invalid entry.
 
 ++NumProbes;


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -877,9 +877,12 @@
 I = (-LastID - 2) + 1;
 
   unsigned NumProbes;
+  bool Invalid = false;
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(I, );
+if (Invalid)
+  return FileID(); // invalid entry.
 if (E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
@@ -897,8 +900,8 @@
   while (true) {
 ++NumProbes;
 unsigned MiddleIndex = (LessIndex - GreaterIndex) / 2 + GreaterIndex;
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex);
-if (E.getOffset() == 0)
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex, );
+if (Invalid)
   return FileID(); // invalid entry.
 
 ++NumProbes;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130847: [clang] SourceManager: fix at SourceManager::getFileIDLoaded for the case of invalid SLockEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 451464.
ivanmurashko added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

Files:
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -877,9 +877,12 @@
 I = (-LastID - 2) + 1;
 
   unsigned NumProbes;
+  bool Invalid = false;
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(I, );
+if (Invalid)
+  return FileID(); // invalid entry.
 if (E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
@@ -897,8 +900,8 @@
   while (true) {
 ++NumProbes;
 unsigned MiddleIndex = (LessIndex - GreaterIndex) / 2 + GreaterIndex;
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex);
-if (E.getOffset() == 0)
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex, );
+if (Invalid)
   return FileID(); // invalid entry.
 
 ++NumProbes;


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -877,9 +877,12 @@
 I = (-LastID - 2) + 1;
 
   unsigned NumProbes;
+  bool Invalid = false;
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(I, );
+if (Invalid)
+  return FileID(); // invalid entry.
 if (E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
@@ -897,8 +900,8 @@
   while (true) {
 ++NumProbes;
 unsigned MiddleIndex = (LessIndex - GreaterIndex) / 2 + GreaterIndex;
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex);
-if (E.getOffset() == 0)
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex, );
+if (Invalid)
   return FileID(); // invalid entry.
 
 ++NumProbes;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130847: [clang] SourceManager: fix at SourceManager::getFileIDLoaded for the case of invalid SLockEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 451166.
ivanmurashko added a comment.

Use Invalid flag to detect invalid SLocEntry


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

Files:
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -877,9 +877,12 @@
 I = (-LastID - 2) + 1;
 
   unsigned NumProbes;
+  bool Invalid = false;
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(I, );
+if (Invalid)
+  return FileID(); // invalid entry.
 if (E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
@@ -897,8 +900,8 @@
   while (true) {
 ++NumProbes;
 unsigned MiddleIndex = (LessIndex - GreaterIndex) / 2 + GreaterIndex;
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex);
-if (E.getOffset() == 0)
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex, );
+if (Invalid)
   return FileID(); // invalid entry.
 
 ++NumProbes;


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -877,9 +877,12 @@
 I = (-LastID - 2) + 1;
 
   unsigned NumProbes;
+  bool Invalid = false;
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(I, );
+if (Invalid)
+  return FileID(); // invalid entry.
 if (E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
@@ -897,8 +900,8 @@
   while (true) {
 ++NumProbes;
 unsigned MiddleIndex = (LessIndex - GreaterIndex) / 2 + GreaterIndex;
-const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex);
-if (E.getOffset() == 0)
+const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex, );
+if (Invalid)
   return FileID(); // invalid entry.
 
 ++NumProbes;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130847: [clang] SourceManager: fix at SourceManager::getFileIDLoaded for the case of invalid SLockEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang/lib/Basic/SourceManager.cpp:882-884
 const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+if (E.getOffset() == 0)
+  return FileID(); // invalid entry.

aaron.ballman wrote:
> This looks incorrect to me -- I don't think an offset of `0` is invalid. I 
> think a better way to do this is:
> ```
> bool Invalid;
> const SrcMgr::SLocEntry  = getLoadedSLocEntry(I, );
> if (Invalid)
>   return FileID(); // Invalid entry.
> ```
That's reasonable, do we want to update other places at the function as well 
(line 903 for instance)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

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

[PATCH] D130847: [clang] SourceManager: fix at SourceManager::getFileIDLoaded for the case of invalid SLockEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D130847#3693668 , @aaron.ballman 
wrote:

> These changes look reasonable, but I verified that the precommit CI failures 
> are valid -- it looks like this change broke a test somehow; perhaps a caller 
> was relying on the old behavior and needs to be reworked?

I fixed the problem. It seems to be reasonable to abort the search procedure at 
the case of invalid `SLockEntrie`s. That also compatible with existent tests. 
I updated the patch title and summary as well.

@aaron.ballman , could you look at it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

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

[PATCH] D130847: [clang] SourceManager: fix isOffsetInFileID for the case of a fake SLocEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 450989.
ivanmurashko added a comment.

stop search if we could not load an entry


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

Files:
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -880,6 +880,8 @@
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
 const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+if (E.getOffset() == 0)
+  return FileID(); // invalid entry.
 if (E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -880,6 +880,8 @@
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
 const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
+if (E.getOffset() == 0)
+  return FileID(); // invalid entry.
 if (E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130847: [clang] SourceManager: fix isOffsetInFileID for the case of a fake SLocEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 450750.
ivanmurashko added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

Files:
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -880,7 +880,7 @@
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
 const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
-if (E.getOffset() <= SLocOffset) {
+if (E.getOffset() != 0 && E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
   NumLinearScans += NumProbes + 1;


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -880,7 +880,7 @@
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
 const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
-if (E.getOffset() <= SLocOffset) {
+if (E.getOffset() != 0 && E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
   NumLinearScans += NumProbes + 1;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130847: [clang] SourceManager: fix isOffsetInFileID for the case of a fake SLocEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D130847#3693668 , @aaron.ballman 
wrote:

> These changes look reasonable, but I verified that the precommit CI failures 
> are valid -- it looks like this change broke a test somehow; perhaps a caller 
> was relying on the old behavior and needs to be reworked?

Ah, I have to verify and fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

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

[PATCH] D130847: [clang] SourceManager: fix isOffsetInFileID for the case of a fake SLocEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang/lib/Basic/SourceManager.cpp:901
 const SrcMgr::SLocEntry  = getLoadedSLocEntry(MiddleIndex);
 if (E.getOffset() == 0)
   return FileID(); // invalid entry.

FYI: the same check is used for the rest of the search procedure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

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

[PATCH] D130847: [clang] SourceManager: fix isOffsetInFileID for the case of a fake SLocEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D130847#3690977 , @aaron.ballman 
wrote:

> 



> Given that this code is on the hot path, should it be the caller's 
> responsibility to have already validated the `FileID` that's passed in so 
> that the fake entry can never be returned?

That is a good point. The crash that I am trying to fix uses incorrectly 
assigned `SourceManager::LastFileIDLookup`, see SourceManager::getFileID 
.
 It's worth avoiding the incorrect cache value assignment. I could determine 
the place at the code where the assignment was made and updated the patch 
accordingly.

@aaron.ballman, could you look at the update. Is it reasonable?

Note: the change introduces a check that similar to one made previously 
 for the 
rest of the search procedure




Comment at: clang/include/clang/Basic/SourceManager.h:1112-1113
 // If our one-entry cache covers this offset, just return it.
 if (isOffsetInFileID(LastFileIDLookup, SLocOffset))
   return LastFileIDLookup;
 

nit: There is the place when `isOffsetInFileID` produces a wrong result at my 
case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

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

[PATCH] D130847: [clang] SourceManager: fix isOffsetInFileID for the case of a fake SLocEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 449261.
ivanmurashko added a comment.

Fix for wrong LastFileIDLookup assignment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

Files:
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -880,7 +880,7 @@
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
 const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
-if (E.getOffset() <= SLocOffset) {
+if (E.getOffset() != 0 && E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
   NumLinearScans += NumProbes + 1;


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -880,7 +880,7 @@
   for (NumProbes = 0; NumProbes < 8; ++NumProbes, ++I) {
 // Make sure the entry is loaded!
 const SrcMgr::SLocEntry  = getLoadedSLocEntry(I);
-if (E.getOffset() <= SLocOffset) {
+if (E.getOffset() != 0 && E.getOffset() <= SLocOffset) {
   FileID Res = FileID::get(-int(I) - 2);
   LastFileIDLookup = Res;
   NumLinearScans += NumProbes + 1;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130847: [clang] SourceManager: fix isOffsetInFileID for the case of a fake SLocEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 448885.
ivanmurashko added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130847

Files:
  clang/include/clang/Basic/SourceManager.h


Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1825,7 +1825,13 @@
   /// specified SourceLocation offset.  This is a very hot method.
   inline bool isOffsetInFileID(FileID FID,
SourceLocation::UIntTy SLocOffset) const {
-const SrcMgr::SLocEntry  = getSLocEntry(FID);
+bool Invalid = false;
+const SrcMgr::SLocEntry  = getSLocEntry(FID, );
+
+// If the entry is invalid, it can't contain it.
+if (Invalid)
+  return false;
+
 // If the entry is after the offset, it can't contain it.
 if (SLocOffset < Entry.getOffset()) return false;
 


Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1825,7 +1825,13 @@
   /// specified SourceLocation offset.  This is a very hot method.
   inline bool isOffsetInFileID(FileID FID,
SourceLocation::UIntTy SLocOffset) const {
-const SrcMgr::SLocEntry  = getSLocEntry(FID);
+bool Invalid = false;
+const SrcMgr::SLocEntry  = getSLocEntry(FID, );
+
+// If the entry is invalid, it can't contain it.
+if (Invalid)
+  return false;
+
 // If the entry is after the offset, it can't contain it.
 if (SLocOffset < Entry.getOffset()) return false;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D130847: [clang] SourceManager: fix isOffsetInFileID for the case of a fake SLocEntry

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: dexonsmith, aaron.ballman.
ivanmurashko added a project: clang.
Herald added subscribers: usaxena95, kadircet.
Herald added a project: All.
ivanmurashko requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.

The `getSLocEntry` might return `SourceManager::FakeSLocEntryForRecovery` 
introduced at D89748 . The result entry is not 
tested and as result the call

  return SLocOffset < getSLocEntryByID(FID.ID+1).getOffset();

might return `true` value. That would mark the tested offset as one that 
belongs to the fake `SLockEntry`.

It might cause a weird clangd crash when preamble contains some header files 
that were removed just after the preamble created. Unfortunately it's not so 
easy to reproduce the crash in the form of a LIT test thus I provided the fix 
only.

Test Plan:

  ninja check-clang


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130847

Files:
  clang/include/clang/Basic/SourceManager.h


Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1825,7 +1825,13 @@
   /// specified SourceLocation offset.  This is a very hot method.
   inline bool isOffsetInFileID(FileID FID,
SourceLocation::UIntTy SLocOffset) const {
-const SrcMgr::SLocEntry  = getSLocEntry(FID);
+bool Invalid = false;
+const SrcMgr::SLocEntry  = getSLocEntry(FID, );
+
+// If the entry is invalid, it can't contain it.
+if (Invalid)
+  return false;
+
 // If the entry is after the offset, it can't contain it.
 if (SLocOffset < Entry.getOffset()) return false;
 


Index: clang/include/clang/Basic/SourceManager.h
===
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -1825,7 +1825,13 @@
   /// specified SourceLocation offset.  This is a very hot method.
   inline bool isOffsetInFileID(FileID FID,
SourceLocation::UIntTy SLocOffset) const {
-const SrcMgr::SLocEntry  = getSLocEntry(FID);
+bool Invalid = false;
+const SrcMgr::SLocEntry  = getSLocEntry(FID, );
+
+// If the entry is invalid, it can't contain it.
+if (Invalid)
+  return false;
+
 // If the entry is after the offset, it can't contain it.
 if (SLocOffset < Entry.getOffset()) return false;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101471: [clang-tidy] Add proper emplace checks to modernize-use-emplace

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG987f9cb6b970: [clang-tidy] Add proper emplace checks to 
modernize-use-emplace (authored by nicovank, committed by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101471

Files:
  clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseEmplaceCheck.h
  clang-tools-extra/docs/clang-tidy/checks/modernize-use-emplace.rst
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-emplace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-emplace.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-emplace.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-emplace.cpp
@@ -10,15 +10,36 @@
 
 namespace std {
 template 
-class initializer_list
-{
+class initializer_list {
 public:
   initializer_list() noexcept {}
 };
 
+template 
+class pair {
+public:
+  pair() = default;
+  pair(const pair &) = default;
+  pair(pair &&) = default;
+
+  pair(const T1 &, const T2 &) {}
+  pair(T1 &&, T2 &&) {}
+
+  template 
+  pair(const pair &){};
+  template 
+  pair(pair &&){};
+};
+
 template 
 class vector {
 public:
+  using value_type = T;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
   vector() = default;
   vector(initializer_list) {}
 
@@ -27,54 +48,230 @@
 
   template 
   void emplace_back(Args &&... args){};
+  template 
+  iterator emplace(const_iterator pos, Args &&...args){};
   ~vector();
 };
+
 template 
 class list {
 public:
+  using value_type = T;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
+  template 
+  iterator emplace(const_iterator pos, Args &&...args){};
   template 
   void emplace_back(Args &&... args){};
+  template 
+  void emplace_front(Args &&...args){};
   ~list();
 };
 
 template 
 class deque {
 public:
+  using value_type = T;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
   void push_back(const T &) {}
   void push_back(T &&) {}
 
+  template 
+  iterator emplace(const_iterator pos, Args &&...args){};
   template 
   void emplace_back(Args &&... args){};
+  template 
+  void emplace_front(Args &&...args){};
   ~deque();
 };
 
-template  struct remove_reference { using type = T; };
-template  struct remove_reference { using type = T; };
-template  struct remove_reference { using type = T; };
+template 
+class forward_list {
+public:
+  using value_type = T;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
+  template 
+  void emplace_front(Args &&...args){};
+  template 
+  iterator emplace_after(const_iterator pos, Args &&...args){};
+};
 
-template  class pair {
+template 
+class set {
 public:
-  pair() = default;
-  pair(const pair &) = default;
-  pair(pair &&) = default;
+  using value_type = T;
 
-  pair(const T1 &, const T2 &) {}
-  pair(T1 &&, T2 &&) {}
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
+  template 
+  void emplace(Args &&...args){};
+  template 
+  iterator emplace_hint(const_iterator pos, Args &&...args){};
+};
+
+template 
+class map {
+public:
+  using value_type = std::pair;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
+  template 
+  void emplace(Args &&...args){};
+  template 
+  iterator emplace_hint(const_iterator pos, Args &&...args){};
+};
+
+template 
+class multiset {
+public:
+  using value_type = T;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
+  template 
+  void emplace(Args &&...args){};
+  template 
+  iterator emplace_hint(const_iterator pos, Args &&...args){};
+};
+
+template 
+class multimap {
+public:
+  using value_type = std::pair;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
+  template 
+  void emplace(Args &&...args){};
+  template 
+  iterator emplace_hint(const_iterator pos, Args &&...args){};
+};
+
+template 
+class unordered_set {
+public:
+  using value_type = T;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
+  template 
+  void emplace(Args &&...args){};
+  template 
+  iterator emplace_hint(const_iterator pos, Args &&...args){};
+};
+
+template 
+class unordered_map {
+public:
+  using value_type = std::pair;
+
+  class iterator {};
+  class const_iterator {};
+  const_iterator begin() { return const_iterator{}; }
+
+  template 
+  void emplace(Args &&...args){};
+  

[PATCH] D101471: [clang-tidy] Add proper emplace checks to modernize-use-emplace

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko accepted this revision.
ivanmurashko added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101471

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

[PATCH] D94727: [clangd] Retire some flags for uncontroversial, stable features.

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.
Herald added projects: clang-tools-extra, All.



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:762
 Opts.ResourceDir = ResourceDir;
-  Opts.BuildDynamicSymbolIndex = EnableIndex;
+  Opts.BuildDynamicSymbolIndex = true;
   Opts.CollectMainFileRefs = CollectMainFileRefs;

@sammccall The option is always true, do we need it as an option?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94727

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

[PATCH] D124840: [RFC] Add and sort decl to maintain order instead of inserting in order

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2deebc0048f9: [RFC] Add and sort decl to maintain order 
instead of inserting in order (authored by kuganv, committed by ivanmurashko).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124840

Files:
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3114,6 +3114,7 @@
   for (auto  : SortedFileDeclIDs) {
 DeclIDInFileInfo  = *FileDeclEntry.second;
 Info.FirstDeclIndex = FileGroupedDeclIDs.size();
+llvm::stable_sort(Info.DeclIDs);
 for (auto  : Info.DeclIDs)
   FileGroupedDeclIDs.push_back(LocDeclEntry.second);
   }
@@ -5462,16 +5463,7 @@
 
   std::pair LocDecl(Offset, ID);
   LocDeclIDsTy  = Info->DeclIDs;
-
-  if (Decls.empty() || Decls.back().first <= Offset) {
-Decls.push_back(LocDecl);
-return;
-  }
-
-  LocDeclIDsTy::iterator I =
-  llvm::upper_bound(Decls, LocDecl, llvm::less_first());
-
-  Decls.insert(I, LocDecl);
+  Decls.push_back(LocDecl);
 }
 
 unsigned ASTWriter::getAnonymousDeclarationNumber(const NamedDecl *D) {


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3114,6 +3114,7 @@
   for (auto  : SortedFileDeclIDs) {
 DeclIDInFileInfo  = *FileDeclEntry.second;
 Info.FirstDeclIndex = FileGroupedDeclIDs.size();
+llvm::stable_sort(Info.DeclIDs);
 for (auto  : Info.DeclIDs)
   FileGroupedDeclIDs.push_back(LocDeclEntry.second);
   }
@@ -5462,16 +5463,7 @@
 
   std::pair LocDecl(Offset, ID);
   LocDeclIDsTy  = Info->DeclIDs;
-
-  if (Decls.empty() || Decls.back().first <= Offset) {
-Decls.push_back(LocDecl);
-return;
-  }
-
-  LocDeclIDsTy::iterator I =
-  llvm::upper_bound(Decls, LocDecl, llvm::less_first());
-
-  Decls.insert(I, LocDecl);
+  Decls.push_back(LocDecl);
 }
 
 unsigned ASTWriter::getAnonymousDeclarationNumber(const NamedDecl *D) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124719: [docs] PCH usage documentation update

[Ivan Murashko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf6112f490cfe: [docs] PCH usage documentation update 
(authored by ivanmurashko).

Changed prior to commit:
  https://reviews.llvm.org/D124719?vs=426588=426589#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124719

Files:
  clang/docs/UsersManual.rst


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1219,23 +1219,22 @@
 Using a PCH File
 
 
-A PCH file can then be used as a prefix header when a :option:`-include`
+A PCH file can then be used as a prefix header when a :option:`-include-pch`
 option is passed to ``clang``:
 
 .. code-block:: console
 
-  $ clang -include test.h test.c -o test
+  $ clang -include-pch test.h.pch test.c -o test
 
-The ``clang`` driver will first check if a PCH file for ``test.h`` is
+The ``clang`` driver will check if the PCH file ``test.h.pch`` is
 available; if so, the contents of ``test.h`` (and the files it includes)
-will be processed from the PCH file. Otherwise, Clang falls back to
-directly processing the content of ``test.h``. This mirrors the behavior
-of GCC.
+will be processed from the PCH file. Otherwise, Clang will report an error.
 
 .. note::
 
   Clang does *not* automatically use PCH files for headers that are directly
-  included within a source file. For example:
+  included within a source file or indirectly via :option:`-include`.
+  For example:
 
   .. code-block:: console
 
@@ -1246,7 +1245,7 @@
 
   In this example, ``clang`` will not automatically use the PCH file for
   ``test.h`` since ``test.h`` was included directly in the source file and not
-  specified on the command line using :option:`-include`.
+  specified on the command line using :option:`-include-pch`.
 
 Relocatable PCH Files
 ^


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1219,23 +1219,22 @@
 Using a PCH File
 
 
-A PCH file can then be used as a prefix header when a :option:`-include`
+A PCH file can then be used as a prefix header when a :option:`-include-pch`
 option is passed to ``clang``:
 
 .. code-block:: console
 
-  $ clang -include test.h test.c -o test
+  $ clang -include-pch test.h.pch test.c -o test
 
-The ``clang`` driver will first check if a PCH file for ``test.h`` is
+The ``clang`` driver will check if the PCH file ``test.h.pch`` is
 available; if so, the contents of ``test.h`` (and the files it includes)
-will be processed from the PCH file. Otherwise, Clang falls back to
-directly processing the content of ``test.h``. This mirrors the behavior
-of GCC.
+will be processed from the PCH file. Otherwise, Clang will report an error.
 
 .. note::
 
   Clang does *not* automatically use PCH files for headers that are directly
-  included within a source file. For example:
+  included within a source file or indirectly via :option:`-include`.
+  For example:
 
   .. code-block:: console
 
@@ -1246,7 +1245,7 @@
 
   In this example, ``clang`` will not automatically use the PCH file for
   ``test.h`` since ``test.h`` was included directly in the source file and not
-  specified on the command line using :option:`-include`.
+  specified on the command line using :option:`-include-pch`.
 
 Relocatable PCH Files
 ^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124719: [docs] PCH usage documentation update

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 426588.
ivanmurashko added a comment.

Some clarifications for the direct include example were added


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124719

Files:
  clang/docs/UsersManual.rst


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1219,23 +1219,22 @@
 Using a PCH File
 
 
-A PCH file can then be used as a prefix header when a :option:`-include`
+A PCH file can then be used as a prefix header when a :option:`-include-pch`
 option is passed to ``clang``:
 
 .. code-block:: console
 
-  $ clang -include test.h test.c -o test
+  $ clang -include-pch test.h.pch test.c -o test
 
-The ``clang`` driver will first check if a PCH file for ``test.h`` is
+The ``clang`` driver will check if the PCH file ``test.h.pch`` is
 available; if so, the contents of ``test.h`` (and the files it includes)
-will be processed from the PCH file. Otherwise, Clang falls back to
-directly processing the content of ``test.h``. This mirrors the behavior
-of GCC.
+will be processed from the PCH file. Otherwise, Clang will report an error.
 
 .. note::
 
   Clang does *not* automatically use PCH files for headers that are directly
-  included within a source file. For example:
+  included within a source file or indirectly via :option:`-include`.
+  For example:
 
   .. code-block:: console
 
@@ -1246,8 +1245,8 @@
 
   In this example, ``clang`` will not automatically use the PCH file for
   ``test.h`` since ``test.h`` was included directly in the source file and not
-  specified on the command line using :option:`-include`.
-
+  specified on the command line using :option:`-include-pch`.
+  
 Relocatable PCH Files
 ^
 


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1219,23 +1219,22 @@
 Using a PCH File
 
 
-A PCH file can then be used as a prefix header when a :option:`-include`
+A PCH file can then be used as a prefix header when a :option:`-include-pch`
 option is passed to ``clang``:
 
 .. code-block:: console
 
-  $ clang -include test.h test.c -o test
+  $ clang -include-pch test.h.pch test.c -o test
 
-The ``clang`` driver will first check if a PCH file for ``test.h`` is
+The ``clang`` driver will check if the PCH file ``test.h.pch`` is
 available; if so, the contents of ``test.h`` (and the files it includes)
-will be processed from the PCH file. Otherwise, Clang falls back to
-directly processing the content of ``test.h``. This mirrors the behavior
-of GCC.
+will be processed from the PCH file. Otherwise, Clang will report an error.
 
 .. note::
 
   Clang does *not* automatically use PCH files for headers that are directly
-  included within a source file. For example:
+  included within a source file or indirectly via :option:`-include`.
+  For example:
 
   .. code-block:: console
 
@@ -1246,8 +1245,8 @@
 
   In this example, ``clang`` will not automatically use the PCH file for
   ``test.h`` since ``test.h`` was included directly in the source file and not
-  specified on the command line using :option:`-include`.
-
+  specified on the command line using :option:`-include-pch`.
+  
 Relocatable PCH Files
 ^
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124719: [clang,doc] PCH usage documentation update

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 426248.
ivanmurashko added a comment.

Minor changes at the doc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124719

Files:
  clang/docs/UsersManual.rst


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1219,23 +1219,22 @@
 Using a PCH File
 
 
-A PCH file can then be used as a prefix header when a :option:`-include`
+A PCH file can then be used as a prefix header when a :option:`-include-pch`
 option is passed to ``clang``:
 
 .. code-block:: console
 
-  $ clang -include test.h test.c -o test
+  $ clang -include-pch test.h.pch test.c -o test
 
-The ``clang`` driver will first check if a PCH file for ``test.h`` is
+The ``clang`` driver will check if the PCH file ``test.h.pch`` is
 available; if so, the contents of ``test.h`` (and the files it includes)
-will be processed from the PCH file. Otherwise, Clang falls back to
-directly processing the content of ``test.h``. This mirrors the behavior
-of GCC.
+will be processed from the PCH file. Otherwise, Clang will report an error.
 
 .. note::
 
   Clang does *not* automatically use PCH files for headers that are directly
-  included within a source file. For example:
+  included within a source file or indirectly via :option:`-include`.
+  For example:
 
   .. code-block:: console
 
@@ -1244,9 +1243,6 @@
 #include "test.h"
 $ clang test.c -o test
 
-  In this example, ``clang`` will not automatically use the PCH file for
-  ``test.h`` since ``test.h`` was included directly in the source file and not
-  specified on the command line using :option:`-include`.
 
 Relocatable PCH Files
 ^


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1219,23 +1219,22 @@
 Using a PCH File
 
 
-A PCH file can then be used as a prefix header when a :option:`-include`
+A PCH file can then be used as a prefix header when a :option:`-include-pch`
 option is passed to ``clang``:
 
 .. code-block:: console
 
-  $ clang -include test.h test.c -o test
+  $ clang -include-pch test.h.pch test.c -o test
 
-The ``clang`` driver will first check if a PCH file for ``test.h`` is
+The ``clang`` driver will check if the PCH file ``test.h.pch`` is
 available; if so, the contents of ``test.h`` (and the files it includes)
-will be processed from the PCH file. Otherwise, Clang falls back to
-directly processing the content of ``test.h``. This mirrors the behavior
-of GCC.
+will be processed from the PCH file. Otherwise, Clang will report an error.
 
 .. note::
 
   Clang does *not* automatically use PCH files for headers that are directly
-  included within a source file. For example:
+  included within a source file or indirectly via :option:`-include`.
+  For example:
 
   .. code-block:: console
 
@@ -1244,9 +1243,6 @@
 #include "test.h"
 $ clang test.c -o test
 
-  In this example, ``clang`` will not automatically use the PCH file for
-  ``test.h`` since ``test.h`` was included directly in the source file and not
-  specified on the command line using :option:`-include`.
 
 Relocatable PCH Files
 ^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124719: [clang,doc] PCH usage documentation update

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
Herald added a project: All.
ivanmurashko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The documentation has an incorrect example of PCH files usage via `-include` 
option. The possibility has not been available since llvm-svn: 174385 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124719

Files:
  clang/docs/UsersManual.rst


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1219,23 +1219,22 @@
 Using a PCH File
 
 
-A PCH file can then be used as a prefix header when a :option:`-include`
+A PCH file can then be used as a prefix header when a :option:`-include-pch`
 option is passed to ``clang``:
 
 .. code-block:: console
 
-  $ clang -include test.h test.c -o test
+  $ clang -include-pch test.h.pch test.c -o test
 
-The ``clang`` driver will first check if a PCH file for ``test.h`` is
-available; if so, the contents of ``test.h`` (and the files it includes)
-will be processed from the PCH file. Otherwise, Clang falls back to
-directly processing the content of ``test.h``. This mirrors the behavior
-of GCC.
+The ``clang`` driver will check if the PCH file ``test.h.pch`` is
+available; if so, the contents of ``test.h.pch`` (and the files it includes)
+will be processed from the PCH file. Otherwise, Clang will report an error.
 
 .. note::
 
   Clang does *not* automatically use PCH files for headers that are directly
-  included within a source file. For example:
+  included within a source file or indirectly via :option:`-include`.
+  For example:
 
   .. code-block:: console
 
@@ -1244,9 +1243,6 @@
 #include "test.h"
 $ clang test.c -o test
 
-  In this example, ``clang`` will not automatically use the PCH file for
-  ``test.h`` since ``test.h`` was included directly in the source file and not
-  specified on the command line using :option:`-include`.
 
 Relocatable PCH Files
 ^


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -1219,23 +1219,22 @@
 Using a PCH File
 
 
-A PCH file can then be used as a prefix header when a :option:`-include`
+A PCH file can then be used as a prefix header when a :option:`-include-pch`
 option is passed to ``clang``:
 
 .. code-block:: console
 
-  $ clang -include test.h test.c -o test
+  $ clang -include-pch test.h.pch test.c -o test
 
-The ``clang`` driver will first check if a PCH file for ``test.h`` is
-available; if so, the contents of ``test.h`` (and the files it includes)
-will be processed from the PCH file. Otherwise, Clang falls back to
-directly processing the content of ``test.h``. This mirrors the behavior
-of GCC.
+The ``clang`` driver will check if the PCH file ``test.h.pch`` is
+available; if so, the contents of ``test.h.pch`` (and the files it includes)
+will be processed from the PCH file. Otherwise, Clang will report an error.
 
 .. note::
 
   Clang does *not* automatically use PCH files for headers that are directly
-  included within a source file. For example:
+  included within a source file or indirectly via :option:`-include`.
+  For example:
 
   .. code-block:: console
 
@@ -1244,9 +1243,6 @@
 #include "test.h"
 $ clang test.c -o test
 
-  In this example, ``clang`` will not automatically use the PCH file for
-  ``test.h`` since ``test.h`` was included directly in the source file and not
-  specified on the command line using :option:`-include`.
 
 Relocatable PCH Files
 ^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:416
 
+  auto CallMoveMatcherLambda = lambdaExpr(
+  forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(

sammccall wrote:
> sammccall wrote:
> > do we want to bind the lambda itself as `moving-call`?
> we should probably have a comment explaining *why* lambdas get handled 
> specially.
> 
> If I understand right:
> - the normal matcher would already match
> - but either MovingCall or ContainingLambda (which?) point at unhelpful nodes 
> and
> - we end up doing the analysis inside the lambda rather than in the enclosing 
> function
> - so never find the following use
> 
> (I wonder if it's possible to fix this slightly more directly by tweaking the 
> MovingCall or ContainingLambda logic)
> If I understand right:

There are some troubles with the original matcher. The most obvious one is 
correctly described at your comment :
The original matcher
```
callExpr(callee(functionDecl(hasName("::std::move"))),
   ... hasAncestor(lambdaExpr().bind("containing-lambda")),
   ...
```
applied to the code
```
auto []() { // lambda_1
   int a = 0;
   ...
   auto [](aa = std::move(a)) { // lambda_2
   }
}
```
will return *lambda_2* binded to the "containing-lambda" but we expect it to be 
*lambda_1*



> (I wonder if it's possible to fix this slightly more directly by tweaking the 
> MovingCall or ContainingLambda logic)
It would be good to find it if it's possible



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:1356
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;

sammccall wrote:
> It's pretty interesting that use-after-move fires for ints!
> 
> Someone might decide to "fix" that though, so probably best to use A like the 
> other tests.
There is also another case that I want to address as a separate patch.
```
void autoCapture() {
  auto var = [](auto &) {
auto f = [blk = std::move(res)]() {};
return std::move(res);
  };
}
```
This one is matched as `UnresolvedLookupExpr` and requires another modified 
matcher


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119165

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

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG71d7c8d870db: [clangd] Crash in __memcmp_avx2_movbe 
(authored by ivanmurashko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

Files:
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/unittests/Tooling/HeaderIncludesTest.cpp


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -51,6 +51,15 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, RepeatedIncludes) {
+  std::string Code;
+  for (int i = 0; i < 100; ++i) {
+Code += "#include \"a.h\"\n";
+  }
+  std::string Expected = Code + "#include \"a2.h\"\n";
+  EXPECT_EQ(Expected, insert(Code, "\"a2.h\""));
+}
+
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
   std::string Code = "#ifndef A_H\n"
  "#define A_H\n"
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -14,6 +14,7 @@
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include 
 #include 
 
 namespace clang {
@@ -97,7 +98,8 @@
   // Map from include name (quotation trimmed) to a list of existing includes
   // (in case there are more than one) with the name in the current file. 
   // and "x" will be treated as the same header when deleting #includes.
-  llvm::StringMap> ExistingIncludes;
+  // std::list is used for pointers stability (see IncludesByPriority)
+  llvm::StringMap> ExistingIncludes;
 
   /// Map from priorities of #include categories to all #includes in the same
   /// category. This is used to find #includes of the same category when


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -51,6 +51,15 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, RepeatedIncludes) {
+  std::string Code;
+  for (int i = 0; i < 100; ++i) {
+Code += "#include \"a.h\"\n";
+  }
+  std::string Expected = Code + "#include \"a2.h\"\n";
+  EXPECT_EQ(Expected, insert(Code, "\"a2.h\""));
+}
+
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
   std::string Code = "#ifndef A_H\n"
  "#define A_H\n"
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -14,6 +14,7 @@
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include 
 #include 
 
 namespace clang {
@@ -97,7 +98,8 @@
   // Map from include name (quotation trimmed) to a list of existing includes
   // (in case there are more than one) with the name in the current file. 
   // and "x" will be treated as the same header when deleting #includes.
-  llvm::StringMap> ExistingIncludes;
+  // std::list is used for pointers stability (see IncludesByPriority)
+  llvm::StringMap> ExistingIncludes;
 
   /// Map from priorities of #include categories to all #includes in the same
   /// category. This is used to find #includes of the same category when
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119165: [clang-tidy] Add processing lambda captures at bugprone-use-after-move check

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: alexfh, sammccall, mboehme, aaron.ballman.
ivanmurashko added projects: clang, clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
ivanmurashko requested review of this revision.
Herald added a subscriber: cfe-commits.

The diff adds processing for `std::move` inside lambda captures at 
`bugprone-use-after-move` check. Especially it detects invalid `std::move` 
inside following code

  int foo() {
int a = 0;
auto fun = [aa = std::move(a)]{
  return aa;
};
return a;
  }

Test Plan

  ./bin/llvm-lit -v  
../clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119165

Files:
  clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -1350,3 +1350,18 @@
 private:
   std::string val_;
 };
+
+// Check std::move usage inside lambda captures
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;
+  int c = 0;
+  auto foo = [aa = std::move(a), bb = std::move(b), cc = c] {
+// CHECK-NOTES: [[@LINE+6]]:10: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-2]]:20: note: move occurred here
+// CHECK-NOTES: [[@LINE+4]]:14: warning: 'b' used after it was moved
+// CHECK-NOTES: [[@LINE-4]]:39: note: move occurred here
+return aa + bb + cc;
+  };
+  return a + b + c;
+};
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -397,11 +397,13 @@
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
-  auto CallMoveMatcher =
+  auto AncestorMatcher =
+  anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
+hasAncestor(functionDecl().bind("containing-func")));
+
+  auto BaseMoveMatcher =
   callExpr(callee(functionDecl(hasName("::std::move"))), 
argumentCountIs(1),
hasArgument(0, declRefExpr().bind("arg")),
-   anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
- hasAncestor(functionDecl().bind("containing-func"))),
unless(inDecltypeOrTemplateArg()),
// try_emplace is a common maybe-moving function that returns a
// bool to tell callers whether it moved. Ignore std::move 
inside
@@ -411,6 +413,16 @@
callee(cxxMethodDecl(hasName("try_emplace")))
   .bind("call-move");
 
+  auto CallMoveMatcherLambda = lambdaExpr(
+  forEachLambdaCapture(lambdaCapture(capturesVar(varDecl(
+  hasInitializer(expr(ignoringParenImpCasts(BaseMoveMatcher))),
+  AncestorMatcher);
+
+  Finder->addMatcher(CallMoveMatcherLambda, this);
+
+  auto CallMoveMatcherNoLambda =
+  expr(ignoringParenImpCasts(BaseMoveMatcher), AncestorMatcher);
+
   Finder->addMatcher(
   traverse(
   TK_AsIs,
@@ -418,7 +430,7 @@
   // for the direct ancestor of the std::move() that isn't one of the
   // node types ignored by ignoringParenImpCasts().
   stmt(
-  forEach(expr(ignoringParenImpCasts(CallMoveMatcher))),
+  forEach(CallMoveMatcherNoLambda),
   // Don't allow an InitListExpr to be the moving call. An
   // InitListExpr has both a syntactic and a semantic form, and the
   // parent-child relationships are different between the two. This


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -1350,3 +1350,18 @@
 private:
   std::string val_;
 };
+
+// Check std::move usage inside lambda captures
+int lambdaCaptures() {
+  int a = 0;
+  int b = 0;
+  int c = 0;
+  auto foo = [aa = std::move(a), bb = std::move(b), cc = c] {
+// CHECK-NOTES: [[@LINE+6]]:10: warning: 'a' used after it was moved
+// CHECK-NOTES: [[@LINE-2]]:20: note: move occurred here
+// CHECK-NOTES: [[@LINE+4]]:14: warning: 'b' used after it was moved
+// CHECK-NOTES: [[@LINE-4]]:39: note: move occurred here
+return aa + bb + cc;
+  };
+  return a + b + c;
+};
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ 

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

In D118755#3301776 , @sammccall wrote:

> Do you have access to commit this or should I land it for you?

I would appreciate your assistance in the land. You can use the following email 
address: `ivan.murashko at gmail.com`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

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

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 406421.
ivanmurashko added a comment.

comment update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

Files:
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/unittests/Tooling/HeaderIncludesTest.cpp


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -51,6 +51,15 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, RepeatedIncludes) {
+  std::string Code;
+  for (int i = 0; i < 100; ++i) {
+Code += "#include \"a.h\"\n";
+  }
+  std::string Expected = Code + "#include \"a2.h\"\n";
+  EXPECT_EQ(Expected, insert(Code, "\"a2.h\""));
+}
+
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
   std::string Code = "#ifndef A_H\n"
  "#define A_H\n"
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -14,6 +14,7 @@
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include 
 #include 
 
 namespace clang {
@@ -97,7 +98,8 @@
   // Map from include name (quotation trimmed) to a list of existing includes
   // (in case there are more than one) with the name in the current file. 
   // and "x" will be treated as the same header when deleting #includes.
-  llvm::StringMap> ExistingIncludes;
+  // std::list is used for pointers stability (see IncludesByPriority)
+  llvm::StringMap> ExistingIncludes;
 
   /// Map from priorities of #include categories to all #includes in the same
   /// category. This is used to find #includes of the same category when


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -51,6 +51,15 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, RepeatedIncludes) {
+  std::string Code;
+  for (int i = 0; i < 100; ++i) {
+Code += "#include \"a.h\"\n";
+  }
+  std::string Expected = Code + "#include \"a2.h\"\n";
+  EXPECT_EQ(Expected, insert(Code, "\"a2.h\""));
+}
+
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
   std::string Code = "#ifndef A_H\n"
  "#define A_H\n"
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -14,6 +14,7 @@
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include 
 #include 
 
 namespace clang {
@@ -97,7 +98,8 @@
   // Map from include name (quotation trimmed) to a list of existing includes
   // (in case there are more than one) with the name in the current file. 
   // and "x" will be treated as the same header when deleting #includes.
-  llvm::StringMap> ExistingIncludes;
+  // std::list is used for pointers stability (see IncludesByPriority)
+  llvm::StringMap> ExistingIncludes;
 
   /// Map from priorities of #include categories to all #includes in the same
   /// category. This is used to find #includes of the same category when
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 406420.
ivanmurashko edited the summary of this revision.
ivanmurashko added a comment.

Comment about std::list was added


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

Files:
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/unittests/Tooling/HeaderIncludesTest.cpp


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -51,6 +51,15 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, RepeatedIncludes) {
+  std::string Code;
+  for (int i = 0; i < 100; ++i) {
+Code += "#include \"a.h\"\n";
+  }
+  std::string Expected = Code + "#include \"a2.h\"\n";
+  EXPECT_EQ(Expected, insert(Code, "\"a2.h\""));
+}
+
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
   std::string Code = "#ifndef A_H\n"
  "#define A_H\n"
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -14,6 +14,7 @@
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include 
 #include 
 
 namespace clang {
@@ -97,7 +98,9 @@
   // Map from include name (quotation trimmed) to a list of existing includes
   // (in case there are more than one) with the name in the current file. 
   // and "x" will be treated as the same header when deleting #includes.
-  llvm::StringMap> ExistingIncludes;
+  // std::list is used for pointers stability that are referenced by
+  // IncludesByPriority
+  llvm::StringMap> ExistingIncludes;
 
   /// Map from priorities of #include categories to all #includes in the same
   /// category. This is used to find #includes of the same category when


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -51,6 +51,15 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, RepeatedIncludes) {
+  std::string Code;
+  for (int i = 0; i < 100; ++i) {
+Code += "#include \"a.h\"\n";
+  }
+  std::string Expected = Code + "#include \"a2.h\"\n";
+  EXPECT_EQ(Expected, insert(Code, "\"a2.h\""));
+}
+
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
   std::string Code = "#ifndef A_H\n"
  "#define A_H\n"
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -14,6 +14,7 @@
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include 
 #include 
 
 namespace clang {
@@ -97,7 +98,9 @@
   // Map from include name (quotation trimmed) to a list of existing includes
   // (in case there are more than one) with the name in the current file. 
   // and "x" will be treated as the same header when deleting #includes.
-  llvm::StringMap> ExistingIncludes;
+  // std::list is used for pointers stability that are referenced by
+  // IncludesByPriority
+  llvm::StringMap> ExistingIncludes;
 
   /// Map from priorities of #include categories to all #includes in the same
   /// category. This is used to find #includes of the same category when
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:100
   // and "x" will be treated as the same header when deleting #includes.
   llvm::StringMap> ExistingIncludes;
 

sammccall wrote:
> An alternative would be to use a std::forward_list here.
> 
> This guarantees pointer stability, it's an extra allocation but seems 
> unlikely to matter.
> 
> It would be more robust if the data structure changes (e.g. becomes large, or 
> is mutated after creation) but probably none of this will ever happen.
> 
> Up to you.
I changed my solution accordingly this suggestion. 
Small note: The `std::list` seems to be more suitable here as soon as the last 
element of the structure is used at the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

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

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 406411.
ivanmurashko added a comment.

There are some changes:

- unit test for HeaderIncludes was added
- lit test for clangd was removed
- Solution uses std::list instead of copying by value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

Files:
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/unittests/Tooling/HeaderIncludesTest.cpp


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -51,6 +51,15 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, RepeatedIncludes) {
+  std::string Code;
+  for (int i = 0; i < 100; ++i) {
+Code += "#include \"a.h\"\n";
+  }
+  std::string Expected = Code + "#include \"a2.h\"\n";
+  EXPECT_EQ(Expected, insert(Code, "\"a2.h\""));
+}
+
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
   std::string Code = "#ifndef A_H\n"
  "#define A_H\n"
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -14,6 +14,7 @@
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include 
 #include 
 
 namespace clang {
@@ -97,7 +98,7 @@
   // Map from include name (quotation trimmed) to a list of existing includes
   // (in case there are more than one) with the name in the current file. 
   // and "x" will be treated as the same header when deleting #includes.
-  llvm::StringMap> ExistingIncludes;
+  llvm::StringMap> ExistingIncludes;
 
   /// Map from priorities of #include categories to all #includes in the same
   /// category. This is used to find #includes of the same category when


Index: clang/unittests/Tooling/HeaderIncludesTest.cpp
===
--- clang/unittests/Tooling/HeaderIncludesTest.cpp
+++ clang/unittests/Tooling/HeaderIncludesTest.cpp
@@ -51,6 +51,15 @@
   EXPECT_EQ(Expected, insert(Code, "\"a.h\""));
 }
 
+TEST_F(HeaderIncludesTest, RepeatedIncludes) {
+  std::string Code;
+  for (int i = 0; i < 100; ++i) {
+Code += "#include \"a.h\"\n";
+  }
+  std::string Expected = Code + "#include \"a2.h\"\n";
+  EXPECT_EQ(Expected, insert(Code, "\"a2.h\""));
+}
+
 TEST_F(HeaderIncludesTest, NoExistingIncludeWithDefine) {
   std::string Code = "#ifndef A_H\n"
  "#define A_H\n"
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -14,6 +14,7 @@
 #include "clang/Tooling/Inclusions/IncludeStyle.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
+#include 
 #include 
 
 namespace clang {
@@ -97,7 +98,7 @@
   // Map from include name (quotation trimmed) to a list of existing includes
   // (in case there are more than one) with the name in the current file. 
   // and "x" will be treated as the same header when deleting #includes.
-  llvm::StringMap> ExistingIncludes;
+  llvm::StringMap> ExistingIncludes;
 
   /// Map from priorities of #include categories to all #includes in the same
   /// category. This is used to find #includes of the same category when
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 405886.
ivanmurashko added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

Files:
  clang-tools-extra/clangd/test/repeated_includes.test
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp


Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -333,7 +333,7 @@
 int Priority = Categories.getIncludePriority(
 CurInclude.Name, /*CheckMainHeader=*/FirstIncludeOffset < 0);
 CategoryEndOffsets[Priority] = NextLineOffset;
-IncludesByPriority[Priority].push_back();
+IncludesByPriority[Priority].push_back(CurInclude);
 if (FirstIncludeOffset < 0)
   FirstIncludeOffset = CurInclude.R.getOffset();
   }
@@ -361,9 +361,9 @@
   unsigned InsertOffset = CatOffset->second; // Fall back offset
   auto Iter = IncludesByPriority.find(Priority);
   if (Iter != IncludesByPriority.end()) {
-for (const auto *Inc : Iter->second) {
-  if (QuotedName < Inc->Name) {
-InsertOffset = Inc->R.getOffset();
+for (const auto  : Iter->second) {
+  if (QuotedName < Inc.Name) {
+InsertOffset = Inc.R.getOffset();
 break;
   }
 }
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -105,8 +105,7 @@
   /// in the order they appear in the source file.
   /// See comment for "FormatStyle::IncludeCategories" for details about 
include
   /// priorities.
-  std::unordered_map>
-  IncludesByPriority;
+  std::unordered_map> IncludesByPriority;
 
   int FirstIncludeOffset;
   // All new headers should be inserted after this offset (e.g. after header
Index: clang-tools-extra/clangd/test/repeated_includes.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/repeated_includes.test
@@ -0,0 +1,33 @@
+# RUN: rm -rf %/t
+# RUN: mkdir -p %t && touch %t/t.h && touch %t/t2.h && touch %t/t3.h
+# RUN: echo '#pragma once' > %t/t.h
+# RUN: echo '#include "t2.h"' >> %t/t.h
+# RUN: echo 'void bar();' >> %t/t.h
+# RUN: echo '#pragma once' > %t/t2.h
+# RUN: echo 'void bar2();' >> %t/t2.h
+
+
+# Run clangd
+# RUN: clangd -lit-test -log error --path-mappings 'C:\client=%t' < %s | 
FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///C:/client/main.cpp","languageId":"cpp","version":1,"text":"#include
 \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\nbar();\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///C:/client/main.cpp"},"position":{"line":40,"character":3}}}
+#  CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": {
+# CHECK-NEXT:"isIncomplete": false,
+# CHECK-NEXT:"items": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"detail": "void",
+# CHECK-NEXT:"documentation": {
+# CHECK-NEXT:  "kind": "plaintext",
+# CHECK-NEXT:  "value": "From \"t.h\""
+# CHECK-NEXT:},
+# CHECK-NEXT:"filterText": "bar",
+# CHECK-NEXT:"insertText": "bar",
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}


Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -333,7 +333,7 @@
 int Priority = Categories.getIncludePriority(
 CurInclude.Name, /*CheckMainHeader=*/FirstIncludeOffset < 0);
 CategoryEndOffsets[Priority] = NextLineOffset;
-IncludesByPriority[Priority].push_back();
+IncludesByPriority[Priority].push_back(CurInclude);
 

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 405814.
ivanmurashko added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

Files:
  clang-tools-extra/clangd/test/repeated_includes.test
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp


Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -333,7 +333,7 @@
 int Priority = Categories.getIncludePriority(
 CurInclude.Name, /*CheckMainHeader=*/FirstIncludeOffset < 0);
 CategoryEndOffsets[Priority] = NextLineOffset;
-IncludesByPriority[Priority].push_back();
+IncludesByPriority[Priority].push_back(CurInclude);
 if (FirstIncludeOffset < 0)
   FirstIncludeOffset = CurInclude.R.getOffset();
   }
@@ -361,9 +361,9 @@
   unsigned InsertOffset = CatOffset->second; // Fall back offset
   auto Iter = IncludesByPriority.find(Priority);
   if (Iter != IncludesByPriority.end()) {
-for (const auto *Inc : Iter->second) {
-  if (QuotedName < Inc->Name) {
-InsertOffset = Inc->R.getOffset();
+for (const auto  : Iter->second) {
+  if (QuotedName < Inc.Name) {
+InsertOffset = Inc.R.getOffset();
 break;
   }
 }
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -105,8 +105,7 @@
   /// in the order they appear in the source file.
   /// See comment for "FormatStyle::IncludeCategories" for details about 
include
   /// priorities.
-  std::unordered_map>
-  IncludesByPriority;
+  std::unordered_map> IncludesByPriority;
 
   int FirstIncludeOffset;
   // All new headers should be inserted after this offset (e.g. after header
Index: clang-tools-extra/clangd/test/repeated_includes.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/repeated_includes.test
@@ -0,0 +1,33 @@
+# RUN: rm -rf %/t
+# RUN: mkdir -p %t && touch %t/t.h && touch %t/t2.h && touch %t/t3.h
+# RUN: echo '#pragma once' > %t/t.h
+# RUN: echo '#include "t2.h"' >> %t/t.h
+# RUN: echo 'void bar();' >> %t/t.h
+# RUN: echo '#pragma once' > %t/t2.h
+# RUN: echo 'void bar2();' >> %t/t2.h
+
+
+# Run clangd
+# RUN: clangd -lit-test -log error --path-mappings 'C:\client=%t' < %s | 
FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///C:/client/main.cpp","languageId":"cpp","version":1,"text":"#include
 \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\nbar();\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///C:/client/main.cpp"},"position":{"line":40,"character":3}}}
+#  CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": {
+# CHECK-NEXT:"isIncomplete": false,
+# CHECK-NEXT:"items": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"detail": "void",
+# CHECK-NEXT:"documentation": {
+# CHECK-NEXT:  "kind": "plaintext",
+# CHECK-NEXT:  "value": "From \"t.h\""
+# CHECK-NEXT:},
+# CHECK-NEXT:"filterText": "bar",
+# CHECK-NEXT:"insertText": "bar",
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}


Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -333,7 +333,7 @@
 int Priority = Categories.getIncludePriority(
 CurInclude.Name, /*CheckMainHeader=*/FirstIncludeOffset < 0);
 CategoryEndOffsets[Priority] = NextLineOffset;
-IncludesByPriority[Priority].push_back();
+IncludesByPriority[Priority].push_back(CurInclude);
 

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:328
   ExistingIncludes.try_emplace(trimInclude(IncludeToAdd.Name)).first;
   Iter->second.push_back(std::move(IncludeToAdd));
   auto  = Iter->second.back();

The crash is caused by vector resize initiated by this line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118755

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

[PATCH] D118755: [clangd] Crash in __memcmp_avx2_movbe

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: alexfh, DmitryPolukhin, sammccall, bruno.
ivanmurashko added projects: clang, clang-tools-extra.
Herald added subscribers: usaxena95, kadircet, arphaman.
ivanmurashko requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.

There is a clangd crash at `__memcmp_avx2_movbe`. Short problem description is 
below.

The method `HeaderIncludes::addExistingInclude` stores `Include` objects by 
reference at 2 places: `ExistingIncludes` (primary storage) and 
`IncludesByPriority` (pointer to the object's location at ExistingIncludes). 
`ExistingIncludes` is a map where value is a `SmallVector`. A new element is 
inserted by `push_back`. The operation might do resize. As result pointers 
stored at `IncludesByPriority` might become invalid.

Typical stack trace

  frame #0: 0x7f11460dcd94 libc.so.6`__memcmp_avx2_movbe + 308
  frame #1: 0x004782b8 clangd`llvm::StringRef::compareMemory(Lhs="
  \"t2.h\"", Rhs="", Length=6) at StringRef.h:76:22
  frame #2: 0x00701253 clangd`llvm::StringRef::compare(this=0x
  7f10de7d8610, RHS=(Data = "", Length = 7166742329480737377)) const at String
  Ref.h:206:34
* frame #3: 0x007603ab clangd`llvm::operator<(llvm::StringRef, llv
  m::StringRef)(LHS=(Data = "\"t2.h\"", Length = 6), RHS=(Data = "", Length =
  7166742329480737377)) at StringRef.h:907:23
  frame #4: 0x02d0ad9f clangd`clang::tooling::HeaderIncludes::inse
  rt(this=0x7f10de7fb1a0, IncludeName=(Data = "t2.h\"", Length = 4), IsAng
  led=false) const at HeaderIncludes.cpp:365:22
  frame #5: 0x012ebfdd clangd`clang::clangd::IncludeInserter::inse
  rt(this=0x7f10de7fb148, VerbatimHeader=(Data = "\"t2.h\"", Length = 6))
  const at Headers.cpp:262:70

A clangd lit test for the crash was created. The proposed solution uses copy by 
value instead of copy by reference.

Test Plan

  ./bin/llvm-lit -v ../clang-tools-extra/clangd/test/repeated_includes.test


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118755

Files:
  clang-tools-extra/clangd/test/repeated_includes.test
  clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
  clang/lib/Tooling/Inclusions/HeaderIncludes.cpp


Index: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
===
--- clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
+++ clang/lib/Tooling/Inclusions/HeaderIncludes.cpp
@@ -333,7 +333,7 @@
 int Priority = Categories.getIncludePriority(
 CurInclude.Name, /*CheckMainHeader=*/FirstIncludeOffset < 0);
 CategoryEndOffsets[Priority] = NextLineOffset;
-IncludesByPriority[Priority].push_back();
+IncludesByPriority[Priority].push_back(CurInclude);
 if (FirstIncludeOffset < 0)
   FirstIncludeOffset = CurInclude.R.getOffset();
   }
@@ -361,9 +361,9 @@
   unsigned InsertOffset = CatOffset->second; // Fall back offset
   auto Iter = IncludesByPriority.find(Priority);
   if (Iter != IncludesByPriority.end()) {
-for (const auto *Inc : Iter->second) {
-  if (QuotedName < Inc->Name) {
-InsertOffset = Inc->R.getOffset();
+for (const auto  : Iter->second) {
+  if (QuotedName < Inc.Name) {
+InsertOffset = Inc.R.getOffset();
 break;
   }
 }
Index: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
===
--- clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
+++ clang/include/clang/Tooling/Inclusions/HeaderIncludes.h
@@ -105,8 +105,7 @@
   /// in the order they appear in the source file.
   /// See comment for "FormatStyle::IncludeCategories" for details about 
include
   /// priorities.
-  std::unordered_map>
-  IncludesByPriority;
+  std::unordered_map> IncludesByPriority;
 
   int FirstIncludeOffset;
   // All new headers should be inserted after this offset (e.g. after header
Index: clang-tools-extra/clangd/test/repeated_includes.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/repeated_includes.test
@@ -0,0 +1,33 @@
+# RUN: rm -rf %/t
+# RUN: mkdir -p %t && touch %t/t.h && touch %t/t2.h && touch %t/t3.h
+# RUN: echo '#pragma once' > %t/t.h
+# RUN: echo '#include "t2.h"' >> %t/t.h
+# RUN: echo 'void bar();' >> %t/t.h
+# RUN: echo '#pragma once' > %t/t2.h
+# RUN: echo 'void bar2();' >> %t/t2.h
+
+
+# Run clangd
+# RUN: clangd -lit-test -log error --path-mappings 'C:\client=%t' < %s | 
FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///C:/client/main.cpp","languageId":"cpp","version":1,"text":"#include
 \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include \"t.h\"\n#include 
\"t.h\"\n#include 

[PATCH] D107944: [hmaptool] Port to python3

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added inline comments.



Comment at: clang/utils/hmaptool/hmaptool:223
 with open(output_path, 'wb') as f:
 f.write(magic.encode())
 f.write(struct.pack(header_fmt, *header))

`magic` is a bytes object that does not have `encode()` method 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107944

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

[PATCH] D106583: [clang] SIGSEGV at DeduceTemplateArgumentsByTypeMatch

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

@erichkeane, could you look at the added LIT test. Is it suitable or require 
additional modifications?

BTW: There are several failed tests but they seems to be a known issue (see bug 
51117 ).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106583

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

[PATCH] D106583: [clang] SIGSEGV at DeduceTemplateArgumentsByTypeMatch

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 361005.
ivanmurashko added a comment.

LIT test was updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106583

Files:
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/test/SemaCXX/pr51171-crash.cpp


Index: clang/test/SemaCXX/pr51171-crash.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/pr51171-crash.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+// Ensure that we don't crash if errors are suppressed by an error limit.
+// RUN: not %clang_cc1 -fsyntax-only -std=c++17 -ferror-limit 1 %s
+
+template 
+struct tv_val {
+};
+
+template 
+auto (const tv_val ) { return val.val(); } // expected-note 
{{possible target for call}}
+
+struct Class {
+  template 
+  struct Entry {
+tv_val val;
+  };
+};
+
+enum Types : int {
+  Class = 1, // expected-note 2 {{struct 'Class' is hidden}}
+};
+
+struct Record {
+  Class *val_;// expected-error {{must use 'struct' tag}}
+  void setClass(Class *); // expected-error {{must use 'struct' tag}}
+};
+
+void Record::setClass(Class *val) { // expected-error {{variable has 
incomplete type 'void'}} \
+   // expected-error {{reference to overloaded 
function}} \
+   // expected-error {{expected ';' after top 
level declarator}}
+  val_ = val;
+}
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -4346,7 +4346,7 @@
 HasDeducedReturnType = true;
   }
 
-  if (!ArgFunctionType.isNull()) {
+  if (!ArgFunctionType.isNull() && !FunctionType.isNull()) {
 unsigned TDF =
 TDF_TopLevelParameterTypeList | TDF_AllowCompatibleFunctionType;
 // Deduce template arguments from the function type.


Index: clang/test/SemaCXX/pr51171-crash.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/pr51171-crash.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+// Ensure that we don't crash if errors are suppressed by an error limit.
+// RUN: not %clang_cc1 -fsyntax-only -std=c++17 -ferror-limit 1 %s
+
+template 
+struct tv_val {
+};
+
+template 
+auto (const tv_val ) { return val.val(); } // expected-note {{possible target for call}}
+
+struct Class {
+  template 
+  struct Entry {
+tv_val val;
+  };
+};
+
+enum Types : int {
+  Class = 1, // expected-note 2 {{struct 'Class' is hidden}}
+};
+
+struct Record {
+  Class *val_;// expected-error {{must use 'struct' tag}}
+  void setClass(Class *); // expected-error {{must use 'struct' tag}}
+};
+
+void Record::setClass(Class *val) { // expected-error {{variable has incomplete type 'void'}} \
+   // expected-error {{reference to overloaded function}} \
+   // expected-error {{expected ';' after top level declarator}}
+  val_ = val;
+}
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -4346,7 +4346,7 @@
 HasDeducedReturnType = true;
   }
 
-  if (!ArgFunctionType.isNull()) {
+  if (!ArgFunctionType.isNull() && !FunctionType.isNull()) {
 unsigned TDF =
 TDF_TopLevelParameterTypeList | TDF_AllowCompatibleFunctionType;
 // Deduce template arguments from the function type.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106583: [clang] SIGSEGV at DeduceTemplateArgumentsByTypeMatch

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 361001.
ivanmurashko added a comment.

The LIT test was added:

  build] ./bin/llvm-lit -v ../clang/test/SemaCXX/pr51171-crash.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106583

Files:
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/test/SemaCXX/pr51171-crash.cpp


Index: clang/test/SemaCXX/pr51171-crash.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/pr51171-crash.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+// Ensure that we don't crash if errors are suppressed by an error limit.
+// RUN: not %clang_cc1 -fsyntax-only -std=c++17 -ferror-limit 1 %s
+
+template 
+struct tv_val {
+};
+
+template 
+auto (const tv_val ) { return val.val(); } // expected-note 
{{possible target for call}}
+
+struct Class {
+  template 
+  struct Entry {
+tv_val val;
+  };
+};
+
+enum Types : int {
+  Class = 1, // expected-note {{struct 'Class' is hidden}} \
+   // expected-note {{struct 'Class' is hidden}}
+};
+
+struct Record {
+  Class *val_;// expected-error {{must use 'struct' tag}}
+  void setClass(Class *); // expected-error {{must use 'struct' tag}}
+};
+
+void Record::setClass(Class *val) { // expected-error {{variable has 
incomplete type 'void'}} \
+   // expected-error {{reference to overloaded 
function}} \
+   // expected-error {{expected ';' after top 
level declarator}}
+  val_ = val;
+}
+
+/// expected-fatal {{too many errors emitted, stopping now [-ferror-limit=]}}
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -4346,7 +4346,7 @@
 HasDeducedReturnType = true;
   }
 
-  if (!ArgFunctionType.isNull()) {
+  if (!ArgFunctionType.isNull() && !FunctionType.isNull()) {
 unsigned TDF =
 TDF_TopLevelParameterTypeList | TDF_AllowCompatibleFunctionType;
 // Deduce template arguments from the function type.


Index: clang/test/SemaCXX/pr51171-crash.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/pr51171-crash.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+// Ensure that we don't crash if errors are suppressed by an error limit.
+// RUN: not %clang_cc1 -fsyntax-only -std=c++17 -ferror-limit 1 %s
+
+template 
+struct tv_val {
+};
+
+template 
+auto (const tv_val ) { return val.val(); } // expected-note {{possible target for call}}
+
+struct Class {
+  template 
+  struct Entry {
+tv_val val;
+  };
+};
+
+enum Types : int {
+  Class = 1, // expected-note {{struct 'Class' is hidden}} \
+   // expected-note {{struct 'Class' is hidden}}
+};
+
+struct Record {
+  Class *val_;// expected-error {{must use 'struct' tag}}
+  void setClass(Class *); // expected-error {{must use 'struct' tag}}
+};
+
+void Record::setClass(Class *val) { // expected-error {{variable has incomplete type 'void'}} \
+   // expected-error {{reference to overloaded function}} \
+   // expected-error {{expected ';' after top level declarator}}
+  val_ = val;
+}
+
+/// expected-fatal {{too many errors emitted, stopping now [-ferror-limit=]}}
Index: clang/lib/Sema/SemaTemplateDeduction.cpp
===
--- clang/lib/Sema/SemaTemplateDeduction.cpp
+++ clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -4346,7 +4346,7 @@
 HasDeducedReturnType = true;
   }
 
-  if (!ArgFunctionType.isNull()) {
+  if (!ArgFunctionType.isNull() && !FunctionType.isNull()) {
 unsigned TDF =
 TDF_TopLevelParameterTypeList | TDF_AllowCompatibleFunctionType;
 // Deduce template arguments from the function type.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106583: [clang] SIGSEGV at DeduceTemplateArgumentsByTypeMatch

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko added a comment.

@erichkeane, is it OK if the reproducer will be added as the LIT test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106583

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

[PATCH] D106583: [clang] SIGSEGV at DeduceTemplateArgumentsByTypeMatch

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: rsmith, DmitryPolukhin.
ivanmurashko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

There is a SIGSEGV at DeduceTemplateArgumentsByTypeMatch. See BUG description 
and reproducer at https://bugs.llvm.org/show_bug.cgi?id=51171

The stack trace is below:

   #0 0x055afcb9 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
/home/ivanmurashko/local/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:22
   #1 0x055afd70 PrintStackTraceSignalHandler(void*) 
/home/ivanmurashko/local/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
   #2 0x055add2d llvm::sys::RunSignalHandlers() 
/home/ivanmurashko/local/llvm-project/llvm/lib/Support/Signals.cpp:97:20
   #3 0x055af701 SignalHandler(int) 
/home/ivanmurashko/local/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
   #4 0x77bc2b20 __restore_rt sigaction.c:0:0
   #5 0x766a337f raise (/lib64/libc.so.6+0x3737f)
   #6 0x7668ddb5 abort (/lib64/libc.so.6+0x21db5)
   #7 0x7668dc89 _nl_load_domain.cold.0 loadmsgcat.c:0:0
   #8 0x7669ba76 .annobin___GI___assert_fail.end assert.c:0:0
   #9 0x0594b210 clang::QualType::getCommonPtr() const 
/home/ivanmurashko/local/llvm-project/clang/include/clang/AST/Type.h:684:5
  #10 0x05a12ca6 clang::QualType::getCanonicalType() const 
/home/ivanmurashko/local/llvm-project/clang/include/clang/AST/Type.h:6467:36
  #11 0x05a137a6 clang::ASTContext::getCanonicalType(clang::QualType) 
const 
/home/ivanmurashko/local/llvm-project/clang/include/clang/AST/ASTContext.h:2433:58
  #12 0x09204584 DeduceTemplateArgumentsByTypeMatch(clang::Sema&, 
clang::TemplateParameterList*, clang::QualType, clang::QualType, 
clang::sema::TemplateDeductionInfo&, 
llvm::SmallVectorImpl&, unsigned int, bool, 
bool) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaTemplateDeduction.cpp:1355:54
  #13 0x0920df0d 
clang::Sema::DeduceTemplateArguments(clang::FunctionTemplateDecl*, 
clang::TemplateArgumentListInfo*, clang::QualType, clang::FunctionDecl*&, 
clang::sema::TemplateDeductionInfo&, bool) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaTemplateDeduction.cpp:4354:47
  #14 0x09012b09 (anonymous 
namespace)::AddressOfFunctionResolver::AddMatchingTemplateFunction(clang::FunctionTemplateDecl*,
 clang::DeclAccessPair const&) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:12026:38
  #15 0x09013030 (anonymous 
namespace)::AddressOfFunctionResolver::FindAllFunctionsThatMatchTargetTypeExactly()
 /home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:12119:9
  #16 0x09012679 (anonymous 
namespace)::AddressOfFunctionResolver::AddressOfFunctionResolver(clang::Sema&, 
clang::Expr*, clang::QualType const&, bool) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:11931:5
  #17 0x09013c91 
clang::Sema::ResolveAddressOfOverloadedFunction(clang::Expr*, clang::QualType, 
bool, clang::DeclAccessPair&, bool*) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:12286:42
  #18 0x08fed85d IsStandardConversion(clang::Sema&, clang::Expr*, 
clang::QualType, bool, clang::StandardConversionSequence&, bool, bool) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:1712:49
  #19 0x08fec8ea TryImplicitConversion(clang::Sema&, clang::Expr*, 
clang::QualType, bool, clang::Sema::AllowedExplicit, bool, bool, bool, bool) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:1433:27
  #20 0x08ff90ba TryCopyInitialization(clang::Sema&, clang::Expr*, 
clang::QualType, bool, bool, bool, bool) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:5273:71
  #21 0x090024fb clang::Sema::AddBuiltinCandidate(clang::QualType*, 
llvm::ArrayRef, clang::OverloadCandidateSet&, bool, unsigned int) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:7755:32
  #22 0x0900513f (anonymous 
namespace)::BuiltinOperatorOverloadBuilder::addGenericBinaryArithmeticOverloads()
 /home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:8633:30
  #23 0x09007624 
clang::Sema::AddBuiltinOperatorCandidates(clang::OverloadedOperatorKind, 
clang::SourceLocation, llvm::ArrayRef, 
clang::OverloadCandidateSet&) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:9205:51
  #24 0x09018734 
clang::Sema::LookupOverloadedBinOp(clang::OverloadCandidateSet&, 
clang::OverloadedOperatorKind, clang::UnresolvedSetImpl const&, 
llvm::ArrayRef, bool) 
/home/ivanmurashko/local/llvm-project/clang/lib/Sema/SemaOverload.cpp:13469:1
  #25 0x09018d56 
clang::Sema::CreateOverloadedBinOp(clang::SourceLocation, 
clang::BinaryOperatorKind, clang::UnresolvedSetImpl const&, clang::Expr*, 
clang::Expr*, bool, bool, 

[PATCH] D104021: [clang-tidy] LIT test fix for Remark diagnostic

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: aaron.ballman, alexfh, DmitryPolukhin.
ivanmurashko added projects: clang, clang-tools-extra.
Herald added a subscriber: xazax.hun.
ivanmurashko requested review of this revision.
Herald added a subscriber: cfe-commits.

There is a followup fix for a unit test introduced at D102906 
. The test file was placed into a temp folder 
and test assumed that it would be visible without the full path specification.

This behaviour can be changed in future and it would be good to specify full 
path to the file at the test.

Test Plan:

  ninja check-clang-tools


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104021

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp
@@ -2,14 +2,13 @@
 // RUN: cp -r %S/Inputs/remarks %t
 // RUN: cp %s %t/t.cpp
 
-// RUN: clang-tidy 
-checks='-*,modernize-use-override,clang-diagnostic-module-import' t.cpp -- \
+// RUN: clang-tidy 
-checks='-*,modernize-use-override,clang-diagnostic-module-import' %t/t.cpp -- \
 // RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
 // RUN: -fsyntax-only \
 // RUN: -I%S/Inputs/remarks \
 // RUN: -working-directory=%t \
-// RUN: -Rmodule-build -Rmodule-import t.cpp 2>&1 |\
+// RUN: -Rmodule-build -Rmodule-import 2>&1 |\
 // RUN: FileCheck %s -implicit-check-not "remark:"
 
 #include "A.h"
 // CHECK: remark: importing module 'A' from {{.*}} 
[clang-diagnostic-module-import]
-


Index: clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp
@@ -2,14 +2,13 @@
 // RUN: cp -r %S/Inputs/remarks %t
 // RUN: cp %s %t/t.cpp
 
-// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-module-import' t.cpp -- \
+// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-module-import' %t/t.cpp -- \
 // RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
 // RUN: -fsyntax-only \
 // RUN: -I%S/Inputs/remarks \
 // RUN: -working-directory=%t \
-// RUN: -Rmodule-build -Rmodule-import t.cpp 2>&1 |\
+// RUN: -Rmodule-build -Rmodule-import 2>&1 |\
 // RUN: FileCheck %s -implicit-check-not "remark:"
 
 #include "A.h"
 // CHECK: remark: importing module 'A' from {{.*}} [clang-diagnostic-module-import]
-
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102906: [clang-tidy] Remark was added to clang tooling Diagnostic

[Ivan Murashko via Phabricator via cfe-commits]

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

Hi @aaron.ballman

> Thanks! Do you need someone to commit on your behalf?

I will appreciate your assistance in the landing the diff.

> If so, what name and email address would you like used for patch attribution?

It would be good to use mine - Ivan Murashko (ivan.murashko at gmail.com) for 
that




Comment at: clang/include/clang/Tooling/Core/Diagnostic.h:72
+Error = DiagnosticsEngine::Error,
+Remark = DiagnosticsEngine::Remark
   };

aaron.ballman wrote:
> DmitryPolukhin wrote:
> > nit, I would move remark first to have list by increasing severity. It 
> > looks like this enum values is not used outside of the sources so I hope it 
> > is safe.
> We have diagnostic notes as well and it's not clear to me how to order remark 
> and note should we go that way in the future. So I'm not opposed to the 
> suggestion, but the order likely doesn't matter all that much.
@DmitryPolukhin fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102906

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

[PATCH] D102906: [clang-tidy] Remark was added to clang tooling Diagnostic

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko updated this revision to Diff 347326.
ivanmurashko added a comment.

@DmitryPolukhin suggestion was applied


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102906

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/Inputs/remarks/A.h
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/remarks/module.modulemap
  clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp
  clang/include/clang/Tooling/Core/Diagnostic.h
  clang/include/clang/Tooling/DiagnosticsYaml.h
  clang/unittests/Tooling/DiagnosticsYamlTest.cpp

Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -47,10 +47,11 @@
  const std::string , int FileOffset,
  const std::string ,
  const StringMap ,
- const SmallVector ) {
+ const SmallVector ,
+ Diagnostic::Level DiagnosticLevel) {
   return Diagnostic(DiagnosticName,
 makeMessage(Message, FileOffset, FilePath, Fix, Ranges), {},
-Diagnostic::Warning, "path/to/build/directory");
+DiagnosticLevel, "path/to/build/directory");
 }
 
 static const char *YAMLContent =
@@ -102,6 +103,14 @@
 "Replacements:[]\n"
 "Level:   Warning\n"
 "BuildDirectory:  'path/to/build/directory'\n"
+"  - DiagnosticName:  'diagnostic#4'\n"
+"DiagnosticMessage:\n"
+"  Message: 'message #4'\n"
+"  FilePath:'path/to/source3.cpp'\n"
+"  FileOffset:  72\n"
+"  Replacements:[]\n"
+"Level:   Remark\n"
+"BuildDirectory:  'path/to/build/directory'\n"
 "...\n";
 
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
@@ -112,7 +121,8 @@
   {"path/to/source.cpp",
Replacements({"path/to/source.cpp", 100, 12, "replacement #1"})}};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55,
-   "path/to/source.cpp", Fix1, {}));
+   "path/to/source.cpp", Fix1, {},
+   Diagnostic::Warning));
 
   StringMap Fix2 = {
   {"path/to/header.h",
@@ -120,15 +130,21 @@
   SmallVector Ranges2 =
   {makeByteRange(10, 10, "path/to/source.cpp")};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60,
-   "path/to/header.h", Fix2, Ranges2));
+   "path/to/header.h", Fix2, Ranges2,
+   Diagnostic::Warning));
 
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
-   "path/to/source2.cpp", {}, {}));
+   "path/to/source2.cpp", {}, {},
+   Diagnostic::Warning));
   TUD.Diagnostics.back().Notes.push_back(
   makeMessage("Note1", 88, "path/to/note1.cpp", {}, {}));
   TUD.Diagnostics.back().Notes.push_back(
   makeMessage("Note2", 99, "path/to/note2.cpp", {}, {}));
 
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#4", "message #4", 72,
+   "path/to/source3.cpp", {}, {},
+   Diagnostic::Remark));
+
   std::string YamlContent;
   raw_string_ostream YamlContentStream(YamlContent);
 
@@ -144,7 +160,7 @@
   YAML >> TUDActual;
 
   ASSERT_FALSE(YAML.error());
-  ASSERT_EQ(3u, TUDActual.Diagnostics.size());
+  ASSERT_EQ(4u, TUDActual.Diagnostics.size());
   EXPECT_EQ("path/to/source.cpp", TUDActual.MainSourceFile);
 
   auto getFixes = [](const StringMap ) {
Index: clang/include/clang/Tooling/DiagnosticsYaml.h
===
--- clang/include/clang/Tooling/DiagnosticsYaml.h
+++ clang/include/clang/Tooling/DiagnosticsYaml.h
@@ -106,6 +106,7 @@
   static void enumeration(IO , clang::tooling::Diagnostic::Level ) {
 IO.enumCase(Value, "Warning", clang::tooling::Diagnostic::Warning);
 IO.enumCase(Value, "Error", clang::tooling::Diagnostic::Error);
+IO.enumCase(Value, "Remark", clang::tooling::Diagnostic::Remark);
   }
 };
 
Index: clang/include/clang/Tooling/Core/Diagnostic.h
===
--- clang/include/clang/Tooling/Core/Diagnostic.h
+++ clang/include/clang/Tooling/Core/Diagnostic.h
@@ -67,6 +67,7 @@
 /// fixes to be applied.
 struct Diagnostic {
   enum Level {
+Remark = 

[PATCH] D102906: [clang-tidy] Remark was added to clang tooling Diagnostic

[Ivan Murashko via Phabricator via cfe-commits]

ivanmurashko created this revision.
ivanmurashko added reviewers: aaron.ballman, alexfh, DmitryPolukhin, gribozavr2.
ivanmurashko added projects: clang, clang-tools-extra.
Herald added a subscriber: xazax.hun.
ivanmurashko requested review of this revision.
Herald added a subscriber: cfe-commits.

The diff adds `Remark` to `Diagnostic::Level` for clang tooling. That makes 
`Remark` diagnostic level ready to use in `clang-tidy` checks: the 
**clang-diagnostic-module-import** becomes visible as a part of the change

Test plan:

  ninja check-clang
  ninja check-clang-tools


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102906

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/Inputs/remarks/A.h
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/remarks/module.modulemap
  clang-tools-extra/test/clang-tidy/infrastructure/remarks.cpp
  clang/include/clang/Tooling/Core/Diagnostic.h
  clang/include/clang/Tooling/DiagnosticsYaml.h
  clang/unittests/Tooling/DiagnosticsYamlTest.cpp

Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -47,10 +47,11 @@
  const std::string , int FileOffset,
  const std::string ,
  const StringMap ,
- const SmallVector ) {
+ const SmallVector ,
+ Diagnostic::Level DiagnosticLevel) {
   return Diagnostic(DiagnosticName,
 makeMessage(Message, FileOffset, FilePath, Fix, Ranges), {},
-Diagnostic::Warning, "path/to/build/directory");
+DiagnosticLevel, "path/to/build/directory");
 }
 
 static const char *YAMLContent =
@@ -102,6 +103,14 @@
 "Replacements:[]\n"
 "Level:   Warning\n"
 "BuildDirectory:  'path/to/build/directory'\n"
+"  - DiagnosticName:  'diagnostic#4'\n"
+"DiagnosticMessage:\n"
+"  Message: 'message #4'\n"
+"  FilePath:'path/to/source3.cpp'\n"
+"  FileOffset:  72\n"
+"  Replacements:[]\n"
+"Level:   Remark\n"
+"BuildDirectory:  'path/to/build/directory'\n"
 "...\n";
 
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
@@ -112,7 +121,8 @@
   {"path/to/source.cpp",
Replacements({"path/to/source.cpp", 100, 12, "replacement #1"})}};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55,
-   "path/to/source.cpp", Fix1, {}));
+   "path/to/source.cpp", Fix1, {},
+   Diagnostic::Warning));
 
   StringMap Fix2 = {
   {"path/to/header.h",
@@ -120,15 +130,21 @@
   SmallVector Ranges2 =
   {makeByteRange(10, 10, "path/to/source.cpp")};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60,
-   "path/to/header.h", Fix2, Ranges2));
+   "path/to/header.h", Fix2, Ranges2,
+   Diagnostic::Warning));
 
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
-   "path/to/source2.cpp", {}, {}));
+   "path/to/source2.cpp", {}, {},
+   Diagnostic::Warning));
   TUD.Diagnostics.back().Notes.push_back(
   makeMessage("Note1", 88, "path/to/note1.cpp", {}, {}));
   TUD.Diagnostics.back().Notes.push_back(
   makeMessage("Note2", 99, "path/to/note2.cpp", {}, {}));
 
+  TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#4", "message #4", 72,
+   "path/to/source3.cpp", {}, {},
+   Diagnostic::Remark));
+
   std::string YamlContent;
   raw_string_ostream YamlContentStream(YamlContent);
 
@@ -144,7 +160,7 @@
   YAML >> TUDActual;
 
   ASSERT_FALSE(YAML.error());
-  ASSERT_EQ(3u, TUDActual.Diagnostics.size());
+  ASSERT_EQ(4u, TUDActual.Diagnostics.size());
   EXPECT_EQ("path/to/source.cpp", TUDActual.MainSourceFile);
 
   auto getFixes = [](const StringMap ) {
Index: clang/include/clang/Tooling/DiagnosticsYaml.h
===
--- clang/include/clang/Tooling/DiagnosticsYaml.h
+++ clang/include/clang/Tooling/DiagnosticsYaml.h
@@ -106,6 +106,7 @@
   static void enumeration(IO , clang::tooling::Diagnostic::Level ) {
 IO.enumCase(Value, "Warning", clang::tooling::Diagnostic::Warning);
 IO.enumCase(Value, "Error", clang::tooling::Diagnostic::Error);
+IO.enumCase(Value,