This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][expr.c] PR 65358 Avoid clobbering partial argument during sibcall


On Thu, Mar 19, 2015 at 02:39:11PM +0000, Kyrill Tkachov wrote:
> Hi all,
> 
> This patch fixes PR 65358. For details look at the excellent write-up
> by Honggyu in bugzilla. The problem is that we're trying to pass a struct
> partially on the stack and partially in regs during a tail-call optimisation
> but the struct we're passing is also a partial incoming arg though the split
> between stack and regs is different from its outgoing usage.
> 
> The emit_push_insn code ends up doing a block move for the on-stack part but
> ends up overwriting the part that needs to be loaded into regs.
> My first thought was to just load the regs part first and then do the stack
> part but that doesn't work as multiple comments in that function indicate
> (the block move being expanded to movmem or other functions being one of the
> reasons).
> 
> My proposed solution is to detect when the overlap happens, find the
> overlapping region and load it before the stack pushing into pseudos and
> after the stack pushing is done move the overlapping values from the pseudos
> into the hard argument regs that they're supposed to go.
> 
> That way this new functionality should only ever be triggered when there's
> the overlap in this PR (causing wrong-code) and shouldn't affect codegen
> anywhere else.
> 
> Bootstrapped and tested on arm-none-linux-gnueabihf, aarch64-none-linux-gnu
> and x86_64-linux-gnu.
> 
> According to the PR this appears at least as far back 4.6 so this isn't a
> regression on the release branches, but it is a wrong-code bug.
> 
> I'll let Honggyu upstream the testcase separately
> (https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00984.html)
> 
> I'll be testing this on the 4.8 and 4.9 branches.
> Thoughts on this approach?
> 
> Thanks,
> Kyrill
> 
Hi, Kyrill

I have verified the generated assembly code and tested on the target board.
PR 65358 testcase works fine now with your patch.
https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00984.html

Here is the generated assembly code of PR 65358:

foo:    (without this patch)    |    (with this patch)
        sub     sp, sp, #8      |    sub     sp, sp, #8
        mov     r0, r1          |    mov     r0, r1
        str     lr, [sp, #-4]!  |    str     lr, [sp, #-4]!
        add     ip, sp, #8      |    mov     r1, r2
(1)     ldr     lr, [sp, #16]   |    ldr     lr, [sp, #16]
        mov     r1, r2          |    mov     r2, r3
        str     r3, [sp, #8]    |    ldr     ip, [sp, #12]
(2)     str     lr, [sp, #12]   |    str     r3, [sp, #8]
        ldr     lr, [sp], #4    |    str     lr, [sp, #12]
        ldmia   ip, {r2, r3}    |    ldr     lr, [sp], #4
        add     sp, sp, #8      |    mov     r3, ip
        b       bar             |    add     sp, sp, #8
                                |    b       bar
 
One the left side(previous code), (1) loads "p.killer", then (2) overwrites
"p.victim" value.
Until this point, "p.victim" is never copied anyway, which makes the
value disappear.

But this bug is clearly fixed with this patch as shown on the right side.

I have tested on x86_64 and got the working code regardless of having
this patch.

I appreciate your patch, Kyrill.

Honggyu


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]