internal pointers in C++ classes cause bugs with D struct move semantics on calls by value

[timotheecour]

eg: return by value cv::Mat causes crash on ~Mat

BUG:D20180110T195539
This seems to cause serious bugs when using real-world libraries, eg opencv's cv::Mat which has a field step of type MatStep with:

// from https://github.com/opencv/opencv/blob/57dc28fe99737a088a7dbb41a234269f5711a50a/modules/core/include/opencv2/core/mat.hpp#L569
struct MatStep
 {
    // ...
     size_t* p; // often points to &buf[0]
     size_t buf[2];
 protected:
     MatStep& operator = (const MatStep&);
 };

I suspect internal pointers causes any code that returns by value an opencv cv::Mat to crash when the ~Mat is called (inside fastFree function which calls free):

cv::Mat test_mat(){
  return cv::Mat::zeros(3, 3, CV_32F);
}
// main.d:
void main(){
  test_mat(); // crashes: main(91658,0x7fff8cec7340) malloc: *** error for object 0x7fc96d412250: pointer being freed was not allocated
}

Indeed, with debugging, inside the ~Mat, step.p == step.buf becomes false even though it should be true (and was true while inside C++ test_mat)

simplified case (may or may not be a reduction of above case)

struct B{
	int x;
	int*y;

	// rule of 5:  https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)
	B(const B &a) : B(a.x) {}

	B(B &&a) : B(a.x) {
          release(a);
      }

  B& operator= (const B &a) {
		initialize(a.x);
		return *this;
  }

  B& operator= (B&& a) {
      if (this == &a) return *this;
      initialize(a.x);
     release(a);
      return *this;
  }

	B(int x){ initialize(x); }

	~B(){ }

  void release(){
    // could release owned resources here
  }

  void initialize(int x){
		this->x=x;
		this->y=&this->x;
		test();
  }

	void test(){
		std::cout<<x<<" "<<y<< " "<< &x << " " << (&x==y) << std::endl;
		assert(&x==y);
	}
};

// main.d:
void main(){
    auto a=B(10);
    void fun1(B x){
        x.test;  // BUG:D20180111T170809 assert &x==y fails
    }
   auto a2=a;// works
   a2=a; // works
   fun1(a); // doesn't work, as it (probably) calls C++ move ctor, then moves the data inside fun1's stack frame (via memcpy)
}

related issues (not necessarily involving internal pointers but involving move semantics instead of calling copy ctor

• another bug: an empty C++ ctor is needed for calypso when no move/copy ctor is provided ; even though not needed in C++, giving this error:
  Error: constructor ℂcpp.tims.B.B.this (int x) is not callable using argument types ()
  BUG:D20180111T124115

• if C++ provides a move ctor, and if that move ctor releases resources (cf function resource above), the following code would cause a bug:
  BUG:D20180111T170655

// main.d:
void main(){
auto a1=B(10);
// this would call `release` on `a` by calling move ctor instead of copy ctor even though `a1` should be kept alive
auto a2=a1;
}

discussion of potential fix

how about calling copy ctor instead of move ctor (as would be the case in C++) in these cases:

auto a2=a1; // call copy ctor, not move ctor
fun1(a); // call copy ctor, not move ctor
a2=a1; // call assignment operator=, not move assignment operator
auto a2=B(1);  // call copy ctor, not `ctor=>move ctor=>delete 1st object`

this behavior (special handling of move semantics) would hold for regular D structs that embed a D/C++ struct by value , eg:

struct B2{
   B a; // embedded D/C++ struct
}
// (eg, with a tag on corresponding TypeInfo to identify whether we need to treat the struct specially for move semantics)

[Syniurge]

6eec174 should help with cv::Mat but doesn't fix the OP test case though. It does appear to be caused by an unwanted memcpy, looking into it.

[Syniurge]

This is simply due to a load actually:

postinvoke10:                                     ; preds = %postinvoke6
  %.loadFromMemory_bitCastAddress = bitcast %struct.B* %__slB57 to { i64, i8* }* ; [#uses = 1]
  %.X86_64_C_struct_rewrite_putResult = load { i64, i8* }, { i64, i8* }* %.loadFromMemory_bitCastAddress ; [#uses = 1]
  invoke void @"_D17copy_ctor_issue704fun1FS6\E2\84\82cpp1B1BZv"({ i64, i8* } %.X86_64_C_struct_rewrite_putResult) #0
          to label %postinvoke12 unwind label %landingPad11

B shouldn't be passed by value, as explained by Clang:

ABIArgInfo DefaultABIInfo::classifyArgumentType(QualType Ty) const {
  Ty = useFirstFieldIfTransparentUnion(Ty);

  if (isAggregateTypeForABI(Ty)) {
    // Records with non-trivial destructors/copy-constructors should not be
    // passed by value.
    if (CGCXXABI::RecordArgABI RAA = getRecordArgABI(Ty, getCXXABI()))
      return getNaturalAlignIndirect(Ty, RAA == CGCXXABI::RAA_DirectInMemory);

    return getNaturalAlignIndirect(Ty);
  }

[Syniurge]

This should be fixed by affee89!

[timotheecour]

Internal a;
fun_value(a);  // uses move ctor => BUG
a.fail_if_a_was_moved; // fails

likewise with these cases:

Internal a2;
a2=a; // uses move ctor => BUG

Internal a2=a;  // uses move ctor => BUG
auto a2=a;  // uses move ctor => BUG```

[Syniurge]

Although the test case worked, the wrong ctor was called (move instead of copy), so cv::Mat was still not behaving properly.

Hopefully this is now definitely fixed by 273e521.

[timotheecour]

reopening: return by value is broken (causing crashes for the simplest opencv program):

struct Internal2{
    int x;
    int*y;
    Internal2(){
        y=&x;
        std::cout << "ctor " << (y==&x) << std::endl;
    }
    ~Internal2(){
        std::cout << "dtor " << (y==&x) << std::endl;
    }
    static Internal2 get(){
        return Internal2();
    }
};

// main.d:
modmap (C++) "test3.h";
import (C++) _.Internal2;
void main(){
Internal2.get;
}

prints:

ctor 1
dtor 0 // BUG!! should be `1`

[timotheecour]

should be fixed once https://github.com/Syniurge/Calypso/tree/avoid-memcpy-cpp-record-returnvalue is merged