Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

Fix 21983 - Initialize remaining elements in a partially dup'ed array if postblit/copy ctor throws#3483

Merged
dlang-bot merged 1 commit intodlang:masterfrom
MoonlightSentinel:dup-throws
Jun 22, 2021
Merged

Fix 21983 - Initialize remaining elements in a partially dup'ed array if postblit/copy ctor throws#3483
dlang-bot merged 1 commit intodlang:masterfrom
MoonlightSentinel:dup-throws

Conversation

@MoonlightSentinel
Copy link
Contributor

This ensures deterministic behaviour in case of an exception and
avoids dtor calls on uninitialized elements.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21983 normal dup leaves a partially constructed array if postblit/copy ctor throws

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3483"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label May 28, 2021
... if postblit/copy ctor throws

This ensures deterministic behaviour in case of an exception and
avoids dtor calls on uninitialized elements.
@MoonlightSentinel
Copy link
Contributor Author

MoonlightSentinel commented Jun 3, 2021

Explicit destruction is not feasible due to the breakage caused by less-qualified constructors.
This patch will now initialize the remaining elements s.t. there won't be dtors run on uninitialised elements.

@MoonlightSentinel MoonlightSentinel changed the title Fix 21983 - Destroy partially dup'ed array if postblit/copy ctor throws Fix 21983 - Initialize remaining elements in a partially dup'ed array if postblit/copy ctor throws Jun 3, 2021
import core.internal.lifetime: emplaceInitializer;
// Initialize all remaining elements to not destruct garbage
foreach (j; i .. a.length)
emplaceInitializer(cast() arr[j]);
Copy link
Contributor

@kinke kinke Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're resetting the element whose postblit/cpctor threw - should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it's necessary because the GC will call the destructor for that element.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern was that the postblit/cpctor might not clean up itself on an exception and depend on the dtor being run on the partially constructed instance. But that's hopefully handled by -preview=dtorfields, and resetting to T.init correct then. Maybe slightly adjust the comment to make that clear, to prevent someone from thinking it's a one-off bug as I did. ;)

Copy link
Contributor

@John-Colvin John-Colvin Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the correct thing instead for copyEmplace to reset its destination to T.init on failure, then this loop would be from i + 1? And same for some of the other emplace functions?

Otherwise this fix needs to be done in a bunch of other places because it would mean copyEmplace always needs a try/catch or scope(failure) to use safely if it might throw.

Copy link
Contributor

@kinke kinke Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that seems to make a lot of sense. After all, regular instances failing their construction aren't destructed, so postblit/cpctor cannot rely on the dtor being run in case they throw. - Maybe just using a try / catch(Exception) to prevent the overhead for nothrow postblits/cpctors/ctors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlang-bot dlang-bot merged commit c5ff8be into dlang:master Jun 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Bug Fix Include reference to corresponding bugzilla issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants