[DebugInfo] More precise variable range for stack locations

[NikolaPrica]

Summary

Variable's stack location can stretch longer than they should. If variable is placed at stack in some nested basic block its range can be calculated to be up to next occurrence of variable's DBG_VALUE or up to the end of function thus covering basic blocks that should not be covered by location range. This is because DbgEntityHistoryCalculator ends register locations at the end of basic block only if register has been changed through course of function which is not case for register used to reference stack objects.
This patch also tries to produce single value location if location list builder managed to merge all locations into one.

[dstenb]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1360

It seems that this can give incorrect single value locations for cases where we have produced location list entries with empty ranges.

For example:

frame-setup PUSH64r undef $rax, implicit-def $rsp, implicit $rsp
DBG_VALUE 123, $noreg, !12, !DIExpression(), debug-location !13 
DBG_VALUE 456, $noreg, !12, !DIExpression(), debug-location !13 
CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, debug-location !14 
$eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, debug-location !15                                                                   
$rcx = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !15 
RETQ killed $eax, debug-location !15

Here, the first debug value will result in a location list entry with an empty range, since it is immediately followed by the second, and such entries are omitted from the location list, resulting in a single entry in Entries for the constant value 456. However, MInsn is the first debug value, so we will create a single location description with the wrong value:

0x00000043:     DW_TAG_variable
                  DW_AT_const_value	(123)

I'm not sure what the check should be to avoid these sort of cases. Perhaps check that all debug value entries in HistoryMapEntries are described by the same location, and have the same expression?

[NikolaPrica]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1360

Such check could be return value of buildLocationList? Or we can just take the last DebugInstr from HistoryMapEntries which could be the last entry or the entry before the last one?

[dstenb]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1360

Or we can just take the last DebugInstr from HistoryMapEntries which could be the last entry or the entry before the last one?

That is perhaps possible. I tried to think if we can land in a situation where the last DebugInstr in HistoryMapEntries produces a location list entry with an empty range. For example something like this:

DBG_VALUE 123, $noreg, !12, !DIExpression(), debug-location !13 
[...]
DBG_VALUE 123, $noreg, !12, !DIExpression(), debug-location !13
DBG_VALUE 456, $noreg, !12, !DIExpression(), debug-location !13 
[end of function]

However, AFAIK we can't insert debug values after terminators currently (the machine verifier will complain about that), and with a presence of a terminator in the last block then the range will not be empty, so perhaps this is guaranteed? I'm not completely sure.

[NikolaPrica]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1360

I'm not sure either. But I suppose that such situation can appear for local variable that ends at some basic block? Maybe it is safer perform such verification in buildLocationList and return it as result?

[dstenb]

(Adding Jeremy as reviewer as he has also been working in this area recently.)

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

375–376

Perhaps this must not be done in this patch, but I wonder if we should close all register-described values here?

For example:

volatile int g;

int baz(int p) {
  int local = 123;

  if (g > 1) {
    local = p;
    g += p;
  }

  return 3;
}

when compiling the above with clang -O1 -g --target=x86_64-unknown-linux-gnu we land in the following MIR after livedebugvalues:

bb.0.entry:
  successors: %bb.1(0x40000000), %bb.2(0x40000000)
  liveins: $edi

  DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
  DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
  DBG_VALUE 123, $noreg, !17, !DIExpression(), debug-location !19
  renamable $eax = MOV32rm $rip, 1, $noreg, @g, $noreg, debug-location !20 :: (volatile dereferenceable load 4 from @g, !tbaa !22)
  CMP32ri8 killed renamable $eax, 2, implicit-def $eflags, debug-location !26
  JCC_1 %bb.2, 12, implicit $eflags, debug-location !27

bb.1.if.then:
  successors: %bb.2(0x80000000)
  liveins: $edi

  DBG_VALUE 123, $noreg, !17, !DIExpression(), debug-location !19
  DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
  DBG_VALUE $edi, $noreg, !17, !DIExpression(), debug-location !19
  ADD32mr $rip, 1, $noreg, @g, $noreg, killed renamable $edi, implicit-def dead $eflags, debug-location !28 :: (volatile store 4 into @g, !tbaa !22), (volatile dereferenceable load 4 from @g, !tbaa !22)

bb.2.if.end:
  DBG_VALUE $edi, $noreg, !16, !DIExpression(), debug-location !18
  $eax = MOV32ri 3, debug-location !31
  RETQ $eax, debug-location !31

As seen, in the g > 1 branch the local variable is described by edi. As edi is not modified, it will not be contained in ChangingRegs.

As we don't close the range here, this means that we'll incorrectly describe local using edi for the rest of the function:

DW_AT_location	(0x00000000
   [0x0000000000000000,  0x000000000000000b): DW_OP_consts +123, DW_OP_stack_value
   [0x000000000000000b,  0x0000000000000017): DW_OP_reg5 RDI)

0000000000000000 baz:
       0: 8b 05 00 00 00 00            	movl	(%rip), %eax
       6: 83 f8 02                     	cmpl	$2, %eax
       9: 7c 06                        	jl	6 <baz+0x11>
       b: 01 3d 00 00 00 00            	addl	%edi, (%rip)
      11: b8 03 00 00 00               	movl	$3, %eax
      16: c3                           	retq

[jmorse]

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

375–376

Perhaps this must not be done in this patch, but I wonder if we should close all register-described values here?

I'd say definite yes -- when I dug into the history [0] for PR41600 (which this patch fixes I think) it sounded like ChangingRegs was a mild hack to work around variable locations being unreliable ~5 years ago. IMHO (YMMV) we might be able to delete it in another patch as a now redundant feature. (Plus as you demonstrate, it can be broken).

[0] https://reviews.llvm.org/D3933

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1360

Great question @ empty ranges; would those be filtered out by this clause [0]?

I believe there are certain circumstances where LLVM won't have a (Machine IR) terminator after a function call declared attribute((noreturn)), often at the end of a function. A quick experiment shows that during IR optimisation another part of the compiler places an 'unreachable' inst after the noreturn call and drops any dbg.values, so I doubt this scenario is accessible from source languages.

[0] https://github.com/llvm/llvm-project/blob/da82ce99b7460164ffb841619aadb44f397d2106/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1189

test/DebugInfo/MIR/X86/dbg-stack-value-range.mir

4–9

IMHO the second sentence should describe the desired behaviour ("DbgEntityHistoryCalculator should close the variable's range at the end of the basic block..."), then afterwards describe the error that this test is for. It's not obvious right now that the second sentence is about the error case, until you read the third sentence.

73

I understand attributes are a pain to maintain in tests; can they be dropped?

106

Nit - most of the default MIR data can be omitted, pretty much every attribute that's false. (--simplify-mir to llc will drop it automatically).

[NikolaPrica]

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

375–376

This is similar problem that I am targeting. It would be solved by closing register-described values at the end of block. The only difference is that local is dead after the branch, but the idea is the same: Having BB1 that branches to BB2 and BB3, different DBG_VALUE from BB2 takes range across BB3 due to lack of variable's DBG_VALUE in it (variable might be dead or it just does lack location at that block).

I stumbled on a case when frame register is not in ChangingRegs but as you shown it can happen for other registers. I needed to modify DwarfDebug part in order to preserve single value variable locations because of partitioning of variable's range. Current condition for recognizing single value locations would not be recognized as single value location).

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1360

Great question @ empty ranges; would those be filtered out by this clause [0]?

They would, but the result location will end up as a DebugLocEntry::Value. The catch is that initialization of DbgVariable takes DBG_VALUE instruction. Maybe we should refactor DbgVariable to have DebugLocEntry::Value instead of DBG_VALUE instruction?

test/DebugInfo/MIR/X86/dbg-stack-value-range.mir

4–9

Sure. I will rewrite it!

73

I usually drop them from .ll but now I needed "no-frame-pointer" for test case so I missed to remove them from .mir. I will drop it.

[NikolaPrica]

I've posted new approach for fixing this problem at https://reviews.llvm.org/D61890. If that approach turns to be better I will abandon this change.

[jmorse]

Just to note: the patch this is based on (r359426) was reverted in r360301 due to compile time issues. I've got a patch incoming (tomorrow) that relies on this patch to fix it: they'll need to be un-reverted / committed in a certain order and possibly all together. (We can work through that when this is approved though).

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

375–376

NB: I think the ChangingRegs situation can be addressed separately. and this patch is fine to go ahead with the other tweaks.

[aprantl]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1356

Tiny nit: Can we use the actual class name instead of paraphrasing it as "location list builder"?

[NikolaPrica]

-Take last HistoryMapEntries as a representative single value location.
-Update test explanation.
-Strip uneccessary parts of test.

[jmorse]

LGTM (I think I can approve?). The source IR needing no-frame-pointer-elim seems like enough reason to keep that attribute around.

As mentioned above, r360301 needs to be reverted before this is committed, and then D61940 is needed to fix the compile time regression that will cause. Best to wait for D61940 to be approved before committing, so that we can do that all at once.

[jmorse]

Nikola mentioned in D61890 that the changes to fission-ranges.ll result in reduced variable location ranges, which is unfortunate. IMO, eyeballing the test, I think the input IR we have at the moment is wrong, and it was only the bug that this patch fixes that caused us to get the output right.

In fission-ranges.ll the dbg.value for !18 / the "e" variable, has a !DILocation !39, which corresonds to the lexical block where 'b' is iterated over. This causes LiveDebugValues to not propagate the variable location out of the block modifying "e", because it believes "e" isn't valid outside of that lexical block. In the source code at the top of the file though we can see that "e" has a function-wide scope. Thus the IR in the test is wrong, the dbg.value should have a different DILocation! We only covered the whole function before because DbgEntityHistoryCalculator was extending variable locations too far.

We should check whether clang still generates this faulty IR, but I've run out of time for today.

[NikolaPrica]

For the sample code in fisson-range.ll we now generate slightly different IR (few more llvm.dbg.values). With such IR, LLVM generates one block larger range but nevertheless the problem there is different nature and it could be used as a test case for further improvement of LiveDebugValues (value from stack gets loaded into a callee clobberabble register and at the entry of a block we have two variable locations and we choose the one which is inserted later - see LiveDebugvalues::transferDebugValue). I will take a look into that.

I suppose that this is ready to land?

[NikolaPrica]

Actually I've just produce fix for fisson-ranges.ll that will have better ranges. I'll post it as child patch of this one.

[jmorse]

In D61600#1509763, @NikolaPrica wrote:

I suppose that this is ready to land?

Indeed, although as mentioned that requires one patch to be unreverted, and D61940 to be ready to fix the performance regression in it :/. There shouldn't be a serious delay, hopefully.

[NikolaPrica]

-Rebase
-Change referenced scope to function scope for variable "b" in fisson-ranges.ll

[dstenb]

Adding a comment about another hand-written example.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1369–1370

If we have input like this:

!13 = !DILocalVariable(name: "local", scope: !7, file: !1, line: 5, type: !14)
!14 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
[...]
  bb.0.entry:
    DBG_VALUE 123, $noreg, !13, !DIExpression(), debug-location !16
    DBG_VALUE 456, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 0, 16), debug-location !18
    DBG_VALUE 789, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !18
    CALL64pcrel32 @bar, csr_64, implicit $rsp, implicit $ssp, implicit-def $rsp, implicit-def $ssp, debug-location !17
    RETQ debug-location !18

buildLocationList() will produce a single entry (a composite location description containing the 456 and 789 fragment). Then MInsn will be:

DBG_VALUE 123, $noreg, !13, !DIExpression(), debug-location !16

and DbgMI:

DBG_VALUE 789, $noreg, !13, !DIExpression(DW_OP_LLVM_fragment, 16, 16), debug-location !18

and End null. As MInsn covers the whole scope, we will produce a single location description, but with DbgMI, which is only one of the two fragmented debug values which the variable is described by:

DW_AT_location	(DW_OP_piece 0x2, DW_OP_constu 0x315, DW_OP_stack_value, DW_OP_piece 0x2)

That input is hand-modified, and perhaps we can never get such debug information here in DwarfDebug when running through the normal pass chain. Still, it feels a bit iffy that a size optimization may result in us dropping information. My feeling is that this framework code should be able to handle that type of silly input.

Maybe we should be conservative and only try to produce single location descriptions in the most obvious cases. The most conservative approach I guess would be to only produce single location descriptions when all debug values are the same. Another is perhaps only doing this if all debug values are non-fragmented?

[NikolaPrica]

• Represent DbgVariable's single value representation by DebugLocEntry::Value insted of representing it with MachineInstr. Such representation will allow us to produce fragment single value representation (for now it is not supported).
• Update DwarfDebug::buildLocationList to check if produced list has single entry that is valid throughout whole variable's scope.

If this seems as a proper way, I suppose that we can make DebugLoc::Value now as a separate class ValueLoc or ValueLocation? Such refactor could go as a separate NFC patch if it is fine?

[aprantl]

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

610 ↗

(On Diff #201236)

typo: Check and has a single

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

247

///

270

per coding style this comment should be on the decl inside the class not on the implementation

1104

If the resulting list

[NikolaPrica]

• Update comments.

[dstenb]

Thanks a lot! This "location list -> single location description" detection seems more robust, and should help us moving towards supporting rewrites of single-entry location lists containing fragments also.

Nit: Perhaps we should move the Value class out of DebugLocEntry now that we use it for DbgVariable also? However, I don't have any good ideas on in where to place it (e.g. which header file). I think we can land this without doing such a refactoring though.

lib/CodeGen/AsmPrinter/DwarfDebug.h

114 ↗

(On Diff #201268)

Nit: Perhaps change single instruction -> single location?

[dstenb]

In D61600#1518010, @dstenb wrote:

Nit: Perhaps we should move the Value class out of DebugLocEntry now that we use it for DbgVariable also? However, I don't have any good ideas on in where to place it (e.g. which header file). I think we can land this without doing such a refactoring though.

Sorry, I saw now that you had already added a comment about that.

[NikolaPrica]

• Updated comment (single instruction -> single location)

@dstenb Nevermind. Thanks for the review anyway! Do you think that this is ready for landing now?

[dstenb]

In D61600#1518157, @NikolaPrica wrote:

Do you think that this is ready for landing now?

LGTM, but I think it would be good to see if @aprantl has any more comments on this.

[aprantl]

LGTM. Have you tested the performance when compiling Clang with ASAN enabled to make sure we're not accidentally regressing because of the more thorough single-location check?

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

360

personally I'd write this as CurElem->first == FrameReg not a big deal, though

lib/CodeGen/AsmPrinter/DwarfDebug.h

119 ↗

(On Diff #201528)

NFC commit for later, make this Optional<unsigned>

[NikolaPrica]

• Rebase.

[NikolaPrica]

Have you tested the performance when compiling Clang with ASAN enabled to make sure we're not accidentally regressing because of the more thorough single-location check?

I've built LLVM with '-DLLVM_USE_SANITIZER=Address' using downloaded clang binary to produce private clang binary. I've run tests and compiled latest version of gdb with new private binary and I didn't see any regression there. Is that what you meant?

[dstenb]

In D61600#1527114, @NikolaPrica wrote:

Have you tested the performance when compiling Clang with ASAN enabled to make sure we're not accidentally regressing because of the more thorough single-location check?

I've built LLVM with '-DLLVM_USE_SANITIZER=Address' using downloaded clang binary to produce private clang binary. I've run tests and compiled latest version of gdb with new private binary and I didn't see any regression there. Is that what you meant?

I think that the idea was to see if this patch increases the compilation time when building such a Clang binary.

(Just make sure to use the RelWithDebInfo build type also.)

[aprantl]

In D61600#1527182, @dstenb wrote:

In D61600#1527114, @NikolaPrica wrote:

Have you tested the performance when compiling Clang with ASAN enabled to make sure we're not accidentally regressing because of the more thorough single-location check?

I've built LLVM with '-DLLVM_USE_SANITIZER=Address' using downloaded clang binary to produce private clang binary. I've run tests and compiled latest version of gdb with new private binary and I didn't see any regression there. Is that what you meant?

I think that the idea was to see if this patch increases the compilation time when building such a Clang binary.

(Just make sure to use the RelWithDebInfo build type also.)

Yes. Enabling ASAN on a large C++ code base such as Clang is a great way to see compiler performance problems.
Can you post the output of "ninja clean && time ninja clang" before and after applying your patch to the *host* compiler?

[NikolaPrica]

In D61600#1527581, @aprantl wrote:

In D61600#1527182, @dstenb wrote:

In D61600#1527114, @NikolaPrica wrote:

Have you tested the performance when compiling Clang with ASAN enabled to make sure we're not accidentally regressing because of the more thorough single-location check?

I've built LLVM with '-DLLVM_USE_SANITIZER=Address' using downloaded clang binary to produce private clang binary. I've run tests and compiled latest version of gdb with new private binary and I didn't see any regression there. Is that what you meant?

I think that the idea was to see if this patch increases the compilation time when building such a Clang binary.

(Just make sure to use the RelWithDebInfo build type also.)

Yes. Enabling ASAN on a large C++ code base such as Clang is a great way to see compiler performance problems.
Can you post the output of "ninja clean && time ninja clang" before and after applying your patch to the *host* compiler?

Thanks for clarification!

before

real 46m29,670s
user 325m27,488s
sys 6m17,255s

after

real 46m41,562s
user 326m16,497s
sys 6m18,981s

Considering user + sys there is 0.25% increment with patch.

[aprantl]

Thanks, that looks like an acceptable performance/accuracy tradeoff.

[NikolaPrica]

Thanks for the review and patience!

lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp

void llvm::calculateDbgEntityHistory(const MachineFunction *MF,
                                      const TargetRegisterInfo *TRI,
                                      DbgValueHistoryMap &DbgValues,
                                      DbgLabelInstrMap &DbgLabels) {
   BitVector ChangingRegs(TRI->getNumRegs());
   collectChangingRegs(MF, TRI, ChangingRegs);
 
   const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
   unsigned SP = TLI->getStackPointerRegisterToSaveRestore();
+  unsigned FrameReg = TRI->getFrameRegister(*MF);
   RegDescribedVarsMap RegVars;
   DbgValueEntriesMap LiveEntries;
   for (const auto &MBB : *MF) {
     for (const auto &MI : MBB) {
       if (!MI.isDebugInstr()) {
         // Not a DBG_VALUE instruction. It may clobber registers which describe
         // some variables.
         for (const MachineOperand &MO : MI.operands()) {
for (const auto &MI : MBB) {
         DbgLabels.addInstr(L, MI);
       }
     }
 
     // Make sure locations for all variables are valid only until the end of
     // the basic block (unless it's the last basic block, in which case let
     // their liveness run off to the end of the function).
     if (!MBB.empty() && &MBB != &MF->back()) {
       // Iterate over all variables that have open debug values.
       SmallSet<unsigned, 8> RegsToClobber;
       for (auto &Pair : LiveEntries) {
         // Iterate over history entries for all open fragments.
         SmallVector<EntryIndex, 8> IdxesToRemove;
         for (EntryIndex Idx : Pair.second) {
           DbgValueHistoryMap::Entry &Ent = DbgValues.getEntry(Pair.first, Idx);
           assert(Ent.isDbgValue() && !Ent.isClosed());
           const MachineInstr *DbgValue = Ent.getInstr();
 
           // If this is a register or indirect DBG_VALUE, apply some futher
           // tests to see if we should clobber it. Perform the clobbering
           // later though, to keep LiveEntries iteration stable.
           if (DbgValue->getOperand(0).isReg()) {
             unsigned RegNo = DbgValue->getOperand(0).getReg();
-            if (TRI->isVirtualRegister(RegNo) || ChangingRegs.test(RegNo))
+            if (TRI->isVirtualRegister(RegNo) || ChangingRegs.test(RegNo) ||
+                FrameReg == RegNo)
               RegsToClobber.insert(RegNo);
           } else {
             // This is a constant, terminate it at end of the block. Store
             // eliminated EntryIdx and delete later, for iteration stability.
             EntryIndex ClobIdx = DbgValues.startClobber(Pair.first, MBB.back());
             DbgValues.getEntry(Pair.first, Idx).endEntry(ClobIdx);
             IdxesToRemove.push_back(Idx);
           }

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (unsigned i = 0, N = Elements.size(); i < N; ++i) {
       auto *DT = cast<DIDerivedType>(Elements[i]);
       if (getName() == DT->getName())
         return resolve(DT->getBaseType());
     }
   }
   return Ty;
 }
 
 ArrayRef<DbgVariable::FrameIndexExpr> DbgVariable::getFrameIndexExprs() const {
   if (FrameIndexExprs.size() == 1)
     return FrameIndexExprs;
 
   assert(llvm::all_of(FrameIndexExprs,
                       [](const FrameIndexExpr &A) {
                         return A.Expr->isFragment();
                       }) &&
          "multiple FI expressions without DW_OP_LLVM_fragment");
   llvm::sort(FrameIndexExprs,
              [](const FrameIndexExpr &A, const FrameIndexExpr &B) -> bool {
                return A.Expr->getFragmentInfo()->OffsetInBits <
                       B.Expr->getFragmentInfo()->OffsetInBits;
              });
 
   return FrameIndexExprs;
 }
 
 void DbgVariable::addMMIEntry(const DbgVariable &V) {
   assert(DebugLocListIndex == ~0U && !MInsn && "not an MMI entry");
   assert(V.DebugLocListIndex == ~0U && !V.MInsn && "not an MMI entry");
   assert(V.getVariable() == getVariable() && "conflicting variable");
   assert(V.getInlinedAt() == getInlinedAt() && "conflicting inlined-at location");
 
   assert(!FrameIndexExprs.empty() && "Expected an MMI entry");
   assert(!V.FrameIndexExprs.empty() && "Expected an MMI entry");
 
   // FIXME: This logic should not be necessary anymore, as we now have proper
   // deduplication. However, without it, we currently run into the assertion
   // below, which means that we are likely dealing with broken input, i.e. two
   // non-fragment entries for the same variable at different frame indices.
   if (FrameIndexExprs.size()) {
if (MI->getOperand(0).isCImm())
     return DebugLocEntry::Value(Expr, MI->getOperand(0).getCImm());
 
   llvm_unreachable("Unexpected 4-operand DBG_VALUE instruction!");
 }
 
 /// Build the location list for all DBG_VALUEs in the function that
 /// describe the same variable. The resulting DebugLocEntries will have
 /// strict monotonically increasing begin addresses and will never
 /// overlap.
 //
 // See the definition of DbgValueHistoryMap::Entry for an explanation of the
 // different kinds of history map entries. One thing to be aware of is that if
 // a debug value is ended by another entry (rather than being valid until the
 // end of the function), that entry's instruction may or may not be included in
 // the range, depending on if the entry is a clobbering entry (it has an
 // instruction that clobbers one or more preceding locations), or if it is an
 // (overlapping) debug value entry. This distinction can be seen in the example
DbgVariable *RegVar = cast<DbgVariable>(createConcreteEntity(TheCU,
                                             *Scope, LocalVar, IV.second));
 
     const MachineInstr *MInsn = HistoryMapEntries.front().getInstr();
     assert(MInsn->isDebugValue() && "History must begin with debug value");
 
     // Check if there is a single DBG_VALUE, valid throughout the var's scope.
     // If the history map contains a single debug value, there may be an
     // additional entry which clobbers the debug value.
+    size_t HistSize = HistoryMapEntries.size();
     bool SingleValueWithClobber =
-        HistoryMapEntries.size() == 2 && HistoryMapEntries[1].isClobber();
-    if (HistoryMapEntries.size() == 1 || SingleValueWithClobber) {
+        HistSize == 2 && HistoryMapEntries[1].isClobber();
+    if (HistSize == 1 || SingleValueWithClobber) {
       const auto *End =
           SingleValueWithClobber ? HistoryMapEntries[1].getInstr() : nullptr;
       if (validThroughout(LScopes, MInsn, End)) {
         RegVar->initializeDbgValue(MInsn);
         continue;
       }
     }
 
     // Do not emit location lists if .debug_loc secton is disabled.
     if (!useLocSection())
       continue;
 
     // Handle multiple DBG_VALUE instructions describing one variable.
     DebugLocStream::ListBuilder List(DebugLocs, TheCU, *Asm, *RegVar, *MInsn);
 
     // Build the location list for this variable.
     SmallVector<DebugLocEntry, 8> Entries;
     buildLocationList(Entries, HistoryMapEntries);
 
+    // Check whether buildLocationList managed to merge all locations to one
+    // that is valid throughout the variable's scope. If so, produce single
+    // value location.
+    // TODO: Add support for single value fragment locations.
+    if (Entries.size() == 1 && !MInsn->getDebugExpression()->isFragment()) {
+      const auto *End = HistoryMapEntries.back().isClobber()
+                            ? HistoryMapEntries.back().getInstr()
+                            : nullptr;
+      const MachineInstr *DbgMI =
+          HistoryMapEntries.back().isDbgValue()
+              ? HistoryMapEntries.back().getInstr()
+              : HistoryMapEntries[HistSize - 2].getInstr();
+      // Use first DBG_VALUE instruction for check.
+      if (validThroughout(LScopes, MInsn, End)) {
+        RegVar->initializeDbgValue(DbgMI);
+        continue;
+      }
+    }
+
     // If the variable has a DIBasicType, extract it.  Basic types cannot have
     // unique identifiers, so don't bother resolving the type with the
     // identifier map.
     const DIBasicType *BT = dyn_cast<DIBasicType>(
         static_cast<const Metadata *>(LocalVar->getType()));
 
     // Finalize the entry by lowering it into a DWARF bytestream.
     for (auto &Entry : Entries)

test/DebugInfo/MIR/X86/dbg-stack-value-range.mir

+# RUN: llc -start-before=livedebugvalues %s -filetype=asm -o -| FileCheck %s
+#
+# DbgEntityHistoryCalculator should close variable's range at the end of
+# the basic block when variable is referenced through non-changing frame
+# pointer register. Having BB1 that branches to BB2 and BB3, where
+# BB2 falls through to BB3, variable can have different locations at BB1 and BB2.
+# Since input locations at BB3 for same variable are different LiveDebugValues
+# wont generate DBG_VALUE for BB3. If last variable location at BB2 is
+# non-changing register, DbgEntityHistoryCalculator will extend range of
+# DBG_VALUE from BB2 to whole BB3 and thus produce incorrect range for
+# case when we took branch BB3 from BB1.
+#
+# Verifies that "local1" stack location is ended at the end of the block (.Ltmp6).
+#
+# CHECK: .Ltmp4:
+# CHECK-NEXT:   #DEBUG_VALUE: foo:local1 <- [DW_OP_constu 4, DW_OP_minus, DW_OP_deref] $rbp
+# CHECK:        jmp     .LBB0_2
+# CHECK-NEXT: .Ltmp6:
+# CHECK: .Lfunc_end0:
+#
+# CHECK: .Linfo_string7:
+# CHECK-NEXT: .asciz  "local1"
+#
+# CHECK: .Ldebug_loc1:
+# CHECK-NEXT: .quad   .Ltmp1-.Lfunc_begin0
+# CHECK-NEXT: .quad   .Ltmp4-.Lfunc_begin0
+# CHECK-NEXT: .short  1                       # Loc expr size
+# CHECK-NEXT: .byte   94                      # super-register DW_OP_reg14
+# CHECK-NEXT: .quad   .Ltmp4-.Lfunc_begin0
+# CHECK-NEXT: .quad   .Ltmp6-.Lfunc_begin0
+# CHECK-NEXT: .short  2                       # Loc expr size
+# CHECK-NEXT: .byte   118                     # DW_OP_breg6
+# CHECK-NEXT: .byte   124                     # -4
+#
+# CHECK: .byte   5                       # Abbrev [5] 0x5d:0xf DW_TAG_variable
+# CHECK-NEXT: .long   .Ldebug_loc1            # DW_AT_location
+# CHECK-NEXT: .long   .Linfo_string7          # DW_AT_name
+
+
+--- |
+  ; ModuleID = 'dbg-stack-value-range.ll'
+  source_filename = "dbg-stack-value-range.c"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  
+  ; Function Attrs: nounwind uwtable
+  define dso_local i32 @foo(i32 %X) local_unnamed_addr #0 !dbg !7 {
+  entry:
+    %local1 = alloca i32, align 4
+    call void @llvm.dbg.value(metadata i32 %X, metadata !12, metadata !DIExpression()), !dbg !15
+    %0 = bitcast i32* %local1 to i8*, !dbg !15
+    call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %0), !dbg !15
+    call void @llvm.dbg.value(metadata i32 5, metadata !14, metadata !DIExpression()), !dbg !15
+    %call = tail call i32 (...) @check(), !dbg !15
+    %tobool = icmp eq i32 %call, 0, !dbg !15
+    br i1 %tobool, label %if.else, label %if.then, !dbg !15
+  
+  if.then:                                          ; preds = %entry
+    call void @llvm.dbg.value(metadata i32 4, metadata !13, metadata !DIExpression()), !dbg !15
+    store i32 4, i32* %local1, align 4, !dbg !15, !tbaa !16
+    call void @llvm.dbg.value(metadata i32* %local1, metadata !13, metadata !DIExpression(DW_OP_deref)), !dbg !15
+    %call1 = call i32 @init(i32* nonnull %local1), !dbg !15
+    call void @llvm.dbg.value(metadata i32 undef, metadata !14, metadata !DIExpression()), !dbg !15
+    br label %if.end, !dbg !15
+  
+  if.else:                                          ; preds = %entry
+    call void @llvm.dbg.value(metadata i32 5, metadata !13, metadata !DIExpression()), !dbg !15
+    store i32 5, i32* %local1, align 4, !dbg !15, !tbaa !16
+    br label %if.end
+  
+  if.end:                                           ; preds = %if.else, %if.then
+    %1 = bitcast i32* %local1 to i8*, !dbg !15
+    call void @llvm.dbg.value(metadata i32 undef, metadata !14, metadata !DIExpression()), !dbg !15
+    %call2 = call i32 (...) @init2(), !dbg !15
+    call void @llvm.dbg.value(metadata i32 undef, metadata !14, metadata !DIExpression()), !dbg !15
+    %2 = load i32, i32* %local1, align 4, !dbg !15, !tbaa !16
+    call void @llvm.dbg.value(metadata i32 %2, metadata !13, metadata !DIExpression()), !dbg !15
+    call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %1), !dbg !15
+    ret i32 %2, !dbg !15
+  }
+  
+  ; Function Attrs: argmemonly nounwind
+  declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
+  
+  declare dso_local i32 @check(...) local_unnamed_addr
+  
+  declare dso_local i32 @init(i32*) local_unnamed_addr
+  
+  declare dso_local i32 @init2(...) local_unnamed_addr
+  
+  ; Function Attrs: argmemonly nounwind
+  declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
+  
+  ; Function Attrs: nounwind readnone speculatable
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**)
+
+  attributes #0 = { nounwind uwtable "no-frame-pointer-elim"="false" "no-frame-pointer-elim-non-leaf" }
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 9.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+  !1 = !DIFile(filename: "dbg-stack-value-range.c", directory: "/")
+  !2 = !{}
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!"clang version 9.0.0"}
+  !7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 11, type: !8, scopeLine: 11, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10, !10}
+  !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !11 = !{!12, !13, !14}
+  !12 = !DILocalVariable(name: "X", arg: 1, scope: !7, file: !1, line: 11, type: !10)
+  !13 = !DILocalVariable(name: "local1", scope: !7, file: !1, line: 12, type: !10)
+  !14 = !DILocalVariable(name: "local2", scope: !7, file: !1, line: 12, type: !10)
+  !15 = !DILocation(line: 11, column: 13, scope: !7)
+  !16 = !{!17, !17, i64 0}
+  !17 = !{!"int", !18, i64 0}
+  !18 = !{!"omnipotent char", !19, i64 0}
+  !19 = !{!"Simple C/C++ TBAA"}
+
+...
+---
+name:            foo
+alignment:       4
+frameInfo:       
+  stackSize:       24
+  offsetAdjustment: -24
+  maxAlignment:    4
+  adjustsStack:    true
+  hasCalls:        true
+fixedStack:      
+  - { id: 0, type: spill-slot, offset: -16, size: 8, alignment: 16, stack-id: 0, 
+      callee-saved-register: '$r14d', callee-saved-restored: true, debug-info-variable: '', 
+      debug-info-expression: '', debug-info-location: '' }
+stack:           
+  - { id: 0, name: local1, type: default, offset: -20, size: 4, alignment: 4, 
+      stack-id: 0, callee-saved-register: '', callee-saved-restored: true, 
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+body:             |
+  bb.0.entry:
+    successors: %bb.3(0x30000000), %bb.1(0x50000000)
+  
+    DBG_VALUE $edi, $noreg, !12, !DIExpression(), debug-location !15
+    frame-setup PUSH64r killed $rbp, implicit-def $rsp, implicit $rsp
+    CFI_INSTRUCTION def_cfa_offset 16
+    CFI_INSTRUCTION offset $rbp, -16
+    $rbp = frame-setup MOV64rr $rsp
+    CFI_INSTRUCTION def_cfa_register $rbp
+    $rsp = frame-setup SUB64ri8 $rsp, 16, implicit-def dead $eflags
+    DBG_VALUE 5, $noreg, !14, !DIExpression(), debug-location !15
+    $r14d = MOV32ri 4, implicit-def $r14
+    DBG_VALUE $r14d, $noreg, !13, !DIExpression(), debug-location !15
+    dead $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $al, debug-location !15
+    CALL64pcrel32 @check, csr_64, implicit $rsp, implicit $ssp, implicit $al, implicit-def $rsp, implicit-def $ssp, implicit-def $eax, debug-location !15
+    TEST32rr killed renamable $eax, renamable $eax, implicit-def $eflags, debug-location !15
+    JCC_1 %bb.3, 4, implicit killed $eflags, debug-location !15
+  
+  bb.1.if.then:
+    successors: %bb.3(0x80000000)
+  
+    MOV32mr $rbp, 1, $noreg, -4, $noreg, $r14d, debug-location !15 :: (store 4 into %ir.local1, !tbaa !16)
+    DBG_VALUE $rbp, $noreg, !13, !DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_deref), debug-location !15
+    renamable $rdi = LEA64r $rbp, 1, $noreg, -4, $noreg
+    CALL64pcrel32 @init, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp, implicit-def dead $eax, debug-location !15
+    DBG_VALUE $noreg, $noreg, !14, !DIExpression(), debug-location !15
+    JMP_1 %bb.3
+  
+  bb.3.if.end:
+    DBG_VALUE $noreg, $noreg, !14, !DIExpression(), debug-location !15
+    dead $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags, implicit-def $al, debug-location !15
+    CALL64pcrel32 @init2, csr_64, implicit $rsp, implicit $ssp, implicit $al, implicit-def $rsp, implicit-def $ssp, implicit-def dead $eax, debug-location !15
+    DBG_VALUE $noreg, $noreg, !14, !DIExpression(), debug-location !15
+    renamable $eax = MOV32rm $rbp, 1, $noreg, -4, $noreg, debug-location !15 :: (dereferenceable load 4 from %ir.local1, !tbaa !16)
+    $rsp = frame-destroy ADD64ri8 $rsp, 16, implicit-def dead $eflags, debug-location !15
+    $rbp = frame-destroy POP64r implicit-def $rsp, implicit $rsp, debug-location !15
+    CFI_INSTRUCTION def_cfa $rsp, 8, debug-location !15
+    RETQ $eax, debug-location !15
+
+...

test/DebugInfo/X86/fission-ranges.ll

 ; CHECK-NEXT: DW_AT_stmt_list
 ; CHECK-NEXT: DW_AT_comp_dir
 ; CHECK-NEXT: DW_AT_GNU_dwo_name
 ; CHECK-NEXT: DW_AT_GNU_dwo_id
 ; CHECK-NEXT: DW_AT_GNU_ranges_base
 ; CHECK-NEXT: DW_AT_GNU_addr_base [DW_FORM_sec_offset]                   (0x00000000)
 
 ; CHECK: .debug_info.dwo contents:
-; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[P:0x[0-9a-z]*]]
+; CHECK: DW_AT_const_value [DW_FORM_sdata]     (1)
+; CHECK-NEXT: DW_AT_name {{.*}} "p"
 ; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[A:0x[0-9a-z]*]]
 ; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[E:0x[0-9a-z]*]]
 ; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[B:0x[0-9a-z]*]]
 ; CHECK: DW_AT_location [DW_FORM_sec_offset]   ([[D:0x[0-9a-z]*]]
 ; CHECK: DW_AT_ranges [DW_FORM_sec_offset]   (0x00000000
 ; CHECK-NOT: .debug_loc contents:
 ; CHECK-NOT: Beginning address offset
 ; CHECK: .debug_loc.dwo contents:
 
 ; Don't assume these locations are entirely correct - feel free to update them
 ; if they've changed due to a bugfix, change in register allocation, etc.
 
-; CHECK:      [[P]]:
-; CHECK-NEXT:   Addr idx 1 (w/ length 204): DW_OP_consts +1, DW_OP_stack_value
 ; CHECK:      [[A]]:
 ; CHECK-NEXT:   Addr idx 2 (w/ length 169): DW_OP_consts +0, DW_OP_stack_value
 ; CHECK-NEXT:   Addr idx 3 (w/ length 15): DW_OP_reg0 RAX
 ; CHECK-NEXT:   Addr idx 4 (w/ length 6): DW_OP_breg7 RSP-8
 ; CHECK-NEXT:   Addr idx 5 (w/ length 4): DW_OP_reg0 RAX
 ; CHECK:      [[E]]:
 ; CHECK-NEXT:   Addr idx 6 (w/ length 9): DW_OP_reg0 RAX
-; CHECK-NEXT:   Addr idx 7 (w/ length 98): DW_OP_breg7 RSP-44
+; CHECK-NEXT:   Addr idx 7 (w/ length 10): DW_OP_breg7 RSP-44
 ; CHECK:      [[B]]:
 ; CHECK-NEXT:   Addr idx 8 (w/ length 15): DW_OP_reg0 RAX
-; CHECK-NEXT:   Addr idx 9 (w/ length 66): DW_OP_breg7 RSP-32
+; CHECK-NEXT:   Addr idx 9 (w/ length 2): DW_OP_breg7 RSP-32
 ; CHECK:      [[D]]:
 ; CHECK-NEXT:   Addr idx 10 (w/ length 15): DW_OP_reg0 RAX
-; CHECK-NEXT:   Addr idx 11 (w/ length 42): DW_OP_breg7 RSP-20
+; CHECK-NEXT:   Addr idx 11 (w/ length 2): DW_OP_breg7 RSP-20
 
 ; Make sure we don't produce any relocations in any .dwo section (though in particular, debug_info.dwo)
 ; HDR-NOT: .rela.{{.*}}.dwo
 
 ; Make sure we have enough stuff in the debug_addr to cover the address indexes
 ; (11 is the last index in debug_loc.dwo, making 12 entries of 8 bytes each,
 ; 12 * 8 == 96 base 10 == 60 base 16)