[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-14 Thread Pavel Labath via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL327490: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds 
(pr36687) (authored by labath, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D44379?vs=138034&id=138316#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D44379

Files:
  lldb/trunk/cmake/modules/LLDBConfig.cmake


Index: lldb/trunk/cmake/modules/LLDBConfig.cmake
===
--- lldb/trunk/cmake/modules/LLDBConfig.cmake
+++ lldb/trunk/cmake/modules/LLDBConfig.cmake
@@ -346,14 +346,18 @@
 
 endif()
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
+if( WIN32 AND NOT CYGWIN )
+  set(PURE_WINDOWS 1)
+endif()
 
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(NOT PURE_WINDOWS)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+  find_package(Threads REQUIRED)
+  list(APPEND system_libs ${CMAKE_THREAD_LIBS_INIT})
 endif()
 
+list(APPEND system_libs ${CMAKE_DL_LIBS})
+
 # Figure out if lldb could use lldb-server.  If so, then we'll
 # ensure we build lldb-server when an lldb target is being built.
 if (CMAKE_SYSTEM_NAME MATCHES "Android|Darwin|FreeBSD|Linux|NetBSD")


Index: lldb/trunk/cmake/modules/LLDBConfig.cmake
===
--- lldb/trunk/cmake/modules/LLDBConfig.cmake
+++ lldb/trunk/cmake/modules/LLDBConfig.cmake
@@ -346,14 +346,18 @@
 
 endif()
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
+if( WIN32 AND NOT CYGWIN )
+  set(PURE_WINDOWS 1)
+endif()
 
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(NOT PURE_WINDOWS)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+  find_package(Threads REQUIRED)
+  list(APPEND system_libs ${CMAKE_THREAD_LIBS_INIT})
 endif()
 
+list(APPEND system_libs ${CMAKE_DL_LIBS})
+
 # Figure out if lldb could use lldb-server.  If so, then we'll
 # ensure we build lldb-server when an lldb target is being built.
 if (CMAKE_SYSTEM_NAME MATCHES "Android|Darwin|FreeBSD|Linux|NetBSD")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-14 Thread Pavel Labath via Phabricator via lldb-commits
labath added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:349
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
-
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(UNIX)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)

krytarowski wrote:
> labath wrote:
> > krytarowski wrote:
> > > Why UNIX here?
> > > 
> > > Why CMAKE_THREAD_PREFER_PTHREAD? It looks like used only on IRIX and that 
> > > one is not going anywhere nowadays. (And certainly similarly to other 
> > > commercial OSes, due to legal work/removing not-owned code, it's not 
> > > possible to push it to Open-Source).
> > > 
> > > Assuming that system_libs can accept "-pthreads", this patch looks good 
> > > to me.
> > For the second part, I copied it out of the llvm's build scripts without 
> > looking at what it does (with the idea of trying to maintain a consistent 
> > build). However, if it's only used at irix, then I guess I can remove that.
> > 
> > The UNIX part is also inspired by llvm, but I simplified it a bit (they use 
> > CYGWIN OR NOT WINDOWS). I was assuming the idea was to make sure we use 
> > native thread support and not pthreads (which are present there sometimes, 
> > I think).
> I see, I would use `CYGWIN OR NOT WINDOWS` without changing the logic.
> 
> Keeping here `CMAKE_THREAD_PREFER_PTHREAD` does not make harm. non-pthreading 
> on UNIX systems is rather in extinct and remnant of 90ties (Minix has 
> something like that.. and lack of pthreads).
> 
> The UNIX part looks correct.
Thanks, I'll update the logic to match and submit.


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Kamil Rytarowski via Phabricator via lldb-commits
krytarowski accepted this revision.
krytarowski added a comment.
This revision is now accepted and ready to land.

I cannot test it shortly on NetBSD, but if there are any issues left (probably 
none) - I will fix it in future.




Comment at: cmake/modules/LLDBConfig.cmake:349
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
-
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(UNIX)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)

labath wrote:
> krytarowski wrote:
> > Why UNIX here?
> > 
> > Why CMAKE_THREAD_PREFER_PTHREAD? It looks like used only on IRIX and that 
> > one is not going anywhere nowadays. (And certainly similarly to other 
> > commercial OSes, due to legal work/removing not-owned code, it's not 
> > possible to push it to Open-Source).
> > 
> > Assuming that system_libs can accept "-pthreads", this patch looks good to 
> > me.
> For the second part, I copied it out of the llvm's build scripts without 
> looking at what it does (with the idea of trying to maintain a consistent 
> build). However, if it's only used at irix, then I guess I can remove that.
> 
> The UNIX part is also inspired by llvm, but I simplified it a bit (they use 
> CYGWIN OR NOT WINDOWS). I was assuming the idea was to make sure we use 
> native thread support and not pthreads (which are present there sometimes, I 
> think).
I see, I would use `CYGWIN OR NOT WINDOWS` without changing the logic.

Keeping here `CMAKE_THREAD_PREFER_PTHREAD` does not make harm. non-pthreading 
on UNIX systems is rather in extinct and remnant of 90ties (Minix has something 
like that.. and lack of pthreads).

The UNIX part looks correct.


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Pavel Labath via Phabricator via lldb-commits
labath added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:349
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
-
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(UNIX)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)

krytarowski wrote:
> Why UNIX here?
> 
> Why CMAKE_THREAD_PREFER_PTHREAD? It looks like used only on IRIX and that one 
> is not going anywhere nowadays. (And certainly similarly to other commercial 
> OSes, due to legal work/removing not-owned code, it's not possible to push it 
> to Open-Source).
> 
> Assuming that system_libs can accept "-pthreads", this patch looks good to me.
For the second part, I copied it out of the llvm's build scripts without 
looking at what it does (with the idea of trying to maintain a consistent 
build). However, if it's only used at irix, then I guess I can remove that.

The UNIX part is also inspired by llvm, but I simplified it a bit (they use 
CYGWIN OR NOT WINDOWS). I was assuming the idea was to make sure we use native 
thread support and not pthreads (which are present there sometimes, I think).


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Kamil Rytarowski via Phabricator via lldb-commits
krytarowski added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:349
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
-
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(UNIX)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)

Why UNIX here?

Why CMAKE_THREAD_PREFER_PTHREAD? It looks like used only on IRIX and that one 
is not going anywhere nowadays. (And certainly similarly to other commercial 
OSes, due to legal work/removing not-owned code, it's not possible to push it 
to Open-Source).

Assuming that system_libs can accept "-pthreads", this patch looks good to me.


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Pavel Labath via Phabricator via lldb-commits
labath updated this revision to Diff 138034.
labath added a comment.

This drops the LLDBStandalone changes (they are replaced by 
https://reviews.llvm.org/D44391 on the llvm
side), and simplifies the CMAKE_DL_LIBS logic.

I've tested this out both with BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds
(for the latter, be sure to specify LLVM_LINK_LLVM_DYLIB also when configuring
lldb), but I'd appreciate more testing (I'm no hurry to land this).


https://reviews.llvm.org/D44379

Files:
  cmake/modules/LLDBConfig.cmake


Index: cmake/modules/LLDBConfig.cmake
===
--- cmake/modules/LLDBConfig.cmake
+++ cmake/modules/LLDBConfig.cmake
@@ -346,14 +346,14 @@
 
 endif()
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
-
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(UNIX)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+  find_package(Threads REQUIRED)
+  list(APPEND system_libs ${CMAKE_THREAD_LIBS_INIT})
 endif()
 
+list(APPEND system_libs ${CMAKE_DL_LIBS})
+
 # Figure out if lldb could use lldb-server.  If so, then we'll
 # ensure we build lldb-server when an lldb target is being built.
 if (CMAKE_SYSTEM_NAME MATCHES "Android|Darwin|FreeBSD|Linux|NetBSD")


Index: cmake/modules/LLDBConfig.cmake
===
--- cmake/modules/LLDBConfig.cmake
+++ cmake/modules/LLDBConfig.cmake
@@ -346,14 +346,14 @@
 
 endif()
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
-
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(UNIX)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+  find_package(Threads REQUIRED)
+  list(APPEND system_libs ${CMAKE_THREAD_LIBS_INIT})
 endif()
 
+list(APPEND system_libs ${CMAKE_DL_LIBS})
+
 # Figure out if lldb could use lldb-server.  If so, then we'll
 # ensure we build lldb-server when an lldb target is being built.
 if (CMAKE_SYSTEM_NAME MATCHES "Android|Darwin|FreeBSD|Linux|NetBSD")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Pavel Labath via Phabricator via lldb-commits
labath added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:354
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)

krytarowski wrote:
> krytarowski wrote:
> > labath wrote:
> > > krytarowski wrote:
> > > > mgorny wrote:
> > > > > krytarowski wrote:
> > > > > > A more portable form of this:
> > > > > > 
> > > > > > ```
> > > > > > foreach(lib ${CMAKE_DL_LIBS})
> > > > > > list(APPEND system_libs -l${lib})
> > > > > > endforeach()
> > > > > > ```
> > > > > > 
> > > > > > Haiku needs 2 libraries, other Unices can use `-lldl` or similar.
> > > > > Didn't this raise the problem of cmake using full paths on some 
> > > > > systems? 
> > > > No.
> > > Do you have some reference for the portability claim? (mainly because I'm 
> > > trying to learn more about how cmake works, but also because I'd rather 
> > > let cmake figure out when to prepend -l instead of doing it myself).
> > > 
> > > I don't see how the "two libraries" thingy comes into play here. It could 
> > > be a problem if we are mismatching lists and strings but I don't see why 
> > > that would necessitate manually tagging with -l.
> > dlopen(3)-like functions in `-ldl` is Linux; HPUX uses `-ldl`, Darwin/BSDs 
> > no extra libraries, Haiku `-lroot -lbe` etc.
> > 
> > https://github.com/Kitware/CMake/search?p=2&q=CMAKE_DL_LIBS&type=&utf8=%E2%9C%93
> > 
> > I know that we don't support multiple OSes as of today, but we can make it 
> > compatible with every recognizable one with this change.
> > 
> > It looks like the proper syntax is to drop `-l` from my first proposal:
> > 
> > ```
> > foreach(lib ${CMAKE_DL_LIBS})
> > list(APPEND system_libs ${lib})
> > endforeach()
> > ```
> Or perhaps even unconditional: `list(APPEND system_libs ${CMAKE_DL_LIBS})`
I like that: :D


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Kamil Rytarowski via Phabricator via lldb-commits
krytarowski added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:354
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)

krytarowski wrote:
> labath wrote:
> > krytarowski wrote:
> > > mgorny wrote:
> > > > krytarowski wrote:
> > > > > A more portable form of this:
> > > > > 
> > > > > ```
> > > > > foreach(lib ${CMAKE_DL_LIBS})
> > > > > list(APPEND system_libs -l${lib})
> > > > > endforeach()
> > > > > ```
> > > > > 
> > > > > Haiku needs 2 libraries, other Unices can use `-lldl` or similar.
> > > > Didn't this raise the problem of cmake using full paths on some 
> > > > systems? 
> > > No.
> > Do you have some reference for the portability claim? (mainly because I'm 
> > trying to learn more about how cmake works, but also because I'd rather let 
> > cmake figure out when to prepend -l instead of doing it myself).
> > 
> > I don't see how the "two libraries" thingy comes into play here. It could 
> > be a problem if we are mismatching lists and strings but I don't see why 
> > that would necessitate manually tagging with -l.
> dlopen(3)-like functions in `-ldl` is Linux; HPUX uses `-ldl`, Darwin/BSDs no 
> extra libraries, Haiku `-lroot -lbe` etc.
> 
> https://github.com/Kitware/CMake/search?p=2&q=CMAKE_DL_LIBS&type=&utf8=%E2%9C%93
> 
> I know that we don't support multiple OSes as of today, but we can make it 
> compatible with every recognizable one with this change.
> 
> It looks like the proper syntax is to drop `-l` from my first proposal:
> 
> ```
> foreach(lib ${CMAKE_DL_LIBS})
> list(APPEND system_libs ${lib})
> endforeach()
> ```
Or perhaps even unconditional: `list(APPEND system_libs ${CMAKE_DL_LIBS})`


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Kamil Rytarowski via Phabricator via lldb-commits
krytarowski added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:354
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)

labath wrote:
> krytarowski wrote:
> > mgorny wrote:
> > > krytarowski wrote:
> > > > A more portable form of this:
> > > > 
> > > > ```
> > > > foreach(lib ${CMAKE_DL_LIBS})
> > > > list(APPEND system_libs -l${lib})
> > > > endforeach()
> > > > ```
> > > > 
> > > > Haiku needs 2 libraries, other Unices can use `-lldl` or similar.
> > > Didn't this raise the problem of cmake using full paths on some systems? 
> > No.
> Do you have some reference for the portability claim? (mainly because I'm 
> trying to learn more about how cmake works, but also because I'd rather let 
> cmake figure out when to prepend -l instead of doing it myself).
> 
> I don't see how the "two libraries" thingy comes into play here. It could be 
> a problem if we are mismatching lists and strings but I don't see why that 
> would necessitate manually tagging with -l.
dlopen(3)-like functions in `-ldl` is Linux; HPUX uses `-ldl`, Darwin/BSDs no 
extra libraries, Haiku `-lroot -lbe` etc.

https://github.com/Kitware/CMake/search?p=2&q=CMAKE_DL_LIBS&type=&utf8=%E2%9C%93

I know that we don't support multiple OSes as of today, but we can make it 
compatible with every recognizable one with this change.

It looks like the proper syntax is to drop `-l` from my first proposal:

```
foreach(lib ${CMAKE_DL_LIBS})
list(APPEND system_libs ${lib})
endforeach()
```


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Pavel Labath via Phabricator via lldb-commits
labath planned changes to this revision.
labath added a comment.

Right, so this will not work for the BUILD_SHARED_LIBS case, and there doesn't 
seem to be an easy way to make it work from this end. I'm going to try fixing 
this from the llvm side and come back to this if we still need the pthread/dl 
fixes.




Comment at: cmake/modules/LLDBConfig.cmake:354
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)

krytarowski wrote:
> mgorny wrote:
> > krytarowski wrote:
> > > A more portable form of this:
> > > 
> > > ```
> > > foreach(lib ${CMAKE_DL_LIBS})
> > > list(APPEND system_libs -l${lib})
> > > endforeach()
> > > ```
> > > 
> > > Haiku needs 2 libraries, other Unices can use `-lldl` or similar.
> > Didn't this raise the problem of cmake using full paths on some systems? 
> No.
Do you have some reference for the portability claim? (mainly because I'm 
trying to learn more about how cmake works, but also because I'd rather let 
cmake figure out when to prepend -l instead of doing it myself).

I don't see how the "two libraries" thingy comes into play here. It could be a 
problem if we are mismatching lists and strings but I don't see why that would 
necessitate manually tagging with -l.


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Kamil Rytarowski via Phabricator via lldb-commits
krytarowski added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:354
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)

mgorny wrote:
> krytarowski wrote:
> > A more portable form of this:
> > 
> > ```
> > foreach(lib ${CMAKE_DL_LIBS})
> > list(APPEND system_libs -l${lib})
> > endforeach()
> > ```
> > 
> > Haiku needs 2 libraries, other Unices can use `-lldl` or similar.
> Didn't this raise the problem of cmake using full paths on some systems? 
No.


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Michał Górny via Phabricator via lldb-commits
mgorny added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:354
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)

krytarowski wrote:
> A more portable form of this:
> 
> ```
> foreach(lib ${CMAKE_DL_LIBS})
> list(APPEND system_libs -l${lib})
> endforeach()
> ```
> 
> Haiku needs 2 libraries, other Unices can use `-lldl` or similar.
Didn't this raise the problem of cmake using full paths on some systems? 


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Kamil Rytarowski via Phabricator via lldb-commits
krytarowski added inline comments.



Comment at: cmake/modules/LLDBConfig.cmake:354
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)

A more portable form of this:

```
foreach(lib ${CMAKE_DL_LIBS})
list(APPEND system_libs -l${lib})
endforeach()
```

Haiku needs 2 libraries, other Unices can use `-lldl` or similar.


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Michał Górny via Phabricator via lldb-commits
mgorny added a comment.

Yes, full standalone.


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Pavel Labath via Phabricator via lldb-commits
labath added a comment.

Ah, yes, another build mode :/. Are you doing a standalone build or not?
I am pretty sure this will be fine for an integrated build, but I have no idea 
what will it do in the standalone mode...


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Michał Górny via Phabricator via lldb-commits
mgorny added a comment.

I can't test this right now but please make sure not to break linking to split 
shared libraries without dylib.


https://reviews.llvm.org/D44379



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


[Lldb-commits] [PATCH] D44379: [cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

2018-03-12 Thread Pavel Labath via Phabricator via lldb-commits
labath created this revision.
labath added reviewers: krytarowski, zturner.
Herald added a subscriber: mgorny.

This patch consists of two relatively independent changes:

- first, I teach the standalone build to detect the case when llvm was built in 
the shared mode and set the appropriate cmake variables to make the 
add_llvm_*** family of functions link against the shared library. Without 
these, the we would end up linking against the constituent .a files while other 
parts of the build (e.g. clang) would pull in the libllvm.so, which resulted in 
multiply defined symbols.

- second, I add detection code for pthread and dl libraries. This is necessary, 
because we have direct calls to these libraries (instead of going through llvm) 
and in the standalone build we cannot rely on llvm to detect these for us. In a 
standalone non-dylib build this was accidentaly working because these libraries 
were pulled in as an interface dependency of the .a files, but in a dylib build 
these are no longer part of the link interface, and so we need to add them 
explicitly.


https://reviews.llvm.org/D44379

Files:
  cmake/modules/LLDBConfig.cmake
  cmake/modules/LLDBStandalone.cmake


Index: cmake/modules/LLDBStandalone.cmake
===
--- cmake/modules/LLDBStandalone.cmake
+++ cmake/modules/LLDBStandalone.cmake
@@ -17,7 +17,9 @@
   "--includedir"
   "--prefix"
   "--src-root"
-  "--cmakedir")
+  "--cmakedir"
+  "--shared-mode"
+  )
 execute_process(
   COMMAND ${CONFIG_COMMAND}
   RESULT_VARIABLE HAD_ERROR
@@ -44,6 +46,7 @@
   list(GET CONFIG_OUTPUT 4 LLVM_OBJ_ROOT)
   list(GET CONFIG_OUTPUT 5 MAIN_SRC_DIR)
   list(GET CONFIG_OUTPUT 6 LLVM_CMAKE_PATH)
+  list(GET CONFIG_OUTPUT 7 SHARED_MODE)
 
   if(NOT MSVC_IDE)
 set(LLVM_ENABLE_ASSERTIONS ${ENABLE_ASSERTIONS}
@@ -72,6 +75,11 @@
 message(FATAL_ERROR "Not found: ${LLVMCONFIG_FILE}")
   endif()
 
+  if(SHARED_MODE STREQUAL "shared")
+set(LLVM_LINK_LLVM_DYLIB ON)
+set(LLVM_DYLIB_COMPONENTS all)
+  endif()
+
   # They are used as destination of target generators.
   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})
Index: cmake/modules/LLDBConfig.cmake
===
--- cmake/modules/LLDBConfig.cmake
+++ cmake/modules/LLDBConfig.cmake
@@ -346,12 +346,15 @@
 
 endif()
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
-
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(UNIX)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+  find_package(Threads REQUIRED)
+  list(APPEND system_libs ${CMAKE_THREAD_LIBS_INIT})
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)
+list(APPEND system_libs ${CMAKE_DL_LIBS})
+  endif()
 endif()
 
 # Figure out if lldb could use lldb-server.  If so, then we'll


Index: cmake/modules/LLDBStandalone.cmake
===
--- cmake/modules/LLDBStandalone.cmake
+++ cmake/modules/LLDBStandalone.cmake
@@ -17,7 +17,9 @@
   "--includedir"
   "--prefix"
   "--src-root"
-  "--cmakedir")
+  "--cmakedir"
+  "--shared-mode"
+  )
 execute_process(
   COMMAND ${CONFIG_COMMAND}
   RESULT_VARIABLE HAD_ERROR
@@ -44,6 +46,7 @@
   list(GET CONFIG_OUTPUT 4 LLVM_OBJ_ROOT)
   list(GET CONFIG_OUTPUT 5 MAIN_SRC_DIR)
   list(GET CONFIG_OUTPUT 6 LLVM_CMAKE_PATH)
+  list(GET CONFIG_OUTPUT 7 SHARED_MODE)
 
   if(NOT MSVC_IDE)
 set(LLVM_ENABLE_ASSERTIONS ${ENABLE_ASSERTIONS}
@@ -72,6 +75,11 @@
 message(FATAL_ERROR "Not found: ${LLVMCONFIG_FILE}")
   endif()
 
+  if(SHARED_MODE STREQUAL "shared")
+set(LLVM_LINK_LLVM_DYLIB ON)
+set(LLVM_DYLIB_COMPONENTS all)
+  endif()
+
   # They are used as destination of target generators.
   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})
Index: cmake/modules/LLDBConfig.cmake
===
--- cmake/modules/LLDBConfig.cmake
+++ cmake/modules/LLDBConfig.cmake
@@ -346,12 +346,15 @@
 
 endif()
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
-
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(UNIX)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+  find_package(Threads REQUIRED)
+  list(APPEND system_libs ${CMAKE_THREAD_LIBS_INIT})
+
+  check_library_exists(dl dlopen "" HAVE_LIBDL)
+  if (HAVE_LIBDL)
+list(APPEND system_libs ${CMAKE_DL_LIBS})
+  endif()
 endif()
 
 # Figure out if lldb could use lldb-server.  If so, then we'll
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/m