4

I'm catching a warning under Clang when testing a library under C++11. I've never come across the warning before and searching is not providing too much in the way of reading and research.

The warning is shown below, and it appears to be related to multiple inheritance and a common base class. But I'm not clear on the details triggering the warning or what I should do to address it.

My first question is, Is this a problem that needs to be addressed? Or is it a matter of efficiency alone?

My second question is (if needed), How do I address the warning? Or what are the options available to remediate it?


Here is some additional information:

  • Compiler: Apple LLVM version 6.0 (clang-600.0.57) (based on LLVM 3.5svn)
  • g++ -DDEBUG -g2 -O2 -std=c++11 -fPIC -march=native -pipe -c test.cpp

Is also reviewed the following on Stack Overflow, but its not clear to me where they intersect:

The library, Crypto++, also make heavy use of Curiously Recurring Template Pattern for compile time polymorphism.


The the header file is available online, and here is the actual warning:

g++ -DDEBUG -g2 -O2 -std=c++11  -Wno-deprecated-declarations -fPIC -march=native -pipe -c rsa.cpp
In file included from rsa.cpp:4:
In file included from ./rsa.h:12:
./pubkey.h:635:26: warning: defaulted move assignment operator of 'InvertibleRSAFunction' will move assign virtual base class 'CryptoMaterial' multiple times [-Wmultiple-move-vbase]
class CRYPTOPP_NO_VTABLE TF_ObjectImpl : public TF_ObjectImplBase<BASE, SCHEME_OPTIONS, KEY_CLASS>
                         ^
./rsa.h:57:44: note: 'CryptoMaterial' is a virtual base class of base class 'CryptoPP::RSAFunction' declared here
class CRYPTOPP_DLL InvertibleRSAFunction : public RSAFunction, public TrapdoorFunctionInverse, public PKCS8PrivateKey
                                           ^~~~~~~~~~~~~~~~~~
./rsa.h:57:96: note: 'CryptoMaterial' is a virtual base class of base class 'CryptoPP::PKCS8PrivateKey' declared here
class CRYPTOPP_DLL InvertibleRSAFunction : public RSAFunction, public TrapdoorFunctionInverse, public PKCS8PrivateKey
                                                                                               ^
1 warning generated.

My apologies for not reducing it. I'm not sure how to reduce it and capture the essence of the warning/complaint.

Community
  • 1
  • 1
jww
  • 83,594
  • 69
  • 338
  • 732
  • 1
    IMHO crypto++ is a broken library. I would suggest writing a few simple c++ wrappers around libopenssl. – Richard Hodges Jan 01 '16 at 11:54
  • @Richard - please forgive my ignorance.... What is broken in the library? (Library development has recently become active again, so there's a good chance the breaks will be fixed). – jww Jan 01 '16 at 12:19
  • In my view the interface is clumsy and it needs a lot of work to get it to compile with c++11 without warnings (which implies to me the danger of logic errors in critically important code). You can write trivial value-semantic wrappers around openssl's components with ease, which gives you compete type-safety, c++-style range-based interfaces and so on. (IMHO, again), crypto++'s attempt to make everything a stream is a logical error. – Richard Hodges Jan 01 '16 at 12:52
  • correct me if i'm wrong but i thing openssl is a bigger library with a wider remit. So more defects will be unsurprising. It's extremely actively maintained since it's the core of almost every linux web server in the world. In addition, it supports hardware-specific optimisations of encryption and decryption - performing the actual encryption in specialised hardware if available on the host. However, I do understand the attraction of using a c++-specific library. The above is just a summary of my experience. I'm sure mileage will vary. – Richard Hodges Jan 01 '16 at 13:20
  • 2
    Defect rate score factors in the code size, so I can't buy the argument that "openssl is a bigger library, therefore it's got to have more defects". I find the maintenance of openssl extremely active - but not necessarily better than, e.g., of Crypto++ or Botan. The fact that openssl is used in almost every linux web server did not save it from several quite severe attacks caused by bugs that persisted for almost a decade. Its hardware acceleration support is probably the best. – Mouse Jan 04 '16 at 00:04

2 Answers2

7

The warning seems self-explanatory to me, it's telling you that move-assigning the derived type will result in move-assigning the base twice.

Reducing it is trivial, just create an inheritance hierarchy using a virtual base and two paths to get to it:

#include <stdio.h>

struct V {
    V& operator=(V&&) { puts("moved"); return *this; }
};

struct A : virtual V { };

struct B : virtual V { };

struct C : A, B { };

int main() {
    C c;
    c = C{};
}

This will print "moved" twice, because the implicit move assignment operators for each of A, B and C will do a memberwise assignment, which means that both A::operator=(A&&) and B::operator=(B&&) will assign the base class. As Alan says, this is a valid implementation of the standard. (The standard specifies that on construction only the most-derived type will construct the virtual base, but it doesn't have the same requirement for assignment).

This isn't specific to move assignment, changing the base class to only support copy assignment and not move assignment will print "copied" twice:

struct V {
    V& operator=(const V&) { puts("copied"); return *this; }
};

This happens for exactly the same reason, both of A::operator=(A&&) and B::operator=(B&&) will assign the base class. The compiler doesn't warn for this case, because doing copy assignment twice is (probably) just suboptimal, not wrong. For move-assignment it might lose data.

If your virtual base doesn't actually have any data that needs to be copied or moved, or only has data members that are trivially copyable, then making it only support copying not moving will suppress the warning:

struct V {
    V& operator=(const V&) = default;
};

This copy assignment operator will still get called twice, but since it doesn't do anything there's no problem. Doing nothing twice is still nothing.

(GCC seems a bit smarter than Clang here, it doesn't warn about the virtual base's move assignment operator being called twice if it's trivial, because a trivial move is equivalent to a copy and so is less likely to be a problem).

If the virtual base does have data that needs to be copied on assignment then making it do a copy not a move might still be a good choice, but it depends what the type is and does. You might need to define copy and move assignment explicitly at every level of the hierarchy. Virtual bases are tricky and hard to use correctly, especially in the face of copying or moving. Treating types with virtual bases as value types that can be copied and moved easily might be a design error.

The iostreams hierarchy uses virtual bases, but is done carefully and correctly. The iostream types are non-copyable, only movable, and the derived types define move assignment explicitly to ensure the basic_ios<> base class only gets updated once. Specifically, basic_iostream::operator=(basic_iostream&&) only operates on the basic_istream base, not the basic_ostream one. The equivalent for the example above would be:

struct C : A, B {
     C& operator=(C&& c) {
         static_cast<A&>(*this) = static_cast<A&&>(c);
         return *this;
     }
};

Iostreams were not copyable at all until C++11 when rvalue references and move semantics made it possible to do with useful semantics. If your class has always been copyable in C++03 it might already have been a questionable design that should have had been non-copyable, or have carefully written copy operations not implicitly-defined ones.

In short, any time you have virtual bases you need to think very carefully about how construction, assignment and destruction work, and whether copying and assignment even make sense for the type.

Jonathan Wakely
  • 153,269
  • 21
  • 303
  • 482
  • Thanks Jonathan. *" If your class has always been copyable in C++03 it might already have been a questionable design that should have had been non-copyable..."* - this could well be the case, and we managed to squeak by until now. If that's the case, I'm happy to make the necessary changes to close the gap. I need to study it more. – jww Jan 07 '16 at 16:07
4

The standard allows implementations to choose a simple but sometimes broken way to handle memberwise assignment in the presence of virtual bases.

http://en.cppreference.com/w/cpp/language/move_assignment:

As with copy assignment, it is unspecified whether virtual base class subobjects that are accessible through more than one path in the inheritance lattice, are assigned more than once by the implicitly-defined move assignment operator.

This is particularly nasty for move assignment, since it may mean assigning from an already moved-from member.

Alan Stokes
  • 18,320
  • 3
  • 41
  • 63
  • Yeah, this can be bad for us... The top level class is `InvertibleRSAFunction`. Is there a way to say `using InvertibleRSAFunction::CryptoMaterial` and discard `RSAFunction::CryptoMaterial`, `TrapdoorFunctionInverse::CryptoMaterial` and `PKCS8PrivateKey::CryptoMaterial`? That is, I want the last three classes to move everything *except* `CryptoMaterial` because `CryptoMaterial` will be move'd in `InvertibleRSAFunction`? It kind of like I want to do a selective delete.... – jww Jan 01 '16 at 11:49
  • Do you need move assignment? I don't know a workaround other than explicitly specifying the operator in each derived class, which is a bit fragile. – Alan Stokes Jan 01 '16 at 13:11
  • *"Do you need move assignment?"* - apparently we do... The compiler is providing one. – jww Jan 07 '16 at 00:25
  • 3
    That doesn't mean you need it, it means it's happening. That's not the same thing. If you add `Derived(const Derived&) = default;` then the compiler won't generate a move assignment operator for `Derived` and rvalues will get copied instead of moved. You'll still get the virtual base assigned to twice, but copy assignments should be safe to do twice, which moves are not necessarily. – Jonathan Wakely Jan 07 '16 at 12:03