<div dir="ltr"><a href="http://llvm-reviews.chandlerc.com/D2232">http://llvm-reviews.chandlerc.com/D2232</a><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/12/3 Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">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 Tue, Dec 3, 2013 at 11:04 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
><br>
><br>
><br>
> 2013/12/3 Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>
>><br>
>> >> ><br>
>> >><br>
>> >> 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.<br>
>> ><br>
>> ><br>
>> > Ooph, that's a big one.<br>
>> > Any more tips to prioritize diving into this codebase?<br>
>> ><br>
>><br>
>> It mostly uses libObject to do the initial read and parse of the<br>
>> object file. Past that I'd suggest adding a parallel set of<br>
>> context/structure like the dwarf one. Ultimately it's going to look<br>
>> somewhat similar.<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>
>> >> Will do.<br>
>> ><br>
>> ><br>
>> > Ping?<br>
>> ><br>
>><br>
>> Can you rebase after the other patches and I'll take a more careful look?<br>
><br>
><br>
> Which ones?<br>
> It's already rebased on ToT.<br>
><br>
<br>
</div>Thought you had it in phabricator somewhere... if not, where is the<br>
current patch again? :)<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
>> -eric<br>
>><br>
>> >><br>
>> >> >> 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 "./",<br>
>> >> >> "../", etc.<br>
>> >> >> c) the slashes in the directory name and filename are not consistent<br>
>> >> >> on<br>
>> >> >> 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>
>> >> 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.<br>
>> ><br>
>> ><br>
>> > OK, will go down this path.<br>
>> ><br>
>> >><br>
>> >> -eric<br>
>> >><br>
>> >> > ping<br>
>> >> ><br>
>> >> >>> -eric<br>
>> >> >>><br>
>> >> >>><br>
>> >> >>> On Mon, Nov 18, 2013 at 9:14 AM, Timur Iskhodzhanov<br>
>> >> >>> <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>> wrote:<br>
>> >> >>>> I wrote some more lit tests for my patch and realized I was<br>
>> >> >>>> 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">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">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<br>
>> >> >>>>> 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<br>
>> >> >>>> atm<br>
>> >> >>>> than the other types of debug info.<br>
>> >> >>>><br>
>> >> >>>>> Looking at the patch, I think the approach the other patch took<br>
>> >> >>>>> of<br>
>> >> >>>>> abstracting the emission of debug information is a bit cleaner<br>
>> >> >>>>> and<br>
>> >> >>>>> it will<br>
>> >> >>>>> probably make life easier when adding more debug formats in the<br>
>> >> >>>>> 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<br>
>> >> >>>>> <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>><br>
>> >> >>>>> wrote:<br>
>> >> >>>>>><br>
>> >> >>>>>> 2013/11/14 Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">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"<br>
>> >> >>>>>> > COFF<br>
>> >> >>>>>> > sections that are sufficient to get function name / filename /<br>
>> >> >>>>>> > linenumber information. That's pretty much everything that's<br>
>> >> >>>>>> > required<br>
>> >> >>>>>> > for ASan to work beautifully in terms of the completeness of<br>
>> >> >>>>>> > error<br>
>> >> >>>>>> > reports.<br>
>> >> >>>>>> ><br>
>> >> >>>>>> > Attached is a prototype patch which I've tried on some simple<br>
>> >> >>>>>> > tests,<br>
>> >> >>>>>> > including some more complex ones with weird #line<br>
>> >> >>>>>> > constructions.<br>
>> >> >>>>>> > It also works just great on ASan/Win tests without any<br>
>> >> >>>>>> > link/run-time<br>
>> >> >>>>>> > warnings (I had a bunch of those during development, so I can<br>
>> >> >>>>>> > tell it<br>
>> >> >>>>>> > works rather than fails silently).<br>
>> >> >>>>>> ><br>
>> >> >>>>>> > I didn't have time to work on threading the command-line flags<br>
>> >> >>>>>> > into<br>
>> >> >>>>>> > the AsmPrinter yet, so currently it just replaces DWARF<br>
>> >> >>>>>> > 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<br>
>> >> >>>>>> > -g<br>
>> >> >>>>>> > ...".<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<br>
>> >> >>>>>> > path<br>
>> >> >>>>>> > forward?<br>
>> >> >>>>>> ><br>
>> >> >>>>>> > I'm very unfamiliar with LLVM CodeGen/MC, so I'm pretty sure I<br>
>> >> >>>>>> > made a<br>
>> >> >>>>>> > few weird things in the code... I've also put a few TODOs<br>
>> >> >>>>>> > 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<br>
>> >> >>>>>> > tables")<br>
>> >> >>>>>> ><br>
>> >> >>>>>> > 2) Am I right that DWARF is pretty much the only debug info<br>
>> >> >>>>>> > format<br>
>> >> >>>>>> > supported by LLVM/AsmPrinter right now? Do we want to take<br>
>> >> >>>>>> > an effort to come up with a generic debuginfogenerator<br>
>> >> >>>>>> > interface<br>
>> >> >>>>>> > to share between DwarfDebug and WinCodeViewLineTables?<br>
>> >> >>>>>> > Then AsmPrinter should just hold a<br>
>> >> >>>>>> > 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<br>
>> >> >>>>>> > Windows?<br>
>> >> >>>>>> > // Yeah, I should have looked at the DwarfDebug LIT tests<br>
>> >> >>>>>> > 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<br>
>> >> >>>>>> text<br>
>> >> >>>>>> format.<br>
>> >> >>>>>> I wrote a simple x86+x86_64 .ll test and FileCheck expectations<br>
>> >> >>>>>> 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<br>
>> >> >>>>>> 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<br>
>> >> >>>>>> things<br>
>> >> >>>>>> improved.<br>
>> >> >>>>>> I also removed the "TODO: test on X64" as I did try it on x64<br>
>> >> >>>>>> and<br>
>> >> >>>>>> 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">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>> >> >>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>> >> >>>>>><br>
>> >> >>>>><br>
>> >> >>>>><br>
>> >> >>>>><br>
>> >> >>>>> --<br>
>> >> >>>>> João Matos<br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>