Fix Issue 13195 - Delete calls destructor but doesn't free

[Orvid]

This is the accompanying PR (which this requires) to dlang/druntime#864, which includes the fix to the delete operator so that it will free the memory of pointers to structures that only have a destructor defined. Due to the overlap in required code between my fix for the delete issue and the fix to call destructors on structs, it would be very difficult to make them 2 separate PRs, so I have instead opted to stay with 1 PR which fixes both.

Originally this PR was part of fixing the GC, but I did finally separate the druntime code required for this PR so that it can be done independently of the main PR.

[rainers]

I think you should extract the code from the druntime PR for _d_delstruct (without the exception handling) into a separate PR, so it can be merged independently from the GC changes.

[rainers]

Doesn't the fall through free the memory twice (if _d_delstruct does it, too)?

[rainers]

Doesn't the fall through free the memory twice (if _d_delstruct does it, too)?

Ignore that comment, I realized that this only adds an argument to the call to _d_delstruct.

[Orvid]

I would extract the code for that into a separate PR, except that it uses rt_finalize_struct to do the actual destroying of the struct. I also will need to add a change to the DMD test suite so that it doesn't try to allocate in the destructors as part of the test suite to this PR, otherwise the test will fail even with the other PR...

[rainers]

I would extract the code for that into a separate PR, except that it uses rt_finalize_struc

Isn't it just about these two lines?

if (inf.xdtor)
 inf.xdtor(p); // call destructor

I.e. it should not translate exceptions to finalize errors for an explicitely called destructor. Also, allocation in the destructor should be fine.

These might have to change slightly if the GC changes are merged, though.

[Orvid]

I also have to clear the finalize attribute from the allocation so that it doesn't get double finalized. I think it would however be possible to separate the code required for this PR out so that it can be merged separately, which I will go ahead and do, as it will mean that all 3 PRs end up passing.

[Orvid]

Alright, the required changes for this are now in dlang/druntime#907. Once that's merged, this PR should pass the auto-tester. After that I'll rebase the primary druntime change to be based on it. Once this PR is merged, that druntime PR should also start passing the tests.

[rainers]

I don't see any relation to the patch that would need changes. Doesn't it work anymore after applying the PR?

Actually, I wonder how test1() worked so far. Isn't it testing the behaviour implemented by this?

[rainers]

Actually, I wonder how test1() worked so far. Isn't it testing the behaviour implemented by this?

Ok, back on track: the destructor is currently called, but the memory is not freed.

I guess you should add a bug report for this issue (or find an existing one).

[Orvid]

The reason this is here is because it was previously trying to allocated in the destructor of a heap-allocated object, which triggers an InvalidMemoryOperation error. This wasn't an issue before, but with the change to having the GC call the destructors of heap allocated arrays of structs with destructors, it does cause an issue. This test was only intended to test that the destructors, constructors, and posblits are called at the right times, so I changed it to not attempt to allocate in the destructor.

[rainers]

Thanks for the explanation. So it is not really related to this PR, but the one doing destruction of heap allocated structs or arrays of structs. Maybe you should add a comment to this respect.

I wonder if it is possible to add a sensible test case that fails without the fix. Maybe allocate a struct without references (so the GC sets the NO_SCAN bit), and after deleting the struct, call GC.query on the deleted pointer: it should have NO_SCAN cleared. This is pretty GC implementation specific, though.

[Orvid]

If the PR with the destruction of heap allocated structs is merged, then there will actually be multiple places in sdtor.d where it will fail if this doesn't work, with the result that it's being tested indirectly, in a way that's not specific to the GC implementation.

[rainers]

I'm ready to merge this and the corresponding druntime request, but for the missing bug report. Otherwise it will probably be forgotten in the next changelog.

[Orvid]

@rainers Well, I created the issue for it here, and had thought I'd said something here, guess not.

[rainers]

Issue 9334 is only related, but I found Issue 13195 - Delete calls destructor but doesn't free now. I guess you mean that one. The link is meant to be part of the description.

The title of this PR is also misleading now it doesn't cover the big PR dlang/druntime#864. Putting "Fix issue 13195" + description into the title will automatically add a comment to the bugzilla entry (and sometimes close it).

[Orvid]

Oh, woops, it looks like the link didn't copy, 13195 was the one I had intended to link. Will update the title as well.

[rainers]

Auto-merge toggled on

[andralex]

From what I see this didn't make it in 2.066, is that correct?

[rainers]

From what I see this didn't make it in 2.066, is that correct?

Yes. It was an old bug, not a regression, and was fixed late in the beta phase of 2.066.