Skip to content

Give a function a frame where it copies a struct argument - #522

Open
j-modernc-org wants to merge 1 commit into
totalspectrum:masterfrom
j-modernc-org:frameless-alloca
Open

j-modernc-org wants to merge 1 commit into
totalspectrum:masterfrom
j-modernc-org:frameless-alloca

Conversation

@j-modernc-org

Copy link
Copy Markdown

A struct passed to a copied reference parameter (AST_COPYREFTYPE) is
copied into memory AstFuncTempMemory takes on the stack, an AST_ALLOCA at
the top of the function's body. AstFuncTempMemory does not set the
function's uses_alloca, which NeedFramePointer reads to give the function
a frame and NeverInline reads to keep it from being inlined; only an
alloca the front ends see in the source sets it. So a function that needs
no frame of its own, one that only passes a struct on, takes the copy with
add ptra, #N (add sp, #N on P1) and returns without giving it back,
and so does a copy of it inlined into a caller. Every call leaves the
stack higher by the size of the struct, and a loop of such calls runs the
stack past the end of memory, with no error.

A struct goes this way when it is larger than 16 bytes or has a member
that is not a long, so this is common: a "getter" like

typedef struct { long long sec; int nsec; void *loc; } Time;
int Minute(Time t) { return (int)(timeAbs(t) % 3600) / 60; }

leaks 16 bytes a call. It showed as a scheduler on a P2-EDGE that stopped
printing, with no error, after about 10800 passes of a search loop.

This sets uses_alloca in AstFuncTempMemory. Test/Expect/stest184.pasm, a
struct of five ints passed on, shows it: fetchv moved sp up by 20 and never
back, and now saves and restores a frame. Test/cexec10.c calls two such
functions 100 times each and prints how far the stack moved: on master
1200 and 2000 bytes on a P2, and on a P1 it hangs; with this change 0.
The bytecode backends pass such a struct their own way and got the sums
in cexec10 wrong before and after this change, so the test prints the
expected text for them, as cexec09 does.

Testing:

  • make test_offline: 588 of 588, with stest184's golden updated; that is
    the only golden the change touches.
  • cexec10 on a P2-EDGE built with -2 -O2 and with -2nu, and on P1 under
    spinsim as ASM and as bytecode (--interp=rom): all print the expected
    text with this change. On master the -2 -O2 build prints 1200 and 2000,
    the P1 ASM build hangs, and -2nu prints 0 already.
  • Builds with OPT="-O2 -Werror".
  • 1546 C programs (the test corpus and fuzzer output of a compiler that
    emits C for flexcc, and its reproducers), compiled with -2 at b5993a8
    with our other patches applied, with and without this change: 5 build
    differently, and on a P2-EDGE each prints what it printed before. A scan
    of their listings for a function that moves ptra up without a frame
    finds 15 before and none after.

A struct passed to a copied reference parameter (AST_COPYREFTYPE) is
copied into memory AstFuncTempMemory takes on the stack, an AST_ALLOCA at
the top of the function's body. AstFuncTempMemory did not set the
function's uses_alloca, by which NeedFramePointer gives a function a frame
and NeverInline keeps it from being inlined; uses_alloca was set only for
an alloca the front ends see in the source. So a function that needs no
frame of its own, one that only passes a struct on, took the copy with an
`add ptra, #N` (`add sp, #N` on P1) and returned without giving it back,
and so did a copy of it inlined into a caller: every call left the stack
higher by the size of the struct, and a loop of such calls ran the stack
past the end of memory.

AstFuncTempMemory now sets uses_alloca. Test/Expect/stest184.pasm, five
ints passed on, shows it: fetchv moved sp up by 20 and never back, and now
saves and restores a frame. Test/cexec10.c calls two such functions 100
times each and prints how far the stack moved: 1200 and 2000 bytes on a P2
before this change, 0 after; on a P1 it hung before.
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.

1 participant