[cfe-commits] r140232 - /cfe/trunk/lib/Sema/SemaExpr.cpp

[Chandler Carruth]

On Wed, Sep 21, 2011 at 3:49 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Wed, Sep 21, 2011 at 3:36 PM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> On Wed, Sep 21, 2011 at 3:25 PM, David Blaikie <dblaikie at gmail.com>wrote:
>>
>>> One other point on this - the issues you've raised already exist in both
>>> assert and llvm_unreachable code, making this change (assert(0) ->
>>> llvm_unreachable) somewhat orthogonal to them.
>>>
>>
>> Not at all. llvm_unreachable has a tremendously different impact in
>> optimized builds than assert(0) does. The latter has zero impact, the
>> control flow remains as expected.
>>
>> But llvm_unreachable tells the optimizer that this code path *cannot be
>> taken*. That in turn constrains the value of the input. That in turn causes
>> other optimizations to fire, and so on. This can cause essentially unbounded
>> undefined behavior given an unexpected input, where as the assert will
>> "merely" fall through.
>>
>> I've specifically seen cases in the disassembler and assembler code where
>> truly unexpected cases are assert(0)'ed, but then handled benignly.
>>
>
> That's pretty much pure luck & potentially just hides the bug for longer in
> retail builds that might otherwise fail more catastrophically (or less,
> certainly - but I don't think it's strictly better to have release builds be
> agnostic to these paths)
>

I really think we're talking past each other. Imagine:

switch (input) {
  default: assert(0 && "Never handled input sequence!!");
  case FOO_InvalidA:
  case FOO_InvalidB:
  case FOO_InvalidC:
    DiagnoseInvalidInput(input);
    break;
  case FOO_...:
    ...
  case FOO_...:
}

Here, we have logic to handle invalid inputs, but we try to cover all of the
invalid squences which we expect, and use an assert to catch them in debug
builds. In opt builds, clearly we want to just gracefully fall back to the
invalid input handling, regardless. This doesn't hide any bugs, and it
doesn't cause any problems.

Fundamentally, I think you should let the authors of the code who actually
know what cases they are and aren't intending to handle decide on the best
construct here. My primary objection is to what seems to be a blind
application of a heuristic... Maybe I'm misunderstanding what you're
proposing.


>
>> I don't think we want to tell the optimizer that this *cannot* happen, but
>> rather catch when it does happen in our test suite. The assert seems well
>> suited to that.
>>
>
> Is this true even in debug builds? My understanding was that
> llvm_unreachable was much like an assert in debug builds - otherwise the use
> of a message is rather useless since the compiler might not even have such
> code anymore.
>

No, in debug builds it aborts with a message. Only in opt builds does it
actually change the optimizer's CFG.


> If it is true, should we consider changing our llvm_unreachable usage to
> assert(0s? That's what I was getting at above. We're already treating the
> same case in two different ways (some times with assert, sometimes with
> llvm_unreachable) & it'd be nice to figure out the right way.
>

I'm trying to explain that there are *different* cases here, and that assert
and llvm_unreachable are different because of that. These are two tools
which, while having similarities, are intended to solve two distinct
problems.

[on that note about optimizations - it'd be nice if our asserts could be
> used to implement the same kind of cascading optimizations that
> llvm_unreachable provides, wouldn't it?]
>

I don't actually like this. I think it will make bugs found in the wild
harder to diagnose and debug, for limited gains...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110921/96e12caf/attachment.html>