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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
