[compare-debug] use call loc for nop_endbr

Message ID orindb6pz5.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • [compare-debug] use call loc for nop_endbr
Related show

Commit Message

Alexandre Oliva Dec. 13, 2017, 7:34 a.m.
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?

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(-)


-- 
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

Comments

Jakub Jelinek Dec. 13, 2017, 8:47 a.m. | #1
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
Tsimbalist, Igor V Dec. 14, 2017, 12:34 p.m. | #2
> -----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
Alexandre Oliva Dec. 14, 2017, 6:36 p.m. | #3
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.

-- 
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
Tsimbalist, Igor V Dec. 15, 2017, 3:17 p.m. | #4
> -----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
H.J. Lu Dec. 15, 2017, 3:33 p.m. | #5
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


-- 
H.J.
Alexandre Oliva Dec. 19, 2017, 5:44 p.m. | #6
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!

-- 
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

Patch

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;
 	    }