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

[Timur Iskhodzhanov]

2013/12/16 Reid Kleckner <rnk at google.com>

>
>   Arg, I drafted and lost a whole bunch of comments on this patch.
>  Sending them out, even though they are now out of date...
>

:(


> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:90
> @@ +89,3 @@
> +  // readily available.
> +  typedef DenseMap<std::pair<const char*, const char*>, char *>
> +      DirAndFilenameToFilepathMapTy;
> ----------------
> Is this specialized to use string comparison or will it use pointer
> comparison?  If Dir and Filename are uniqued in the IR pointer comparison
> could be OK.
>

Good point - will check.


> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.h:88
> @@ +87,3 @@
> +
> +  // TODO: we can probably get rid of this map if MDNode holds the
> fullpath
> +  // readily available.
> ----------------
> This TODO seems obsolete now.
>

Yep, it's gone in the newer version of the patch.


> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:42-44
> @@ +41,5 @@
> +  size_t Cursor = 0;
> +  // First, replace all slashes with backslashes.
> +  while ((Cursor = Filepath.find('/', Cursor)) != std::string::npos)
> +    Filepath[Cursor++] = '\\';
> +
> ----------------
> I suppose if the user is cross-compiling from Linux targeting Windows,
> they always want backslashes, so llvm::sys::path::native() is wrong.
>
> You can use its shorter implementation though:
>   std::replace(path.begin(), path.end(), '/', '\\')
>

Good catch - will do.


> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:30
> @@ +29,3 @@
> +  char *&Result =
> +      DirAndFilenameToFilepathMap[std::make_pair(Dir.data(),
> Filename.data())];
> +  if (Result != 0)
> ----------------
> Is this map here for performance or reducing memory usage?  I think the
> simpler way to reduce memory usage is to do the path canonicalization
> unconditionally, and then intern the string with a StringSet.  Saves a
> template instantiation.  *shrug*
>

I think this improves performance without sacrificing the memory usage too
much.
We call getFullFilename() for each debug location (think asm instruction),
so I think performance is really important here.


> ================
> Comment at: lib/CodeGen/AsmPrinter/WinCodeViewLineTables.cpp:51-52
> @@ +50,4 @@
> +
> +  // Replace all "\XXX\..\" with "\".  Don't try too hard though as the
> original
> +  // path should be well-formatted, e.g. start with a drive letter, etc.
> +  Cursor = 0;
> ----------------
> I guess we have to do it textually like this because we can't look at the
> filesystem.  We could be doing distributed cross-compilation, in which case
> the paths won't actually make sense.
>

Exactly.
Even running LL lit tests on the same machine after a while makes the
filepaths dangle.

http://llvm-reviews.chandlerc.com/D2232
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131216/d696f6de/attachment.html>