[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread walter erquinigo via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe538c6fc3048: [LLDB] Ensure LLDB symbols are exported in 
LLDB_EXPORT_ALL_SYMBOLS is provided. (authored by wallace).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere accepted this revision.
JDevlieghere added a comment.

Thanks! LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread walter erquinigo via Phabricator via lldb-commits 
wallace updated this revision to Diff 510588.
wallace added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,13 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+set_target_properties(${name} PROPERTIES CXX_VISIBILITY_PRESET default)
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread walter erquinigo via Phabricator via lldb-commits 
wallace added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:175
+CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()

JDevlieghere wrote:
> Rather than changing the compile options directly, can we change the 
> `CXX_VISIBILITY_PRESET` property?
> 
> ```
> set_target_properties((${name} PROPERTIES CXX_VISIBILITY_PRESET default)
> ```
Thanks @JDevlieghere , that works and is cleaner


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:175
+CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()

Rather than changing the compile options directly, can we change the 
`CXX_VISIBILITY_PRESET` property?

```
set_target_properties((${name} PROPERTIES CXX_VISIBILITY_PRESET default)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread River Riddle via Phabricator via lldb-commits 
rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.

LGTM, and makes sense to me given that downstream users often want a build of 
LLVM with `CMAKE_CXX_VISIBILITY_PRESET=hidden`, but lldb should still be able 
to build/link/work in the presence of that (the LLDB driver and tools currently 
fail to link if LLVM is built `CMAKE_CXX_VISIBILITY_PRESET=hidden`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread walter erquinigo via Phabricator via lldb-commits 
wallace updated this revision to Diff 510557.
wallace added a comment.

gate the target OS


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,16 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows" AND
+CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,16 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows" AND
+CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread walter erquinigo via Phabricator via lldb-commits 
wallace added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:173
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")

rriddle wrote:
> Other places uses `if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")` to check for 
> non-windows, should that check be used here as well? Not sure what's 
> consistent for the lldb codebase.
I initially thought that it might be fine just to check the compiler id, but 
better be safer gating the target OS as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread River Riddle via Phabricator via lldb-commits 
rriddle added inline comments.



Comment at: lldb/cmake/modules/AddLLDB.cmake:173
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")

Other places uses `if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")` to check for 
non-windows, should that check be used here as well? Not sure what's consistent 
for the lldb codebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

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

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread walter erquinigo via Phabricator via lldb-commits 
wallace updated this revision to Diff 510551.
wallace added a comment.

another nit...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,15 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,15 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB libraries even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread walter erquinigo via Phabricator via lldb-commits 
wallace updated this revision to Diff 510550.
wallace added a comment.

nits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,15 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB symbols even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,15 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility for all LLDB symbols even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden`is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147453: [LLDB] Ensure LLDB symbols are exported in LLDB_EXPORT_ALL_SYMBOLS is provided.

2023-04-03 Thread walter erquinigo via Phabricator via lldb-commits 
wallace created this revision.
wallace added a reviewer: rriddle.
Herald added a project: All.
wallace requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

If we want to export all lldb symbols (i.e
LLDB_EXPORT_ALL_SYMBOLS=ON), we need to use default visibility for all 
LLDB symbols even if a global `CMAKE_CXX_VISIBILITY_PRESET=hidden` is present.A


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147453

Files:
  lldb/cmake/modules/AddLLDB.cmake


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,16 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility# for all LLDB symbols even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden` is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)


Index: lldb/cmake/modules/AddLLDB.cmake
===
--- lldb/cmake/modules/AddLLDB.cmake
+++ lldb/cmake/modules/AddLLDB.cmake
@@ -165,6 +165,16 @@
   else()
 set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
   endif()
+
+
+  # If we want to export all lldb symbols (i.e LLDB_EXPORT_ALL_SYMBOLS=ON), we
+  # need to use default visibility# for all LLDB symbols even if a global
+  # `CMAKE_CXX_VISIBILITY_PRESET=hidden` is present.
+  if (LLDB_EXPORT_ALL_SYMBOLS)
+if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU")
+  target_compile_options(${name} PRIVATE "-fvisibility=default")
+endif()
+  endif()
 endfunction(add_lldb_library)
 
 function(add_lldb_executable name)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits