llvm dev - Sep 2017 - Jump Threading duplicates dbg.declare intrinsics for fragments, bug?

Hi,

I'm hitting an assertion "overlapping or duplicate fragments" in
the
DWARF codegen in addFragmentOffset(). This originates from a
duplicated dbg.declare intrinsic, declaring the same fragment twice.
The duplicated call was generated by the jump threading pass.

I have a patch (see below) that removes simply such duplicates, but
I'm not sure whether that is the right approach.

Cheers,
Björn


diff --git a/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
b/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 499780a173b..308b6bd2b9f 100644
--- a/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -220,6 +220,13 @@ ArrayRef<DbgVariable::FrameIndexExpr>
DbgVariable::getFrameIndexExprs() const {
               return A.Expr->getFragmentInfo()->OffsetInBits <
                      B.Expr->getFragmentInfo()->OffsetInBits;
             });
+
+  auto last = std::unique(FrameIndexExprs.begin(), FrameIndexExprs.end(),
+      [](const FrameIndexExpr &A, const FrameIndexExpr &B) -> bool {
+        return A.FI == B.FI && A.Expr == B.Expr;
+      });
+  FrameIndexExprs.erase(last, FrameIndexExprs.end());
+
   return FrameIndexExprs;
 }

Hi Björn,

I don't have any answers, just more confusion. Hopefully someone else 
can bring some light to this.

I'm also interested in dbg.declares and what the rules are regarding 
them since it's not very clear to me at the moment.

A similar fix as yours was made for duplicate non-fragment dbg.declares 
in r305244 and there is a bit of discussion about it in 
https://bugs.llvm.org//show_bug.cgi?id=33157
Maybe you've seen that?

I've run into a case where the inliner leaves two dbg.declares (with 
different locations since they origin from two different inlined call 
sites) connected to a single alloca after alloca-merging.

Then the inliner moves (what it thinks is) the one and only dbg.declare 
to the alloca (and thus leaves the other one in a loop). The help 
methods 
replaceDbgDeclareForAlloca()/replaceDbgDeclare()/FindAllocaDbgDeclare() 
all seems to think there is only one dbg.declare connected to one 
alloca. At least they all ignore all except the first one found.

Later loop unroll comes and unrolls the loop and then suddenly we have 
two absolutely identical dbg.declares and the assert in 
addFragmentOffset() blows. Who's at fault?

There is also an existing testcase that checks that there _are_ indeed 
two dbg.declares to a single alloca: 
Transforms/Inline/alloca-dbgdeclare-merge.ll

Regards,
Mikael

On 09/19/2017 02:27 PM, Björn Steinbrink via llvm-dev
wrote:
> Hi,
> 
> I'm hitting an assertion "overlapping or duplicate fragments"
in the
> DWARF codegen in addFragmentOffset(). This originates from a
> duplicated dbg.declare intrinsic, declaring the same fragment twice.
> The duplicated call was generated by the jump threading pass.
> 
> I have a patch (see below) that removes simply such duplicates, but
> I'm not sure whether that is the right approach.
> 
> Cheers,
> Björn
> 
> 
> diff --git a/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
> b/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
> index 499780a173b..308b6bd2b9f 100644
> --- a/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
> +++ b/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
> @@ -220,6 +220,13 @@ ArrayRef<DbgVariable::FrameIndexExpr>
> DbgVariable::getFrameIndexExprs() const {
>                 return A.Expr->getFragmentInfo()->OffsetInBits <
>                        B.Expr->getFragmentInfo()->OffsetInBits;
>               });
> +
> +  auto last = std::unique(FrameIndexExprs.begin(), FrameIndexExprs.end(),
> +      [](const FrameIndexExpr &A, const FrameIndexExpr &B) ->
bool {
> +        return A.FI == B.FI && A.Expr == B.Expr;
> +      });
> +  FrameIndexExprs.erase(last, FrameIndexExprs.end());
> +
>     return FrameIndexExprs;
>   }
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

Hi Mikael,

2017-09-19 15:33 GMT+02:00 Mikael Holmén <mikael.holmen at
ericsson.com>:
> A similar fix as yours was made for duplicate non-fragment dbg.declares in
> r305244 and there is a bit of discussion about it in
> https://bugs.llvm.org//show_bug.cgi?id=33157
> Maybe you've seen that?
I saw the code, but not the bug report it originated from. Thanks!

My patch had the same idea of trying not to complicate other passes
to avoid duplicate declarations.
> I've run into a case where the inliner leaves two dbg.declares (with
> different locations since they origin from two different inlined call
sites)
> connected to a single alloca after alloca-merging.
>
> Then the inliner moves (what it thinks is) the one and only dbg.declare to
> the alloca (and thus leaves the other one in a loop). The help methods
> replaceDbgDeclareForAlloca()/replaceDbgDeclare()/FindAllocaDbgDeclare() all
> seems to think there is only one dbg.declare connected to one alloca. At
> least they all ignore all except the first one found.
>
> Later loop unroll comes and unrolls the loop and then suddenly we have two
> absolutely identical dbg.declares and the assert in addFragmentOffset()
> blows. Who's at fault?
Sounds like the same issue just with a different pass triggering the problem.
> There is also an existing testcase that checks that there _are_ indeed two
> dbg.declares to a single alloca:
> Transforms/Inline/alloca-dbgdeclare-merge.ll
This is different. The test doesn't check for it, but the declarations
actually
describe different vars in different scopes, so that is fine.

Cheers,
Björn

> On Sep 19, 2017, at 6:33 AM, Mikael Holmén via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Hi Björn,
> 
> I don't have any answers, just more confusion. Hopefully someone else
can bring some light to this.
> 
> I'm also interested in dbg.declares and what the rules are regarding
them since it's not very clear to me at the moment.
A dbg.declare describes a stack-allocated variable. There may only be one
dbg.declare per source variable, with the one exception that if the source
variable is split up into multiple fragments (such as SROA) there may be one
dbg.declare per variable fragment. A dbg.declare has no means of describing
liveness of a variable and is always describing the location of the variable for
its entire lexical scope.
> 
> A similar fix as yours was made for duplicate non-fragment dbg.declares in
r305244 and there is a bit of discussion about it in
https://bugs.llvm.org//show_bug.cgi?id=33157
> Maybe you've seen that?
> 
> I've run into a case where the inliner leaves two dbg.declares (with
different locations since they origin from two different inlined call sites)
connected to a single alloca after alloca-merging.
> 
> Then the inliner moves (what it thinks is) the one and only dbg.declare to
the alloca (and thus leaves the other one in a loop). The help methods
replaceDbgDeclareForAlloca()/replaceDbgDeclare()/FindAllocaDbgDeclare() all
seems to think there is only one dbg.declare connected to one alloca. At least
they all ignore all except the first one found.
> 
> Later loop unroll comes and unrolls the loop and then suddenly we have two
absolutely identical dbg.declares and the assert in addFragmentOffset() blows.
Who's at fault?
Without having read the code yet, my intuition says that the unroller should not
be duplicating dbg.declares, only dbg.values.

-- adrian
> 
> There is also an existing testcase that checks that there _are_ indeed two
dbg.declares to a single alloca: Transforms/Inline/alloca-dbgdeclare-merge.ll
> 
> Regards,
> Mikael
> 
> On 09/19/2017 02:27 PM, Björn Steinbrink via llvm-dev wrote:
>> Hi,
>> I'm hitting an assertion "overlapping or duplicate
fragments" in the
>> DWARF codegen in addFragmentOffset(). This originates from a
>> duplicated dbg.declare intrinsic, declaring the same fragment twice.
>> The duplicated call was generated by the jump threading pass.
>> I have a patch (see below) that removes simply such duplicates, but
>> I'm not sure whether that is the right approach.
>> Cheers,
>> Björn
>> diff --git a/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> b/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> index 499780a173b..308b6bd2b9f 100644
>> --- a/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> +++ b/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>> @@ -220,6 +220,13 @@ ArrayRef<DbgVariable::FrameIndexExpr>
>> DbgVariable::getFrameIndexExprs() const {
>>                return A.Expr->getFragmentInfo()->OffsetInBits
<
>>                       B.Expr->getFragmentInfo()->OffsetInBits;
>>              });
>> +
>> +  auto last = std::unique(FrameIndexExprs.begin(),
FrameIndexExprs.end(),
>> +      [](const FrameIndexExpr &A, const FrameIndexExpr &B)
-> bool {
>> +        return A.FI == B.FI && A.Expr == B.Expr;
>> +      });
>> +  FrameIndexExprs.erase(last, FrameIndexExprs.end());
>> +
>>    return FrameIndexExprs;
>>  }
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev