[PATCH] [prototype] Adding line table debug information to LLVM on Windows

[Rafael Espíndola]

This is now 99% debug info, so I think Eric should be the one to give
the final LGTM. In any case, the MC bits LGTM.

On 20 December 2013 16:46, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>
>   Please take another look!
>
>
> ================
> Comment at: include/llvm/MC/MCObjectFileInfo.h:132
> @@ -131,1 +131,3 @@
>
> +  const MCSection *COFFDebugInfo;
> +
> ----------------
> Eric Christopher wrote:
>> Is all COFF debug info in one section or just the line tables?
>>
>> In general adding the sections to the MC layer can just be added as a separate small patch.
> Good catch - it should actually be `COFFDebugSymbolsSection`.
> Not sure adding it in a separate patch is worth it.
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:127
> @@ +126,3 @@
> +
> +  Asm->EmitInt32(4);  // Starting magic number.
> +
> ----------------
> Eric Christopher wrote:
>> Magic!
>>
>> How about an enum defined somewhere?
> Yikes, fixed!
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:37
> @@ +36,3 @@
> +
> +  struct FunctionDI {
> +    SmallVector<MCSymbol *, 10> Instrs;
> ----------------
> Eric Christopher wrote:
>> Comment of what this is and what it's used for.
> Renamed to `FunctionInfo` and added a comment.
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:48
> @@ +47,3 @@
> +
> +  struct InstrInfoTy {
> +    StringRef Filename;
> ----------------
> Eric Christopher wrote:
>> Comment of what this is and what it's used for.
> Done
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:59
> @@ +58,3 @@
> +
> +  struct FileNameRegistryTy {
> +    SmallVector<StringRef, 10> Filenames;
> ----------------
> Eric Christopher wrote:
>> Comment of what this is and what it's used for.
> Done
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:71
> @@ +70,3 @@
> +
> +    void add(StringRef Filename) {
> +      if (Infos.count(Filename))
> ----------------
> Eric Christopher wrote:
>> Comments.
> Done
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:245
> @@ +244,3 @@
> +
> +// TODO: This function was copied from DwardDebug.cpp. Any reason not to move
> +// the code to DebugLoc.h?
> ----------------
> Eric Christopher wrote:
>> Typo.
> Addressed the TODO, so no more typo needed :o)
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:246
> @@ +245,3 @@
> +// TODO: This function was copied from DwardDebug.cpp. Any reason not to move
> +// the code to DebugLoc.h?
> +// Walk up the scope chain of given debug loc and find line number info
> ----------------
> Eric Christopher wrote:
>> No particular reason I don't think.
> ditto
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:256
> @@ +255,3 @@
> +    // TODO: What's the 19 magic number? Is it a DWARF limitation or the way to
> +    // distinguish between what and what?
> +    if (SP->getNumOperands() > 19)
> ----------------
> Replaced with a FIXME to name the magic constant.
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:136
> @@ +135,3 @@
> +  // which holds the PC to file:line table.
> +  for (size_t I = 0, E = VisitedFunctions.size(); I != E; ++I) {
> +    const Function *GV = VisitedFunctions[I];
> ----------------
> Eric Christopher wrote:
>> If you could split this loop into functions it would make it a bit easier.
> Done, also improved the comments a little bit.
>
>
> http://llvm-reviews.chandlerc.com/D2232