<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2013/11/21 Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank" class="cremed">echristo@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, Nov 20, 2013 at 9:46 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" class="cremed">timurrrr@google.com</a>> wrote:<br>
> Eric, David,<br>
><br>
> 2013/11/19 Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" class="cremed">timurrrr@google.com</a>>:<br>
>> Attached is a slightly updated patch.<br>
>> (it doesn't include D2222 yet).<br>
><br>
> The new version of the patch stopped fitting into the llvmdev 100K limit,<br>
> so I've uploaded it to <a href="http://llvm-reviews.chandlerc.com/D2232" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/D2232</a><br>
> (you need to apply D2222 first if you'd like to give D2232 a try)<br>
><br>
<br>
</div>That's a big patch. :)<br></blockquote><div><br></div><div>Mostly because of the tests, I hope :)</div><div><br></div><div>At least I already committed some obviously-independent parts of it...</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
>> 2013/11/19 Eric Christopher <<a href="mailto:echristo@gmail.com" class="cremed">echristo@gmail.com</a>>:<br>
>>> In general I do think we're going to need to abstract it out as much<br>
>>> as possible. I'm not sure what the previous patch looks like, but<br>
>>> abstracting the interface out would be general goodness for this. We<br>
>>> can talk about designs for that as we move on.<br>
>><br>
>> How about <a href="http://llvm-reviews.chandlerc.com/D2222" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/D2222</a> ?<br>
>> // Ha! A lucky number!<br>
>><br>
>>> As far as how to<br>
>>> migrate the decision down we can have it both as an option to code gen<br>
>>> maybe or, for now, make it dependent upon triple. The former is, I<br>
>>> think, the best option there.<br>
>><br>
>> You mean LLVM CodeGen or Clang CodeGen?<br>
><br>
<br>
</div>LLVM... and we really need to change that directory in Clang. :)<br>
<div class="im"><br>
> On the second thought, "llc" seems to choose ELF vs COFF by using a triple.<br>
> I think we should make the DWARF vs CodeView choice in sync with the<br>
> object file format choice, at least to begin with.<br>
> WDYT?<br>
><br>
<br>
</div>Yep. Sounds good to me. I'd suggest triple, that way you can base it<br>
on "windows" versus "coff".<br>
<div class="im"><br>
>> Can you suggest a few places in the code where I can find the clues for that?<br>
>> I'm not yet familiar with this part of the project...<br>
>><br>
<br>
</div>lib/CodeGen/AsmPrinter/DwarfDebug.cpp has some checks for triple/os.</blockquote><div><br></div><div>OK, I use the Triple().isOSWindows() now.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
><br>
> I tried my patch on Chromium and it hit the llvm_unreachable I wrote<br>
> in WinCOFFStreamer::EmitCOFFStaticOffset().<br>
> Now that it also supports using a fixup to calculate the offset (that<br>
> happens as the second pass, not supported by MCAsmStreamer, right?), I<br>
> think I do have to write at least a simple dumper...<br>
><br>
> Any hints on how to do that? Should it be a separate app or built into<br>
> some COFF reader readily available? Should it have its own tests, i.e.<br>
> binary COFF files added to the repo?<br>
><br>
<br>
</div>Probably the same structure that we've been going down via<br>
lib/DebugInfo/. A set of files than handle reading and parsing and<br>
both some binary files and some files produced by the backend.</blockquote><div><br></div><div>Ooph, that's a big one.</div><div>Any more tips to prioritize diving into this codebase?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> Good news - other than that, the emitter seems to work fine on some<br>
> medium-sized Chromium tests and generate symbolized ASan reports if I<br>
> manually introduce an error in the binary.<br>
><br>
<br>
</div>Awesome.<br>
<div class="im"><br>
>>> Any other questions I missed?<br>
>><br>
>> Please see the TODOs in the attached patch.<br>
>> You are very likely to come up with a better design/ideas given I'm<br>
>> new to this part of the codebase.<br>
>><br>
<br>
</div>Will do.</blockquote><div><br></div><div>Ping?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
>> One particular question I'd like to emphasize is getting a full<br>
>> filepath for a given MDNode.<br>
>> As far as I can tell, the metadata for scopes holds pairs of<br>
>> <filename, directory>, which reflects how DWARF stores them.<br>
>> However, CodeView stores full paths as entire strings (I admit that<br>
>> ain't efficient).<br>
>> Currently, I concat the directory and filename together, but it<br>
>> a) requires some extra memory management<br>
>> b) requires special tricks to handle filenames starting from "./", "../", etc.<br>
>> c) the slashes in the directory name and filename are not consistent on Windows<br>
>> and is ugly in general.<br>
>><br>
>> Do you think it's appropriate to change the scope metadata format to<br>
>> store <filename, directory, fullpath> instead?<br>
>> That'd require changing Clang, right?<br>
><br>
<br>
</div>It would require changing clang. Since this is a single string per<br>
file I'm not against adding the full path to the source file in the<br>
metadata along side the basename/compilation dir pair.</blockquote><div><br></div><div>OK, will go down this path.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888">
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> ping<br>
><br>
>>> -eric<br>
>>><br>
>>><br>
>>> On Mon, Nov 18, 2013 at 9:14 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" class="cremed">timurrrr@google.com</a>> wrote:<br>
>>>> I wrote some more lit tests for my patch and realized I was generating<br>
>>>> some redundant info. This is fixed now. Attached is a new version<br>
>>>> of the prototype patch with some more tests.<br>
>>>><br>
>>>> 2013/11/15 João Matos <<a href="mailto:ripzonetriton@gmail.com" class="cremed">ripzonetriton@gmail.com</a>>:<br>
>>>>> Hi Timur,<br>
>>>>><br>
>>>>> There's also a pending patch adding CodeView support in Phab:<br>
>>>>> <a href="http://llvm-reviews.chandlerc.com/D165" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/D165</a><br>
>>>><br>
>>>> I haven't looked at your patch yet, but based on the very low phab<br>
>>>> review number, I'm pretty sure there are good reasons this wasn't<br>
>>>> committed.<br>
>>>><br>
>>>>> Does your patch provide just a subset of the CodeView debug info provided in<br>
>>>>> the other patch?<br>
>>>><br>
>>>> Yes. I prefer small incremental changes.<br>
>>>> Also, the file:line debug info is much much more important for me atm<br>
>>>> than the other types of debug info.<br>
>>>><br>
>>>>> Looking at the patch, I think the approach the other patch took of<br>
>>>>> abstracting the emission of debug information is a bit cleaner and it will<br>
>>>>> probably make life easier when adding more debug formats in the future.<br>
>>>><br>
>>>> Why wasn't the "abstract the emission of DI" part of that patch<br>
>>>> reviewed/committed separately?<br>
>>>><br>
>>>>> On Fri, Nov 15, 2013 at 4:39 PM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" class="cremed">timurrrr@google.com</a>><br>
>>>>> wrote:<br>
>>>>>><br>
>>>>>> 2013/11/14 Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" class="cremed">timurrrr@google.com</a>>:<br>
>>>>>> > Hi David, Eric, LLVM devs,<br>
>>>>>> ><br>
>>>>>> > You've probably heard about AddressSanitizer (ASan) and other<br>
>>>>>> > sanitizers based on LLVM. One of the things that makes ASan<br>
>>>>>> > not as awesome on Windows as it is on Linux<br>
>>>>>> > is the symbolization of the stacks.<br>
>>>>>> ><br>
>>>>>> > Currently, ASan runtime on Windows uses<br>
>>>>>> > CaptureStackBackTrace/SymFromAddr/SymGetLineFromAddr64<br>
>>>>>> > to unwind and symbolize stacks. This works like a charm<br>
>>>>>> > in-process for stack frames built with CL, but yields<br>
>>>>>> > "function+0x0ff537" for frames built with Clang.<br>
>>>>>> ><br>
>>>>>> > I came up with a prototype which emits "old-style debug info" COFF<br>
>>>>>> > sections that are sufficient to get function name / filename /<br>
>>>>>> > linenumber information. That's pretty much everything that's required<br>
>>>>>> > for ASan to work beautifully in terms of the completeness of error<br>
>>>>>> > reports.<br>
>>>>>> ><br>
>>>>>> > Attached is a prototype patch which I've tried on some simple tests,<br>
>>>>>> > including some more complex ones with weird #line constructions.<br>
>>>>>> > It also works just great on ASan/Win tests without any link/run-time<br>
>>>>>> > warnings (I had a bunch of those during development, so I can tell it<br>
>>>>>> > works rather than fails silently).<br>
>>>>>> ><br>
>>>>>> > I didn't have time to work on threading the command-line flags into<br>
>>>>>> > the AsmPrinter yet, so currently it just replaces DWARF entirely.<br>
>>>>>> > Of course, this should be fixed before this lands into trunk.<br>
>>>>>> > Currently, one can try this patch by using "clang-cl -Xclang -g ...".<br>
>>>>>> > Eventually we should have some dedicated flag for clang-cl.<br>
>>>>>> ><br>
>>>>>> > Can you please take a look at the patch and suggest a good path forward?<br>
>>>>>> ><br>
>>>>>> > I'm very unfamiliar with LLVM CodeGen/MC, so I'm pretty sure I made a<br>
>>>>>> > few weird things in the code... I've also put a few TODOs with<br>
>>>>>> > questions and suggestions.<br>
>>>>>> ><br>
>>>>>> > Some general questions:<br>
>>>>>> > 1) Threading flags from the driver down to CodeGen.<br>
>>>>>> > How do we do that? Should we support all 4 combinations<br>
>>>>>> > of no-info/DWARF/CVLT/both?<br>
>>>>>> > How about "-Zmlt" as the clang-cl flag name? ("minimal line tables")<br>
>>>>>> ><br>
>>>>>> > 2) Am I right that DWARF is pretty much the only debug info format<br>
>>>>>> > supported by LLVM/AsmPrinter right now? Do we want to take<br>
>>>>>> > an effort to come up with a generic debuginfogenerator interface<br>
>>>>>> > to share between DwarfDebug and WinCodeViewLineTables?<br>
>>>>>> > Then AsmPrinter should just hold a SmallVector<DebugInfoEmitter*><br>
>>>>>> > rather than a pair of DD/DE pointers.<br>
>>>>>> ><br>
>>>>>> > 3) How would you suggest to write WinCodeViewLineTables tests<br>
>>>>>> > given that dumpbin is not available everywhere except Windows?<br>
>>>>>> > // Yeah, I should have looked at the DwarfDebug LIT tests and<br>
>>>>>> > // written some; but the prototype development went faster<br>
>>>>>> > // than I expected...<br>
>>>>>><br>
>>>>>> I found the MCAsmStreamer being used by llc which gives a decent text<br>
>>>>>> format.<br>
>>>>>> I wrote a simple x86+x86_64 .ll test and FileCheck expectations for<br>
>>>>>> the llc asm output.<br>
>>>>>> Is this the right approach to write tests?<br>
>>>>>> If so, I'll convert my remaining C program test cases into such an<br>
>>>>>> .ll+llc tests.<br>
>>>>>><br>
>>>>>> > Can you suggest ways to split this patch so it's easier<br>
>>>>>> > to review part-by-part before this hits trunk?<br>
>>>>>><br>
>>>>>> Attached is an updated patch with a new test and a few minor things<br>
>>>>>> improved.<br>
>>>>>> I also removed the "TODO: test on X64" as I did try it on x64 and no<br>
>>>>>> changes were required.<br>
>>>>>><br>
>>>>>> Looking forward to your feedback!<br>
>>>>>><br>
>>>>>> > Thanks!<br>
>>>>>> > --<br>
>>>>>> > Timur Iskhodzhanov,<br>
>>>>>> > Google<br>
>>>>>><br>
>>>>>> _______________________________________________<br>
>>>>>> LLVM Developers mailing list<br>
>>>>>> <a href="mailto:LLVMdev@cs.uiuc.edu" class="cremed">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank" class="cremed">http://llvm.cs.uiuc.edu</a><br>
>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>>>>>><br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>> --<br>
>>>>> João Matos<br>
</div></div></blockquote></div><br></div></div>