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

Pull Request #1026 broke the console stack #1134

Closed
jamesu opened this Issue Jan 28, 2015 · 10 comments

Comments

Projects
None yet
2 participants
@jamesu
Contributor

jamesu commented Jan 28, 2015

There have been numerous reports that the console stack is throwing out asserts in the development branch. I've tracked it down to a certain pull request of mine, PR 1026 (#1026). Intended to fix issue #976 .

It seems while it fixed the reported issue, it has the unintended side-effect of not cleaning the stack out correctly during future function calls.

@crabmusket crabmusket added the Bug label Jan 28, 2015

@crabmusket crabmusket added this to the 3.7 milestone Jan 28, 2015

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Feb 5, 2015

Contributor

I have a preliminary fix for this which resolves this by changing the stack behaviour of executef and the callback helpers (i.e. it makes them all go through templates which properly manage the stack). I'd like to write some basic tests however to ensure the new behaviour isn't breaking anything...

Contributor

jamesu commented Feb 5, 2015

I have a preliminary fix for this which resolves this by changing the stack behaviour of executef and the callback helpers (i.e. it makes them all go through templates which properly manage the stack). I'd like to write some basic tests however to ensure the new behaviour isn't breaking anything...

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 14, 2015

Contributor

Is there a consistent way to repro this? At the moment my way of testing it will just involve playing around with the engine to make sure nothing looks broken :P. Are there any unit tests among those you've added that fail before, and pass after the PR?

Contributor

crabmusket commented Feb 14, 2015

Is there a consistent way to repro this? At the moment my way of testing it will just involve playing around with the engine to make sure nothing looks broken :P. Are there any unit tests among those you've added that fail before, and pass after the PR?

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Feb 14, 2015

Contributor

There are unit tests now for the execute stack handling.

ATM Most stuff works with the new code, the problem I guess is some minor things handle console stack values strangely which don't have tests and thus need to manually be tested...

Contributor

jamesu commented Feb 14, 2015

There are unit tests now for the execute stack handling.

ATM Most stuff works with the new code, the problem I guess is some minor things handle console stack values strangely which don't have tests and thus need to manually be tested...

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 14, 2015

Contributor

Yeah, such as that dFree bug earlier. This is why we want to have a pre-release 😂

Contributor

crabmusket commented Feb 14, 2015

Yeah, such as that dFree bug earlier. This is why we want to have a pre-release 😂

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Feb 14, 2015

Contributor

It looks like there is a problem with the string iterators. Looks a bit messy, will see if I can work around it.

Contributor

jamesu commented Feb 14, 2015

It looks like there is a problem with the string iterators. Looks a bit messy, will see if I can work around it.

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Feb 14, 2015

Contributor

Should be noted that using dIsSpace is a pretty bad idea here as on some platforms the behavior will change depending on locale settings.

Contributor

jamesu commented Feb 14, 2015

Should be noted that using dIsSpace is a pretty bad idea here as on some platforms the behavior will change depending on locale settings.

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Feb 14, 2015

Contributor

Looks like startsWith is somehow corrupting the value in the string buffer. VERY strange.

Contributor

jamesu commented Feb 14, 2015

Looks like startsWith is somehow corrupting the value in the string buffer. VERY strange.

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Feb 16, 2015

Contributor

Upon further investigation it seems the console stack is getting resized WHILE the string iterator is being used, causing invalid pointers. Gonna have to wrap up that pointer with a helper class so it is offset from the correct position.

Contributor

jamesu commented Feb 16, 2015

Upon further investigation it seems the console stack is getting resized WHILE the string iterator is being used, causing invalid pointers. Gonna have to wrap up that pointer with a helper class so it is offset from the correct position.

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Feb 16, 2015

Contributor

Pushed a fix for this which uses a relative pointer type. In addition I've added a new console variable type which acts as a pointer to a stack string which should help reduce the overall stack size (by reusing the data in the previous frames of the stack).

Contributor

jamesu commented Feb 16, 2015

Pushed a fix for this which uses a relative pointer type. In addition I've added a new console variable type which acts as a pointer to a stack string which should help reduce the overall stack size (by reusing the data in the previous frames of the stack).

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Feb 19, 2015

Contributor

Will test soon.

Contributor

crabmusket commented Feb 19, 2015

Will test soon.

@crabmusket crabmusket closed this Mar 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment