Page MenuHomePhabricator

[CodeView] Initial support for emitting S_BLOCK32 symbols for lexical scopes
ClosedPublic

Authored by bwyma on Feb 5 2018, 11:48 AM.

Details

Summary

Right now, when two variables from different lexical scopes have the same name they are lumped together in the routine scope.
This can cause Visual Studio to display both variables simultaneously in the locals window and report "Variable is optimized away and not available" when referencing one of these variables in the watch window. For example:

if (...) {
  int local = ...;
} else {
  int local = ...;
}

This patch adds initial CodeView support for emitting S_BLOCK32 symbols to describe the lexical scopes containing these variables. Because the CodeView encoding of this symbol only permits a single begin and end location it is not possible to describe lexical scopes with multiple address ranges. Furthermore, this patch makes no attempt to sort variables into lexical blocks originating from inline functions as this functionality is better implemented incrementally.

This patch includes a new test, lexicalblock.ll, which demonstrates what the CodeView output will look like.
The test register-variables.ll required a minor update to change the order in which variables 'a' and 'c' are emitted due to the emission of the lexical blocks.

Diff Detail

Repository
rL LLVM

Event Timeline

bwyma created this revision.Feb 5 2018, 11:48 AM

Very excited to see someone doing this. We've been meaning to do it for some time but always get sidetracked on other things.

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?

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
361 ↗(On Diff #132858)

Looks like this can be marked const?

2311–2314 ↗(On Diff #132858)

Shouldn't we be emitting the actual values for PtrParent and PtrEnd here?

rnk added a comment.Feb 5 2018, 5:58 PM

Thanks! So, I guess nesting S_DEFRANGE inside S_BLOCK32 works in VS? I thought we did experiments to show that it didn't, but hey, if it works, great.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
369 ↗(On Diff #132858)

I suspect we'll want to move the logic in DwarfFile::addScopeVariable into DebugHandlerBase so that parameters come out in the right order, etc.

2311–2314 ↗(On Diff #132858)

No, these are always zero in the object file.

2375 ↗(On Diff #132858)

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.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
131 ↗(On Diff #132858)

The same scope can appear multiple times in the same function after inlining:

int getval();
void useit(int);
static inline void g(int cond) {
  if (cond) {
    int x = getval();
    useit(x);
  }
}
void f() {
  g(1);
  g(2);
}

They key here isn't quite right. This is replicating some of the functionality of the LexicalScopes class.

bwyma updated this revision to Diff 134428.Feb 15 2018, 7:37 AM

Thanks everyone for the wonderful feedback! I'm uploading a new diff with the requested changes thus far.
I will provide additional answers for some of the feedback I received after uploading this patch.

bwyma marked an inline comment as done.Feb 15 2018, 8:14 AM

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.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
369 ↗(On Diff #132858)

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.

2311–2314 ↗(On Diff #132858)

As Reid mentioned, the compiler is required to emit these fields as zero.

2375 ↗(On Diff #132858)

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.

majnemer added inline comments.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2374–2375 ↗(On Diff #134428)

This may be a silly question but I am curious... Could one emit a S_BLOCK32 entry for each range?

bwyma added inline comments.Feb 20 2018, 5:28 AM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2374–2375 ↗(On Diff #134428)

We could emit a S_BLOCK32 entry for each address range, but for each one we would also need to emit each child scope and contained variable which is live within that range. And you would probably not want to emit all of the ranges where a variable is live when the parent lexical scope only covers one of them, so the location description of each variable instance would be dependent upon the range covered by the specific branch of the lexical scope tree the variable resides in.

I suspect if there was a critical need to solve this properly then CodeView would have been extended to support ranges on lexical blocks. This may still happen someday. Until then, the worst-case scenario here is the compiler emits exactly what it does today... no lexical block.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
131 ↗(On Diff #132858)

In the patch summary, I stated:

Furthermore, this patch makes no attempt to sort variables into lexical blocks originating from inline functions as this functionality is better implemented incrementally.

I did change the key to DILexicalBlockBase as this makes more sense.

rnk accepted this revision.Feb 28 2018, 4:36 PM

I'm happy with this. I think we should go ahead and land this and iterate on it in tree. I think we might be able to simplify and share some logic between InlineSite and LexicalScope.

This revision is now accepted and ready to land.Feb 28 2018, 4:36 PM

Did this get committed?

bwyma added a comment.Mar 9 2018, 11:59 AM

Did this get committed?

Not yet. It looks like I'll have time to commit it on Monday if that is okay?

rnk added a comment.Mar 14 2018, 6:19 PM

Did this get committed?

Not yet. It looks like I'll have time to commit it on Monday if that is okay?

I'd like to see this sooner than later, so let us know. :)

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Mar 15 2018, 2:02 PM

This had hashtable UAF issues. The fix for it is to move the FunctionInfo out of the DenseMap so that its members become stable.

rnk added a comment.Mar 15 2018, 2:27 PM

I re-landed this as rL327670.