non-mutating func that still mutates a struct, compiler not aware


(Raphael Sebbe) #1

In the example below, solve is a non-mutating func within struct type
Matrix, that uses an Array<T> to store its values.

This function mutates self, even though it's declared as non-mutating.

My understanding is that the compiler doesn't make a real copy in the acopy
= self instruction, and then provides that contents to the mx_gels_
function which modifies the memory contents.

public func solve(rhs b: Matrix<T>) -> Matrix<T>? {

// ...

var acopy = self

// ...

T.mx_gels_(&trans, &m, &n, &nrhs, UnsafeMutablePointer<T>(acopy.values),
&lda, UnsafeMutablePointer<T>(x.values), &ldb,
UnsafeMutablePointer<T>(workspace),
&lwork, &status);

// ...

}

Is this expected? I mean, I can force a real copy of course, but value
semantics would suggest the code above is correct and wouldn't need that.
Shouldn't the cast trigger the copy somehow? Or is there a better way of
expressing this operation? Thx.

Raphael


(Brent Royal-Gordon) #2

The `acopy = self` line only copies the reference to the internal buffer. However, converting the array into a pointer will—or at least, if done properly, *should*—force the array to create and switch to using a unique copy of its buffer in preparation for writes through the UnsafeMutablePointer. I believe that happens here: <https://github.com/apple/swift/blob/master/stdlib/public/core/Pointer.swift#L79>

(I say "should" because I'm not sure you're actually creating those pointers correctly. I believe you ought to be able to just say `&acopy.values` and `&x.values`, and that should be a more reliable way to do it.)

···

On Aug 4, 2016, at 1:25 AM, Raphael Sebbe via swift-users <swift-users@swift.org> wrote:

My understanding is that the compiler doesn't make a real copy in the acopy = self instruction, and then provides that contents to the mx_gels_ function which modifies the memory contents.

public func solve(rhs b: Matrix<T>) -> Matrix<T>? {
  // ...
  var acopy = self
  // ...

  T.mx_gels_(&trans, &m, &n, &nrhs, UnsafeMutablePointer<T>(acopy.values), &lda, UnsafeMutablePointer<T>(x.values), &ldb, UnsafeMutablePointer<T>(workspace), &lwork, &status);
    
  // ...
  }

Is this expected? I mean, I can force a real copy of course, but value semantics would suggest the code above is correct and wouldn't need that. Shouldn't the cast trigger the copy somehow? Or is there a better way of expressing this operation? Thx.

--
Brent Royal-Gordon
Architechies


(Raphael Sebbe) #3

Do you see a reason why the copy isn't happening in this specific case? Is
it a bug, or a mistake in my code?

Thank you,

Raphael

···

On Thu, Aug 4, 2016 at 4:18 PM Brent Royal-Gordon <brent@architechies.com> wrote:

> On Aug 4, 2016, at 1:25 AM, Raphael Sebbe via swift-users < > swift-users@swift.org> wrote:
>
> My understanding is that the compiler doesn't make a real copy in the
acopy = self instruction, and then provides that contents to the mx_gels_
function which modifies the memory contents.
>
>
> public func solve(rhs b: Matrix<T>) -> Matrix<T>? {
> // ...
> var acopy = self
> // ...
>
> T.mx_gels_(&trans, &m, &n, &nrhs,
UnsafeMutablePointer<T>(acopy.values), &lda,
UnsafeMutablePointer<T>(x.values), &ldb,
UnsafeMutablePointer<T>(workspace), &lwork, &status);
>
> // ...
> }
>
>
> Is this expected? I mean, I can force a real copy of course, but value
semantics would suggest the code above is correct and wouldn't need that.
Shouldn't the cast trigger the copy somehow? Or is there a better way of
expressing this operation? Thx.

The `acopy = self` line only copies the reference to the internal buffer.
However, converting the array into a pointer will—or at least, if done
properly, *should*—force the array to create and switch to using a unique
copy of its buffer in preparation for writes through the
UnsafeMutablePointer. I believe that happens here: <
https://github.com/apple/swift/blob/master/stdlib/public/core/Pointer.swift#L79
>

(I say "should" because I'm not sure you're actually creating those
pointers correctly. I believe you ought to be able to just say
`&acopy.values` and `&x.values`, and that should be a more reliable way to
do it.)

--
Brent Royal-Gordon
Architechies


(Jordan Rose) #4

Those UnsafeMutablePointer<T>(…) calls aren’t correct. When an array is implicitly converted to a pointer in Swift, the pointer is only valid for the immediate call that it’s being passed to. In this case, that’s the UnsafeMutablePointer initializer, and after that the pointer is invalid. So it’s possible that either the same space is being reused for both pointers, or that it’s not uniquing the array because it thinks it only has to make an UnsafePointer<T>, or both.

As Brent says, passing the arrays directly to the method using inout semantics (&) should get you the behavior you want.

(Yes, there should be a diagnostic for this. I’m pretty sure we have a bug already, so no need to file. The Swift 3 pointer APIs make this a little harder to do by accident, too.)

Jordan

···

On Aug 5, 2016, at 08:14, Raphael Sebbe via swift-users <swift-users@swift.org> wrote:

Do you see a reason why the copy isn't happening in this specific case? Is it a bug, or a mistake in my code?

Thank you,

Raphael

On Thu, Aug 4, 2016 at 4:18 PM Brent Royal-Gordon <brent@architechies.com <mailto:brent@architechies.com>> wrote:
> On Aug 4, 2016, at 1:25 AM, Raphael Sebbe via swift-users <swift-users@swift.org <mailto:swift-users@swift.org>> wrote:
>
> My understanding is that the compiler doesn't make a real copy in the acopy = self instruction, and then provides that contents to the mx_gels_ function which modifies the memory contents.
>
>
> public func solve(rhs b: Matrix<T>) -> Matrix<T>? {
> // ...
> var acopy = self
> // ...
>
> T.mx_gels_(&trans, &m, &n, &nrhs, UnsafeMutablePointer<T>(acopy.values), &lda, UnsafeMutablePointer<T>(x.values), &ldb, UnsafeMutablePointer<T>(workspace), &lwork, &status);
>
> // ...
> }
>
>
> Is this expected? I mean, I can force a real copy of course, but value semantics would suggest the code above is correct and wouldn't need that. Shouldn't the cast trigger the copy somehow? Or is there a better way of expressing this operation? Thx.

The `acopy = self` line only copies the reference to the internal buffer. However, converting the array into a pointer will—or at least, if done properly, *should*—force the array to create and switch to using a unique copy of its buffer in preparation for writes through the UnsafeMutablePointer. I believe that happens here: <https://github.com/apple/swift/blob/master/stdlib/public/core/Pointer.swift#L79>

(I say "should" because I'm not sure you're actually creating those pointers correctly. I believe you ought to be able to just say `&acopy.values` and `&x.values`, and that should be a more reliable way to do it.)

--
Brent Royal-Gordon
Architechies

_______________________________________________
swift-users mailing list
swift-users@swift.org
https://lists.swift.org/mailman/listinfo/swift-users


(Raphael Sebbe) #5

Hi Jordan,

this is Swift 3 code btw, no warning / error.

you mention the same space being reused. It's self data that's being
modified when mx_gels() returns, still within solve(). self storage isn't
deallocated at that time, so that is not possible or I'm misunderstanding
something.

For the second reason you provide, I don't understand, could you please
elaborate?

I'm not against changing the code to pass inout instead, but if those
really are programming errors and are not obvious neither to me, nor to the
compiler, these errors will happen again.

Raphael

···

On Fri, Aug 5, 2016 at 6:58 PM Jordan Rose <jordan_rose@apple.com> wrote:

Those UnsafeMutablePointer<T>(…) calls aren’t correct. When an array is
implicitly converted to a pointer in Swift, the pointer is only valid for
the *immediate* call that it’s being passed to. In this case, that’s the
UnsafeMutablePointer initializer, and after that the pointer is invalid. So
it’s possible that either the same space is being reused for both pointers,
or that it’s *not* uniquing the array because it thinks it only has to
make an UnsafePointer<T>, or both.

As Brent says, passing the arrays directly to the method using inout
semantics (&) should get you the behavior you want.

(Yes, there should be a diagnostic for this. I’m pretty sure we have a bug
already, so no need to file. The Swift 3 pointer APIs make this a little
harder to do by accident, too.)

Jordan

On Aug 5, 2016, at 08:14, Raphael Sebbe via swift-users < > swift-users@swift.org> wrote:

Do you see a reason why the copy isn't happening in this specific case? Is
it a bug, or a mistake in my code?

Thank you,

Raphael

On Thu, Aug 4, 2016 at 4:18 PM Brent Royal-Gordon <brent@architechies.com> > wrote:

> On Aug 4, 2016, at 1:25 AM, Raphael Sebbe via swift-users < >> swift-users@swift.org> wrote:
>
> My understanding is that the compiler doesn't make a real copy in the
acopy = self instruction, and then provides that contents to the mx_gels_
function which modifies the memory contents.
>
>
> public func solve(rhs b: Matrix<T>) -> Matrix<T>? {
> // ...
> var acopy = self
> // ...
>
> T.mx <http://t.mx>_gels_(&trans, &m, &n, &nrhs,
UnsafeMutablePointer<T>(acopy.values), &lda,
UnsafeMutablePointer<T>(x.values), &ldb,
UnsafeMutablePointer<T>(workspace), &lwork, &status);
>
> // ...
> }
>
>
> Is this expected? I mean, I can force a real copy of course, but value
semantics would suggest the code above is correct and wouldn't need that.
Shouldn't the cast trigger the copy somehow? Or is there a better way of
expressing this operation? Thx.

The `acopy = self` line only copies the reference to the internal buffer.
However, converting the array into a pointer will—or at least, if done
properly, *should*—force the array to create and switch to using a unique
copy of its buffer in preparation for writes through the
UnsafeMutablePointer. I believe that happens here: <
https://github.com/apple/swift/blob/master/stdlib/public/core/Pointer.swift#L79
>

(I say "should" because I'm not sure you're actually creating those
pointers correctly. I believe you ought to be able to just say
`&acopy.values` and `&x.values`, and that should be a more reliable way to
do it.)

--
Brent Royal-Gordon
Architechies

_______________________________________________

swift-users mailing list
swift-users@swift.org
https://lists.swift.org/mailman/listinfo/swift-users


(Dave Abrahams) #6

If you want to be safe in the general case, use

  a.withUnsafe[Mutable]BufferPointer { let p = $0.baseAddress; ... }

which will force the pointer to remain valid through the execution of
the entire closure. Yes, it's syntactically a *lot* heavier than just
passing &a, but it doesn't break without warning when some maintainer
comes along and passes &a through a function.

We ought to prevent the &a -> UnsafeMutablePointer conversion trick from
working except when calling imported C APIs directly.

···

on Sat Aug 06 2016, Raphael Sebbe <swift-users-AT-swift.org> wrote:

Those UnsafeMutablePointer<T>(…) calls aren’t correct. When an array is
implicitly converted to a pointer in Swift, the pointer is only valid for
the *immediate* call that it’s being passed to. In this case, that’s the
UnsafeMutablePointer initializer, and after that the pointer is invalid. So
it’s possible that either the same space is being reused for both pointers,
or that it’s *not* uniquing the array because it thinks it only has to
make an UnsafePointer<T>, or both.

As Brent says, passing the arrays directly to the method using inout
semantics (&) should get you the behavior you want.

(Yes, there should be a diagnostic for this. I’m pretty sure we have a bug
already, so no need to file. The Swift 3 pointer APIs make this a little
harder to do by accident, too.)

--
-Dave