Fix Issue 2834 - The GC will now call destructors on heap allocated structs

[Orvid]

This PR is not quite ready to merge, as I need to add a set of tests for it, and also need to get it to work on arrays of structures as well. I am creating it now to make sure that I've not gone and broken druntime terribly with my approach to fixing the issue, as well as to take feedback on my method of solving the issue. And the link to the issue this solves is here.

In the case where a struct is a member of a class, the struct's destructor will be called after the body of the class's destructor.

This PR is now ready to be reviewed and hopefully merged. It is not feasible to implement calling destructors of structs in heap allocated arrays for various reasons, including the inability to know which elements of the array are actually initialized, as well as which elements are valid.

This requires dlang/dmd#3727

https://issues.dlang.org/show_bug.cgi?id=2834

[Orvid]

Alright, figured out the cause, maybe, but I have no idea why it's only showing up now. Would someone kindly explain to me why there are multiple places in sdtor.d, line 1374 being one of them, where it allocates in a destructor?

[rainers]

If one allocation in a pool is a struct with destructor, the GCStructInfoArray will allocate 50% of the pool memory on 64-bit platforms. I think this is too inefficient. I would rather expect the type info to be stored at the end of the allocation. (Which might be useful for other operations, too, like precise GC).

It also seems to me that introducing STRUCT_FINALIZE is unnecessary, All the info is available in the TypeInfo passed to the GC. (_d_newItem should pass ti.next to GC.malloc).

[Orvid]

That was my initial thought as well, but I wasn't sure how much effort it would take to deal with the modification of the allocation size, I'll take that approach for my next version of this. The STRUCT_FINALIZE flag is also probably not really needed, so I'll see if there's a way to get rid of it in all cases.

The more interesting thing however is why the auto-tester is failing, it's not because I've broken anything, but because the tests are actually incorrect, the current behavior of the delete keyword doesn't actually free any memory, nor does it tell the GC to call destructor, instead it calls the destructor directly, and cannot handle the destructor being called again, which causes the assertions to fail, those failed assertions then attempt to allocate memory, which leads to the failure you see in the tester. The problem is, I'm unsure how to go about solving this problem, should I make the tests call the newly-exposed GC.runFinalizers rather than delete? Or should I make delete itself call GC.runFinalizers?

Regardless of the approach I end up taking to make the auto-tester pass, how would I go about telling the auto-tester that this PR requires a specific DMD PR as well?

[yebblies]

the current behavior of the delete keyword doesn't actually free any memory

It should.

how would I go about telling the auto-tester that this PR requires a specific DMD PR as well?

You can't. If there really is no other way, ping me when you're ready and I can trick the autotester into doing this. It is usually possible to structure the changes so that druntime support is added, then dmd is changed, then druntime support for the old way is removed.

[Orvid]

The generated assembly for:

int sdtor7;

struct S7
{   int b = 7;
    ~this()
    {
        printf("~S7()\n");
        assert(b == 7);
        assert(sdtor7 >= 1 && sdtor7 <= 3);
        sdtor7++;
    }
}

struct T7
{
    int a = 3;
    S7[3] s;
    ~this()
    {   printf("~T7() %d\n", sdtor7);
        assert(a == 3);
        assert(sdtor7 == 0);
        sdtor7++;
    }
}

void test7()
{
    T7* s = new T7();
    delete s;
    assert(sdtor7 == 4);
}

is currently (as dissassembled by IDA using a couple of tricks to get it to load symbol names nicely for me :P):

_TEXT:004031B8 void main.test7() proc near             ; CODE XREF: __Dmain+44�p
_TEXT:004031B8
_TEXT:004031B8 var_C           = dword ptr -0Ch
_TEXT:004031B8 var_8           = dword ptr -8
_TEXT:004031B8 var_4           = dword ptr -4
_TEXT:004031B8
_TEXT:004031B8                 push    ebp
_TEXT:004031B9                 mov     ebp, esp
_TEXT:004031BB                 sub     esp, 0Ch
_TEXT:004031BE                 push    ebx
_TEXT:004031BF                 push    esi
_TEXT:004031C0                 mov     eax, offset off_43BB30
_TEXT:004031C5                 push    eax
_TEXT:004031C6                 call    __d_newitemiT
_TEXT:004031CB                 mov     [ebp+var_8], eax
_TEXT:004031CE                 mov     dword ptr [eax], 3
_TEXT:004031D4                 push    3
_TEXT:004031D6                 mov     [ebp+var_4], 7
_TEXT:004031DD                 mov     ecx, [ebp+var_4]
_TEXT:004031E0                 push    ecx
_TEXT:004031E1                 lea     edx, [eax+4]
_TEXT:004031E4                 push    edx
_TEXT:004031E5                 call    __memset32
_TEXT:004031EA                 mov     ebx, [ebp+var_8]
_TEXT:004031ED                 mov     [ebp+var_C], ebx
_TEXT:004031F0                 mov     eax, ebx
_TEXT:004031F2                 call    main.T7.__aggrDtor()
_TEXT:004031F7                 mov     esi, large fs:2Ch
_TEXT:004031FE                 mov     ecx, [esi]
_TEXT:00403200                 cmp     dword ptr [ecx+28h], 4
_TEXT:00403207                 jz      short loc_403213
_TEXT:00403209                 mov     eax, 19Eh
_TEXT:0040320E                 call    someAssertFunction
_TEXT:00403213
_TEXT:00403213 loc_403213:                             ; CODE XREF: main.test7()+4F�j
_TEXT:00403213                 add     esp, 10h
_TEXT:00403216                 pop     esi
_TEXT:00403217                 pop     ebx
_TEXT:00403218                 leave
_TEXT:00403219                 retn
_TEXT:00403219 void main.test7() endp

And finally in C-style code, as generated by hexrays:

int __cdecl main_test7()
{
  int v0; // eax@1
  int result; // eax@1

  v0 = _d_newitemiT(&off_43BB30);
  *(_DWORD *)v0 = 3;
  _memset32(v0 + 4, 7, 3);
  result = main_T7___aggrDtor();
  if ( *(_DWORD *)(*(_DWORD *)__readfsdword(44) + 40) != 4 )
    result = someAssertFunction();
  return result;
}

So it doesn't appear to actually be freeing any memory, rather it appears do be doing what destroy does, minus the zeroing.

[rainers]

So it doesn't appear to actually be freeing any memory, rather it appears do be doing what destroy does, minus the zeroing.

This seems to be a bug in the compiler. It stops calling the GC if there is a delete-overload or a destructor, while the spec says it should only do so with an operator overload. (See DeleteExp::semantic, which replaces DeleteExp with some other expression).

[Orvid]

Alright, I've updated the PR, it will now pass the test suite, however it requires both this PR, and dlang/dmd#3727 in order to work.

[Orvid]

Alright, this has now been updated so that it stores the type info for a finalizable struct at the end of the page allocated for the struct. Now to remove STRUCT_FINALIZE.

[Orvid]

And, STRUCT_FINALIZE is now gone. I believe that was the last thing to be addressed for the moment, as finalizing arrays of structs will be in a future PR.

The tester is always going to fail on this due to the fact you need both PR's for the tests to pass.

Edit:
Alright, it appears I have broken it partially, as the tests should be failing, not passing as they currently are.

[Orvid]

Hmmm... It actually appears that I can't get around needing the struct finalize attribute due to the need for it to persist across calls to getBits and setBits. I have to maintain whether the allocation needs to be finalized as a structure in the block attributes flags because of the fact there are plenty of outside places that can call them that won't pass in the type info, so I can't rely on that to determine that.

[Orvid]

And once more, time to change my mind. I can, and have now, implemented getBits and setBits in such a way that the struct finalize bit is retained if you do a call to setBits(getBits()). I also fixed the issue that was causing the tests to pass, as I wasn't putting the right type info at the end of the page, which prevented the struct destructors from actually being called.

[Orvid]

Alright, decided to forgo creating another PR to address the calling of destructors of structs in arrays, and instead have added it to this PR. Now I just have to fix DMD's test suite for destructors to not allocate in it's destructors as part of the tests -_-.....

[rainers]

I think these unittests are overkill. Most of them test basic functionality like whether FILE works and don't belong here.

[Orvid]

The only reason I have them is because FinalizeError had them, I do agree though, they aren't really needed.

[rainers]

So better remove them (or at least all but one).

[Orvid]

Updated to address the StructInfo concern, as well as moved the type info interaction out of the GC and into the runtime, to be consistent with array type info.

[rainers]

ti.next should be used in lifetime.d when calling GC.malloc, not here. It would be

 if (ti || bits)
     gcx.setBits(pool, cast(size_t)(p - pool.baseAddr) >> pool.shiftBy, bits, ti);

[Orvid]

The only reason I didn't change what was being passed in by newitemT is because I didn't want to accidentally break something somewhere in the GC that assumed that ti was in fact the pointer.

[complexmath]

For what it's worth, FinalizeError probably shouldn't exist any longer. That was created back before exception chaining when Walter said that throwing from a finalizer should be considered an Error as it is in C++.

[Orvid]

I actually believe that FinalizeError should exist, because an exception being thrown in a finalizer doesn't logically have something to handle it, because there is no real parent execution context. Also, FinalizeError's are already chained.

[Orvid]

If the thing I just tried doesn't get the auto-tester passing, then I have no clue why it would be failing, as I can't reproduce it locally....

[andralex]

What's the status of this? I see failures on Linux 64/64, that should be easy to reproduce.

[Orvid]

It would be easy to reproduce if I had access to a 64-bit linux system, but unfortunately I don't currently. Give me a bit to setup an ubuntu VM and I'll see if I can't figure out why it's failing. (I'd actually thought I'd gotten it back to passing)

[andralex]

Thanks!

[MartinNowak]

I'd highly appreciate if someone cleaned up/refactored rt.lifetime.

[MartinNowak]

And there should be a changelog pull, explaining this change (and the option to turn it on).
See dlang/dlang.org#722 for an example.

[andralex]

This is big! Thanks to @Orvid and everyone involved in the review! @MartinNowak you may want to file issues regarding refactoring rt.lifetime with a few specifics and changelog entry. The second is a release blocker - this change is very significant and must be mentioned.

[MartinNowak]

The second is a release blocker - this change is very significant and must be mentioned.

It's not activated by default for now. I'll keep track of the changelog.

simplify/cleanup rt.lifetime

[rainers]

It's not activated by default for now.

It is active by default, see lifetime_init.

[rainers]

Looks like you should split the bounty. If anyone doesn't want his part, you can spend it on Bountysource on issues of your choice https://www.bountysource.com/teams/d/issues.

I'm not doing this for the bounty, so @Orvid, please go grab it...
If you want to share it putting it on other issues sounds like a good idea.

[MartinNowak]

It is active by default, see lifetime_init

All the better

[Orvid]

@rainers Alright, I just hit the button to claim it, I'd already intended to use a good portion of it to be put back on other issues, and most of the rest will go towards paying for my college.

[rainers]

I noticed some missed calls that crept in before the merge: #1113

[rainers]

I recall there was a related discussion earlier on: should we call invariants before running the destructors during GC? I think we shouldn't.

@andralex Walter seems to disagree: dlang/dmd#4136 (comment)

[MartinNowak]

@Orvid can you please take care of the changelog?

[MartinNowak]

@Orvid can you please take care of the changelog?

This and some basic documentation are really important for the upcoming 2.067 release.

[CyberShadow]

Question: Why is the TypeInfo_Struct pointer at the end of small allocations, and at the beginning of large ones? (see the if blocks in finalize_array2)

The current scheme doesn't work very well with MEMSTOMP/SENTINEL/VALGRIND.

[MartinNowak]

I guess the idea of writing the structinfo at the end was to save a few bytes for alignment.
Why it's stored at the beginning for large allocations, I don't know. Maybe it seemed simpler when extending arrays.

[Orvid]

The TypeInfo_Struct pointer is at the beginning of large allocations for the same reason the length of a large allocation is at the beginning. It's also because there was already unused space in the padding that the pointer could be slipped into without difficulty.

[schveiguy]

The reason the array length was stored at the beginning was because large blocks can be extended in-place, and then where your metadata is stored moves. This can complicate things when dealing with caches and multiple threads.

Small blocks can't be extended, so the metadata never moves for a given base pointer. In addition, for small arrays, I only need 1 or 2 bytes to store the length, and one byte was already being used at the end as a pad to prevent cross-array pointers. With required 16-byte alignment for the first array element, it was the most unintrusive thing I could do.

The current scheme doesn't work very well with MEMSTOMP/SENTINEL/VALGRIND.

I would love to simply fix this by storing the metadata elsewhere. But it would be a huge task.

[MartinNowak]

I would love to simply fix this by storing the metadata elsewhere. But it would be a huge task.

Couldn't those "debug" tools be implemented as layers on top of the current GC. I find all that offsetting code extremely noisy to work with.

[MartinNowak]

@Orvid, @rainers this pull doesn't handle struct destructors for AA values, which is the most common source of heap allocated structs.
Can we please try to fix this for 2.067.1?
Issue 14423 – struct destructors not finalized for AA values

[schveiguy]

@MartinNowak this PR has already been merged, you need to attach the milestone to the new PR that would fix what you are asking.

[rainers]

Both precise GC versions create a TypeInfo_Entry at runtime, but it is pretty ugly. I was hoping for the templated AA implementation to create the appropriate structs.

[MartinNowak]

Both precise GC versions create a TypeInfo_Entry at runtime, but it is pretty ugly. I was hoping for the templated AA implementation to create the appropriate structs.

There have been a lot of problems with the library AA, and it remains unclear when we're going to reproach it. Ugly isn't really an important metric for the AA implementation, so I'd be glad for that hack.

[MartinNowak]

you need to attach the milestone to the new PR that would fix what you are asking

Yeah, I just abused that as a reminder.

[rainers]

@Orvid, @rainers this pull doesn't handle struct destructors for AA values, which is the most common source of heap allocated structs.

#1212

[JinShil]

@rainers This change is forcing us to lie about the throwability of _d_arrayshinkfit to assumeSafeAppend. See https://github.com/dlang/druntime/pull/2647/files#r295209606

Also, it seems to be in conflict with the comment right above it that says "it won't throw". I know was an awfully long time ago, but is it possible to remove this in order to make _d_arrayshrinkfit nothrow so we don't have to do such hacks?

[rainers]

onInvalidMemoryOperationError is nothrow as it throws an Error.

[rainers]

Ah, the problem is finalize_array. I don't see how this can be guaranteed to be nothrow, as it calls arbitrary destructors. I don't think _d_arrayshrinkfit can be assumed nothrow if the whole thing isn't made a template.

[JinShil]

Sorry, I don't understand. How would making it a template help?

[rainers]

Templated _d_arrayshrinkfit and finalize_array could be inferred to be nothrow if the destructor of the struct is nothrow (if it is an array of structs to begin with).