Page MenuHomePhabricator

Add -lpthread to LLDB shared lib link line unconditionally
ClosedPublic

Authored by loladiro on Sep 21 2014, 2:52 PM.

Details

Summary

Usually -lpthread is included due to LLVM link options,
but when LLVM threading is disabled, this does not happen.
pthread is still needed however because LLDB uses threading
regardless of whether LLVM is built with threading support or not.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 13914.Sep 21 2014, 2:52 PM
loladiro retitled this revision from to Add -lpthread to LLDB shared lib link line unconditionally.
loladiro updated this object.
loladiro edited the test plan for this revision. (Show Details)
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: Unknown Object (MLST).

Bump, just had somebody run into this again. Does this look ok?

emaste added a subscriber: emaste.Oct 24 2014, 4:48 PM

Is the cmake build not affected?

I suspect it is, but I don't know enough about the CMake build system to confirm.

loladiro updated this revision to Diff 17650.Dec 28 2014, 12:45 AM
loladiro set the repository for this revision to rL LLVM.

I got around to checking with cmake and it doesn't seems to be affected. I did have to add one small fix to make it build (included here), but other than that it went through fine. If there's no further problems, I'll commit this soon then.

loladiro abandoned this revision.Apr 13 2015, 1:31 PM

This seems to have been fixed by D8991.

loladiro reclaimed this revision.Jun 11 2015, 11:03 AM

I was wrong, this is still an issue. Would like to commit this.

loladiro added a subscriber: labath.

@labath Would you mind taking a look at this as well?

labath edited edge metadata.Jun 18 2015, 5:17 PM

The makefile changes can go in, but I would like more explanation on the cmake change.

scripts/CMakeLists.txt
1 ↗(On Diff #17650)

What was the exact reason you needed to add this?

loladiro added inline comments.Jun 18 2015, 5:21 PM
scripts/CMakeLists.txt
1 ↗(On Diff #17650)

We disable threads and LLDB_DISABLE_PYTHON and all of this disables python stuff that isn't needed (and errors if you don't use python/don't have it installed). Originally I only had the Makefile change, but I was asked to check if cmake worked in this configuration, and this was the one thing I had to change to make it work. I'll happily make this a separate commit, it was just here for convenience.

I'm just trying to understand what are the implications of this.

Right now, using LLDB_DISABLE_PYTHON will prevent lldb from linking
against python. Is that correct?

With your patch the flag will also prevent the generation of the
python SBAPI? Or what exactly?

Yes, that's correct. LLDB_DISABLE_PYTHON prevents LLDB from linking python (and disables python specific functionality in the source of course). This also disables the swig wrapper generation.

Currently, the description of LLDB_DISABLE_PYTHON is "Disables the
Python scripting integration". I guess we could say that the swig
wrapper falls in this category. I personally have no issue with that,
but I don't know if anyone depends on that behaviour.

Currently, the description of LLDB_DISABLE_PYTHON is "Disables the
Python scripting integration". I guess we could say that the swig
wrapper falls in this category. I personally have no issue with that,
but I don't know if anyone depends on that behaviour.

There are plans to extend the scripting integration to other languages supported by SWIG and so we'll probably have to revisit this when that happens.

I don't really care what the switches are named as long as there is an option that removes any dependency on Python.

Bump. Is the CMake change ok or would you like to ponder it further?

I will go ahead with the Makefile changes and submit a separate revision for the CMake ones.

Sure, commit the makefile changes any time you like.

Sorry for the delay. I was not sure if I want this change or not, and I was hoping someone else will have a say. BTW, is the cmake change even necessary now? I just noticed that the root CMakeLists.txt has

if (NOT LLDB_DISABLE_PYTHON)
  add_subdirectory(scripts)
endif ()
This revision was automatically updated to reflect the committed changes.

Yup, seems like that would do it as well. Great, thanks!