Use a version number for idempotency.#107
Merged
vext01 merged 1 commit intoykjit:mainfrom Mar 19, 2025
Merged
Conversation
vext01
reviewed
Mar 14, 2025
src/lvm.c
Outdated
| // because idempotent support isn't yet implemented for 32-bit return values. | ||
| __attribute__((yk_idempotent)) | ||
| uint64_t yk_load_inst(const Instruction *pc) { | ||
| uint64_t load_inst(__attribute__((unused)) uint64_t pv, const Instruction *pc) { |
Contributor
There was a problem hiding this comment.
Better remove unused because it now is, and that might confuse us and/or the compiler.
Contributor
|
please squash. |
It can happen -- we've seen it! -- that two `Proto`s, allocated at different times, end up at the same address P. `load_inst` then implicitly treats them as the same `Proto` even though they might be completely different. In this commit, we attach a "proto version" to every `Proto`, which records what the `global_proto_version` was when the `Proto` was constructed. Every time a `Proto` is freed, we increment this version. Thus two `Proto`s at the same address will always have different "proto versions", invalidating any traces specialised to the old `Proto`, while not invaldating traces for other `Proto`s that are still alive.
Contributor
Author
|
I'm now fairly confident that we really have fixed a bug here (I am investigating an entirely separate issue in yk that I thought was correlated, but I'm now fairly confident isn't). I think we can merge this, and update the yklua commit in yk. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Contributor
Author
|
@vext01 I think we should retry this one. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It can happen -- we've seen it! -- that two
Protos, allocated at different times, end up at the same address P.load_instthen implicitly treats them as the sameProtoeven though they might be completely different.In this commit, we attach a "proto version" to every
Proto, which records what theglobal_proto_versionwas when theProtowas constructed. Every time aProtois freed, we increment this version. Thus twoProtos at the same address will always have different "proto versions", invalidating any traces specialised to the oldProto, while not invaldating traces for otherProtos that are still alive.