C++ template member initialization: move-construction with rvalue but reference with lvalue

Question

The constructor of my class initializes reference members. How can I make it safe to pass temporary objects for the initialization of those reference members?

I have got a class of Objects and classes of operations performed on those objects. The operations are all derived from an abstract base class that provides the common interface. Otherwise, the operations may contain considerable additional information for the specific implementation.

class Object;

class Operation {
  public:
  virtual void do_something( Object& obj ) = 0;
  /* ... */
}

class FooOperation : public Operation { /*...*/ }
class BarOperation : public Operation { /*...*/ }
class BazOperation : public Operation { /*...*/ }

I also want to compose operations. One correct way of doing that is the following class:

class ComposedOperation : Operation {

  private:

    const Operation& m_op1; const Operation& m_op2;

  public:

    ComposedOperation( const Operation& op1, const Operation& op2 )
    : m_op1(op1), m_op2( op2 ) { };

    void do_something( Object& obj ) override 
    { m_op1.do_something(obj); m_op2.do_something(obj); }
}

The problem is that the composed operations can only bind lvalue references. A composition like the following would be dangerous:

auto op = ComposedOperation( BarOperation(), ComposedOperation( FooOperation(), BazOperation() ) );

How can I make the class aware whether the first/second Operation is an lvalue or rvalue? Preferably without overloading the constructor. I wonder whether templates could help here.

You could determine rvalue vs lvalue using https://stackoverflow.com/q/36296425/4253931 and use that information in a `if constexpr`. Why is it, that you want to avoid overloading the constructor? — eike, Sep 16 '19 at 21:12

user673679 · Answer 1 · 2019-09-19T09:29:59.710

I think there are two options:

Store all the operations somewhere separate from the tree and ensure that they live longer than the references to them.
Have each Operation own its children and store them either by value or in unique_ptrs.

Option 1 might be useful in some situations (if you have a registry of Operations somewhere, and the Operation tree is simply an organisational thing). However, from your usage example this doesn't seem to be the case.

Option 2 is definitely simpler, and implementing move semantics should avoid any performance problems.

If you really need to avoid a copy / move in a particular instance, you could create a Ref wrapper that stores an Operation by reference, and implements the Operation interface itself. You would obviously need to ensure the correct lifetime yourself when using it.

So (approximately):

template<class A, class B>
struct FooOperation {
    A a; B b;

    // ... implement operation concept
};

template<class T>
struct Ref {
    T& t;

    // ... implement operation concept
};

auto a = BarOperation();
auto b = FooOperation<Ref<A>, B>(Ref(a), BazOperation());

Or if we need the run-time polymorphism:

struct FooOperation : Operation {
    std::unique_ptr<Operation> a, b;

    // ... implement operation interface
};

struct Ptr : Operation {
    Operation* t;

    // ... implement operation interface
};

auto a = std::make_unique<BarOperation>();
auto b = FooOperation(std::make_unique<Ptr>(a), std::make_unique<BazOperation>());

julien · Answer 2 · 2019-09-19T12:52:39.917

I might be dumb but I don't see why your

auto op = ComposedOperation( BarOperation(), ComposedOperation( FooOperation(), BazOperation() ) );

would be "dangerous", and why you would need to distinguish between lvalue and rvalue.

Maybe you're speaking about old C++ versions (pre C++11 I think) in which the rvalue lifetime wasn't extended to match the scope of the reference ?

Edit: I typed your code and tried to compile it, with a few minor modifications such as adding consts where they are needed and making the inheritance public.

class Object {
  public:
  int i;
};

class Operation {
  public:
  virtual void do_something( Object& obj ) const = 0;
  /* ... */
};

class FooOperation : public Operation { void do_something( Object& obj )const{obj.i++;} };
class BarOperation : public Operation { void do_something( Object& obj )const{obj.i+=2;} };
class BazOperation : public Operation { void do_something( Object& obj )const{obj.i+=4;} };

class ComposedOperation : public Operation {

  private:

    const Operation& m_op1; const Operation& m_op2;

  public:

    ComposedOperation( const Operation& op1, const Operation& op2 )
    : m_op1(op1), m_op2( op2 ) { };

    void do_something( Object& obj ) const override
    { m_op1.do_something(obj); m_op2.do_something(obj); }
};

int main(){
  Object obj;
  auto op = ComposedOperation( BarOperation(), ComposedOperation( FooOperation(), BazOperation() ) );
  op.do_something(obj);
  return 0;
}

This compiles without any issue when using g++ (8.3.0) without any flag. If I force it to use c++98 (or anything older than c++11/c++0x) I have to get rid of the auto in main and replace it by ComposedOperation, and then even in c++98 it compiles. Even with -pedantic and -Wall I don't have any warning. I can also run the binary, it exits cleanly.

Edit2: the error message I get with -fsanitize:

julien@jeannot:~/tmp$ g++ -std=c++17 -g -pedantic -fsanitize=address test.cpp
julien@jeannot:~/tmp$ ./a.out
=================================================================
==30438==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7ffdd41d8720 at pc 0x561189efd8de bp 0x7ffdd41d8690 sp 0x7ffdd41d8688
READ of size 8 at 0x7ffdd41d8720 thread T0
    #0 0x561189efd8dd in ComposedOperation::do_something(Object&) const /home/julien/tmp/test.cpp:30
    #1 0x561189efd4ac in main /home/julien/tmp/test.cpp:36
    #2 0x7f3f97e4509a in __libc_start_main ../csu/libc-start.c:308
    #3 0x561189efd159 in _start (/home/julien/tmp/a.out+0x1159)

Address 0x7ffdd41d8720 is located in stack of thread T0 at offset 96 in frame
    #0 0x561189efd224 in main /home/julien/tmp/test.cpp:33

  This frame has 6 object(s):
    [32, 36) 'obj'
    [96, 104) '<unknown>' <== Memory access at offset 96 is inside this variable
    [160, 168) '<unknown>'
    [224, 232) '<unknown>'
    [288, 312) 'op'
    [352, 376) '<unknown>'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope /home/julien/tmp/test.cpp:30 in ComposedOperation::do_something(Object&) const
Shadow bytes around the buggy address:
  0x10003a833090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10003a8330a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10003a8330b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10003a8330c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10003a8330d0: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 04 f2 f2 f2
=>0x10003a8330e0: f2 f2 f2 f2[f8]f2 f2 f2 f2 f2 f2 f2 f8 f2 f2 f2
  0x10003a8330f0: f2 f2 f2 f2 f8 f2 f2 f2 f2 f2 f2 f2 00 00 00 f2
  0x10003a833100: f2 f2 f2 f2 f8 f8 f8 f2 f3 f3 f3 f3 00 00 00 00
  0x10003a833110: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10003a833120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10003a833130: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==30438==ABORTING

I have been playing with different variants while adding some cout in the constructors/destructors and the do_something. It appears that the temporaries get destructed at the end of the line but before the do_something line. If I put everything into a single line

ComposedOperation( BarOperation(), ComposedOperation( FooOperation(), BazOperation() ) ).do_something(obj);

Then things get extended far enough and I get the correct order:

julien@jeannot:~/tmp$ ./a.out 
Operation construction
Operation construction
Operation construction
ComposedOperation construction
Operation construction
Operation construction
ComposedOperation construction
ComposedOperation doing something
ComposedOperation doing something
ComposedOperation destruction
Operation destruction
Operation destruction
ComposedOperation destruction
Operation destruction
Operation destruction
Operation destruction
7

So the correct interpretation should be that references to temporaries only allow extension to the scope of the full-expression, not the scope of the object.

What is a bit confusing is that with

const Operation& foo = FooOperation();
const Operation& baz = BazOperation();
const ComposedOperation& tmp = ComposedOperation( foo, baz );
const Operation& bar = BarOperation();
ComposedOperation op = ComposedOperation( bar, tmp );

you do get an extension of the lifetime of the temporary objects, until the end of main().

So, as a conclusion I believe, here is what I suggest: replace your references by unique_ptr. Then you can easily use a clone function and make sure that you don't get dangling references:

#include <iostream>
#include <memory>

class Object {
  public:
  int i=0;
};

class Operation {
  public:
  virtual void do_something( Object& obj ) const = 0;
  /* ... */
  Operation() { std::cout << "Operation construction\n"; };
  std::unique_ptr<Operation> clone() const {
    return std::unique_ptr<Operation>(clone_impl());
  };
  virtual Operation* clone_impl() const = 0;
};

class FooOperation : public Operation { FooOperation* clone_impl() const {return new FooOperation();} void do_something( Object& obj )const{obj.i++;} };
class BarOperation : public Operation { BarOperation* clone_impl() const {return new BarOperation();}; void do_something( Object& obj )const{obj.i+=2;} };
class BazOperation : public Operation { BazOperation* clone_impl() const {return new BazOperation();} void do_something( Object& obj )const{obj.i+=4;} };

class ComposedOperation : public Operation {

  private:

    std::unique_ptr<Operation> m_op1; std::unique_ptr<Operation> m_op2;

  public:

    ComposedOperation( const Operation& op1, const Operation& op2 )
    : m_op1(op1.clone()), m_op2(op2.clone()) { std::cout << "ComposedOperation construction\n"; };

    ComposedOperation* clone_impl() const {return new ComposedOperation(*m_op1->clone(),*m_op2->clone());}

    void do_something( Object& obj ) const override
    { std::cout<<"ComposedOperation doing something\n"; m_op1->do_something(obj); m_op2->do_something(obj); }
};

int main(){
  Object obj;
  const ComposedOperation& op = ComposedOperation( BarOperation(), ComposedOperation( FooOperation(), BazOperation() ) );
  op.do_something(obj);
  std::cout << obj.i << std::endl;
  return 0;
}

If you want at the same time to be able to avoid copies when passing a simple lvalue which you know will live long enough, you can do this with by overloading the ComposedOperation constructor like:

ComposedOperation( const Operation& op1, const Operation& op2 )
: m_op1(op1.clone()), m_op2(op2.clone()) { };
ComposedOperation( const Operation&& op1, const Operation&& op2 )
: m_op1(op1.move()), m_op2(op2.move()) { };

where you should have something like

FooOperation* move_impl() const {return new FooOperation(std::move(*this));}

I thought it was only valid in C++11 and newer, but my test suggests that it works even in the oldest versions. Except if you disagree with some of the small modifications I had to do to make it compile. — julien, Sep 17 '19 at 17:47
Thanks. However, I get a segmentation fault, compiling it with `g++ -std=c++17 -g -pedantic` and gcc 8.3, so I presume your example is not portable. Valgrind complains about the last do_something line. — shuhalo, Sep 17 '19 at 19:32
If I add `std::cout << obj.i << std::endl;` after this last do_something (and initialise i in Object() ) I do get 7 as expected. Running directly or under gdb or under valgrind (with no leak detected) doesn't change anything. Could you copy-paste the exact valgrind complain? And add a gdb backtrace? In case it matters, I tested it on a Debian Buster with a Linux 4.19.0-5-amd64. — julien, Sep 18 '19 at 08:48
What happens on your machine if you compile with ` -fsanitize=address`? — shuhalo, Sep 18 '19 at 08:55

C++ template member initialization: move-construction with rvalue but reference with lvalue

2 Answers2