Page MenuHomePhabricator

[PDB] Support PDB-backed expressions evaluation
ClosedPublic

Authored by aleksandr.urakov on Oct 26 2018, 6:12 AM.

Details

Summary

This patch contains several small fixes, which makes it possible to evaluate expressions on Windows using information from PDB. The changes are:

  • several sanitize checks;
  • make IRExecutionUnit::MemoryManager::getSymbolAddress to not return a magic value on a failure, because callers wait 0 in this case;
  • entry point required to be a file address, not RVA, in the ObjectFilePECOFF;
  • do not crash on a debuggee second chance exception - it may be an expression evaluation crash;
  • create parameter declarations for functions in AST to make it possible to call debugee functions from expressions;
  • relax name searching rules for variables, functions, namespaces and types. Now it works just like in the DWARF plugin;
  • fix endless recursion in SymbolFilePDB::ParseCompileUnitFunctionForPDBFunc.

Each change is small and it is hard to test each change separately, so I think that create one review for them all is not a bad idea, especially because they make together the test to work.

I remember about the new native PDB plugin, but these changes are old enough, for last two or three weeks I'm just releasing my stash :) And some of the changes may be useful for the new plugin too.

This review depends on D52461, D52618, D53368, x64 testing depends on D53753.

Diff Detail

Repository
rL LLVM

Event Timeline

Hui added a subscriber: Hui.Oct 26 2018, 8:25 AM
Hui added inline comments.
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
819 ↗(On Diff #171284)

This still needs to be the offset into the section.

A couple of comments, but looks good otherwise. I'd wait for someone else to have a look as well.

source/Plugins/Process/Windows/Common/ProcessWindows.cpp
959 ↗(On Diff #171284)

Are we going to determine later whether it is supposed to be a crash or just never crash on second chance exceptions?

source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
1085 ↗(On Diff #171284)

Nit: Maybe add curly braces here. Since the if statement has them, the code is easier to read if the else does as well.

This revision is now accepted and ready to land.Oct 26 2018, 10:21 AM
aleksandr.urakov marked 2 inline comments as done.Oct 29 2018, 5:58 AM
aleksandr.urakov added inline comments.
source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
819 ↗(On Diff #171284)

If we are here, then we can't retrieve a section list, then we can't specify a section offset here. Setting an offset without setting a section means that offset must be treated as a file address (see e.g. the implementation of the Address::GetFileAddress() function). I think that misleading here is the name of the variable offset, I'll fix it, thanks.

source/Plugins/Process/Windows/Common/ProcessWindows.cpp
959 ↗(On Diff #171284)

Yes, it's a good question... I can't understand how to figure out at this point if the exception was thrown during an expression evaluation or a debuggee execution. On other hand, it seems that Visual Studio also doesn't crash an application in such situations:

int main() {
  char *buf = nullptr;
  buf[0] = 0; // Unhandled exception is here, but you can safely continue an execution if you will drag an arrow on the next line in Visual Studio
  int x = 5;
  return x - 5;
}

Also it was the only place where eStateCrashed was set, so it seems that on other platforms we do not crash a debuggee in such situations too. So may be it's not a bad idea to not set the crashed state here at all. But if someone don't agree with me and has an idea how to do it better, I'm ready to fix it.

source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
1085 ↗(On Diff #171284)

Sure, thanks!

aleksandr.urakov marked 2 inline comments as done.

Update the diff according to comments.

Ping!

Can someone to have a look as well?

asmith accepted this revision.Nov 27 2018, 5:09 AM

LGTM

labath added a subscriber: labath.Nov 27 2018, 7:36 AM
labath added inline comments.
source/Core/RichManglingContext.cpp
134–135 ↗(On Diff #171489)

Why is this necessary? It looks like somebody is misusing the returned StringRef by assuming that it always points to at least a single valid byte (which is definitely not the case in general, even for StringRefs with a non-null data()).

It would be better to fix the caller instead.

aleksandr.urakov marked 2 inline comments as done.Nov 28 2018, 12:20 AM
aleksandr.urakov added inline comments.
source/Core/RichManglingContext.cpp
134–135 ↗(On Diff #171489)

Yes, you are right. This change was made much time ago, I've fixed in this way the same problem, which Aaron have fixed in D52626, and have forgot to remove this. Thanks for pointing to this!

This revision was automatically updated to reflect the committed changes.
aleksandr.urakov marked an inline comment as done.

It looks like after this change went in, the tests on Windows starting timing out instead of completing. The last run before the change took about 2 minutes and starting with the first run with this change, they're running for 40 minutes before being killed:

After:
http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1774

Before:
http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1773

Can you fix this today or pull the change out?