<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 13, 2015 at 10:32 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Good question. I mentioned it in one of the very first commit messages, but I guess that's been a while with many more commit messages since then, so it's been lost.<br><br>Basically, the dwarf tests are tests against llvm-dwarfdump. So without llvm-pdbdump, there can't be any tests. I could iterate, as you said, but right now llvm-pdbdump doesn't really support many command line options or ways to specify what to dump. It just dumps everything. Which is often very slow and pollutes the output, making it difficult to check that only the thing you care about is the way you expect and also making the test suite run slowly. Also, it's not just that I'm iterating right now, it's that some stuff just prints wrong, or doesn't print at all.</div></blockquote><div><br>Slow? Usually the inputs (reduced test cases) are so small that we're dealing with a tiny amount of debug info. Are the APIs/COM stuff so abysmally slow as to be observable even on such small test cases?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Most of those kinks are worked out though, and the patch I'm working on right now is one to finally add the correct options for limiting the output to specific interesting things. </div></div></blockquote><div><br>We didn't have that in llvm-dwarfdump until a year or two ago. Before that we'd just dump it all (but, again, in a reduced test case so there wasn't a /lot/ of output) & CHECK the bits the test was interested in.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>After that, I plan to start adding tests.<br></div><div><br></div><div>One question I have, is that some information about what's in the PDB you can only learn by looking in it. How would one test that? It doesn't seem very useful to look in the PDB, then write a test that it matches what you know it already is. I guess it's useful for verifying someone doesn't break the formatting. What are your thoughts there?</div></div></blockquote><div><br>My basic strategy is "if something got fixed/change, it's something that can be tested" (& my bar on "can be" to "should be" is pretty low - much lower than other people's). If there are improvements to the printing of functions and signatures I'd expect to see a test showing what the current format looks like (ideally, maybe, even a change to an existing test so I could look at the CHECK line diffs in the commit and see what the improvement was - what we did before, and what we do now).<br><br>Even if it's not finished, it's a great way to iteratively build up the test suite, avoid regressing one piece you fixed yesterday when you go to improve something else tomorrow, and saves going back at the end and trying to build up a comprehensive test suite - doing it iteratively seems easier & more likely to be thorough. (doing it at the end/after a lot of development it's hard to remember all the interesting corner cases you had to address along the way)<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote">On Fri Feb 13 2015 at 10:27:32 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Feb 13, 2015 at 9:57 AM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: zturner<br>
Date: Fri Feb 13 11:57:09 2015<br>
New Revision: 229129<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=229129&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=229129&view=rev</a><br>
Log:<br>
llvm-pdbdump: Improve printing of functions and signatures.<br>
<br>
This correctly prints the function pointers, and also prints<br>
function signatures for symbols as opposed to just types. So<br>
actual functions in your program will now be printed with full<br>
name and signature, as opposed to just name as before.<br></blockquote></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br>What's the story on testing this stuff? llvm-dwarfdump is tested with checked in binaries (though I realize pdbdump tests would only be runnable on Windows (where the library is available)) and it'd be nice to see the actual dumping format in tests changing/improving as you iterate here.<br> </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Modified:<br>
llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolFunc.h<br>
llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolTypeFunctionSig.h<br>
llvm/trunk/lib/DebugInfo/PDB/PDBSymbolExe.cpp<br>
llvm/trunk/lib/DebugInfo/PDB/PDBSymbolFunc.cpp<br>
llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypeFunctionSig.cpp<br>
llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypePointer.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolFunc.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolFunc.h?rev=229129&r1=229128&r2=229129&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolFunc.h?rev=229129&r1=229128&r2=229129&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolFunc.h (original)<br>
+++ llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolFunc.h Fri Feb 13 11:57:09 2015<br>
@@ -24,6 +24,8 @@ public:<br>
<br>
void dump(raw_ostream &OS, int Indent, PDB_DumpLevel Level) const override;<br>
<br>
+ std::unique_ptr<PDBSymbolTypeFunctionSig> getSignature() const;<br>
+<br>
DECLARE_PDB_SYMBOL_CONCRETE_TYPE(PDB_SymType::Function)<br>
<br>
FORWARD_SYMBOL_METHOD(getAccess)<br>
<br>
Modified: llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolTypeFunctionSig.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolTypeFunctionSig.h?rev=229129&r1=229128&r2=229129&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolTypeFunctionSig.h?rev=229129&r1=229128&r2=229129&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolTypeFunctionSig.h (original)<br>
+++ llvm/trunk/include/llvm/DebugInfo/PDB/PDBSymbolTypeFunctionSig.h Fri Feb 13 11:57:09 2015<br>
@@ -24,7 +24,12 @@ public:<br>
<br>
DECLARE_PDB_SYMBOL_CONCRETE_TYPE(PDB_SymType::FunctionSig)<br>
<br>
+ std::unique_ptr<PDBSymbol> getReturnType() const;<br>
+ std::unique_ptr<IPDBEnumSymbols> getArguments() const;<br>
+ std::unique_ptr<PDBSymbol> getClassParent() const;<br>
+<br>
void dump(raw_ostream &OS, int Indent, PDB_DumpLevel Level) const override;<br>
+ void dumpArgList(raw_ostream &OS) const;<br>
<br>
FORWARD_SYMBOL_METHOD(getCallingConvention)<br>
FORWARD_SYMBOL_METHOD(getClassParentId)<br>
<br>
Modified: llvm/trunk/lib/DebugInfo/PDB/PDBSymbolExe.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/PDBSymbolExe.cpp?rev=229129&r1=229128&r2=229129&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/PDBSymbolExe.cpp?rev=229129&r1=229128&r2=229129&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/DebugInfo/PDB/PDBSymbolExe.cpp (original)<br>
+++ llvm/trunk/lib/DebugInfo/PDB/PDBSymbolExe.cpp Fri Feb 13 11:57:09 2015<br>
@@ -47,6 +47,26 @@ void PDBSymbolExe::dump(raw_ostream &OS,<br>
auto ChildrenEnum = getChildStats(Stats);<br>
OS << stream_indent(Indent + 2) << "Children: " << Stats << "\n";<br>
while (auto Child = ChildrenEnum->getNext()) {<br>
+ // Skip uninteresting types. These are useful to print as part of type<br>
+ // hierarchies, but as general children of the global scope, they are<br>
+ // not very interesting.<br>
+ switch (Child->getSymTag()) {<br>
+ case PDB_SymType::ArrayType:<br>
+ case PDB_SymType::BaseClass:<br>
+ case PDB_SymType::BuiltinType:<br>
+ case PDB_SymType::CompilandEnv:<br>
+ case PDB_SymType::CustomType:<br>
+ case PDB_SymType::Dimension:<br>
+ case PDB_SymType::Friend:<br>
+ case PDB_SymType::ManagedType:<br>
+ case PDB_SymType::VTableShape:<br>
+ case PDB_SymType::PointerType:<br>
+ case PDB_SymType::FunctionSig:<br>
+ case PDB_SymType::FunctionArg:<br>
+ continue;<br>
+ default:<br>
+ break;<br>
+ }<br>
Child->dump(OS, Indent + 4, PDB_DumpLevel::Normal);<br>
OS << "\n";<br>
}<br>
<br>
Modified: llvm/trunk/lib/DebugInfo/PDB/PDBSymbolFunc.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/PDBSymbolFunc.cpp?rev=229129&r1=229128&r2=229129&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/PDBSymbolFunc.cpp?rev=229129&r1=229128&r2=229129&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/DebugInfo/PDB/PDBSymbolFunc.cpp (original)<br>
+++ llvm/trunk/lib/DebugInfo/PDB/PDBSymbolFunc.cpp Fri Feb 13 11:57:09 2015<br>
@@ -23,6 +23,10 @@ PDBSymbolFunc::PDBSymbolFunc(const IPDBS<br>
std::unique_ptr<IPDBRawSymbol> Symbol)<br>
: PDBSymbol(PDBSession, std::move(Symbol)) {}<br>
<br>
+std::unique_ptr<PDBSymbolTypeFunctionSig> PDBSymbolFunc::getSignature() const {<br>
+ return Session.getConcreteSymbolById<PDBSymbolTypeFunctionSig>(getTypeId());<br>
+}<br>
+<br>
void PDBSymbolFunc::dump(raw_ostream &OS, int Indent,<br>
PDB_DumpLevel Level) const {<br>
OS << stream_indent(Indent);<br>
@@ -30,7 +34,7 @@ void PDBSymbolFunc::dump(raw_ostream &OS<br>
uint32_t FuncStart = getRelativeVirtualAddress();<br>
uint32_t FuncEnd = FuncStart + getLength();<br>
if (FuncStart == 0 && FuncEnd == 0) {<br>
- OS << "func [???]";<br>
+ OS << "func [???] ";<br>
} else {<br>
OS << "func ";<br>
OS << "[" << format_hex(FuncStart, 8);<br>
@@ -52,21 +56,34 @@ void PDBSymbolFunc::dump(raw_ostream &OS<br>
<br>
OS << " ";<br>
uint32_t FuncSigId = getTypeId();<br>
- if (auto FuncSig = Session.getConcreteSymbolById<PDBSymbolTypeFunctionSig>(<br>
- FuncSigId)) {<br>
- OS << "(" << FuncSig->getCallingConvention() << ") ";<br>
- }<br>
+ if (auto FuncSig = getSignature()) {<br>
+ // If we have a signature, dump the name with the signature.<br>
+ if (auto ReturnType = FuncSig->getReturnType()) {<br>
+ ReturnType->dump(OS, 0, PDB_DumpLevel::Compact);<br>
+ OS << " ";<br>
+ }<br>
+<br>
+ OS << FuncSig->getCallingConvention() << " ";<br>
<br>
- uint32_t ClassId = getClassParentId();<br>
- if (ClassId != 0) {<br>
- if (auto Class = Session.getSymbolById(ClassId)) {<br>
- if (auto UDT = dyn_cast<PDBSymbolTypeUDT>(Class.get()))<br>
- OS << UDT->getName() << "::";<br>
- else<br>
- OS << "{class " << Class->getSymTag() << "}::";<br>
+ if (auto ClassParent = FuncSig->getClassParent()) {<br>
+ ClassParent->dump(OS, 0, PDB_DumpLevel::Compact);<br>
+ OS << "::";<br>
}<br>
+<br>
+ OS << getName();<br>
+ FuncSig->dumpArgList(OS);<br>
+ } else {<br>
+ uint32_t ClassId = getClassParentId();<br>
+ if (ClassId != 0) {<br>
+ if (auto Class = Session.getSymbolById(ClassId)) {<br>
+ if (auto UDT = dyn_cast<PDBSymbolTypeUDT>(Class.get()))<br>
+ OS << UDT->getName() << "::";<br>
+ else<br>
+ OS << "{class " << Class->getSymTag() << "}::";<br>
+ }<br>
+ }<br>
+ OS << getName();<br>
}<br>
- OS << getName();<br>
} else {<br>
OS << getName();<br>
}<br>
<br>
Modified: llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypeFunctionSig.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypeFunctionSig.cpp?rev=229129&r1=229128&r2=229129&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypeFunctionSig.cpp?rev=229129&r1=229128&r2=229129&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypeFunctionSig.cpp (original)<br>
+++ llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypeFunctionSig.cpp Fri Feb 13 11:57:09 2015<br>
@@ -17,35 +17,72 @@<br>
<br>
using namespace llvm;<br>
<br>
+namespace {<br>
+class FunctionArgEnumerator : public IPDBEnumSymbols {<br>
+public:<br>
+ typedef ConcreteSymbolEnumerator<PDBSymbolTypeFunctionArg> ArgEnumeratorType;<br>
+<br>
+ FunctionArgEnumerator(const IPDBSession &PDBSession,<br>
+ const PDBSymbolTypeFunctionSig &Sig)<br>
+ : Session(PDBSession),<br>
+ Enumerator(Sig.findAllChildren<PDBSymbolTypeFunctionArg>()) {}<br>
+<br>
+ FunctionArgEnumerator(const IPDBSession &PDBSession,<br>
+ std::unique_ptr<ArgEnumeratorType> ArgEnumerator)<br>
+ : Session(PDBSession), Enumerator(std::move(ArgEnumerator)) {}<br>
+<br>
+ uint32_t getChildCount() const { return Enumerator->getChildCount(); }<br>
+<br>
+ std::unique_ptr<PDBSymbol> getChildAtIndex(uint32_t Index) const {<br>
+ auto FunctionArgSymbol = Enumerator->getChildAtIndex(Index);<br>
+ if (!FunctionArgSymbol)<br>
+ return nullptr;<br>
+ return Session.getSymbolById(FunctionArgSymbol->getTypeId());<br>
+ }<br>
+<br>
+ std::unique_ptr<PDBSymbol> getNext() {<br>
+ auto FunctionArgSymbol = Enumerator->getNext();<br>
+ if (!FunctionArgSymbol)<br>
+ return nullptr;<br>
+ return Session.getSymbolById(FunctionArgSymbol->getTypeId());<br>
+ }<br>
+<br>
+ void reset() { Enumerator->reset(); }<br>
+<br>
+ MyType *clone() const {<br>
+ std::unique_ptr<ArgEnumeratorType> Clone(Enumerator->clone());<br>
+ return new FunctionArgEnumerator(Session, std::move(Clone));<br>
+ }<br>
+<br>
+private:<br>
+ const IPDBSession &Session;<br>
+ std::unique_ptr<ArgEnumeratorType> Enumerator;<br>
+};<br>
+}<br>
+<br>
PDBSymbolTypeFunctionSig::PDBSymbolTypeFunctionSig(<br>
const IPDBSession &PDBSession, std::unique_ptr<IPDBRawSymbol> Symbol)<br>
: PDBSymbol(PDBSession, std::move(Symbol)) {}<br>
<br>
-void PDBSymbolTypeFunctionSig::dump(raw_ostream &OS, int Indent,<br>
- PDB_DumpLevel Level) const {<br>
- OS << stream_indent(Indent);<br>
+std::unique_ptr<PDBSymbol> PDBSymbolTypeFunctionSig::getReturnType() const {<br>
+ return Session.getSymbolById(getTypeId());<br>
+}<br>
<br>
- uint32_t ReturnTypeId = getTypeId();<br>
- if (auto ReturnType = Session.getSymbolById(ReturnTypeId)) {<br>
- ReturnType->dump(OS, 0, PDB_DumpLevel::Compact);<br>
- OS << " ";<br>
- }<br>
- // TODO: We need a way to detect if this is a pointer to function so that we<br>
- // can print the * between the return type and the argument list. The only<br>
- // way to do this is to pass the parent into this function, but that will<br>
- // require a larger interface change.<br>
- OS << getCallingConvention() << " ";<br>
+std::unique_ptr<IPDBEnumSymbols><br>
+PDBSymbolTypeFunctionSig::getArguments() const {<br>
+ return llvm::make_unique<FunctionArgEnumerator>(Session, *this);<br>
+}<br>
+<br>
+std::unique_ptr<PDBSymbol> PDBSymbolTypeFunctionSig::getClassParent() const {<br>
uint32_t ClassId = getClassParentId();<br>
- if (ClassId != 0) {<br>
- if (auto ClassParent = Session.getSymbolById(ClassId)) {<br>
- OS << "(";<br>
- ClassParent->dump(OS, 0, PDB_DumpLevel::Compact);<br>
- OS << "::*)";<br>
- }<br>
- }<br>
- OS.flush();<br>
+ if (ClassId == 0)<br>
+ return nullptr;<br>
+ return Session.getSymbolById(ClassId);<br>
+}<br>
+<br>
+void PDBSymbolTypeFunctionSig::dumpArgList(raw_ostream &OS) const {<br>
OS << "(";<br>
- if (auto ChildEnum = findAllChildren<PDBSymbolTypeFunctionArg>()) {<br>
+ if (auto ChildEnum = getArguments()) {<br>
uint32_t Index = 0;<br>
while (auto Arg = ChildEnum->getNext()) {<br>
Arg->dump(OS, 0, PDB_DumpLevel::Compact);<br>
@@ -54,4 +91,27 @@ void PDBSymbolTypeFunctionSig::dump(raw_<br>
}<br>
}<br>
OS << ")";<br>
+ if (isConstType())<br>
+ OS << " const";<br>
+ if (isVolatileType())<br>
+ OS << " volatile";<br>
+}<br>
+<br>
+void PDBSymbolTypeFunctionSig::dump(raw_ostream &OS, int Indent,<br>
+ PDB_DumpLevel Level) const {<br>
+ OS << stream_indent(Indent);<br>
+<br>
+ if (auto ReturnType = getReturnType()) {<br>
+ ReturnType->dump(OS, 0, PDB_DumpLevel::Compact);<br>
+ OS << " ";<br>
+ }<br>
+<br>
+ OS << getCallingConvention() << " ";<br>
+ if (auto ClassParent = getClassParent()) {<br>
+ OS << "(";<br>
+ ClassParent->dump(OS, 0, PDB_DumpLevel::Compact);<br>
+ OS << "::*)";<br>
+ }<br>
+<br>
+ dumpArgList(OS);<br>
}<br>
<br>
Modified: llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypePointer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypePointer.cpp?rev=229129&r1=229128&r2=229129&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypePointer.cpp?rev=229129&r1=229128&r2=229129&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypePointer.cpp (original)<br>
+++ llvm/trunk/lib/DebugInfo/PDB/PDBSymbolTypePointer.cpp Fri Feb 13 11:57:09 2015<br>
@@ -10,6 +10,7 @@<br>
<br>
#include "llvm/DebugInfo/PDB/IPDBSession.h"<br>
#include "llvm/DebugInfo/PDB/PDBSymbol.h"<br>
+#include "llvm/DebugInfo/PDB/PDBSymbolTypeFunctionSig.h"<br>
#include "llvm/DebugInfo/PDB/PDBSymbolTypePointer.h"<br>
#include <utility><br>
<br>
@@ -27,7 +28,18 @@ void PDBSymbolTypePointer::dump(raw_ostr<br>
if (isVolatileType())<br>
OS << "volatile ";<br>
uint32_t PointeeId = getTypeId();<br>
- if (auto PointeeType = Session.getSymbolById(PointeeId))<br>
- PointeeType->dump(OS, 0, PDB_DumpLevel::Compact);<br>
- OS << ((isReference()) ? "&" : "*");<br>
+ if (auto PointeeType = Session.getSymbolById(PointeeId)) {<br>
+ // Function pointers get special treatment, since we need to print the * in<br>
+ // the middle of the signature.<br>
+ if (auto FuncSig = dyn_cast<PDBSymbolTypeFunctionSig>(PointeeType.get())) {<br>
+ if (auto ReturnType = FuncSig->getReturnType())<br>
+ ReturnType->dump(OS, 0, PDB_DumpLevel::Compact);<br>
+ OS << " (" << FuncSig->getCallingConvention() << " ";<br>
+ OS << ((isReference()) ? "&" : "*") << ")";<br>
+ FuncSig->dumpArgList(OS);<br>
+ } else {<br>
+ PointeeType->dump(OS, 0, PDB_DumpLevel::Compact);<br>
+ OS << ((isReference()) ? "&" : "*");<br>
+ }<br>
+ }<br>
}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div></blockquote></div>
</div></div></blockquote></div><br></div></div>