[cfe-commits] [patch] Alternative fix for pr9614

[John McCall]

On Oct 26, 2011, at 1:54 PM, Peter Collingbourne wrote:
> On Wed, Oct 26, 2011 at 04:46:09PM -0400, Rafael Ávila de Espíndola wrote:
>>> +  // PR9614. Avoid cases where the source code is lying to us. An available
>>> +  // externally function should have an equivalent function somewhere else,
>>> +  // but a function that calls itself is clearly not equivalent to the real
>>> +  // implementation.
>>> +  if (isTriviallyRecursiveViaAsm(F))
>>> +    return false;
>>> +  if (F->hasAttr<AlwaysInlineAttr>())
>>> +    return true;
>>> +  if (CodeGenOpts.OptimizationLevel == 0)
>>> +    return false;
>>> +  return true;
>>> 
>>> Please keep the CodeGenOpts.OptimizationLevel test first, and put the
>>> isTriviallyRecursiveViaAsm test at the end.
>> 
>> I did that, but it changes the meaning for always_inline functions. We  
>> can always revisit this if one is found in the wild.
>> 
>> ...
>> 
>> 
>> Besides that, it seems okay to me.
> 
> It looks ok to me, but wouldn't it be more efficient to somehow abort
> IR generation for the current function if we find ourselves building
> a trivially recursive call instruction?

Call sites are far more common than function definitions that we
aren't required to emit, and IR generation is not designed for
function definitions to be aborted like this, nor should it be.

This should be treated as a header bug that we are specifically
working around, not a real feature to be designed for.  For the record,
the appropriate way to do something like this is to shadow the
function with a macro that calls a different function.

John.