[cfe-commits] r165273 - in /cfe/trunk: lib/CodeGen/CodeGenFunction.cpp test/CodeGen/catch-undef-behavior.c test/CodeGenCXX/catch-undef-behavior.cpp test/CodeGenCXX/return.cpp

[Chandler Carruth]

On Wed, Jan 23, 2013 at 10:27 AM, Daniel Dunbar <daniel at zuster.org> wrote:

>
>
>
> On Tue, Jan 22, 2013 at 5:55 PM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> On Tue, Jan 22, 2013 at 5:42 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com>wrote:
>>
>>> On Jan 22, 2013, at 5:23 PM, Chandler Carruth <chandlerc at google.com>
>>> wrote:
>>>
>>> On Tue, Jan 22, 2013 at 4:15 PM, Daniel Dunbar <daniel at zuster.org>wrote:
>>>
>>>> Hi Richard,
>>>>
>>>> I object pretty strongly to using unreachable here for C++ code bases.
>>>>
>>>
>>> There was some discussion surrounding this, and I'm still pretty
>>> strongly in favor of it...
>>>
>>>
>>> IMHO you still haven't given enough justification; your proposed
>>> optimization opportunity in
>>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-October/025326.html is
>>> specific to a full covered switch, which can be handled with an
>>> optimization that targets this case.
>>>
>>
>> It is applicable to any circumstance where there is a programmer-known
>> invariant that all paths terminate in a return which is not statically
>> determinable. I've seen both predicated code with if/else chains of this
>> form, and switches.
>>
>
> The right way to handle this is with appropriate annotations. This is
> simply not a good argument for emitting unreachable.
>
> But I actually think we're approaching this from the wrong direction. The
>> C++ standard is extremely clear that this is UB. Given that, I think we
>> should aggressively tell users about this problem with their code. If the
>> optimization benefits aren't worthwhile, then I would argue for emitting
>> llvm.trap in *all* cases rather than just in non-optimized builds. While I
>> think the optimization benefits are both easy to get and worth it, I don't
>> have a collection of benchmarks that rely on it so I'm not going to fight
>> tooth and nail for it. I just don't understand sacrificing it when we don't
>> have to.
>>
>
> I'd much prefer llvm.trap in all cases instead of unreachable, although I
> still don't see much value in forcing users to fix code that from their
> perspective "works".
>

The fundamental question is: do you want to try to define as a language
extension the behavior here? If so, we have a process for that, but I think
it will be a painful and fruitless one as I think this language behavior
was very well considered by the C++ committee. I do not think this behavior
was chosen in ignorance but in a desire that the language work in a certain
way.

If we don't want to define the behavior here, we do a disservice to the
users and to our selves to allow it to persist in the code. Eventually it
*will* be exploited to the users detriment and we should tell the user
about it quickly and effectively. This is the argument for using llvm.trap.
This is the argument I feel *very* strongly about.

The argument to switch from llvm.trap to unreachable in an optimized build
is simply one of principles: the user told us to optimize, and the language
gave us an opportunity. There isn't some practical, useful behavior being
blocked here. We should optimize these cases just as we optimize signed
integer overflow even if in the particular situation it didn't help the
performance or code size signficantly.


If you want to think about the extent to which this matters, consider
inlining. Once inlined, the undefined nature of this pattern is more likely
to result in an active bug, and thus the user will more greatly appreciate
the trap diagnosing it for them. Also, once inlined the unreachable has
much more power and can result in significant simplifications.

To sum up:
- if we aren't going to define the behavior, we should trap. This will make
both programs and future optimizations much more predictable.
- if we are going to trap, then when optimizing we should leverage that in
the hope of secondary and tertiary simplifications
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130123/f23d75d1/attachment.html>