[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D43002#3205190 , @MaskRay wrote:

> Some test nits:) I don't know CodeView :(

Fixed, thanks @MaskRay !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Some test nits:) I don't know CodeView :(




Comment at: clang/test/CodeGenCXX/debug-info-objname.cpp:1
+// RUN: cp %s %T/debug-info-objname.cpp
+// RUN: cd %T

The whole file appears to be CRLF where almost all other tests use LF in 
llvm-project.



Comment at: clang/test/CodeGenCXX/debug-info-objname.cpp:2
+// RUN: cp %s %T/debug-info-objname.cpp
+// RUN: cd %T
+

%T is deprecated in lit 
(https://llvm.org/docs/CommandGuide/lit.html#substitutions): it is easy to 
cause multi-threading file reuse problems because lit runs tests parallelly.

I personally prefer `RUN: rm -rf %t && mkdir %t && cd %t` if you need to cd.

My personal comment style for non-RUN non-CHECK lines is `///` - it stands out 
in many editors and may help possible FileCheck/lit's future direction to catch 
stale CHECK prefixes.



Comment at: clang/test/CodeGenCXX/debug-info-objname.cpp:30
+// RUN: %clang_cl /c /Z7 -nostdinc -fdebug-compilation-dir=. 
/clang:-save-temps debug-info-objname.cpp
+// RUN: cat debug-info-objname.asm | FileCheck %s --check-prefix=ASM
+

Prefer `--input-file` or input redirection to `cat xxx | yyy`



Comment at: clang/test/CodeGenCXX/debug-info-objname.cpp:36
+
+// ABSOLUTE: S_OBJNAME [size = {{[0-9]+}}] sig=0, 
`{{.+}}debug-info-objname.obj`
+// RELATIVE: S_OBJNAME [size = {{[0-9]+}}] sig=0, `debug-info-objname.obj`

`{{[0-9]+}}` -> `[[#]]`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Alexandre Ganea via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf44e3fbadd15: [CodeView] Emit S_OBJNAME record (authored by 
aganea).

Changed prior to commit:
  https://reviews.llvm.org/D43002?vs=374884=395663#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Job.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CMakeLists.txt
  clang/test/CodeGenCXX/debug-info-objname.cpp
  llvm/include/llvm/MC/MCTargetOptions.h
  llvm/include/llvm/Support/Caching.h
  llvm/include/llvm/Support/ToolOutputFile.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Support/Caching.cpp
  llvm/test/DebugInfo/COFF/globals.ll
  llvm/test/DebugInfo/COFF/multifunction.ll
  llvm/test/DebugInfo/COFF/pr28747.ll
  llvm/test/DebugInfo/COFF/simple.ll
  llvm/test/DebugInfo/COFF/vframe-fpo.ll
  llvm/test/MC/AArch64/coff-debug.ll
  llvm/test/MC/ARM/coff-debugging-secrel.ll
  llvm/test/MC/COFF/cv-compiler-info.ll
  llvm/tools/llc/llc.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp

Index: llvm/tools/llvm-lto2/llvm-lto2.cpp
===
--- llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -378,7 +378,7 @@
 std::error_code EC;
 auto S = std::make_unique(Path, EC, sys::fs::OF_None);
 check(EC, Path);
-return std::make_unique(std::move(S));
+return std::make_unique(std::move(S), Path);
   };
 
   auto AddBuffer = [&](size_t Task, std::unique_ptr MB) {
Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -605,6 +605,9 @@
   GetOutputStream(TheTarget->getName(), TheTriple.getOS(), argv[0]);
   if (!Out) return 1;
 
+  // Ensure the filename is passed down to CodeViewDebug.
+  Target->Options.ObjectFilenameForDebug = Out->outputFilename();
+
   std::unique_ptr DwoOut;
   if (!SplitDwarfOutputFile.empty()) {
 std::error_code EC;
Index: llvm/test/MC/COFF/cv-compiler-info.ll
===
--- llvm/test/MC/COFF/cv-compiler-info.ll
+++ llvm/test/MC/COFF/cv-compiler-info.ll
@@ -1,4 +1,6 @@
-; RUN: llc -mtriple i686-pc-windows-msvc < %s | FileCheck %s
+; RUN: llc -mtriple i686-pc-windows-msvc < %s | FileCheck %s --check-prefixes=CHECK,STDOUT
+; RUN: llc -mtriple i686-pc-windows-msvc < %s -o %t
+; RUN: FileCheck %s --input-file=%t --check-prefixes=CHECK,FILE
 ; ModuleID = 'D:\src\scopes\foo.cpp'
 source_filename = "D:\5Csrc\5Cscopes\5Cfoo.cpp"
 target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
@@ -20,19 +22,23 @@
 ; One .debug$S section should contain an S_COMPILE3 record that identifies the
 ; source language and the version of the compiler based on the DICompileUnit.
 ; CHECK: 	.section	.debug$S,"dr"
+; CHECK:.short  4353# Record kind: S_OBJNAME
+; CHECK-NEXT:   .long   0   # Signature
+; STDOUT-NEXT:  .byte   0   # Object name
+; FILE-NEXT:.asciz	"{{.*}}{{|/}}cv-compiler-info.ll.tmp" # Object name
 ; CHECK: 		.short	4412  # Record kind: S_COMPILE3
-; CHECK: 		.long	1   # Flags and language
-; CHECK: 		.short	7 # CPUType
-; CHECK: 		.short	4 # Frontend version
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	[[BACKEND_VERSION:[0-9]+]]  # Backend version
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.asciz	"clang version 4.0.0 "  # Null-terminated compiler version string
-; CHECK-NOT: .short	4412  # Record kind: S_COMPILE3
+; CHECK-NEXT:   .long	1   # Flags and language
+; CHECK-NEXT: 	.short	7 # CPUType
+; CHECK-NEXT: 	.short	4 # Frontend version
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	[[BACKEND_VERSION:[0-9]+]]  # Backend version
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.asciz	"clang version 4.0.0 "  # Null-terminated compiler version string
+; CHECK-NOT:.short	4412  # Record kind: S_COMPILE3
 !1 = !DIFile(filename: "D:\5Csrc\5Cscopes\5Cfoo.cpp", directory: "D:\5Csrc\5Cscopes\5Cclang")
 !2 = !{}
 !7 = !{i32 2, !"CodeView", i32 1}
Index: llvm/test/MC/ARM/coff-debugging-secrel.ll
===
--- llvm/test/MC/ARM/coff-debugging-secrel.ll
+++ 

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm

Sorry for the delay, I got sick last week and read through email newest to 
oldest.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea removed a reviewer: deadalnix.
aganea added a comment.

Ping! Any further comments? Thanks in advance for your time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:752
 
+static void unescapeSlashes(SmallVectorImpl ) {
+  auto Read = Str.begin();

rnk wrote:
> aganea wrote:
> > rnk wrote:
> > > This isn't unescaping them, it's just canonicalizing double slashes to 
> > > one slash, right? Would `llvm::sys::native` suffice?
> > `llvm::sys::path::native()` doesn't remove consecutive (back)slashes. I 
> > think @zturner's main intent was when debugging in Visual Studio with 
> > arguments from LIT tests, they sometimes contain double backslashes.
> I see. I think "unescape" shouldn't be in the name, this isn't about escape 
> characters, it's about cleaning up or canonicalizing the path.
I removed this function. Now using `sys::path::remove_dots` which removes the 
consecutive slashes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 374884.
aganea marked 5 inline comments as done.
aganea edited reviewers, added: akhuang; removed: zturner, gratianlup.
aganea edited subscribers, added: zturner, gratianlup; removed: aganea.
aganea added a comment.
Herald added subscribers: dang, mgorny.

As suggested by @rnk


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Job.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CMakeLists.txt
  clang/test/CodeGenCXX/debug-info-objname.cpp
  llvm/include/llvm/LTO/LTO.h
  llvm/include/llvm/Support/ToolOutputFile.h
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
  llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
  llvm/lib/LTO/Caching.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/test/DebugInfo/COFF/globals.ll
  llvm/test/DebugInfo/COFF/multifunction.ll
  llvm/test/DebugInfo/COFF/pr28747.ll
  llvm/test/DebugInfo/COFF/simple.ll
  llvm/test/DebugInfo/COFF/vframe-fpo.ll
  llvm/test/MC/AArch64/coff-debug.ll
  llvm/test/MC/ARM/coff-debugging-secrel.ll
  llvm/test/MC/COFF/cv-compiler-info.ll
  llvm/tools/llc/llc.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp

Index: llvm/tools/llvm-lto2/llvm-lto2.cpp
===
--- llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ llvm/tools/llvm-lto2/llvm-lto2.cpp
@@ -369,7 +369,7 @@
 std::error_code EC;
 auto S = std::make_unique(Path, EC, sys::fs::OF_None);
 check(EC, Path);
-return std::make_unique(std::move(S));
+return std::make_unique(std::move(S), Path);
   };
 
   auto AddBuffer = [&](size_t Task, std::unique_ptr MB) {
Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -576,6 +576,9 @@
   GetOutputStream(TheTarget->getName(), TheTriple.getOS(), argv[0]);
   if (!Out) return 1;
 
+  // Ensure the filename is passed down to CodeViewDebug.
+  Target->Options.ObjectFilenameForDebug = Out->outputFilename();
+
   std::unique_ptr DwoOut;
   if (!SplitDwarfOutputFile.empty()) {
 std::error_code EC;
Index: llvm/test/MC/COFF/cv-compiler-info.ll
===
--- llvm/test/MC/COFF/cv-compiler-info.ll
+++ llvm/test/MC/COFF/cv-compiler-info.ll
@@ -1,4 +1,6 @@
-; RUN: llc -mtriple i686-pc-windows-msvc < %s | FileCheck %s
+; RUN: llc -mtriple i686-pc-windows-msvc < %s | FileCheck %s --check-prefixes=CHECK,STDOUT
+; RUN: llc -mtriple i686-pc-windows-msvc < %s -o %t
+; RUN: FileCheck %s --input-file=%t --check-prefixes=CHECK,FILE
 ; ModuleID = 'D:\src\scopes\foo.cpp'
 source_filename = "D:\5Csrc\5Cscopes\5Cfoo.cpp"
 target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
@@ -20,19 +22,23 @@
 ; One .debug$S section should contain an S_COMPILE3 record that identifies the
 ; source language and the version of the compiler based on the DICompileUnit.
 ; CHECK: 	.section	.debug$S,"dr"
+; CHECK:.short  4353# Record kind: S_OBJNAME
+; CHECK-NEXT:   .long   0   # Signature
+; STDOUT-NEXT:  .byte   0   # Object name
+; FILE-NEXT:.asciz	"{{.*}}{{|/}}cv-compiler-info.ll.tmp" # Object name
 ; CHECK: 		.short	4412  # Record kind: S_COMPILE3
-; CHECK: 		.long	1   # Flags and language
-; CHECK: 		.short	7 # CPUType
-; CHECK: 		.short	4 # Frontend version
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	[[BACKEND_VERSION:[0-9]+]]  # Backend version
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.short	0
-; CHECK: 		.asciz	"clang version 4.0.0 "  # Null-terminated compiler version string
-; CHECK-NOT: .short	4412  # Record kind: S_COMPILE3
+; CHECK-NEXT:   .long	1   # Flags and language
+; CHECK-NEXT: 	.short	7 # CPUType
+; CHECK-NEXT: 	.short	4 # Frontend version
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	[[BACKEND_VERSION:[0-9]+]]  # Backend version
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.short	0
+; CHECK-NEXT: 	.asciz	"clang version 4.0.0 "  # Null-terminated compiler version string
+; CHECK-NOT:.short	4412  # Record kind: S_COMPILE3
 !1 = !DIFile(filename: "D:\5Csrc\5Cscopes\5Cfoo.cpp", directory: "D:\5Csrc\5Cscopes\5Cclang")
 !2 = !{}
 !7 = !{i32 2, !"CodeView", i32 1}
Index: llvm/test/MC/ARM/coff-debugging-secrel.ll
===
--- llvm/test/MC/ARM/coff-debugging-secrel.ll
+++ llvm/test/MC/ARM/coff-debugging-secrel.ll
@@ -42,10 

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Daniel Paoliello via Phabricator via cfe-commits]

dpaoliello added a comment.
Herald added a subscriber: ormris.

@rnk @aganea Ping... Just wanted to see what was happening with this change, 
and if there is anything I can do to help get it merged in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:212
+  /// Output filename used in the COFF debug information.
+  std::string COFFOutputFilename;
+

aganea wrote:
> rnk wrote:
> > Let's bikeshed the name a bit. This thing is the `-o`/`/Fo` value, plus 
> > some processing. It can be symbolic, as in `-`. It could be the name of a 
> > bitcode file with `-flto`. It could be the name of an assembly file if you 
> > do `clang -S --target=x86_64-windows-msvc -g t.cpp -o t.s`. It could be the 
> > name of an ELF file if you try hard. I think in any of these cases where 
> > the user directly emits something that isn't a COFF object, it's OK to use 
> > that name for the S_OBJNAME record.
> > 
> > What it is, is the name of the compiler's output file, as we would like it 
> > to appear in debug info. With that in mind, here are some ideas:
> > - CodeViewObjectName: very directly referring to S_OBJNAME
> > - ObjectFilename: very general
> > - ObjectFilenameForDebug: generalizing over cv/dwarf
> > - OutputFilenameForDebug: generalizing over assembly, bc, and obj
> > 
> > I think I like ObjectFilenameForDebug best. DebugObjectFilename seems like 
> > a no-go, since that sounds like the dwo file name.
> > 
> > ---
> > 
> > This brings me to the case of `-save-temps` / `/Fa`. In these cases, the 
> > compile action is broken into pieces, and the assembler is invoked in a 
> > follow-up process. Does that mean the driver needs to pass an extra flag 
> > along to the -cc1 action? That would be a bummer.
> > 
> I think we should fix `-save-temps` and `/Fa`, thanks for raising that! Would 
> you mind if I added a new cc1 flag for that purpose? Something like 
> `-target-file-name`.
> When using `-save-temps`, each cc1 command doesn't know about the final 
> filename, except for the last cc1 command. We would need to provide that name 
> somehow when passing `-S`.
Yes, I think we'll need a new cc1 flag. I'd avoid "target" in the flag name, 
since it makes me thing of target as in platform, OS, ISA, etc. Maybe just 
-object-file-name? It's very boring, and reasonably specific.



Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:752
 
+static void unescapeSlashes(SmallVectorImpl ) {
+  auto Read = Str.begin();

aganea wrote:
> rnk wrote:
> > This isn't unescaping them, it's just canonicalizing double slashes to one 
> > slash, right? Would `llvm::sys::native` suffice?
> `llvm::sys::path::native()` doesn't remove consecutive (back)slashes. I think 
> @zturner's main intent was when debugging in Visual Studio with arguments 
> from LIT tests, they sometimes contain double backslashes.
I see. I think "unescape" shouldn't be in the name, this isn't about escape 
characters, it's about cleaning up or canonicalizing the path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Alexandre Ganea via Phabricator via cfe-commits]

aganea marked 8 inline comments as done.
aganea added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:212
+  /// Output filename used in the COFF debug information.
+  std::string COFFOutputFilename;
+

rnk wrote:
> Let's bikeshed the name a bit. This thing is the `-o`/`/Fo` value, plus some 
> processing. It can be symbolic, as in `-`. It could be the name of a bitcode 
> file with `-flto`. It could be the name of an assembly file if you do `clang 
> -S --target=x86_64-windows-msvc -g t.cpp -o t.s`. It could be the name of an 
> ELF file if you try hard. I think in any of these cases where the user 
> directly emits something that isn't a COFF object, it's OK to use that name 
> for the S_OBJNAME record.
> 
> What it is, is the name of the compiler's output file, as we would like it to 
> appear in debug info. With that in mind, here are some ideas:
> - CodeViewObjectName: very directly referring to S_OBJNAME
> - ObjectFilename: very general
> - ObjectFilenameForDebug: generalizing over cv/dwarf
> - OutputFilenameForDebug: generalizing over assembly, bc, and obj
> 
> I think I like ObjectFilenameForDebug best. DebugObjectFilename seems like a 
> no-go, since that sounds like the dwo file name.
> 
> ---
> 
> This brings me to the case of `-save-temps` / `/Fa`. In these cases, the 
> compile action is broken into pieces, and the assembler is invoked in a 
> follow-up process. Does that mean the driver needs to pass an extra flag 
> along to the -cc1 action? That would be a bummer.
> 
I think we should fix `-save-temps` and `/Fa`, thanks for raising that! Would 
you mind if I added a new cc1 flag for that purpose? Something like 
`-target-file-name`.
When using `-save-temps`, each cc1 command doesn't know about the final 
filename, except for the last cc1 command. We would need to provide that name 
somehow when passing `-S`.



Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:752
 
+static void unescapeSlashes(SmallVectorImpl ) {
+  auto Read = Str.begin();

rnk wrote:
> This isn't unescaping them, it's just canonicalizing double slashes to one 
> slash, right? Would `llvm::sys::native` suffice?
`llvm::sys::path::native()` doesn't remove consecutive (back)slashes. I think 
@zturner's main intent was when debugging in Visual Studio with arguments from 
LIT tests, they sometimes contain double backslashes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I think the biggest concern here is what to do about `/Fa` / `-save-temps`. If 
we do nothing, the S_OBJNAME record will change if you run the same compilation 
twice with and without those flags. Generally, we strive to ensure that the 
directly emitted object file is identical to one rinsed through textual 
assembly, but that doesn't always work. I'm not sure if `/Fa` works reliably 
since we switched to Intel style assembly syntax either.




Comment at: clang/include/clang/Basic/CodeGenOptions.h:212
+  /// Output filename used in the COFF debug information.
+  std::string COFFOutputFilename;
+

Let's bikeshed the name a bit. This thing is the `-o`/`/Fo` value, plus some 
processing. It can be symbolic, as in `-`. It could be the name of a bitcode 
file with `-flto`. It could be the name of an assembly file if you do `clang -S 
--target=x86_64-windows-msvc -g t.cpp -o t.s`. It could be the name of an ELF 
file if you try hard. I think in any of these cases where the user directly 
emits something that isn't a COFF object, it's OK to use that name for the 
S_OBJNAME record.

What it is, is the name of the compiler's output file, as we would like it to 
appear in debug info. With that in mind, here are some ideas:
- CodeViewObjectName: very directly referring to S_OBJNAME
- ObjectFilename: very general
- ObjectFilenameForDebug: generalizing over cv/dwarf
- OutputFilenameForDebug: generalizing over assembly, bc, and obj

I think I like ObjectFilenameForDebug best. DebugObjectFilename seems like a 
no-go, since that sounds like the dwo file name.

---

This brings me to the case of `-save-temps` / `/Fa`. In these cases, the 
compile action is broken into pieces, and the assembler is invoked in a 
follow-up process. Does that mean the driver needs to pass an extra flag along 
to the -cc1 action? That would be a bummer.




Comment at: clang/lib/Frontend/CompilerInstance.cpp:717
+
+  getCodeGenOpts().COFFOutputFilename = OutputPathName;
+

I think saving the output file name during output file creation seems like an 
unexpected side effect for this function. Can we handle it earlier, maybe near 
dwo file name handling?



Comment at: clang/test/CodeGenCXX/debug-info-objname.cpp:4
+
+// No output file provided, input file is relative, we emit an absolute path 
(MSVC behavior).
+// RUN: %clang_cl /c /Z7 -nostdinc debug-info-objname.cpp

I checked, this is also consistent with clang's regular /Z7 output for source 
filenames.



Comment at: clang/test/CodeGenCXX/debug-info-objname.cpp:20-21
+
+// The input file name is relative and we specify -fdebug-compilation-dir, we 
emit a relative path.
+// RUN: %clang_cl /c /Z7 -nostdinc -fdebug-compilation-dir=. 
debug-info-objname.cpp
+// RUN: llvm-pdbutil dump -all debug-info-objname.obj | FileCheck %s 
--check-prefix=RELATIVE

And this is the thing we rely on, so it should all work.



Comment at: llvm/include/llvm/MC/MCTargetOptions.h:63
   std::string SplitDwarfFile;
+  std::string COFFOutputFilename;
 

This should probably be in llvm/include/llvm/Target/TargetOptions.h. I think 
MCTargetOptions is intended to control assembler behavior, but this option is 
read while producing assembly in CodeGen, not during assembling.



Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:752
 
+static void unescapeSlashes(SmallVectorImpl ) {
+  auto Read = Str.begin();

This isn't unescaping them, it's just canonicalizing double slashes to one 
slash, right? Would `llvm::sys::native` suffice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D43002

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits