[RFC] Solve linking inconsistency, proposal two

[labath]

Summary

This "solves" the ODR violations by making the linker firewall a feature. llvm
libraries used by liblldb are considered to be private implementation details and
we do not provide them to the users. If some of them want to use llvm for their
private purposes, they should include their own copy. LLDB_EXPORT_ALL_SYMBOLS=OFF
is the official way of building liblldb, and the =ON version is a hack to make
backtrace(3) output more useful on linux. It still should not affect the way
dependant executables are linked, as none of them should be using lldb_private
symbols in the first place.

We need to be careful to not use llvm classes anywhere along the API boundary, or
bad things will happen. With our current SB API policy this should not be a
problem, but we may need to revisit this decision if that changes.

This is my less preferred solution, as it increases code size, and has potential
to introduce latent bugs.

[clayborg]

I prefer this solution for now over exporting all of llvm. The other solution introduces fragility in a shared library that currently vends a solid reliable API.

[mehdi_amini]

and has potential to introduce latent bugs.

Can you elaborate on this?

[labath]

In D26190#584882, @mehdi_amini wrote:

and has potential to introduce latent bugs.

Can you elaborate on this?

If some of the duplicated objects get passed across the api boundary, things will start to break, although the breakage may not be apparent immediately (e.g. In Todd's case things were only breaking in debug builds because that enabled additional constructors). As we noted in the other thread, it should be fine as long as we keep our API free of the llvm classes.

[mehdi_amini]

OK!

How is it different from the other proposal? In the other proposal, using a LLVM class in the API boundary would break the same way under the same conditions, or did I miss something?

Something that isn't clear to me with this proposal, is how a user supposed to include its own copy of LLVM? Let say I write a client code that link to libLLVM.so and libLLDB.so?

[labath]

In D26190#584933, @mehdi_amini wrote:

OK!

How is it different from the other proposal? In the other proposal, using a LLVM class in the API boundary would break the same way under the same conditions, or did I miss something?

In the other proposal, the user would not need to include his own copy of llvm, as he could use the one that is already contained in liblldb. Everything would work fine and you could pass objects around freely (except on windows, apparently). What would *not* work is if the user upgraded liblldb, but did not recompile the client (e.g. lldb-mi, or xcode), in this case he would probably get errors at load time due to mismatched symbols.

Something that isn't clear to me with this proposal, is how a user supposed to include its own copy of LLVM? Let say I write a client code that link to libLLVM.so and libLLDB.so?

We are assuming static linking everywhere. liblldb includes a static copy of llvm libraries, and the "user" could do the same.

In case LLVM itself is a shared library and both liblldb and lldb-mi link to it dynamically (-DLLVM_LINK_LLVM_DYLIB) getting a second copy is hard or impossible, but then it does not really matter, as all references will be resolved correctly and uniquely. Everything should work fine as long as you don't actually *depend* on having a separate copy of llvm (which is pretty pointless as it does not have global state (apart from the crazy cl globals)). So, what we are talking about here actually is whether liblldb should re-export llvm interfaces when it has a static copy of it.

Does that make sense?

[jingham]

Note that in this proposal, lldb is doing pretty much the same thing that libclang does. libclang exports its specific functionality but makes no attempt to export the llvm classes it uses. The only difference is that lldb exports a tightly controlled C++ API, whereas libclang exports a C interface.

[mehdi_amini]

Does that make sense?

This makes sense (assuming static linking reduces some possibility though), but LLVM is not robust to mix and match build settings: building half of the source with -DNDEBUG and not the other is likely to cause weird runtime failures. That can be an issue because now you need libLLDB built in two modes and the client app to link the right one.

Also, exporting more than the minimum prevent an efficient LTO build of libLLDB.so

Everything should work fine as long as you don't actually *depend* on having a separate copy of llvm (which is pretty pointless as it does not have global state (apart from the crazy cl globals)).

Is it pointless? Users have dependencies they don't control. I have seen mentioned in the past issues with "symbol pollution" from external library that was affecting LLVM users (a quick search yields https://root.cern.ch/phpBB3/viewtopic.php?f=3&t=22462&sid=dfd0c149390349defea19eb9ce0073c5 )

[labath]

In D26190#585016, @mehdi_amini wrote:

Does that make sense?

This makes sense (assuming static linking reduces some possibility though)

I am not sure we are on the same page here. What is the scenario you have in mind here? I am not forbidding anyone from building liblldb linking to libllvm (in fact, I think it should work). I am assuming static linking just because that is the use case most developers use.

, but LLVM is not robust to mix and match build settings: building half of the source with -DNDEBUG and not the other is likely to cause weird runtime failures. That can be an issue because now you need libLLDB built in two modes and the client app to link the right one.

I don't see how this applies. This is true when you build liblldb, and it's true that lldb's assertion settings need to match llvm's, but since liblldb's api is stable wrt. N/_DEBUG, it's user should not need to care about that.

Also, exporting more than the minimum prevent an efficient LTO build of libLLDB.so

This proposal is about exporting the bare minimum (i.e. lldb namespace).

Everything should work fine as long as you don't actually *depend* on having a separate copy of llvm (which is pretty pointless as it does not have global state (apart from the crazy cl globals)).

Is it pointless? Users have dependencies they don't control. I have seen mentioned in the past issues with "symbol pollution" from external library that was affecting LLVM users (a quick search yields https://root.cern.ch/phpBB3/viewtopic.php?f=3&t=22462&sid=dfd0c149390349defea19eb9ce0073c5 )

Again I think we are misunderstanding each other, the "pointless" was referring to the "relying on global state of libllvm" part of the statement. It seems the problem you quote was caused by a library exporting interfaces it should have kept private, which is what we are doing here (but in your first sentence you seem to indicate you prefer dynamic linking, in which case you cannot hide the symbols (or can you?)).

I think you'll have to repeat your point, as now I am totally confused about what you are trying to say. :)

[mehdi_amini]

Just to clarify: I don't have a dog in this, I'm just lurking here to learn something, and I'm curious about the tradeoff.

In D26190#585059, @labath wrote:

In D26190#585016, @mehdi_amini wrote:

Does that make sense?

This makes sense (assuming static linking reduces some possibility though)

I am not sure we are on the same page here. What is the scenario you have in mind here? I am not forbidding anyone from building liblldb linking to libllvm (in fact, I think it should work). I am assuming static linking just because that is the use case most developers use.

Same case as I mentioned below: namespace pollution.

, but LLVM is not robust to mix and match build settings: building half of the source with -DNDEBUG and not the other is likely to cause weird runtime failures. That can be an issue because now you need libLLDB built in two modes and the client app to link the right one.

I don't see how this applies. This is true when you build liblldb, and it's true that lldb's assertion settings need to match llvm's, but since liblldb's api is stable wrt. N/_DEBUG, it's user should not need to care about that.

It applies at the moment you consider that the client code does not need to link to LLVM because LLDB provides a copy. It means that now, even if it is not relevant for the LLVM interface, the client code needs to be built with the same configuration as the libLLDB shared lib.

Not exposing LLVM but only the minimal non-LLVM API prevents the client code from using any LLVM symbol from libLLDB, and thus makes it possible to mix-n-match a release build of libLLDB with an assert built client code.

Make sense?

Also, exporting more than the minimum prevent an efficient LTO build of libLLDB.so

This proposal is about exporting the bare minimum (i.e. lldb namespace).

Yes: this is a good point of this proposal that I was just pointing (sorry I wasn't clear).

Everything should work fine as long as you don't actually *depend* on having a separate copy of llvm (which is pretty pointless as it does not have global state (apart from the crazy cl globals)).

Is it pointless? Users have dependencies they don't control. I have seen mentioned in the past issues with "symbol pollution" from external library that was affecting LLVM users (a quick search yields https://root.cern.ch/phpBB3/viewtopic.php?f=3&t=22462&sid=dfd0c149390349defea19eb9ce0073c5 )

Again I think we are misunderstanding each other, the "pointless" was referring to the "relying on global state of libllvm" part of the statement. It seems the problem you quote was caused by a library exporting interfaces it should have kept private, which is what we are doing here (but in your first sentence you seem to indicate you prefer dynamic linking, in which case you cannot hide the symbols (or can you?)).

I don't have any preference (or rather: I prefer static for performance/efficiency reason). I'm more concerned about what use-case exists out-there. Exporting LLVM APIs can cause problem to folks that want/need to build against various third-party.

I think you'll have to repeat your point, as now I am totally confused about what you are trying to say. :)

Sorry, I hope it is more clear now?

[labath]

In D26190#585106, @mehdi_amini wrote:

, but LLVM is not robust to mix and match build settings: building half of the source with -DNDEBUG and not the other is likely to cause weird runtime failures. That can be an issue because now you need libLLDB built in two modes and the client app to link the right one.

I don't see how this applies. This is true when you build liblldb, and it's true that lldb's assertion settings need to match llvm's, but since liblldb's api is stable wrt. N/_DEBUG, it's user should not need to care about that.

It applies at the moment you consider that the client code does not need to link to LLVM because LLDB provides a copy. It means that now, even if it is not relevant for the LLVM interface, the client code needs to be built with the same configuration as the libLLDB shared lib.

Not exposing LLVM but only the minimal non-LLVM API prevents the client code from using any LLVM symbol from libLLDB, and thus makes it possible to mix-n-match a release build of libLLDB with an assert built client code.

Make sense?

Ok, so you were pointing out a flaw in the first proposal, not this one. I think that's the part I missed.

Yes, If we exported the llvm interface, the user could not use a different version of llvm elsewhere in the program. The thing is, the user is not supposed to do that anyway, as it violates the One Definition Rule of c++:

§4: Every program shall contain exactly one definition of every non-inline function or variable that is odr-used
in that program; no diagnostic required. [...]
§6: There can be more than one definition of a class type (Clause 9), enumeration type (7.2), [list of other kinds of entities] in a program provided that each definition appears in a different translation unit, and provided the definitions satisfy the following requirements. Given such an entity named D defined in more than one translation unit, then:
- each definition of D shall consist of the same sequence of tokens; and
- [...]

The last part fails as soon as mix different NDEBUG versions. We can make that work by not exporting the llvm interface, but then it's not really C++. :) And it can break in case you don't know what you are doing (occasionally you see people statically linking the standard c++ library into a part of their project and then marveling at the results).

This is why I wanted to propose both, although at this point I think I have convinced myself as well than the first one wasn't a good idea.

I think you'll have to repeat your point, as now I am totally confused about what you are trying to say. :)

Sorry, I hope it is more clear now?

Thanks. :)

lldb/trunk/tools/lldb-mi/CMakeLists.txt

-include(${LLDB_PROJECT_ROOT}/cmake/LLDBDependencies.cmake)
-
 set(LLDB_MI_SOURCES
   MICmdArgContext.cpp
   MICmdArgSet.cpp
   MICmdArgValBase.cpp
   MICmdArgValConsume.cpp
   MICmdArgValFile.cpp
   MICmdArgValListBase.cpp
   MICmdArgValListOfN.cpp

 
 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" OR CMAKE_SYSTEM_NAME MATCHES "NetBSD" )
   add_definitions( -DIMPORT_LIBLLDB )
   list(APPEND LLDB_MI_SOURCES
     ${LLDB_SOURCE_ROOT}/Host/common/GetOptInc.cpp
     )
 endif ()
 
+# We need to include the llvm components we depend on manually, as liblldb does
+# not re-export those.
+set(LLVM_LINK_COMPONENTS Support)
 add_lldb_executable(lldb-mi ${LLDB_MI_SOURCES})
 
 target_link_libraries(lldb-mi liblldb)
 if ( NOT CMAKE_SYSTEM_NAME MATCHES "Windows" )
   target_link_libraries(lldb-mi pthread)
 endif ()
 
-llvm_config(lldb-mi ${LLVM_LINK_COMPONENTS})
-
 set_target_properties(lldb-mi PROPERTIES VERSION ${LLDB_VERSION})
 
 install(TARGETS lldb-mi
   RUNTIME DESTINATION bin)