Re: [PATCH] Fix ICE with CET and -g (PR target/84146)

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Feb 6, 2018 at 3:57 PM, Tsimbalist, Igor V
<igor.v.tsimbalist@intel.com> wrote:
>> -----Original Message-----
>> From: Jakub Jelinek [mailto:jakub@redhat.com]
>> Sent: Wednesday, January 31, 2018 9:57 PM
>> To: Uros Bizjak <ubizjak@gmail.com>; Kirill Yukhin
>> <kirill.yukhin@gmail.com>
>> Cc: gcc-patches@gcc.gnu.org; Tsimbalist, Igor V
>> <igor.v.tsimbalist@intel.com>
>> Subject: [PATCH] Fix ICE with CET and -g (PR target/84146)
>>
>> Hi!
>>
>> We ICE on the following test because rest_of_insert_endbranch
>> separates a setjmp call from the following
>> NOTE_INSN_CALL_ARG_LOCATION
>> that must always immediately follow the call.
>> No other note or debug insn (which aren't around after var-tracking anyway)
>> needs to follow the call, so the loop it was doing is unnecessary, on the
>> other side, we are already at a late state where bb boundaries are fuzzy and
>> we are going to throw away the cfg before doing final.
>>
>> So, all we need is just check if the call is followed by this note and
>> if yes, emit the endbr after it.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Just in case, ok from CET viewpoint.

LGTM, now that we have review from the author.

Thanks,
Uros.

> Thanks,
> Igor
>
>> 2018-01-31  Jakub Jelinek  <jakub@redhat.com>
>>
>>       PR target/84146
>>       * config/i386/i386.c (rest_of_insert_endbranch): Only skip
>>       NOTE_INSN_CALL_ARG_LOCATION after a call, not anything else,
>>       and skip it regardless of bb boundaries.  Use CALL_P macro,
>>       don't test INSN_P (insn) together with CALL_P or JUMP_P check
>>       unnecessarily, formatting fix.
>>
>>       * gcc.target/i386/pr84146.c: New test.
>>
>> --- gcc/config/i386/i386.c.jj 2018-01-31 09:26:18.341505667 +0100
>> +++ gcc/config/i386/i386.c    2018-01-31 14:13:33.815243832 +0100
>> @@ -2609,31 +2609,27 @@ rest_of_insert_endbranch (void)
>>        for (insn = BB_HEAD (bb); insn != NEXT_INSN (BB_END (bb));
>>          insn = NEXT_INSN (insn))
>>       {
>> -       if (INSN_P (insn) && GET_CODE (insn) == CALL_INSN)
>> +       if (CALL_P (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.  */
>> +           /* Skip notes that must immediately follow the call insn.  */
>>             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);
>> +           if (NEXT_INSN (insn)
>> +               && NOTE_P (NEXT_INSN (insn))
>> +               && (NOTE_KIND (NEXT_INSN (insn))
>> +                   == NOTE_INSN_CALL_ARG_LOCATION))
>> +             next_insn = NEXT_INSN (insn);
>>
>>             cet_eb = gen_nop_endbr ();
>>             emit_insn_after_setloc (cet_eb, next_insn, INSN_LOCATION
>> (insn));
>>             continue;
>>           }
>>
>> -       if (INSN_P (insn) && JUMP_P (insn) && flag_cet_switch)
>> +       if (JUMP_P (insn) && flag_cet_switch)
>>           {
>>             rtx target = JUMP_LABEL (insn);
>>             if (target == NULL_RTX || ANY_RETURN_P (target))
>> @@ -2668,7 +2664,7 @@ rest_of_insert_endbranch (void)
>>         if ((LABEL_P (insn) && LABEL_PRESERVE_P (insn))
>>             || (NOTE_P (insn)
>>                 && NOTE_KIND (insn) == NOTE_INSN_DELETED_LABEL))
>> -/* TODO.  Check /s bit also.  */
>> +         /* TODO.  Check /s bit also.  */
>>           {
>>             cet_eb = gen_nop_endbr ();
>>             emit_insn_after (cet_eb, insn);
>> --- gcc/testsuite/gcc.target/i386/pr84146.c.jj        2018-01-31
>> 16:32:28.099929916 +0100
>> +++ gcc/testsuite/gcc.target/i386/pr84146.c   2018-01-31
>> 14:04:17.796122397 +0100
>> @@ -0,0 +1,14 @@
>> +/* PR target/84146 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -g -mcet -fcf-protection=full" } */
>> +
>> +int __setjmp (void **);
>> +void *buf[64];
>> +
>> +void
>> +foo (void)
>> +{
>> +  __setjmp (buf);
>> +  for (;;)
>> +    ;
>> +}
>>
>>       Jakub