[PATCH] D42926: [CodeView] Initial support for emitting S_BLOCK32 symbols for lexical scopes

[Brock Wyma via Phabricator via llvm-commits]

bwyma marked an inline comment as done.
bwyma added a comment.

> Did you confirm that when stepping through a program in the debugger that variables are correctly shown in the Autos window depending on the source line?

Yes.  When stopping at a machine instruction, only the locals visible from the corresponding lexical scope are displayed.

> So, I guess nesting S_DEFRANGE inside S_BLOCK32 works in VS?

I have not noticed any problems with Visual Studio 2015 or 2017.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:369
+    // This variable goes into the corresponding lexical scope.
+    ScopeVariables[LS].emplace_back(Var);
   }
----------------
rnk wrote:
> I suspect we'll want to move the logic in `DwarfFile::addScopeVariable` into DebugHandlerBase so that parameters come out in the right order, etc.
The parameter ordering is already established in CodeViewDebug::emitLocalVariableList(...).
If we want to merge the two implementations into DebugHandlerBase then I think it should be done as a separate patch.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2311-2314
+  OS.AddComment("PtrParent"); 
+  OS.EmitIntValue(0, 4);                                  // PtrParent 
+  OS.AddComment("PtrEnd"); 
+  OS.EmitIntValue(0, 4);                                  // PtrEnd 
----------------
rnk wrote:
> zturner wrote:
> > Shouldn't we be emitting the actual values for `PtrParent` and `PtrEnd` here?
> No, these are always zero in the object file.
As Reid mentioned, the compiler is required to emit these fields as zero.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:2375
+  const SmallVectorImpl<InsnRange> &Ranges = Scope.getRanges();
+  if (Ranges.size() != 1 || !getLabelAfterInsn(Ranges.front().second)) {
+    // This lexical block scope does not have a valid address range, or has
----------------
rnk wrote:
> I suppose this works. I was going to try widening the scope to include the discontiguous portion in the middle, so the debugger would incorrectly think some extra variables were in scope in that code.
> 
> We should get a test case for this. I think you can do some stuff with noreturn or __builtin_expect to get us to sink the cold code to the bottom of the function.
Covering all of the dis-contiguous ranges of the lexical block with a single range is actually a bad idea.  I added a comment (below) which describes why so someone doesn't accidentally do this.

I added a lexical block to the test in lexicalblock.ll which contains a __builtin_expect call which causes the code within the lexical block to be split into two pieces and generate two address ranges for which we emit only the first.


https://reviews.llvm.org/D42926