compiler icon indicating copy to clipboard operation
compiler copied to clipboard

Nested array returns cause bad memory/invalid instructions

Open Southclaws opened this issue 6 years ago • 4 comments

Is this a BUG REPORT, FEATURE REQUEST or QUESTION?:

  • [x] Bug Report
  • [ ] Feature Request
  • [ ] Question

What happened:

Returning an array into another return into the arguments of another function result in a runtime crash.

What you expected to happen:

No runtime crash, I expect to nest functions in arguments any number of times with no issues.

How to reproduce it (as minimally and precisely as possible):

With: https://github.com/sampctl/pawn-array-return-bug

sampctl get sampctl/pawn-array-return-bug
cd pawn-array-return-bug
sampctl package run --forceBuild

Anything else we need to know?:

I'm not entirely sure if this is something that can be fixed via the compiler but from my limited knowledge, I think it could be fixed.

People run into this a lot when they cut corners by writing a GetPlayerName wrapper that returns the name instead of writing it into a pass-by-reference. I often advise people to avoid doing this, but it still seems like something that should be fixed if it can be.

Environment:

  • Operating System: All
  • Compiler version: 3.10.4

Southclaws avatar Mar 16 '18 17:03 Southclaws

https://github.com/sampctl/pawn-array-return-bug/issues/1:

returnString()
{
	return stringOrigin();
}

Compiles as:

	heap 30
	push.alt
	push.c 0
	call .stringOrigin
	pop.pri
	heap ffffffd0
	load.s.alt c
	movs 30

That allocates space on the heap for the return (heap 30); calls stringOrigin, clears the heap (heap -30 (ffffffd0)), THEN copies the data out of the temporary location (movs 30).

This:

    heap ffffffd0
    load.s.alt c
    movs 30

Should be:

    load.s.alt c
    movs 30
    heap ffffffd0

Or better yet, don't allocate a local temporary at all, since it is just there to be copied in to and out of again. Pass the current function's target address as the next function's target as well (the target is a hidden extra parameter, which is not included in the parameter count, hence push.c 0, and needs explicitly removing, hence pop.pri):

    push.s c
    push.c 0
    call .stringOrigin
    pop.pri

Y-Less avatar Mar 16 '18 17:03 Y-Less

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Oct 26 '18 16:10 stale[bot]

Fixed in #567. @pawn-lang Close?

Daniel-Cortez avatar Sep 20 '20 16:09 Daniel-Cortez

This issue has been automatically marked as stale because it has not had recent activity.

stale[bot] avatar Dec 24 '20 11:12 stale[bot]