[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

[John McCall]

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

To be clear, there are divisions here.  I am, personally, in favor of exposing a -fno-strict-return flag to disable exploitation of missing-return violations, analogous to how we expose a flag to disable exploitation of aliasing violations.
 
> The problem is that this can (and has, and will) subtly break code in ways users might not notice.
> 
> I don't entirely understand this specific concern -- in non-optimized builds this becomes a trap pointing at the end of the function which lacked a return. While optimized builds can certainly fail in subtle ways, the non-optimized build seems to fail in a very understandable way here.

This is a bit disingenuous, because both of these behaviors are new to this change.  Our previous behavior matches the behavior of (AFAIK) every other compiler in existence by just following the C semantics.  I do think that optimizers need to be careful about exploiting undefined behavior in ways that weren't previously exploited.  For example, it is undefined behavior to load an uninitialized variable in both C++ ([conv.lval]p1) and C ([6.3.2.1]p2, but only for "object[s] of automatic storage duration that could have been declared with the register storage class (never had its address taken)" (!!)), but we don't actually exploit that;  we just produce a trap value.  We could change that, but it would break a lot of code.
 
> While I agree we are within our rights to do it, unlike other situations where we make changes that can break users' code (like strict aliasing, where it opens up many different optimization potentials) the optimizations that this is going to open up almost certainly *will* break the code that triggers this, but not otherwise improve things. So this change will just force users to edit their code in order to get the old behavior, without improving anything.
> 
> I disagree. Previously, the code had undefined behavior but got away with it. After editing the code, it has well defined behavior. And it doesn't even seem possible to edit the code and "get the old behavior" back, because the old behavior returned 'undef', which AFAIK cannot be reasonably done in C++ without invoking undefined behavior... What are the semantics for non-primitive types? Destructors? I think there is a reason the C++ spec made this UB: the code and language work better if the user writes the return.

C++ generally avoids talking about undefined values;  everything that would get one in C is just undefined behavior in C++.  That's a simplification in the standard — especially given the existence of non-POD types — but I don't know that I'd assign huge moral value to it.

> There are plenty of cases where the user wants the C++ behavior. Look at all of the functions in LLVM and Clang which have llvm_unreachable just before their close curly brace (typically after returning from each case in a switch on an enumerator). While some of those cases are merely to silence GCC warnings, many do actually have a performance and code size impact. While I'm a fan of explicitly annotating such cases, there is a lot of code which does not follow this practice.
> 
> 
> The other thing that surprises me is that this behavior has been in place for a few months now without any significant user complaints that I have seen (in the open source community or internally within Google).

Not seeing any complaints from those people who happen to have picked up an unreleased version of a compiler over the holiday season is not really the most impressive standard in the world.  :)

Google seems to have a truly impressive lack of terrible vendor code, which is great for you.  Please remember that you're an outlier among platforms — a shining beacon on a hill, if you like.

> I'm assuming you're seeing complaints internally about this. How extensive is this problem? What is the nature of the patterns that trigger this warning? I ask in part because we had to fix up essentially none of the open source code we depend on here to deal with this change.

A significant part of the problem, I believe, is that there's a lot of mostly-externally-maintained C code which, at Apple, happens to need to be compiled as C++.

John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130123/ddfc0ddb/attachment.html>