[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

[Daniel Dunbar]

On Wed, Jan 23, 2013 at 10:44 AM, Chandler Carruth <chandlerc at google.com>wrote:

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

No, but just because we choose not to use unreachable doesn't mean we are
defining it as a language extension, it just means its  an implementation
choice.


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

I don't understand why you are pursuing this rabbit hole, just because the
language defines something as undefined doesn't mean the compiler should
implement it that way (and obviously we don't do this).

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

I don't agree with this statement. It is in our interest to provide good
tools to help users find UB. We do that. It is not necessarily in our
interest to *force* users to find all cases of UB.

That may well be in your interest for your use case at Google, but that is
a policy decision you can enforce in other ways (ubsan, warning policy,
etc).

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

I don't agree with this, as before.

You are failing to give good arguments for why this behavior benefits our
users. From my perspective, the important question here is "what value is
there to the user of emitting unreachable". I haven't heard a good argument
for this.

Note that from a user's perspective, just "helping" them find undefined
behavior is not value. We have lots of good tools for helping them do that
(static analyzer, compiler warnings, ubsan), and so users who want to do
that can do so. But there are many code bases and users for whom that is
not a priority, and forcing them to do so is actively hurting them. That is
an okay tradeoff if there are strong benefits in other ways
(strict-aliasing), but if there are not it is just causing them to do
"unnecessary" work.

 - Daniel

- 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/ce56b73d/attachment.html>