<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Nov 20, 2013 at 9:46 AM, Timur Iskhodzhanov <span dir="ltr"><<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Eric, David,<br>
<br>
2013/11/19 Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com">timurrrr@google.com</a>>:<br>
<div class="im">> Attached is a slightly updated patch.<br>
> (it doesn't include D2222 yet).<br>
<br>
</div>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">http://llvm-reviews.chandlerc.com/D2232</a><br>
(you need to apply D2222 first if you'd like to give D2232 a try)<br>
<div class="im"><br>
> 2013/11/19 Eric Christopher <<a href="mailto:echristo@gmail.com">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">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>
</div>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?</blockquote><div><br></div><div>I think we can use the triple OS to choose a sensible default here (win32 -> CodeView, mingw -> DWARF), but eventually some users might want to force DWARF on win32 with a flag because it is more complete.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> 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>
>> I'm not sure if you're going to want to support both debug info at the<br>
>> same time, but it's a readonly format at debug emission so I don't see<br>
>> it as being a problem.<br>
><br>
> Well, if two debug formats share the same section name, they might<br>
> conflict with each other.<br>
> I don't think this is the case for Dwarf&CodeView [yet?].<br>
><br>
>> -Zmlt makes sense as the clang-cl name, or just<br>
>> make it whatever the debug mode flag is for cl.exe - this is at least<br>
>> a start down that path.<br>
><br>
> 2013/11/19 Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>>:<br>
>> I'd just use /Z7, since that's the cl.exe flag for the compatible format.<br>
>> This will need a long-form clang -cc1 flag name, though.<br>
><br>
> Z7 implies a much more complete debug info than what we'll emit short-term.<br>
> Since Z7 is not widely used, I don't think threading Z7 is any simpler<br>
> than Zmlt.<br>
> I don't have a strong opinion though, WDYT?<br>
<br>
</div>[I agreed to use Z7 on this thread]<br>
<div class="im"><br>
>> I think the best way for the tests will be to write a set of parsers,<br>
>> etc that can dump the info. I realize this is likely a very large<br>
>> undertaking as well, but it seems like the only way to ensure we're<br>
>> getting some decent testing out.<br>
><br>
> As you can see from my patch, I actually succeeded in writing *some*<br>
> tests without a dumper - just by using the MCAsmStreamer.<br>
> Do you think we should really write a dumper too?<br>
> That's kinda hard and we don't plan to fully support the CodeView format yet...<br>
<br>
</div>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>
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>
<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>
> 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>
</div>ping</blockquote><div><br></div><div>I think the problems you mention can be resolved without changing the DI metadata format.  Debug info metadata already consumes too much memory.</div></div></div></div>