[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 23, 2013, at 12:42 PM, Daniel Dunbar <daniel at zuster.org> wrote:
> On Wed, Jan 23, 2013 at 12:08 PM, John McCall <rjmccall at apple.com> wrote:
> On Jan 23, 2013, at 11:57 AM, Daniel Dunbar <daniel at zuster.org> wrote:
>> On Wed, Jan 23, 2013 at 11:44 AM, Chandler Carruth <chandlerc at google.com> wrote:
>> 
>> On Wed, Jan 23, 2013 at 11:07 AM, John McCall <rjmccall at apple.com> wrote:
>> 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++.
>> 
>> FWIW, this makes perfect sense, and also makes perfect sense out of a flag to essentially get C's return semantics in a C++ compilation in order to support such code.
>> 
>> This is still the wrong direction of the flag. I just haven't seen good justification for changing the compiler in this way to merit the possibility of breaking working code.
> 
> Every change can break working code.  Warning changes can break working code if it's compiled with -Werror.  "Show me a whole-percentage speedup or take the optimization out" is not really a reasonable response to every last proposal.
> 
> Yes, but that doesn't mean such changes should be made without consideration either. My argument is that I do not think there is sufficient user benefit to motivate this change.
> 
>  In LLVM and clang, we have a lot of places where we use unreachable annotations;  I think Chandler's argument is quite correct that these situations come up all the time for many users and that it's ultimately not reasonable to expect non-compiler people to use those annotations pervasively.
> 
> Our specific internal problem that makes this seem like a non-starter is that we have a pool of known code that's very awkward to fix.  We do control the build environment for that code, though.  For purposes of investigation, we can reasonably assume that any project that turns off -Wreturn-value should probably also disable the optimization.  Any stragglers can be tracked down and fixed just like we would with any other compiler change.
> 
> You are hijacking my argument. My opinion doesn't have anything to do with an internal problem, I just happen to think this is the wrong choice for users.
> 
> In my opinion, for *most* users and most code it is more important that the code work than that it be optimal. I think this is the kind of optimization that compiler hackers and low-level optimization people might find very desirable, but anyone writing code that depended on it should still be using an attribute or other marker.
> 
> Again in my opinion, for most users, the compiler is just a tool they use to get work done. They like it to optimize, and they like it to give nice warnings, but overall they want it to help them get work done and not force them to change their code.

I do not think that this is a reasonable standard by which we can judge optimizations.  The compiler is a tool.  Like most tools, it can do good things but is capable of mischief.  Like most tools, users come to take the good things for granted, but they notice the mischief immediately.  It's wrong to forsake the good just because it's less visible;  you have to actually make a thoughtful decision to balance these things, and that means not immediately throwing out all the good the first time you see the bad.

There are many optimizations that rely on assumptions about the behavior of code instead of just exploiting local efficiencies.  Every single one of those optimizations has the potential to misbehave if the assumptions are violated.  Here are the things you need to consider in order to actually make a balanced decision:
1.  How reasonable is it to violate the assumptions?
2.  How visible it is to the developer that they're violating the assumptions?
3.  How reasonable is it to fix the code to not violate the assumptions?
4.  How bad are the existing repercussions to violating the assumptions?
5.  How bad are the proposed repercussions to violating the assumptions?
6.  How reasonable is it to isolate code that wishes to willfully violate the assumptions?

The answers in this case:
1.  Not very reasonable.  I think most people understand that not returning correctly from a function is a pretty bad bug.
2.  Extraordinarily visible.  There is an existing warning, enabled by default in clang for several years and present in basically every compiler in the world, that (quite unusually for this kind of thing) points out the exact problem with very high reliability.
3.  In most cases, very reasonable.  Again, the warning points out everywhere that needs to be fixed.  Most such functions just need to be changed to return void;  for the others, "return 0;" will suffice for almost all return types.
4.  Somewhat bad.  If the return value is ignored by the caller, code will work correctly.  If it's not, inlining could easily cause somewhat similar problems already.
4.  Bad.  In unoptimized builds, a reliable crash upon reaching the violation, which should aid in debugging.  In optimized builds, a likely crash on reaching the violation, but a hard-to-debug crash involving the implicit deletion of code.
5.  Pretty reasonable.  Because of the warning, mass violations of this rule are rare and likely to be isolated within projects that mass-disable the warning for their builds.  Such projects could easily adopt an option to disable the optimization, for which there's plenty of precedent.

In my opinion, the quality of the warning almost settles the argument immediately.  The optimization can be useful in real-world code.  Code that violates the assumption it makes is basically guaranteed to have seen a warning about it, and it's a warning that responsible developers know to take seriously.  It is easy to detect projects that are likely to have major problems with this optimization, because they're very likely to have disabled the warning;  it should be similarly easy for them to simply add another flag to disable the optimization as well, if they're unwilling to take this opportunity to clean up their code.

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