Making ZeroC Ice work with Clang, C++11 and libc++
ZeroC Ice (devel/ice) is an important part of the infrastructure of our internal services, therefore naturally it's the first major C++ port I'm trying to compile and run using Clang/C++11/libc++ on FreeBSD 9.1 RC1 as part of my project of migrating to FreeBSD 9.1, Clang, C++11 and libc++.
Earlier this year I made an effort to make Ice compile and run using Clang (and gcc47), but this was still using C++03 and gcc's libstdc++. I encountered a couple of obstacles while making this compile and run correctly using clang++ -std=c++11 -stdlib=libc++.
Naming conflicts (compile time, trivial)
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).
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 (compile time, forgivable)
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 (runtime, unfortunate)
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 (runtime, annoying)
Unit test IceGrid/deployer failed. As it turns out this is due a problem in the way libc++'s InputStream implementation works. It seems to be a similar problem that VC++6 had (a workaround for that is already in place). Basically tellg() returns -1 in case eof has been hit while gcc returns the last known position in the stream. Extending the workaround for VC++6 to also apply to libc++ fixed the problem.
--- a/cpp/src/IceGrid/FileCache.cpp
+++ b/cpp/src/IceGrid/FileCache.cpp
@@ -195,7 +195,10 @@ 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 (defined(_MSC_VER) && (_MSC_VER < 1300)) || defined(_LIBCPP_VERSION)
+ // TODO: Once this is fixed in libc++ check for maximum _LIBCPP_VERSION
+ // we're not checking for __clang__, since this is a libc++ only problem
if(is.eof())
{
newOffset += line.size();
Update 2012-09-11: 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();
}
The C++ Standard says:
"tellg(): (27.6.1.3)
After constructing a sentry object, if fail() != false, returns pos_type(-1) to indicate failure. Otherwise, returns rdbuf()->pubseekoff(0, cur, in).
sentry (27.6.1.1.2):
if noskipws is zero and is.flags() & ios_base::skipws is nonzero, the function extracts and discards each character as long as the next available input character c is a whitespace character. If is.rdbuf()->sbumpc() or is.rdbuf()->sgetc() returns traits::eof(), the function calls setstate(failbit | eofbit) (which may throw ios_base::failure).
So basically
- tellg() creates sentry object:
- sentry extracts white space characters and should set failbit after getting to eof.
- tellg() sees failbit should return eof() (-1)
So gcc-4.6 seems to behave correctly.."
User Artyom on Stackoverflow
Fixing Freeze so it works (more or less, beyond annoying)
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)
{
(Preliminary) Conclusion
This was a lot more complicated and time consuming than expected. While resolving compile time problems is easy, the runtime issues are definitely concerning. I definitely expect more problems to show up, that haven't been covered by the unit tests provided with Ice. Shortcomings in libc++ are annoying, but will most certainly go away soon - what worries me are the issues caused by the C++11's noexcept destructors, which haven't been detected at compile time - no warnings whatsoever - and lead to aborts at runtime. I'm pretty certain I'll see those a lot in the near future while trying to move our complete tool chain to C++11.
Update 2012-09-10: Changed Clang specific feature detection to C++ language version detection, so it still compiles and tests ok using gcc4.2. Added missing ICE_NOEXCEPT_TRUE definition for C++03 compilers. Removed speculation about shortcomings in Clang's iostream implementation. Corrected typos.
