<div dir="ltr">Thanks for the detailed review! Comments inline, and a new patch incoming soon.<br><br><div class="gmail_quote">On Fri Jan 23 2015 at 2:39:49 PM Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Additional thoughts and comments to what Chandler posted.<br>
<br>
> Index: tools/CMakeLists.txt<br>
> ==============================<u></u>==============================<u></u>=======<br>
> --- tools/CMakeLists.txt<br>
> +++ tools/CMakeLists.txt<br>
> @@ -61,6 +61,10 @@<br>
><br>
> add_llvm_tool_subdirectory(<u></u>llvm-go)<br>
><br>
> +if(MSVC)<br>
> + add_llvm_tool_subdirectory(<u></u>llvm-pdbdump)<br>
> +endif()<br>
> +<br>
> if(NOT CYGWIN AND LLVM_ENABLE_PIC)<br>
> add_llvm_tool_subdirectory(<u></u>lto)<br>
> add_llvm_tool_subdirectory(<u></u>llvm-lto)<br>
> Index: tools/LLVMBuild.txt<br>
> ==============================<u></u>==============================<u></u>=======<br>
> --- tools/LLVMBuild.txt<br>
> +++ tools/LLVMBuild.txt<br>
> @@ -16,7 +16,7 @@<br>
> ;===--------------------------<u></u>------------------------------<u></u>----------------===;<br>
><br>
> [common]<br>
> -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<br>
> +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<br>
><br>
> [component_0]<br>
> type = Group<br>
> Index: tools/llvm-pdbdump/CMakeLists.<u></u>txt<br>
> ==============================<u></u>==============================<u></u>=======<br>
> --- /dev/null<br>
> +++ tools/llvm-pdbdump/CMakeLists.<u></u>txt<br>
> @@ -0,0 +1,16 @@<br>
> +set(LLVM_LINK_COMPONENTS<br>
> + Support<br>
> + )<br>
> +<br>
> +set(MSVC_DIA_SDK_DIR "$ENV{VSINSTALLDIR}DIA SDK")<br>
> +include_directories(${MSVC_<u></u>DIA_SDK_DIR}/include)<br>
> +if (CMAKE_SIZEOF_VOID_P EQUAL 8)<br>
> + link_directories(${MSVC_DIA_<u></u>SDK_DIR}/lib/amd64)<br>
> +else()<br>
> + link_directories(${MSVC_DIA_<u></u>SDK_DIR}/lib)<br>
> +endif()<br>
> +<br>
> +add_llvm_tool(llvm-pdbdump<br>
> + llvm-pdbdump.cpp<br>
> + )<br>
> +target_link_libraries(llvm-<u></u>pdbdump diaguids)<br>
> \ No newline at end of file<br>
> Index: tools/llvm-pdbdump/COMExtras.h<br>
> ==============================<u></u>==============================<u></u>=======<br>
> --- /dev/null<br>
> +++ tools/llvm-pdbdump/COMExtras.h<br>
> @@ -0,0 +1,264 @@<br>
> +//===- COMExtras.h - Helper files for COM operations -------------*- C++-*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===------------------------<u></u>------------------------------<u></u>----------------===//<br>
> +<br>
> +#ifndef LLVM_TOOLS_LLVM_PDBDUMP_<u></u>COMEXTRAS_H<br>
> +#define LLVM_TOOLS_LLVM_PDBDUMP_<u></u>COMEXTRAS_H<br>
> +<br>
> +#include "llvm/ADT/ArrayRef.h"<br>
> +#include "llvm/ADT/SmallVector.h"<br>
> +<br>
> +template <class F> struct function_traits;<br>
<br>
This nit applies to the entire patch: prefer typename over class in<br>
template delcarations. Also, this should all be namespaced.<br>
<br>
> +<br>
> +template <class R, class... Args><br>
> +struct function_traits<R (*)(Args...)> : public function_traits<R(Args...)> {};<br>
<br>
Variadic templates will be a problem until we drop MSVC 2012 support.<br></blockquote><div>Since this is intended only to support my use case of COM, and ultimately doesn't need to be completely generic, I'm going to add a codepath for the case of no variadic template support that only provides a 3 and 5 argument version.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +};<br>
> +<br>
> +template <class C, class R, class... Args><br>
> +struct function_traits<R (__stdcall C::*)(Args...)><br>
> + : public function_traits<R(Args...)> {};<br>
> +<br>
> +template <class FuncTraits, std::size_t arg> struct function_arg {<br>
> + // Writing function_arg as a separate class that accesses the tuple from<br>
> + // function_traits is necessary due to what appears to be a bug in MSVC.<br>
> + // If you write a nested class inside function_traits like this:<br>
> + // template<std::size_t ArgIndex><br>
> + // struct Argument<br>
> + // {<br>
> + // typedef typename<br>
> + // std::tuple_element<ArgIndex, std::tuple<Args...>>::type type;<br>
> + // };<br>
> + // MSVC encounters a parsing error.<br>
<br>
What parsing error does MSVC hit?<br></blockquote><div>This is the smallest repro I could come up with:</div><div><br></div><div><div>#include <tuple></div><div><br></div><div>class FooClass</div><div>{</div><div>public:</div><div> int __stdcall FooFunc(int a, double b, float c)</div><div> {</div><div> return 0;</div><div> }</div><div>};</div><div><br></div><div>template <class F> struct function_traits;</div><div><br></div><div>template <class R, class... Args></div><div>struct function_traits<R (*)(Args...)> : public function_traits<R(Args...)> {};</div><div><br></div><div>template <class R, class... Args> struct function_traits<R(Args...)> {</div><div> using return_type = R;</div><div><br></div><div> using args_tuple = std::tuple<Args...>;</div><div><br></div><div> template<std::size_t N></div><div> struct Argument</div><div> {</div><div> typedef typename std::tuple_element<N, std::tuple<Args...>>::type type;</div><div> };</div><div>};</div><div><br></div><div>template <class C, class R, class... Args></div><div>struct function_traits<R (__stdcall C::*)(Args...)></div><div> : public function_traits < R(Args...) > {};</div><div><br></div><div>template <typename ClassType></div><div>struct TestClass</div><div>{</div><div> typedef typename function_traits<decltype(&ClassType::FooFunc)>::Argument<2>::type mytype;</div><div>};</div><div><br></div><div>int _tmain(int argc, _TCHAR* argv[])</div><div>{</div><div> TestClass<FooClass> test;</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>return 0;</div><div>}</div><div><br></div></div><div><br></div><div><div>1>c:\users\zachary\documents\visual studio 2013\projects\debuginfotest\debuginfotest.cpp(40): error C2059: syntax error : '<'</div><div>1> c:\users\zachary\documents\visual studio 2013\projects\debuginfotest\debuginfotest.cpp(41) : see reference to class template instantiation 'TestClass<ClassType>' being compiled</div><div>1>c:\users\zachary\documents\visual studio 2013\projects\debuginfotest\debuginfotest.cpp(40): error C2238: unexpected token(s) preceding ';'</div></div><div><br></div><div>I don't consider myself a C++ expert, but note the same line works if I pull it out of TestClass and replace ClassType with FooClass.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> +//===========================<u></u>==============================<u></u>====================<br>
> +template <class EnumeratorType> class ComDataRecordIterator {<br>
> +public:<br>
> + explicit ComDataRecordIterator(CComPtr<<u></u>EnumeratorType> enumerator,<br>
> + uint32_t currentRecord)<br>
> + : Enumerator(enumerator), CurrentRecord(currentRecord) {}<br>
> + ComDataRecordIterator() {}<br>
> +<br>
> + ComDataRecordIterator &operator++() {<br>
> + // Release the current item so that Enumerator->Next() is happy.<br>
> + ++CurrentRecord;<br>
> + ReadNextRecord();<br>
<br>
This weirds me out a bit. Since ReadNextRecord can fail, but you are<br>
incrementing CurrentRecord regardless, doesn't this mean you can get<br>
to end() but not compare equal to something returned by end()?<br></blockquote><div> </div><div>Presumably, but isn't incrementing end() undefined behavior anyway?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> +<br>
> +private:<br>
> + void ReadNextRecord() {<br>
> + RecordData.clear();<br>
> + ULONG Count = 0;<br>
> + DWORD RequiredBufferSize;<br>
> + HRESULT hr = Enumerator->Next(1, 0, &RequiredBufferSize, nullptr, &Count);<br>
> + if (hr == S_OK) {<br>
<br>
Please use the SUCCEEDED macro.<br></blockquote><div>Specifically here, that won't work. <a href="https://msdn.microsoft.com/en-us/library/64c9d90x.aspx">https://msdn.microsoft.com/en-us/library/64c9d90x.aspx</a> It returns S_OK if it succeeds, and S_FALSE if it fails. Note that SUCCEEDED(S_FALSE) is true.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + RecordData.resize(<u></u>RequiredBufferSize);<br>
> + DWORD BytesRead = 0;<br>
> + hr = Enumerator->Next(1, RequiredBufferSize, &BytesRead,<br>
> + RecordData.data(), &Count);<br>
> + }<br>
> + if (hr == S_FALSE) {<br>
<br>
Please use the FAILED macro. This code will fail to end the<br>
enumeration if Next() fails for any reason other than S_FALSE.<br></blockquote><div>Same issue as before, in that FAILED(S_FALSE) is false. But your point is still valid, that other error cases aren't being handled here. So I think I need to check hr == S_FALSE || FAILED(hr)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + // This is the end of the enumeration.<br>
> + RecordData.clear();<br>
> + }<br>
> + }<br>
> +<br>
> + CComPtr<EnumeratorType> Enumerator;<br>
> + uint32_t CurrentRecord;<br>
> + llvm::SmallVector<uint8_t, 128> RecordData;<br>
<br>
Why 128? It seems rather large, and like something the caller might<br>
have a much better idea over.<br></blockquote><div>You're right it's probably large. 32 or 64 might be better. Some experiments show that most records are 16, 32, or 40 bytes. I didn't know that until I wrote this and ran my first dump though. On the other hand, letting the caller specify it clutters up the template argument list, which is kind of unfortunate. Perhaps best to just err on the smaller side with something like 32?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> +namespace llvm {<br>
> +namespace sys {<br>
> +namespace windows {<br>
> +extern std::error_code UTF8ToUTF16(StringRef utf8,<br>
> + SmallVectorImpl<wchar_t> &utf16);<br>
> +extern std::error_code UTF16ToUTF8(const wchar_t *utf16, size_t utf16_len,<br>
> + SmallVectorImpl<char> &utf8);<br>
<br>
Ugh, this really needs to find a reasonable home. I'm starting to<br>
suspect that now is the time.<br></blockquote><div>Noo!!!!!!!!!!!!!!! Don't make me do it :) Or at least, lemme get this change in first. I agree that it's unfortunate. Months ago I pushed for allowing windows-specific stuff in Support to be exposed through headers, but it seemed like there was considerable resistance to it. This is exactly the kind of thing I was worried about. </div><div> </div></div></div>