Prevent line 0 instructions from dividing a lexical block into ranges

[ykhatav]

Summary

Certain instructions with line 0 source correlation create discontiguous/split address ranges in lexical blocks. This affects Visual Studio's ability to print variables reliably because Codeview debug format used on Windows does not support describing a lexical block with multiple address ranges. As a result, LLVM compiler's Codeview emission code evaporates these lexical blocks and pushes variables to the enclosing routine scope, causing gaps in their location ranges and potential confusion with similarly named variables in the debugger. While this issue is also visible on Linux, the Dwarf debugging format can handle it without impacting debugging.

To prevent the line 0 instructions from breaking up a lexical block we fold the line zero machine instructions into the range when the switch "UnknownLocations" is not Enabled. This essentially removes the split address ranges and allows the lexical block to be treated as a contiguous range of instructions.

[ykhatav]

Nit:test-fix

[ykhatav]

Fix metadata numbering

[ykhatav]

ping

[probinson]

It seems strange to define the enum in a header for a module that uses it only once, and externalize it in a module that uses it several times.

I'd always thought this DefaultOnOff business should be generalized... Hmm I see boolOrDefault in CommandLine.h. Maybe we could convert UnknownLocations to use that, remove static from the definition, and then LexicalScopes.cpp can reference it as an extern.

(Converting the rest of the DwarfDebug.cpp options from DefaultOnOff to boolOrDefault should not be part of your task. But maybe add a FIXME on the enum definition.)

[ykhatav]

In D144337#4167827, @probinson wrote:

It seems strange to define the enum in a header for a module that uses it only once, and externalize it in a module that uses it several times.

I'd always thought this DefaultOnOff business should be generalized... Hmm I see boolOrDefault in CommandLine.h. Maybe we could convert UnknownLocations to use that, remove static from the definition, and then LexicalScopes.cpp can reference it as an extern.
I agree with you that UnknownLocations may be better defined with boolorDefault instead of DefaultOnOff. I can change that.

However, the reason I moved the definition of UnknownLocations to LexicalScopes.cpp was to avoid a link-time error. Defining UnknownLocations as extern in LexicalScopes.cpp gives a link-time error despite removing the "static" keyword from its definition in DwarfDebug.cpp. I see that the two object files are in different directories and while building the LexicalScopes.cpp file it is unable to find the definition of UnknownLocations. I am aware that it may not be the optimal approach so I would appreciate any feedback or consider alternative methods.

(Converting the rest of the DwarfDebug.cpp options from DefaultOnOff to boolOrDefault should not be part of your task. But maybe add a FIXME on the enum definition.)

[probinson]

Defining UnknownLocations as extern in LexicalScopes.cpp gives a link-time error despite removing the "static" keyword from its definition in DwarfDebug.cpp.

Ah, the layering is in the other direction than I thought (AsmPrinter depends on CodeGen, not vice versa). Okay, it has to be defined in LexicalScopes and made extern from DwarfDebug.

Please do change over to boolOrDefault for this one flag, which means you can leave the DefaultOnOff enum in DwarfDebug.cpp.

[dblaikie]

Does this need a flag? The patch describes doing this for CodeView? Could/should it be done unconditionally for CV?

Though I'll mention that there's a risk of messing up profiles with this - those zero line instructions do have scopes and other things attached to them, if they were of the appropriate scope, even with a zero line, then it wouldn't be fragmenting the scope. So it's because the location isn't attributed to the inline scope that it's causing fragmentation - but that means the instruction doesn't necessarily come from that scope, and so a sample based profile might incorrectly conclude something about reachability if it is instead attributed to this scope... maybe?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

68 ↗

(On Diff #498842)

We shouldn't introduce declarations of foreign entities into .cpp files like this (there's a risk of the decl/def getting out of date/inconsistent with each other) - there should be some common header to declare this in. Perhaps that means it goes up in LexicalScopes.h?

[ykhatav]

In D144337#4172962, @dblaikie wrote:

Does this need a flag? The patch describes doing this for CodeView? Could/should it be done unconditionally for CV?

When UnknownLocations is left at default, the line table includes line 0 entries, and the lexical scopes utilize them while computing the ranges. However, if the unknown locations are disabled, the line table already ignores line 0 entries. This patch modifies the lexical scope behavior to ignore line 0 entries when UnknownLocations is not enabled. If this change is applied unconditionally, the line table entries and lexical scope will not match, causing inconsistencies
Although the DWARF format has the ability to describe the split address ranges, it is possible that the lexical blocks with split address ranges can result in buggy behavior on Linux as well.

[ykhatav]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

68 ↗

(On Diff #498842)

Unfortunately, declaring it in LexicalScopes.h does not work due to dependency issue between AsmPrinter and Codegen.

[ykhatav]

Change UnknownLocations to use boolorDefault

[dblaikie]

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

68 ↗

(On Diff #498842)

Could you describe the dependency issue in more detail? I think it's important that we figure out the layering & where things should be declared - not introduce multiple independent declarations of the same entity.

[ykhatav]

In D144337#4183551, @dblaikie wrote:

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

The reason why we want to check for “UnknownLocations” flag is that this flag is used to check if we want to add line 0 entries in the line table (in DwarfDebug.cpp) and thereby controlling whether or not line zero instructions are represented in the debug information. If we don’t check for this flag and skip the line 0 entries while extracting lexical scopes there will be a mismatch in the line table entries and lexical scopes.

[ykhatav]

Move UnknownLocations declaration to LexicalScopes.h

[ykhatav]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

68 ↗

(On Diff #498842)

Sorry, I misunderstood what you said.. I was able to move "UnknownLocations" to LexicalScope.h

[dblaikie]

In D144337#4185295, @ykhatav wrote:

In D144337#4183551, @dblaikie wrote:

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

The reason why we want to check for “UnknownLocations” flag is that this flag is used to check if we want to add line 0 entries in the line table (in DwarfDebug.cpp) and thereby controlling whether or not line zero instructions are represented in the debug information. If we don’t check for this flag and skip the line 0 entries while extracting lexical scopes there will be a mismatch in the line table entries and lexical scopes.

Sorry, still not quite following. Could you walk me through an example of the bug you're trying to address with this patch? And an example of the bug you're trying to avoid introducing/that causes you to need this extra UnknownLocations checking?

[ykhatav]

In D144337#4190626, @dblaikie wrote:

In D144337#4185295, @ykhatav wrote:

In D144337#4183551, @dblaikie wrote:

Could you describe why this relates to/depends on the "UnknownLocations" flag? I'm not following from the description - we could still describe these locations as being at line zero, while including them in lexical scopes to avoid fragmented ranges, I think?

The reason why we want to check for “UnknownLocations” flag is that this flag is used to check if we want to add line 0 entries in the line table (in DwarfDebug.cpp) and thereby controlling whether or not line zero instructions are represented in the debug information. If we don’t check for this flag and skip the line 0 entries while extracting lexical scopes there will be a mismatch in the line table entries and lexical scopes.

Sorry, still not quite following. Could you walk me through an example of the bug you're trying to address with this patch? And an example of the bug you're trying to avoid introducing/that causes you to need this extra UnknownLocations checking?

If you consider the attached test case (llvm/test/DebugInfo/X86/contiguous-lexical-block.ll) you can see that the lexical block is fragmented into ranges due to the presence of an instruction having line number 0. This I believe is a bug and If the same test case is run on windows, Visual Studio' can't reliably print variable "i" because CodeView does not provide a mechanism for describing a lexical block with more than one address ranges. In order to prevent the line 0 instructions from breaking up a lexical block, in this patch, we fold the line zero machine instructions into the range.
I don't have a test case that demonstrates whether using or not using UnknownLocations could lead to a bug. It is an internal flag and is suitable for controlling whether or not line zero instructions are represented in the debug information and so, in my opinion, can be used to control the scope tree as well.. Do you have any other flags in mind that can be used instead of "UnknownLocations"?

[dblaikie]

If you consider the attached test case (llvm/test/DebugInfo/X86/contiguous-lexical-block.ll) you can see that the lexical block is fragmented into ranges due to the presence of an instruction having line number 0. This I believe is a bug and If the same test case is run on windows, Visual Studio' can't reliably print variable "i" because CodeView does not provide a mechanism for describing a lexical block with more than one address ranges. In order to prevent the line 0 instructions from breaking up a lexical block, in this patch, we fold the line zero machine instructions into the range.

Sure, I'm on-board there. I'll take your word for it that CV can't represent discontiguous scopes and making scopes contiguous is the better tradeoff than omitting them entirely.

I don't have a test case that demonstrates whether using or not using UnknownLocations could lead to a bug. It is an internal flag and is suitable for controlling whether or not line zero instructions are represented in the debug information and so, in my opinion, can be used to control the scope tree as well.. Do you have any other flags in mind that can be used instead of "UnknownLocations"?

I don't understand this bit - there can be zero locations for other reasons, apart from UnknownLocations - you could just check for the zero locations, without needing to check UnknownLocations too, right?

But maybe you're using UnknownLocations as a proxy for CV/MSVC debug info emission? That seems unfortunate/not suitable, and this behavior should be gated on/more directly test for CV output, I think?

@hans maybe you've got thoughts on this stuff

[hans]

In D144337#4194463, @dblaikie wrote:

@hans maybe you've got thoughts on this stuff

Sorry, I don't know much about Codeview. @zequanwu knows more.

[ykhatav]

In D144337#4194463, @dblaikie wrote:

If you consider the attached test case (llvm/test/DebugInfo/X86/contiguous-lexical-block.ll) you can see that the lexical block is fragmented into ranges due to the presence of an instruction having line number 0. This I believe is a bug and If the same test case is run on windows, Visual Studio' can't reliably print variable "i" because CodeView does not provide a mechanism for describing a lexical block with more than one address ranges. In order to prevent the line 0 instructions from breaking up a lexical block, in this patch, we fold the line zero machine instructions into the range.

Sure, I'm on-board there. I'll take your word for it that CV can't represent discontiguous scopes and making scopes contiguous is the better tradeoff than omitting them entirely.

I don't have a test case that demonstrates whether using or not using UnknownLocations could lead to a bug. It is an internal flag and is suitable for controlling whether or not line zero instructions are represented in the debug information and so, in my opinion, can be used to control the scope tree as well.. Do you have any other flags in mind that can be used instead of "UnknownLocations"?

I don't understand this bit - there can be zero locations for other reasons, apart from UnknownLocations - you could just check for the zero locations, without needing to check UnknownLocations too, right?

But maybe you're using UnknownLocations as a proxy for CV/MSVC debug info emission? That seems unfortunate/not suitable, and this behavior should be gated on/more directly test for CV output, I think?

No, my intent was not to use UnknownLocations as a proxy for CV, but I think it is safe not to fold line 0 instructions into a scope range when UnknownLocations is enabled. AFAIK, when UnknownLocations is enabled line 0 entries are emitted in the debug information. In my opinion, Folding these instructions when the switch is enabled could break stuff.

@hans maybe you've got thoughts on this stuff

[dblaikie]

No, my intent was not to use UnknownLocations as a proxy for CV, but I think it is safe not to fold line 0 instructions into a scope range when UnknownLocations is enabled. AFAIK, when UnknownLocations is enabled line 0 entries are emitted in the debug information. In my opinion, Folding these instructions when the switch is enabled could break stuff.

Yeah, I'm still really not following this. UnknownLocations isn't the only source of zero lines (I think the most obvious/easy example is, if I recall correctly, line zero is used for merged function calls). And even if it was, I don't understand why it would need to be tested in LexicalScopes.cpp where the line can be tested for zero directly, right? So why do we need to test anything else other than that it's line zero?

[probinson]

(I think the most obvious/easy example is, if I recall correctly, line zero is used for merged function calls)

Actually, any merged instructions not originally from the same source location. (Because we don't have a way to represent multiple source locations for a single instruction.)

I agree with @dblaikie, for CV you can just eliminate the line-zero records without reference to UnknownLocations.

[ykhatav]

I will open a design discussion to discuss what an appropriate flag will be to control this behavior.

llvm/test/DebugInfo/X86/contiguous-lexical-block.ll

 !8 = !{!9}
 !9 = !DILocalVariable(name: "i", scope: !10, file: !3, line: 17, type: !13)
 !10 = distinct !DILexicalBlock(scope: !11, file: !3, line: 34, column: 1)
 !11 = distinct !DILexicalBlock(scope: !12, file: !3, line: 30, column: 3)
 !12 = distinct !DILexicalBlock(scope: !5, file: !3, line: 29, column: 1)
 !13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
 !14 = !DILocation(line: 34, column: 1, scope: !10)
 !15 = !DILocation(line: 0, scope: !16)
-!16 = !DILexicalBlockFile(scope: !5, file: !17, discriminator: 0)
-!17 = !DIFile(filename: "CPU_DEVICE_RT", directory: "/")
+!16 = !DILexicalBlockFile(scope: !5, file: !3, discriminator: 0)
 !18 = !DILocation(line: 36, column: 21, scope: !19)
 !19 = distinct !DILexicalBlock(scope: !20, file: !3, line: 36, column: 7)
 !20 = distinct !DILexicalBlock(scope: !21, file: !3, line: 36, column: 7)
 !21 = distinct !DILexicalBlock(scope: !10, file: !3, line: 35, column: 5)
 !22 = !DILocation(line: 37, column: 23, scope: !19)
 !23 = !DILocation(line: 36, column: 27, scope: !19)