This is an archive of the discontinued LLVM Phabricator instance.

Add llvm-pdbdump to tools
ClosedPublic

Authored by zturner on Jan 23 2015, 1:13 PM.

Details

Summary

I'm adding a few random windows people so this doesn't slip under the radar of anyone who may be interested. Feel free to ignore, or add others as appropriate. Also +rsmith specifically for COMExtras.h. The purpose of this file is to enable the use of ranged-based for loops for iterating for iterating COM objects (see https://msdn.microsoft.com/en-us/library/k58bkfe9.aspx for an example of what this looks like hand-written)


llvm-pdbdump is a tool which can be used to dump the contents of Microsoft-generated PDB files. It makes use of the Microsoft DIA SDK, which is a COM based library designed specifically for this purpose.

The initial commit of this tool dumps the raw bytes from PDB data streams. Future commits will dump more semantic information such as types, symbols, source files, etc similar to the types of information accessible via llvm-dwarfdump.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 18691.Jan 23 2015, 1:13 PM
zturner retitled this revision from to Add llvm-pdbdump to tools.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: rnk, majnemer, aaron.ballman, rsmith.
zturner added a subscriber: Unknown Object (MLST).

Mostly nit picking style issues. I don't know enough about COM or anything else here to do more. ;]

tools/CMakeLists.txt
64 ↗(On Diff #18691)

Shouldn't this be checking if the host OS is Windows rather than the compiler?

tools/llvm-pdbdump/COMExtras.h
18–19 ↗(On Diff #18691)

MSVC 2012 doesn't support variadiac templates I thought?

36–40 ↗(On Diff #18691)

Should the example code her be formatted consistently?

50–51 ↗(On Diff #18691)

Use doxygen comment style instead?

63 ↗(On Diff #18691)

I very much prefer that we capitalize initialisms in names properly if we're using capital letters at all. So "COMIterator" here. However, for iterator and STL-ish types we often use STL-ish naming conventions and I'd be fine with com_iterator.

tools/llvm-pdbdump/llvm-pdbdump.cpp
44–47 ↗(On Diff #18691)

If these aren't provided by Support, they should be. If they are, just include the header rather than declaring them yourself.

57 ↗(On Diff #18691)

I wouldn't bother with the comment, it seems obvious from the code.

127 ↗(On Diff #18691)

Why the _s?

zturner added inline comments.Jan 23 2015, 1:48 PM
tools/CMakeLists.txt
64 ↗(On Diff #18691)

COM uses a bunch of Microsoft extensions and stuff. I'm pretty sure this won't compile with MinGW, for example. Is there a better check for "any MSVC compatible compiler, including clang"?

tools/llvm-pdbdump/COMExtras.h
18–19 ↗(On Diff #18691)

Umm.. Crap? Can we drop support for VS 2012 yet? lol. I'm not really sure how to implement this without it.

tools/llvm-pdbdump/llvm-pdbdump.cpp
44–47 ↗(On Diff #18691)

They're part of that stuff that I used to frequently complain about which is in Support, but not exposed through a header file since it's Windows-specific.

127 ↗(On Diff #18691)

Because I use llvm::Sys::Process::GetArgumentVector() so there ends up being a second argv. I used _ to make it clear that they're not going to be used, and that the argv vector should be used instead.

aaron.ballman edited edge metadata.Jan 23 2015, 2:39 PM

Additional thoughts and comments to what Chandler posted.

Index: tools/CMakeLists.txt

  • tools/CMakeLists.txt

+++ tools/CMakeLists.txt
@@ -61,6 +61,10 @@

add_llvm_tool_subdirectory(llvm-go)

+if(MSVC)
+ add_llvm_tool_subdirectory(llvm-pdbdump)
+endif()
+
if(NOT CYGWIN AND LLVM_ENABLE_PIC)

add_llvm_tool_subdirectory(lto)
add_llvm_tool_subdirectory(llvm-lto)

Index: tools/LLVMBuild.txt

  • tools/LLVMBuild.txt

+++ tools/LLVMBuild.txt
@@ -16,7 +16,7 @@
;===------------------------------------------------------------------------===;

[common]
-subdirectories = bugpoint llc lli llvm-ar llvm-as llvm-bcanalyzer llvm-cov llvm-diff llvm-dis llvm-dwarfdump llvm-extract llvm-jitlistener llvm-link llvm-lto llvm-mc llvm-nm llvm-objdump llvm-profdata llvm-rtdyld llvm-size macho-dump opt llvm-mcmarkup verify-uselistorder dsymutil
+subdirectories = bugpoint llc lli llvm-ar llvm-as llvm-bcanalyzer llvm-cov llvm-diff llvm-dis llvm-dwarfdump llvm-extract llvm-jitlistener llvm-link llvm-lto llvm-mc llvm-nm llvm-objdump llvm-pdbdump llvm-profdata llvm-rtdyld llvm-size macho-dump opt llvm-mcmarkup verify-uselistorder dsymutil

[component_0]
type = Group

Index: tools/llvm-pdbdump/CMakeLists.txt

  • /dev/null

+++ tools/llvm-pdbdump/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+ Support
+ )
+
+set(MSVC_DIA_SDK_DIR "$ENV{VSINSTALLDIR}DIA SDK")
+include_directories(${MSVC_DIA_SDK_DIR}/include)
+if (CMAKE_SIZEOF_VOID_P EQUAL 8)
+ link_directories(${MSVC_DIA_SDK_DIR}/lib/amd64)
+else()
+ link_directories(${MSVC_DIA_SDK_DIR}/lib)
+endif()
+
+add_llvm_tool(llvm-pdbdump
+ llvm-pdbdump.cpp
+ )
+target_link_libraries(llvm-pdbdump diaguids)
\ No newline at end of file

Index: tools/llvm-pdbdump/COMExtras.h

  • /dev/null

+++ tools/llvm-pdbdump/COMExtras.h
@@ -0,0 +1,264 @@
+===- COMExtras.h - Helper files for COM operations -------------*- C++-*-===
+
+
The LLVM Compiler Infrastructure
+
+
This file is distributed under the University of Illinois Open Source
+ License. See LICENSE.TXT for details.
+

+===----------------------------------------------------------------------===
+
+#ifndef LLVM_TOOLS_LLVM_PDBDUMP_COMEXTRAS_H
+#define LLVM_TOOLS_LLVM_PDBDUMP_COMEXTRAS_H
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/SmallVector.h"
+
+template <class F> struct function_traits;

This nit applies to the entire patch: prefer typename over class in
template delcarations. Also, this should all be namespaced.

+
+template <class R, class... Args>
+struct function_traits<R (*)(Args...)> : public function_traits<R(Args...)> {};

Variadic templates will be a problem until we drop MSVC 2012 support.

+
+template <class R, class... Args> struct function_traits<R(Args...)> {
+ using return_type = R;
+
+ using args_tuple = std::tuple<Args...>;

You should probably be including <tuple> instead of relying on it
being pulled in by other ADT headers.

+};
+
+template <class C, class R, class... Args>
+struct function_traits<R (__stdcall C::*)(Args...)>
+ : public function_traits<R(Args...)> {};
+
+template <class FuncTraits, std::size_t arg> struct function_arg {
+ Writing function_arg as a separate class that accesses the tuple from
+
function_traits is necessary due to what appears to be a bug in MSVC.
+ If you write a nested class inside function_traits like this:
+
template<std::size_t ArgIndex>
+ struct Argument
+
{
+ typedef typename
+
std::tuple_element<ArgIndex, std::tuple<Args...>>::type type;
+ };
+
MSVC encounters a parsing error.

What parsing error does MSVC hit?

+ typedef
+ typename std::tuple_element<arg, typename FuncTraits::args_tuple>::type
+ type;
+};
+
+template <class T> struct remove_double_pointer {};
+template <class T> struct remove_double_pointer<T **> { typedef T type; };
+
+=============================================================================
+
class ComIterator<>
+
+
A common idiom in the COM world is to have an enumerator interface, say
+ IMyEnumerator. It's responsible for enumerating over some child data type,
+
say IChildType. You do the enumeration by calling IMyEnumerator::Next()
+ to get a pointer to a pointer to the child type. Eventually it fails,
+
and that means you're at the end.
+
+
ComIterator represents a single point-in-time of this iteration. It is
+ used by ComEnumerator to support iterating in this fashion via range-based
+
for loops and other common C++ paradigms.
+//=============================================================================
+template <class EnumeratorType, std::size_t ArgIndex> class ComIterator {
+private:

No need for the private access specifier.

+ using FunctionTraits = function_traits<decltype(&EnumeratorType::Next)>;
+ typedef typename function_arg<FunctionTraits, ArgIndex>::type FuncArgType;
+ FuncArgType is now something like ISomeCOMInterface **. Remove both
+
pointers, so we can make a CComPtr<T> out of it.
+ typedef typename remove_double_pointer<FuncArgType>::type EnumDataType;
+
+public:
+ explicit ComIterator(CComPtr<EnumeratorType> Enumerator,
+ CComPtr<EnumDataType> Current)
+ : EnumeratorObject(Enumerator), CurrentItem(Current) {}
+ ComIterator() {}
+
+ ComIterator &operator++() {
+ // EnumeratorObject->Next() expects CurrentItem to be NULL.
+ CurrentItem.Release();
+ ULONG Count = 0;
+ HRESULT hr = EnumeratorObject->Next(1, &CurrentItem, &Count);
+ if (FAILED(hr) || Count == 0)
+ *this = ComIterator();
+
+ return *this;
+ }
+
+ CComPtr<EnumDataType> operator*() { return CurrentItem; }
+
+ bool operator==(const ComIterator &other) const {
+ return (EnumeratorObject == other.EnumeratorObject) &&
+ (CurrentItem == other.CurrentItem);
+ }
+
+ bool operator!=(const ComIterator &other) const { return !(*this == other); }
+
+ ComIterator &operator=(const ComIterator &other) {
+ EnumeratorObject = other.EnumeratorObject;
+ CurrentItem = other.CurrentItem;
+ return *this;
+ }

Since this is an iterator, it should expose other iterator
functionality (like operator++(int), and the iterator typedefs
value_type, difference_type, pointer, reference, and
iterator_category).

+
+private:
+ CComPtr<EnumeratorType> EnumeratorObject;
+ CComPtr<EnumDataType> CurrentItem;

Might as well stick these up with the other private items.

+};
+
+=============================================================================
+
class ComEnumerator<>
+
+
ComEnumerator is the top-level "range class" used to support iteration via
+ range-based for loops. It simply provides a begin() and end() method which
+
return appropriately constructed ComIterator<> classes.
+//=============================================================================
+template <class EnumeratorType, std::size_t ArgIndex> class ComEnumerator {
+private:

Private access specifier not required.

+ typedef function_traits<decltype(&EnumeratorType::Next)> FunctionTraits;
+ typedef typename function_arg<FunctionTraits, ArgIndex>::type FuncArgType;
+ typedef typename remove_double_pointer<FuncArgType>::type EnumDataType;
+
+public:
+ ComEnumerator(CComPtr<EnumeratorType> Enumerator)
+ : EnumeratorObject(Enumerator) {}
+
+ ComIterator<EnumeratorType, ArgIndex> begin() {
+ if (!EnumeratorObject)
+ return end();
+
+ EnumeratorObject->Reset();
+ ULONG Count = 0;
+ CComPtr<EnumDataType> FirstItem;
+ HRESULT hr = EnumeratorObject->Next(1, &FirstItem, &Count);
+ return (FAILED(hr) || Count == 0)
+ ? end()
+ : ComIterator<EnumeratorType, ArgIndex>(EnumeratorObject, FirstItem);
+ }
+
+ ComIterator<EnumeratorType, ArgIndex> end() {
+ return ComIterator<EnumeratorType, ArgIndex>();
+ }
+
+private:
+ CComPtr<EnumeratorType> EnumeratorObject;

Might as well move this up as well.

+};
+
+=============================================================================
+
class ComDataRecordIterator<>
+
+
Similar to ComIterator<>, but uses a

I'm dying to know what this uses. ;-)

+=============================================================================
+template <class EnumeratorType> class ComDataRecordIterator {
+public:
+ explicit ComDataRecordIterator(CComPtr<EnumeratorType> enumerator,
+ uint32_t currentRecord)
+ : Enumerator(enumerator), CurrentRecord(currentRecord) {}
+ ComDataRecordIterator() {}
+
+ ComDataRecordIterator &operator++() {
+
Release the current item so that Enumerator->Next() is happy.
+ ++CurrentRecord;
+ ReadNextRecord();

This weirds me out a bit. Since ReadNextRecord can fail, but you are
incrementing CurrentRecord regardless, doesn't this mean you can get
to end() but not compare equal to something returned by end()?

+ return *this;
+ }
+
+ llvm::ArrayRef<uint8_t> operator*() {
+ if (CurrentRecord == 0)
+ ReadNextRecord();
+
+ return llvm::ArrayRef<uint8_t>(RecordData.begin(), RecordData.end());
+ }
+
+ bool operator==(const ComDataRecordIterator &other) const {
+ return (Enumerator == other.Enumerator) &&
+ (CurrentRecord == other.CurrentRecord);
+ }
+
+ bool operator!=(const ComDataRecordIterator &other) const {
+ return !(*this == other);
+ }

Same comments here regarding the iterator interface as above.

+
+private:
+ void ReadNextRecord() {
+ RecordData.clear();
+ ULONG Count = 0;
+ DWORD RequiredBufferSize;
+ HRESULT hr = Enumerator->Next(1, 0, &RequiredBufferSize, nullptr, &Count);
+ if (hr == S_OK) {

Please use the SUCCEEDED macro.

+ RecordData.resize(RequiredBufferSize);
+ DWORD BytesRead = 0;
+ hr = Enumerator->Next(1, RequiredBufferSize, &BytesRead,
+ RecordData.data(), &Count);
+ }
+ if (hr == S_FALSE) {

Please use the FAILED macro. This code will fail to end the
enumeration if Next() fails for any reason other than S_FALSE.

+ // This is the end of the enumeration.
+ RecordData.clear();
+ }
+ }
+
+ CComPtr<EnumeratorType> Enumerator;
+ uint32_t CurrentRecord;
+ llvm::SmallVector<uint8_t, 128> RecordData;

Why 128? It seems rather large, and like something the caller might
have a much better idea over.

+};
+
+=============================================================================
+
class ComEnumerator<>
+
+
ComEnumerator is the top-level "range class" used to support iteration via
+ range-based for loops. It simply provides a begin() and end() method which
+
return appropriately constructed ComIterator<> classes.

This comment is incorrect copypasta.

+=============================================================================
+template <class EnumeratorType> class ComDataRecordEnumerator {
+public:
+ ComDataRecordEnumerator(CComPtr<EnumeratorType> enumerator)
+ : Enumerator(enumerator) {}
+
+ ComDataRecordIterator<EnumeratorType> begin() {
+ if (Enumerator)
+ Enumerator->Reset();
+ return ComDataRecordIterator<EnumeratorType>(Enumerator, 0);
+ }
+
+ ComDataRecordIterator<EnumeratorType> end() {
+ LONG NumElts = 0;
+ HRESULT hr = Enumerator->get_Count(&NumElts);
+ return (FAILED(hr))
+ ? ComDataRecordIterator<EnumeratorType>(Enumerator, 0)
+ : ComDataRecordIterator<EnumeratorType>(Enumerator, NumElts);
+ }
+
+private:
+ CComPtr<EnumeratorType> Enumerator;
+};
+
+
=============================================================================
+ function com_enumerator<>
+

+ com_enumerator puts together all the magic C++ incantations to deduce all
+
necessary types (enumerator, child type) automatically. You need only write
+ for (auto item : com_enumerator(MyEnumerator))
+
{
+ }
+
=============================================================================
+template <class EnumeratorType>
+ComEnumerator<EnumeratorType, 1>
+com_enumerator(CComPtr<EnumeratorType> Enumerator) {
+ return ComEnumerator<EnumeratorType, 1>(Enumerator);
+}

make_com_enumerator to be more STL-like?

+
+=============================================================================
+
function com_data_record_enumerator<>
+
+
com_data_record_enumerator returns a ComDataRecordEnumerator appropriately
+ parameterized so that it can be used in a range-based for loop. You need
+
only write
+ for (auto record : com_data_record_enumerator(MyEnumerator))
+
{
+ }
+
=============================================================================
+template <class EnumeratorType>
+ComDataRecordEnumerator<EnumeratorType>
+com_data_record_enumerator(CComPtr<EnumeratorType> Enumerator) {
+ return ComDataRecordEnumerator<EnumeratorType>(Enumerator);
+}

make_com_data_record_enumerator is a mouthful, but yay consistency in
suggestions?

+#endif
Index: tools/llvm-pdbdump/LLVMBuild.txt

The rest of the patch seems like it's a separate commit from COM
functionality, but I'm not strongly tied to that idea.

  • /dev/null

+++ tools/llvm-pdbdump/LLVMBuild.txt
@@ -0,0 +1,23 @@
+;===- ./tools/llvm-pdbdump/LLVMBuild.txt -----------------------*- Conf -*--===;
+;
+; The LLVM Compiler Infrastructure
+;
+; This file is distributed under the University of Illinois Open Source
+; License. See LICENSE.TXT for details.
+;
+;===------------------------------------------------------------------------===;
+;
+; This is an LLVMBuild description file for the components in this subdirectory.
+;
+; For more information on the LLVMBuild system, please see:
+;
+; http://llvm.org/docs/LLVMBuild.html
+;
+;===------------------------------------------------------------------------===;
+
+[component_0]
+type = Tool
+name = llvm-pdbdump
+parent = Tools
+required_libraries =
+

Index: tools/llvm-pdbdump/llvm-pdbdump.cpp

  • /dev/null

+++ tools/llvm-pdbdump/llvm-pdbdump.cpp
@@ -0,0 +1,152 @@
+===- llvm-pdbdump.cpp - Dump debug info from a PDB file -------*- C++ -*-===
+
+
The LLVM Compiler Infrastructure
+
+
This file is distributed under the University of Illinois Open Source
+ License. See LICENSE.TXT for details.
+

+===----------------------------------------------------------------------===
+
+
Dumps debug information present in PDB files. This utility makes use of
+ the Microsoft Windows SDK, so will not compile or run on non-Windows
+
platforms.
+
+
===----------------------------------------------------------------------===//
+
+#define NTDDI_VERSION NTDDI_VISTA
+#define _WIN32_WINNT _WIN32_WINNT_VISTA
+#define WINVER _WIN32_WINNT_VISTA
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+
+#include <atlbase.h>
+#include <windows.h>
+#include <dia2.h>
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/Format.h"
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/Process.h"
+#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Signals.h"
+
+#include "COMExtras.h"
+
+using namespace llvm;
+
+namespace llvm {
+namespace sys {
+namespace windows {
+extern std::error_code UTF8ToUTF16(StringRef utf8,
+ SmallVectorImpl<wchar_t> &utf16);
+extern std::error_code UTF16ToUTF8(const wchar_t *utf16, size_t utf16_len,
+ SmallVectorImpl<char> &utf8);

Ugh, this really needs to find a reasonable home. I'm starting to
suspect that now is the time.

+}
+}
+}
+
+namespace opts {
+cl::list<std::string> InputFilenames(cl::Positional,
+ cl::desc("<input PDB files>"),
+ cl::OneOrMore);
+
+ -streams, -s
+cl::opt<bool> Streams("streams", cl::desc("Display data stream information"));
+cl::alias StreamsShort("s", cl::desc("Alias for --streams"),
+ cl::aliasopt(Streams));
+
+
stream-data, -S
+cl::opt<bool> StreamData("stream-data",
+ cl::desc("Dumps stream record data as bytes"));
+cl::alias StreamDataShort("S", cl::desc("Alias for --stream-data"),
+ cl::aliasopt(StreamData));
+}
+
+static void dumpDataStreams(IDiaSession *session) {
+ CComPtr<IDiaEnumDebugStreams> DebugStreams = nullptr;
+ if (session->getEnumDebugStreams(&DebugStreams) == S_OK) {

Should use the SUCCEEDED macro.

+ LONG Count = 0;
+ if (FAILED(DebugStreams->get_Count(&Count)))
+ return;
+ outs() << "Data Streams [count=" << Count << "]\n";
+
+ llvm::SmallString<32> Name8;
+
+ for (auto Stream : com_enumerator(DebugStreams)) {
+ BSTR Name16;
+ if (Stream->get_name(&Name16) != S_OK)

Should use the FAILED macro.

+ continue;
+ if (!llvm::sys::windows::UTF16ToUTF8(Name16, SysStringLen(Name16), Name8))
+ outs() << " " << Name8;
+ ::SysFreeString(Name16);
+ if (SUCCEEDED(Stream->get_Count(&Count))) {
+ outs() << " [" << Count << " records]\n";
+ if (opts::StreamData) {
+ int RecordIndex = 0;
+ for (auto StreamRecord : com_data_record_enumerator(Stream)) {
+ outs() << " Record " << RecordIndex << " ["
+ << StreamRecord.size() << " bytes]";
+ for (size_t i = 0; i < StreamRecord.size(); ++i) {

Why not a range-based for loop here as well?

+ outs() << " "
+ << llvm::format_hex(StreamRecord[i], 2, true, false);
+ }
+ outs() << "\n";
+ ++RecordIndex;
+ }
+ }
+ } else
+ outs() << "\n";
+ }
+ }
+ outs().flush();
+}
+
+static void dumpInput(StringRef Path) {
+ SmallVector<wchar_t, 128> path_utf16;
+ std::error_code EC = llvm::sys::windows::UTF8ToUTF16(Path, path_utf16);
+ CComPtr<IDiaDataSource> source;
+ HRESULT hr =
+ ::CoCreateInstance(CLSID_DiaSource, nullptr, CLSCTX_INPROC_SERVER,
+ __uuidof(IDiaDataSource), (void **)&source);
+ if (FAILED(hr))
+ return;
+ if (FAILED(source->loadDataFromPdb(path_utf16.data())))
+ return;
+ CComPtr<IDiaSession> session;
+ if (FAILED(source->openSession(&session)))
+ return;
+ if (opts::Streams || opts::StreamData) {
+ dumpDataStreams(session);
+ }
+}
+
+int main(int argc_, const char *argv_[]) {
+ Print a stack trace if we signal out.
+ sys::PrintStackTraceOnErrorSignal();
+ PrettyStackTraceProgram X(argc_, argv_);
+
+ SmallVector<const char *, 256> argv;
+ llvm::SpecificBumpPtrAllocator<char> ArgAllocator;
+ std::error_code EC = llvm::sys::Process::GetArgumentVector(
+ argv, llvm::makeArrayRef(argv_, argc_), ArgAllocator);
+ if (EC) {
+ llvm::errs() << "error: couldn't get arguments: " << EC.message() << '\n';
+ return 1;
+ }
+
+ llvm_shutdown_obj Y;
Call llvm_shutdown() on exit.
+
+ cl::ParseCommandLineOptions(argv.size(), argv.data(), "LLVM PDB Dumper\n");
+
+ CoInitializeEx(nullptr, COINIT_MULTITHREADED);
+
+ std::for_each(opts::InputFilenames.begin(), opts::InputFilenames.end(),
+ dumpInput);
+
+ CoUninitialize();
+ return 0;
+}

~Aaron

Thanks for the detailed review! Comments inline, and a new patch incoming
soon.

zturner updated this revision to Diff 18695.Jan 23 2015, 3:12 PM
zturner edited edge metadata.

Updated based on Chandler and Aaron's suggestions.

By the way, I'm kind of mixed about the idea of supporting a full iterator interface (postfix ++, copying, etc). The reason is that it requires cloning the COM interface. It's possible, and the interface provides a method to do it, but it's probably a very slow operation, and generally not needed. Also not needed for anything in the pdb dumper (at least in the foreseeable future).

If you think it's better to do it anyway I can, but the main reason it's not there is just because Clone() is expensive and wasn't needed, so I decided not to add it.

zturner updated this revision to Diff 18698.Jan 23 2015, 4:26 PM

iterator support related changes suggested by Aaron.

(I don't usually do this kind of thing, so let me know if mucked up these types. But it seems consistent with what I'm returning for operator*, for example)

Going to submit this a bit later today if there are no further comments.

rnk accepted this revision.Jan 26 2015, 11:03 AM
rnk edited edge metadata.

lgtm, if we remove use of lib/Support/Windows functions

tools/llvm-pdbdump/llvm-pdbdump.cpp
45–48 ↗(On Diff #18698)

I'd rather not manually prototype private implementation details of lib/Support/Windows. We have an equivalent of one of these called convertUTF16ToUTF8String that's public, and we should just implement the equivalent operation in the other direction. Let me go write that real quick...

72 ↗(On Diff #18698)

You can exit early here. I doubt you need to flush when you don't print anything.

87 ↗(On Diff #18698)

Generally LLVM encourages early exit to reduce indentation, so you could invert this to:

if (!SUCCEEDED(Stream->get_Count(&Count)) {
  outs() << '\n';
  continue;
}
// success case with reduced indentation
This revision is now accepted and ready to land.Jan 26 2015, 11:03 AM
zturner updated this revision to Diff 18787.Jan 26 2015, 2:19 PM
zturner edited edge metadata.

This should address Reid's comments, and the rest of Aaron's comments.

Aaron, did you specifically want me to wait for your follow up, or were you just wanting me to wait for any LGTM?

This revision was automatically updated to reflect the committed changes.

This seems to be causing quite a few failures in our Visual Studio builds:

f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(40): error C2873: 'args_tuple' : symbol cannot be used in a using-declaration (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(40): error C2143: syntax error : missing ';' before '=' (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(40): error C2238: unexpected token(s) preceding ';' (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(46): error C2873: 'args_tuple' : symbol cannot be used in a using-declaration (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(46): error C2143: syntax error : missing ';' before '=' (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(46): error C2238: unexpected token(s) preceding ';' (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(85): error C2873: 'FunctionTraits' : symbol cannot be used in a using-declaration (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(85): error C2143: syntax error : missing ';' before '=' (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(85): error C2238: unexpected token(s) preceding ';' (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(86): error C2065: 'FunctionTraits' : undeclared identifier (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(86): error C2923: 'llvm::function_arg' : 'FunctionTraits' is not a valid template type argument for parameter 'FuncTraits' (F:\work\src\llvm\tools\llvm-pdbdump\DIAExtras.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(40): error C2873: 'args_tuple' : symbol cannot be used in a using-declaration (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(40): error C2143: syntax error : missing ';' before '=' (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(40): error C2238: unexpected token(s) preceding ';' (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(46): error C2873: 'args_tuple' : symbol cannot be used in a using-declaration (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(46): error C2143: syntax error : missing ';' before '=' (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(46): error C2238: unexpected token(s) preceding ';' (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(85): error C2873: 'FunctionTraits' : symbol cannot be used in a using-declaration (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(85): error C2143: syntax error : missing ';' before '=' (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(85): error C2238: unexpected token(s) preceding ';' (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(86): error C2065: 'FunctionTraits' : undeclared identifier (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\COMExtras.h(86): error C2923: 'llvm::function_arg' : 'FunctionTraits' is not a valid template type argument for parameter 'FuncTraits' (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.h(261): error C4519: default template arguments are only allowed on a class template (F:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp) ...
f:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp(312): error C2783: 'llvm::sys::windows::DIAResult<U> llvm::sys::windows::DIASymbol::InternalGetDIAValue(HRESULT (__cdecl IDiaSymbol::* )(T *))' : could not deduce template argument for 'U' ...
f:\work\src\llvm\tools\llvm-pdbdump\DIASymbol.cpp(316): error C2783: 'llvm::sys::windows::DIAResult<U> llvm::sys::windows::DIASymbol::InternalGetDIAValue(HRESULT (__cdecl IDiaSymbol::* )(T *))' : could not deduce template argument for 'U' ...
//.... lots more

Note that D7132 needs to be patched before you can reach these failures (I've suggested a fix there). I expect it to go away soon, but you can manually patch that one if you plan to reproduce these failures.

Thanks.

  • Asiri

Hmmm, perhaps this commit assumes VS2013? (which is coming soon)?

This should hopefully be rectified with r227456, thank you for pointing it out!

~Aaron

Hi Aaron,

That brought down the errors from about 190 to 160. I should've given you the full log. I've attached it here:

We're pushing on the VS2013 build but we'd need a week or so for that. Really appreciate you taking time to address these failures.

Thanks.

  • Asiri

I happened to update between r227459 and r227479 and tried to build with VS2012.

It turns out that just a couple of more changes fixed the build errors. I haven't tried to use the tool, but at least it builds for me.

Here's what I did, in case anyone using VS2012 is interested in trying the pdbdump tool:

Index: DIASymbol.h

  • DIASymbol.h (revision 227485)

+++ DIASymbol.h (working copy)
@@ -250,7 +250,17 @@

DIAResult<BOOL> isVolatileType();

private:

  • template <class T, class U = T>

+ template <class T>
+ DIAResult<T>
+ InternalGetDIAValue(HRESULT (__stdcall IDiaSymbol::*Method)(T *)) {
+ T Value;
+ if (S_OK == (Symbol->*Method)(&Value))
+ return DIAResult<T>(T(Value));
+ else
+ return DIAResult<T>();
+ }
+
+ template <class T, class U>

DIAResult<U>
InternalGetDIAValue(HRESULT (__stdcall IDiaSymbol::*Method)(T *)) {
  T Value;

Index: COMExtras.h

  • COMExtras.h (revision 227485)

+++ COMExtras.h (working copy)
@@ -82,7 +82,7 @@
/ used by ComEnumerator to support iterating in this fashion via range-based
/ for loops and other common C++ paradigms.
template <class EnumeratorType, std::size_t ArgIndex> class com_iterator {

  • using FunctionTraits = function_traits<decltype(&EnumeratorType::Next)>;

+ typedef function_traits<decltype(&EnumeratorType::Next)> FunctionTraits;

typedef typename function_arg<FunctionTraits, ArgIndex>::type FuncArgType;
// FuncArgType is now something like ISomeCOMInterface **.  Remove both
// pointers, so we can make a CComPtr<T> out of it.

Thanks. Sorry about the trouble. I had tested it locally on VS 2012
before submitting, but I thought to test the no-variadic template codepath,
and it was before I had added the using.