[PATCH] D108261: [DebugInfo] Fix end_sequence of debug_line in LTO Object

Fri Sep 10 17:34:10 PDT 2021

dblaikie added a comment.

In D108261#2966644 <https://reviews.llvm.org/D108261#2966644>, @kyulee wrote:

> In D108261#2966544 <https://reviews.llvm.org/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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108261/new/

https://reviews.llvm.org/D108261