Make .destroy() work with attribute inference

[JakobOvrum]

Discussed in this thread.

With this patch, the @safety etc. of object.destroy depends on the destructor of the struct.

Also adds postblitRecurse to enable explicit copying with attribute inference, to be used in emplace.

Attribute inference for destruction/postblit of fixed-length arrays with struct elements seems to be broken even for implicit/in-built destruction/postblit, so that needs to be fixed in the compiler before it's tackled here.

This impacts attribute inference in a couple of key Phobos building blocks, such as emplace and the Array container. Grepping for malloc tends to be a good way to find code that uses explicit destruction.

I plan to file one or more dependent Phobos PRs to leverage this.

[schveiguy]

As I mentioned in the discussion, I don't like that we are re-implementing what the compiler already has done. But I think the idea of doing this is OK, and as long as it's completely private, we should be good. I'd like to see a compiler-assisted solution here for the implementation of the destructor call order (seeing that the compiler already does this).

Can you rename the private templates stolen from phobos to something else? I don't want to see any name conflicts appearing for existing code. I know they are private, but you know how D likes to ignore that fact. And every module imports object.di

Speaking of that, nothing from phobos will use this code, because you didn't update object.di.

[JakobOvrum]

As I mentioned in the discussion, I don't like that we are re-implementing what the compiler already has done. But I think the idea of doing this is OK, and as long as it's completely private, we should be good. I'd like to see a compiler-assisted solution here for the implementation of the destructor call order (seeing that the compiler already does this).

I completely agree, and this patch doesn't have to be permanent. If someone with the aptitude to hack on DMD to fix this in a more uniform manner steps up, they can simply erase the functions added by this patch. The case for postblitRecurse is less clear, since it adds a public function; I suggest we keep it undocumented until a destroy-like public API is devised, if ever. emplace is the only use-case for it in Phobos that I could find, though more thorough grepping might uncover more.

Can you rename the private templates stolen from phobos to something else? I don't want to see any name conflicts appearing for existing code. I know they are private, but you know how D likes to ignore that fact. And every module imports object.di

Yeah, I ran the Phobos unit tests and were satisfied that they still passed, but you never know... I'll prefix them with something.

Speaking of that, nothing from phobos will use this code, because you didn't update object.di.

Thanks, will do.

[schveiguy]

Yeah, I ran the Phobos unit tests and were satisfied that they still passed, but you never know... I'll prefix them with something.

I don't want to say that I'm 100% sure there would be name conflicts, but simply due to the fact it was not in object.di, the phobos tests didn't test it.

[JakobOvrum]

I don't want to say that I'm 100% sure there would be name conflicts, but simply due to the fact it was not in object.di, the phobos tests didn't test it.

Ah, right.

Well I prefixed the templates and added the code to object.di as well. Seems to work.

[schveiguy]

I feel like it's odd that you are not constraining this template based on whether E is a struct, but I don't see how it's a problem, you'd get an error either way.

[JakobOvrum]

Only structs with elaborate destructors and fixed-length arrays with elaborate-destructor element types pass hasElaborateCopyConstructor. For everything else it's a no-op, like the overload for structs.

edit:

s/destructor/postblit/

[schveiguy]

So postblitRecurse isn't used in destroy at all, right? Should this not be split into 2 pulls, one for the postblit and one for destroy? Of course one will depend on the other for the inclusion of object_satisfyAny.

I'm actually unsure as to why we need postblitRecurse in object anyway, since it's only to be used in Phobos.

[MartinNowak]

Plase move those to core.internal.traits.

[MartinNowak]

I'm in favor of adding the change. The lack of an attribute correct destroy is an issue when using manual memory management.

[JakobOvrum]

Ah, it's a lot cleaner now that the helper templates are in core.internal.traits. Thanks.

[JakobOvrum]

I'm actually unsure as to why we need postblitRecurse in object anyway, since it's only to be used in Phobos.

It appears that .dup contains a hacky workaround to make itself conditionally @safe, but nothing to make it conditionally pure or nothrow. This could be fixed by using postblitRecurse instead. There are many other places in druntime that use explicit postblit but I don't know if any of them are sensitive to attribute inference.

[schveiguy]

I like how you did this. Very clever.

[MartinNowak]

Maybe too clever, if the compiler would replace it's destructor code with .destroy the test nils out.

[schveiguy]

OK, I like how traits moved to core.internal. I see nothing wrong with this, and it's easy to fix once the compiler exposes its function (or maybe the compiler should CALL this function?).

LGTM.

[schveiguy]

There are many other places in druntime that use explicit postblit but I don't know if any of them are sensitive to attribute inference.

I think druntime mostly glosses over this because the API is via extern C and it basically ignores attributes. Not the best thing, but it is what it is.

[JakobOvrum]

I prefixed destructRecurse and postblitRecurse with an underscore to be consistent with the convention in object.

[monarchdodra]

It appears that .dup contains a hacky workaround to make itself conditionally @safe, but nothing to make it conditionally pure or nothrow. This could be fixed by using postblitRecurse instead.

yes, .dup is hacky as shit. These are VERY welcome changes, and will have VERY positive repercussions throughout the language (dup, emplace, appender, to, and everything built on top of that will immediately benefit).

EDIT:

emplace is the only use-case for it in Phobos that I could find, though more thorough grepping might uncover more.

Right, but emplace is basically the foundation of everything.

There are many other places in druntime that use explicit postblit but I don't know if any of them are sensitive to attribute inference.

Using postblitRecurse over the typeid is still preferable, if only for performance consideration. That said, I do believe that most of the explicit posbliting that happens in druntime operates on run-time types, so using it is not possible anyways.

[monarchdodra]

These seem to not handle the case where a postblit fails, in which case every fully constructed object should be destroyed.

For static arrays, this should be pretty easy to handle, with low overhead, so we should do it. It might be a bit harder to do it in a "smart" way for structs, as the current static foreach loops don't really lend themselves to this very well.

Maybe a mixin loop though? Something like:

foreach (member : FieldNameTuple!T)
{
    mixin(member ~ ".postblitRecurse();");
    mixin(scope(failure) member ~ ".destroyRecurse();");
}

[monarchdodra]

Maybe this should forward to a posblitArrayRecurse(T)(T[] arr) function that does not depend on N?

The current solution here will generate a new function for every different array size for a given T, which is less than ideal.

On the other hand, I'll assume the compiler can exploit compile time info and unroll the code for small arrays, generating more efficient code?

Maybe something like

if (N > 4)
{
    // Forward to generic array implementation
    _postblitArrayRecurse(arr);
}
else
{
    // Do this here so compiler can exploit N's value
    ...
}

Or maybe this is premature optimisation.

[JakobOvrum]

The baseline for this patch is the compiler-generated code called by TypeInfo.destroy/postblit, so I'll have to see how/if those handle this issue, probably with some disassembly.

[MartinNowak]

The call can be inlined anyhow, leave the unroll decision to the optimizer.

[JakobOvrum]

These seem to not handle the case where a postblit fails, in which case every fully constructed object should be destroyed.
For static arrays, this should be pretty easy to handle, with low overhead, so we should do it. It might be a bit harder to do it in a "smart" way for structs, as the current static foreach loops don't really lend themselves to this very well.

Thanks, that would've been a sneaky bug! I believe static-foreach and static-if do not introduce a new scope, so the obvious solution should work. At any rate, I'll fix it and add appropriate tests ASAP.

I've also filed issue 14242 for the problem with in-built destruction of fixed-length arrays. I should add some tests that test the fixed-length array support in a separate test where attributes are not yet tested. The problem is only for destruction; copying seems to work correctly.

[monarchdodra]

I never bothered filing it, but another outstanding issue in druntime is that subsequent exceptions that may be thrown during the cleanup-destruction of a failed postblit should not interrupt the clean up, but rather, be collected and thrown back as a chained exception. Or, at the very least, at least just not interrupt if you don't chain.

Thankfully, very few things throw on destruction, so I'm not too worried about that one.

[HK47196]

ping, what's the status on this?

[JakobOvrum]

ping, what's the status on this?

I'm back in my regular schedule and can continue work on the unittests. It's more work than it would be if the compiler-generated code behaved correctly in all cases, but this PR shouldn't be blocked by that, I'll figure it out.

[JakobOvrum]

Thanks, that would've been a sneaky bug! I believe static-foreach and static-if do not introduce a new scope, so the obvious solution should work. At any rate, I'll fix it and add appropriate tests ASAP.

Argh, static-if does indeed not introduce a new scope and thus works nicely for injecting scope statements, but static-foreach does introduce a new scope. I wonder if this is a recent development, I swear I used to get variable name conflicts when declaring variables inside the body of a static-foreach loop, but I can't reproduce it now.

Without making a mess of try-catch and even more recursion, I guess the string-mixin approach is the only viable choice.

[Geod24]

I wonder if this is a recent development, I swear I used to get variable name conflicts when declaring variables inside the body of a static-foreach loop, but I can't reproduce it now.

Or maybe it was function declaration:

  foreach (T; TypeTuple!(string, int)) {
    void func(T arg) {}
  }

[monarchdodra]

Or maybe it was function declaration

Yeah, that is probably it. I also seem to remember that things like templates and/or enums could create conflicts, but more like mangling conflicts, not conflicting variable conflict.

[MartinNowak]

Might make sense to move that functionality completely into druntime and have the compiler use it.

The function is already pretty concise in the compiler, so it wouldn't make too much sense.
https://github.com/D-Programming-Language/dmd/blob/4e811cba25f0f7e51d1b686c893030bc4b8ab2fb/src/clone.c#L919

[JakobOvrum]

Implemented the string-mixin solution for failing postblit. Also implemented proper destruction when postblitting a static array fails.

[monarchdodra]

I forget how "scope(failure)" deals with thrown Errors. I seem to remember that it catches them? Totally minor, but the correct implementation should not catch errors here.

The real reason I'm asking is I'm wondering how well the compiler optimizes away all these scope failure calls in case of nothrow functions (and in particular, if it can optimize those away at all, if it has to deal with thrown errors...)

So the question is: Did you test how efficient this code is? Are those catches optimized away? Maybe it warrants testing if the postblit can throw at all? Unfortunately, the test would require functionAttributes...

[JakobOvrum]

I haven't tested anything for performance/bloat. Still need to check the array postblit code, too.

Maybe it warrants testing if the postblit can throw at all? Unfortunately, the test would require functionAttributes...

That would be clever. The compiler has historically been pretty bad at optimizing scope(...) statements in my experience, like inserting a bunch of try-catch code for scope(success).

[MartinNowak]

If the compiler already implements all of that correctly for TypeInfo.xdestroy, shouldn't we use that instead, i.e. make it available as T.__aggrDtor or T.__destroy and call it.

[schveiguy]

@MartinNowak said that in my first comment. But this is a PR that is ready to be pulled, and I see no PR to fix the compiler...

[MartinNowak]

version (unittest) unittest?

[MartinNowak]

Auto-merge toggled on

[MartinNowak]

Can we please work on a compiler solution @JakobOvrum. I can help you with the details if you have any question.

[JakobOvrum]

Possible follow ups:

• Expose compiler-generated primitives and use those instead
• Measure performance and code size characteristics of primitives introduced in this patch (would be obsoleted by the above)
• Optimize destroy to use memset (although this is really the compiler's job; important because slice assignment is a memory safe interface, while memset is not, requiring @trusted)
• Define how D should behave on failed destruction (throwing from a destructor)

[MartinNowak]

Compiler primitives are now available dlang/dmd#4650, can you do a follow-up?

[JakobOvrum]

Thanks! Will do.

[MartinNowak]

@JakobOvrum what about the follow up?

[CyberShadow]

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=14746

[aG0aep6G]

This introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=14839