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

Include identifier in store opcode #2156

Merged
merged 1 commit into from
Nov 26, 2017

Conversation

tsholmes
Copy link
Member

@tsholmes tsholmes commented Oct 28, 2017

Includes the identifier name in the store, storeexist, storelocal and storeglobal opcodes.

This is the first step to fixing #2152

@Dunbaratu
Copy link
Member

@tsholmes Do you want to put the rest of fixing issue #2152 into this same PR or make separate PR's for them later?
The reason I ask is that normally when a PR is associated to an issue, if that PR gets merged then github automatically closes that issue, claiming the issue is done. In this case you say this PR only goes partyway toward resolving the issue. That automated behaviour from github would be the wrong thing to do, so we need to be mindful of that if we merge this PR and you planned to have more PR's coming later on for the issue. We'd need to re-open the automatically closed issue after merging.

@tsholmes
Copy link
Member Author

I'm going to make another PR for it later. I made sure to not use any of the trigger phrases (fixing #2152 won't trigger the auto-close).

@Dunbaratu Dunbaratu merged commit 06506dc into KSP-KOS:develop Nov 26, 2017
@tsholmes tsholmes deleted the feat/ident_in_store branch November 28, 2017 05:17
@Dunbaratu Dunbaratu added this to the v1.1.4.0 milestone Dec 23, 2017
Dunbaratu added a commit to Dunbaratu/KOS-1 that referenced this pull request Jul 3, 2018
It turns out there was a problem buried deep in
the compiler, and it was introduced first with commit
number #06506dc, with Pull Request KSP-KOS#2156.

That commit altered the boilerplate code inserted for
triggered locks (steering, throttle, etc), reducing
the number of instructions that existed in the
boilerplate code chunk.  But just prior to those
edits, on the line above, there was a br.true +4.
Becuse of those edits, there was now one fewer
opcode to skip over and the br.true +4 needed to be
changed to a br.true +3, and it hadn't been.

The result is that when the function $steering*
already exists, and it tries to skip over the
setting of it, it skips the wrong distance missing
a "push", and thus everything on the stack is
mis-aligned after that.

The reason it hasn't been noticed before is that usually,
this case doesn't happen in typical cases.  It requires
running a boot file that runs a compiled KSM file to
encounter the case where that br.true will fire off. Most
of the time it was false and not executing, thus the wrong
size of how far it jumps wasn't having any effect.
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.

2 participants