This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add new built-in: __builtin_unreachable()


On Tue, Jun 9, 2009 at 12:14 PM, David Daney<ddaney@caviumnetworks.com> wrote:
> Richard Henderson wrote:
>>
>> David Daney wrote:
>>>
>>> Ian Lance Taylor wrote:
>>>>
>>>> David Daney <ddaney@caviumnetworks.com> writes:
>>>>
>>>>> +@deftypefn {Built-in Function} void __builtin_unreachable (void)
>>>>> +This function is used to indicate to the compiler that control flow
>>>>> +will never reach the point of the @code{__builtin_unreachable}. ?If
>>>>> +control flow does reach @code{__builtin_unreachable}, program behavior
>>>>> +is undefined. ?The only valid use of this builtin is immediately
>>>>> +following an @code{asm} statement that either never exits or transfers
>>>>> +control elsewhere never returning.
>>>>
>>>> I have wanted __builtin_unreachable for a while, but never got around to
>>>> doing anything. ?But I only want it if it works in general, not just
>>>> after an asm statement. ?I think it should mean "if control flow reaches
>>>> this point, the program is undefined." ?Ideally there should be a
>>>> command line option to control it, so that it is possible to compile
>>>> __builtin_unreachable as expanding to abort, or something like that,
>>>> when debugging.
>>>>
>>>
>>> Ok, then how about this version? ?All my original commentary still
>>> applies: http://gcc.gnu.org/ml/gcc-patches/2009-06/msg00317.html
>>>
>>> I have added a new command line flag (-funreachable-traps) that causes
>>> __builtin_unreachable() to be expanded exactly as if it were a
>>> __builtin_trap().
>>>
>>>
>>> Tested by bootstrapping and regression testing all default languages on
>>> x86_64-pc-linux-gnu with no regressions. ?Also tested by building and
>>> running the Linux kernel for x86_64 and mips64 after replacing the for(;;)
>>> with __builtin_unreachable() in their respective BUG() macros/functions.
>>>
>>> OK to commit?
>>>
>>> gcc/
>>> 2009-06-08 ?David Daney ?<ddaney@caviumnetworks.com>
>>>
>>> ? ?* doc/extend.texi ( __builtin_unreachable): Document new builtin.
>>> ? ?* doc/invoke.texi (-funreachable-traps): Document new option.
>>> ? ?* doc/rtl.texi (NOTE_INSN_UNREACHABLE): Document new note type.
>>> ? ?* final.c (final_scan_insn): Handle NOTE_INSN_UNREACHABLE.
>>> ? ?* builtins.c (expand_builtin_unreachable): New function.
>>> ? ?(expand_builtin): Handle BUILT_IN_UNREACHABLE case.
>>> ? ?* insn-notes.def (UNREACHABLE): Add new INSN_NOTE type.
>>> ? ?* builtins.def (BUILT_IN_UNREACHABLE): Add new builtin.
>>> ? ?* jump.c (cleanup_barriers): Don't reorder NOTE_INSN_UNREACHABLE
>>> ? ?across a barrier.
>>> ? ?* cfgbuild.c (control_flow_insn_p): Return true for
>>> ? ?NOTE_INSN_UNREACHABLE.
>>> ? ?* cfgcleanup.c (old_insns_match_p): Return true if both insns are
>>> ? ?NOTE_INSN_UNREACHABLE.
>>> ? ?* common.opt (funreachable-traps): Add new option.
>>> ? ?* rtl.h (NOTE_INSN_UNREACHABLE_P): New macro.
>>> ? ?* cfgrtl.c (can_delete_note_p): Return true for NOTE_INSN_UNREACHABLE.
>>>
>>> gcc/testsuite/
>>> 2009-06-08 ?David Daney ?<ddaney@caviumnetworks.com>
>>>
>>> ? ?* gcc.dg/builtin-unreachable-1.c: New test.
>>> ? ?* gcc.dg/builtin-unreachable-2.c: Same.
>>> ? ?* gcc.dg/builtin-unreachable-3.c: Same.
>>>
>>
>>
>>> - ? ? ?if (BARRIER_P (insn))
>>> + ? ? ?if (BARRIER_P (insn) && prev && !NOTE_INSN_UNREACHABLE_P (prev))
>>
>> I can tell you that I don't like note-followed-by-barrier meaning
>> something different than barrier by itself. ?If we wanted a special
>> kind of barrier, I would prefer a flag on the barrier node itself.
>>
>> That said, I can't figure out why your change to cleanup_barriers
>> affects anything that would cause crash. ?Was this merely to prevent
>> the barrier from being separated from its note?
>
> Yes. ?At this point it doesn't really matter, but while debugging the patch,
> the note was escaping in a different situation and causing CFG verification
> to fail. ?I had wanted to prevent that from happening again. ?But my new
> patch doesn't use the note so it is moot.
>
>>
>>> @@ -84,6 +84,9 @@ control_flow_insn_p (const_rtx insn)
>>> ? switch (GET_CODE (insn))
>>> ? ? {
>>> ? ? case NOTE:
>>> + ? ? ?/* __builtin_unreachable is treated like a noreturn function. */
>>> + ? ? ?return NOTE_INSN_UNREACHABLE_P (insn);
>>
>> Egads. ?Definitely unacceptable.
>>
>> Let's try
>>
>> #define BARRIER_BUILTIN_UNREACHABLE(X) \
>> ?(RTL_FLAG_CHECK1("BARRIER_BUILTIN_UNREACHABLE", (X), BARRIER)->volatil)
>>
>
> I think it is not needed.
>
> My initial patch just emitted the barrier, but was causing CFG verification
> to fail in the case of a block that was empty because it contained only a
> __builtin_unreachable(). ?My first attempt to fix the empty block problem
> was to add the NOTE_INSN_UNREACHABLE that you found objectionable. ?Since no
> amount of extra flags on the barrier would solve the empty block issue, I
> think the best approach is to fix the CFG verifier. ?That leads to this new
> patch.
>
> This is exactly the first version of the patch, but we fix
> rtl_verify_flow_info() so that it properly handles empty blocks when
> searching for missing barriers.
>
> Tested by bootstrapping on x86_64-pc-linux-gnu all default languages with no
> regressions. ?Also tested by building and running the Linux kernel for
> x86_64 and mips64 after replacing the for(;;) with __builtin_unreachable()
> in their respective BUG() macros/functions.
>
> Ok to commit?
>
> gcc/
> 2009-06-09 ?David Daney ?<ddaney@caviumnetworks.com>
>
> ? ? ? ?* doc/extend.texi ( __builtin_unreachable): Document new builtin.
> ? ? ? ?* builtins.c (expand_builtin_unreachable): New function.
> ? ? ? ?(expand_builtin): Handle BUILT_IN_UNREACHABLE case.
> ? ? ? ?* builtins.def (BUILT_IN_UNREACHABLE): Add new builtin.
> ? ? ? ?* cfgrtl.c (rtl_verify_flow_info): Handle empty blocks when
> ? ? ? ?searching for missing barriers.
>
> gcc/testsuite/
> 2009-06-09 ?David Daney ?<ddaney@caviumnetworks.com>
>
> ? ? ? ?* gcc.dg/builtin-unreachable-1.c: New test.
> ? ? ? ?* gcc.dg/builtin-unreachable-2.c: Same.


Does this implement

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=39252

If yes, could you please update PR and include PR # in ChangeLog?

Thanks.


H.J.
---


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]