[llvm] r312318 - Debug info for variables whose type is shrinked to bool
Friedman, Eli via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 11 16:47:02 PDT 2017
No, not solved. See https://reviews.llvm.org/D38830 .
-Eli
On 9/21/2017 3:54 AM, Mikael Holmén via llvm-commits wrote:
> Hi Nikola and reviewers,
>
> If I understand correctly this change was resubmitted in r313870?
>
> Did you consider the problem I described below? Is it solved/cannot
> happen now?
>
> Regards,
> Mikael
>
> On 09/07/2017 09:36 AM, Mikael Holmén wrote:
>> Hi Nikola and reviewers,
>>
>> I think the debug info produced with this patch triggers a problem in
>> llc.
>>
>> Specifically, in DwarfDebug::beginModule() we do
>>
>> CU.getOrCreateGlobalVariableDIE(GV,
>> sortGlobalExprs(GVMap[GV]));
>>
>> It's the sorting done by sortGlobalExprs that doesn't seem to handle
>> the case when we have mixed DW_OP_LLVM_fragment and DW_OP_deref very
>> well.
>>
>> Input to sort:
>> !DIExpression(DW_OP_LLVM_fragment, 0, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 16, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 32, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 48, 16)
>> !DIExpression(DW_OP_deref, DW_OP_constu, 1, DW_OP_mul, DW_OP_constu,
>> 0, DW_OP_plus, DW_OP_stack_value)
>> !DIExpression(DW_OP_LLVM_fragment, 80, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 96, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 112, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 128, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 144, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 160, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 176, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 192, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 208, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 224, 16)
>> !DIExpression(DW_OP_deref, DW_OP_constu, 1, DW_OP_mul, DW_OP_constu,
>> 0, DW_OP_plus, DW_OP_stack_value)
>> !DIExpression(DW_OP_LLVM_fragment, 256, 16)
>>
>> Output from sort:
>> !DIExpression(DW_OP_LLVM_fragment, 0, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 16, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 32, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 48, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 128, 16)
>> !DIExpression(DW_OP_deref, DW_OP_constu, 1, DW_OP_mul, DW_OP_constu,
>> 0, DW_OP_plus, DW_OP_stack_value)
>> !DIExpression(DW_OP_LLVM_fragment, 80, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 96, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 112, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 144, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 160, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 176, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 192, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 208, 16)
>> !DIExpression(DW_OP_LLVM_fragment, 224, 16)
>> !DIExpression(DW_OP_deref, DW_OP_constu, 1, DW_OP_mul, DW_OP_constu,
>> 0, DW_OP_plus, DW_OP_stack_value)
>> !DIExpression(DW_OP_LLVM_fragment, 256, 16)
>>
>> The above "sorting" then causes an assert to blow in
>> DwarfExpression::addFragmentOffset since that function seems to
>> assume that we handle fragments in strictly growing offset order.
>>
>> The sortGlobalExprs looks like
>>
>> /// Sort and unique GVEs by comparing their fragment offset.
>> static SmallVectorImpl<DwarfCompileUnit::GlobalExpr> &
>> sortGlobalExprs(SmallVectorImpl<DwarfCompileUnit::GlobalExpr> &GVEs) {
>> std::sort(GVEs.begin(), GVEs.end(),
>> [](DwarfCompileUnit::GlobalExpr A,
>> DwarfCompileUnit::GlobalExpr B) {
>> if (A.Expr != B.Expr && A.Expr && B.Expr) {
>> auto FragmentA = A.Expr->getFragmentInfo();
>> auto FragmentB = B.Expr->getFragmentInfo();
>> if (FragmentA && FragmentB)
>> return FragmentA->OffsetInBits <
>> FragmentB->OffsetInBits;
>> }
>> return false;
>> });
>> GVEs.erase(std::unique(GVEs.begin(), GVEs.end(),
>> [](DwarfCompileUnit::GlobalExpr A,
>> DwarfCompileUnit::GlobalExpr B) {
>> return A.Expr == B.Expr;
>> }),
>> GVEs.end());
>> return GVEs;
>> }
>>
>> if I add
>>
>> if (FragmentA)
>> return true;
>>
>> after the check
>>
>> if (FragmentA && FragmentB)
>> return FragmentA->OffsetInBits <
>>
>> then it gets nicely sorted.
>>
>> But maybe we need to be even more careful if we want to make sure the
>> fragments are all properly sorted when there are all kinds of things
>> in the input, and we should change the sorting to:
>> // Sort all expressions containing fragment offsets (relative to
>> each other),
>> // and put all other expressions in random order (relative to each
>> other) but
>> // after the fragment expressions.
>> std::sort(GVEs.begin(), GVEs.end(),
>> [](DwarfCompileUnit::GlobalExpr A,
>> DwarfCompileUnit::GlobalExpr B) {
>> auto FragmentA = A.Expr ? A.Expr->getFragmentInfo() :
>> None;
>> auto FragmentB = B.Expr ? B.Expr->getFragmentInfo() :
>> None;
>> if (FragmentA && FragmentB)
>> return FragmentA->OffsetInBits <
>> FragmentB->OffsetInBits;
>> if (FragmentA)
>> return true;
>> return false;
>> });
>>
>> so we ensure a proper sorting also when there are GlobalExprs with no
>> Expr?
>>
>> Also note that the attempt to remove duplicates in sortGlobalExprs
>> will not remove all duplicates! Since std::unique only removes
>> _consecutive_ equal elements, and the aobve sorting really only sorts
>> the fragments, it might leave duplicate non-fragment expressions.
>> I've no idea if this is a problem though.
>>
>> I see the above problems with
>>
>> llc -o - sort.ll
>>
>> where sort.ll is a heavily reduced ll file originally produced using
>> r312318. llc end stops with
>>
>> llc: ../lib/CodeGen/AsmPrinter/DwarfExpression.cpp:405: void
>> llvm::DwarfExpression::addFragmentOffset(const llvm::DIExpression *):
>> Assertion `FragmentOffset >= OffsetInBits && "overlapping or
>> duplicate fragments"' failed.
>>
>> Also note that if I use another compiler to compile llc (e.g. gcc
>> compared to clang) the problem assert doesn't show up. I suppose this
>> is due to luck, and has to do with the order in which std::sort
>> compares the objects in the input.
>>
>> Regards,
>> Mikael
>>
>> On 09/01/2017 12:05 PM, Strahinja Petrovic via llvm-commits wrote:
>>> Author: spetrovic
>>> Date: Fri Sep 1 03:05:27 2017
>>> New Revision: 312318
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=312318&view=rev
>>> Log:
>>> Debug info for variables whose type is shrinked to bool
>>>
>>> This patch provides such debug information for integer
>>> variables whose type is shrinked to bool by providing
>>> dwarf expression which returns either constant initial
>>> value or other value.
>>>
>>> Patch by Nikola Prica.
>>>
>>> Differential Revision: https://reviews.llvm.org/D35994
>>>
>>> Modified:
>>> llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
>>> llvm/trunk/lib/IR/DebugInfoMetadata.cpp
>>> llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp?rev=312318&r1=312317&r2=312318&view=diff
>>>
>>> ==============================================================================
>>>
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfExpression.cpp Fri Sep 1
>>> 03:05:27 2017
>>> @@ -338,6 +338,7 @@ void DwarfExpression::addExpression(DIEx
>>> break;
>>> case dwarf::DW_OP_plus:
>>> case dwarf::DW_OP_minus:
>>> + case dwarf::DW_OP_mul:
>>> emitOp(Op->getOp());
>>> break;
>>> case dwarf::DW_OP_deref:
>>>
>>> Modified: llvm/trunk/lib/IR/DebugInfoMetadata.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfoMetadata.cpp?rev=312318&r1=312317&r2=312318&view=diff
>>>
>>> ==============================================================================
>>>
>>> --- llvm/trunk/lib/IR/DebugInfoMetadata.cpp (original)
>>> +++ llvm/trunk/lib/IR/DebugInfoMetadata.cpp Fri Sep 1 03:05:27 2017
>>> @@ -643,6 +643,7 @@ bool DIExpression::isValid() const {
>>> case dwarf::DW_OP_plus_uconst:
>>> case dwarf::DW_OP_plus:
>>> case dwarf::DW_OP_minus:
>>> + case dwarf::DW_OP_mul:
>>> case dwarf::DW_OP_deref:
>>> case dwarf::DW_OP_xderef:
>>> break;
>>>
>>> Modified: llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp?rev=312318&r1=312317&r2=312318&view=diff
>>>
>>> ==============================================================================
>>>
>>> --- llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp Fri Sep 1 03:05:27
>>> 2017
>>> @@ -36,6 +36,7 @@
>>> #include "llvm/IR/Module.h"
>>> #include "llvm/IR/Operator.h"
>>> #include "llvm/IR/ValueHandle.h"
>>> +#include "llvm/IR/DebugInfoMetadata.h"
>>> #include "llvm/Pass.h"
>>> #include "llvm/Support/Debug.h"
>>> #include "llvm/Support/ErrorHandling.h"
>>> @@ -1603,12 +1604,47 @@ static bool TryToShrinkGlobalToBoolean(G
>>> assert(InitVal->getType() != Type::getInt1Ty(GV->getContext()) &&
>>> "No reason to shrink to bool!");
>>> + SmallVector<DIGlobalVariableExpression *, 1> GVs;
>>> + GV->getDebugInfo(GVs);
>>> +
>>> // If initialized to zero and storing one into the global, we
>>> can use a cast
>>> // instead of a select to synthesize the desired value.
>>> bool IsOneZero = false;
>>> - if (ConstantInt *CI = dyn_cast<ConstantInt>(OtherVal))
>>> + if (ConstantInt *CI = dyn_cast<ConstantInt>(OtherVal)){
>>> IsOneZero = InitVal->isNullValue() && CI->isOne();
>>> + ConstantInt *CIInit = dyn_cast<ConstantInt>(GV->getInitializer());
>>> + uint64_t ValInit = CIInit->getZExtValue();
>>> + uint64_t ValOther = CI->getZExtValue();
>>> + uint64_t ValMinus = ValOther - ValInit;
>>> +
>>> + for(auto *GVe : GVs){
>>> + DIGlobalVariable *DGV = GVe->getVariable();
>>> + DIExpression *E = GVe->getExpression();
>>> +
>>> + // val * (ValOther - ValInit) + ValInit:
>>> + // DW_OP_deref DW_OP_constu <ValMinus>
>>> + // DW_OP_mul DW_OP_constu <ValInit> DW_OP_plus DW_OP_stack_value
>>> + E = DIExpression::get(NewGV->getContext(),
>>> + {dwarf::DW_OP_deref,
>>> + dwarf::DW_OP_constu,
>>> + ValMinus,
>>> + dwarf::DW_OP_mul,
>>> + dwarf::DW_OP_constu,
>>> + ValInit,
>>> + dwarf::DW_OP_plus,
>>> + dwarf::DW_OP_stack_value});
>>> + DIGlobalVariableExpression *DGVE =
>>> + DIGlobalVariableExpression::get(NewGV->getContext(), DGV, E);
>>> + NewGV->addDebugInfo(DGVE);
>>> + }
>>> + } else {
>>> + // FIXME: This will only emit address for debugger on which will
>>> + // be written only 0 or 1.
>>> + for(auto *GV : GVs)
>>> + NewGV->addDebugInfo(GV);
>>> + }
>>> +
>>> while (!GV->use_empty()) {
>>> Instruction *UI = cast<Instruction>(GV->user_back());
>>> if (StoreInst *SI = dyn_cast<StoreInst>(UI)) {
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
More information about the llvm-commits
mailing list