Variant segfault #10697
Merged
Variant segfault #10697
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Thanks for your pull request and interest in making D better, @Inkrementator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#10697" |
LightBender
approved these changes
Mar 23, 2025
Contributor
|
@Inkrementator does this build on #10690 ? Fixes for segfaults should go to stable, but if this requires code that is already in master to work then that is fine to target master. |
Contributor
Author
|
Sorry, I "ignored" this because I had issues building the stable branch on my machine. |
vpanteleev-sym
pushed a commit
to vpanteleev-sym/phobos
that referenced
this pull request
Jun 5, 2025
* Reference correct issue for test * Variant: Fix Segfault on adding in wrapped AA
Inkrementator
added a commit
to Inkrementator/phobos
that referenced
this pull request
Jan 8, 2026
variant.get!Variant used to segfault, but after PR dlang#10697 it only threw an exception. The reason for that was that get!Variant called the `handler` with OpID.get. Handler is a function pointer to a templated function parametrized on the type currently wrapped by Variant. Because Variant generally get flattened, so `Variant(Variant(4))` just wraps the value `4`, instead of `Variant(4)`, the wrapped type (should generally) not be "Variant", and thus the OpID.get operation will fail. This couldn't be fixed in the handler, because OpID.get uses typeinfo for the target type, but VariantN is a template, so afaik we can't really test it against the infinite set of valid VariantN instantiations. `get` still gets the target type as a template parameter, so we can handle the problem there. Supporting get!Variant incidentally also fixed dlang#10518. Adding variant inside an associative array of variants failed: ``` Variant variant = Variant([ "one": Variant(1), ]); variant["four"] = Variant(4); ``` This was because `opIndexAssign` wrapps the index as well as the value you want to assign in a Variant again. ``` Variant opIndexAssign(T, N)(T value, N i) { Variant[2] args = [ Variant(value), Variant(i) ]; fptr(OpID.indexAssign, &store, &args) == 0 || assert(false); return args[0]; } ``` The handler of `variant` containing the AA is parametrized with `Variant[string]`, but opIndexAssign passes a variant of type int instead of Variant(Variant(int)) due to the aformentioned flattening. Eventually in the handler, it ends up calling `Variant(i).get!Variant`, which crashed. For the github automation: Fixes dlang#10431 Fixes dlang#10518
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adresses part of #10518, now adding to Variant(Variant[string]) doesn't segfault anymore, but at least throws.
The issue was calling the destructor of unitinialized memory.
How to fix the exception is harder. I think there are 3 places where this could handled, either in
get(T),tryPuttinginside the handler, orcase OpID.indexAssign:. Of these,tryPuttingprobably is the most "correct", but also seems the hardest to understand to me and will have the biggest fallout in unrelated places if you get it wrong.Whatever place is eventually chosen, you can't just memcopy the storage, as big types are often implicitly allocated on the heap and stored as a pointer.