[Patch,avr]: Fix wrong warning PR59396

[Patch,avr]: Fix wrong warning PR59396

[Georg-Johann Lay]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

This is a fix of a wrong warning for a bas ISR name.  The assumption was that 
if DECL_ASSEMBLER_NAME is set, it would always starts with a *.

This is not the case for LTO compiler where the assembler name is the plain 
name of the function (except an assembler name is set).

Thus, do a more restrictive test if the first character of the function name 
has to be skipped.

Ok to commit?

Johann


	PR target/59396
	* config/avr/avr.c (avr_set_current_function): If the first char
	of the function name is skipped, make sure it is actually '*'.

[-- Attachment #2: pr59396.diff --]
[-- Type: text/x-patch, Size: 655 bytes --]

Index: config/avr/avr.c
===================================================================
--- config/avr/avr.c	(revision 205709)
+++ config/avr/avr.c	(working copy)
@@ -599,7 +599,8 @@ avr_set_current_function (tree decl)
       tree ret = TREE_TYPE (TREE_TYPE (decl));
       const char *name;
 
-      name = DECL_ASSEMBLER_NAME_SET_P (decl)
+      name = (DECL_ASSEMBLER_NAME_SET_P (decl)
+              && '*' == *IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl)))
         /* Remove the leading '*' added in set_user_assembler_name.  */
         ? 1 + IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl))
         : IDENTIFIER_POINTER (DECL_NAME (decl));

Re: [Patch,avr]: Fix wrong warning PR59396

[Richard Biener]

On Thu, Dec 5, 2013 at 3:53 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
> This is a fix of a wrong warning for a bas ISR name.  The assumption was
> that if DECL_ASSEMBLER_NAME is set, it would always starts with a *.
>
> This is not the case for LTO compiler where the assembler name is the plain
> name of the function (except an assembler name is set).

That sounds odd to me.  Does the bug reproduce with -fwhole-program?
Or if the interrupt handler is static?

Richard.

> Thus, do a more restrictive test if the first character of the function name
> has to be skipped.
>
> Ok to commit?
>
> Johann
>
>
>         PR target/59396
>         * config/avr/avr.c (avr_set_current_function): If the first char
>         of the function name is skipped, make sure it is actually '*'.

Re: [Patch,avr]: Fix wrong warning PR59396

[Georg-Johann Lay]

Am 12/05/2013 04:09 PM, schrieb Richard Biener:
> On Thu, Dec 5, 2013 at 3:53 PM, Georg-Johann Lay wrote:
>> This is a fix of a wrong warning for a bas ISR name.  The assumption was
>> that if DECL_ASSEMBLER_NAME is set, it would always starts with a *.
>>
>> This is not the case for LTO compiler where the assembler name is the plain
>> name of the function (except an assembler name is set).
>
> That sounds odd to me.  Does the bug reproduce with -fwhole-program?
> Or if the interrupt handler is static?
>
> Richard.

Yes, with -fwhole-program the issue persists (except -flto is removed).

Same with a static function, both with and without externally_visible.  Because 
externally_visible conflicts static, I also tried without externally_visible, 
but with no avail.

Johann


>> Thus, do a more restrictive test if the first character of the function name
>> has to be skipped.
>>
>> Ok to commit?
>>
>> Johann
>>
>>
>>          PR target/59396
>>          * config/avr/avr.c (avr_set_current_function): If the first char
>>          of the function name is skipped, make sure it is actually '*'.

Re: [Patch,avr]: Fix wrong warning PR59396

[Richard Biener]

On Thu, Dec 5, 2013 at 4:38 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
> Am 12/05/2013 04:09 PM, schrieb Richard Biener:
>
>> On Thu, Dec 5, 2013 at 3:53 PM, Georg-Johann Lay wrote:
>>>
>>> This is a fix of a wrong warning for a bas ISR name.  The assumption was
>>> that if DECL_ASSEMBLER_NAME is set, it would always starts with a *.
>>>
>>> This is not the case for LTO compiler where the assembler name is the
>>> plain
>>> name of the function (except an assembler name is set).
>>
>>
>> That sounds odd to me.  Does the bug reproduce with -fwhole-program?
>> Or if the interrupt handler is static?
>>
>> Richard.
>
>
> Yes, with -fwhole-program the issue persists (except -flto is removed).
>
> Same with a static function, both with and without externally_visible.
> Because externally_visible conflicts static, I also tried without
> externally_visible, but with no avail.

Ah, I was asking if the issue is not LTO but us bringing the interrupt
handler local - which should be reproducible without LTO with just
-fwhole-program or by making the interrupt handler local in the
first place (by declaring it static).  Now your first sentence says
that -fwhole-program alone does not reproduce the issue, right?

Richard.

> Johann
>
>
>
>>> Thus, do a more restrictive test if the first character of the function
>>> name
>>> has to be skipped.
>>>
>>> Ok to commit?
>>>
>>> Johann
>>>
>>>
>>>          PR target/59396
>>>          * config/avr/avr.c (avr_set_current_function): If the first char
>>>          of the function name is skipped, make sure it is actually '*'.
>
>

Re: [Patch,avr]: Fix wrong warning PR59396

[Georg-Johann Lay]

Am 12/06/2013 10:32 AM, schrieb Richard Biener:
> On Thu, Dec 5, 2013 at 4:38 PM, Georg-Johann Lay wrote:
>> Am 12/05/2013 04:09 PM, schrieb Richard Biener:
>>
>>> On Thu, Dec 5, 2013 at 3:53 PM, Georg-Johann Lay wrote:
>>>>
>>>> This is a fix of a wrong warning for a bas ISR name.  The assumption was
>>>> that if DECL_ASSEMBLER_NAME is set, it would always starts with a *.
>>>>
>>>> This is not the case for LTO compiler where the assembler name is the
>>>> plain
>>>> name of the function (except an assembler name is set).
>>>
>>>
>>> That sounds odd to me.  Does the bug reproduce with -fwhole-program?
>>> Or if the interrupt handler is static?
>>>
>>> Richard.
>>
>>
>> Yes, with -fwhole-program the issue persists (except -flto is removed).
>>
>> Same with a static function, both with and without externally_visible.
>> Because externally_visible conflicts static, I also tried without
>> externally_visible, but with no avail.
>
> Ah, I was asking if the issue is not LTO but us bringing the interrupt
> handler local - which should be reproducible without LTO with just
> -fwhole-program or by making the interrupt handler local in the
> first place (by declaring it static).  Now your first sentence says
> that -fwhole-program alone does not reproduce the issue, right?

Right.

> Richard.
>
>> Johann
>>
>>
>>
>>>> Thus, do a more restrictive test if the first character of the function
>>>> name
>>>> has to be skipped.
>>>>
>>>> Ok to commit?
>>>>
>>>> Johann
>>>>
>>>>
>>>>           PR target/59396
>>>>           * config/avr/avr.c (avr_set_current_function): If the first char
>>>>           of the function name is skipped, make sure it is actually '*'.

Re: [Patch,avr]: Fix wrong warning PR59396

[Georg-Johann Lay]

Am 12/05/2013 04:09 PM, schrieb Richard Biener:
> On Thu, Dec 5, 2013 at 3:53 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
>> This is a fix of a wrong warning for a bas ISR name.  The assumption was
>> that if DECL_ASSEMBLER_NAME is set, it would always starts with a *.
>>
>> This is not the case for LTO compiler where the assembler name is the plain
>> name of the function (except an assembler name is set).
>
> That sounds odd to me.  Does the bug reproduce with -fwhole-program?
> Or if the interrupt handler is static?

Hi, I tried to debug lto1.

What I see is that SET_DECL_ASSEMBLER_NAME with "__vector_14", i.e. without a 
leading '*', is called from

tree-streamer-in.c:lto_input_ts_decl_with_vis_tree_pointers().

Hope that helps in narrowing down the issue.

Johann

> Richard.
>
>> Thus, do a more restrictive test if the first character of the function name
>> has to be skipped.
>>
>> Ok to commit?
>>
>> Johann
>>
>>          PR target/59396
>>          * config/avr/avr.c (avr_set_current_function): If the first char
>>          of the function name is skipped, make sure it is actually '*'.

Re: [Patch,avr]: Fix wrong warning PR59396

[Richard Biener]

On Tue, Dec 17, 2013 at 2:05 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
> Am 12/05/2013 04:09 PM, schrieb Richard Biener:
>>
>> On Thu, Dec 5, 2013 at 3:53 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>
>>> This is a fix of a wrong warning for a bas ISR name.  The assumption was
>>> that if DECL_ASSEMBLER_NAME is set, it would always starts with a *.
>>>
>>> This is not the case for LTO compiler where the assembler name is the
>>> plain
>>> name of the function (except an assembler name is set).
>>
>>
>> That sounds odd to me.  Does the bug reproduce with -fwhole-program?
>> Or if the interrupt handler is static?
>
>
> Hi, I tried to debug lto1.
>
> What I see is that SET_DECL_ASSEMBLER_NAME with "__vector_14", i.e. without
> a leading '*', is called from
>
> tree-streamer-in.c:lto_input_ts_decl_with_vis_tree_pointers().
>
> Hope that helps in narrowing down the issue.

You need to debug the LTO IL creating process (cc1) then - this code merely
restores what the compile-stage assigned the assembler name to.  See
tree.c:assign_assembler_name_if_needed.

Richard.

> Johann
>
>
>> Richard.
>>
>>> Thus, do a more restrictive test if the first character of the function
>>> name
>>> has to be skipped.
>>>
>>> Ok to commit?
>>>
>>> Johann
>>>
>>>          PR target/59396
>>>          * config/avr/avr.c (avr_set_current_function): If the first char
>>>          of the function name is skipped, make sure it is actually '*'.
>
>

Re: [Patch,avr]: Fix wrong warning PR59396

[Georg-Johann Lay]

Am 12/18/2013 12:37 PM, schrieb Richard Biener:
> On Tue, Dec 17, 2013 at 2:05 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
>> Am 12/05/2013 04:09 PM, schrieb Richard Biener:
>>>
>>> On Thu, Dec 5, 2013 at 3:53 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>>
>>>> This is a fix of a wrong warning for a bas ISR name.  The assumption was
>>>> that if DECL_ASSEMBLER_NAME is set, it would always starts with a *.
>>>>
>>>> This is not the case for LTO compiler where the assembler name is the
>>>> plain
>>>> name of the function (except an assembler name is set).
>>>
>>>
>>> That sounds odd to me.  Does the bug reproduce with -fwhole-program?
>>> Or if the interrupt handler is static?
>>
>>
>> Hi, I tried to debug lto1.
>>
>> What I see is that SET_DECL_ASSEMBLER_NAME with "__vector_14", i.e. without
>> a leading '*', is called from
>>
>> tree-streamer-in.c:lto_input_ts_decl_with_vis_tree_pointers().
>>
>> Hope that helps in narrowing down the issue.
>
> You need to debug the LTO IL creating process (cc1) then - this code merely
> restores what the compile-stage assigned the assembler name to.  See
> tree.c:assign_assembler_name_if_needed.
>
> Richard.

Seems that assembler_name is not only used for names set with asm() but also 
(ab)used in many other occasions like C++ or setting the name of libgcc 
function calls.

The easiest and least intrusive fix is still in the avr backend as proposed in

http://gcc.gnu.org/ml/gcc-patches/2013-12/msg00529.html

in particular because the assembler_name bits are not well documented, e.g. in 
tree.h.

Or do you know an other way to fix this without any side effects on other 
target / languages?


Johann


>>>> Thus, do a more restrictive test if the first character of the function
>>>> name
>>>> has to be skipped.
>>>>
>>>> Ok to commit?
>>>>
>>>> Johann
>>>>
>>>>           PR target/59396
>>>>           * config/avr/avr.c (avr_set_current_function): If the first char
>>>>           of the function name is skipped, make sure it is actually '*'.