[DebugInfo] Fix end_sequence of debug_line in LTO Object

[kyulee]

Summary

In a LTO build, the end_sequence in debug_line table for each compile unit (CU) points the end of text section which merged all CUs. The end_sequence needs to point to the end of each CU's range. This bug often causes invalid debug_line table in the final .dSYM binary for MachO after running dsymutil which tries to compensate an out-of-range address of end_sequence.
The fix is to sync the line table termination with the range operations that are already maintained in DwarfDebug. When CU or section changes, or nodebug functions appear or module is finished, the prior pending line table is terminated using the last range label. In the MC path where no range is tracked, the old logic is conservatively used to end the line table using the section end symbol.

[dblaikie]

Would it be possible and easier to test this issue without any LTO concerns, with code like this:

void f1() { }
void __attribute__((nodebug)) void f2() { }

Looks like the bug manifests here by having the line table cover f2, when it should end at the end of f1 instead?

(admittedly there's still a problem, maybe this isn't sufficiently representative, if there was void f3() { } at the end - we don't end the sequence around f2 and restart it afterwards... )

Well, if you want to keep the existing LTO-related testing, you could link two IR files together with simple empty functions, rather than interesting functions like you have in your examples. Just void f1() { } and void f2() { } in two files, llvm-linkd together would suffice, I think?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2158–2160

What's this refactoring for? Looks like it causes duplicate lookups of the DwarfCompileUnit, that might be nice to avoid/not introduce? (if it is necessary/preferred for some reason, might be worth doing separately from the rest of this patch?)

(both callers already have the DwarfCompileUnit available - perhaps this function should take that instead of the MachineFunction? that way it could avoid the extra lookups)

2204–2208

Perhaps this could be done once outside the loop?

// MBBSectionRanges is always non-empty at this point, right? (could assert just in case, but I think it's a reasonable invariant)
Asm->OutStreamer->getContext()
        .getMCDwarfLineTable(getDwarfCompileUnitID(MF))
        .getMCLineSections()
        .updateEndLabel(Asm->MBBSectionRanges.back().second.EndLabel);

llvm/lib/MC/MCObjectStreamer.cpp

516–517

When is this condition false? (is it tested?)

llvm/test/DebugInfo/lto-debugline-main.ll

1 ↗

(On Diff #367079)

This patch only applies to llvm's codegen - so the test probably shouldn't include opt or LTO tools - the post-LTO'd IR should be checked in directly & then fed into llc in the test to exercise the codegen codepath, most likely?

[clayborg]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2208

Are the "Asm->MBBSectionRanges" sorted? If not, do we need to find the highest EndLabel and only add that one?

[kyulee]

Update the change based on the feedback

• Refactor NFC change to a separate revision, https://reviews.llvm.org/D108271
• Use a non-LTO unit test
• Set EndLabel once after the loop
• Simpliy the logic to directly use EndLabel for end_sequence address

[kyulee]

Remove unnecessary MCObjectWriter.h inclusion.

[kyulee]

Restore symbol diff logic to fix Window failures

[kyulee]

Rebase. getDwarfCompileUnitID -> getDwarfCompileUnitIDForLineTable in https://reviews.llvm.org/D108271

[dblaikie]

llvm/lib/MC/MCObjectStreamer.cpp

516–517

Still curious about this question ^

[kyulee]

llvm/lib/MC/MCObjectStreamer.cpp

516–517

I've attempted to delete this symbol diff check in https://reviews.llvm.org/D108261?id=367123 but
https://buildkite.com/llvm-project/premerge-checks/builds/53063#d29a2537-c103-4035-86a0-4e01e0188dd4 showed many builds failed like

Assertion failed: Abs && "We created a line delta with an invalid expression"

So, I restored this logic back.

As for InSet = false, I think it confirms the absolute symbol difference, or this check can be true optimistically by https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/llvm/lib/MC/MachObjectWriter.cpp#L680-L681.
I tested the following test could fail if I set InSet = true.

DebugInfo/X86/gmlt.test

[dblaikie]

llvm/lib/MC/MCObjectStreamer.cpp

516–517

This seems suspicious to me - "isSymbolRefDifferenceFullyResolvedImpl" (also a bit suspicious that this is calling an "Impl" function - usually those are only used as implementation details for some non-impl function, where the latter is intended as the general entry point) should only return false if the two symbols aren't in the same section. I wouldn't've thought that should happen - when/where/how/why does that happen?

[kyulee]

Use isSymbolRefDifferenceFullyResolved instead of isSymbolRefDifferenceFullyResolvedImpl

[kyulee]

llvm/lib/MC/MCObjectStreamer.cpp

516–517

I've just replaced isSymbolRefDifferenceFullyResolvedImpl by isSymbolRefDifferenceFullyResolved.
As shown https://github.com/llvm/llvm-project/blob/d480f968ad8b56d3ee4a6b6df5532d485b0ad01e/llvm/include/llvm/MC/MCObjectWriter.h#L67-L86, the arguments are very similar in these two except that one is with MCSymbolRefExpr and the other is with MCSymbol.

Are you suggesting a new API isSymbolRefDifferenceFullyResolved(constMCAssembler &Asm, constMCSymbolRefExpr *A, constMCSymbolRefExpr *B) that defaults with InSet = false, and use it here?

[dblaikie]

llvm/lib/MC/MCObjectStreamer.cpp

516–517

Sorry, no I don't mean to rename or refactor this API.

I'd like to understand why this function call ever produces a result of false - it seems to me that EndLabel and LastLabel should always be from the same section, and if they are, I think this function should never return false - so I think I'm misunderstanding something/don't understand where these labels are coming from such that they could end up being from different sections. I'd like to understand how that situation can arise before approving this patch - to understand better why this approach/test is suitable.

[kyulee]

llvm/lib/MC/MCObjectStreamer.cpp

516–517

I'm not familiar with the whole logic, but I've traced a particular case below.

DebugInfo/X86/cu-ranges.ll

This test runs with -function-sections so it appears each function goes to each section (comdat?).
I found LastLabel and EndLabel are in the same section for the last function only because EndLabel is set from the last function.
This means all the prior functions go with the end section label because this symbol difference check fails.
For the last function, EndLabel is actually the same offset to the end section label because each section has its own function only.
So, I think this logic seems to cover this case correctly resulting in a valid line table. I'm not sure if there is a better way to guard this case.

[kyulee]

Set EndLabel per Section. A compilation unit (CU) can have multiple sections (e.g., comdat) so track EndLabel per section.
This seems a way to avoid unnecessary symbol diff check because EndLabel and LastLabel would be in the same section.

[kyulee]

Handle DebugLineEntries and EndLabel per Section

[kyulee]

@dblaikie Could you take another look? Thanks!

[dblaikie]

I'm a bit confused/unclear how this algorithm works/how this fixes things and whether it's the right direction. It'll be a while before I can find the time/brain bandwidth to really dig into this - so perhaps you can help me.

What I'm confused by is why multiple line tables would be unterminated & then end up needing to terminate them all/figure out which one to terminate? Instead I'd expect whenever another line table got an entry, the previously current line table entry would need to end - so there shouldn't be a need for the extra maps & maybe we can just keep track of the current "open" line table (whichever was the last one to emit an entry)?

[kyulee]

In D108261#2966544, @dblaikie wrote:

I'm a bit confused/unclear how this algorithm works/how this fixes things and whether it's the right direction. It'll be a while before I can find the time/brain bandwidth to really dig into this - so perhaps you can help me.

What I'm confused by is why multiple line tables would be unterminated & then end up needing to terminate them all/figure out which one to terminate? Instead I'd expect whenever another line table got an entry, the previously current line table entry would need to end - so there shouldn't be a need for the extra maps & maybe we can just keep track of the current "open" line table (whichever was the last one to emit an entry)?

Here is my understanding. Basically a line table is emitted per compilation unit (CU). For simplicity, I consider two cases:

1. When multiple sections exist in a CU, we still want to emit them into a single line table (with multiple end_sequences per section).
2. In a LTO case, multiple CUs appear in a single (merged) object. In that case, we want to emit multiple line tables per CU.

So, this algorithm tries to track the end label of each function range per section in a CU. Likewise each line entries are already aggregated per section in a CU. This ensures emitting the end label per section per CU.
In case for asm parser path (instead of normal compilation path), those line entries/function range may not be emitted, so in that case (EndLabel = null), I fall back to the old logic that just uses the section end label conservatively.

This is two examples that this change tries to fix/improve.

1. Multiple sections in a CU case -- similar to Comdat (function per section).

void f2()
{
}
// The end of .text in CU line table

__attribute__((section(".s1")))
void f1()
{
}
// The end of .s1 in CU line table
__attribute__((nodebug,section(".s1")))
void f1nodebug()
{
}
// --> Before this change: The line table pointed to here because it's the end of the section .s1.

#if 0
// llvm-objdump -d t1.o
Disassembly of section .text:

0000000000000000 <f2>:
..
       5: c3                            retq

Disassembly of section .s1:

0000000000000000 <f1>:
..
       5: c3                            retq

0000000000000010 <f1nodebug>:
..
      15: c3                            retq


// llvm-dwarfdump --debugline t1.o
Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      2      0      1   0             0  is_stmt
0x0000000000000004      3      1      1   0             0  is_stmt prologue_end
0x0000000000000006      3      1      1   0             0  is_stmt end_sequence
0x0000000000000000      8      0      1   0             0  is_stmt
0x0000000000000004      9      1      1   0             0  is_stmt prologue_end
0x0000000000000006      9      1      1   0             0  is_stmt end_sequence ---> Before this change, it pointed to offset 16, the end of the section .s1.
#endif

2. LTO case: Multiple CUs in a single section

// LTO object effectively has the view of the merged CUs.
// CU1 from t1.c
void t1()
{
}
// End of .text in CU1 line table

// CU2 from t2.c
void t2()
{
}
// End of .text in CU2 line table

#if 0
$ llvm-objdump -d lto.o
0000000000000000 <t1>:
...
       5: c3                            retq

0000000000000010 <t2>:
..
      15: c3                            retq

$ llvm-dwarfdump --debug-line lto.o
Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      4      0      1   0             0  is_stmt
0x0000000000000004      5      1      1   0             0  is_stmt prologue_end
0x0000000000000006      5      1      1   0             0  is_stmt end_sequence --> Before this change, it pointed to offset 16 (the end of .text section).

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000010      4      0      1   0             0  is_stmt
0x0000000000000014      5      1      1   0             0  is_stmt prologue_end
0x0000000000000016      5      1      1   0             0  is_stmt end_sequence
#endif

[dblaikie]

In D108261#2966644, @kyulee wrote:

In D108261#2966544, @dblaikie wrote:

I'm a bit confused/unclear how this algorithm works/how this fixes things and whether it's the right direction. It'll be a while before I can find the time/brain bandwidth to really dig into this - so perhaps you can help me.

What I'm confused by is why multiple line tables would be unterminated & then end up needing to terminate them all/figure out which one to terminate? Instead I'd expect whenever another line table got an entry, the previously current line table entry would need to end - so there shouldn't be a need for the extra maps & maybe we can just keep track of the current "open" line table (whichever was the last one to emit an entry)?

Here is my understanding. Basically a line table is emitted per compilation unit (CU). For simplicity, I consider two cases:

1. When multiple sections exist in a CU, we still want to emit them into a single line table (with multiple end_sequences per section).
2. In a LTO case, multiple CUs appear in a single (merged) object. In that case, we want to emit multiple line tables per CU.

^ Presumably multiple line tables per Module, one per CU.

So, this algorithm tries to track the end label of each function range per section in a CU. Likewise each line entries are already aggregated per section in a CU. This ensures emitting the end label per section per CU.
In case for asm parser path (instead of normal compilation path), those line entries/function range may not be emitted, so in that case (EndLabel = null), I fall back to the old logic that just uses the section end label conservatively.

Ah, I see, and you've in the "normal compilation path" the change here in DwarfDebug's endFunctionImpl to add an end label for the section. Wouldn't this have issues if you interleave functions from CUs in the same section - like CU1:func1:.text, then CU2:func2:.text, then CU1:func3:.text. (ah, right, I mentioned this in my first comment, but I'm feeling more like fixing the issue your seeing should involve the more general fix to that interleaved issue too)

Yeah, in that case you get CU1's line table covering the whole range, including func2, which isn't intended/desirable.

So I think this solution you have is incomplete & I'd say it's essentially the same bug - at least I think of it that way. But I guess it depends what sort of "invalid debug_line table in the final dsym" you're dealing with - they're all not great, but maybe there's a more severe invalidity in the end case?

I guess what I'm suggesting would probably still require that extra signal from DwarfDebug in the normal compilation path, but it could be more robust/address the interleaved issue which wouldn't be fixed by this approach.

What if whenever the section changes (in raw MC) or when a new line entry is emitted to another CU (or in DwarfDebug, if a nodebug function starts) - then emit an end_prologue to whatever the current open line table is? (have to keep track of that, I guess - "last emitted line table MCLineDivision")

That would miss some opportunities for shorter line table encodings (in the func1/2/3 scenario above, if func2 was in another section, then the line table for func1/3 wouldn't actually need to have two separate chunks - they could be contiguous) - so more advanced would be keeping "Last MCLineDivision per section". Terminate the last MCLineDivision whenever somethingr is emitted to that section that isn't the same division. DwarfDebug would have one extra bonus: If it starts a nodebug function, it could also pre-emptively terminate any current MCLineDivision.

I think that'd fix all these issues, probably?

[kyulee]

In D108261#2995771, @dblaikie wrote:

In D108261#2966644, @kyulee wrote:

In D108261#2966544, @dblaikie wrote:

I'm a bit confused/unclear how this algorithm works/how this fixes things and whether it's the right direction. It'll be a while before I can find the time/brain bandwidth to really dig into this - so perhaps you can help me.

What I'm confused by is why multiple line tables would be unterminated & then end up needing to terminate them all/figure out which one to terminate? Instead I'd expect whenever another line table got an entry, the previously current line table entry would need to end - so there shouldn't be a need for the extra maps & maybe we can just keep track of the current "open" line table (whichever was the last one to emit an entry)?

Here is my understanding. Basically a line table is emitted per compilation unit (CU). For simplicity, I consider two cases:

1. When multiple sections exist in a CU, we still want to emit them into a single line table (with multiple end_sequences per section).
2. In a LTO case, multiple CUs appear in a single (merged) object. In that case, we want to emit multiple line tables per CU.

^ Presumably multiple line tables per Module, one per CU.

So, this algorithm tries to track the end label of each function range per section in a CU. Likewise each line entries are already aggregated per section in a CU. This ensures emitting the end label per section per CU.
In case for asm parser path (instead of normal compilation path), those line entries/function range may not be emitted, so in that case (EndLabel = null), I fall back to the old logic that just uses the section end label conservatively.

Ah, I see, and you've in the "normal compilation path" the change here in DwarfDebug's endFunctionImpl to add an end label for the section. Wouldn't this have issues if you interleave functions from CUs in the same section - like CU1:func1:.text, then CU2:func2:.text, then CU1:func3:.text. (ah, right, I mentioned this in my first comment, but I'm feeling more like fixing the issue your seeing should involve the more general fix to that interleaved issue too)

Yeah, in that case you get CU1's line table covering the whole range, including func2, which isn't intended/desirable.

So I think this solution you have is incomplete & I'd say it's essentially the same bug - at least I think of it that way. But I guess it depends what sort of "invalid debug_line table in the final dsym" you're dealing with - they're all not great, but maybe there's a more severe invalidity in the end case?

I guess what I'm suggesting would probably still require that extra signal from DwarfDebug in the normal compilation path, but it could be more robust/address the interleaved issue which wouldn't be fixed by this approach.

What if whenever the section changes (in raw MC) or when a new line entry is emitted to another CU (or in DwarfDebug, if a nodebug function starts) - then emit an end_prologue to whatever the current open line table is? (have to keep track of that, I guess - "last emitted line table MCLineDivision")

That would miss some opportunities for shorter line table encodings (in the func1/2/3 scenario above, if func2 was in another section, then the line table for func1/3 wouldn't actually need to have two separate chunks - they could be contiguous) - so more advanced would be keeping "Last MCLineDivision per section". Terminate the last MCLineDivision whenever somethingr is emitted to that section that isn't the same division. DwarfDebug would have one extra bonus: If it starts a nodebug function, it could also pre-emptively terminate any current MCLineDivision.

I think that'd fix all these issues, probably?

Thanks for comments! I roughly get your points but may need sometime to digest it.
I may misunderstand an important piece on your example -- CU1:func1:.text, then CU2:func2:.text, then CU1:func3:.text.
My understanding is CU is a compilation unit which seems to be processed sequentially (in LLVM). How could we interleave CUs like this? I thought we only interleave sections within a CU.
Does this interleaving come from a ThinLTO compilation? I would appreciate it if you can provide an example.

[dblaikie]

Thanks for comments! I roughly get your points but may need sometime to digest it.
I may misunderstand an important piece on your example -- CU1:func1:.text, then CU2:func2:.text, then CU1:func3:.text.
My understanding is CU is a compilation unit which seems to be processed sequentially (in LLVM).

Ah, that's not quite correct - a CU exists both by its existence in the cu list named module metadata, and by its reference from DISubprograms (which exist by reference from llvm::Functions). A CU is not processed atomically - some handling is done up front (at the start of the module, iterating the named module metadata list of CUs) and some is done at the end of the module - but between those, for all the DISubprograms, they're visited in the order of the llvm::Functions that reference them - so they can be arbitrarily interleaved.

How could we interleave CUs like this? I thought we only interleave sections within a CU.
Does this interleaving come from a ThinLTO compilation? I would appreciate it if you can provide an example.

I don't know that I have a concrete example that creates the interleaving I can reproduce by hand - but it's not an IR invariant that all Functions that reference CUs through DISubprograms must be contiguous for that CU.

To create such a situation by hand, create two files, let's say a.cpp and b.cpp and have two functions a1 and a2 in a.cpp and main in b.cpp, have main call a1 and a2. Compile to IR, link the IR, hand-modify the IR to reorder the functions so it goes a1, then main, then a2 - then you should be able to reproduce/see the strange situation where the line table for a.cpp continues/extends over main/b.cpp.

[clayborg]

Any progress on this? This causes serious debugging issues.

[kyulee]

Incorporated the feedback.

• Bookkeep the current line table for CU so that when a line entry needs to be emitted for different CU, terminate the current line table by injecting an end sequence.
• Didn't break this on section change, becuase the line table is already being tracked per section.
• Still didn't handle nodebug function case -- no debug function may span debug functions in the line table. Terminating the line table was a bit tricky becuse no debug function basically does not have a reference line entry. But I think this impreciion is less concern in practice.

[kyulee]

Fix the test

[kyulee]

Rebase

[kyulee]

@dblaikie Can you take a look again when you're available? Thanks!

[dblaikie]

In D108261#3124854, @kyulee wrote:

@dblaikie Can you take a look again when you're available? Thanks!

Yep, it's on my list.

[dblaikie]

Rather than trying to do this at the MCDwarf/streamer level - what if we did this in the AsmPrinter/DwarfDebug level? It could call some kind of MC function to terminate the line table because it knows when to do so? (it'd terminate basically whenever DwarfDebug was about to change the value of its PrevCU member (so in DwarfCompileUnit::addRange/DwarfDebug::setPrevCU and in DwarfDebug::skippedNonDebugFunction))

[kyulee]

Incorporate the feedback

• Terminate the line table when switching CU in DwarfDebug.
• Update the test since this now handles nodebug case correctly.

[dblaikie]

llvm/include/llvm/MC/MCDwarf.h

194

When does this case come up? I think all of this would only happen when PrevCU was non-null/had been populated with some content first, right?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2170

presumably this function shouldn't be called if PrevCU == NewCU, right? (maybe that could be asserted, rather than tested)

2174

Does is this empty? I /think/ that "PrevCU" is only set when PrevCU has already been populated with some content, right?

llvm/lib/MC/MCDwarf.cpp

232

Would be nice if we could eliminate the need for this case - since any time this gets used it risks being because a line table was allowed to "flow off the end" further than it should.

What would it be like if we handled the line table similar to the way ranges are handled? Always terminated at the end of each function, but then extended if the next function happens to start immediately after the last entry ended?

[kyulee]

llvm/include/llvm/MC/MCDwarf.h

194

Correct. I changed it to an assert.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

2170

I checked this condition at the call-site and updated the comments.

2174

Yeah. I deleted the check.

[kyulee]

Update changes per feedbacks.

[kyulee]

llvm/lib/MC/MCDwarf.cpp

232

Yeah. That sounds great, but it seems similar to what I initially tried, which tracked the end label of function as functions were added.
But I wonder whether there is a case where the end label of the prior function is not matched with the start label of the next function due to some alignment, etc? In that case, do we want to terminate the line table for the prior function because of mismatch? I guess that seems an overkill.

[kyulee]

Fix the test failures by restoring PrevCU check instead of assertion.

• Line table entrty insertion is independent of PrevCU creation.

[kyulee]

llvm/include/llvm/MC/MCDwarf.h

194

It turned out that there is the case where the line entry is empty in PrevCU. So I restored it with this check.

[kyulee]

llvm/lib/MC/MCDwarf.cpp

232

From the second thought, addLineEntry is used during function emission (which is independent of Range operation) while Range is added at the end of function. It doesn't seem to work to extend the line table using the end of function range.

[dblaikie]

llvm/lib/MC/MCDwarf.cpp

190–192

When does this occur? I guess when the last function with debug info in this section is followed by a function in another section (with or without debug info) or a nodebug function?

That does seem a bit subtle and like it'd be nicer if this API's invariant was just that all callers terminated their own lists. (this would involve fixing up the GenDwarfForAssembly codepath to do that too, I'd guess)

For DwarfDebug/etc I guess this would be addressed by calling terminateLineTableForPrevCU (or perhaps resetPrevCU) in finishModule or whatever it's called?

232

Yeah, though I was thinking more driven by the DwarfDebug code so it's not tracked in two different places that might diverge/reduce the code/logic needed to handle these cases.

I think alignment doesn't break this strategy (it doesn't seem to break it for the ranges data using this approach) - though the alignment does come between the start of one function and the beginning of the next function - the code in DwarfDebug/DwarfCompileUnit/etc extends the CU ranges to cover that & hopefully the line table could be powered by the same logic.

[kyulee]

llvm/lib/MC/MCDwarf.cpp

190–192

I can see we can terminate the line table for DwarfDebug in finishModule via resetPrevCU -- this will use an end label of range.

However, for assembly path, I don't think we have explicit labels/symbols to terminate. I may introduce an api to patch the line table for all sections that do not have an end entry at the end. But in that case, I still need to synthesize/emit a label using section symbol like MCStreamer::endSection. It's just moving the place when we fill this gap -- either here or before coming here.

[kyulee]

• Ensure the line table is terminated in DwarfDebug (compiler) path.
• Still preserve the old code to synthesize the end entry for the assembly path.

[dblaikie]

Looks roughly right - one trailing question I wouldn't mind knowing the answer to (might merit a comment so it can be cleaned up later) - and also, maybe worth adding an assembly test case. Something like this:

.text
.file   1 "small.c"
.loc    1 1 0
nop
.section .text.2
.loc    1 2 0
nop
.text
nop

Showing that line table locations in assembly do flow on to the next chunk of a section even if there's an intermediate section switch. So unlike with the DwarfDebug case, which can end the line table after the function/whenever switching compilation units - the assembly mode might have any number of outstanding sections that need to be terminated at the end. (I guess in theory we could optimize the IR/DwarfDebug case slightly by keeping some of these open - but I don't think it likely in practice & seems cleaner with what this patch does now)

llvm/include/llvm/MC/MCDwarf.h

194

When does that case arise? (maybe when a function is zero-length/has no instructions? That's something I/we would like to fix at some point, FWIW)

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

497 ↗

(On Diff #387053)

Unrelated change - could commit this separately.

[kyulee]

Terminate the line table aligning with Range creation:

• When a new range is formed due to section or CU change
• When nodebug function starts
• When module is finished

From the above, I think the line table is well-formed in DwarfDebug (normal compiler path).
No need specialization for asm streamer or object stremer to end the line table.

However, the MC path seems to still terminate using the section end symbol conservatively,
because we do not track the range like the DwarfDebug path as above.
So, I still preserve emitDwarfLineEndEntry when no end entry is emitted in the loop.

[kyulee]

Remove a white-space diff

[dblaikie]

llvm/lib/MC/MCDwarf.cpp

166

When does this situation come up? (I think this question got lost from its previous place here: https://reviews.llvm.org/D108261#inline-1086075 during refactoring)

(if it is necessary, it might be suitable to use find on the MCLineDivisions rather than operator[] (so this call doesn't create an entry when it's empty/unused))

[kyulee]

llvm/lib/MC/MCDwarf.cpp

166

Even though we have a range, we may not have any debug line entry in unit tests.

Makes sense using find. Will update it with additional assembly test.

[dblaikie]

llvm/lib/MC/MCDwarf.cpp

166

Seems like we shouldn't be adding extra API surface area only for unit tests, though? But it's possible this comes up in real cases of empty functions - if that's the case (worth testing/validating), leaving a FIXME for when we remove/fix the existence of empty functions would be good.

[kyulee]

• Add an assembly test to cover the MC path that still emits the end entry using the section end symbol.
• Add a check for addEndEntry before adding an end entry. The MCStreamer path may not populate the line entry.

[dblaikie]

llvm/lib/MC/MCDwarf.cpp

166

find is preferred over count - so that the result can be used rather than a duplicate lookup:

auto I = MCLineDivisions.find(Sec);
if (I != MCLineDivisions.end()) {
  auto &Entries = I->second;
  ...
}

Though I'm still curious to better understand under what conditions this is needed.

llvm/test/DebugInfo/XCOFF/empty.ll

230 ↗

(On Diff #387127)

Hmm, what aspect of the change caused these labels to change name?

[kyulee]

llvm/lib/MC/MCDwarf.cpp

166

It turned out the MCAsmStreamer path (as opposed to MCObjectStreamer) typically doesn't populate the line entry.
So, I keep this check to bail-out cloning/adding the end entry in that case. Or, hundreds of LIT tests failed.

166

Yea. find seems better since I need to use the map anyhow inside. Will update it.

As commented above in the code, the assembly output path (MCAsmStreamer as opposed to MCObjectStreamer), the line entry is not added instead .loc directives are emitted during the function emission. So, the line entries are often irrelevant in the assembly output except a certain target -- I guess XCOFF shown below in the tests.
I was thinking to check streamer and specialize this logic outside this, but not sure exactly how to do so.

llvm/test/DebugInfo/XCOFF/empty.ll

230 ↗

(On Diff #387127)

I think this XCOFF seems a unique path that generates the line table for the assembly output in DwarfDebug.
So, the new logic is still kicked in, which adds an entry based on the range end label (instead of the section end label in the fall-back path).
I think the range for function uses function labels, so that's why the change happens.
Although this is the same in this unit tests, in theory, I think this new change is more precise.

[kyulee]

Use find instead of count for a check in addEndEntry.

[dblaikie]

llvm/lib/MC/MCDwarf.cpp

166

Ah, OK! Right - the solution to that would be to have a virtual function in MCStreamer that's differently implemented (a no-op in the asm streamer case - maybe someone'll eventually extend the assembly syntax to support terminating line contributions - so nodebug could be properly done in assembly) this would be side-by-side with the MCStreamer::emitDwarfLocDirective - or, perhaps it could use that specific function, with an extra parameter or flag value for "end entry"?

[kyulee]

llvm/lib/MC/MCDwarf.cpp

166

Yea. I tried to create emitTerminateLineTable in MCStreamer and differently extends it for MCAsmStreamer, which resolved the most cases.
But I'm still seeing dozens of failures in unit tests which are probably because of incomplete debug data in the test cases that are manually synthesized.
For instance, DebugInfo/X86/multiple-at-const-val.ll, it was like

define i32 @main() !dbg !960 {
entry:
  %call1.i = tail call %"class.std::basic_ostream"* @test(%"class.std::basic_ostream"* @_ZSt4cout, i8* getelementptr inbounds ([6 x i8], [6 x i8]* @.str, i64 0, i64 0), i64 5)
  ret i32 0
}

This means the function appears to have a debug info, but there is no tag like DILocation for each line.
I presume although the above is unlikely the real case from the compiler, but I think this seems a valid case (opt or any transformation may drop debug tag) so we should cope with.
Having said that, I think this check seems worth being kept -- then I don't see the reason above to specialize streamer just for this.

[dblaikie]

Thanks for all the work/iteration/research here - sorry it was a bit fussy.

llvm/lib/MC/MCDwarf.cpp

166

Ah, hmm - yeah, there's certainly ways we could arrive at a function without any instructions having a location. Though I wonder what the line table should look like for that? Not having the line table cover those instructions seems a bit weird too.

But if that's the state of things today, might as well leave ti as-is. Could you include a comment describing how that occurs with the MCObjectStreamer use case as well as the one outlined with MCAsmStreamer?

[kyulee]

In D108261#3130364, @dblaikie wrote:

Thanks for all the work/iteration/research here - sorry it was a bit fussy.

Thanks for the thorough review! I had a chance to dive a bit in this area.
Indeed, this debug world seems subtle to make it right.

llvm/lib/MC/MCDwarf.cpp

166

For the above case where instructions have no location at all, the line entries in the table didn't appear in dwarf although other contents appeared.

Yeah. Will update the comments to include two cases (MCAsmStreamer and MCObjectStreamer).

[kyulee]

Update the comments in addEndEntry for the skipping reasons.

[kyulee]

Fix a typo in the comment

llvm/include/llvm/MC/MCDwarf.h

public:
   void addLineEntry(const MCDwarfLineEntry &LineEntry, MCSection *Sec) {
     MCLineDivisions[Sec].push_back(LineEntry);
   }
 
   using MCDwarfLineEntryCollection = std::vector<MCDwarfLineEntry>;
   using iterator = MCDwarfLineEntryCollection::iterator;
   using const_iterator = MCDwarfLineEntryCollection::const_iterator;
   using MCLineDivisionMap = MapVector<MCSection *, MCDwarfLineEntryCollection>;
 
 private:
   // A collection of MCDwarfLineEntry for each section.
   MCLineDivisionMap MCLineDivisions;
 
+  // Points to the end label of the last function range.
+  MCSymbol *EndLabel = nullptr;
+
 public:
   // Returns the collection of MCDwarfLineEntry for a given Compile Unit ID.
   const MCLineDivisionMap &getMCLineEntries() const {
     return MCLineDivisions;
   }
+
+  // Set the end label from the last function range.
+  void setEndLabel(MCSymbol *FuncEndLabel) { EndLabel = FuncEndLabel; }
+
+  // Get the end label for the CU.
+  MCSymbol *getEndLabel() const { return EndLabel; }
 };
 
 struct MCDwarfLineTableParams {
   /// First special line opcode - leave room for the standard opcodes.
   /// Note: If you want to change this, you'll have to update the
   /// "StandardOpcodeLengths" table that is emitted in
   /// \c Emit().
   uint8_t DWARF2LineOpcodeBase = 13;

llvm/include/llvm/MC/MCObjectStreamer.h

void emitValueToOffset(const MCExpr *Offset, unsigned char Value,
                          SMLoc Loc) override;
   void emitDwarfLocDirective(unsigned FileNo, unsigned Line, unsigned Column,
                              unsigned Flags, unsigned Isa,
                              unsigned Discriminator,
                              StringRef FileName) override;
   void emitDwarfAdvanceLineAddr(int64_t LineDelta, const MCSymbol *LastLabel,
                                 const MCSymbol *Label,
                                 unsigned PointerSize) override;
-  void emitDwarfLineEndEntry(MCSection *Section, MCSymbol *LastLabel) override;
+  void emitDwarfLineEndEntry(MCSection *Section, MCSymbol *LastLabel,
+                             MCSymbol *EndLabel) override;
   void emitDwarfAdvanceFrameAddr(const MCSymbol *LastLabel,
                                  const MCSymbol *Label);
   void emitCVLocDirective(unsigned FunctionId, unsigned FileNo, unsigned Line,
                           unsigned Column, bool PrologueEnd, bool IsStmt,
                           StringRef FileName, SMLoc Loc) override;
   void emitCVLinetableDirective(unsigned FunctionId, const MCSymbol *Begin,
                                 const MCSymbol *End) override;
   void emitCVInlineLinetableDirective(unsigned PrimaryFunctionId,

llvm/include/llvm/MC/MCStreamer.h

public:
   /// Return the end symbol generated inside, the caller needs to emit it.
   virtual MCSymbol *emitDwarfUnitLength(const Twine &Prefix,
                                         const Twine &Comment);
 
   /// Emit the debug line start label.
   virtual void emitDwarfLineStartLabel(MCSymbol *StartSym);
 
   /// Emit the debug line end entry.
-  virtual void emitDwarfLineEndEntry(MCSection *Section, MCSymbol *LastLabel) {}
+  virtual void emitDwarfLineEndEntry(MCSection *Section, MCSymbol *LastLabel,
+                                     MCSymbol *EndLabel) {}
 
   /// If targets does not support representing debug line section by .loc/.file
   /// directives in assembly output, we need to populate debug line section with
   /// raw debug line contents.
   virtual void emitDwarfAdvanceLineAddr(int64_t LineDelta,
                                         const MCSymbol *LastLabel,
                                         const MCSymbol *Label,
                                         unsigned PointerSize) {}

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::beginFunctionImpl(const MachineFunction *MF) {
   Asm->OutStreamer->getContext().setDwarfCompileUnitID(
       getDwarfCompileUnitID(CU));
 
   // Record beginning of function.
   PrologEndLoc = emitInitialLocDirective(
       *MF, Asm->OutStreamer->getContext().getDwarfCompileUnitID());
 }
 
 unsigned DwarfDebug::getDwarfCompileUnitID(const DwarfCompileUnit &CU) {
   // Set DwarfDwarfCompileUnitID in MCContext to the Compile Unit this function
   // belongs to so that we add to the correct per-cu line table in the
   // non-asm case.
   if (Asm->OutStreamer->hasRawTextSupport())
     // Use a single line table if we are generating assembly.
     return 0;
   else
     return CU.getUniqueID();
 }
 
 void DwarfDebug::skippedNonDebugFunction() {
   // If we don't have a subprogram for this function then there will be a hole
   // in the range information. Keep note of this by setting the previously used
   // section to nullptr.
   PrevCU = nullptr;
   CurFn = nullptr;
 }
 
 // Gather and emit post-function debug information.
 void DwarfDebug::endFunctionImpl(const MachineFunction *MF) {
   const DISubprogram *SP = MF->getFunction().getSubprogram();
 
   assert(CurFn == MF &&
       "endFunction should be called with the same function as beginFunction");
void DwarfDebug::endFunctionImpl(const MachineFunction *MF) {
   DenseSet<InlinedEntity> Processed;
   collectEntityInfo(TheCU, SP, Processed);
 
   // Add the range of this function to the list of ranges for the CU.
   // With basic block sections, add ranges for all basic block sections.
   for (const auto &R : Asm->MBBSectionRanges)
     TheCU.addRange({R.second.BeginLabel, R.second.EndLabel});
 
+  // Set the end label from the last function range to the CU's line table.
+  assert(!Asm->MBBSectionRanges.empty());
+  Asm->OutStreamer->getContext()
+      .getMCDwarfLineTable(getDwarfCompileUnitID(TheCU))
+      .getMCLineSections()
+      .setEndLabel(Asm->MBBSectionRanges.back().second.EndLabel);
+
   // Under -gmlt, skip building the subprogram if there are no inlined
   // subroutines inside it. But with -fdebug-info-for-profiling, the subprogram
   // is still needed as we need its source location.
   if (!TheCU.getCUNode()->getDebugInfoForProfiling() &&
       TheCU.getCUNode()->getEmissionKind() == DICompileUnit::LineTablesOnly &&
       LScopes.getAbstractScopesList().empty() && !IsDarwin) {
     assert(InfoHolder.getScopeVariables().empty());
     PrevLabel = nullptr;

llvm/lib/MC/MCAsmStreamer.cpp

public:
 
   void emitDwarfUnitLength(uint64_t Length, const Twine &Comment) override;
 
   MCSymbol *emitDwarfUnitLength(const Twine &Prefix,
                                 const Twine &Comment) override;
 
   void emitDwarfLineStartLabel(MCSymbol *StartSym) override;
 
-  void emitDwarfLineEndEntry(MCSection *Section, MCSymbol *LastLabel) override;
+  void emitDwarfLineEndEntry(MCSection *Section, MCSymbol *LastLabel,
+                             MCSymbol *EndLabel = nullptr) override;
 
   void emitDwarfAdvanceLineAddr(int64_t LineDelta, const MCSymbol *LastLabel,
                                 const MCSymbol *Label,
                                 unsigned PointerSize) override;
 
   void doFinalizationAtSectionEnd(MCSection *Section) override;
 };
 
if (!MAI->needsDwarfSectionSizeInHeader()) {
 
     emitAssignment(StartSym, OuterSym);
     return;
   }
   MCStreamer::emitDwarfLineStartLabel(StartSym);
 }
 
 void MCAsmStreamer::emitDwarfLineEndEntry(MCSection *Section,
-                                          MCSymbol *LastLabel) {
+                                          MCSymbol *LastLabel,
+                                          MCSymbol *EndLabel) {
   // If the targets write the raw debug line data for assembly output (We can
   // not switch to Section and add the end symbol there for assembly output)
   // we currently use the .text end label as any section end. This will not
   // impact the debugability as we will jump to the caller of the last function
   // in the section before we come into the .text end address.
   assert(!MAI->usesDwarfFileAndLocDirectives() &&
          ".loc should not be generated together with raw data!");
 

llvm/lib/MC/MCDwarf.cpp

 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/Config/config.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCObjectStreamer.h"
+#include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSection.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/EndianStream.h"
makeStartPlusIntExpr(MCContext &Ctx, const MCSymbol &Start, int IntVal) {
   const MCExpr *Res = MCBinaryExpr::create(MCBinaryExpr::Add, LHS, RHS, Ctx);
   return Res;
 }
 
 //
 // This emits the Dwarf line table for the specified section from the entries
 // in the LineSection.
 //
-static inline void emitDwarfLineTable(
-    MCStreamer *MCOS, MCSection *Section,
-    const MCLineSection::MCDwarfLineEntryCollection &LineEntries) {
+static inline void
+emitDwarfLineTable(MCStreamer *MCOS, MCSection *Section,
+                   const MCLineSection::MCDwarfLineEntryCollection &LineEntries,
+                   MCSymbol *EndLabel) {
   unsigned FileNum = 1;
   unsigned LastLine = 1;
   unsigned Column = 0;
   unsigned Flags = DWARF2_LINE_DEFAULT_IS_STMT ? DWARF2_FLAG_IS_STMT : 0;
   unsigned Isa = 0;
   unsigned Discriminator = 0;
   MCSymbol *LastLabel = nullptr;
 
   // Loop through each MCDwarfLineEntry and encode the dwarf line number table.
   for (const MCDwarfLineEntry &LineEntry : LineEntries) {
     int64_t LineDelta = static_cast<int64_t>(LineEntry.getLine()) - LastLine;
 
     if (FileNum != LineEntry.getFileNum()) {
       FileNum = LineEntry.getFileNum();
       MCOS->emitInt8(dwarf::DW_LNS_set_file);
       MCOS->emitULEB128IntValue(FileNum);
     }
     if (Column != LineEntry.getColumn()) {
       Column = LineEntry.getColumn();
       MCOS->emitInt8(dwarf::DW_LNS_set_column);
       MCOS->emitULEB128IntValue(Column);
     }
     if (Discriminator != LineEntry.getDiscriminator() &&
         MCOS->getContext().getDwarfVersion() >= 4) {
       Discriminator = LineEntry.getDiscriminator();
       unsigned Size = getULEB128Size(Discriminator);
       MCOS->emitInt8(dwarf::DW_LNS_extended_op);
       MCOS->emitULEB128IntValue(Size + 1);
       MCOS->emitInt8(dwarf::DW_LNE_set_discriminator);
       MCOS->emitULEB128IntValue(Discriminator);
     }
MCOS->emitDwarfAdvanceLineAddr(LineDelta, LastLabel, Label,
                                    asmInfo->getCodePointerSize());
 
     Discriminator = 0;
     LastLine = LineEntry.getLine();
     LastLabel = Label;
   }
 
   // Generate DWARF line end entry.
-  MCOS->emitDwarfLineEndEntry(Section, LastLabel);
+  MCOS->emitDwarfLineEndEntry(Section, LastLabel, EndLabel);
 }
 
 //
 // This emits the Dwarf file and the line tables.
 //
 void MCDwarfLineTable::emit(MCStreamer *MCOS, MCDwarfLineTableParams Params) {
   MCContext &context = MCOS->getContext();
 

 }
 
 void MCDwarfLineTable::emitCU(MCStreamer *MCOS, MCDwarfLineTableParams Params,
                               Optional<MCDwarfLineStr> &LineStr) const {
   MCSymbol *LineEndSym = Header.Emit(MCOS, Params, LineStr).second;
 
   // Put out the line tables.
   for (const auto &LineSec : MCLineSections.getMCLineEntries())
-    emitDwarfLineTable(MCOS, LineSec.first, LineSec.second);
+    emitDwarfLineTable(MCOS, LineSec.first, LineSec.second,
+                       MCLineSections.getEndLabel());
 
   // This is the end of the section, so set the value of the symbol at the end
   // of this section (that was used in a previous expression).
   MCOS->emitLabel(LineEndSym);
 }
 
 Expected<unsigned> MCDwarfLineTable::tryGetFile(StringRef &Directory,
                                                 StringRef &FileName,

llvm/lib/MC/MCObjectStreamer.cpp

if (AddrDelta->evaluateAsAbsolute(Res, getAssemblerPtr())) {
     MCDwarfLineAddr::Emit(this, Assembler->getDWARFLinetableParams(), LineDelta,
                           Res);
     return;
   }
   insert(new MCDwarfLineAddrFragment(LineDelta, *AddrDelta));
 }
 
 void MCObjectStreamer::emitDwarfLineEndEntry(MCSection *Section,
-                                             MCSymbol *LastLabel) {
-  // Emit a DW_LNE_end_sequence for the end of the section.
-  // Use the section end label to compute the address delta and use INT64_MAX
+                                             MCSymbol *LastLabel,
+                                             MCSymbol *EndLabel) {
+  // Emit a DW_LNE_end_sequence with EndLabel. If EndLabel is null,
+  // use the section end label to compute the address delta and use INT64_MAX
   // as the line delta which is the signal that this is actually a
   // DW_LNE_end_sequence.
-  MCSymbol *SectionEnd = endSection(Section);
+  if (!EndLabel)
+    EndLabel = endSection(Section);
 
   // Switch back the dwarf line section, in case endSection had to switch the
   // section.
   MCContext &Ctx = getContext();
   SwitchSection(Ctx.getObjectFileInfo()->getDwarfLineSection());
 
   const MCAsmInfo *AsmInfo = Ctx.getAsmInfo();
-  emitDwarfAdvanceLineAddr(INT64_MAX, LastLabel, SectionEnd,
+  emitDwarfAdvanceLineAddr(INT64_MAX, LastLabel, EndLabel,
                            AsmInfo->getCodePointerSize());
 }
 
 void MCObjectStreamer::emitDwarfAdvanceFrameAddr(const MCSymbol *LastLabel,
                                                  const MCSymbol *Label) {
   const MCExpr *AddrDelta = buildSymbolDiff(*this, Label, LastLabel);
   int64_t Res;
   if (AddrDelta->evaluateAsAbsolute(Res, getAssemblerPtr())) {

llvm/test/DebugInfo/debugline-endsequence.ll

+; RUN: %llc_dwarf %s -filetype=obj -o - | llvm-dwarfdump -all - | FileCheck %s
+
+; CHECK: DW_AT_name ("<stdin>")
+; CHECK: DW_AT_high_pc ([[EndSeq:.*]])
+
+; Expect the line table ends with the highest address for each CU.
+; CHECK: [[EndSeq]] [[T:.*]] end_sequence
+
+target triple = "x86_64-apple-macosx11.0.0"
+
+define void @f1() !dbg !10 {
+  ret void, !dbg !13
+}
+
+define void @f2() {
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6, !7, !8}
+!llvm.ident = !{!9}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "LLVM", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None, sysroot: "/")
+!1 = !DIFile(filename: "<stdin>", directory: "/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{i32 7, !"uwtable", i32 1}
+!8 = !{i32 7, !"frame-pointer", i32 2}
+!9 = !{!"clang"}
+!10 = distinct !DISubprogram(name: "f1", scope: !1, file: !1, line: 1, type: !11, scopeLine: 1, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!11 = !DISubroutineType(types: !12)
+!12 = !{null}
+!13 = !DILocation(line: 1, column: 13, scope: !10)