weidai11 / cryptopp

free C++ class library of cryptographic schemes
https://cryptopp.com
Other
4.8k stars 1.49k forks source link

"Broken module found" on Clang 7 #1031

Closed smessmer closed 3 years ago

smessmer commented 3 years ago

Crypto++ Issue Report

When updating the CryFS dependency from CryptoPP 8.2 to 8.5, I get a compiler error on Clang 7. It only happens on Clang 7 and only happens when optimizations are enabled.

Error message:

Alias must point to a definition
void (%"class.CryptoPP::IteratedHashBase"*)* @_ZN8CryptoPP16IteratedHashBaseImNS_18HashTransformationEED1Ev
Alias must point to a definition
void (%"class.CryptoPP::IteratedHashBase.266"*)* @_ZN8CryptoPP16IteratedHashBaseIjNS_18HashTransformationEED1Ev
LLVM ERROR: Broken module found, compilation aborted!
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Full build log: https://pastebin.com/BpF3CZP9

CryptoPP version: 8.5 OS: Happens on both Ubuntu 18.04 and 20.04 Compiler: Error happens on Clang 7. Newer versions of Clang don't trigger the error and neither GCC 7 or later. I haven't tested older Clang or GCC versions. Build settings: Happens with build settings RelWithDebInfo and Release. Building with Debug doesn't trigger the error.

In case this doesn't repro when building only CryptoPP, the steps to repro would be to take the https://github.com/cryfs/cryfs/tree/feature/cryptopp850 branch from CryFS (which is the branch with the CryptoPP upgrade) and build CryFS.

noloader commented 3 years ago

This looks like https://reviews.llvm.org/D24682. Can you update the compiler?

Another suggestion is to avoid -flto=thin at https://gitmemory.com/issue/ClangBuiltLinux/linux/509/501845067. But its not clear to me you are using LTO.

I'm also seeing some talk of Bitcode. I don't believe the library works with Bitcode. Or it did not work when I experimented with it. Also see https://www.cryptopp.com/wiki/Bitcode.

The source file in question has not changed in about 4 years. We updated the documentation, but there were no material changes to the code.


Here's what I am seeing as changed from 8.2 to 8.5 with respect to iterhash.{h|cpp}:

$ git diff CRYPTOPP_8_2_0..CRYPTOPP_8_5_0 -- iterhash.h iterhash.cpp
diff --git a/iterhash.h b/iterhash.h
index edaa18e2..5a65b5ba 100644
--- a/iterhash.h
+++ b/iterhash.h
@@ -1,5 +1,8 @@
 // iterhash.h - originally written and placed in the public domain by Wei Dai

+/// \file iterhash.h
+/// \brief Base classes for iterated hashes
+
 #ifndef CRYPTOPP_ITERHASH_H
 #define CRYPTOPP_ITERHASH_H

@@ -126,7 +129,7 @@ public:
        typedef T_Endianness ByteOrderClass;
        typedef T_HashWordType HashWordType;

-       CRYPTOPP_CONSTANT(BLOCKSIZE = T_BlockSize)
+       CRYPTOPP_CONSTANT(BLOCKSIZE = T_BlockSize);
        // BCB2006 workaround: can't use BLOCKSIZE here
        CRYPTOPP_COMPILE_ASSERT((T_BlockSize & (T_BlockSize - 1)) == 0);       // blockSize is a power of 2

@@ -138,7 +141,7 @@ public:
        unsigned int BlockSize() const {return T_BlockSize;}

        /// \brief Provides the byte order of the hash
-       /// \returns the byte order of the hash as an enumeration
+       /// \return the byte order of the hash as an enumeration
        /// \details GetByteOrder() returns <tt>T_Endianness::ToEnum()</tt>.
        /// \sa ByteOrder()
        ByteOrder GetByteOrder() const {return T_Endianness::ToEnum();}
@@ -159,8 +162,9 @@ public:
        }

 protected:
+       enum { Blocks = T_BlockSize/sizeof(T_HashWordType) };
        T_HashWordType* DataBuf() {return this->m_data;}
-       FixedSizeSecBlock<T_HashWordType, T_BlockSize/sizeof(T_HashWordType)> m_data;
+       FixedSizeSecBlock<T_HashWordType, Blocks> m_data;
 };

 /// \brief Iterated hash with a static transformation function
@@ -177,7 +181,7 @@ class CRYPTOPP_NO_VTABLE IteratedHashWithStaticTransform
        : public ClonableImpl<T_Transform, AlgorithmImpl<IteratedHash<T_HashWordType, T_Endianness, T_BlockSize>, T_Transform> >
 {
 public:
-       CRYPTOPP_CONSTANT(DIGESTSIZE = T_DigestSize ? T_DigestSize : T_StateSize)
+       CRYPTOPP_CONSTANT(DIGESTSIZE = T_DigestSize ? T_DigestSize : T_StateSize);

        virtual ~IteratedHashWithStaticTransform() {}

@@ -191,8 +195,9 @@ protected:
        void HashEndianCorrectedBlock(const T_HashWordType *data) {T_Transform::Transform(this->m_state, data);}
        void Init() {T_Transform::InitState(this->m_state);}

+       enum { Blocks = T_BlockSize/sizeof(T_HashWordType) };
        T_HashWordType* StateBuf() {return this->m_state;}
-       FixedSizeAlignedSecBlock<T_HashWordType, T_BlockSize/sizeof(T_HashWordType), T_StateAligned> m_state;
+       FixedSizeAlignedSecBlock<T_HashWordType, Blocks, T_StateAligned> m_state;
 };

 #if !defined(__GNUC__) && !defined(__clang__)
smessmer commented 3 years ago

I'm using clang 7.0.1-12, which is the newest version of clang 7 available on Ubuntu 18.04 or 20.04. I'm using thin-lto, I'm not using bitcode. The issue happens with CryptoPP 8.5 only, CryptoPP 8.2 builds fine.

It could indeed be a compiler bug. Is there maybe a way to workaround it in the crypto++ library?

noloader commented 3 years ago

You will probably have to skip LTO. We know there's problems on several platforms. We also know there's bad link optimizations when LTO is used. Also see https://www.cryptopp.com/wiki/Link_Time_Optimization.

noloader commented 3 years ago

I installed clang-7 on Ubuntu 20.

The build is OK with just Clang-7. The self tests are OK with just Clang-7.

 CXX=clang++-7 make -j 12

This causes a link problem.

CXX=clang++-7 CXXFLAGS="-DNDEBUG -g2 -O3 -flto=thin" make -j 12
...
/usr/bin/ld: ./libcryptopp.a: error adding symbols: archive has no index; run ranlib to add one
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [GNUmakefile:1450: cryptest.exe] Error 1

The link error appears to be https://bugs.llvm.org/show_bug.cgi?id=42684, which we reported 2 years ago.

I think you are going to have to forgo LTO.

smessmer commented 3 years ago

Thanks, disabling LTO fixed it.

yuanfang-chen commented 3 years ago

Newest report is here https://bugs.llvm.org/show_bug.cgi?id=49009.

The workaround is to move virtual IteratedHashBase::~IteratedHashBase(); definition to iterhash.cpp .