| Message ID | orindb6pz5.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [compare-debug] use call loc for nop_endbr | expand |
On Wed, Dec 13, 2017 at 05:34:22AM -0200, Alexandre Oliva wrote: > We skip debug insns and notes after a call that needs a nop_endbr, but > since a debug insn could be the last in a block, it may affect the loc > in the emitted nop_endbr insn. Although this has no effect on > codegen, it does mess with debug info a bit, and it causes > -fcompare-debug to fail for e.g. libsanitizer's > tsan/tsan_platform_linux.cc on x86_64. > > So, pick the location of the call insn for the nop_endbr insn, to > avoid the line number differences in dumps, including -fcompare-debug > ones. > > Also, we don't need to determine what the insert point would be unless > we're actually emitting the nop_endbr insn after the call, so > rearrange the code to avoid wasting cycles. > > Finally, it seems like testing for barriers is a mistake. We probably > never actually pass that test, for the barriers would hit BB_END > first. If we did, we'd end up emitting the nop_endbr outside any BB, > even after the end of the function! That would be Very Bad (TM). > Now, since the test as it is can't hurt, I figured I wouldn't change > the logic right now, just add a comment so that someone involved in > endbr stuff can have a second look and hopefully fix it. > > I'd appreciate if you'd try to drop the BARRIER_P from the loop test, > Igor, so as to address the final ??? in the comment I add. Narrowing > the skipped notes to only the relevant post-call ones might make sense > as well, but it's not quite as important IMHO. I believe the only insn that needs to be skipped is NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_CALL_ARG_LOCATION and there should be at most one of these after the call. Anything else I believe can be separated from the call without problems. Jakub
> -----Original Message----- > From: Alexandre Oliva [mailto:aoliva@redhat.com] > Sent: Wednesday, December 13, 2017 8:34 AM > To: gcc-patches@gcc.gnu.org > Cc: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > Subject: [compare-debug] use call loc for nop_endbr > > We skip debug insns and notes after a call that needs a nop_endbr, but > since a debug insn could be the last in a block, it may affect the loc > in the emitted nop_endbr insn. Although this has no effect on > codegen, it does mess with debug info a bit, and it causes > -fcompare-debug to fail for e.g. libsanitizer's > tsan/tsan_platform_linux.cc on x86_64. > > So, pick the location of the call insn for the nop_endbr insn, to > avoid the line number differences in dumps, including -fcompare-debug > ones. > > Also, we don't need to determine what the insert point would be unless > we're actually emitting the nop_endbr insn after the call, so > rearrange the code to avoid wasting cycles. > > Finally, it seems like testing for barriers is a mistake. We probably > never actually pass that test, for the barriers would hit BB_END > first. If we did, we'd end up emitting the nop_endbr outside any BB, > even after the end of the function! That would be Very Bad (TM). > Now, since the test as it is can't hurt, I figured I wouldn't change > the logic right now, just add a comment so that someone involved in > endbr stuff can have a second look and hopefully fix it. > > I'd appreciate if you'd try to drop the BARRIER_P from the loop test, > Igor, so as to address the final ??? in the comment I add. Narrowing > the skipped notes to only the relevant post-call ones might make sense > as well, but it's not quite as important IMHO. > > Regstrapping with -fcompare-debug on stage3 host and target builds on > x86_64- and i686-linux-gnu; ok to install? Ok from me. Am I correct the error you had was related to improper location information, not the placement of the instruction? I will try to skip NOTE insns only. Igor > for gcc/ChangeLog > > * config/i386/i386.c (rest_of_insert_endbranch): Use call loc > for its nop_endbr. > --- > gcc/config/i386/i386.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > index e323102cef59..8960b966b7fc 100644 > --- a/gcc/config/i386/i386.c > +++ b/gcc/config/i386/i386.c > @@ -2609,21 +2609,25 @@ rest_of_insert_endbranch (void) > { > if (INSN_P (insn) && GET_CODE (insn) == CALL_INSN) > { > - rtx_insn *next_insn = insn; > + if (find_reg_note (insn, REG_SETJMP, NULL) == NULL) > + continue; > + /* Generate ENDBRANCH after CALL, which can return more than > + twice, setjmp-like functions. */ > > + /* Skip notes and debug insns that must be next to the > + call insn. ??? This might skip a lot more than > + that... ??? Skipping barriers and emitting code > + after them surely looks like a mistake; we probably > + won't ever hit it, for we'll hit BB_END first. */ > + rtx_insn *next_insn = insn; > while ((next_insn != BB_END (bb)) > && (DEBUG_INSN_P (NEXT_INSN (next_insn)) > || NOTE_P (NEXT_INSN (next_insn)) > || BARRIER_P (NEXT_INSN (next_insn)))) > next_insn = NEXT_INSN (next_insn); > > - /* Generate ENDBRANCH after CALL, which can return more than > - twice, setjmp-like functions. */ > - if (find_reg_note (insn, REG_SETJMP, NULL) != NULL) > - { > - cet_eb = gen_nop_endbr (); > - emit_insn_after (cet_eb, next_insn); > - } > + cet_eb = gen_nop_endbr (); > + emit_insn_after_setloc (cet_eb, next_insn, INSN_LOCATION > (insn)); > continue; > } > > > -- > Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
On Dec 14, 2017, "Tsimbalist, Igor V" <igor.v.tsimbalist@intel.com> wrote: >> Regstrapping with -fcompare-debug on stage3 host and target builds on >> x86_64- and i686-linux-gnu; ok to install? > Ok from me. Thanks, I went ahead and installed it. > Am I correct the error you had was related to improper location information, Yeah, only location information. > I will try to skip NOTE insns only. You probably want to skip debug insns and notes, too. Actually, IIRC you insert these insns after var-tracking, so you probably only have to deal with notes. You don't have to, but if bindings are intended to take effect right after the call, it would probably be nice if they still did so, e.g., even if you happen to single-step out of the call and stop at the nop_endbr insn. BTW, is this the subject of a Cauldron 2017 talk in which I raised an issue about PLT entries possibly needing special opcodes to enable them to be used as call targets or somesuch? I had initially retracted my question, when it was stated that only indirect calls needed special treatment, but later I realized that in some cases PLT entries *are* used as function addresses even for functions that have their addresses taken. Please let me know if you're familiar with the issue and would like me to detail the problem.
> -----Original Message----- > From: Alexandre Oliva [mailto:aoliva@redhat.com] > Sent: Thursday, December 14, 2017 7:37 PM > To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [compare-debug] use call loc for nop_endbr > > On Dec 14, 2017, "Tsimbalist, Igor V" <igor.v.tsimbalist@intel.com> wrote: > > >> Regstrapping with -fcompare-debug on stage3 host and target builds on > >> x86_64- and i686-linux-gnu; ok to install? > > > Ok from me. > > Thanks, I went ahead and installed it. > > > Am I correct the error you had was related to improper location > information, > > Yeah, only location information. > > > I will try to skip NOTE insns only. > > You probably want to skip debug insns and notes, too. Actually, IIRC > you insert these insns after var-tracking, so you probably only have to > deal with notes. You don't have to, but if bindings are intended to > take effect right after the call, it would probably be nice if they > still did so, e.g., even if you happen to single-step out of the call > and stop at the nop_endbr insn. Yes, I expect this behavior. > BTW, is this the subject of a Cauldron 2017 talk in which I raised an > issue about PLT entries possibly needing special opcodes to enable them > to be used as call targets or somesuch? I had initially retracted my > question, when it was stated that only indirect calls needed special > treatment, but later I realized that in some cases PLT entries *are* > used as function addresses even for functions that have their addresses > taken. Please let me know if you're familiar with the issue and would > like me to detail the problem. Please give more info. I do not remember all details but PLT entries were changes to have endbr instruction (if this is relevant to your question :). HJ did this. Thanks, Igor > -- > Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
On Fri, Dec 15, 2017 at 7:17 AM, Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> wrote: >> -----Original Message----- >> From: Alexandre Oliva [mailto:aoliva@redhat.com] >> Sent: Thursday, December 14, 2017 7:37 PM >> To: Tsimbalist, Igor V <igor.v.tsimbalist@intel.com> >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: [compare-debug] use call loc for nop_endbr >> >> On Dec 14, 2017, "Tsimbalist, Igor V" <igor.v.tsimbalist@intel.com> wrote: >> >> >> Regstrapping with -fcompare-debug on stage3 host and target builds on >> >> x86_64- and i686-linux-gnu; ok to install? >> >> > Ok from me. >> >> Thanks, I went ahead and installed it. >> >> > Am I correct the error you had was related to improper location >> information, >> >> Yeah, only location information. >> >> > I will try to skip NOTE insns only. >> >> You probably want to skip debug insns and notes, too. Actually, IIRC >> you insert these insns after var-tracking, so you probably only have to >> deal with notes. You don't have to, but if bindings are intended to >> take effect right after the call, it would probably be nice if they >> still did so, e.g., even if you happen to single-step out of the call >> and stop at the nop_endbr insn. > Yes, I expect this behavior. > >> BTW, is this the subject of a Cauldron 2017 talk in which I raised an >> issue about PLT entries possibly needing special opcodes to enable them >> to be used as call targets or somesuch? I had initially retracted my >> question, when it was stated that only indirect calls needed special >> treatment, but later I realized that in some cases PLT entries *are* >> used as function addresses even for functions that have their addresses >> taken. Please let me know if you're familiar with the issue and would >> like me to detail the problem. > Please give more info. I do not remember all details but PLT entries > were changes to have endbr instruction (if this is relevant to your question :). > HJ did this. PLT is covered. See x86 psABI for CET: https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI
On Dec 15, 2017, "Tsimbalist, Igor V" <igor.v.tsimbalist@intel.com> wrote: > Please give more info. I do not remember all details but PLT entries > were changes to have endbr instruction (if this is relevant to your question :). Yeah, that was this was about. Good to know it's covered. Thanks!
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index e323102cef59..8960b966b7fc 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -2609,21 +2609,25 @@ rest_of_insert_endbranch (void) { if (INSN_P (insn) && GET_CODE (insn) == CALL_INSN) { - rtx_insn *next_insn = insn; + if (find_reg_note (insn, REG_SETJMP, NULL) == NULL) + continue; + /* Generate ENDBRANCH after CALL, which can return more than + twice, setjmp-like functions. */ + /* Skip notes and debug insns that must be next to the + call insn. ??? This might skip a lot more than + that... ??? Skipping barriers and emitting code + after them surely looks like a mistake; we probably + won't ever hit it, for we'll hit BB_END first. */ + rtx_insn *next_insn = insn; while ((next_insn != BB_END (bb)) && (DEBUG_INSN_P (NEXT_INSN (next_insn)) || NOTE_P (NEXT_INSN (next_insn)) || BARRIER_P (NEXT_INSN (next_insn)))) next_insn = NEXT_INSN (next_insn); - /* Generate ENDBRANCH after CALL, which can return more than - twice, setjmp-like functions. */ - if (find_reg_note (insn, REG_SETJMP, NULL) != NULL) - { - cet_eb = gen_nop_endbr (); - emit_insn_after (cet_eb, next_insn); - } + cet_eb = gen_nop_endbr (); + emit_insn_after_setloc (cet_eb, next_insn, INSN_LOCATION (insn)); continue; }