| Message ID | 52A0937F.8020105@gjlay.de |
|---|---|
| State | New |
| Headers | show |
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 '*'.
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 '*'.
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 '*'. > >
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 '*'.
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 '*'.
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 '*'. > >
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 '*'.
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));