[DebugInfo] Fix file path separator when targeting windows.

[zequanwu]

Summary

This fixes two problems:

1. When crossing compiling for windows on linux, source file path in debug info is concatenated with directory by host native separator ('/'). For windows local build, they are concatenated by '\'. This causes non-determinism bug.
   
   The solution here is to let LangOptions.UseTargetPathSeparator to control if we should use host native separator or not.

2. Objectfile path in CodeView also uses host native separator when generated.
   
   It's fixed by changing the path separator in /Fo to '\' if the path is not an absolute path when adding the -object-file-name= flag.

[hans]

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.

clang/lib/CodeGen/CGDebugInfo.cpp

544

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.

clang/test/CodeGen/debug-info-slash.c

6

Does the test runner write the 'clang/test/CodeGen' path with forward slashes also on Windows?

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

787

This handles codeview. Does anything need to be done for dwarf on windows? mstorsjo might have input on that.

llvm/test/DebugInfo/COFF/build-info.ll

13 ↗

(On Diff #509771)

s/canalized/canonicalized/

18 ↗

(On Diff #509771)

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.

21 ↗

(On Diff #509771)

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?

[zequanwu]

Address comments.

[zequanwu]

In 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.

clang/lib/CodeGen/CGDebugInfo.cpp

544

Yeah. If it's already absolute filename, it will just use that one user provided.

clang/test/CodeGen/debug-info-slash.c

6

No, it uses clang\\test\\CodeGen. Removed this part.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

787

It looks like TM.Options.ObjectFilenameForDebug is only used for codeview. I guess dwarf doesn't store the object file path.

llvm/test/DebugInfo/COFF/build-info.ll

18 ↗

(On Diff #509771)

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.

21 ↗

(On Diff #509771)

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.

[probinson]

I think we cannot be 100% sure about source paths in a cross-compile situation. Cross-compiling on platform A targeting platform B does not mean your sources and debugger UI are on platform B. My users keep source and debugger UI on platform A, debugging target B remotely. We need to preserve the host pathnames. It is not clear to me that this patch does so.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

787

Right, DWARF only stores the source path.

[hans]

In D147256#4237099, @probinson wrote:

I think we cannot be 100% sure about source paths in a cross-compile situation. Cross-compiling on platform A targeting platform B does not mean your sources and debugger UI are on platform B. My users keep source and debugger UI on platform A, debugging target B remotely. We need to preserve the host pathnames. It is not clear to me that this patch does so.

I believe the idea is to preserve host pathnames as long as LangOptions.UseTargetPathSeparator is unset. But I see that might not be the case for CodeViewDebug::emitObjName() in the current version of the patch. Could we make clang handle that path instead depending on the flag?

llvm/test/DebugInfo/COFF/build-info.ll

18 ↗

(On Diff #509771)

Right, I'm just thinking that we can't be sure that we can (or should) write files into the parent of %t-dir.

Would it work if you instead create %t-dir/subdir, cd into that, and then do -o ../build-info.o?

[zequanwu]

• Pass LangOptions.UseTargetPathSeparator down to TargetOptions to use host path separator when it's set.
• Add a test case. The problem is when if we are targeting to linux, clang won't generate codeview debuginfo with -gcodeview. So, I only add the test to target on windows.

[probinson]

An LLVM code change should be testable on its own; this has it tested by Clang.
I think you need a new command-line option to set TargetOptions::UseTargetPathSeparator e.g. via llvm-mc. Other TargetOptions are handled this way.

[zequanwu]

• Add a -use-target-path-separator flag for llc.
• Add test for llc with that flag.

[hans]

In D147256#4249527, @zequanwu wrote:

• Add a -use-target-path-separator flag for llc.
• Add test for llc with that flag.

But where does TM.Options.ObjectFilenameForDebug come from? Presumably it comes from Clang at some point, so is there any chance we can fix it "at the source" instead?

[zequanwu]

In D147256#4257797, @hans wrote:

In D147256#4249527, @zequanwu wrote:

• Add a -use-target-path-separator flag for llc.
• Add test for llc with that flag.

But where does TM.Options.ObjectFilenameForDebug come from? Presumably it comes from Clang at some point, so is there any chance we can fix it "at the source" instead?

TM.Options.ObjectFilenameForDebug either comes from llc's -o or clang-cl's object-file-name= which is translated from /Fo[ObjectFileName].

For Chromium, the /Fo[ObjectFileName] we pass to clang-cl is the same when building on Windows and targeting Windows from Linux. The problem comes llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true); in CodeViewDebug.cpp. That function always convert the path to use host's path separator. And I don't think we can write a remove_dots function that doesn't change path separator, because \ can only be used as path separator on Windows but is a valid path character on Linux.

Or we could just not use llvm::sys::path::remove_dots, use the user's input as it is for object file path.

[zequanwu]

In D147256#4258419, @zequanwu wrote:

In D147256#4257797, @hans wrote:

In D147256#4249527, @zequanwu wrote:

• Add a -use-target-path-separator flag for llc.
• Add test for llc with that flag.

But where does TM.Options.ObjectFilenameForDebug come from? Presumably it comes from Clang at some point, so is there any chance we can fix it "at the source" instead?

TM.Options.ObjectFilenameForDebug either comes from llc's -o or clang-cl's object-file-name= which is translated from /Fo[ObjectFileName].

For Chromium, the /Fo[ObjectFileName] we pass to clang-cl is the same when building on Windows and targeting Windows from Linux. The problem comes llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true); in CodeViewDebug.cpp. That function always convert the path to use host's path separator. And I don't think we can write a remove_dots function that doesn't change path separator, because \ can only be used as path separator on Windows but is a valid path character on Linux.

Or we could just not use llvm::sys::path::remove_dots, use the user's input as it is for object file path.

Well, MSVC cl removes redundant dots so we shouldn't remove llvm::sys::path::remove_dots.
Generally, I think it's okay to use target path separator for object file path in codeview when user input is not an absolute path, because we can only generate codeview when targeting on windows.

[hans]

Well, MSVC cl removes redundant dots so we shouldn't remove llvm::sys::path::remove_dots.

Could we do the remove_dots on the Clang side, where we can decide based on the LangOpts?

[zequanwu]

In D147256#4261949, @hans wrote:

Well, MSVC cl removes redundant dots so we shouldn't remove llvm::sys::path::remove_dots.

Could we do the remove_dots on the Clang side, where we can decide based on the LangOpts?

Yes, we can do that. Is there a reason to favor there over here? At here, the object path in llc output can also benefits from remove_dots, not just clang.

[hans]

In D147256#4262002, @zequanwu wrote:

In D147256#4261949, @hans wrote:

Well, MSVC cl removes redundant dots so we shouldn't remove llvm::sys::path::remove_dots.

Could we do the remove_dots on the Clang side, where we can decide based on the LangOpts?

Yes, we can do that. Is there a reason to favor there over here? At here, the object path in llc output can also benefits from remove_dots, not just clang.

The benefit is that if we do it in Clang, we can just look at the LangOpts and don't have to add a new target option, command line flag etc.

For llc, since it's an internal tool, I think it would be fine to just use the name as given.

[aganea]

+ @saudi @thieta

[zequanwu]

• Move remove_dots to clang driver.
• Revert changes to llc.

[hans]

clang/lib/Driver/ToolChains/Clang.cpp

583

Won't the code above (line 580) make many filenames absolute and cause us to use native slashes even when we want backslashes?

This would also need a test.

[zequanwu]

Added clang-cl option test.

clang/lib/Driver/ToolChains/Clang.cpp

583

Won't the code above (line 580) make many filenames absolute and cause us to use native slashes even when we want backslashes?

Yes, I don't think we should change anything if it's converted to an absolute path.

Only if the -fdebug-compilation-dir is not given or is an absolute path, the path in /Fo will be converted to absolute path. Users can avoid the path being converted to absolute path by passing a relative path to -fdebug-compilation-dir, just like what we do in chrome build (-fdebug-compilation-dir=.).

Added a test.

[hans]

clang/lib/Driver/ToolChains/Clang.cpp

583

Apologies for all the questions, but I find the logic a bit complex still. The test cases are helpful though.

Could we simplify by just always using Windows slashes here, regardless of -fdebug-compilation-dir etc.?

[zequanwu]

clang/lib/Driver/ToolChains/Clang.cpp

583

The object file path will be converted to absolute path depending on whether -fdebug-compilation-dir is given or whether it's absolute path. I don't think we want to change a native absolute path of object file to use Windows slashes. Or this could happen on linux: /usr/local/src/a.obj -> \usr\local\src\a.obj. So, we only use windows slashes when it's not an absolute path here.

[hans]

lgtm

clang/lib/Driver/ToolChains/Clang.cpp

583

Thanks, that makes sense. Can you add a comment with that explanation?

[zequanwu]

Add comment.

clang/lib/Driver/ToolChains/Clang.cpp

583

Done.

clang/include/clang/Basic/LangOptions.h

public:
   bool IsHeaderFile = false;
 
   /// The default stream kind used for HIP kernel launching.
   GPUDefaultStreamKind GPUDefaultStream;
 
   /// The seed used by the randomize structure layout feature.
   std::string RandstructSeed;
 
-  /// Indicates whether the __FILE__ macro should use the target's
-  /// platform-specific file separator or whether it should use the build
-  /// environment's platform-specific file separator.
+  /// Indicates whether to use target's platform-specific file separator when
+  /// __FILE__ macro is used and when concatenating filename with directory or
+  /// to use build environment environment's platform-specific file separator.
   ///
   /// The plaform-specific path separator is the backslash(\) for Windows and
   /// forward slash (/) elsewhere.
   bool UseTargetPathSeparator = false;
 
   LangOptions();
 
   /// Set language defaults for the given input language and

clang/lib/CodeGen/CGDebugInfo.cpp

void CGDebugInfo::CreateCompileUnit() {
   // mismatch with source locations and create extra lexical scopes or
   // mismatched debug info (a CU with a DW_AT_file of "-", because that's what
   // the driver passed, but functions/other things have DW_AT_file of "<stdin>"
   // because that's what the SourceManager says)
 
   // Get absolute path name.
   SourceManager &SM = CGM.getContext().getSourceManager();
   auto &CGO = CGM.getCodeGenOpts();
+  const LangOptions &LO = CGM.getLangOpts();
   std::string MainFileName = CGO.MainFileName;
   if (MainFileName.empty())
     MainFileName = "<stdin>";
 
   // The main file name provided via the "-main-file-name" option contains just
   // the file name itself with no path information. This file name may have had
   // a relative path, so we look into the actual file entry for the main
   // file to determine the real absolute path for the file.
   std::string MainFileDir;
   if (OptionalFileEntryRef MainFile =
           SM.getFileEntryRefForID(SM.getMainFileID())) {
     MainFileDir = std::string(MainFile->getDir().getName());
     if (!llvm::sys::path::is_absolute(MainFileName)) {
       llvm::SmallString<1024> MainFileDirSS(MainFileDir);
-      llvm::sys::path::append(MainFileDirSS, MainFileName);
-      MainFileName =
-          std::string(llvm::sys::path::remove_leading_dotslash(MainFileDirSS));
+      llvm::sys::path::Style Style =
+          LO.UseTargetPathSeparator
+              ? (CGM.getTarget().getTriple().isOSWindows()
+                     ? llvm::sys::path::Style::windows_backslash
+                     : llvm::sys::path::Style::posix)
+              : llvm::sys::path::Style::native;
+      llvm::sys::path::append(MainFileDirSS, Style, MainFileName);
+      MainFileName = std::string(
+          llvm::sys::path::remove_leading_dotslash(MainFileDirSS, Style));
     }
     // If the main file name provided is identical to the input file name, and
     // if the input file is a preprocessed source, use the module name for
     // debug info. The module name comes from the name specified in the first
     // linemarker if the input is a preprocessed source.
     if (MainFile->getName() == MainFileName &&
         FrontendOptions::getInputKindForExtension(
             MainFile->getName().rsplit('.').second)
             .isPreprocessed())
       MainFileName = CGM.getModule().getName().str();
 
     CSKind = computeChecksum(SM.getMainFileID(), Checksum);
   }
 
   llvm::dwarf::SourceLanguage LangTag;
-  const LangOptions &LO = CGM.getLangOpts();
   if (LO.CPlusPlus) {
     if (LO.ObjC)
       LangTag = llvm::dwarf::DW_LANG_ObjC_plus_plus;
     else if (CGO.DebugStrictDwarf && CGO.DwarfVersion < 5)
       LangTag = llvm::dwarf::DW_LANG_C_plus_plus;
     else if (LO.CPlusPlus14)
       LangTag = llvm::dwarf::DW_LANG_C_plus_plus_14;
     else if (LO.CPlusPlus11)

clang/lib/Driver/ToolChains/Clang.cpp

static void addDebugObjectName(const ArgList &Args, ArgStringList &CmdArgs,
   SmallString<128> ObjFileNameForDebug(OutputFileName);
   if (ObjFileNameForDebug != "-" &&
       !llvm::sys::path::is_absolute(ObjFileNameForDebug) &&
       (!DebugCompilationDir ||
        llvm::sys::path::is_absolute(DebugCompilationDir))) {
     // Make the path absolute in the debug infos like MSVC does.
     llvm::sys::fs::make_absolute(ObjFileNameForDebug);
   }
+  llvm::sys::path::Style Style =
+      llvm::sys::path::is_absolute(ObjFileNameForDebug)
+          ? llvm::sys::path::Style::native
+          : llvm::sys::path::Style::windows_backslash;
+  llvm::sys::path::remove_dots(ObjFileNameForDebug, /*remove_dot_dot=*/true,
+                               Style);
   CmdArgs.push_back(
       Args.MakeArgString(Twine("-object-file-name=") + ObjFileNameForDebug));
 }
 
 /// Add a CC1 and CC1AS option to specify the debug file path prefix map.
 static void addDebugPrefixMapArg(const Driver &D, const ToolChain &TC,
                                  const ArgList &Args, ArgStringList &CmdArgs) {
   auto AddOneArg = [&](StringRef Map, StringRef Name) {

clang/test/CodeGen/debug-info-slash.c

+// RUN: %clang -target x86_64-pc-win32  -ffile-reproducible -emit-llvm -S -g %s -o - | FileCheck --check-prefix=WIN %s
+// RUN: %clang -target x86_64-linux-gnu  -ffile-reproducible -emit-llvm -S -g %s -o - | FileCheck --check-prefix=LINUX %s
+int main() { return 0; }
+
+// WIN:   !DIFile(filename: "{{.*}}\\debug-info-slash.c"
+// LINUX: !DIFile(filename: "{{.*}}/debug-info-slash.c"

clang/test/Driver/cl-outputs.c

 
 // RUN: %clang_cl /P /obar.x /Fifoo.x -### -- %s 2>&1 | FileCheck -check-prefix=FioRACE1 %s
 // FioRACE1: "-E"
 // FioRACE1: "-o" "foo.x"
 
 // RUN: %clang_cl /P /Fifoo.x /obar.x -### -- %s 2>&1 | FileCheck -check-prefix=FioRACE2 %s
 // FioRACE2: "-E"
 // FioRACE2: "-o" "foo.x"
+
+// RUN: %clang_cl /Z7 /Foa.obj -### -- %s 2>&1 | FileCheck -check-prefix=ABSOLUTE_OBJPATH %s
+// ABSOLUTE_OBJPATH: "-object-file-name={{.*}}a.obj"
+
+// RUN: %clang_cl -fdebug-compilation-dir=. /Z7 /Foa.obj -### -- %s 2>&1 | FileCheck -check-prefix=RELATIVE_OBJPATH1 %s
+// RELATIVE_OBJPATH1: "-object-file-name=a.obj"
+
+// RUN: %clang_cl -fdebug-compilation-dir=. /Z7 /Fofoo/a.obj -### -- %s 2>&1 | FileCheck -check-prefix=RELATIVE_OBJPATH2 %s
+// RELATIVE_OBJPATH2: "-object-file-name=foo\\a.obj"

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

StringRef S(reinterpret_cast<const char *>(GHR.Hash.data()),
                 GHR.Hash.size());
     OS.emitBinaryData(S);
   }
 }
 
 void CodeViewDebug::emitObjName() {
   MCSymbol *CompilerEnd = beginSymbolRecord(SymbolKind::S_OBJNAME);
 
   StringRef PathRef(Asm->TM.Options.ObjectFilenameForDebug);
   llvm::SmallString<256> PathStore(PathRef);
 
   if (PathRef.empty() || PathRef == "-") {
     // Don't emit the filename if we're writing to stdout or to /dev/null.
     PathRef = {};
   } else {
-    llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true);
     PathRef = PathStore;
   }
 
   OS.AddComment("Signature");
   OS.emitIntValue(0, 4);
 
   OS.AddComment("Object name");
   emitNullTerminatedSymbolName(OS, PathRef);