Fixup for 9975: swap and pointsTo

[monarchdodra]

Fixing 9975 (http://d.puremagic.com/issues/show_bug.cgi?id=9975) revealed that checking pointsTo was creating problems in swap for legitimate use cases.

This is because:

1. pointsTo can sometimes create "false positives"
2. Internal pointing is not outright forbidden, it is just that the runtime is allowed to make the assumption they don't exist.

While swapping aliasing objects is a sign of stink, it should still "just work".

[DmitryOlshansky]

Internal pointing is not outright forbidden

They are by spec. The whole idea of structs is to have them bit-copyable (or movable).
Postblit may not be called in certain cases so there is simply no way for Internal pointing structs to not cause a segfault down the road.

I'm not sure who/what and when should check that though.

[monarchdodra]

Yes, structs should be bit moveable. However, the fact that they may have an internal pointer is not guaranteed wrong code. For example, a struct might just so happen to reference an array, and be a part of that same array.

Also, power coder can rely on moving things with internal pointers. It is a way, for example, to implement node insertion by intializing the sentinel node with internal pointing, then moving* the sentinel into the target node, while keeping valid pointers to the sentinel.

More problematically though, pointsTo, is currently returning false positives (in case of unions), which means we can't make assertions (just in case).

So while I agree with you that internal pointers will usually lead to pain and suffering, I don't think it is swap's job to halt a program because it detects a potentially dangerous situation.

*Just noticed move has the same problem, which also needs fixing.

[monarchdodra]

Rebased. Could I get a second opinion on this?

There these asserts are creating problems:
http://d.puremagic.com/issues/show_bug.cgi?id=9975
http://d.puremagic.com/issues/show_bug.cgi?id=10690
http://d.puremagic.com/issues/show_bug.cgi?id=10859

[DmitryOlshansky]

2/3 of these report look like playing with raw memory in a wrong way but the first one is clean-cut bug.
I'd say go for it.

[monarchdodra]

I'd say go for it.

I would, but I submitted this, so I can't. Woe is me.

[dnadlinger]

@andralex: IIRC, this is your design, so do you have any comments on the issue?

Imho, it's pretty clear that alias analysis can't work in a binary way in an environment that allows for bitwise data reinterpretation (for example, but not limited to, using unions), but I'm not sure whether introducing two mayPointTo/certainlyPointsTo (for the lack of a better name) variants is worth the added complexity.

In assertions such as the ones discussed here certainlyPointsTo would be used since the runtime is free to assume that objects actually don't contain any inner pointers anyway. On the other hand, functions that need to pick a different, conservative approach in case there might be any aliasing would use mayPointTo.

[monarchdodra]

One problem we have is that we don't know (or at least I) don't know how to write certainlyPointsTo. When a struct contains an anonymous union, there is no way to detect it. Ideally certainlyPointsTo would ignore members in a union, so... that'd be a problem. A certainlyPointsTo would still choke on the original test case (unless you can point me to an implementation that can iterate on members, while skipping those inside unions).

emplace was recently fixed, and the remaining problems in Appender are about to get fixed too, which solves most of our current error cases that failed in swap, due to previous memory corruption.

Still, as evidenced in the original error report, it is currently not possible to sort JSon entries, as these can't be swapped. This is a problem.

I think the best course of action is the implementation of a certainlyPointsTo, but as mentioned above, I'd need help in deploying that.

[DmitryOlshansky]

Re-ping? There are some signs of broken merge.

[monarchdodra]

Re-ping? There are some signs of broken merge.

Fixed.

[DmitryOlshansky]

I going to merge, it fixes an issue with no other apparent workarounds.
Standard library primitives are hardly the right place for testing undue aliasing in structs/unions.

[andralex]

I don't agree with this solution that disables all checking for the sake of union. The principled solution is to insert the check and make it such that it conservatively passes when unions are involved. @blackwhale since you merged you just volunteered :o).

[monarchdodra]

The principled solution is to insert the check and make it such that it conservatively passes when unions are involved.

A "definitely points to"? When I submitted the pull, I didn't know how I'd write such a thing. However, using Kenji's "isMutable" template re-write that handles unions, I think I have a better idea on how to handle it. I'll give it a try.

[monarchdodra]

I've formalized the change I'm thinking of into an ER. I'll get on it in a couple of weeks, when I get the time.

[monarchdodra]

I don't agree with this solution that disables all checking for the sake of union. The principled solution is to insert the check and make it such that it conservatively passes when unions are involved. @blackwhale since you merged you just volunteered :o).

@andralex : #2167