[Lldb-commits] [PATCH] D156270: [lldb][NFCI] Change logic to find clang resource dir in standalone builds

2023-08-17 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added a comment.

When I made D141907 , I want 
`get_clang_resource_dir` in cmake and GetResourcesPath 
 in 
clang to have the same behavior and it also respects the doc string of 
`CLANG_RESOURCE_DIR`, but the behavior of `GetResourcesPath` itself is 
unintuitive when custom resource path is empty.

In D156270#4557902 , @mgorny wrote:

> I'm not sure if it was really intended but the new API is kinda horrible, at 
> least for us in Gentoo.
>
> Our install prefix is `/usr/lib/llvm/NN`, whereas clang resource dir is 
> `/usr/lib/clang/NN`.
>
> If I don't override `CLANG_RESOURCE_DIR`, it infers the wrong directory:
> `/usr/lib/llvm/18/lib64/cmake/clang/../../..//lib64/clang/18`.

IIUC, the resource dir should be `/usr/lib/llvm/18/lib64/clang/18` when 
`CLANG_RESOURCE_DIR` is empty.

> To get the correct directory, I need to pass:
> `-DCLANG_RESOURCE_DIR="../../../clang/${LLVM_MAJOR}"`
> which is absolutely counterintuitive (the path gets appended to:
> `/usr/lib/llvm/18/lib64/cmake/clang/../../../bin`
> ).
>
> Not saying it's not workable but I'd hardly call that an improvement over the 
> previous API.
>
> CC @tstellar, @paperchalice.

The API here is indeed user unfriendly, a possible improvement is to let 
`CLANG_RESOURCE_DIR` aslo accept absolute path.
Also, `CLANG_RESOURCE_DIR` doesn't seem to work in standalone mode, some extra 
variables should be exported to `ClangConfig.cmake` e.g. the install prefix and 
bin dir of clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156270

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

[Lldb-commits] [PATCH] D152225: [lldb] fix dangling reference in `ClangHost.cpp`

2023-06-06 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added a comment.

If this patch fixes the CI failure 
,
 I need someone to commit this change, thanks in advance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152225

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-06-06 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added a comment.

In D141907#4398196 , @protze.joachim 
wrote:

> The review should not be closed, since the patch was reverted and should be 
> recommitted (this time possibly with a reference to this review?)

This is recommitted as 0beffb854209a41f31beb18f9631258349a99299 
, but 
break some lldb builds, so I created https://reviews.llvm.org/D152225 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D152225: [lldb] mark `clang_resource_path` as static

2023-06-05 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice created this revision.
paperchalice added a reviewer: aprantl.
Herald added a project: All.
paperchalice requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The lifetime of `clang_resource_path` should be same as `kResourceDirSuffixes`, 
because `kResourceDirSuffixes` doesn't own `clang_resource_path`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152225

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -52,7 +52,7 @@
   Log *log = GetLog(LLDBLog::Host);
   std::string raw_path = lldb_shlib_spec.GetPath();
   llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
-  const std::string clang_resource_path =
+  static const std::string clang_resource_path =
   clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
 
   static const llvm::StringRef kResourceDirSuffixes[] = {


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -52,7 +52,7 @@
   Log *log = GetLog(LLDBLog::Host);
   std::string raw_path = lldb_shlib_spec.GetPath();
   llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
-  const std::string clang_resource_path =
+  static const std::string clang_resource_path =
   clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
 
   static const llvm::StringRef kResourceDirSuffixes[] = {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-06-03 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added a comment.

In D141907#4361594 , @tstellar wrote:

> In D141907#4355229 , @paperchalice 
> wrote:
>
>> In D141907#4355228 , @tstellar 
>> wrote:
>>
>>> @paperchalice Do you need someone to commit this for you?
>>
>> Sure, I don't have the commit access.
>
> What name / email address do you want me to use for the commit author field?

Sorry for the late, the information in phabricator is OK, and 
`GetClangResourceDir.cmake` is not landed correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-05-18 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added a comment.

In D141907#4355228 , @tstellar wrote:

> @paperchalice Do you need someone to commit this for you?

Sure, I don't have the commit access.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-05-05 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added inline comments.



Comment at: cmake/Modules/GetClangResourceDir.cmake:13
+  if(DEFINED CLANG_RESOURCE_DIR AND NOT CLANG_RESOURCE_DIR STREQUAL "")
+set(ret_dir bin/${CLANG_RESOURCE_DIR})
+  else()

tstellar wrote:
> tstellar wrote:
> > tstellar wrote:
> > > paperchalice wrote:
> > > > tstellar wrote:
> > > > > Why is the bin prefix here?
> > > > See 
> > > > https://clang.llvm.org/doxygen/classclang_1_1driver_1_1Driver.html#acda8dfdf4f80efa84df98157e1779152
> > > > `GetResourcesPath` will return `/lib/` when 
> > > > `CLANG_RESOURCE_DIR` is empty, `/bin/CLANG_RESOURCE_DIR` 
> > > > otherwise.
> > > > 
> > > `GetResourcesPath` calls `sys::path::parent_path` twice on the path to 
> > > the clang executable which is going to give you `/usr ` on most Linux 
> > > systems, so it's returning `/CLANG_RESOURCE_DIR` not 
> > > `/bin/CLANG_RESOURCE_DIR`.
> > Sorry, you are correct.  I was looking at the wrong branch.  It's really 
> > strange that it does that.
> @paperchalice Can you update the description of the CLANG_RESOURCE_DIR cache 
> variable in clang/CMakeLists.txt to mention that the path should be relative 
> to the directory with the clang executable.
Already in [[ 
https://github.com/llvm/llvm-project/blob/c5fefbc8dbc49d1c3133615b46fc9a3ca93dcdc2/clang/CMakeLists.txt#LL178C53-L178C53
 | codebase ]]:
```
set(CLANG_RESOURCE_DIR "" CACHE STRING
  "Relative directory from the Clang binary to its resource files.")
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-05-04 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added inline comments.



Comment at: cmake/Modules/GetClangResourceDir.cmake:13
+  if(DEFINED CLANG_RESOURCE_DIR AND NOT CLANG_RESOURCE_DIR STREQUAL "")
+set(ret_dir bin/${CLANG_RESOURCE_DIR})
+  else()

tstellar wrote:
> Why is the bin prefix here?
See 
https://clang.llvm.org/doxygen/classclang_1_1driver_1_1Driver.html#acda8dfdf4f80efa84df98157e1779152
`GetResourcesPath` will return `/lib/` when 
`CLANG_RESOURCE_DIR` is empty, `/bin/CLANG_RESOURCE_DIR` otherwise.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-03-13 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added a comment.

Ping.




Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp:56
+  const std::string clang_resource_path =
+  clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
 

Should be "${CMAKE_INSTALL_BINDIR}/lldb" here, but need another variable in 
`config.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-02-02 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice updated this revision to Diff 494223.
paperchalice added a comment.

- Use `clang::driver::Driver::GetResourcesPath`
- Use `ARG` as prefix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

Files:
  clang/lib/Headers/CMakeLists.txt
  clang/lib/Tooling/CMakeLists.txt
  clang/runtime/CMakeLists.txt
  cmake/Modules/GetClangResourceDir.cmake
  compiler-rt/cmake/base-config-ix.cmake
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
  lldb/unittests/Expression/ClangParserTest.cpp
  llvm/cmake/modules/LLVMExternalProjectUtils.cmake
  openmp/CMakeLists.txt

Index: openmp/CMakeLists.txt
===
--- openmp/CMakeLists.txt
+++ openmp/CMakeLists.txt
@@ -86,8 +86,8 @@
 if(${OPENMP_STANDALONE_BUILD})
   set(LIBOMP_HEADERS_INSTALL_PATH "${CMAKE_INSTALL_INCLUDEDIR}")
 else()
-  string(REGEX MATCH "[0-9]+" CLANG_VERSION ${PACKAGE_VERSION})
-  set(LIBOMP_HEADERS_INSTALL_PATH "${OPENMP_INSTALL_LIBDIR}/clang/${CLANG_VERSION}/include")
+  include(GetClangResourceDir)
+  get_clang_resource_dir(LIBOMP_HEADERS_INSTALL_PATH SUBDIR include)
 endif()
 
 # Build host runtime library, after LIBOMPTARGET variables are set since they are needed
Index: llvm/cmake/modules/LLVMExternalProjectUtils.cmake
===
--- llvm/cmake/modules/LLVMExternalProjectUtils.cmake
+++ llvm/cmake/modules/LLVMExternalProjectUtils.cmake
@@ -257,7 +257,11 @@
 if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
   string(REGEX MATCH "^[0-9]+" CLANG_VERSION_MAJOR
  ${PACKAGE_VERSION})
-  set(resource_dir "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION_MAJOR}")
+  if(DEFINED CLANG_RESOURCE_DIR AND NOT CLANG_RESOURCE_DIR STREQUAL "")
+set(resource_dir ${LLVM_TOOLS_BINARY_DIR}/${CLANG_RESOURCE_DIR})
+  else()
+set(resource_dir "${LLVM_LIBRARY_DIR}/clang/${CLANG_VERSION_MAJOR}")
+  endif()
   set(flag_types ASM C CXX MODULE_LINKER SHARED_LINKER EXE_LINKER)
   foreach(type ${flag_types})
 set(${type}_flag -DCMAKE_${type}_FLAGS=-resource-dir=${resource_dir})
Index: lldb/unittests/Expression/ClangParserTest.cpp
===
--- lldb/unittests/Expression/ClangParserTest.cpp
+++ lldb/unittests/Expression/ClangParserTest.cpp
@@ -7,6 +7,8 @@
 //===--===//
 
 #include "clang/Basic/Version.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
 
 #include "Plugins/ExpressionParser/Clang/ClangHost.h"
 #include "TestingSupport/SubsystemRAII.h"
@@ -37,13 +39,11 @@
 TEST_F(ClangHostTest, ComputeClangResourceDirectory) {
 #if !defined(_WIN32)
   std::string path_to_liblldb = "/foo/bar/lib/";
-  std::string path_to_clang_dir =
-  "/foo/bar/" LLDB_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING;
 #else
-  std::string path_to_liblldb = "C:\\foo\\bar\\lib";
-  std::string path_to_clang_dir =
-  "C:\\foo\\bar\\lib\\clang\\" CLANG_VERSION_MAJOR_STRING;
+  std::string path_to_liblldb = "C:\\foo\\bar\\lib\\";
 #endif
+  std::string path_to_clang_dir = clang::driver::Driver::GetResourcesPath(
+  path_to_liblldb + "liblldb", CLANG_RESOURCE_DIR);
   EXPECT_EQ(ComputeClangResourceDir(path_to_liblldb), path_to_clang_dir);
 
   // The path doesn't really exist, so setting verify to true should make
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -10,6 +10,7 @@
 
 #include "clang/Basic/Version.h"
 #include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
@@ -51,11 +52,14 @@
   Log *log = GetLog(LLDBLog::Host);
   std::string raw_path = lldb_shlib_spec.GetPath();
   llvm::StringRef parent_dir = llvm::sys::path::parent_path(raw_path);
+  const std::string clang_resource_path =
+  clang::driver::Driver::GetResourcesPath("bin/lldb", CLANG_RESOURCE_DIR);
 
   static const llvm::StringRef kResourceDirSuffixes[] = {
   // LLVM.org's build of LLDB uses the clang resource directory placed
-  // in $install_dir/lib{,64}/clang/$clang_version.
-  CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING,
+  // in $install_dir/lib{,64}/clang/$clang_version or
+  // $install_dir/bin/$CLANG_RESOURCE_DIR
+  clang_resource_path,
   // swift-lldb uses the clang resource directory copied from swift, which
   // by default is placed in $install_dir/lib{,64}/lldb/clang. LLDB places
   // it there, so we use LLDB_INSTALL_LIBDIR_BASENAME.
@@ -82,7 +86,8 @@
 }
 
 bool lldb_private::ComputeClangResourceDirectory(FileSpec _shlib_spec,
-

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

2023-02-01 Thread Junchang Liu via Phabricator via lldb-commits 
paperchalice added a comment.

In D141907#4094748 , @MaskRay wrote:

> The CMake part of this patch improves the matter. Manually constructed 
> resource dir (many duplicates) string is replace with a library function call.
>
> However, the introduced conditions in C++ code like
>
>   std::string path_to_clang_dir = std::string(CLANG_RESOURCE_DIR).empty()
>   ? "/foo/bar/" 
> LLDB_INSTALL_LIBDIR_BASENAME
> "/clang/" CLANG_VERSION_MAJOR_STRING
>   : "/foo/bar/bin/" CLANG_RESOURCE_DIR;
>
> makes me uncomfortable.
>
> I wish that we can just replace all manually constructed 
> `CLANG_INSTALL_LIBDIR_BASENAME "/clang/" CLANG_VERSION_MAJOR_STRING` with a 
> simple `CLANG_RESOURCE_DIR` which is guaranteed to be non-empty.
>
> edeaf16f2c2f02d6e43312d48d26d354d87913f3 (2011) added the CMake variable 
> `CLANG_RESOURCE_DIR` but did not explain why. 
> In the long term, the CMake variable `CLANG_RESOURCE_DIR` probably should be 
> removed.

clang::driver::Driver::GetResourcesPath 

 would be a better choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141907

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