[LLDB][MIPS] Check if libatomic needs to be specified explicitly.

[nitesh.jain]

Summary

The problem appears while linking liblldb.so. The class Address contain atomic variable m_offset. The loading and storing of variable is access via atomic_load_8 and atomic_store_8 functions. Some target fail to implicitly link libatomic, which cause undefine reference to atomic_store_8/atomic_load_8. This patch uses http://reviews.llvm.org/D20896 to check if libatomic need to be explicitly link.

[zturner]

This sounds like an issue that either has already been solved in LLVM's
CMake, or should be solved in LLVM's CMake. Can you find out how they
decide whether to use -latomic and piggyback off that?

[nitesh.jain]

We can't use LLVM's CheckAtomic cmake module since it is fix for 32 bit atomic operations. The LLDB's CheckAtomic cmake module allows us to check for 64-bit atomics operations.

[nitesh.jain]

There was similar fix in libcxx-specific one.
http://reviews.llvm.org/D16613

[zturner]

So I don't really know much about this, but it seems wrong to me to be duplicating this code. As far as I can tell, the only difference between the one in llvm is that it allows for checking of 64 bit atomics. Why not fold that logic into LLVM's CheckAtomic though? Or at the very least, why not make a separate CheckAtomic64 in LLVM that both libcxx and now LLDB could both use? The code here and in libcxx is almost the same, it doesn't make much sense to me to be duplicating it.

cmake/modules/CheckLLDBAtomic.cmake

7 ↗

(On Diff #57909)

This isn't going to work with MSVC. If the compiler is MSVC we should simply set the variable to true, since it is known to support atomics.

[vkalintiris]

From what I remember it boils down to the fact that we don't want to have atomics whose width isn't natively supported on our targets. Take a look at the comments from this thread: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/328321.html.

Or at the very least, why not make a separate CheckAtomic64 in LLVM that both libcxx and now LLDB could both use? The code here and in libcxx is almost the same, it doesn't make much sense to me to be duplicating it.

That's an option too. I wasn't aware that LLDB needs the same type of check at the time.

[nitesh.jain]

This patch uses http://reviews.llvm.org/D20896 to check if libatomic need to be explicitly link

[labath]

In general, the idea sounds reasonable.

I think you should be able to achieve the same effect by adding the library to LLDB_SYSTEM_LIBS in LLDBDependencies.cmake. This should be a much cleaner (and definitely more consistent) way of doing things.

[nitesh.jain]

Thanks Labath.
Updated diff as per suggestion.

[labath]

lgtm, thanks.

[Eugene.Zelenko]

This change broke standalone build. Looks like check which should set HAVE_CXX_ATOMICS64_WITHOUT_LIB was not performed.

I build LLDB with LLVM STL which doesn't have libatomic.

[nitesh.jain]

In D20464#470527, @Eugene.Zelenko wrote:

This change broke standalone build. Looks like check which should set HAVE_CXX_ATOMICS64_WITHOUT_LIB was not performed.

I build LLDB with LLVM STL which doesn't have libatomic.

The CheckAtomic.cmake module (http://reviews.llvm.org/D20896) in llvm will set HAVE_CXX_ATOMICS64_WITHOUT_LIB to true if atomics work without the library. Please can you check whether it is set to true in your case ?

Thanks

[Eugene.Zelenko]

I see HAVE_CXX_ATOMICS64_WITHOUT_LIB messages during LLVM Cmake run, but I don't see such message during LLDB CMake run.

Is this value is supposed to be read form LLVM CMake cache?

[nitesh.jain]

In D20464#471803, @Eugene.Zelenko wrote:

I see HAVE_CXX_ATOMICS64_WITHOUT_LIB messages during LLVM Cmake run, but I don't see such message during LLDB CMake run.

Is this value is supposed to be read form LLVM CMake cache?

Yes.

[Eugene.Zelenko]

I run CMake with --trace and is mentioned only in condition added there.

[nitesh.jain]

In D20464#472733, @Eugene.Zelenko wrote:

I run CMake with --trace and is mentioned only in condition added there.

I have attach log of CMake with --trace.

log23 MBDownload

[Eugene.Zelenko]

As fas as I could judge from log, you built LLDB with LLVM/Clang. But problem happens when it's necessary to build LLDB separately from LLVM/Clang (standalone build).

[nitesh.jain]

In D20464#473884, @Eugene.Zelenko wrote:

As fas as I could judge from log, you built LLDB with LLVM/Clang. But problem happens when it's necessary to build LLDB separately from LLVM/Clang (standalone build).

Then we need to introduce a 64 bit atomic check in cmake/modules/LLDBStandalone.cmake. What do you think?

Thanks

[Eugene.Zelenko]

Then we need to introduce a 64 bit atomic check in cmake/modules/LLDBStandalone.cmake. What do you think?

Thanks

I think this should be right solution.

cmake/LLDBDependencies.cmake

if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows" AND NOT __ANDROID_NDK__)
   endif()
   if (NOT LLDB_DISABLE_CURSES)
     list(APPEND LLDB_SYSTEM_LIBS ${CURSES_LIBRARIES})
     if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
       list(APPEND LLDB_SYSTEM_LIBS ${TERMINFO_LIBS})
     endif()
   endif()
 endif()
+
+if (NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB )
+    list(APPEND LLDB_SYSTEM_LIBS atomic)
+endif()
+
 # On FreeBSD/NetBSD backtrace() is provided by libexecinfo, not libc.
 if (CMAKE_SYSTEM_NAME MATCHES "FreeBSD" OR CMAKE_SYSTEM_NAME MATCHES "NetBSD")
   list(APPEND LLDB_SYSTEM_LIBS execinfo)
 endif()
 
 if (NOT LLDB_DISABLE_PYTHON AND NOT LLVM_BUILD_STATIC)
   list(APPEND LLDB_SYSTEM_LIBS ${PYTHON_LIBRARIES})
 endif()