Assertion failed: ("conflicting locations for variable")

[adrian-prantl]

Bugzilla Link	33157
Resolution	FIXED
Resolved on	Jun 13, 2017 01:44
Version	trunk
OS	All
CC	@adrian-prantl,@mveriksson,@pogo59,@rnk

Extended Description

Assertion failure when compiling Adium.

$ cat test.mi
typedef signed char BOOL;
@​protocol NSObject
@​end
typedef double CGFloat;
struct CGPoint {
CGFloat x;
CGFloat y;
};
typedef struct CGPoint CGPoint;
struct CGSize {
CGFloat width;
CGFloat height;
};
typedef struct CGSize CGSize;
struct CGRect {
CGPoint origin;
CGSize size;
};
typedef struct CGRect CGRect;
typedef CGRect NSRect;
CGRect NSMakeRect(CGFloat x, CGFloat y, CGFloat w, CGFloat h) {
CGRect r;
r.origin.x = x;
r.size.width = w;
return r;
}
CGFloat NSMaxX(NSRect aRect) { return (aRect.origin.x + aRect.size.width); }
@​protocol NSAccessibilityElement
@​end
@​interface NSView
@​end
@​interface KNShelfSplitView : NSView {
CGSize backgroundSize;
}
@​end
@​implementation KNShelfSplitView

• (void)
  :(NSRect)aRect active:(BOOL)isActive {
  NSRect sourceRect =
  NSMakeRect(0, 0, backgroundSize.width, backgroundSize.height);
  NSRect destRect = NSMakeRect(aRect.origin.x, aRect.origin.y,
  sourceRect.size.width, aRect.size.height);
  while ((destRect.origin.x < NSMaxX(aRect)) && destRect.size.width > 0) {
  if (NSMaxX(destRect) > NSMaxX(aRect)) {
  sourceRect.size.width = NSWidth(destRect);
  }
  }
  }
  @​end

$ clang -cc1 -triple x86_64-apple-macosx10.6.0 -emit-obj -dwarf-column-info -debug-info-kind=standalone -dwarf-version=4 -debugger-tuning=lldb -Os -std=gnu99 -fblocks -x objective-c test.mi 2>&1 | grep all_of

Assertion failed: (all_of(FrameIndexExprs, [](FrameIndexExpr &FIE) { return FIE.Expr && FIE.Expr->isFragment(); }) && "conflicting locations for variable"), function addMMIEntry, file ../lib/CodeGen/AsmPrinter/DwarfDebug.h, line 142.

This started failing with r302544:

Re-land "Use the frame index side table for byval and inalloca arguments"

This re-lands r302483. It was not the cause of #32324 .

git-original-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@302544

[rnk]

Why does the IR have four llvm.dbg.declare calls in it? That's odd.

$ grep %aRect t.ll
define double @​NSMaxX(%struct.CGRect* byval nocapture readonly align 8 %aRect) local_unnamed_addr #​3 !dbg !​52 {
tail call void @​llvm.dbg.declare(metadata %struct.CGRect* %aRect, metadata !​57, metadata !​45), !dbg !​58
%x = getelementptr inbounds %struct.CGRect, %struct.CGRect* %aRect, i64 0, i32 0, i32 0, !dbg !​58
%width = getelementptr inbounds %struct.CGRect, %struct.CGRect* %aRect, i64 0, i32 1, i32 0, !dbg !​58
define internal void @"\01-[KNShelfSplitView :active:]"(%0* nocapture readonly %self, i8* nocapture readnone %_cmd, %struct.CGRect* byval nocapture readonly align 8 %aRect, i8 signext %isActive) #​0 !dbg !​67 {
tail call void @​llvm.dbg.declare(metadata %struct.CGRect* %aRect, metadata !​81, metadata !​45), !dbg !​86
%4 = bitcast %struct.CGRect* %aRect to i64*, !dbg !​95
tail call void @​llvm.dbg.declare(metadata %struct.CGRect* %aRect, metadata !​57, metadata !​45), !dbg !​104
%width.i25 = getelementptr inbounds %struct.CGRect, %struct.CGRect* %aRect, i64 0, i32 1, i32 0, !dbg !​104
tail call void @​llvm.dbg.declare(metadata %struct.CGRect* %aRect, metadata !​57, metadata !​45), !dbg !​110
tail call void @​llvm.dbg.declare(metadata %struct.CGRect* %aRect, metadata !​57, metadata !​45), !dbg !​104

[rnk]

Nevermind, they all have different variables from different inlined versions of NSMaxX.

[adrian-prantl]

By the way I haven't looked at this further at all so far. It is entirely possible that your commit just uncovered a bug in the DWARF backend.

[rnk]

I think this is an uncovered bug, not a bug in the new code. What's happening is that we have two dbg.declares that get code duplicated during loop rotation after inlining and memcpy optimization. These are the ones that matter:
; before loop
tail call void @​llvm.dbg.declare(metadata %struct.CGRect* %aRect, metadata !​57, metadata !​45), !dbg !​104
; in loop backedge
tail call void @​llvm.dbg.declare(metadata %struct.CGRect* %aRect, metadata !​57, metadata !​45), !dbg !​104

They use the same location, variable metadata, DILocation, etc. They don't conflict, they're just code duplicated.

Anyway, I would suggest this patch, but it needs more tests. There are more edge cases here. It may be possible to craft a test case that uses parameters declared to live in static allocas that triggers the same assertion and isn't dependent on my change, but I don't have time to find it.

diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
index 8fb3db2..b38cece 100644
--- a/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
@@ -757,7 +757,14 @@ void DwarfDebug::collectVariableInfoFromMFTable(
"Expected inlined-at fields to agree");

 InlinedVariable Var(VI.Var, VI.Loc->getInlinedAt());

• Processed.insert(Var);

• // Ignore variables that we've already processed. Passes may code duplicate

• // calls to dbg.declare.

• // FIXME: Check that the duplicate entry in ConcreteVariables has the same

• // frame index and expression.

• if (Processed.insert(Var).second)

•  continue;
  

• LexicalScope *Scope = LScopes.findLexicalScope(VI.Loc);

  // If variable scope is not found then skip this variable.

[adrian-prantl]

Nice! That patch seems reasonable. As mentioned in the FIXME, it should assert that the FI and expr is identical.

Do you know which pass is duplicating the dbg.declare? Can/should that be fixed instead/in addition?

[rnk]

It was some loop pass, which wasn't apparent from -print-after-all, because it only prints each loop individually. It's hard to see the big picture.

This was my attempt to write a manual reduction, but it did not work:

@​g = external global i32
@​h = external global i32

declare void @​llvm.dbg.declare(metadata, metadata, metadata)

define void @​f(i32* byval %p, i1 %c) !dbg !​5 {
br i1 %c, label %x, label %y

x:
call void @​llvm.dbg.declare(metadata i32* %p, metadata !​10, metadata !DIExpression()), !dbg !​12
store i32 42, i32* @​g
br label %done

y:
call void @​llvm.dbg.declare(metadata i32* %p, metadata !​10, metadata !DIExpression()), !dbg !​12
store i32 42, i32* @​h
br label %done

done:
ret void
}

!llvm.dbg.cu = !{#0}
!llvm.module.flags = !{#22, !​23}

!​0 = distinct !DICompileUnit(language: DW_LANG_ObjC, file: !​1, producer: "clang version 5.0.0 ", isOptimized: true, runtimeVersion: 2, emissionKind: FullDebug)
!​1 = !DIFile(filename: "", directory: "C:\5Csrc\5Cllvm-project\5Cbuild")
!​22 = !{i32 2, !"Dwarf Version", i32 4}
!​23 = !{i32 2, !"Debug Info Version", i32 3}

!​5 = distinct !DISubprogram(name: "f", isLocal: true, isDefinition: true, scopeLine: 37, flags: DIFlagPrototyped, isOptimized: true, unit: !​0, type: !​99, scope: !​1)

!​10 = !DILocalVariable(name: "aRect", arg: 1, scope: !​98, file: !​1, line: 38)

!​12 = !DILocation(line: 43, scope: !​98, inlinedAt: !​13)
!​13 = distinct !DILocation(line: 43, scope: !​5)

!​98 = distinct !DISubprogram(name: "NSMaxX", scope: !​1, file: !​1, line: 27, isLocal: true, isDefinition: true, scopeLine: 27, flags: DIFlagPrototyped, isOptimized: true, unit: !​0, variables: !​62, type: !​99)
!​99 = !DISubroutineType(types: !​100)
!​100 = !{null}

!​62 = !{#10}

[mveriksson]

Nice! That patch seems reasonable. As mentioned in the FIXME, it should
assert that the FI and expr is identical.

Do you know which pass is duplicating the dbg.declare? Can/should that be
fixed instead/in addition?

LoopUnswitch does this!

opt -S bah.ll -o - -loop-unswitch | grep dbg.declare
declare void @​llvm.dbg.declare(metadata, metadata, metadata) #​0
tail call void @​llvm.dbg.declare(metadata %struct.S* %p1__, metadata !​11, metadata !​16), !dbg !​17
tail call void @​llvm.dbg.declare(metadata %struct.S* %p1__, metadata !​11, metadata !​16), !dbg !​17

I found this because we ran into the same issue in internal testing.

If it really is illegal to have duplicated dbg.declare shouldn't the verify-debug-info catch this case?

[mveriksson]

Loop unswitch test

[adrian-prantl]

If it really is illegal to have duplicated dbg.declare shouldn't the verify-debug-info catch this case?

Yes.

Alternatively, we could also filter out duplicate entries before we we enter them into the MF side-table.

In either case we should still fix LoopUnswitch to not duplicate dbg.declare intrinsics as this is a waste of space.

[mveriksson]

If it really is illegal to have duplicated dbg.declare shouldn't the verify-debug-info catch this case?

Yes.

Alternatively, we could also filter out duplicate entries before we we enter
them into the MF side-table.

In either case we should still fix LoopUnswitch to not duplicate dbg.declare
intrinsics as this is a waste of space.

I wrote a new issue for the LoopUnswitch case: bug 33355. I think it's not the same bug as this because here the dbg.declare is duplicated by LoopRotate if I understood correctly.

[adrian-prantl]

I committed a fix similar to the one Reid was proposing in r305244.
The testcase was missing a lexical scope for the variable and was thus triggering an early exit in collectVariableInfoFromMFTable().

[mveriksson]

*** Bug llvm/llvm-bugzilla-archive#33355 has been marked as a duplicate of this bug. ***

[mveriksson]

mentioned in issue llvm/llvm-bugzilla-archive#33355