Patch for compiling Ice with Clang, C++11 and libc++

grembo · September 2012

This patch allows for compilation and running of ZeroC Ice using Clang 3.1,
C++11 and libc++ (tested under FreeBSD 9.1 RC1). To compile Ice you would
typically set the following environment variables:

CC=clang
CXX=clang++
CXXFLAGS+=-std=c++11 -stdlib=libc++
CPP=clang-cpp

The changes done include simple fixes for compile time issues as well as
resolving more complicated runtime problems that only showed up while running
unit tests. It is indeed possible that there are additional undiscovered
problems.

To apply the patch to a fresh Ice 3.4.2 source distribution:

cd Ice-3.4.2
patch -p0 < ice_for_clang_c++11_libc++_2012-09-14.patch.txt

For those who are interested, I've put up a more detailed description of the
patch available at:

Making ZeroC Ice work with Clang, C++11 and libc++

This patch also includes the following patches from the ZeroC forums:

http://www.zeroc.com/forums/patches/5627-patch-3-ice-3-4-2-glacier2-memory-leak.html
http://www.zeroc.com/forums/patches/5645-two-small-patches-freeze-icegrid-former-potentially-bad.html
http://www.zeroc.com/forums/patches/5646-improvements-unit-tests-fix-encoding-freebsd-support.html
http://www.zeroc.com/forums/patches/5647-patch-compiling-ice-clang-gcc4-7-a.html
http://www.zeroc.com/forums/patches/5663-patch-prevent-icegrid-node-registry-replica-name-spoofing.html
http://www.zeroc.com/forums/patches/5781-patch-4-ice-3-4-2-icestorm-assert-bug-fix.html
http://www.zeroc.com/forums/patches/5815-improvements-python-unit-tests-fix-encoding-freebsd-jail-support.html

It should also work with the OS X version of Ice (e.g. through MacPorts or
Homebrew); minor adjustments might be necessary though.

Please note that even though I tried to make this as flawless as possible,
use of the patch is at your own risk.

Cheers,
Michael

grembo · September 2012

Explaining the patch - part 1

Since not everybody wants to follow links and some of those issues might actually be relevant to users of newer versions of gcc and its C++ standard library as well [even not in C++11 mode]), see some details from Making ZeroC Ice work with Clang, C++11 and libc++ below (please check the blog entry for a even more details, but this will capture the essentials):

Naming conflicts

The slice2*x* compilers used an IceUtil::Mutex called mutex. Since the Ice source code unfortunately uses using namespace std a lot, this lead to a name resolution conflict. The obvious fix is to rename all those instances to to a different name (mtx) - no patch here, since it is trivial (the original patch contains it of course).

A similar problem exists in Network.cpp, where a call to bind (the C POSIX socket function) is mistaken with std::bind (again, using namespace std is just a bad idea). The error message generated by clang isn't as obvious as one might hope, but anyway, qualifying the call (::bind) fixed the problem:

--- a/cpp/src/Ice/Network.cpp
+++ b/cpp/src/Ice/Network.cpp
@@ -1102,7 +1102,7 @@ IceInternal::doBind(SOCKET fd, const struct
sockaddr_storage& addr)
         size = 0; // Keep the compiler happy.
     }

-    if(bind(fd, reinterpret_cast<const struct sockaddr*>(&addr), size) == SOCKET_ERROR)
+    if(::bind(fd, reinterpret_cast<const struct sockaddr*>(&addr), size) == SOCKET_ERROR)
     {
         closeSocketNoThrow(fd);
         SocketException ex(__FILE__, __LINE__);

Deleted implicit copy constructor

The GroupNodeInfo structure has a couple of const members, which make C++11 (implicitly) delete the implicit copy constructor. A simple fix is to remove the const keyword from the members (shouldn't have any ill side effects):

--- a/cpp/src/IceStorm/Replica.h
+++ b/cpp/src/IceStorm/Replica.h
@@ -23,9 +23,9 @@ struct GroupNodeInfo
     GroupNodeInfo(int i, LogUpdate l, const Ice::ObjectPrx& o = Ice::ObjectPrx());
     bool operator<(const GroupNodeInfo& rhs) const;
     bool operator==(const GroupNodeInfo& rhs) const;
-    const int id;
-    const LogUpdate llu;
-    const Ice::ObjectPrx observer;
+    int id;
+    LogUpdate llu;
+    Ice::ObjectPrx observer;
 };

Overloading problem for vector<bool> specialization

Ice's streaming class implementation provides a specialized overload for vector<bool> (which by the way is probably one of the most terrible design choices taken by the C++ Standards Committee ever). The overload itself worked properly, but when the output stream write function is called, the compiler selects the wrong overloaded function - without any warning, so only running the Ice/stream unit test reveals that there is a problem.

I'm not 100% certain if this is a problem of Clang, C++11 or libc++. Whatever the exact problem is, a static_cast to bool fixes the problem in a portable way:

--- a/cpp/include/Ice/Stream.h
+++ b/cpp/include/Ice/Stream.h
@@ -667,7 +667,7 @@ struct StreamWriter<StreamTraitTypeSequenceBool>
         outS->writeSize(static_cast<Int>(v.size()));
         for(typename T::const_iterator p = v.begin(); p != v.end(); ++p)
         {
-            outS->write(*p);
+            outS->write(static_cast<bool>(*p));
         }
     }
 };

InputStream implementation in libc++ and workaround

Unit test IceGrid/deployer failed due to a different implementation of InputStream in libc++

It seems like that this might be actually expected behavior (hitting eof sets the failed bit, which in turn makes tellg return -1, it's not immediately apparent, reading the standard on tellg, seekg and getline). I ended up using the following version, which should actually work on all compilers we care about, including gcc4.2, gcc4.6 and gcc4.7 and their standard libraries:

--- a/cpp/src/IceGrid/FileCache.cpp
+++ b/cpp/src/IceGrid/FileCache.cpp
@@ -195,15 +195,12 @@ FileCache::read(const string& file, Ice::Long offset, int size, Ice::Long& newOf

         totalSize += lineSize;
         lines.push_back(line);
-#if defined(_MSC_VER) && (_MSC_VER < 1300)
-        if(is.eof())
+
+        if(is.eof() || is.fail())
         {
             newOffset += line.size();
         }
         else
-#else
-        if(!is.fail())
-#endif
         {
             newOffset = is.tellg();
         }

grembo · September 2012

Explaining the patch - part2

Fixing Freeze

Unit test Freeze/evictor crashed consistently (abort, signal 6, bus error). As it turns out, the implementation of Freeze depends on throwing from destructors. Obviously this is not the kind of design anybody wants to see (and as further investigations showed, this seems to be unique within the project). Since C++11 changed the default for destructors to be non-throwing (noexcept(true)) this leads to crashes every time the unit test is run (especially DeadLockExceptions, which happen a lot in this unit test, seem to be a problem). Personally I think this design is questionable. Since I'm not on a mission to improve the project per se, but just want to make it work on a new platform, the solution looks more like a workaround:

Create a set of new macros

These macros are defined based on if the compiler supports the noexcept keyword (which no C++03 compiler does). ICE_NOEXCEPT_FALSE will be applied to destructors that are known to throw eventually.

--- a/cpp/include/IceUtil/Config.h
+++ b/cpp/include/IceUtil/Config.h
@@ -248,3 +248,16 @@ public:
 #define ICE_DEFAULT_MUTEX_PROTOCOL PrioNone

 #endif
+
+
+//
+// Macro used for declaring destructors that might throw - required for C++11
+//
+#if __cplusplus >= 201103L
+#define ICE_DESTRUCTORS_DONT_THROW_BY_DEFAULT
+#define ICE_NOEXCEPT_FALSE noexcept(false)
+#define ICE_NOEXCEPT_TRUE noexcept(true)
+#else
+#define ICE_NOEXCEPT_FALSE
+#define ICE_NOEXCEPT_TRUE
+#endif

Change a couple of base classes to declare throwing destructors

These base classes are required to have the same lose exception requirements as inherited classes - since Cache is only used by Freeze, it seemed okay to just change this, for SimpleShared I created a new alternative called SimpleSharedUnsafeDestructor to inherit from:

--- a/cpp/include/IceUtil/Cache.h
+++ b/cpp/include/IceUtil/Cache.h
@@ -77,7 +77,7 @@ protected:
     {
     }

-    virtual ~Cache()
+    virtual ~Cache() ICE_NOEXCEPT_FALSE
     {
     }

--- a/cpp/include/IceUtil/Shared.h
+++ b/cpp/include/IceUtil/Shared.h
@@ -50,6 +50,11 @@
 //
 // A non thread-safe base class for reference-counted types.
 //
+// IceUtil::SimpleSharedUnsafeDestructor
+// =====================
+//
+// A non thread-safe base class for reference-counted types - destructor might throw.
+//
 // IceUtil::Shared
 // ===============
 //
@@ -109,6 +114,57 @@ private:
     bool _noDelete;
 };

+class ICE_UTIL_API SimpleSharedUnsafeDestructor
+{
+public:
+
+    SimpleSharedUnsafeDestructor();
+    SimpleSharedUnsafeDestructor(const SimpleSharedUnsafeDestructor&);
+
+    virtual ~SimpleSharedUnsafeDestructor() ICE_NOEXCEPT_FALSE
+    {
+    }
+
+    SimpleSharedUnsafeDestructor& operator=(const SimpleSharedUnsafeDestructor&)
+    {
+        return *this;
+    }
+
+    void __incRef()
+    {
+        assert(_ref >= 0);
+        ++_ref;
+    }
+
+    void __decRef()
+    {
+        assert(_ref > 0);
+        if(--_ref == 0)
+        {
+            if(!_noDelete)
+            {
+                _noDelete = true;
+                delete this;
+            }
+        }
+    }
+
+    int __getRef() const
+    {
+    {
+        return _ref;
+    }
+
+    void __setNoDelete(bool b)
+    {
+        _noDelete = b;
+    }
+
+private:
+
+    int _ref;
+    bool _noDelete;
+};
+
 class ICE_UTIL_API Shared
 {
 public:

Change Freeze specific code to enable throwing destructors

This is possible for almost all classes involved. Only MapDb, which inherits from Db and is part of Berkeley DB doesn't really allow this, so instead the throw is changed to an error log (this is about closing the database, so not much to recover from anyway). Again, the design choice to throw in destructors seems unfortunate to me.

--- a/cpp/demo/Freeze/customEvictor/Evictor.h
+++ b/cpp/demo/Freeze/customEvictor/Evictor.h
@@ -66,6 +66,7 @@ class Evictor : public Ice::ServantLocator
 public:

     Evictor(CurrentDatabase&, int);
+    virtual ~Evictor() ICE_NOEXCEPT_TRUE {};

     virtual Ice::ObjectPtr locate(const Ice::Current&, Ice::LocalObjectPtr&);
     virtual void finished(const Ice::Current&, const Ice::ObjectPtr&, const Ice::LocalObjectPtr&);

--- a/cpp/src/Freeze/MapDb.cpp
+++ b/cpp/src/Freeze/MapDb.cpp
@@ -72,7 +72,13 @@ Freeze::MapDb::~MapDb()
         }
         catch(const ::DbException& dx)
         {
+#if defined(ICE_DESTRUCTORS_DONT_THROW_BY_DEFAULT)
+            Error out(_communicator->getLogger());
+            out << "DbException while closing database " << _dbName << ": "
+                << dx.what();
+#else
             throw DatabaseException(__FILE__, __LINE__, dx.what());
+#endif
         }
     }
 }
--- a/cpp/src/Freeze/MapI.h
+++ b/cpp/src/Freeze/MapI.h
@@ -63,12 +63,13 @@ public:
     void
     close();

-    class Tx : public IceUtil::SimpleShared
+    class Tx : public IceUtil::SimpleSharedUnsafeDestructor
     {
     public:

         Tx(const MapHelperI&);
-        ~Tx();
+        ~Tx() ICE_NOEXCEPT_FALSE;
+        ;

         void dead();

--- a/cpp/src/Freeze/ObjectStore.cpp
+++ b/cpp/src/Freeze/ObjectStore.cpp
@@ -189,7 +189,7 @@ Freeze::ObjectStoreBase::ObjectStoreBase(const string& facet, const string& face
     }
 }

-Freeze::ObjectStoreBase::~ObjectStoreBase()
+Freeze::ObjectStoreBase::~ObjectStoreBase() ICE_NOEXCEPT_FALSE
 {
     try
     {

--- a/cpp/src/Freeze/ObjectStore.h
+++ b/cpp/src/Freeze/ObjectStore.h
@@ -36,7 +36,7 @@ public:
     ObjectStoreBase(const std::string&, const std::string&, bool, EvictorIBase*,
                     const std::vector<IndexPtr>&, bool);

-    virtual ~ObjectStoreBase();
+    virtual ~ObjectStoreBase() ICE_NOEXCEPT_FALSE;

     const Ice::ObjectPtr& sampleServant() const;


--- a/cpp/src/Freeze/TransactionalEvictorContext.cpp
+++ b/cpp/src/Freeze/TransactionalEvictorContext.cpp
@@ -273,7 +273,7 @@ Freeze::TransactionalEvictorContext::ServantHolder::ServantHolder() :
 }


-Freeze::TransactionalEvictorContext::ServantHolder::~ServantHolder()
+Freeze::TransactionalEvictorContext::ServantHolder::~ServantHolder() ICE_NOEXCEPT_FALSE
 {
     if(_ownBody && _body.ownServant)
     {

--- a/cpp/src/Freeze/TransactionalEvictorContext.h
+++ b/cpp/src/Freeze/TransactionalEvictorContext.h
@@ -34,7 +34,7 @@ public:
     public:

         ServantHolder();
-        ~ServantHolder();
+        ~ServantHolder() ICE_NOEXCEPT_FALSE;

         void init(const TransactionalEvictorContextPtr&, const Ice::Current&, ObjectStore<TransactionalEvictorElement>*);

--- a/cpp/src/Freeze/TransactionalEvictorI.cpp
+++ b/cpp/src/Freeze/TransactionalEvictorI.cpp
@@ -346,7 +346,7 @@ Freeze::TransactionalEvictorI::dispatch(Request& request)
         {
         }

-        ~CtxHolder()
+        ~CtxHolder() ICE_NOEXCEPT_FALSE
         {
             if(_ownCtx)
             {

I hope these explanations help to understand how the patch works.

grembo · November 2012

Now in FreeBSD ports

The latest version of the FreeBSD port devel/ice contains those changes, upgrading with portupgrade/portmaster should work as expected.

See also:
ports/171643: [MAINTAINER] devel/ice: Fixes for Clang, C++11, libc++

Archived

Patch for compiling Ice with Clang, C++11 and libc++

Comments

Categories