Fix functions marked 'DANGEROUS' in irep

[reuk]

This patch introduces a class which abstracts copy-on-write behaviour. This class allows the user to specify whether write should make the object non-shareable. This modified write method makes it safe to give away references to internal fields of the irep. (The technique is discussed here, though the approach has been modified to re-use the small intrusive shared pointer from #786.)

The observable effect of this patch is that code such as the following no longer has unexpected behaviour.

irept a;
auto& comments = a.get_comments();
const auto b(a);
comments.<some mutating method>();
// prior to this patch, b would be modified here
// with the patch applied, b is unaffected

Other benefits of this patch are that it uses RAII properly (all allocation and freeing takes place in constructors and destructors respectively), and that it separates the details of resource management into simple, reusable (unit-testable) classes.

As a side effect of this change, I discovered a bug in ansi-c/padding where the insert on line 262 invalidates the iterators in the vector, which had the possibility of making the reference on line 206 dangle, leading to undefined behaviour on line 269 when the reference was passed to a function. Taking a copy instead of a reference fixes the bug.

[tautschnig]

Overall this now looks good to me. Yet, as said before, those changes need a proper performance test to be in place. Thus it would be great to factor out the bug fix of the padding code into a separate pull request as that should be merged as soon as possible.

[reuk]

The fix for the padding bug is now in #836.

Regarding the performance test: This patch fixes a bug in irep's copy-on-write implementation, and improves the modularity and readability of the code, while keeping the memory usage the same as the existing irep. I'd argue that, even if this change is slightly slower, it's still worthwhile in terms of maintainability and avoiding programmer error. I understand that performance is important, but it should not come at the expense of correctness.

[tautschnig]

I am not in a position to make the ultimate call, but I'd refuse a change to this core data structure without proper performance testing. Indeed the performance might even improve here - but "slightly slower" would be a major difference when talking about 10^10 calls, which we see even on small benchmarks.

This is no criticism of any sorts, and indeed I highly appreciate this work and the improvements it is bringing about. As this isn't a beauty contest, we need data, not high judgement.

[tautschnig]

@reuk Could you please rebase this one? It should be a trivial rebase, but I'd like to run a decent performance comparison against current develop. An initial run with the current state looks good, but I'd like to be clinical here.

[reuk]

@tautschnig This is rebased now

[tautschnig]

I have run the performance comparison and would estimate the overhead (if any) at 1%, which seems acceptable to me. I still need to properly re-read the code, it's been a while.

[tautschnig]

(And of course the Travis failures are worrying ...)

[reuk]

The segfaults appear to be because ireps are now shared more conservatively, and copies are made more frequently. The first segfault I found looks like this:

  exprt &op0=code.op0().op0();

  op0.operands().push_back(exprt("cpp-this"));
  op0.type()=
    pointer_type(cpp_namet(parent_base_name, source_location).as_type());
  op0.add_source_location()=source_location;

  code.operands().push_back(exprt("explicit-typecast"));
  exprt &op1=code.op1();

  op0.type()=
    pointer_type(cpp_namet(parent_base_name, source_location).as_type());

We take a reference into a vector op0, then call push_back on the vector, and finally try to write to the initial object. This shouldn't work for any type with value semantics (we shouldn't depend on references staying valid after calling push_back), so it's not surprising that it breaks here. However, fixing all of these segfaults may be kind of time-consuming.

[reuk]

I've got the go-ahead from @pkesseli to spend half a day on this. I'll try to fix as many of the segfaults as I can, after which I may have to delegate this work to someone else.

[reuk]

Looks like all the segfaults have the same cause... which was a copy-paste bug I introduced a few months ago while I was 'fixing' code to do with setting pointer_typet invariants. Sorry about that.

[reuk]

@tautschnig This passes CI now, could you take another look please?

[reuk]

@tautschnig this should be ready for re-review now

[tautschnig]

Looks good to me. Assigning to @kroening for a final opinion.

[kroening]

Squash 286e5765 into c2b1b00?

[reuk]

@kroening done

[tautschnig]

@reuk Could you please drop 20692b0 so that this can be merged?

[reuk]

This should be mergeable now

[reuk]

Sorry, I think an edit to the second commit got lost somewhere. Hold off on this for now

[smowton]

@reuk are you still attending to these, or should someone take it over?

[reuk]

Yes, someone should take ownership of this. In the second commit, the method call should be on op1 instead of op0 - I introduced a typo a while ago (check git blame on that file). Other than that, it's ready to go.

[tautschnig]

I have created #1704 to selectively fix the typo. @reuk, would it be feasible (given you might have totally different commitments these days) for you to just drop the second commit from this PR? I would seemingly be the most efficient approach.

[owen-mc-diffblue]

I asked @reuk about this yesterday and he said it would be better if other people took over his branches. I've made #1742 which is a rebased copy of this PR with the commit that @tautschnig mentioned dropped.

[tautschnig]

@owen-jones-diffblue, @reuk Thank you!!