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

Integrate dsymbol #673

Merged
merged 4 commits into from
Oct 13, 2022
Merged

Conversation

WebFreak001
Copy link
Member

used code dlang-community/dsymbol@46873f0

fix #669

depends on #672

@WebFreak001
Copy link
Member Author

ok so there is an issue: with LDC -O1 and above, DCD segfaults in the tests (a little bit randomly when it happens, but it always happens with the current test suite)

I removed all the custom allocators locally and replaced it all with GCAllocator, then it works properly. It seems there are double frees attempted, because of cyclic DSymbol* trees as well as detached trees that may reference other trees in their children which get freed at other times in the program. Also it doesn't seem to be possible to -dip1000 the containers library, so it's hard to check lifetimes through the compiler there.

@WebFreak001
Copy link
Member Author

WebFreak001 commented Oct 12, 2022

given that there is a lot of code in DCD that isn't written with the memory lifetime in mind and that we aren't using anything from the compiler to help us, I would suggest to simply change all the memory allocations to GCAllocator, where I have passing tests locally.

So with that reasoning in mind I'm simply going to push that in here and let's get that in to actually progress with development in other PRs as well.

We should try to adopt -dip1000 to avoid further such segfaults/issues. The containers library is probably a good start for that.

@WebFreak001 WebFreak001 force-pushed the integrate-dsymbol branch 7 times, most recently from 9f8cb2a to 1da7195 Compare October 13, 2022 02:28
@WebFreak001
Copy link
Member Author

so merging this with the allocator changes to GCAllocator because the GC simply avoids the segfault issues for us and most allocators were already using GCAllocator (but still breaking somewhere somehow)

If we can better enforce lifetime of things at compile time using DIP1000, attributes and templated types, we can incrementally bring back allocators to where they actually bring benefit.

Otherwise this PR adds CI improvements, testing dsymbol as well as DCD and bumping the minimum required compiler version to what emsi_containers requires, so I think the remaining changes are pretty uncontroversial.

Going to merge when tests are green.

@WebFreak001 WebFreak001 merged commit 5c529f3 into dlang-community:master Oct 13, 2022
@WebFreak001 WebFreak001 deleted the integrate-dsymbol branch October 13, 2022 16:18
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

Successfully merging this pull request may close these issues.

Merging libdparse, dsymbol, dcd in a single repo
1 participant