Give an escaped parameter a slot before reloading it - #339
Open
alanhc wants to merge 1 commit into
Open
Conversation
prepare_operand() reloads an address-taken variable from its stack slot
instead of reading the register holding it, since a store through the
pointer would not have reached the register. The reload assumes the slot
holds the variable, and for a parameter it does not: parameters arrive
in registers, and a slot is reserved the first time something spills
one. Reloading before that read whatever the frame happened to hold.
int f(int a, int b)
{
int x = a - b;
int *p = &b;
return x;
}
returned a rather than a - b, because the subtraction read an
uninitialised slot that happened to contain zero. Nothing is stored
through the pointer here; taking the address is enough, and the value
is wrong before the pointer is ever used.
Write the register back before the reload when the variable has no slot
yet, which is what makes the slot stand for the variable, and keep using
the register it is already in.
All three targets were affected.
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
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.
Fixes #337.
prepare_operand()reloads an address-taken variable from its stack slotrather than reading the register it is already in, since a store through the
pointer would not have reached the register. The reload needs the slot to hold
the value, and for a parameter it does not: parameters arrive in registers,
and a slot is reserved the first time something spills one. Reloading before
that read whatever the frame happened to hold.
returned
arather thana - bon all four targets, because the subtractionread an uninitialised slot that happened to contain zero. Nothing is stored
through the pointer here; taking the address is enough, and the value is
already wrong before the pointer is ever used.
Write the register back before the reload when the variable has no slot yet,
which is what makes the slot stand for the variable, and go on using the
register it is already in.
The shape is narrow, which is likely why it has gone unnoticed: it needs the
escaped variable to be a parameter -- a local is stored to its slot when it is
initialised -- and the address to be taken after the expression rather than
before.
tests/escaped-param.ccovers the subtraction, the same shape aroundan addition, and the address taken of the first operand rather than the
second.
The suite passes on x64, arm, arm64 and riscv at both stages;
clang-format-18is clean on the changed files.One thing I would like a second opinion on, repeated from the issue: this sits
in
prepare_operand(), which everything goes through. A narrower fix inwhatever gives parameters their slots might be more appropriate -- I could not
find a natural place to do it there, because the slot is deliberately
allocated lazily.
Summary by cubic
Fixes #337: escaped parameters now write their register back to the stack slot before a reload, so address-taken parameters no longer read uninitialized frame memory.
a - bwhere the address is taken after the computation.prepare_operand()now stores the register to the slot first when no slot is allocated, then reloads from it.tests/escaped-param.ccovering subtraction, addition, and the address taken of the first operand.Written for commit 4bcb7fc. Summary will update on new commits.