[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-21 Thread Sven van Haastregt via Phabricator via cfe-commits 
svenvh added a comment.

As a followup to r363496, I've added llvm-dwarfdump as a clang test dependency 
in r364021.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-15 Thread Aaron Puchert via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
aaronpuchert marked 3 inline comments as done.
Closed by commit rL363496: [Clang] Harmonize Split DWARF options with llc 
(authored by aaronpuchert, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59673?vs=204661=204927#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59673

Files:
  cfe/trunk/include/clang/Basic/CodeGenOptions.h
  cfe/trunk/include/clang/Driver/CC1Options.td
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/test/CodeGen/split-debug-filename.c
  cfe/trunk/test/CodeGen/split-debug-output.c
  cfe/trunk/test/CodeGen/split-debug-single-file.c
  cfe/trunk/test/CodeGen/thinlto-split-dwarf.c
  cfe/trunk/test/Driver/split-debug.c
  llvm/trunk/include/llvm/LTO/Config.h
  llvm/trunk/lib/LTO/LTOBackend.cpp

Index: llvm/trunk/lib/LTO/LTOBackend.cpp
===
--- llvm/trunk/lib/LTO/LTOBackend.cpp
+++ llvm/trunk/lib/LTO/LTOBackend.cpp
@@ -314,7 +314,7 @@
 return;
 
   std::unique_ptr DwoOut;
-  SmallString<1024> DwoFile(Conf.DwoPath);
+  SmallString<1024> DwoFile(Conf.SplitDwarfOutput);
   if (!Conf.DwoDir.empty()) {
 std::error_code EC;
 if (auto EC = llvm::sys::fs::create_directories(Conf.DwoDir))
@@ -323,11 +323,12 @@
 
 DwoFile = Conf.DwoDir;
 sys::path::append(DwoFile, std::to_string(Task) + ".dwo");
-  }
+TM->Options.MCOptions.SplitDwarfFile = DwoFile.str().str();
+  } else
+TM->Options.MCOptions.SplitDwarfFile = Conf.SplitDwarfFile;
 
   if (!DwoFile.empty()) {
 std::error_code EC;
-TM->Options.MCOptions.SplitDwarfFile = DwoFile.str().str();
 DwoOut = llvm::make_unique(DwoFile, EC, sys::fs::F_None);
 if (EC)
   report_fatal_error("Failed to open " + DwoFile + ": " + EC.message());
Index: llvm/trunk/include/llvm/LTO/Config.h
===
--- llvm/trunk/include/llvm/LTO/Config.h
+++ llvm/trunk/include/llvm/LTO/Config.h
@@ -88,10 +88,16 @@
   /// The directory to store .dwo files.
   std::string DwoDir;
 
+  /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
+  /// attribute in the skeleton CU. This should generally only be used when
+  /// running an individual backend directly via thinBackend(), as otherwise
+  /// all objects would use the same .dwo file. Not used as output path.
+  std::string SplitDwarfFile;
+
   /// The path to write a .dwo file to. This should generally only be used when
   /// running an individual backend directly via thinBackend(), as otherwise
-  /// all .dwo files will be written to the same path.
-  std::string DwoPath;
+  /// all .dwo files will be written to the same path. Not used in skeleton CU.
+  std::string SplitDwarfOutput;
 
   /// Optimization remarks file path.
   std::string RemarksFilename = "";
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -472,7 +472,7 @@
   Options.EmitAddrsig = CodeGenOpts.Addrsig;
 
   if (CodeGenOpts.getSplitDwarfMode() != CodeGenOptions::NoFission)
-Options.MCOptions.SplitDwarfFile = CodeGenOpts.SplitDwarfOutput;
+Options.MCOptions.SplitDwarfFile = CodeGenOpts.SplitDwarfFile;
   Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
   Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
   Options.MCOptions.MCUseDwarfDirectory = !CodeGenOpts.NoDwarfDirectoryAsm;
@@ -1428,7 +1428,8 @@
   Conf.RemarksWithHotness = CGOpts.DiagnosticsWithHotness;
   Conf.RemarksFilename = CGOpts.OptRecordFile;
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfOutput;
+  Conf.SplitDwarfFile = CGOpts.SplitDwarfFile;
+  Conf.SplitDwarfOutput = CGOpts.SplitDwarfOutput;
   switch (Action) {
   case Backend_EmitNothing:
 Conf.PreCodeGenModuleHook = [](size_t Task, const Module ) {
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -616,7 +616,7 @@
   CGOpts.DwarfDebugFlags, RuntimeVers,
   (CGOpts.getSplitDwarfMode() != CodeGenOptions::NoFission)
   ? ""
-  : CGOpts.SplitDwarfOutput,
+  : CGOpts.SplitDwarfFile,
   EmissionKind, DwoId, CGOpts.SplitDwarfInlining,
   CGOpts.DebugInfoForProfiling,
   CGM.getTarget().getTriple().isNVPTX()
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -4108,11 +4108,14 @@

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-14 Thread Teresa Johnson via Phabricator via cfe-commits 
tejohnson added inline comments.



Comment at: llvm/include/llvm/LTO/Config.h:92
+  /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
+  /// attribute in the skeleton CU.
+  std::string SplitDwarfFile;

aaronpuchert wrote:
> tejohnson wrote:
> > Probably needs a comment similar to the one below about being for running 
> > individual backends directly. Otherwise (for in process ThinLTO) we use the 
> > DwoDir.
> Sure. Any objections to the following wording?
> 
> ```
>   /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
>   /// attribute in the skeleton CU. This should generally only be used when
>   /// running an individual backend directly via thinBackend(), as otherwise
>   /// all objects would use the same .dwo file. Not used as output path.
> ```
Looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-14 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added inline comments.



Comment at: llvm/include/llvm/LTO/Config.h:92
+  /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
+  /// attribute in the skeleton CU.
+  std::string SplitDwarfFile;

tejohnson wrote:
> Probably needs a comment similar to the one below about being for running 
> individual backends directly. Otherwise (for in process ThinLTO) we use the 
> DwoDir.
Sure. Any objections to the following wording?

```
  /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
  /// attribute in the skeleton CU. This should generally only be used when
  /// running an individual backend directly via thinBackend(), as otherwise
  /// all objects would use the same .dwo file. Not used as output path.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-14 Thread Teresa Johnson via Phabricator via cfe-commits 
tejohnson added a comment.

lgtm for the LTO bits. Suggestion below for comment.




Comment at: llvm/include/llvm/LTO/Config.h:92
+  /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
+  /// attribute in the skeleton CU.
+  std::string SplitDwarfFile;

Probably needs a comment similar to the one below about being for running 
individual backends directly. Otherwise (for in process ThinLTO) we use the 
DwoDir.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-13 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added a reviewer: tejohnson.
aaronpuchert added a comment.

Sorry for making the inline comments disappear, but I had to switch to the big 
repository.

@tejohnson Could you have a look at the LTO-related changes (BackendUtil.cpp + 
all files with LTO in the name)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-13 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert updated this revision to Diff 204661.
aaronpuchert added a comment.
Herald added subscribers: llvm-commits, dang, dexonsmith, steven_wu, hiraditya, 
mehdi_amini.
Herald added a project: LLVM.

Make sure the flags have the same meaning for LTO. Also slightly reworded the 
HelpText.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59673

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/split-debug-filename.c
  clang/test/CodeGen/split-debug-output.c
  clang/test/CodeGen/split-debug-single-file.c
  clang/test/CodeGen/thinlto-split-dwarf.c
  clang/test/Driver/split-debug.c
  llvm/include/llvm/LTO/Config.h
  llvm/lib/LTO/LTOBackend.cpp

Index: llvm/lib/LTO/LTOBackend.cpp
===
--- llvm/lib/LTO/LTOBackend.cpp
+++ llvm/lib/LTO/LTOBackend.cpp
@@ -313,7 +313,7 @@
 return;
 
   std::unique_ptr DwoOut;
-  SmallString<1024> DwoFile(Conf.DwoPath);
+  SmallString<1024> DwoFile(Conf.SplitDwarfOutput);
   if (!Conf.DwoDir.empty()) {
 std::error_code EC;
 if (auto EC = llvm::sys::fs::create_directories(Conf.DwoDir))
@@ -322,11 +322,12 @@
 
 DwoFile = Conf.DwoDir;
 sys::path::append(DwoFile, std::to_string(Task) + ".dwo");
-  }
+TM->Options.MCOptions.SplitDwarfFile = DwoFile.str().str();
+  } else
+TM->Options.MCOptions.SplitDwarfFile = Conf.SplitDwarfFile;
 
   if (!DwoFile.empty()) {
 std::error_code EC;
-TM->Options.MCOptions.SplitDwarfFile = DwoFile.str().str();
 DwoOut = llvm::make_unique(DwoFile, EC, sys::fs::F_None);
 if (EC)
   report_fatal_error("Failed to open " + DwoFile + ": " + EC.message());
Index: llvm/include/llvm/LTO/Config.h
===
--- llvm/include/llvm/LTO/Config.h
+++ llvm/include/llvm/LTO/Config.h
@@ -88,10 +88,14 @@
   /// The directory to store .dwo files.
   std::string DwoDir;
 
+  /// The name for the split debug info file used for the DW_AT_[GNU_]dwo_name
+  /// attribute in the skeleton CU.
+  std::string SplitDwarfFile;
+
   /// The path to write a .dwo file to. This should generally only be used when
   /// running an individual backend directly via thinBackend(), as otherwise
-  /// all .dwo files will be written to the same path.
-  std::string DwoPath;
+  /// all .dwo files will be written to the same path. Not used in skeleton CU.
+  std::string SplitDwarfOutput;
 
   /// Optimization remarks file path.
   std::string RemarksFilename = "";
Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -3,7 +3,7 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
 //
-// CHECK-ACTIONS: "-split-dwarf-output" "split-debug.dwo"
+// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
@@ -14,12 +14,14 @@
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS-SINGLE-SPLIT < %t %s
 //
 // CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf=single"
-// CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-output" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-file" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### -o %tfoo.o %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SINGLE-SPLIT-FILENAME < %t %s
 //
-// CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-output" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
@@ -41,7 +43,7 @@
 // RUN: %clang -target amdgcn-amd-amdhsa -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-OPTION < %t %s
 //
-// CHECK-OPTION: "-split-dwarf-output" "split-debug.dwo"
+// CHECK-OPTION: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ASM < %t %s
@@ -58,6 +60,7 @@
 //
 // CHECK-GMLT-WITH-SPLIT: "-enable-split-dwarf"
 // CHECK-GMLT-WITH-SPLIT: "-debug-info-kind=line-tables-only"
+// CHECK-GMLT-WITH-SPLIT: "-split-dwarf-file"
 // CHECK-GMLT-WITH-SPLIT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -g

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-12 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

tejohnson wrote:
> aaronpuchert wrote:
> > tejohnson wrote:
> > > aaronpuchert wrote:
> > > > aaronpuchert wrote:
> > > > > @pcc Your documentation for `DwoPath` suggests that this should be 
> > > > > the actual output filename. However, the test that you added together 
> > > > > with this line in rC333677 doesn't fail whatever garbage I write into 
> > > > > that field here. What can I add to that so that it fails when we 
> > > > > don't do the right thing here?
> > > > @pcc Could you (or perhaps @tejohnson) comment on what the intended 
> > > > behavior is here, and how I can change the test so that it fails when I 
> > > > do the wrong thing? Is this the name of the file we write the split 
> > > > debug info to, or is it the value we use for the DW_AT_[GNU_]dwo_name 
> > > > attribute in the skeleton CU?
> > > It is the name of the file the split debug info is written to. If you 
> > > test by changing the file name given to the -split-dwarf-file option in 
> > > test/CodeGen/thinlto-split-dwarf.c, make sure you clean up the old one in 
> > > your test output directory. When I tested it just now, it initially still 
> > > passed because I had an old one sitting in the output directory from a 
> > > prior run. Once I removed that it failed with the new name (without 
> > > changing the corresponding llvm-readobj invocation).
> > Thanks, I didn't consider that. I wasn't even aware that test output is 
> > persisted.
> > 
> > It seems `DwoPath` is used both as output filename and as value for 
> > `DW_AT_[GNU_]dwo_name` in the skeleton CU. `llc` has separate options for 
> > both: `-split-dwarf-file` for the attribute and `-split-dwarf-output` for 
> > the output filename. I want to do the same for Clang. Then we should 
> > probably separate them for LTO, too.
> > 
> > What is the use case for `-split-dwarf-file` with an individual thin 
> > backend? Is that used for distributed/remote builds? Also is there any 
> > non-cc1 way to use it? There is `--plugin-opt=dwo_dir=...` for the LTO 
> > linker plugin, but that seems to go a different route.
> This is the path taken for distributed ThinLTO, the plugin-opt passed to 
> linker are for in-process ThinLTO. While -split-dwarf-file is a cc1 option, 
> it is normally set automatically from the filename under -gsplit-dwarf (see 
> where it is set in Clang::ConstructJob).
Ok, then it makes sense to do the changes for LTO as well.

You're right that `-split-dwarf-file` is produced for `-gsplit-dwarf`, but not 
with LTO, right? When I set `-gsplit-dwarf` on a vanilla ThinLTO build, then 
the flag is just ignored and I'm getting the debug info in the final 
executables/shared objects. (At least with Clang 8.)

I'm asking because this changes the cc1-interface, and while I have adapted 
`Clang::ConstructJob`, I'm not sure how distributed ThinLTO works, and whether 
there have to be changes.

If it works like test/CodeGen/thinlto-split-dwarf.c, using cc1-options, then 
users might have to adapt after this change.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-12 Thread Teresa Johnson via Phabricator via cfe-commits 
tejohnson added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

aaronpuchert wrote:
> tejohnson wrote:
> > aaronpuchert wrote:
> > > aaronpuchert wrote:
> > > > @pcc Your documentation for `DwoPath` suggests that this should be the 
> > > > actual output filename. However, the test that you added together with 
> > > > this line in rC333677 doesn't fail whatever garbage I write into that 
> > > > field here. What can I add to that so that it fails when we don't do 
> > > > the right thing here?
> > > @pcc Could you (or perhaps @tejohnson) comment on what the intended 
> > > behavior is here, and how I can change the test so that it fails when I 
> > > do the wrong thing? Is this the name of the file we write the split debug 
> > > info to, or is it the value we use for the DW_AT_[GNU_]dwo_name attribute 
> > > in the skeleton CU?
> > It is the name of the file the split debug info is written to. If you test 
> > by changing the file name given to the -split-dwarf-file option in 
> > test/CodeGen/thinlto-split-dwarf.c, make sure you clean up the old one in 
> > your test output directory. When I tested it just now, it initially still 
> > passed because I had an old one sitting in the output directory from a 
> > prior run. Once I removed that it failed with the new name (without 
> > changing the corresponding llvm-readobj invocation).
> Thanks, I didn't consider that. I wasn't even aware that test output is 
> persisted.
> 
> It seems `DwoPath` is used both as output filename and as value for 
> `DW_AT_[GNU_]dwo_name` in the skeleton CU. `llc` has separate options for 
> both: `-split-dwarf-file` for the attribute and `-split-dwarf-output` for the 
> output filename. I want to do the same for Clang. Then we should probably 
> separate them for LTO, too.
> 
> What is the use case for `-split-dwarf-file` with an individual thin backend? 
> Is that used for distributed/remote builds? Also is there any non-cc1 way to 
> use it? There is `--plugin-opt=dwo_dir=...` for the LTO linker plugin, but 
> that seems to go a different route.
This is the path taken for distributed ThinLTO, the plugin-opt passed to linker 
are for in-process ThinLTO. While -split-dwarf-file is a cc1 option, it is 
normally set automatically from the filename under -gsplit-dwarf (see where it 
is set in Clang::ConstructJob).


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-12 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

tejohnson wrote:
> aaronpuchert wrote:
> > aaronpuchert wrote:
> > > @pcc Your documentation for `DwoPath` suggests that this should be the 
> > > actual output filename. However, the test that you added together with 
> > > this line in rC333677 doesn't fail whatever garbage I write into that 
> > > field here. What can I add to that so that it fails when we don't do the 
> > > right thing here?
> > @pcc Could you (or perhaps @tejohnson) comment on what the intended 
> > behavior is here, and how I can change the test so that it fails when I do 
> > the wrong thing? Is this the name of the file we write the split debug info 
> > to, or is it the value we use for the DW_AT_[GNU_]dwo_name attribute in the 
> > skeleton CU?
> It is the name of the file the split debug info is written to. If you test by 
> changing the file name given to the -split-dwarf-file option in 
> test/CodeGen/thinlto-split-dwarf.c, make sure you clean up the old one in 
> your test output directory. When I tested it just now, it initially still 
> passed because I had an old one sitting in the output directory from a prior 
> run. Once I removed that it failed with the new name (without changing the 
> corresponding llvm-readobj invocation).
Thanks, I didn't consider that. I wasn't even aware that test output is 
persisted.

It seems `DwoPath` is used both as output filename and as value for 
`DW_AT_[GNU_]dwo_name` in the skeleton CU. `llc` has separate options for both: 
`-split-dwarf-file` for the attribute and `-split-dwarf-output` for the output 
filename. I want to do the same for Clang. Then we should probably separate 
them for LTO, too.

What is the use case for `-split-dwarf-file` with an individual thin backend? 
Is that used for distributed/remote builds? Also is there any non-cc1 way to 
use it? There is `--plugin-opt=dwo_dir=...` for the LTO linker plugin, but that 
seems to go a different route.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-12 Thread Teresa Johnson via Phabricator via cfe-commits 
tejohnson added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

aaronpuchert wrote:
> aaronpuchert wrote:
> > @pcc Your documentation for `DwoPath` suggests that this should be the 
> > actual output filename. However, the test that you added together with this 
> > line in rC333677 doesn't fail whatever garbage I write into that field 
> > here. What can I add to that so that it fails when we don't do the right 
> > thing here?
> @pcc Could you (or perhaps @tejohnson) comment on what the intended behavior 
> is here, and how I can change the test so that it fails when I do the wrong 
> thing? Is this the name of the file we write the split debug info to, or is 
> it the value we use for the DW_AT_[GNU_]dwo_name attribute in the skeleton CU?
It is the name of the file the split debug info is written to. If you test by 
changing the file name given to the -split-dwarf-file option in 
test/CodeGen/thinlto-split-dwarf.c, make sure you clean up the old one in your 
test output directory. When I tested it just now, it initially still passed 
because I had an old one sitting in the output directory from a prior run. Once 
I removed that it failed with the new name (without changing the corresponding 
llvm-readobj invocation).


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-12 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added a subscriber: tejohnson.
aaronpuchert added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

aaronpuchert wrote:
> @pcc Your documentation for `DwoPath` suggests that this should be the actual 
> output filename. However, the test that you added together with this line in 
> rC333677 doesn't fail whatever garbage I write into that field here. What can 
> I add to that so that it fails when we don't do the right thing here?
@pcc Could you (or perhaps @tejohnson) comment on what the intended behavior is 
here, and how I can change the test so that it fails when I do the wrong thing? 
Is this the name of the file we write the split debug info to, or is it the 
value we use for the DW_AT_[GNU_]dwo_name attribute in the skeleton CU?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-11 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-11 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert marked 2 inline comments as done.
aaronpuchert added a comment.

In D59673#1521413 , @dblaikie wrote:

> Might be easier as a few patches - renaming the existing option,


D63130 

> adding the new one,

This change.

> then removing the single split dwarf flag handling in favor of implying that 
> by the absence of an output file name. (if I'm reading what this patch does)

D63167 


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-11 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert updated this revision to Diff 204017.
aaronpuchert added a comment.

Correct an oversight.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673

Files:
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/CC1Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-filename.c
  test/CodeGen/split-debug-output.c
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c

Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -3,7 +3,7 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
 //
-// CHECK-ACTIONS: "-split-dwarf-output" "split-debug.dwo"
+// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
@@ -14,12 +14,14 @@
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS-SINGLE-SPLIT < %t %s
 //
 // CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf=single"
-// CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-output" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-file" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### -o %tfoo.o %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SINGLE-SPLIT-FILENAME < %t %s
 //
-// CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-output" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
@@ -41,7 +43,7 @@
 // RUN: %clang -target amdgcn-amd-amdhsa -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-OPTION < %t %s
 //
-// CHECK-OPTION: "-split-dwarf-output" "split-debug.dwo"
+// CHECK-OPTION: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ASM < %t %s
@@ -58,6 +60,7 @@
 //
 // CHECK-GMLT-WITH-SPLIT: "-enable-split-dwarf"
 // CHECK-GMLT-WITH-SPLIT: "-debug-info-kind=line-tables-only"
+// CHECK-GMLT-WITH-SPLIT: "-split-dwarf-file"
 // CHECK-GMLT-WITH-SPLIT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -g -fno-split-dwarf-inlining -S -### %s 2> %t
@@ -86,12 +89,14 @@
 //
 // CHECK-GMLT-OVER-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-GMLT-OVER-SPLIT: "-debug-info-kind=line-tables-only"
+// CHECK-GMLT-OVER-SPLIT-NOT: "-split-dwarf-file"
 // CHECK-GMLT-OVER-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gmlt -gsplit-dwarf -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SPLIT-OVER-GMLT < %t %s
 //
 // CHECK-SPLIT-OVER-GMLT: "-enable-split-dwarf" "-debug-info-kind=limited"
+// CHECK-SPLIT-OVER-GMLT: "-split-dwarf-file"
 // CHECK-SPLIT-OVER-GMLT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -g0 -fno-split-dwarf-inlining -S -### %s 2> %t
@@ -99,6 +104,7 @@
 //
 // CHECK-G0-OVER-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-G0-OVER-SPLIT-NOT: "-debug-info-kind
+// CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-file"
 // CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -g0 -S -### %s 2> %t
@@ -108,6 +114,7 @@
 //
 // CHECK-G0-OVER-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-G0-OVER-SPLIT-NOT: "-debug-info-kind
+// CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-file"
 // CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -g0 -gsplit-dwarf -S -### %s 2> %t
@@ -116,4 +123,5 @@
 // RUN: FileCheck -check-prefix=CHECK-SPLIT-OVER-G0 < %t %s
 //
 // CHECK-SPLIT-OVER-G0: "-enable-split-dwarf" "-debug-info-kind=limited"
+// CHECK-SPLIT-OVER-G0: "-split-dwarf-file"
 // CHECK-SPLIT-OVER-G0: "-split-dwarf-output"
Index: test/CodeGen/split-debug-single-file.c
===
--- test/CodeGen/split-debug-single-file.c
+++ test/CodeGen/split-debug-single-file.c
@@ -2,13 +2,13 @@
 
 // Testing to ensure -enable-split-dwarf=single allows to place .dwo sections into regular output object.
 //  RUN: %clang_cc1 -debug-info-kind=limited -triple x86_64-unknown-linux \
-//  RUN:   -enable-split-dwarf=single -split-dwarf-output %t.o -emit-obj -o %t.o %s
+//  RUN:   -enable-split-dwarf=single -split-dwarf-file %t.o -emit-obj -o %t.o %s
 //  RUN: llvm-readobj -S %t.o | FileCheck --check-prefix=MODE-SINGLE

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-11 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert updated this revision to Diff 204013.
aaronpuchert added a comment.

Split from other changes as suggested. A predecessor is in D63130 
, and a successor will come soon.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673

Files:
  include/clang/Basic/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-filename.c
  test/CodeGen/split-debug-output.c
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c

Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -3,7 +3,7 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
 //
-// CHECK-ACTIONS: "-split-dwarf-output" "split-debug.dwo"
+// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
@@ -14,12 +14,14 @@
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS-SINGLE-SPLIT < %t %s
 //
 // CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf=single"
-// CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-output" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-file" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### -o %tfoo.o %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SINGLE-SPLIT-FILENAME < %t %s
 //
-// CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-output" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
@@ -41,7 +43,7 @@
 // RUN: %clang -target amdgcn-amd-amdhsa -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-OPTION < %t %s
 //
-// CHECK-OPTION: "-split-dwarf-output" "split-debug.dwo"
+// CHECK-OPTION: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ASM < %t %s
@@ -58,6 +60,7 @@
 //
 // CHECK-GMLT-WITH-SPLIT: "-enable-split-dwarf"
 // CHECK-GMLT-WITH-SPLIT: "-debug-info-kind=line-tables-only"
+// CHECK-GMLT-WITH-SPLIT: "-split-dwarf-file"
 // CHECK-GMLT-WITH-SPLIT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -g -fno-split-dwarf-inlining -S -### %s 2> %t
@@ -86,12 +89,14 @@
 //
 // CHECK-GMLT-OVER-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-GMLT-OVER-SPLIT: "-debug-info-kind=line-tables-only"
+// CHECK-GMLT-OVER-SPLIT-NOT: "-split-dwarf-file"
 // CHECK-GMLT-OVER-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gmlt -gsplit-dwarf -S -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SPLIT-OVER-GMLT < %t %s
 //
 // CHECK-SPLIT-OVER-GMLT: "-enable-split-dwarf" "-debug-info-kind=limited"
+// CHECK-SPLIT-OVER-GMLT: "-split-dwarf-file"
 // CHECK-SPLIT-OVER-GMLT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -g0 -fno-split-dwarf-inlining -S -### %s 2> %t
@@ -99,6 +104,7 @@
 //
 // CHECK-G0-OVER-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-G0-OVER-SPLIT-NOT: "-debug-info-kind
+// CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-file"
 // CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -g0 -S -### %s 2> %t
@@ -108,6 +114,7 @@
 //
 // CHECK-G0-OVER-SPLIT-NOT: "-enable-split-dwarf"
 // CHECK-G0-OVER-SPLIT-NOT: "-debug-info-kind
+// CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-file"
 // CHECK-G0-OVER-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -g0 -gsplit-dwarf -S -### %s 2> %t
@@ -116,4 +123,5 @@
 // RUN: FileCheck -check-prefix=CHECK-SPLIT-OVER-G0 < %t %s
 //
 // CHECK-SPLIT-OVER-G0: "-enable-split-dwarf" "-debug-info-kind=limited"
+// CHECK-SPLIT-OVER-G0: "-split-dwarf-file"
 // CHECK-SPLIT-OVER-G0: "-split-dwarf-output"
Index: test/CodeGen/split-debug-single-file.c
===
--- test/CodeGen/split-debug-single-file.c
+++ test/CodeGen/split-debug-single-file.c
@@ -2,13 +2,13 @@
 
 // Testing to ensure -enable-split-dwarf=single allows to place .dwo sections into regular output object.
 //  RUN: %clang_cc1 -debug-info-kind=limited -triple x86_64-unknown-linux \
-//  RUN:   -enable-split-dwarf=single -split-dwarf-output %t.o -emit-obj -o %t.o %s
+//  RUN:   -enable-split-dwarf=single -split-dwarf-file %t.o -emit-obj -o %t.o

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-05 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added a comment.

In D59673#1527487 , @aaronpuchert 
wrote:

> In D59673#1521413 , @dblaikie wrote:
>
> > Might be easier as a few patches - renaming the existing option, adding the 
> > new one, then removing the single split dwarf flag handling in favor of 
> > implying that by the absence of an output file name.
>
>
> No problem, makes sense to me. I'll see if it's possible to separate these 
> changes.


I have managed to split this into commits that build and test fine own their 
own, but there are temporary inconsistencies. If I rename `-split-dwarf-file` 
to `-split-dwarf-output` first, then I have to use that option for 
`-enable-split-dwarf=single`, and then I reintroduce `-split-dwarf-file` and 
have to change it back. It's not a huge problem, but it might feel a bit weird. 
On the other hand, the changes are indeed smaller and easier to review. I will 
push them later today (hopefully).


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-06-03 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.

In D59673#1521413 , @dblaikie wrote:

> Might be easier as a few patches - renaming the existing option, adding the 
> new one, then removing the single split dwarf flag handling in favor of 
> implying that by the absence of an output file name.


No problem, makes sense to me. I'll see if it's possible to separate these 
changes.

> if I'm reading what this patch does

Yes, that seems to be how `llc` behaves, and I want to copy that.




Comment at: lib/CodeGen/BackendUtil.cpp:847
   default:
-if (!CodeGenOpts.SplitDwarfFile.empty() &&
-(CodeGenOpts.getSplitDwarfMode() == CodeGenOptions::SplitFileFission)) 
{
-  DwoOS = openOutputFile(CodeGenOpts.SplitDwarfFile);
+if (CodeGenOpts.EnableSplitDwarf && !CodeGenOpts.SplitDwarfOutput.empty()) 
{
+  DwoOS = openOutputFile(CodeGenOpts.SplitDwarfOutput);

dblaikie wrote:
> Why did this add a check for EnableSplitDwarf here that wasn't there before?
I just changed the order, the check was there as 
`(CodeGenOpts.getSplitDwarfMode() == CodeGenOptions::SplitFileFission)` in the 
second line of the `if`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-05-29 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

Might be easier as a few patches - renaming the existing option, adding the new 
one, then removing the single split dwarf flag handling in favor of implying 
that by the absence of an output file name. (if I'm reading what this patch 
does)




Comment at: include/clang/Basic/CodeGenOptions.h:185-188
+  /// The name for the split debug info file used in the skeleton CU.
   std::string SplitDwarfFile;
 
+  /// Output filename for the split debug info, not used in the skeleton CU.

Comments are a bit inconsistent - "filename for the split debug info" versus 
"name for the split debug info file" - makes it perhaps a bit harder to see 
what the important difference is between these two values.

It might be helpful to clarify "used in the skeleton CU" as "Used as the 
dwo_name in the DWARF" versus "the name of the file to write the .dwo sections 
to" or something like that?



Comment at: lib/CodeGen/BackendUtil.cpp:847
   default:
-if (!CodeGenOpts.SplitDwarfFile.empty() &&
-(CodeGenOpts.getSplitDwarfMode() == CodeGenOptions::SplitFileFission)) 
{
-  DwoOS = openOutputFile(CodeGenOpts.SplitDwarfFile);
+if (CodeGenOpts.EnableSplitDwarf && !CodeGenOpts.SplitDwarfOutput.empty()) 
{
+  DwoOS = openOutputFile(CodeGenOpts.SplitDwarfOutput);

Why did this add a check for EnableSplitDwarf here that wasn't there before?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-05-22 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added a comment.

Ping. I'm sorry that the change turned out so big, but note that it doesn't 
change the behavior of any non-cc1 flags.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-04-22 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert updated this revision to Diff 196156.
aaronpuchert added a comment.

Adapt one more test case.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673

Files:
  include/clang/Basic/CodeGenOptions.def
  include/clang/Basic/CodeGenOptions.h
  include/clang/Driver/CC1Options.td
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-filename.c
  test/CodeGen/split-debug-output.c
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s
  test/Misc/cc1as-split-dwarf.s
  tools/driver/cc1as_main.cpp

Index: tools/driver/cc1as_main.cpp
===
--- tools/driver/cc1as_main.cpp
+++ tools/driver/cc1as_main.cpp
@@ -98,7 +98,7 @@
   llvm::DebugCompressionType CompressDebugSections =
   llvm::DebugCompressionType::None;
   std::string MainFileName;
-  std::string SplitDwarfFile;
+  std::string SplitDwarfOutput;
 
   /// @}
   /// @name Frontend Options
@@ -258,7 +258,7 @@
   }
   Opts.LLVMArgs = Args.getAllArgValues(OPT_mllvm);
   Opts.OutputPath = Args.getLastArgValue(OPT_o);
-  Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
+  Opts.SplitDwarfOutput = Args.getLastArgValue(OPT_split_dwarf_output);
   if (Arg *A = Args.getLastArg(OPT_filetype)) {
 StringRef Name = A->getValue();
 unsigned OutputType = StringSwitch(Name)
@@ -367,8 +367,8 @@
   if (!FDOS)
 return true;
   std::unique_ptr DwoOS;
-  if (!Opts.SplitDwarfFile.empty())
-DwoOS = getOutputStream(Opts.SplitDwarfFile, Diags, IsBinary);
+  if (!Opts.SplitDwarfOutput.empty())
+DwoOS = getOutputStream(Opts.SplitDwarfOutput, Diags, IsBinary);
 
   // FIXME: This is not pretty. MCContext has a ptr to MCObjectFileInfo and
   // MCObjectFileInfo needs a MCContext reference in order to initialize itself.
@@ -527,8 +527,8 @@
   if (Failed) {
 if (Opts.OutputPath != "-")
   sys::fs::remove(Opts.OutputPath);
-if (!Opts.SplitDwarfFile.empty() && Opts.SplitDwarfFile != "-")
-  sys::fs::remove(Opts.SplitDwarfFile);
+if (!Opts.SplitDwarfOutput.empty() && Opts.SplitDwarfOutput != "-")
+  sys::fs::remove(Opts.SplitDwarfOutput);
   }
 
   return Failed;
Index: test/Misc/cc1as-split-dwarf.s
===
--- test/Misc/cc1as-split-dwarf.s
+++ test/Misc/cc1as-split-dwarf.s
@@ -1,5 +1,5 @@
 // REQUIRES: x86-registered-target
-// RUN: %clang -cc1as -triple x86_64-pc-linux-gnu %s -filetype obj -o %t1 -split-dwarf-file %t2
+// RUN: %clang -cc1as -triple x86_64-pc-linux-gnu %s -filetype obj -o %t1 -split-dwarf-output %t2
 // RUN: llvm-objdump -s %t1 | FileCheck --check-prefix=O %s
 // RUN: llvm-objdump -s %t2 | FileCheck --check-prefix=DWO %s
 
Index: test/Driver/split-debug.s
===
--- test/Driver/split-debug.s
+++ test/Driver/split-debug.s
@@ -3,7 +3,7 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
 //
-// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
+// CHECK-ACTIONS: "-split-dwarf-output" "split-debug.dwo"
 
 // Check we pass -split-dwarf-file to `as` if -gsplit-dwarf=split.
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=split -c -### %s 2> %t
Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -3,7 +3,7 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
 //
-// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
+// CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
@@ -13,13 +13,15 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-ACTIONS-SINGLE-SPLIT < %t %s
 //
-// CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf=single"
+// CHECK-ACTIONS-SINGLE-SPLIT: "-enable-split-dwarf"
 // CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-file" "split-debug.o"
+// CHECK-ACTIONS-SINGLE-SPLIT-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### -o %tfoo.o %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-SINGLE-SPLIT-FILENAME < %t %s
 //
 // CHECK-SINGLE-SPLIT-FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
+// CHECK-SINGLE-SPLIT-FILENAME-NOT: "-split-dwarf-output"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
@@ -41,7 +43,7 @@
 // RUN: %clang -target

[PATCH] D59673: [Clang] Harmonize Split DWARF options with llc

2019-04-22 Thread Aaron Puchert via Phabricator via cfe-commits 
aaronpuchert added a subscriber: pcc.
aaronpuchert added inline comments.



Comment at: lib/CodeGen/BackendUtil.cpp:1345
   Conf.RemarksPasses = CGOpts.OptRecordPasses;
-  Conf.DwoPath = CGOpts.SplitDwarfFile;
+  Conf.DwoPath = CGOpts.SplitDwarfOutput;
   switch (Action) {

@pcc Your documentation for `DwoPath` suggests that this should be the actual 
output filename. However, the test that you added together with this line in 
rC333677 doesn't fail whatever garbage I write into that field here. What can I 
add to that so that it fails when we don't do the right thing here?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59673



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