Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weird crashes in Json #2205

Closed
FeepingCreature opened this issue Aug 22, 2018 · 9 comments
Closed

Weird crashes in Json #2205

FeepingCreature opened this issue Aug 22, 2018 · 9 comments

Comments

@FeepingCreature
Copy link
Contributor

FeepingCreature commented Aug 22, 2018

I got weird nonsensical crashes in my app around sites of JSON associative-array accesses. Reading around the code led me to the "interesting" void[...] "optimization" in json.d.

Is this really necessary?

I put it back as union and the crashes seem to have disappeared.

@wilzbach
Copy link
Member

FYI: it was planned to single out the JSON part as https://github.com/s-ludwig/std_data_json (and even put it in Phobos).

The false pointer issue might not be an issue anymore as everyone uses x86_64 these days, but I wasn't the one who experienced the issues with union, so maybe put up a PR and see what the CIs say?
2.067 hasn't been supported since a long time.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Aug 22, 2018

#2206 PR up.

I really don't understand why union would ever cause a false pointer issue that doesn't happen with void[], when the whole point of void[] is that ostensibly the gc scans it indiscriminately.

edit: Also, what in the actual fuck is up with enum m_size = max((BigInt.sizeof+(void*).sizeof), 2);. That's foulest witchcraft, that is.

@s-ludwig
Copy link
Member

The false pointers were caused by garbage being left in the parts of the union that were not currently used (e.g. a bool stored and the high-order bytes contained a pointer that was still on the stack). The void[] array is explicitly initialized, which causes it to be zeroed out.

I have no idea whether the union handling of the compiler has changed since then, but we'd have to be very careful when reintroducing that code. The memory leaks were quite nasty and hard to track down.

enum m_size = max((BigInt.sizeof+(void*).sizeof), 2);

This is an interesting evolution that apparently nobody noticed:

void*[2] m_data;
void*[max((BigInt.sizeof+(void*).sizeof-1)/(void*).sizeof, 2)] m_data;
void[max((BigInt.sizeof+(void*).sizeof), 2)] m_data;

Should be replaced by std.traits.Largest!(...).

@FeepingCreature
Copy link
Contributor Author

Changed PR to completely zero out the union on construction.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Aug 30, 2018

I checked again and to my best ability to discern, in our usecase the union hack makes Vibe hang outright (in druntime AA code). Even in debug builds. Switching to my union PR makes it work.

@Geod24
Copy link
Contributor

Geod24 commented Jan 6, 2020

@FeepingCreature : Still happening I suppose ?

P.S: When adding a link on the issue, press 'y' before copying the address. It gives you a link to the commit, not the branch. E.g.:

void[m_size] m_data = (void[m_size]).init;

@FeepingCreature
Copy link
Contributor Author

We largely stopped using vibe in the meantime, so I don't have a repro. I have no reason to expect it to have started working.

@s-ludwig
Copy link
Member

s-ludwig commented Jan 7, 2020

I'd say it makes sense to simply switch to taggedalgebraic.taggedunion.TaggedUnion instead of relying on a custom tagged union implementation. If the same issue happens there, it would also be a lot more valuable to have it fixed.

@s-ludwig
Copy link
Member

s-ludwig commented Sep 1, 2021

Appears to be fixed by #2206

@s-ludwig s-ludwig closed this as completed Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants