[Bug rtl-optimization/64010] New: [msp430-elf] struct function dereference clobbers parameter passed to function

[Bug rtl-optimization/64010] New: [msp430-elf] struct function dereference clobbers parameter passed to function

[amykyta3 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

            Bug ID: 64010
           Summary: [msp430-elf] struct function dereference clobbers
                    parameter passed to function
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: amykyta3 at gmail dot com

Created attachment 34061
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34061&action=edit
msp430-elf src file

Using TI's prepackaged msp430-elf-gcc (version 4.9.1 20140707)
Also verified with most recent build (version 5.0.0 20141121)

When compiling with -O1 or higher, optimization is clobbering the 'dbe' struct
pointer in the snippet below:

--------------------------------------------------------------------
void param_to_string(char *dst_str, uint8_t dst_size, convert_entry_t *dbe){
    dbe->to_string(dst_str, dst_size, dbe);
}
--------------------------------------------------------------------

When dereferencing to_string, the original dbe is overwritten and passed into
the to_string function.
Resulting disassembly:

--------------------------------------------------------------------
void param_to_string(char *dst_str, uint8_t dst_size, convert_entry_t *dbe){
    dbe->to_string(dst_str, dst_size, dbe);
    459c:    1e 4e 02 00     mov    2(r14),    r14    ;

000045a0 <.LVL11>:
    45a0:    8e 12           call    r14        ;

000045a2 <.LVL12>:
}
    45a2:    30 41           ret
--------------------------------------------------------------------

Full src file included for more context

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[nickc at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #3 from Nick Clifton <nickc at redhat dot com> ---
Hi Alex,

  This appears to be a reload bug.  Before reload we have:

(call_insn 12 (call:HI (mem:HI (mem:HI (plus:HI (reg:HI R14)
                                                (const_int 2))))
                       (const_int 0)) 

  After reload this becomes:

(insn 17 (set (reg:HI R14)
              (mem:HI (plus:HI (reg:HI R14)
                               (const_int 2)))))
(call_insn 12 (call:HI (mem:HI (reg:HI R14))
                       (const_int 0)))

  The uploaded patch fixes this but, not being a reload expert, I am not sure
if this is the correct way to solve the problem.  It also seems suspicious that
if this is a generic reload problem, then why has it not been reported and
fixed before now ?

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

Ulrich Weigand <uweigand at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |uweigand at gcc dot gnu.org

--- Comment #4 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Yes, this seems a generic reload bug.  The comment ahead of the lines you're
adding say:

If [...] the operand contains a register that dies in this insn *and is used
nowhere else* [...]

which is supposed to be implemented by this check:

            && ! refers_to_regno_for_reload_p (regno,
                                               end_hard_regno (rel_mode,
                                                               regno),
                                               PATTERN (this_insn), inloc)

But this doesn't look into registers used as function arguments.

I'm not sure why this hasn't occured elsewhere ... however, in your particular
case, it is triggered by a call insn pattern using memory-indirect addressing,
which is probably not available on many targets.

Your patch is a little too conservative, however: it rejects any register that
could potentially be used as function argument, even if it isn't actually used
in this particular call.

Can you check whether this alternative patch (using find_reg_fusage) also fixes
the problem for you?

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #5 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Created attachment 34170
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34170&action=edit
Do not clobber function argument registers

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[pab at pabigot dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #6 from Peter A. Bigot <pab at pabigot dot com> ---
The alternative patch (using find_reg_fusage) does fix the problem in my
reproducing test case and in a real application.  Thank you.

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[pab at pabigot dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #7 from Peter A. Bigot <pab at pabigot dot com> ---
I don't know if https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64160 is related;
it appears to be a similar problem affecting a value returned in multiple
registers.  It's present both before and after the alternative patch was
applied (now that the code that reset the target has been fixed).

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[amykyta3 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #8 from Alex Mykyta <amykyta3 at gmail dot com> ---
Also confirmed Ulrich's patch.
Thank you for taking care of this.

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[nickc at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #9 from Nick Clifton <nickc at redhat dot com> ---
Hi Ulrich,

  Thanks - ypur patch does work, and it is certainly better than mine.  Will
you be applying it to the gcc mainline sources ?  (And maybe the 4.9 branch as
well ?)

Cheers
  Nick

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #10 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Author: uweigand
Date: Wed Dec  3 21:59:10 2014
New Revision: 218335

URL: https://gcc.gnu.org/viewcvs?rev=218335&root=gcc&view=rev
Log:
    PR rtl-optimization/64010
    * reload.c (push_reload): Before reusing a register contained
    in an operand as input reload register, ensure that it is not
    used in CALL_INSN_FUNCTION_USAGE.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/reload.c

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #11 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Hi Nick,

I've checked this in to mainline now. I'd like to wait for a couple of days to
see if anything breaks before backporting ...

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #12 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Author: uweigand
Date: Wed Dec 17 15:07:28 2014
New Revision: 218821

URL: https://gcc.gnu.org/viewcvs?rev=218821&root=gcc&view=rev
Log:
2014-12-17  Ulrich Weigand  <Ulrich.Weigand@de.ibm.com>

    Backport from mainline
    2014-12-03  Ulrich Weigand  <Ulrich.Weigand@de.ibm.com>

    PR rtl-optimization/64010
    * reload.c (push_reload): Before reusing a register contained
    in an operand as input reload register, ensure that it is not
    used in CALL_INSN_FUNCTION_USAGE.


Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/reload.c

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

Ulrich Weigand <uweigand at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED
           Assignee|unassigned at gcc dot gnu.org      |uweigand at gcc dot gnu.org

--- Comment #14 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Fixed.

[Bug rtl-optimization/64010] [msp430-elf] struct function dereference clobbers parameter passed to function

[uweigand at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64010

--- Comment #13 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
Since this has been in mainline for two weeks without reported issues, and it
should in general be a safe change, I've backported the patch to 4.9 now.