[PATCH] D147256: [DebugInfo] Fix file path separator when targeting windows.

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

In D147256#4236522 <https://reviews.llvm.org/D147256#4236522>, @hans wrote:

> Thanks for working on this!
>
> The `-ffile-reproducible` flag name refers to making `#file` directives reproducible, but `LangOptions.UseTargetPathSeparator` sounds a lot broader :) I don't know what others think, but it would be nice to not have to introduce any more flags at least.

Oh, I was already using `LangOptions.UseTargetPathSeparator`. Updated the comment on UseTargetPathSeparator.



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:544
     MainFileDir = std::string(MainFile->getDir().getName());
     if (!llvm::sys::path::is_absolute(MainFileName)) {
       llvm::SmallString<1024> MainFileDirSS(MainFileDir);
----------------
hans wrote:
> Do we want to fix absolute filenames too?
> I can see arguments for and against:
> - Using what the user provided makes sense
> - Some build systems might use absolute paths for everything. But on the other hand such builds have larger determinism problems (including the full paths).
> So the current decision probably makes sense.
Yeah. If it's already absolute filename, it will just use that one user provided. 


================
Comment at: clang/test/CodeGen/debug-info-slash.c:5
+
+// WIN:   !DIFile(filename: "{{.*}}clang/test/CodeGen\\debug-info-slash.c"
+// LINUX: !DIFile(filename: "{{.*}}clang/test/CodeGen/debug-info-slash.c"
----------------
hans wrote:
> Does the test runner write the 'clang/test/CodeGen' path with forward slashes also on Windows?
No, it uses `clang\\test\\CodeGen`. Removed this part.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:787
 
   StringRef PathRef(Asm->TM.Options.ObjectFilenameForDebug);
   llvm::SmallString<256> PathStore(PathRef);
----------------
hans wrote:
> This handles codeview. Does anything need to be done for dwarf on windows? mstorsjo might have input on that.
It looks like `TM.Options.ObjectFilenameForDebug` is only used for codeview. I guess dwarf doesn't store the object file path.


================
Comment at: llvm/test/DebugInfo/COFF/build-info.ll:18
+; RUN: cd %t-dir
+; RUN: llc -filetype=obj -mtriple i686-pc-windows-msvc %s -o ../build-info.o
+; RUN: llvm-readobj --codeview ../build-info.o | FileCheck %s --check-prefix=OBJ
----------------
hans wrote:
> Does this write the .o file into the test directory? I don't think that's allowed (it may be read-only). But the test could create another subdirectory under `%t-dir`.
It writes the .o file into the parent directory of the temporary dir %t-dir. It will just be the directory path of a normal `%t`, not the source test directory. The reason I'm not using `%t-dir/build-info.o` in the parent dir is because it will be translated into an absolute address. That will remain unchanged in ObjectName.


================
Comment at: llvm/test/DebugInfo/COFF/build-info.ll:21
+
+; OBJ: ObjectName: ..\build-info.o
+
----------------
hans wrote:
> But in the `llc` invocation, the user wrote a relative path with a forward slash. What behavior do we want? What should happen if there are more then one slash - I think remove_dots just works on the first one?
Yeah, the input uses forward slash but we convert it into backslash. When there are multiple slashs, they will all be converted into backslashs, which is done by `llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true, llvm::sys::path::Style::windows_backslash);`. `remove_dots` not only remove redundant dots but also canonicalize path separator based on the style.

I think ideally we want just take what user provided as the ObjectName with the redundant dots removed. But `remove_dots` always canonicalizes the path separator based on the style. I don't find any function that doesn't canonicalize the path. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147256/new/

https://reviews.llvm.org/D147256