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

fix Issue 20855 - stack overflow when compiling large file #12409

Merged
merged 1 commit into from Apr 20, 2021

Conversation

WalterBright
Copy link
Member

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
20855 minor stack overflow when compiling large file

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 + dmd#12409"

Geod24
Geod24 previously requested changes Apr 9, 2021
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

Since the code is so trivial, I think it'd be better to just write a script generating it.
Also this doesn't really fix the issue, just adds a hard-coded limit which works on your system.

@WalterBright
Copy link
Member Author

Yes, switching the test case to be generative is better; done.

The fundamental problem here is the backend is recursive in nature. It's difficult to figure out just where to put the checks without engaging in whack-a-mole. The check I put in is conservative, it needs to go double that before it stack overflows. You're doing something wrong anyway with 10,000 statements in a function.

I also put the check in the backend rather than the frontend, as other compilers have their own means of dealing with the problem.

@ibuclaw
Copy link
Member

ibuclaw commented Apr 9, 2021

Isn't the issue that your doing something quadratic when traversing trees in the dmd backend? Why can't it be fixed to not be quadratic? (i.e: isn't this just issue 6401)

@WalterBright
Copy link
Member Author

Quadratic doesn't cause stack overflow. Recursive depth does. Trying different statement counts in the attached test clearly indicate a stack overflow in optelem(), and optelem() is recursive.

There are many quadratic algorithms in the optimizer, which is why it is slow.

@WalterBright
Copy link
Member Author

Don't approve yet. I'm going to try out another idea.

@WalterBright
Copy link
Member Author

I completely changed the approach. Instead of connecting ExpStatements with OPcomma, it creates a new block for each ExpStatement. It resolves the stack overflow by changing the data structure from recursive to iterative. But it does not resolve the quadratic behavior in the optimizer. I cut the iterations in the test case from 100,000 to around 8,000 so it does not bog down the test suite.

There are already separate bugzilla issues for the quadratic behavior, so I won't concern this PR with them.

@WalterBright
Copy link
Member Author

The test failures have something to do with -preview=fieldwise. Will investigate more tomorrow.

@WalterBright
Copy link
Member Author

This PR uncovered an unrelated optimizer bug. I disabled the problem code, and will fix the optimizer in a followup PR.

@WalterBright
Copy link
Member Author

Ubuntu test fails with:

CI agent stopped responding!

@WalterBright
Copy link
Member Author

Windows_VisualD_LDC x86-mscoff failed

"D:\a\1\s\generated\Windows\RelWithAsserts\Win32\dmd.exe" -conf= -g -m32mscoff -O -w -preview=dip1000 -I../druntime\import -unittest -version=StdUnittest -version=CoreUnittest -c  -ofunittest2a.obj std\array.d  std\functional.d  std\path.d  std\outbuffer.d  std\utf.d

core.exception.AssertError@..\dmd\backend\cgcod.d(1045): Assertion failure

I have no idea why this should fail only when built with LDC?

@WalterBright
Copy link
Member Author

buildkite AuburnSounds fails with:

core.exception.AssertError@source/inteli/xmmintrin.d(1154): unittest failure

Who is in charge of AuburnSounds?

@thewilsonator
Copy link
Contributor

Guillaume Piolat @p0nce

@WalterBright
Copy link
Member Author

@p0nce please come help with #12409

@WalterBright WalterBright force-pushed the fix20855 branch 2 times, most recently from 675b0d0 to 393960b Compare April 13, 2021 07:28
@WalterBright
Copy link
Member Author

@Geod24 This is ready to go now, but pull #12444 first. (This incorporates the changes from 12444 so it'll pass the test suite.)

The test suite failures are the usual heisenbugs which are plaguing the test suites.

@WalterBright
Copy link
Member Author

@Geod24 @RazvanN7 this is ready to go.

Comment on lines +663 to +668
version (MARS)
// Switched to one block per Statement, do not undo it
enum merge = false;
else
enum merge = true;

Copy link
Member

Choose a reason for hiding this comment

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

This looks like it's going to come back to haunt you down the road 🤷

@dlang-bot dlang-bot merged commit 7404614 into dlang:master Apr 20, 2021
@WalterBright
Copy link
Member Author

Yay!

@MoonlightSentinel
Copy link
Contributor

Maybe related to these changes?
https://issues.dlang.org/show_bug.cgi?id=22098

@CyberShadow
Copy link
Member

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

@FeepingCreature
Copy link
Contributor

FeepingCreature commented Oct 25, 2021

It's still regressed on master. Has been since 2.097.

@ibuclaw
Copy link
Member

ibuclaw commented May 3, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants