[LLVMdev] Adding line table debug information to LLVM on Windows

[Eric Christopher]

On Wed, Nov 20, 2013 at 9:46 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> Eric, David,
>
> 2013/11/19 Timur Iskhodzhanov <timurrrr at google.com>:
>> Attached is a slightly updated patch.
>> (it doesn't include D2222 yet).
>
> The new version of the patch stopped fitting into the llvmdev 100K limit,
> so I've uploaded it to http://llvm-reviews.chandlerc.com/D2232
> (you need to apply D2222 first if you'd like to give D2232 a try)
>

That's a big patch. :)

>> 2013/11/19 Eric Christopher <echristo at gmail.com>:
>>> In general I do think we're going to need to abstract it out as much
>>> as possible. I'm not sure what the previous patch looks like, but
>>> abstracting the interface out would be general goodness for this. We
>>> can talk about designs for that as we move on.
>>
>> How about http://llvm-reviews.chandlerc.com/D2222 ?
>> // Ha! A lucky number!
>>
>>> As far as how to
>>> migrate the decision down we can have it both as an option to code gen
>>> maybe or, for now, make it dependent upon triple. The former is, I
>>> think, the best option there.
>>
>> You mean LLVM CodeGen or Clang CodeGen?
>

LLVM... and we really need to change that directory in Clang. :)

> On the second thought, "llc" seems to choose ELF vs COFF by using a triple.
> I think we should make the DWARF vs CodeView choice in sync with the
> object file format choice, at least to begin with.
> WDYT?
>

Yep. Sounds good to me. I'd suggest triple, that way you can base it
on "windows" versus "coff".

>> Can you suggest a few places in the code where I can find the clues for that?
>> I'm not yet familiar with this part of the project...
>>

lib/CodeGen/AsmPrinter/DwarfDebug.cpp has some checks for triple/os.

>
> I tried my patch on Chromium and it hit the llvm_unreachable I wrote
> in WinCOFFStreamer::EmitCOFFStaticOffset().
> Now that it also supports using a fixup to calculate the offset (that
> happens as the second pass, not supported by MCAsmStreamer, right?), I
> think I do have to write at least a simple dumper...
>
> Any hints on how to do that? Should it be a separate app or built into
> some COFF reader readily available? Should it have its own tests, i.e.
> binary COFF files added to the repo?
>

Probably the same structure that we've been going down via
lib/DebugInfo/. A set of files than handle reading and parsing and
both some binary files and some files produced by the backend.

> Good news - other than that, the emitter seems to work fine on some
> medium-sized Chromium tests and generate symbolized ASan reports if I
> manually introduce an error in the binary.
>

Awesome.

>>> Any other questions I missed?
>>
>> Please see the TODOs in the attached patch.
>> You are very likely to come up with a better design/ideas given I'm
>> new to this part of the codebase.
>>

Will do.

>> One particular question I'd like to emphasize is getting a full
>> filepath for a given MDNode.
>> As far as I can tell, the metadata for scopes holds pairs of
>> <filename, directory>, which reflects how DWARF stores them.
>> However, CodeView stores full paths as entire strings (I admit that
>> ain't efficient).
>> Currently, I concat the directory and filename together, but it
>> a) requires some extra memory management
>> b) requires special tricks to handle filenames starting from "./", "../", etc.
>> c) the slashes in the directory name and filename are not consistent on Windows
>> and is ugly in general.
>>
>> Do you think it's appropriate to change the scope metadata format to
>> store <filename, directory, fullpath> instead?
>> That'd require changing Clang, right?
>

It would require changing clang. Since this is a single string per
file I'm not against adding the full path to the source file in the
metadata along side the basename/compilation dir pair.

-eric

> ping
>
>>> -eric
>>>
>>>
>>> On Mon, Nov 18, 2013 at 9:14 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>> I wrote some more lit tests for my patch and realized I was generating
>>>> some redundant info. This is fixed now. Attached is a new version
>>>> of the prototype patch with some more tests.
>>>>
>>>> 2013/11/15 João Matos <ripzonetriton at gmail.com>:
>>>>> Hi Timur,
>>>>>
>>>>> There's also a pending patch adding CodeView support in Phab:
>>>>> http://llvm-reviews.chandlerc.com/D165
>>>>
>>>> I haven't looked at your patch yet, but based on the very low phab
>>>> review number, I'm pretty sure there are good reasons this wasn't
>>>> committed.
>>>>
>>>>> Does your patch provide just a subset of the CodeView debug info provided in
>>>>> the other patch?
>>>>
>>>> Yes. I prefer small incremental changes.
>>>> Also, the file:line debug info is much much more important for me atm
>>>> than the other types of debug info.
>>>>
>>>>> Looking at the patch, I think the approach the other patch took of
>>>>> abstracting the emission of debug information is a bit cleaner and it will
>>>>> probably make life easier when adding more debug formats in the future.
>>>>
>>>> Why wasn't the "abstract the emission of DI" part of that patch
>>>> reviewed/committed separately?
>>>>
>>>>> On Fri, Nov 15, 2013 at 4:39 PM, Timur Iskhodzhanov <timurrrr at google.com>
>>>>> wrote:
>>>>>>
>>>>>> 2013/11/14 Timur Iskhodzhanov <timurrrr at google.com>:
>>>>>> > Hi David, Eric, LLVM devs,
>>>>>> >
>>>>>> > You've probably heard about AddressSanitizer (ASan) and other
>>>>>> > sanitizers based on LLVM. One of the things that makes ASan
>>>>>> > not as awesome on Windows as it is on Linux
>>>>>> > is the symbolization of the stacks.
>>>>>> >
>>>>>> > Currently, ASan runtime on Windows uses
>>>>>> > CaptureStackBackTrace/SymFromAddr/SymGetLineFromAddr64
>>>>>> > to unwind and symbolize stacks.  This works like a charm
>>>>>> > in-process for stack frames built with CL, but yields
>>>>>> > "function+0x0ff537" for frames built with Clang.
>>>>>> >
>>>>>> > I came up with a prototype which emits "old-style debug info" COFF
>>>>>> > sections that are sufficient to get function name / filename /
>>>>>> > linenumber information.  That's pretty much everything that's required
>>>>>> > for ASan to work beautifully in terms of the completeness of error
>>>>>> > reports.
>>>>>> >
>>>>>> > Attached is a prototype patch which I've tried on some simple tests,
>>>>>> > including some more complex ones with weird #line constructions.
>>>>>> > It also works just great on ASan/Win tests without any link/run-time
>>>>>> > warnings (I had a bunch of those during development, so I can tell it
>>>>>> > works rather than fails silently).
>>>>>> >
>>>>>> > I didn't have time to work on threading the command-line flags into
>>>>>> > the AsmPrinter yet, so currently it just replaces DWARF entirely.
>>>>>> > Of course, this should be fixed before this lands into trunk.
>>>>>> > Currently, one can try this patch by using "clang-cl -Xclang -g ...".
>>>>>> > Eventually we should have some dedicated flag for clang-cl.
>>>>>> >
>>>>>> > Can you please take a look at the patch and suggest a good path forward?
>>>>>> >
>>>>>> > I'm very unfamiliar with LLVM CodeGen/MC, so I'm pretty sure I made a
>>>>>> > few weird things in the code...  I've also put a few TODOs with
>>>>>> > questions and suggestions.
>>>>>> >
>>>>>> > Some general questions:
>>>>>> > 1) Threading flags from the driver down to CodeGen.
>>>>>> >   How do we do that? Should we support all 4 combinations
>>>>>> >   of no-info/DWARF/CVLT/both?
>>>>>> >   How about "-Zmlt" as the clang-cl flag name? ("minimal line tables")
>>>>>> >
>>>>>> > 2) Am I right that DWARF is pretty much the only debug info format
>>>>>> >   supported by LLVM/AsmPrinter right now?  Do we want to take
>>>>>> >   an effort to come up with a generic debuginfogenerator interface
>>>>>> >   to share between DwarfDebug and WinCodeViewLineTables?
>>>>>> >   Then AsmPrinter should just hold a SmallVector<DebugInfoEmitter*>
>>>>>> >   rather than a pair of DD/DE pointers.
>>>>>> >
>>>>>> > 3) How would you suggest to write WinCodeViewLineTables tests
>>>>>> >   given that dumpbin is not available everywhere except Windows?
>>>>>> >   // Yeah, I should have looked at the DwarfDebug LIT tests and
>>>>>> >   // written some; but the prototype development went faster
>>>>>> >   // than I expected...
>>>>>>
>>>>>> I found the MCAsmStreamer being used by llc which gives a decent text
>>>>>> format.
>>>>>> I wrote a simple x86+x86_64 .ll test and FileCheck expectations for
>>>>>> the llc asm output.
>>>>>> Is this the right approach to write tests?
>>>>>> If so, I'll convert my remaining C program test cases into such an
>>>>>> .ll+llc tests.
>>>>>>
>>>>>> > Can you suggest ways to split this patch so it's easier
>>>>>> > to review part-by-part before this hits trunk?
>>>>>>
>>>>>> Attached is an updated patch with a new test and a few minor things
>>>>>> improved.
>>>>>> I also removed the "TODO: test on X64" as I did try it on x64 and no
>>>>>> changes were required.
>>>>>>
>>>>>> Looking forward to your feedback!
>>>>>>
>>>>>> > Thanks!
>>>>>> > --
>>>>>> > Timur Iskhodzhanov,
>>>>>> > Google
>>>>>>
>>>>>> _______________________________________________
>>>>>> LLVM Developers mailing list
>>>>>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> João Matos