[llvm] r369159 - [Attributor] Fix: Make sure we set the changed flag

[David Jones via llvm-commits]

FYI: reverted in r369241.

On Mon, Aug 19, 2019 at 12:58 AM David Jones <dlj at google.com> wrote:

> It looks like this revision breaks some tests under UBSAN.
>
> Example failure:
> llvm/test/Transforms/FunctionAttrs/arg_returned.ll
>
>
> [...]/llvm/lib/Transforms/IPO/Attributor.cpp:891:17: runtime error: load
> of value 64, which is not a valid value for type 'bool'
>     #0 [...] in
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::$_9::operator()(llvm::Value&,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState&, bool) const
> [...]/llvm/lib/Transforms/IPO/Attributor.cpp:891:17
>     #1 [...] in void llvm::function_ref<void (llvm::Value&,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState&,
> bool)>::callback_fn<AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::$_9>(long,
> llvm::Value&,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState&, bool)
> [...]/llvm/include/llvm/ADT/STLExtras.h:125:12
>     #2 [...] in llvm::function_ref<void (llvm::Value&,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState&,
> bool)>::operator()(llvm::Value&,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState&, bool) const
> [...]/llvm/include/llvm/ADT/STLExtras.h:142:12
>     #3 [...] in bool genericValueTraversal<llvm::AAReturnedValues,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState>(llvm::Attributor&,
> llvm::IRPosition, llvm::AAReturnedValues const&,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState&,
> llvm::function_ref<void (llvm::Value&,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState&, bool)>
> const&, int) [...]/llvm/lib/Transforms/IPO/Attributor.cpp:207:5
>     #4 [...] in
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::$_2::operator()(llvm::Value&,
> AAReturnedValuesImpl::updateImpl(llvm::Attributor&)::RVState&) const
> [...]/llvm/lib/Transforms/IPO/Attributor.cpp:902:12
>     #5 [...] in AAReturnedValuesImpl::updateImpl(llvm::Attributor&)
> [...]/llvm/lib/Transforms/IPO/Attributor.cpp:959:9
>     #6 [...] in llvm::AbstractAttribute::update(llvm::Attributor&)
> [...]/llvm/lib/Transforms/IPO/Attributor.cpp:264:16
>     #7 [...] in llvm::Attributor::run()
> [...]/llvm/lib/Transforms/IPO/Attributor.cpp:2526:17
>     #8 [...] in runAttributorOnModule(llvm::Module&)
> [...]/llvm/lib/Transforms/IPO/Attributor.cpp:2864:12
>     #9 [...] in (anonymous
> namespace)::MPPassManager::runOnModule(llvm::Module&)
> [...]/llvm/lib/IR/LegacyPassManager.cpp:1750:27
>     #10 [...] in llvm::legacy::PassManagerImpl::run(llvm::Module&)
> [...]/llvm/lib/IR/LegacyPassManager.cpp:1863:44
>     #11 [...] in main [...]/llvm/tools/opt/opt.cpp:892:12
>     #12 [...] in __libc_start_main (/usr/grte/v4/lib64/libc.so.6+[...])
>     #13 [...] in _start
> /usr/grte/v4/debug-src/src/csu/../sysdeps/x86_64/start.S:108
>
> SUMMARY: UndefinedBehaviorSanitizer: invalid-bool-load
> [...]/llvm/lib/Transforms/IPO/Attributor.cpp:891:17 in
> FileCheck error: '-' is empty.
>
> On Fri, Aug 16, 2019 at 2:53 PM Johannes Doerfert via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: jdoerfert
>> Date: Fri Aug 16 14:55:01 2019
>> New Revision: 369159
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=369159&view=rev
>> Log:
>> [Attributor] Fix: Make sure we set the changed flag
>>
>> The flag was updated *before* we actually run the visitor callback so we
>> might miss updates.
>>
>> Modified:
>>     llvm/trunk/lib/Transforms/IPO/Attributor.cpp
>>
>> Modified: llvm/trunk/lib/Transforms/IPO/Attributor.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/Attributor.cpp?rev=369159&r1=369158&r2=369159&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/IPO/Attributor.cpp (original)
>> +++ llvm/trunk/lib/Transforms/IPO/Attributor.cpp Fri Aug 16 14:55:01 2019
>> @@ -878,7 +878,7 @@ ChangeStatus AAReturnedValuesImpl::updat
>>      // The map in which we collect return values -> return instrs.
>>      decltype(ReturnedValues) &RetValsMap;
>>      // The flag to indicate a change.
>> -    bool Changed;
>> +    bool &Changed;
>>      // The return instrs we come from.
>>      SmallPtrSet<ReturnInst *, 2> RetInsts;
>>    };
>> @@ -906,9 +906,8 @@ ChangeStatus AAReturnedValuesImpl::updat
>>    // Callback for all "return intructions" live in the associated
>> function.
>>    auto CheckReturnInst = [this, &VisitReturnedValue,
>> &Changed](Instruction &I) {
>>      ReturnInst &Ret = cast<ReturnInst>(I);
>> -    RVState RVS({ReturnedValues, false, {}});
>> +    RVState RVS({ReturnedValues, Changed, {}});
>>      RVS.RetInsts.insert(&Ret);
>> -    Changed |= RVS.Changed;
>>      return VisitReturnedValue(*Ret.getReturnValue(), RVS);
>>    };
>>
>> @@ -955,7 +954,8 @@ ChangeStatus AAReturnedValuesImpl::updat
>>        if (Argument *Arg = dyn_cast<Argument>(RetVal)) {
>>          // Arguments are mapped to call site operands and we begin the
>> traversal
>>          // again.
>> -        RVState RVS({NewRVsMap, false, RetValAAIt.second});
>> +        bool Unused;
>> +        RVState RVS({NewRVsMap, Unused, RetValAAIt.second});
>>          VisitReturnedValue(*CB->getArgOperand(Arg->getArgNo()), RVS);
>>          continue;
>>        } else if (isa<CallBase>(RetVal)) {
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190819/2ab5c394/attachment.html>