Page MenuHomePhabricator

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

Authored by nitesh.jain on May 20 2016, 2:37 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain retitled this revision from to [LLDB][MIPS] Introduce a cmake module to check whether we need to link with libatomic..
nitesh.jain updated this object.
nitesh.jain added reviewers: emaste, zturner, ovyalov.
nitesh.jain set the repository for this revision to rL LLVM.
nitesh.jain added subscribers: bhushan, jaydeep, dsanders and 2 others.
zturner edited edge metadata.May 20 2016, 8:53 AM
zturner added a subscriber: 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?

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.

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

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 edited edge metadata.May 23 2016, 10:56 AM

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.

zturner resigned from this revision.Jun 2 2016, 8:38 PM
zturner removed a reviewer: zturner.
nitesh.jain retitled this revision from [LLDB][MIPS] Introduce a cmake module to check whether we need to link with libatomic. to [LLDB][MIPS] Check if libatomic needs to be specified explicitly..
nitesh.jain updated this object.
nitesh.jain edited reviewers, added: zturner; removed: vkalintiris.
nitesh.jain edited subscribers, added: vkalintiris; removed: zturner.

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

labath requested changes to this revision.Jun 24 2016, 3:59 AM
labath edited edge metadata.

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.

This revision now requires changes to proceed.Jun 24 2016, 3:59 AM
nitesh.jain edited edge metadata.

Thanks Labath.
Updated diff as per suggestion.

labath accepted this revision.Jun 29 2016, 4:18 AM
labath edited edge metadata.

lgtm, thanks.

This revision is now accepted and ready to land.Jun 29 2016, 4:18 AM
nitesh.jain closed this revision.Jun 29 2016, 7:06 AM

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.

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

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?

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.

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

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

I have attach log of CMake with --trace.

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 added a comment.EditedJul 7 2016, 2:29 AM

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

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.