[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added a comment.

This breaks builds when LLVM is included with CMake’s `add_subdirectory`.
I think the zstd target needs to be marked as imported.
The following patch fixes the problem, although I’m not familiar enough with 
CMake to know if this is the right way to go:

  diff --git a/llvm/cmake/modules/FindZSTD.cmake 
b/llvm/cmake/modules/FindZSTD.cmake
  index 43ccf7232138..8e6ec6e8a988 100644
  --- a/llvm/cmake/modules/FindZSTD.cmake
  +++ b/llvm/cmake/modules/FindZSTD.cmake
  @@ -12,6 +12,10 @@ find_package_handle_standard_args(
   ZSTD_LIBRARY ZSTD_INCLUDE_DIR)
  
   if(ZSTD_FOUND)
  +if(NOT TARGET Zstd::zstd)
  +  add_library(Zstd::zstd UNKNOWN IMPORTED)
  +  set_target_properties(Zstd::zstd PROPERTIES IMPORTED_LOCATION 
"${ZSTD_LIBRARY}")
  +endif()
   set(ZSTD_LIBRARIES ${ZSTD_LIBRARY})
   set(ZSTD_INCLUDE_DIRS ${ZSTD_INCLUDE_DIR})
   endif()
  diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt
  index 52b95c5377d3..031adfa33ba8 100644
  --- a/llvm/lib/Support/CMakeLists.txt
  +++ b/llvm/lib/Support/CMakeLists.txt
  @@ -26,7 +26,7 @@ if(LLVM_ENABLE_ZLIB)
   endif()
  
   if(LLVM_ENABLE_ZSTD)
  -  list(APPEND imported_libs zstd)
  +  list(APPEND imported_libs Zstd::zstd)
   endif()
  
   if( MSVC OR MINGW )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128465

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

[Lldb-commits] [PATCH] D132316: [CMake] `${LLVM_BINARY_DIR}/lib(${LLVM_LIBDIR_SUFFIX})?` -> `${LLVM_LIBRARY_DIR}`

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added inline comments.



Comment at: llvm/tools/llvm-shlib/CMakeLists.txt:89
 
-  set(LLVM_EXPORTED_SYMBOL_FILE ${LLVM_BINARY_DIR}/libllvm-c.exports)
 

The lib here is not used as a folder, so this should probably not be replaced?
It should probably include CMAKE_CFG_INTDIR though (i.e. 
`${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/libllvm-c.exports`), to be consistent 
with other places, though this only affects the ninja multi-config build. I’m 
not sure if that works at all and it’s outside of the scope of this patch 
anyway.



Comment at: llvm/tools/llvm-shlib/CMakeLists.txt:91
 
   set(LIB_DIR ${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
   set(LIB_NAME ${LIB_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}LLVM)

Should this be `set(LIB_DIR ${LLVM_LIBRARY_DIR})`?



Comment at: llvm/tools/llvm-shlib/CMakeLists.txt:139
   foreach(lib ${LIB_NAMES})
 list(APPEND FULL_LIB_NAMES 
${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib/${lib}.lib)
   endforeach()

This should probably use LLVM_LIBRARY_DIR as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132316

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

[Lldb-commits] [PATCH] D132316: [CMake] `${LLVM_BINARY_DIR}/lib(${LLVM_LIBDIR_SUFFIX})?` -> `${LLVM_LIBRARY_DIR}`

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added inline comments.



Comment at: llvm/tools/llvm-shlib/CMakeLists.txt:91
 
   set(LIB_DIR ${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
   set(LIB_NAME ${LIB_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}LLVM)

Ericson2314 wrote:
> sebastian-ne wrote:
> > Should this be `set(LIB_DIR ${LLVM_LIBRARY_DIR})`?
> I am not sure what is up with `CMAKE_CFG_INTDIR`, so I rather leave that for 
> a later revision.
`LLVM_LIBRARY_DIR` includes `CMAKE_CFG_INTDIR` as it’s set through
```
set(LLVM_LIBRARY_OUTPUT_INTDIR 
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
set(LLVM_LIBRARY_DIR  ${LLVM_LIBRARY_OUTPUT_INTDIR})
```

As far as I understand, `CMAKE_CFG_INTDIR` is set to `.`, unless one does a 
multi-config build, i.e. one build directory for debug and release builds. 
multi-config builds are the default when creating Visual Studio solutions and 
ninja supports multi-config builds since some time (rather recently).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132316

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

[Lldb-commits] [PATCH] D132316: [CMake] Avoid `LLVM_BINARY_DIR` when other more specific variable are better-suited

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added a comment.

The build afterwards succeeded again. Seems like every ~20th build on that 
windows machine fails.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132316

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

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added inline comments.



Comment at: clang/CMakeLists.txt:100
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
-  set(LLVM_LIBRARY_OUTPUT_INTDIR 
${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib)
   if(WIN32 OR CYGWIN)

Just to check if your intention aligns with my understanding, removing the 
suffix here is fine because the destination is in the binary dir and not the 
final install destination?



Comment at: clang/runtime/CMakeLists.txt:90-92
+  -DCMAKE_INSTALL_BINDIR="${CMAKE_INSTALL_BINDIR}"
+  -DCMAKE_INSTALL_LIBDIR="${CMAKE_INSTALL_LIBDIR}"
+  
-DCMAKE_INSTALL_INCLUDEDIR="${CMAKE_INSTALL_INCLUDEDIR}"

nit: The indentation is wrong



Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:65
   set(output_dir "${LLVM_LIBRARY_OUTPUT_INTDIR}")
-  set(install_dir "${CMAKE_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
+  set(install_dir "${CMAKE_INSTALL_PREFIX}/lib")
 else()

This is an install directory, so should this be something like
```
extend_path(install_dir "${CMAKE_INSTALL_PREFIX}" "${CMAKE_INSTALL_LIBDIR}")
```
instead?



Comment at: compiler-rt/cmake/base-config-ix.cmake:48
   set(COMPILER_RT_EXEC_OUTPUT_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})
-  set(COMPILER_RT_INSTALL_PATH lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION})
+  set(COMPILER_RT_INSTALL_PATH lib/clang/${CLANG_VERSION})
   option(COMPILER_RT_INCLUDE_TESTS "Generate and build compiler-rt unit tests."

This is an install path, so should it use 
`${CMAKE_INSTALL_LIBDIR}/clang/${CLANG_VERSION}` instead?



Comment at: compiler-rt/cmake/base-config-ix.cmake:103
 ${COMPILER_RT_OUTPUT_DIR}/lib)
-  extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" lib)
+  extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" 
"${CMAKE_INSTALL_LIBDIR}")
   set(COMPILER_RT_INSTALL_LIBRARY_DIR "${default_install_path}" CACHE PATH

What is the result we expect here?
In case that CMAKE_INSTALL_LIBDIR is defined as lib64, this path will change.

In some cases it would have been `lib64/clang/14.0.0/lib`,
but with this patch it would be `lib/clang/14.0.0/lib64` if I understand 
correctly.



Comment at: lldb/source/API/CMakeLists.txt:116
 if(LLDB_ENABLE_PYTHON AND (BUILD_SHARED_LIBS OR LLVM_LINK_LLVM_DYLIB) AND UNIX 
AND NOT APPLE)
-  set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH 
"\$ORIGIN/../../../../lib${LLVM_LIBDIR_SUFFIX}")
+  set_property(TARGET liblldb APPEND PROPERTY INSTALL_RPATH 
"\$ORIGIN/../../../../lib")
 endif()

It looks to me like this path is used for installation, so should it have the 
(potential) suffix?
In AddLLVM.cmake, _install_rpath uses `${CMAKE_INSTALL_LIBDIR}`.



Comment at: mlir/cmake/modules/AddMLIRPython.cmake:412
 set_property(TARGET ${target} APPEND PROPERTY
-  INSTALL_RPATH 
"${_origin_prefix}/${ARG_RELATIVE_INSTALL_ROOT}/lib${LLVM_LIBDIR_SUFFIX}")
+  INSTALL_RPATH "${_origin_prefix}/${ARG_RELATIVE_INSTALL_ROOT}/lib")
   endif()

Same here as above, the rpath should probably use `${CMAKE_INSTALL_LIBDIR}`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added inline comments.



Comment at: clang/CMakeLists.txt:100
   set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin)
-  set(LLVM_LIBRARY_OUTPUT_INTDIR 
${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX})
+  set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib)
   if(WIN32 OR CYGWIN)

Ericson2314 wrote:
> sebastian-ne wrote:
> > Just to check if your intention aligns with my understanding, removing the 
> > suffix here is fine because the destination is in the binary dir and not 
> > the final install destination?
> Yes exactly.
> 
> I really should write up the "rules" that I've (a) discovered from reading 
> (b) invented somewhere. Any idea where?
I think the commit message is a good place.



Comment at: llvm/CMakeLists.txt:59-60
+if (NOT DEFINED CMAKE_INSTALL_LIBDIR AND DEFINED LLVM_LIBDIR_SUFFIX)
+  message(WARNING "\"LLVM_LIBDIR_SUFFIX\" is deprecated. "
+  "Please set \"CMAKE_INSTALL_LIBDIR\" directly instead.")
+  set(CMAKE_INSTALL_LIBDIR "lib${LLVM_LIBDIR_SUFFIX}")

There’s also `message(DEPRECATION …)` :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne accepted this revision.
sebastian-ne added a comment.

Looks good to me, thanks!




Comment at: 
utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h:70
 
 /* Multilib suffix for libdir. */
+#define CLANG_INSTALL_LIBDIR_BASENAME "lib"

This comment is outdated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added a comment.

In D130586#3731021 , @Ericson2314 
wrote:

> Anyone have any idea what this Debian test failure is about?

I haven’t seen a passing debian pre-merge check for months now, so I usually 
ignore it (the failures seems to be related to debuginfod currently).
The bazel failure seems to be a bug or flake in the build system as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[Lldb-commits] [PATCH] D132316: [CMake] Avoid `LLVM_BINARY_DIR` when other more specific variable are better-suited

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added a comment.

This is probably caused by using `CMAKE_CFG_INTDIR` (indirectly) in more 
places. Seems like it expands to `$(Configuration)` for Visual Studio.
Searching more about it, it’s only set at build time, but not at install time, 
which is a problem as well.
On top of all, `CMAKE_CFG_INTDIR` is deprecated and superseded by generator 
expression: https://cmake.org/cmake/help/latest/variable/CMAKE_CFG_INTDIR.html


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132316

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

[Lldb-commits] [PATCH] D132608: [CMake] Clean up CMake binary dir handling

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added a comment.

I’m not sure if it’s the case for all places (as `CMAKE_CFG_INTDIR` is not 
defined at install time), but I think the `CMAKE_BINARY_LIBDIR` introduced here 
serves the same purpose as the already existing `LLVM_LIBRARY_DIR`.
Same for `CMAKE_BINARY_INCLUDEDIR`, which is `LLVM_INCLUDE_DIR` and 
`CMAKE_BINARY_BINDIR` which is `LLVM_TOOLS_BINARY_DIR`.

E.g. llvm/CMakeLists.txt uses

  set( CMAKE_LIBRARY_OUTPUT_DIRECTORY ${LLVM_LIBRARY_DIR} )
  set( CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${LLVM_LIBRARY_DIR} )

while clang uses

  set( CMAKE_LIBRARY_OUTPUT_DIRECTORY 
${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX} )
  set( CMAKE_ARCHIVE_OUTPUT_DIRECTORY 
${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX} )

and this patch replaces the clang code with

  set( CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_LIBDIR} )
  set( CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_LIBDIR} )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132608

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

[Lldb-commits] [PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne created this revision.
sebastian-ne added reviewers: jloser, MaskRay, dblaikie, jsilvanus.
Herald added subscribers: ormris, StephenFan, wenlei, hiraditya.
Herald added a project: All.
sebastian-ne requested review of this revision.
Herald added projects: clang, LLDB, LLVM.
Herald added subscribers: llvm-commits, lldb-commits, cfe-commits.

This facilitates replacing llvm::Any with std::any.

- Deprecate any_isa in favor of using any_cast(Any*) and checking for nullptr 
because C++17 has no any_isa.
- Remove the assert from any_cast(Any*), so it returns nullptr if the type is 
not correct. This aligns it with std::any_cast(any*).

Use any_cast(Any*) throughout LLVM instead of checks with any_isa.

This is the first part outlined in
https://discourse.llvm.org/t/rfc-switching-from-llvm-any-to-std-any/67176


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139973

Files:
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp
  lldb/include/lldb/Core/RichManglingContext.h
  llvm/include/llvm/ADT/Any.h
  llvm/lib/CodeGen/MachinePassManager.cpp
  llvm/lib/IR/LLVMContextImpl.h
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
  llvm/lib/Transforms/Scalar/LoopPassManager.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/unittests/ADT/AnyTest.cpp
  llvm/unittests/IR/PassBuilderCallbacksTest.cpp

Index: llvm/unittests/IR/PassBuilderCallbacksTest.cpp
===
--- llvm/unittests/IR/PassBuilderCallbacksTest.cpp
+++ llvm/unittests/IR/PassBuilderCallbacksTest.cpp
@@ -290,17 +290,17 @@
   return std::string(name);
 }
 
-template <> std::string getName(const llvm::Any &WrappedIR) {
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName();
+template <> std::string getName(const Any &WrappedIR) {
+  if (const auto *const *M = any_cast(&WrappedIR))
+return (*M)->getName().str();
+  if (const auto *const *F = any_cast(&WrappedIR))
+return (*F)->getName().str();
+  if (const auto *const *L = any_cast(&WrappedIR))
+return (*L)->getName().str();
+  if (const auto *const *L = any_cast(&WrappedIR))
+return (*L)->getName().str();
+  if (const auto *const *C = any_cast(&WrappedIR))
+return (*C)->getName();
   return "";
 }
 /// Define a custom matcher for objects which support a 'getName' method.
Index: llvm/unittests/ADT/AnyTest.cpp
===
--- llvm/unittests/ADT/AnyTest.cpp
+++ llvm/unittests/ADT/AnyTest.cpp
@@ -24,55 +24,55 @@
 
   // An empty Any is not anything.
   EXPECT_FALSE(A.has_value());
-  EXPECT_FALSE(any_isa(A));
+  EXPECT_FALSE(any_cast(&A));
 
   // An int is an int but not something else.
   EXPECT_TRUE(B.has_value());
-  EXPECT_TRUE(any_isa(B));
-  EXPECT_FALSE(any_isa(B));
+  EXPECT_TRUE(any_cast(&B));
+  EXPECT_FALSE(any_cast(&B));
 
   EXPECT_TRUE(C.has_value());
-  EXPECT_TRUE(any_isa(C));
+  EXPECT_TRUE(any_cast(&C));
 
   // A const char * is a const char * but not an int.
   EXPECT_TRUE(D.has_value());
-  EXPECT_TRUE(any_isa(D));
-  EXPECT_FALSE(any_isa(D));
+  EXPECT_TRUE(any_cast(&D));
+  EXPECT_FALSE(any_cast(&D));
 
   // A double is a double but not a float.
   EXPECT_TRUE(E.has_value());
-  EXPECT_TRUE(any_isa(E));
-  EXPECT_FALSE(any_isa(E));
+  EXPECT_TRUE(any_cast(&E));
+  EXPECT_FALSE(any_cast(&E));
 
   // After copy constructing from an int, the new item and old item are both
   // ints.
   llvm::Any F(B);
   EXPECT_TRUE(B.has_value());
   EXPECT_TRUE(F.has_value());
-  EXPECT_TRUE(any_isa(F));
-  EXPECT_TRUE(any_isa(B));
+  EXPECT_TRUE(any_cast(&F));
+  EXPECT_TRUE(any_cast(&B));
 
   // After move constructing from an int, the new item is an int and the old one
   // isn't.
   llvm::Any G(std::move(C));
   EXPECT_FALSE(C.has_value());
   EXPECT_TRUE(G.has_value());
-  EXPECT_TRUE(any_isa(G));
-  EXPECT_FALSE(any_isa(C));
+  EXPECT_TRUE(any_cast(&G));
+  EXPECT_FALSE(any_cast(&C));
 
   // After copy-assigning from an int, the new item and old item are both ints.
   A = F;
   EXPECT_TRUE(A.has_value());
   EXPECT_TRUE(F.has_value());
-  EXPECT_TRUE(any_isa(A));
-  EXPECT_TRUE(any_isa(F));
+  EXPECT_TRUE(any_cast(&A));
+  EXPECT_TRUE(any_cast(&F));
 
   // After move-assigning from an int, the new item and old item are both ints.
   B = std::move(G);
   EXPECT_TRUE(B.has_value());
   EXPECT_FALSE(G.has_value());
-  EXPECT_TRUE(any_isa(B));
-  EXPECT_FALSE(any_isa(G));
+  EXPECT_TRUE(any_cast(&B));
+  EXPECT_FALSE(any_cast(&G));
 }
 
 TEST(AnyTest, GoodAnyCast) {
Index: llvm/lib/Transforms/Utils/Debugify.cpp
==

[Lldb-commits] [PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne updated this revision to Diff 483975.
sebastian-ne marked 2 inline comments as done.
sebastian-ne added a comment.

> It is surprising to me that std::any can work without RTTI. Never thought it 
> could be implemented.

It seems like libstdc++ and libc++ both implement it the way llvm::Any is 
implemented when RTTI is not available (actually, I think llvm::Any was copied 
from libstdc++ at some point).

> This "compare function pointer" trick is awesome, but it might not always 
> work as explained in this commit.
> This question however, suggest that any_cast doesn't always work with RTTI 
> either, which is weird.
> I don't know of any other potential issues. std::visitor can't be used with 
> std::any in absense of std::any::type, but that's minor, I think.

As you noticed, it is very difficult to implement Any correctly on every 
platform under any circumstances and compiler flags.
That is exactly what this patch aims at: Moving the responsibility to implement 
Any correctly to the standard library.

Unfortunatly, we can’t replace llvm::Any with std::any just yet, because of 
bugs in msvc that were only fixed recently.

> llvm::any_isa<> is kind of convenient and is aligned with llvm::isa<>. Same 
> for llvm::any_cast.

It is, however, there is no std::any_isa or std::any_cast_or_null, so the 
question gets wether we want to keep llvm::Any around as a wrapper of std::any 
once we can use it (in this case this patch would be obsolete)
or if we we want to remove llvm::Any and use std::any only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

Files:
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp
  lldb/include/lldb/Core/RichManglingContext.h
  llvm/include/llvm/ADT/Any.h
  llvm/lib/CodeGen/MachinePassManager.cpp
  llvm/lib/IR/LLVMContextImpl.h
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
  llvm/lib/Transforms/Scalar/LoopPassManager.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/unittests/ADT/AnyTest.cpp
  llvm/unittests/IR/PassBuilderCallbacksTest.cpp

Index: llvm/unittests/IR/PassBuilderCallbacksTest.cpp
===
--- llvm/unittests/IR/PassBuilderCallbacksTest.cpp
+++ llvm/unittests/IR/PassBuilderCallbacksTest.cpp
@@ -290,17 +290,17 @@
   return std::string(name);
 }
 
-template <> std::string getName(const llvm::Any &WrappedIR) {
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName();
+template <> std::string getName(const Any &WrappedIR) {
+  if (const auto *const *M = any_cast(&WrappedIR))
+return (*M)->getName().str();
+  if (const auto *const *F = any_cast(&WrappedIR))
+return (*F)->getName().str();
+  if (const auto *const *L = any_cast(&WrappedIR))
+return (*L)->getName().str();
+  if (const auto *const *L = any_cast(&WrappedIR))
+return (*L)->getName().str();
+  if (const auto *const *C = any_cast(&WrappedIR))
+return (*C)->getName();
   return "";
 }
 /// Define a custom matcher for objects which support a 'getName' method.
Index: llvm/unittests/ADT/AnyTest.cpp
===
--- llvm/unittests/ADT/AnyTest.cpp
+++ llvm/unittests/ADT/AnyTest.cpp
@@ -24,55 +24,55 @@
 
   // An empty Any is not anything.
   EXPECT_FALSE(A.has_value());
-  EXPECT_FALSE(any_isa(A));
+  EXPECT_FALSE(any_cast(&A));
 
   // An int is an int but not something else.
   EXPECT_TRUE(B.has_value());
-  EXPECT_TRUE(any_isa(B));
-  EXPECT_FALSE(any_isa(B));
+  EXPECT_TRUE(any_cast(&B));
+  EXPECT_FALSE(any_cast(&B));
 
   EXPECT_TRUE(C.has_value());
-  EXPECT_TRUE(any_isa(C));
+  EXPECT_TRUE(any_cast(&C));
 
   // A const char * is a const char * but not an int.
   EXPECT_TRUE(D.has_value());
-  EXPECT_TRUE(any_isa(D));
-  EXPECT_FALSE(any_isa(D));
+  EXPECT_TRUE(any_cast(&D));
+  EXPECT_FALSE(any_cast(&D));
 
   // A double is a double but not a float.
   EXPECT_TRUE(E.has_value());
-  EXPECT_TRUE(any_isa(E));
-  EXPECT_FALSE(any_isa(E));
+  EXPECT_TRUE(any_cast(&E));
+  EXPECT_FALSE(any_cast(&E));
 
   // After copy constructing from an int, the new item and old item are both
   // ints.
   llvm::Any F(B);
   EXPECT_TRUE(B.has_value());
   EXPECT_TRUE(F.has_value());
-  EXPECT_TRUE(any_isa(F));
-  EXPECT_TRUE(any_isa(B));
+  EXPECT_TRUE(any_cast(&F));
+  EXPECT_TRUE(any_cast(&B));
 
   // After move constructing from an int, the new item is an int and the old one
   // isn't.
   llvm::Any G(std::move(C));
   EXPECT_FALSE(C.has_value());
   EXPECT_TRUE(G.has_value());
-  EXPE

[Lldb-commits] [PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added inline comments.



Comment at: lldb/include/lldb/Core/RichManglingContext.h:90-91
 assert(parser.has_value());
-assert(llvm::any_isa(parser));
+assert(llvm::any_cast(&parser));
 return llvm::any_cast(parser);
   }

barannikov88 wrote:
> This is not intuitively clear. In assert you pass an address, but in 'return' 
> below the object is passed by reference.
> 
I changed it to use an address + explicit dereference in the return. I hope 
that makes it clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

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

[Lldb-commits] [PATCH] D139973: [llvm] Make llvm::Any similar to std::any

[Sebastian Neubauer via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbb7940e25f6c: [llvm] Make llvm::Any similar to std::any 
(authored by sebastian-ne).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139973

Files:
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp
  lldb/include/lldb/Core/RichManglingContext.h
  llvm/include/llvm/ADT/Any.h
  llvm/lib/CodeGen/MachinePassManager.cpp
  llvm/lib/IR/LLVMContextImpl.h
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/lib/Transforms/IPO/SampleProfileProbe.cpp
  llvm/lib/Transforms/Scalar/LoopPassManager.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/unittests/ADT/AnyTest.cpp
  llvm/unittests/IR/PassBuilderCallbacksTest.cpp

Index: llvm/unittests/IR/PassBuilderCallbacksTest.cpp
===
--- llvm/unittests/IR/PassBuilderCallbacksTest.cpp
+++ llvm/unittests/IR/PassBuilderCallbacksTest.cpp
@@ -290,17 +290,17 @@
   return std::string(name);
 }
 
-template <> std::string getName(const llvm::Any &WrappedIR) {
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName().str();
-  if (any_isa(WrappedIR))
-return any_cast(WrappedIR)->getName();
+template <> std::string getName(const Any &WrappedIR) {
+  if (const auto *const *M = any_cast(&WrappedIR))
+return (*M)->getName().str();
+  if (const auto *const *F = any_cast(&WrappedIR))
+return (*F)->getName().str();
+  if (const auto *const *L = any_cast(&WrappedIR))
+return (*L)->getName().str();
+  if (const auto *const *L = any_cast(&WrappedIR))
+return (*L)->getName().str();
+  if (const auto *const *C = any_cast(&WrappedIR))
+return (*C)->getName();
   return "";
 }
 /// Define a custom matcher for objects which support a 'getName' method.
Index: llvm/unittests/ADT/AnyTest.cpp
===
--- llvm/unittests/ADT/AnyTest.cpp
+++ llvm/unittests/ADT/AnyTest.cpp
@@ -24,55 +24,55 @@
 
   // An empty Any is not anything.
   EXPECT_FALSE(A.has_value());
-  EXPECT_FALSE(any_isa(A));
+  EXPECT_FALSE(any_cast(&A));
 
   // An int is an int but not something else.
   EXPECT_TRUE(B.has_value());
-  EXPECT_TRUE(any_isa(B));
-  EXPECT_FALSE(any_isa(B));
+  EXPECT_TRUE(any_cast(&B));
+  EXPECT_FALSE(any_cast(&B));
 
   EXPECT_TRUE(C.has_value());
-  EXPECT_TRUE(any_isa(C));
+  EXPECT_TRUE(any_cast(&C));
 
   // A const char * is a const char * but not an int.
   EXPECT_TRUE(D.has_value());
-  EXPECT_TRUE(any_isa(D));
-  EXPECT_FALSE(any_isa(D));
+  EXPECT_TRUE(any_cast(&D));
+  EXPECT_FALSE(any_cast(&D));
 
   // A double is a double but not a float.
   EXPECT_TRUE(E.has_value());
-  EXPECT_TRUE(any_isa(E));
-  EXPECT_FALSE(any_isa(E));
+  EXPECT_TRUE(any_cast(&E));
+  EXPECT_FALSE(any_cast(&E));
 
   // After copy constructing from an int, the new item and old item are both
   // ints.
   llvm::Any F(B);
   EXPECT_TRUE(B.has_value());
   EXPECT_TRUE(F.has_value());
-  EXPECT_TRUE(any_isa(F));
-  EXPECT_TRUE(any_isa(B));
+  EXPECT_TRUE(any_cast(&F));
+  EXPECT_TRUE(any_cast(&B));
 
   // After move constructing from an int, the new item is an int and the old one
   // isn't.
   llvm::Any G(std::move(C));
   EXPECT_FALSE(C.has_value());
   EXPECT_TRUE(G.has_value());
-  EXPECT_TRUE(any_isa(G));
-  EXPECT_FALSE(any_isa(C));
+  EXPECT_TRUE(any_cast(&G));
+  EXPECT_FALSE(any_cast(&C));
 
   // After copy-assigning from an int, the new item and old item are both ints.
   A = F;
   EXPECT_TRUE(A.has_value());
   EXPECT_TRUE(F.has_value());
-  EXPECT_TRUE(any_isa(A));
-  EXPECT_TRUE(any_isa(F));
+  EXPECT_TRUE(any_cast(&A));
+  EXPECT_TRUE(any_cast(&F));
 
   // After move-assigning from an int, the new item and old item are both ints.
   B = std::move(G);
   EXPECT_TRUE(B.has_value());
   EXPECT_FALSE(G.has_value());
-  EXPECT_TRUE(any_isa(B));
-  EXPECT_FALSE(any_isa(G));
+  EXPECT_TRUE(any_cast(&B));
+  EXPECT_FALSE(any_cast(&G));
 }
 
 TEST(AnyTest, GoodAnyCast) {
Index: llvm/lib/Transforms/Utils/Debugify.cpp
===
--- llvm/lib/Transforms/Utils/Debugify.cpp
+++ llvm/lib/Transforms/Utils/Debugify.cpp
@@ -1031,19 +1031,19 @@
   PIC.registerBeforeNonSkippedPassCallback([this](StringRef P, Any IR) {
 if (isIgnoredPass(P))
   return;
-if (any_isa(IR))
-  applyDebugify(*const_cast(any_cast(IR)),
+if (const auto **F = any_cast(&IR))
+  applyDebugify(*const_cast(*F),
 Mode, DebugInfoBeforePass, P);
-else if (any_isa(IR))
-  applyDebugify(*const_cast(

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

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne added inline comments.



Comment at: cmake/Modules/GetClangResourceDir.cmake:15
+  else()
+string(REGEX MATCH "^[0-9]+" CLANG_VERSION_MAJOR ${PACKAGE_VERSION})
+set(ret_dir lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION_MAJOR})

This fails in our downstream project.
We use add_subdirectory() to include LLVM and our parent project already sets 
PACKAGE_VERSION to something that is not the LLVM version.

Parsing PACKAGE_VERSION only if CLANG_VERSION_MAJOR is not already known seems 
to work:
```
if (NOT CLANG_VERSION_MAJOR)
  string(REGEX MATCH "^[0-9]+" CLANG_VERSION_MAJOR ${PACKAGE_VERSION})
endif()
```


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] D132316: [CMake] Avoid `LLVM_BINARY_DIR` when other more specific variable are better-suited, part 2

[Sebastian Neubauer via Phabricator via lldb-commits]

sebastian-ne accepted this revision.
sebastian-ne added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132316

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