[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In https://reviews.llvm.org/D52296#1258677, @alexshap wrote:

> @grimar, this is an interesting observation which I've had on my mind for 
> quite some time as well; a couple of things which I have not double-checked 
> yet - just in case - do both gold and lld completely ignore dwo-related 
> sections ? (did you check that ?),


LLVM emits them with the SHF_EXCLUDE flag since the 
https://reviews.llvm.org/rL342800 "[lib/MC] - Set SHF_EXCLUDE flag for .dwo 
sections.". So if linker supports SHF_EXCLUDE flag properly, it will ignore it 
properly.
That is true for bfd/gold/LLD atm.

LLD does not ignore ".dwo" sections by name and I do not expect other linkers 
do (and that is fine, we do not want to ignore sections by name generally), so 
having this flag is an important and clean thing for things to work.

> and another small question - just wondering if the debuggers (GDB and LLDB) 
> are okay with it / or smth needs to be adjusted or fixed on their side. I 
> guess everything should be fine, but asking just in case.

The patch for LLDB is ready to be landed: https://reviews.llvm.org/D52403. It 
waits for this one, since its test case mentions/uses -gsingle-file-split-dwarf 
option.
I am thinking about rewriting the comment and landing it independently.

I did not check the GDB yet.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Maybe `-gno-dwo`? So we would write `-genable-split-dwarf -gno-dwo`?


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar updated this revision to Diff 171480.
grimar added a comment.

Ping.

- Rebased.


https://reviews.llvm.org/D52296

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s

Index: test/Driver/split-debug.s
===
--- test/Driver/split-debug.s
+++ test/Driver/split-debug.s
@@ -5,6 +5,9 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
 
+// Check we do not pass any -split-dwarf commands to `as` if -gsingle-file-split-dwarf is specified.
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -gsingle-file-split-dwarf -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -5,6 +5,16 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
 
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -gsingle-file-split-dwarf -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS-SINGLE-SPLIT < %t %s
+//
+// CHECK-ACTIONS-SINGLE-SPLIT: "-single-file-split-dwarf"
+// CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-file" "split-debug.o"
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -gsingle-file-split-dwarf -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"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/CodeGen/split-debug-single-file.c
===
--- test/CodeGen/split-debug-single-file.c
+++ test/CodeGen/split-debug-single-file.c
@@ -0,0 +1,16 @@
+// REQUIRES: x86-registered-target
+
+// Testing to ensure -single-file-split-dwarf 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 -split-dwarf-file %t.o -emit-obj -single-file-split-dwarf -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=SINGLE-FILE-SPLIT %s
+//  SINGLE-FILE-SPLIT: .dwo
+
+// Testing to ensure that the dwo name gets output into the compile unit.
+//  RUN: %clang_cc1 -debug-info-kind=limited -split-dwarf-file foo.o -single-file-split-dwarf \
+//  RUN:   -S -emit-llvm -o - %s | FileCheck %s --check-prefix=DWONAME
+//  DWONAME: !DICompileUnit({{.*}}, splitDebugFilename: "foo.o"
+
+int main (void) {
+  return 0;
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -598,6 +598,8 @@
   Opts.EnableSplitDwarf = Args.hasArg(OPT_enable_split_dwarf);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
+  Opts.SingleFileSplitDwarf = Args.hasArg(OPT_single_file_split_dwarf);
+
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
   Opts.DebugExplicitImport = Args.hasArg(OPT_dwarf_explicit_import);
   Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -54,7 +54,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0]));
+   SplitDebugName(Args, Inputs[0], Output));
 }
 
 void tools::MinGW::Linker::AddLibGCC(const ArgList &Args,
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -817,7 +817,7 @@
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
   getToolChain().getTriple().isOSLinux())
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0]));
+   SplitDebugName(Args, Inputs[0], Output));
 }
 
 namespace {
Index: lib/Driver/ToolChains/CommonArgs.h
===
--- lib/Driver/ToolChains/CommonArgs.h
+++ lib/

[PATCH] D56663: [MSP430] Improve support of 'interrupt' attribute

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Sema/SemaDeclAttr.cpp:5381
+  // MSP430 'interrupt' attribute is applied to
+  // a function with no parameters and void return type
+  if (!isFunctionOrMethod(D)) {

nit: code comments usually have a dot `.` at the end of the sentence.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56663



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

[PATCH] D56925: Do not use frame pointer by default for MSP430

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Testcase?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56925



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

[PATCH] D56927: Disable PIC/PIE for MSP430 target

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Can it be tested with a test case?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56927



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

[PATCH] D56925: Do not use frame pointer by default for MSP430

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Thanks for adding a test case! I know not enough about msp430/clang to verify 
it, unfortunately.
(I just asked for a test because it is always useful to have one for any 
non-NFC patch).


Repository:
  rC Clang

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

https://reviews.llvm.org/D56925



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

[PATCH] D56927: Disable PIC/PIE for MSP430 target

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

What I noticed, this seems to introduce the first test case in 
`test\CodeGen\MSP430` folder which uses `clang`. All the others use `llc`. Is 
it OK?


Repository:
  rC Clang

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

https://reviews.llvm.org/D56927



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

[PATCH] D56927: Disable PIC/PIE for MSP430 target

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Oh sorry, I looked in the wrong folder, my previous comment was not correct.

So, this patch adds a test to `test\CodeGen` when we have `test\CodeGen\MSP430` 
folder.
And `test\CodeGen` does not have any tests now at all.
So I **guess** the correct way would probably be:

1. To move the test to `test\CodeGen\MSP430` folder.
2. To make it use 'llc`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56927



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

[PATCH] D56927: Disable PIC/PIE for MSP430 target

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Ok, please ignore my comments above. I applied the patch and found it creates 
the test in `llvm\tools\clang\test\CodeGen` and not in `llvm\test\CodeGen`. Was 
confused by phab, sorry for the noise.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56927



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In https://reviews.llvm.org/D52296#1281642, @echristo wrote:

> Can you elaborate on your motivations and what you're trying to do?
>
> Thanks!


We want to see:

- No extra files. The compiler produces just a .o.
- The linker leaves most debug info in the .o files.

That makes the build friendly to existing tools and avoids most of the static 
linker work.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Nice :) 
So seems the last unresolved question left is the naming of the new option?
If we do not want to go with `-gsingle-file-split-dwarf` then what it should be?

What about `-fdebug-fission` as an alias for `-gsplit-dwarf`.
And `-fsingle-file-debug-fission` for the new option?

Or, may be:

`-fdebug-fission` for the new option and then
`-fdebug-fission -fdebug-split-dwo` would work together as `-gsplit-dwarf`?


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In https://reviews.llvm.org/D52296#1284130, @probinson wrote:

> In https://reviews.llvm.org/D52296#1283691, @grimar wrote:
>
> > Nice :) 
> >  So seems the last unresolved question left is the naming of the new option?
> >  If we do not want to go with `-gsingle-file-split-dwarf` then what it 
> > should be?
> >
> > What about `-fdebug-fission` as an alias for `-gsplit-dwarf`.
> >  And `-fsingle-file-debug-fission` for the new option?
> >
> > Or, may be:
> >
> > `-fdebug-fission` for the new option and then
> >  `-fdebug-fission -fdebug-split-dwo` would work together as `-gsplit-dwarf`?
>
>
> Only DWARF supports this, correct?


I am not aware of any kind of debug information splitting except DWARF 
splitting.

> So I would suggest: `-fdwarf-fission[={split,single}]` where the parameter 
> defaults to `split`. Is there any particular value in having two separate 
> options?

Probably not. `-fdwarf-fission[={split,single}]` sounds good for me.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[George Rimar via Phabricator via cfe-commits]

grimar updated this revision to Diff 172953.
grimar retitled this revision from "[Clang] - Add -gsingle-file-split-dwarf 
option." to "[Clang] - Add -fdwarf-fission=split,single option.".
grimar added a comment.

Reimplemented option as `-fdwarf-fission[=split,single]`.


https://reviews.llvm.org/D52296

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s

Index: test/Driver/split-debug.s
===
--- test/Driver/split-debug.s
+++ test/Driver/split-debug.s
@@ -5,6 +5,13 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
 
+// Check we pass -split-dwarf-file to `as` if -fdwarf-fission=split.
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=split -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
+
+// Check we do not pass any -split-dwarf* commands to `as` if -fdwarf-fission=single.
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=single -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -5,6 +5,21 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
 
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=split -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=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: "-split-dwarf-file" "split-debug.o"
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=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"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/CodeGen/split-debug-single-file.c
===
--- test/CodeGen/split-debug-single-file.c
+++ test/CodeGen/split-debug-single-file.c
@@ -0,0 +1,17 @@
+// REQUIRES: x86-registered-target
+
+// 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-file %t.o -emit-obj -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=MODE-SINGLE %s
+//  MODE-SINGLE: .dwo
+
+// 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=split -split-dwarf-file %t.o -emit-obj -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo
+
+int main (void) {
+  return 0;
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -596,9 +596,25 @@
   Opts.MacroDebugInfo = Args.hasArg(OPT_debug_info_macro);
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibility_public_std);
-  Opts.EnableSplitDwarf = Args.hasArg(OPT_enable_split_dwarf);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
+ 
+  if (Arg *A =
+  Args.getLastArg(OPT_enable_split_dwarf, OPT_enable_split_dwarf_EQ)) {
+if (A->getOption().matches(options::OPT_enable_split_dwarf)) {
+  Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFission);
+} else {
+  StringRef Name = A->getValue();
+  if (Name == "single")
+Opts.setSplitDwarfMode(CodeGenOptions::SingleFileFission);
+  else if (Name == "split")
+Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFissi

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In https://reviews.llvm.org/D52296#1290212, @dblaikie wrote:

> Thanks! - though on reflection I'm going to invoke @echristo again about the 
> naming. It's unfortunately a bit backwards that the pre-standard flag is 
> -gsplit-dwarf and what we're proposing as the standard flag is 
> -fdwarf-fission, when the DWARF standard doesn't use the word "Fission" at 
> all (only "split DWARF").
>
> Maybe it'd be easy enough/better to add the "=single" suffix to the existing 
> "-gsplit-dwarf"? (as much as I'm still a bit confused by which debug options 
> use a '-g' and which ones use a '-f', etc).
>
> Really sorry to go back/forth/around about on this, George.


No problems. The naming of the new flags is an important thing, no need to rush 
here I believe.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[George Rimar via Phabricator via cfe-commits]

grimar updated this revision to Diff 173135.
grimar marked an inline comment as done.
grimar added a comment.

- Addressed review comments.


https://reviews.llvm.org/D52296

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s

Index: test/Driver/split-debug.s
===
--- test/Driver/split-debug.s
+++ test/Driver/split-debug.s
@@ -5,6 +5,13 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
 
+// Check we pass -split-dwarf-file to `as` if -fdwarf-fission=split.
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=split -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
+
+// Check we do not pass any -split-dwarf* commands to `as` if -fdwarf-fission=single.
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=single -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -5,6 +5,21 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
 
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=split -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=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: "-split-dwarf-file" "split-debug.o"
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -fdwarf-fission=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"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/CodeGen/split-debug-single-file.c
===
--- test/CodeGen/split-debug-single-file.c
+++ test/CodeGen/split-debug-single-file.c
@@ -0,0 +1,17 @@
+// REQUIRES: x86-registered-target
+
+// 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-file %t.o -emit-obj -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=MODE-SINGLE %s
+//  MODE-SINGLE: .dwo
+
+// 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=split -split-dwarf-file %t.o -emit-obj -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo
+
+int main (void) {
+  return 0;
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -596,9 +596,25 @@
   Opts.MacroDebugInfo = Args.hasArg(OPT_debug_info_macro);
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibility_public_std);
-  Opts.EnableSplitDwarf = Args.hasArg(OPT_enable_split_dwarf);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
+ 
+  if (Arg *A =
+  Args.getLastArg(OPT_enable_split_dwarf, OPT_enable_split_dwarf_EQ)) {
+if (A->getOption().matches(options::OPT_enable_split_dwarf)) {
+  Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFission);
+} else {
+  StringRef Name = A->getValue();
+  if (Name == "single")
+Opts.setSplitDwarfMode(CodeGenOptions::SingleFileFission);
+  else if (Name == "split")
+Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFission);
+  else
+Diags.Report(diag::err_drv_invalid_value)
+<< A->getAsString(Args) << Name;
+}
+  }
+
   

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:2999-3001
+StringRef Value = A->getOption().matches(options::OPT_fdwarf_fission_EQ)
+  ? A->getValue()
+  : "split";

dblaikie wrote:
> Rather than creating a string in the case where there's no need for string 
> matching/parsing, perhaps that could be avoided?
> 
>   if (!A->getOption().matches(options::OPT_fdwarf_fission_EQ))
> return DwarfFissionKind::Split;
> 
>   StringRef Value = A->getValue();
>   if (Value == "split")
>   ...
> 
> 
Done.



Comment at: lib/Driver/ToolChains/Clang.cpp:3010-3011
+  }
+  if (ArgIndex)
+*ArgIndex = 0;
+  return DwarfFissionKind::None;

dblaikie wrote:
> I'd probably either accept this as a precondition (assert(!ArgIndex || 
> ArgIndex == 0)) or do this bit at the start of the function rather than the 
> end here, to make it simpler/clearer that all paths assign some value to 
> ArgIndex before exit of the function (even though that does mean 
> checking/assigning ArgIndex unnecessarily in the case where the argument is 
> given, etc)
It turns out that `A->getIndex()` returned can be different from index in 
`Args` array. 
So it is not correct to use something like `size_t Index = A->getIndex(); ; 
 Arg* A = Args.getArgs()[Index]`.

I rewrote this method.




Comment at: lib/Driver/ToolChains/Clang.cpp:5889
   const llvm::Triple &T = getToolChain().getTriple();
-  if (Args.hasArg(options::OPT_gsplit_dwarf) &&
+  if ((getDebugFissionKind(D, Args) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {

dblaikie wrote:
> Could having more than one call to getDebugFissionKind lead to the error 
> message (for -fdwarf-fission=garbage) being printed multiple times? Or is 
> this call on a distinct/mutually exclusive codepath to the other?
I think it should not be possible. Currently, there are 2 inclusions of the 
`getDebugFissionKind `. One is used for construction clang job,
and one is here, it is used to construct an assembler tool job. I do not see a 
way how it can be printed multiple times.

For example, the following invocation will print it only once:

```
clang main.c -o out.s -S -fdwarf-fission=foo
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
```


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:5889
   const llvm::Triple &T = getToolChain().getTriple();
-  if (Args.hasArg(options::OPT_gsplit_dwarf) &&
+  if ((getDebugFissionKind(D, Args) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {

probinson wrote:
> grimar wrote:
> > dblaikie wrote:
> > > Could having more than one call to getDebugFissionKind lead to the error 
> > > message (for -fdwarf-fission=garbage) being printed multiple times? Or is 
> > > this call on a distinct/mutually exclusive codepath to the other?
> > I think it should not be possible. Currently, there are 2 inclusions of the 
> > `getDebugFissionKind `. One is used for construction clang job,
> > and one is here, it is used to construct an assembler tool job. I do not 
> > see a way how it can be printed multiple times.
> > 
> > For example, the following invocation will print it only once:
> > 
> > ```
> > clang main.c -o out.s -S -fdwarf-fission=foo
> > clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> > ```
> The example you give here will not create an assembler job. Try
> `clang main.c -fdwarf-fission=foo -fno-integrated-as` which I think will 
> exercise the path David is asking about.
Oh, I did not know.

Seems the important option to test is `-save-temps`. Seems I have the same 
result with `-fno-integrated-as`/`-fintegrated-as`,
but with/without `-save-temps` I got:

```
clang main.cpp -fdwarf-fission=foo -o 1.o
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='

clang main.cpp -fdwarf-fission=foo -o 1.o -save-temps
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
```

Will fix, thanks!

(The behavior seems weird to me. I would not expect tool will be creating any 
jobs should happen when the command line is invalid from the start. I am not 
very familiar with the clang code/logic though to say it wrong).


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:5889
   const llvm::Triple &T = getToolChain().getTriple();
-  if (Args.hasArg(options::OPT_gsplit_dwarf) &&
+  if ((getDebugFissionKind(D, Args) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {

grimar wrote:
> probinson wrote:
> > grimar wrote:
> > > dblaikie wrote:
> > > > Could having more than one call to getDebugFissionKind lead to the 
> > > > error message (for -fdwarf-fission=garbage) being printed multiple 
> > > > times? Or is this call on a distinct/mutually exclusive codepath to the 
> > > > other?
> > > I think it should not be possible. Currently, there are 2 inclusions of 
> > > the `getDebugFissionKind `. One is used for construction clang job,
> > > and one is here, it is used to construct an assembler tool job. I do not 
> > > see a way how it can be printed multiple times.
> > > 
> > > For example, the following invocation will print it only once:
> > > 
> > > ```
> > > clang main.c -o out.s -S -fdwarf-fission=foo
> > > clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> > > ```
> > The example you give here will not create an assembler job. Try
> > `clang main.c -fdwarf-fission=foo -fno-integrated-as` which I think will 
> > exercise the path David is asking about.
> Oh, I did not know.
> 
> Seems the important option to test is `-save-temps`. Seems I have the same 
> result with `-fno-integrated-as`/`-fintegrated-as`,
> but with/without `-save-temps` I got:
> 
> ```
> clang main.cpp -fdwarf-fission=foo -o 1.o
> clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> 
> clang main.cpp -fdwarf-fission=foo -o 1.o -save-temps
> clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> ```
> 
> Will fix, thanks!
> 
> (The behavior seems weird to me. I would not expect tool will be creating any 
> jobs should happen when the command line is invalid from the start. I am not 
> very familiar with the clang code/logic though to say it wrong).
"I would not expect tool will be creating any jobs should happen" -> "I would 
not expect tool will be creating any jobs when ..."


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Looks like this behavior is what clang already have atm. 
Messages for the options that use `D.Diag` to report invalid values can be 
printed multiple times sometimes.

The example is below:

  clang main.cpp -fdwarf-fission=foo -o 1.o -mthread-model bar 
-fcf-runtime-abi=zed
  clang-8: error: invalid thread model 'bar' in '-mthread-model bar' for this 
target
  clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
  clang-8: error: invalid CoreFoundation Runtime ABI 'zed'; must be one of 
'objc', 'standalone', 'swift', 'swift-5.0', 'swift-4.2', 'swift-4.1'



  clang main.cpp -fdwarf-fission=foo -o 1.o -mthread-model bar 
-fcf-runtime-abi=zed -save-temps
  clang-8: error: invalid thread model 'bar' in '-mthread-model bar' for this 
target
  clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
  clang-8: error: invalid CoreFoundation Runtime ABI 'zed'; must be one of 
'objc', 'standalone', 'swift', 'swift-5.0', 'swift-4.2', 'swift-4.1'
  clang-8: error: invalid thread model 'bar' in '-mthread-model bar' for this 
target
  clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
  clang-8: error: invalid CoreFoundation Runtime ABI 'zed'; must be one of 
'objc', 'standalone', 'swift', 'swift-5.0', 'swift-4.2', 'swift-4.1'
  clang-8: error: invalid thread model 'bar' in '-mthread-model bar' for this 
target
  clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
  clang-8: error: invalid CoreFoundation Runtime ABI 'zed'; must be one of 
'objc', 'standalone', 'swift', 'swift-5.0', 'swift-4.2', 'swift-4.1'
  clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='

So it seems to me this patch just follows the behavior and nothing should be 
fixed at this point right now?


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Driver/ToolChains/Clang.cpp:5889
   const llvm::Triple &T = getToolChain().getTriple();
-  if (Args.hasArg(options::OPT_gsplit_dwarf) &&
+  if ((getDebugFissionKind(D, Args) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {

grimar wrote:
> grimar wrote:
> > probinson wrote:
> > > grimar wrote:
> > > > dblaikie wrote:
> > > > > Could having more than one call to getDebugFissionKind lead to the 
> > > > > error message (for -fdwarf-fission=garbage) being printed multiple 
> > > > > times? Or is this call on a distinct/mutually exclusive codepath to 
> > > > > the other?
> > > > I think it should not be possible. Currently, there are 2 inclusions of 
> > > > the `getDebugFissionKind `. One is used for construction clang job,
> > > > and one is here, it is used to construct an assembler tool job. I do 
> > > > not see a way how it can be printed multiple times.
> > > > 
> > > > For example, the following invocation will print it only once:
> > > > 
> > > > ```
> > > > clang main.c -o out.s -S -fdwarf-fission=foo
> > > > clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> > > > ```
> > > The example you give here will not create an assembler job. Try
> > > `clang main.c -fdwarf-fission=foo -fno-integrated-as` which I think will 
> > > exercise the path David is asking about.
> > Oh, I did not know.
> > 
> > Seems the important option to test is `-save-temps`. Seems I have the same 
> > result with `-fno-integrated-as`/`-fintegrated-as`,
> > but with/without `-save-temps` I got:
> > 
> > ```
> > clang main.cpp -fdwarf-fission=foo -o 1.o
> > clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> > 
> > clang main.cpp -fdwarf-fission=foo -o 1.o -save-temps
> > clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> > clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> > clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> > clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
> > ```
> > 
> > Will fix, thanks!
> > 
> > (The behavior seems weird to me. I would not expect tool will be creating 
> > any jobs should happen when the command line is invalid from the start. I 
> > am not very familiar with the clang code/logic though to say it wrong).
> "I would not expect tool will be creating any jobs should happen" -> "I would 
> not expect tool will be creating any jobs when ..."
And returning to this my example, indeed it should not have to create assembler 
job.
My mistake, sorry, just my head was overloaded with tons of new things.

The correct example should be:

```
clang library.s -fdwarf-fission=foo -o 1.o
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
```

So in this case clang creates the assembler job and prints the message only 
once.

If we add some *.cpp then we will see multiple messages (one for each job):

```
clang library.s main.cpp -fdwarf-fission=foo -o 1.o
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
```

But this is consistent with what clang already do. For example `-masm=` will 
trigger the same behavior:

```
~/LLVM/build_lldb/bin/clang library.s main.cpp -fdwarf-fission=foo -o 1.o 
-masm=foo
clang-8: error: unsupported argument 'foo' to option 'masm='
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission='
clang-8: error: unsupported argument 'foo' to option 'masm='
clang-8: error: unsupported argument 'foo' to option 'fdwarf-fission=
```

i.e. for options that are used both in main clang job and "secondary" jobs, 
like `as` that is possible already.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[George Rimar via Phabricator via cfe-commits]

grimar updated this revision to Diff 173643.
grimar retitled this revision from "[Clang] - Add -fdwarf-fission=split,single 
option." to "[Clang] - Add '-gsplit-dwarf[=split,=single]' version for 
'-gsplit-dwarf' option.".
grimar edited the summary of this revision.
grimar added a comment.

Thanks, David!

Changed:

- Introduced a -gsplit-dwarf[=split,=single] version for '-gsplit-dwarf' option


https://reviews.llvm.org/D52296

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s

Index: test/Driver/split-debug.s
===
--- test/Driver/split-debug.s
+++ test/Driver/split-debug.s
@@ -5,6 +5,13 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "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
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
+
+// Check we do not pass any -split-dwarf* commands to `as` if -gsplit-dwarf=single.
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=single -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -5,6 +5,21 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "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
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf=split -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS < %t %s
+
+// 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: "-split-dwarf-file" "split-debug.o"
+
+// 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"
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/CodeGen/split-debug-single-file.c
===
--- test/CodeGen/split-debug-single-file.c
+++ test/CodeGen/split-debug-single-file.c
@@ -0,0 +1,17 @@
+// REQUIRES: x86-registered-target
+
+// 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-file %t.o -emit-obj -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=MODE-SINGLE %s
+//  MODE-SINGLE: .dwo
+
+// 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=split -split-dwarf-file %t.o -emit-obj -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo
+
+int main (void) {
+  return 0;
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -596,9 +596,25 @@
   Opts.MacroDebugInfo = Args.hasArg(OPT_debug_info_macro);
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibility_public_std);
-  Opts.EnableSplitDwarf = Args.hasArg(OPT_enable_split_dwarf);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
+
+  if (Arg *A =
+  Args.getLastArg(OPT_enable_split_dwarf, OPT_enable_split_dwarf_EQ)) {
+if (A->getOption().matches(options::OPT_enable_split_dwarf)) {
+  Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFission);
+} else {
+  StringRef Name = A->getValue();
+  if (Name == "single")
+Opts.setSplitDwarfMode(CodeGenOptions::Singl

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());

dblaikie wrote:
> If this function now takes the output file name - could it be simplified to 
> just always use that, rather than the conditional code that follows and tests 
> whether -o is specified and builds up something like the output file name & 
> uses the dwo suffix?
I am investigating this.



Comment at: test/CodeGen/split-debug-single-file.c:12
+//  RUN:   -enable-split-dwarf=split -split-dwarf-file %t.o -emit-obj -o %t.o 
%s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck 
--check-prefix=MODE-SPLIT %s
+//  MODE-SPLIT-NOT: .dwo

dblaikie wrote:
> This looks like an end-to-end test, which we don't usually do in Clang (or in 
> the LLVM project in general).
> 
> For example, the previous testing for split-dwarf had a driver test (that 
> tested only that the driver produced the right cc1 invocation) and a CodeGen 
> test (that tested that the right IR was produced), but I don't see any test 
> like this (that tested the resulting object file)?
> 
> I know there's a narrow gap in IR testing - things that don't go in the IR, 
> but instead go through MCOptions  & that the SplitDwarfFile is one of those.
> 
> So, yeah, in this case it's a bit of a test hole - if you're going to keep 
> this test, perhaps it could test assembly, rather than the object file? Since 
> it doesn't need complex parsing, etc, perhaps?
> So, yeah, in this case it's a bit of a test hole - if you're going to keep 
> this test, perhaps it could test assembly, rather than the object file? Since 
> it doesn't need complex parsing, etc, perhaps?

I am not sure assembly can help here. For example
`clang main.c -S -g -gsplit-dwarf` would create single asm file output anyways
and what I tried to check here is that we can either place .dwo sections into 
the
same output or into a different one depending on new option.

> For example, the previous testing for split-dwarf had a driver test (that 
> tested only that the driver produced the right cc1 invocation) and a CodeGen 
> test (that tested that the right IR was produced), but I don't see any test 
> like this (that tested the resulting object file)?

I think `split-debug-filename.c` do that?
See: 
https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/split-debug-filename.c#L18

Also, `relax.c`: 
https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/relax.c
And `mips-inline-asm-abi.c`:  
https://github.com/llvm-mirror/clang/blob/master/test/CodeGen/mips-inline-asm-abi.c

Seems it is not common, but still acceptable?


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());

grimar wrote:
> dblaikie wrote:
> > If this function now takes the output file name - could it be simplified to 
> > just always use that, rather than the conditional code that follows and 
> > tests whether -o is specified and builds up something like the output file 
> > name & uses the dwo suffix?
> I am investigating this.
So what I see now is that when the function becomes:

```
const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
  const InputInfo &Output) {
  SmallString<128> T(Output.getFilename());
  llvm::sys::path::replace_extension(T, "dwo");
  return Args.MakeArgString(T);
}
```

Then no clang tests (check-clang) fail. This does not seem normal to me.
I would expect that such a change should break some `-fdebug-compilation-dir` 
relative 
logic at least and I suspect we just have no test(s) atm.

What about if I add test case(s) and post a follow-up refactor patch for this 
place?
(I started work on it, but it will take some time).


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[George Rimar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC346837: [Clang] - Add 
'-gsplit-dwarf[=split,=single]' version for 
'-gsplit-dwarf'… (authored by grimar, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52296?vs=173643&id=173998#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52296

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s

Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -596,9 +596,25 @@
   Opts.MacroDebugInfo = Args.hasArg(OPT_debug_info_macro);
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibility_public_std);
-  Opts.EnableSplitDwarf = Args.hasArg(OPT_enable_split_dwarf);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
+
+  if (Arg *A =
+  Args.getLastArg(OPT_enable_split_dwarf, OPT_enable_split_dwarf_EQ)) {
+if (A->getOption().matches(options::OPT_enable_split_dwarf)) {
+  Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFission);
+} else {
+  StringRef Name = A->getValue();
+  if (Name == "single")
+Opts.setSplitDwarfMode(CodeGenOptions::SingleFileFission);
+  else if (Name == "split")
+Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFission);
+  else
+Diags.Report(diag::err_drv_invalid_value)
+<< A->getAsString(Args) << Name;
+}
+  }
+
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
   Opts.DebugExplicitImport = Args.hasArg(OPT_dwarf_explicit_import);
   Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -468,7 +468,7 @@
   Options.EmitStackSizeSection = CodeGenOpts.StackSizeSection;
   Options.EmitAddrsig = CodeGenOpts.Addrsig;
 
-  if (CodeGenOpts.EnableSplitDwarf)
+  if (CodeGenOpts.getSplitDwarfMode() != CodeGenOptions::NoFission)
 Options.MCOptions.SplitDwarfFile = CodeGenOpts.SplitDwarfFile;
   Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
   Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
@@ -832,7 +832,8 @@
 break;
 
   default:
-if (!CodeGenOpts.SplitDwarfFile.empty()) {
+if (!CodeGenOpts.SplitDwarfFile.empty() &&
+(CodeGenOpts.getSplitDwarfMode() == CodeGenOptions::SplitFileFission)) {
   DwoOS = openOutputFile(CodeGenOpts.SplitDwarfFile);
   if (!DwoOS)
 return;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -581,8 +581,11 @@
   CGOpts.EmitVersionIdentMetadata ? Producer : "",
   LO.Optimize || CGOpts.PrepareForLTO || CGOpts.PrepareForThinLTO,
   CGOpts.DwarfDebugFlags, RuntimeVers,
-  CGOpts.EnableSplitDwarf ? "" : CGOpts.SplitDwarfFile, EmissionKind,
-  0 /* DWOid */, CGOpts.SplitDwarfInlining, CGOpts.DebugInfoForProfiling,
+  (CGOpts.getSplitDwarfMode() != CodeGenOptions::NoFission)
+  ? ""
+  : CGOpts.SplitDwarfFile,
+  EmissionKind, 0 /* DWOid */, CGOpts.SplitDwarfInlining,
+  CGOpts.DebugInfoForProfiling,
   CGM.getTarget().getTriple().isNVPTX()
   ? llvm::DICompileUnit::DebugNameTableKind::None
   : static_cast(
Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -808,7 +808,12 @@
   return false;
 }
 
-const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input) {
+const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
+  const InputInfo &Output) {
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());
Index: lib/Driver/ToolChains/MinGW.cpp

[PATCH] D54576: [clang] - Simplify tools::SplitDebugName.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

`SplitDebugName` is called from the following 4 places:

1. void Clang::ConstructJob()

(https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L3933)

We can get here when clang construct jobs for regular case (cpp files):
EX: main.cpp -g -gsplit-dwarf  -o ooo

Clang by itself does not recognize the '-fdebug-compilation-dir':
clang main.cpp -g -gsplit-dwarf  -o ooo -### -fdebug-compilation-dir
clang-8: error: unknown argument: '-fdebug-compilation-dir'

So I believe this option can not be used, be in Args 
and hence affect on anything during this flow.

In other places clang constructs assembly jobs:

2. void ClangAs::ConstructJob()

(https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L5900)

Here it tries to use the OPT_fdebug_compilation_dir option from Args:
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/CommonArgs.cpp#L824

And this is used to construct the argument for -split-dwarf-file.

The following also would not work:
clang main.s -g -gsplit-dwarf  -o ooo -fdebug-compilation-dir,xxx
clang-8: error: unknown argument: '-fdebug-compilation-dir,xxx'

So I do not see the way to add the '-fdebug-compilation-dir' to Args here too,
so `SplitDebugName` does not use the '-fdebug-compilation-dir' I think
and its logic is dead for this case too I believe)

3, 4) These are similar:

tools::gnutools::Assembler::ConstructJob()
(https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Gnu.cpp#L820)

tools::MinGW::Assembler::ConstructJob()
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/MinGW.cpp#L57

Both call `SplitDebugName` to construct calls for objcopy:
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/CommonArgs.cpp#L833

But again, `Args` for their `ConstructJob` seems can never contain the 
`-fdebug-compilation-dir`
I just do not see any way to achieve that.

Clang's code adds the `-fdebug-compilation-dir` by itself in 
`Clang::ConstructJob()` and
`ClangAs::ConstructJob()`, but it will never be used in `SplitDebugName` I 
think:
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L601
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L5795
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L4172

That `-fdebug-compilation-dir` is used in another places, not in the 
`SplitDebugName`
(used in invocations for -cc1 and -cc1as:
https://github.com/llvm-mirror/clang/blob/master/tools/driver/cc1as_main.cpp#L234
https://github.com/llvm-mirror/clang/blob/master/lib/Frontend/CompilerInvocation.cpp#L944)

After changes done by this patch, no check-llvm/check-clang tests started to 
fail for me.


https://reviews.llvm.org/D54576



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

[PATCH] D54576: [clang] - Simplify tools::SplitDebugName.

[George Rimar via Phabricator via cfe-commits]

grimar created this revision.
grimar added a reviewer: dblaikie.
grimar added a comment.

`SplitDebugName` is called from the following 4 places:

1. void Clang::ConstructJob()

(https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L3933)

We can get here when clang construct jobs for regular case (cpp files):
EX: main.cpp -g -gsplit-dwarf  -o ooo

Clang by itself does not recognize the '-fdebug-compilation-dir':
clang main.cpp -g -gsplit-dwarf  -o ooo -### -fdebug-compilation-dir
clang-8: error: unknown argument: '-fdebug-compilation-dir'

So I believe this option can not be used, be in Args 
and hence affect on anything during this flow.

In other places clang constructs assembly jobs:

2. void ClangAs::ConstructJob()

(https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L5900)

Here it tries to use the OPT_fdebug_compilation_dir option from Args:
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/CommonArgs.cpp#L824

And this is used to construct the argument for -split-dwarf-file.

The following also would not work:
clang main.s -g -gsplit-dwarf  -o ooo -fdebug-compilation-dir,xxx
clang-8: error: unknown argument: '-fdebug-compilation-dir,xxx'

So I do not see the way to add the '-fdebug-compilation-dir' to Args here too,
so `SplitDebugName` does not use the '-fdebug-compilation-dir' I think
and its logic is dead for this case too I believe)

3, 4) These are similar:

tools::gnutools::Assembler::ConstructJob()
(https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Gnu.cpp#L820)

tools::MinGW::Assembler::ConstructJob()
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/MinGW.cpp#L57

Both call `SplitDebugName` to construct calls for objcopy:
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/CommonArgs.cpp#L833

But again, `Args` for their `ConstructJob` seems can never contain the 
`-fdebug-compilation-dir`
I just do not see any way to achieve that.

Clang's code adds the `-fdebug-compilation-dir` by itself in 
`Clang::ConstructJob()` and
`ClangAs::ConstructJob()`, but it will never be used in `SplitDebugName` I 
think:
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L601
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L5795
https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L4172

That `-fdebug-compilation-dir` is used in another places, not in the 
`SplitDebugName`
(used in invocations for -cc1 and -cc1as:
https://github.com/llvm-mirror/clang/blob/master/tools/driver/cc1as_main.cpp#L234
https://github.com/llvm-mirror/clang/blob/master/lib/Frontend/CompilerInvocation.cpp#L944)

After changes done by this patch, no check-llvm/check-clang tests started to 
fail for me.


This should be NFC change.

`SplitDebugName` started to accept the `Output` that
can be used to simplify the logic a bit, also it
seems that code in `SplitDebugName` that uses
`OPT_fdebug_compilation_dir` is simply dead.

My understanding may be wrong because I am
not very familiar with clang, but I investigated this and
below in the comment, there are my
arguments for that.


https://reviews.llvm.org/D54576

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp


Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -54,7 +54,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 void tools::MinGW::Linker::AddLibGCC(const ArgList &Args,
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -817,7 +817,7 @@
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
   getToolChain().getTriple().isOSLinux())
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 namespace {
Index: lib/Driver/ToolChains/CommonArgs.h
===
--- lib/Driver/ToolChains/CommonArgs.h
+++ lib/Driver/ToolChains/CommonArgs.h
@@ -63,7 +63,7 @@
 const Tool &T);
 
 const char *SplitDebugName(const llvm::opt::ArgList &Args,
-   const InputInfo &Input, const InputInfo &Output);
+   const InputInfo &Output);
 
 void SplitDebugInfo(const ToolChain &TC, Compilation &C, const Tool &T,
 const JobAction &JA, const llvm::opt::ArgL

[PATCH] D54576: [clang] - Simplify tools::SplitDebugName.

[George Rimar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347035: [clang] - Simplify tools::SplitDebugName. (authored 
by grimar, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54576?vs=174193&id=174335#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54576

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
  cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
  cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
  cfe/trunk/lib/Driver/ToolChains/MinGW.cpp


Index: cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
+++ cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
@@ -54,7 +54,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 void tools::MinGW::Linker::AddLibGCC(const ArgList &Args,
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3936,7 +3936,7 @@
   const char *SplitDWARFOut;
   if (SplitDWARF) {
 CmdArgs.push_back("-split-dwarf-file");
-SplitDWARFOut = SplitDebugName(Args, Input, Output);
+SplitDWARFOut = SplitDebugName(Args, Output);
 CmdArgs.push_back(SplitDWARFOut);
   }
 
@@ -5902,7 +5902,7 @@
   if ((getDebugFissionKind(D, Args, A) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {
 CmdArgs.push_back("-split-dwarf-file");
-CmdArgs.push_back(SplitDebugName(Args, Input, Output));
+CmdArgs.push_back(SplitDebugName(Args, Output));
   }
 
   assert(Input.isFilename() && "Invalid input.");
Index: cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
+++ cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
@@ -808,26 +808,15 @@
   return false;
 }
 
-const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
+const char *tools::SplitDebugName(const ArgList &Args,
   const InputInfo &Output) {
   if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
 if (StringRef(A->getValue()) == "single")
   return Args.MakeArgString(Output.getFilename());
 
-  Arg *FinalOutput = Args.getLastArg(options::OPT_o);
-  if (FinalOutput && Args.hasArg(options::OPT_c)) {
-SmallString<128> T(FinalOutput->getValue());
-llvm::sys::path::replace_extension(T, "dwo");
-return Args.MakeArgString(T);
-  } else {
-// Use the compilation dir.
-SmallString<128> T(
-Args.getLastArgValue(options::OPT_fdebug_compilation_dir));
-SmallString<128> F(llvm::sys::path::stem(Input.getBaseInput()));
-llvm::sys::path::replace_extension(F, "dwo");
-T += F;
-return Args.MakeArgString(F);
-  }
+  SmallString<128> T(Output.getFilename());
+  llvm::sys::path::replace_extension(T, "dwo");
+  return Args.MakeArgString(T);
 }
 
 void tools::SplitDebugInfo(const ToolChain &TC, Compilation &C, const Tool &T,
Index: cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
===
--- cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
+++ cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
@@ -63,7 +63,7 @@
 const Tool &T);
 
 const char *SplitDebugName(const llvm::opt::ArgList &Args,
-   const InputInfo &Input, const InputInfo &Output);
+   const InputInfo &Output);
 
 void SplitDebugInfo(const ToolChain &TC, Compilation &C, const Tool &T,
 const JobAction &JA, const llvm::opt::ArgList &Args,
Index: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
@@ -817,7 +817,7 @@
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
   getToolChain().getTriple().isOSLinux())
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 namespace {


Index: cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
+++ cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
@@ -54,7 +54,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 void tools::MinGW::Linker::AddLibGCC(const ArgLi

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

grimar created this revision.
grimar added reviewers: dblaikie, JonasToth.

This is an updated version of the D54576 , 
which was reverted.

Problem was that `SplitDebugName` calls the `InputInfo::getFilename`
which  asserts if `InputInfo` given is not of type `Filename`:

  const char *getFilename() const {
assert(isFilename() && "Invalid accessor.");
return Data.Filename;
  }

At the same time at that point, it can be of type `Nothing` and
we need to use `getBaseInput()` it seems, like original code did.


https://reviews.llvm.org/D55006

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp


Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -54,7 +54,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 void tools::MinGW::Linker::AddLibGCC(const ArgList &Args,
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -817,7 +817,7 @@
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
   getToolChain().getTriple().isOSLinux())
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 namespace {
Index: lib/Driver/ToolChains/CommonArgs.h
===
--- lib/Driver/ToolChains/CommonArgs.h
+++ lib/Driver/ToolChains/CommonArgs.h
@@ -63,7 +63,7 @@
 const Tool &T);
 
 const char *SplitDebugName(const llvm::opt::ArgList &Args,
-   const InputInfo &Input, const InputInfo &Output);
+   const InputInfo &Output);
 
 void SplitDebugInfo(const ToolChain &TC, Compilation &C, const Tool &T,
 const JobAction &JA, const llvm::opt::ArgList &Args,
Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -808,26 +808,18 @@
   return false;
 }
 
-const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
+const char *tools::SplitDebugName(const ArgList &Args,
   const InputInfo &Output) {
+  SmallString<128> F(Output.isFilename()
+ ? Output.getFilename()
+ : llvm::sys::path::stem(Output.getBaseInput()));
+
   if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
 if (StringRef(A->getValue()) == "single")
-  return Args.MakeArgString(Output.getFilename());
+  return Args.MakeArgString(F);
 
-  Arg *FinalOutput = Args.getLastArg(options::OPT_o);
-  if (FinalOutput && Args.hasArg(options::OPT_c)) {
-SmallString<128> T(FinalOutput->getValue());
-llvm::sys::path::replace_extension(T, "dwo");
-return Args.MakeArgString(T);
-  } else {
-// Use the compilation dir.
-SmallString<128> T(
-Args.getLastArgValue(options::OPT_fdebug_compilation_dir));
-SmallString<128> F(llvm::sys::path::stem(Input.getBaseInput()));
-llvm::sys::path::replace_extension(F, "dwo");
-T += F;
-return Args.MakeArgString(F);
-  }
+  llvm::sys::path::replace_extension(F, "dwo");
+  return Args.MakeArgString(F);
 }
 
 void tools::SplitDebugInfo(const ToolChain &TC, Compilation &C, const Tool &T,
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3966,7 +3966,7 @@
   const char *SplitDWARFOut;
   if (SplitDWARF) {
 CmdArgs.push_back("-split-dwarf-file");
-SplitDWARFOut = SplitDebugName(Args, Input, Output);
+SplitDWARFOut = SplitDebugName(Args, Output);
 CmdArgs.push_back(SplitDWARFOut);
   }
 
@@ -5933,7 +5933,7 @@
   if ((getDebugFissionKind(D, Args, A) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {
 CmdArgs.push_back("-split-dwarf-file");
-CmdArgs.push_back(SplitDebugName(Args, Input, Output));
+CmdArgs.push_back(SplitDebugName(Args, Output));
   }
 
   assert(Input.isFilename() && "Invalid input.");


Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -54,7 +54,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

With that the original issue mentioned in the following comment is fixed for me:
https://reviews.llvm.org/rL347035#299232


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

https://reviews.llvm.org/D55006



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

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D55006#1311391 , @dblaikie wrote:

> Cool - thanks! Any chance/way to add a test for this that'd show up sooner 
> than the breakage caused by the previous version?


Maybe, I am not sure. I was not able to trigger this without using the 
`clang-tidy` call yet.


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

https://reviews.llvm.org/D55006



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

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D55006#1311400 , @dblaikie wrote:

> In D55006#1311398 , @grimar wrote:
>
> > In D55006#1311391 , @dblaikie 
> > wrote:
> >
> > > Cool - thanks! Any chance/way to add a test for this that'd show up 
> > > sooner than the breakage caused by the previous version?
> >
> >
> > Maybe, I am not sure. I was not able to trigger this without using the 
> > `clang-tidy` call yet.
>
>
> Any chance other clang tools might reproduce this? clang-check maybe (sort of 
> the most trivial clang tool)?


I'll try to investigate this tomorrow. Never used it I think :)


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

https://reviews.llvm.org/D55006



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

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D55006#1312012 , @JonasToth wrote:

> The issues seems to be resolved for me as well, i will post this patch to the 
> mailing list and ask the other guy if his build is fine, too.


Thanks, Jonas!


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

https://reviews.llvm.org/D55006



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

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

grimar updated this revision to Diff 175843.
grimar added a comment.

- Added the test case.


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

https://reviews.llvm.org/D55006

Files:
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp
  test/Tooling/clang-check-extra-arg.cpp

Index: test/Tooling/clang-check-extra-arg.cpp
===
--- test/Tooling/clang-check-extra-arg.cpp
+++ test/Tooling/clang-check-extra-arg.cpp
@@ -2,4 +2,8 @@
 
 // CHECK: unknown warning option '-Wunimplemented-warning-before'
 // CHECK: unknown warning option '-Wunimplemented-warning'
+
+// Check we do not crash with -extra-arg=-gsplit-dwarf (we did, under linux).
+// RUN: clang-check "%s" -extra-arg=-gsplit-dwarf -- -c
+
 void a(){}
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -54,7 +54,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 void tools::MinGW::Linker::AddLibGCC(const ArgList &Args,
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -817,7 +817,7 @@
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
   getToolChain().getTriple().isOSLinux())
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 namespace {
Index: lib/Driver/ToolChains/CommonArgs.h
===
--- lib/Driver/ToolChains/CommonArgs.h
+++ lib/Driver/ToolChains/CommonArgs.h
@@ -63,7 +63,7 @@
 const Tool &T);
 
 const char *SplitDebugName(const llvm::opt::ArgList &Args,
-   const InputInfo &Input, const InputInfo &Output);
+   const InputInfo &Output);
 
 void SplitDebugInfo(const ToolChain &TC, Compilation &C, const Tool &T,
 const JobAction &JA, const llvm::opt::ArgList &Args,
Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -808,26 +808,18 @@
   return false;
 }
 
-const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
+const char *tools::SplitDebugName(const ArgList &Args,
   const InputInfo &Output) {
+  SmallString<128> F(Output.isFilename()
+ ? Output.getFilename()
+ : llvm::sys::path::stem(Output.getBaseInput()));
+
   if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
 if (StringRef(A->getValue()) == "single")
-  return Args.MakeArgString(Output.getFilename());
+  return Args.MakeArgString(F);
 
-  Arg *FinalOutput = Args.getLastArg(options::OPT_o);
-  if (FinalOutput && Args.hasArg(options::OPT_c)) {
-SmallString<128> T(FinalOutput->getValue());
-llvm::sys::path::replace_extension(T, "dwo");
-return Args.MakeArgString(T);
-  } else {
-// Use the compilation dir.
-SmallString<128> T(
-Args.getLastArgValue(options::OPT_fdebug_compilation_dir));
-SmallString<128> F(llvm::sys::path::stem(Input.getBaseInput()));
-llvm::sys::path::replace_extension(F, "dwo");
-T += F;
-return Args.MakeArgString(F);
-  }
+  llvm::sys::path::replace_extension(F, "dwo");
+  return Args.MakeArgString(F);
 }
 
 void tools::SplitDebugInfo(const ToolChain &TC, Compilation &C, const Tool &T,
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3967,7 +3967,7 @@
   const char *SplitDWARFOut;
   if (SplitDWARF) {
 CmdArgs.push_back("-split-dwarf-file");
-SplitDWARFOut = SplitDebugName(Args, Input, Output);
+SplitDWARFOut = SplitDebugName(Args, Output);
 CmdArgs.push_back(SplitDWARFOut);
   }
 
@@ -5934,7 +5934,7 @@
   if ((getDebugFissionKind(D, Args, A) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {
 CmdArgs.push_back("-split-dwarf-file");
-CmdArgs.push_back(SplitDebugName(Args, Input, Output));
+CmdArgs.push_back(SplitDebugName(Args, Output));
   }
 
   assert(Input.isFilename() && "Invalid input.");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D55006#1316985 , @JonasToth wrote:

> from my side no objections, mailing list did not react AFAIK (just in case 
> your waiting for me until you recommit).


David requested the test case, I added it and now waiting for his approval :]


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

https://reviews.llvm.org/D55006



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

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348352: [clang] - Simplify tools::SplitDebugName. (authored 
by grimar, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55006?vs=175843&id=176782#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55006

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
  cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
  cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
  cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
  cfe/trunk/test/Tooling/clang-check-extra-arg.cpp

Index: cfe/trunk/test/Tooling/clang-check-extra-arg.cpp
===
--- cfe/trunk/test/Tooling/clang-check-extra-arg.cpp
+++ cfe/trunk/test/Tooling/clang-check-extra-arg.cpp
@@ -2,4 +2,8 @@
 
 // CHECK: unknown warning option '-Wunimplemented-warning-before'
 // CHECK: unknown warning option '-Wunimplemented-warning'
+
+// Check we do not crash with -extra-arg=-gsplit-dwarf (we did, under linux).
+// RUN: clang-check "%s" -extra-arg=-gsplit-dwarf -- -c
+
 void a(){}
Index: cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
+++ cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
@@ -54,7 +54,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 void tools::MinGW::Linker::AddLibGCC(const ArgList &Args,
Index: cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
===
--- cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
+++ cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
@@ -63,7 +63,7 @@
 const Tool &T);
 
 const char *SplitDebugName(const llvm::opt::ArgList &Args,
-   const InputInfo &Input, const InputInfo &Output);
+   const InputInfo &Output);
 
 void SplitDebugInfo(const ToolChain &TC, Compilation &C, const Tool &T,
 const JobAction &JA, const llvm::opt::ArgList &Args,
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3949,7 +3949,7 @@
   const char *SplitDWARFOut;
   if (SplitDWARF) {
 CmdArgs.push_back("-split-dwarf-file");
-SplitDWARFOut = SplitDebugName(Args, Input, Output);
+SplitDWARFOut = SplitDebugName(Args, Output);
 CmdArgs.push_back(SplitDWARFOut);
   }
 
@@ -5916,7 +5916,7 @@
   if ((getDebugFissionKind(D, Args, A) == DwarfFissionKind::Split) &&
   (T.isOSLinux() || T.isOSFuchsia())) {
 CmdArgs.push_back("-split-dwarf-file");
-CmdArgs.push_back(SplitDebugName(Args, Input, Output));
+CmdArgs.push_back(SplitDebugName(Args, Output));
   }
 
   assert(Input.isFilename() && "Invalid input.");
Index: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
@@ -817,7 +817,7 @@
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
   getToolChain().getTriple().isOSLinux())
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0], Output));
+   SplitDebugName(Args, Output));
 }
 
 namespace {
Index: cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
+++ cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
@@ -808,26 +808,18 @@
   return false;
 }
 
-const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
+const char *tools::SplitDebugName(const ArgList &Args,
   const InputInfo &Output) {
+  SmallString<128> F(Output.isFilename()
+ ? Output.getFilename()
+ : llvm::sys::path::stem(Output.getBaseInput()));
+
   if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
 if (StringRef(A->getValue()) == "single")
-  return Args.MakeArgString(Output.getFilename());
+  return Args.MakeArgString(F);
 
-  Arg *FinalOutput = Args.getLastArg(options::OPT_o);
-  if (FinalOutput && Args.hasArg(options::OPT_c)) {
-SmallString<128> T(FinalOutput->getValue());
-llvm::sys::path::replace_extension(T, "dwo");
-return Args.MakeArgString(T);
-  } else {
-// Use the compilation dir.
-SmallString<128> T(
-Args.getLastArgValue(options::OPT_fdebug_compilation_dir));
-SmallString<128> F(llvm::sys::path::stem(I

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.
Herald added a subscriber: jdoerfert.
Herald added a project: LLVM.

I am planning to revert it because of 
https://bugs.llvm.org/show_bug.cgi?id=41161


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55006



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar created this revision.
grimar added reviewers: dblaikie, echristo, probinson, compnerd.
Herald added subscribers: JDevlieghere, aprantl.

The DWARF5 specification says(Appendix F.1):

"The sections that do not require relocation, however, **can be written to the 
relocatable object (.o) file but ignored by the
 linker** or they can be written to a separate DWARF object (.dwo) file that 
need not be accessed by the linker."

The first part describes a single file split DWARF feature and there is no way 
to trigger this behavior atm I think. 
Fortunately, no many changes are required to keep *.dwo sections in a .o, the 
patch does that.

Not sure who is an appropriate reviewer for that.


https://reviews.llvm.org/D52296

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s

Index: test/Driver/split-debug.s
===
--- test/Driver/split-debug.s
+++ test/Driver/split-debug.s
@@ -5,6 +5,9 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
 
+// Check we do not pass any -split-dwarf commands to `as` if -gsingle-file-split-dwarf is specified.
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -gsingle-file-split-dwarf -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/Driver/split-debug.c
===
--- test/Driver/split-debug.c
+++ test/Driver/split-debug.c
@@ -5,6 +5,17 @@
 //
 // CHECK-ACTIONS: "-split-dwarf-file" "split-debug.dwo"
 
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -gsingle-file-split-dwarf -c -### %s 2> %t
+// RUN: FileCheck -check-prefix=CHECK-ACTIONS-SINGLE-SPLIT < %t %s
+//
+// CHECK-ACTIONS-SINGLE-SPLIT: "-single-file-split-dwarf"
+// CHECK-ACTIONS-SINGLE-SPLIT: "-split-dwarf-file" "split-debug.o"
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -gsingle-file-split-dwarf -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"
+
 
 // RUN: %clang -target x86_64-macosx -gsplit-dwarf -c -### %s 2> %t
 // RUN: FileCheck -check-prefix=CHECK-NO-ACTIONS < %t %s
Index: test/CodeGen/split-debug-single-file.c
===
--- test/CodeGen/split-debug-single-file.c
+++ test/CodeGen/split-debug-single-file.c
@@ -0,0 +1,16 @@
+// REQUIRES: x86-registered-target
+
+// Testing to ensure -single-file-split-dwarf 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 -split-dwarf-file %t.o -emit-obj -single-file-split-dwarf -o %t.o %s
+//  RUN: llvm-objdump -section-headers %t.o | FileCheck --check-prefix=SINGLE-FILE-SPLIT %s
+//  SINGLE-FILE-SPLIT: .dwo
+
+// Testing to ensure that the dwo name gets output into the compile unit.
+//  RUN: %clang_cc1 -debug-info-kind=limited -split-dwarf-file foo.o -single-file-split-dwarf \
+//  RUN:   -S -emit-llvm -o - %s | FileCheck %s --check-prefix=DWONAME
+//  DWONAME: !DICompileUnit({{.*}}, splitDebugFilename: "foo.o"
+
+int main (void) {
+  return 0;
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -597,6 +597,8 @@
   Opts.EnableSplitDwarf = Args.hasArg(OPT_enable_split_dwarf);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
+  Opts.SingleFileSplitDwarf = Args.hasArg(OPT_single_file_split_dwarf);
+
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
   Opts.DebugExplicitImport = Args.hasArg(OPT_dwarf_explicit_import);
   Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -52,7 +52,7 @@
 
   if (Args.hasArg(options::OPT_gsplit_dwarf))
 SplitDebugInfo(getToolChain(), C, *this, JA, Args, Output,
-   SplitDebugName(Args, Inputs[0]));
+   SplitDebugName(Args, Inputs[0], Output));
 }
 
 void tools::MinGW::Linker::AddLibGCC(const ArgList &Args,
Index: lib/Driver/ToolChains/

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

We want to take the benefit that debug fission provides, but keep the debug 
information in the objects.
That is a bit less intrusive way than using regular split dwarf flow.

For a regular build and debug cycle that allows the linker to do minimal 
processing of the .debug_str.
Since .debug_str.dwo is ignored, that allows to significantly reduce the link 
time,
as strings processing is slow and string sections are large by themselves to 
handle them.

And at the same time the build system still does not see anything 
unusual/strange
as it sees only .o files like it is just a regular non-split-dwarf flow.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

To summarise:

- It shares most of the benefits with the .dwo solution.
- Unlike .dwo, there is no need to split the file and it is friendlier to other 
tools (ccache, distcc, etc)
- But unlike .dwo a distributed build system has to copy larger .o files.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In https://reviews.llvm.org/D52296#1243688, @echristo wrote:

> In https://reviews.llvm.org/D52296#1241928, @probinson wrote:
>
> > Do we generate the .dwo file directly these days?  If not, I can imagine 
> > wanting to avoid the overhead of the objcopy hack; as long as the linker is 
> > smart enough not to bother with the .debug_*.dwo sections this seems like a 
> > build-time win.
>
>
> We do generate them generically with no objcopy hack.


Eric, could you elaborate for me your position, please

> As far as the standard text here, IMO it was just there in case people didn't 
> have an objcopy around or don't want to split it.

Yeah, we do not want to split it and I see no other way to say clang to keep 
them in a .o files rather than introducing the new flag.
Am I missing something?

> I'm not sure why we would want the ability. 
>  That said, if we do I'd rather have it as dwarf5 without split-dwarf as an 
> option rather than a -gsingle-file-split-dwarf option.

What do you mean as "dwarf5 without split-dwarf as an option" here? Do you mean 
to do split-dwarf by default? It is orthogonal to what this patch does.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Ping.


https://reviews.llvm.org/D52296



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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

Ping.


https://reviews.llvm.org/D52296



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

[PATCH] D65564: Improve raw_ostream so that you can "write" colors using operator<

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

It looks good to me. A minor nit is inlined.




Comment at: clang/tools/diagtool/TreeView.cpp:30
 
-  TreePrinter(llvm::raw_ostream &out)
-  : out(out), ShowColors(hasColors(out)), Internal(false) {}
-
-  void setColor(llvm::raw_ostream::Colors Color) {
-if (ShowColors)
-  out << llvm::sys::Process::OutputColor(Color, false, false);
-  }
-
-  void resetColor() {
-if (ShowColors)
-  out << llvm::sys::Process::ResetColor();
+  TreePrinter(llvm::raw_ostream &out) : out(out), Internal(false) {
+if (&out != &llvm::errs() && &out != &llvm::outs())

nit: Seems `out` should be uppercase here.
(I see it was like that before your changes, but seems you touching all the 
places where it was used, so seems you can fix it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65564



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

[PATCH] D69043: [RFC] Adding time-trace to LLD?

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D69043#1714134 , @russell.gallop 
wrote:

> In D69043#1714060 , @ruiu wrote:
>
> > (I couldn't download your file because the server times out perhaps due to 
> > the file size.)
>
>
> That's odd. I can download it okay from here.


It probably was a temporary issue. I was unable to download it ~2h ago too, but 
now it works fine for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69043



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

[PATCH] D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D77184#1959351 , @andrewng wrote:

> The following patch fixes my issues on Windows, but I haven't tested that it 
> doesn't break anything else:
>
>   diff --git a/llvm/cmake/modules/AddLLVM.cmake 
> b/llvm/cmake/modules/AddLLVM.cmake
>   index 91bec7d8081..f630af211fd 100644
>   --- a/llvm/cmake/modules/AddLLVM.cmake
>   +++ b/llvm/cmake/modules/AddLLVM.cmake
>   @@ -1495,7 +1495,7 @@ function(configure_lit_site_cfg site_in site_out)
>string(REPLACE ";" "\\;" ARG_PATH_VALUES_ESCAPED "${ARG_PATH_VALUES}")
>get_filename_component(OUTPUT_DIR ${site_out} DIRECTORY)
>execute_process(COMMAND "${PYTHON_EXECUTABLE}" "-c"
>   -  "import os, sys; sys.stdout.write(';'.join(os.path.relpath(p, 
> sys.argv[1]).replace(os.sep, '/') if p else '' for p in 
> sys.argv[2].split(';')))"
>   +  "import os, sys; drive = os.path.splitdrive(sys.argv[1])[0]; 
> sys.stdout.write(';'.join('' if not p else p if os.path.splitdrive(p)[0] != 
> drive else os.path.relpath(p, sys.argv[1]).replace(os.sep, '/') for p in 
> sys.argv[2].split(';')))"
>  ${OUTPUT_DIR}
>  ${ARG_PATH_VALUES_ESCAPED}
>  OUTPUT_VARIABLE ARG_PATH_VALUES_RELATIVE)
>


This helped me, thanks for posting this! (I've just updated and faced the build 
issue on windows. Seems it doesn't like that I build it on `D:` drive)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77184



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

[PATCH] D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D77184#1960437 , @thakis wrote:

> grimar, andrewng: You both have checkout and build on different drives too, 
> yes?


Nope. I think my configuration is just normal, except I do not use "C:".
My checkout is "D:\Work3\LLVM\llvm-project" and the build folder is inside: 
"D:\Work3\LLVM\llvm-project\build".

(Just in case, to clarify: I have no issues with building the todays LLVM).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77184



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

[PATCH] D77484: [Vector] Pass VectLib to LTO backend so TLI build correct vector function list

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: lld/ELF/Options.td:491
   HelpText<"Sample profile file path">;
+def lto_vector_library: J<"lto-vector-library=">,
+  HelpText<"Vector functions library">;

You need to update the documentation either (`lld/docs/ld.lld.1`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77484



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

[PATCH] D78016: [ADT/STLExtras.h] - Add llvm::is_sorted wrapper and update callers.

[George Rimar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1647ff6e2753: [ADT/STLExtras.h] - Add llvm::is_sorted 
wrapper and update callers. (authored by grimar).
Herald added subscribers: cfe-commits, jrtc27.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78016

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/Format/Format.cpp
  clang/lib/Format/SortJavaScriptImports.cpp
  clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  clang/lib/Tooling/Syntax/Tokens.cpp
  lld/wasm/InputFiles.cpp
  llvm/include/llvm/ADT/CoalescingBitVector.h
  llvm/include/llvm/ADT/STLExtras.h
  llvm/include/llvm/CodeGen/LiveInterval.h
  llvm/lib/Analysis/CFLAndersAliasAnalysis.cpp
  llvm/lib/Analysis/LoopCacheAnalysis.cpp
  llvm/lib/Analysis/TargetLibraryInfo.cpp
  llvm/lib/Bitcode/Writer/ValueEnumerator.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
  llvm/lib/Frontend/OpenMP/OMPContext.cpp
  llvm/lib/IR/AsmWriter.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/MC/MCSubtargetInfo.cpp
  llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/Mips/MipsCCState.cpp
  llvm/lib/Target/X86/X86EvexToVex.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86InstrFMA3Info.cpp
  llvm/lib/Target/X86/X86InstrFoldTables.cpp
  llvm/lib/Target/X86/X86IntrinsicsInfo.h
  llvm/lib/Transforms/IPO/CalledValuePropagation.cpp
  llvm/tools/llvm-exegesis/lib/Target.cpp
  llvm/tools/llvm-objcopy/ELF/Object.cpp
  llvm/tools/llvm-objcopy/MachO/MachOLayoutBuilder.cpp
  llvm/unittests/ADT/SimpleIListTest.cpp
  llvm/unittests/Support/ParallelTest.cpp

Index: llvm/unittests/Support/ParallelTest.cpp
===
--- llvm/unittests/Support/ParallelTest.cpp
+++ llvm/unittests/Support/ParallelTest.cpp
@@ -31,7 +31,7 @@
 i = dist(randEngine);
 
   sort(parallel::par, std::begin(array), std::end(array));
-  ASSERT_TRUE(std::is_sorted(std::begin(array), std::end(array)));
+  ASSERT_TRUE(llvm::is_sorted(array));
 }
 
 TEST(Parallel, parallel_for) {
Index: llvm/unittests/ADT/SimpleIListTest.cpp
===
--- llvm/unittests/ADT/SimpleIListTest.cpp
+++ llvm/unittests/ADT/SimpleIListTest.cpp
@@ -436,8 +436,8 @@
 // Check setup.
 EXPECT_EQ(4u, L1.size());
 EXPECT_EQ(6u, L2.size());
-EXPECT_TRUE(std::is_sorted(L1.begin(), L1.end()));
-EXPECT_TRUE(std::is_sorted(L2.begin(), L2.end()));
+EXPECT_TRUE(llvm::is_sorted(L1));
+EXPECT_TRUE(llvm::is_sorted(L2));
 
 // Merge.
 auto &LHS = IsL1LHS ? L1 : L2;
@@ -445,7 +445,7 @@
 LHS.merge(RHS);
 EXPECT_TRUE(RHS.empty());
 EXPECT_FALSE(LHS.empty());
-EXPECT_TRUE(std::is_sorted(LHS.begin(), LHS.end()));
+EXPECT_TRUE(llvm::is_sorted(LHS));
 auto I = LHS.begin();
 for (Node &N : Ns)
   EXPECT_EQ(&N, &*I++);
@@ -473,8 +473,8 @@
 // Check setup.
 EXPECT_EQ(3u, L1.size());
 EXPECT_EQ(2u, L2.size());
-EXPECT_TRUE(std::is_sorted(L1.begin(), L1.end(), makeFalse));
-EXPECT_TRUE(std::is_sorted(L2.begin(), L2.end(), makeFalse));
+EXPECT_TRUE(llvm::is_sorted(L1, makeFalse));
+EXPECT_TRUE(llvm::is_sorted(L2, makeFalse));
   };
 
   // Merge.  Should be stable.
@@ -482,7 +482,7 @@
   L1.merge(L2, makeFalse);
   EXPECT_TRUE(L2.empty());
   EXPECT_FALSE(L1.empty());
-  EXPECT_TRUE(std::is_sorted(L1.begin(), L1.end(), makeFalse));
+  EXPECT_TRUE(llvm::is_sorted(L1, makeFalse));
   auto I = L1.begin();
   EXPECT_EQ(&Ns[0], &*I++);
   EXPECT_EQ(&Ns[3], &*I++);
@@ -497,7 +497,7 @@
   L2.merge(L1, makeFalse);
   EXPECT_TRUE(L1.empty());
   EXPECT_FALSE(L2.empty());
-  EXPECT_TRUE(std::is_sorted(L2.begin(), L2.end(), makeFalse));
+  EXPECT_TRUE(llvm::is_sorted(L2, makeFalse));
   I = L2.begin();
   EXPECT_EQ(&Ns[1], &*I++);
   EXPECT_EQ(&Ns[2], &*I++);
@@ -521,7 +521,7 @@
 // Check setup.
 EXPECT_EQ(4u, L1.size());
 EXPECT_TRUE(L2.empty());
-EXPECT_TRUE(std::is_sorted(L1.begin(), L1.end()));
+EXPECT_TRUE(llvm::is_sorted(L1));
 
 // Merge.
 auto &LHS = IsL1LHS ? L1 : L2;
@@ -529,7 +529,7 @@
 LHS.merge(RHS);
 EXPECT_TRUE(RHS.empty());
 EXPECT_FALSE(LHS.empty());
-EXPECT_TRUE(std::is_sorted(LHS.begin(), LHS.end()));
+EXPECT_TRUE(llvm::is_sorted(LHS));
 auto I = LHS.begin();
 for (Node &N : Ns)
   EXPECT_EQ(&N, &*I++);
@@ -554,11 +554,11 @@
 
   // Check setup.
   EXPECT_EQ(10u, L.size());
-  EXPECT_FALSE(std::is_sorted(L.begin(), L.end()));
+  EXPECT_FALSE(llvm::is_sorted(L));
 
   // Sort.
   L.sort();
-  EXPECT_TRUE(std::is_sorted(L.begin(), L.end()));
+  EXPECT_TRUE(llvm::is_sorted(L));
   auto I = L.begin();
   for (Node &N

[PATCH] D78024: [FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D78024#2087800 , @Higuoxing wrote:

> > btw, do you know why FileCheck seems intentionally allows the case when 
> > --check-prefixes=KNOWN,UNKNOWN?
> >  I've mentioned in the description of D78110 
> >  that there are about 1000 tests that have 
> > this. but is it a feature or a something that we might want to fix?>
>
> I noticed this strange behavior of `FileCheck` as well. When I try to fix it, 
> there are lots of test failures. /subscribe


Yeah. To fix it we should prepare patches for those ~1000 tests in

> CodeGen/*
>  DebugInfo/X86/*
>  Instrumentation/*
>  Linker/*
>  MC/*
>  Other/*
>  Transforms/*

(and few other places) first. When I investigated them, I came to conclusion 
they just have unused prefixes
for no solid reason. I am not an expert in those folders/areas though. I  
thought about starting from a patch for one of those folders to see
if it will be approved without problems by people who work on the features 
there. But I had no chance to work on that yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78024



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

[PATCH] D78024: [FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix.

[George Rimar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG65a2de7e6c98: [FileCheck] - Fix the false positive when 
-implicit-check-not is used with an… (authored by grimar).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D78024?vs=257671&id=258021#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78024

Files:
  clang/test/CodeGen/catch-implicit-conversions-basics-negatives.c
  llvm/include/llvm/Support/FileCheck.h
  llvm/lib/Support/FileCheck.cpp
  llvm/test/FileCheck/implicit-check-not.txt
  llvm/test/tools/llvm-objcopy/MachO/strip-debug.test

Index: llvm/test/tools/llvm-objcopy/MachO/strip-debug.test
===
--- llvm/test/tools/llvm-objcopy/MachO/strip-debug.test
+++ llvm/test/tools/llvm-objcopy/MachO/strip-debug.test
@@ -3,7 +3,7 @@
 # RUN: yaml2obj %p/Inputs/strip-all-with-dwarf.yaml -o %t
 
 # RUN: llvm-objcopy --strip-debug %t %t.stripped
-# RUN: llvm-readobj --sections %t.stripped | FileCheck /dev/null --check-prefix=NODWARF \
+# RUN: llvm-readobj --sections %t.stripped | FileCheck /dev/null \
 # RUN:   --implicit-check-not='Name: __debug' --implicit-check-not='Name: __apple'
 
 ## Make sure that all symbols are kept.
Index: llvm/test/FileCheck/implicit-check-not.txt
===
--- llvm/test/FileCheck/implicit-check-not.txt
+++ llvm/test/FileCheck/implicit-check-not.txt
@@ -1,4 +1,16 @@
 ; RUN: sed 's#^;.*##' %s | FileCheck -check-prefix=CHECK-PASS -implicit-check-not=warning: %s
+
+; Check we report an error when an unknown prefix is used together with `-implicit-check-not`.
+; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=UNKNOWN-PREFIX -implicit-check-not=abc %s 2>&1 | FileCheck %s -DPREFIX=UNKNOWN-PREFIX -check-prefix CHECK-PREFIX-ERROR
+; CHECK-PREFIX-ERROR: error: no check strings found with prefix '[[PREFIX]]:'
+
+; Check we report an error when the "CHECK" prefix is used explicitly with `-implicit-check-not`, but not present in the input.
+; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK -implicit-check-not=abc %s 2>&1 | FileCheck %s -DPREFIX=CHECK -check-prefix CHECK-PREFIX-ERROR
+
+; Check we allow using `-implicit-check-not` when there is no `-check-prefix` specified and there
+; is no default `CHECK` line in an input.
+; RUN: sed 's#^;.*##' %s | FileCheck -implicit-check-not="unique_string" %s
+
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL1 -implicit-check-not=warning: %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR1
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL2 -implicit-check-not=warning: %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR2
 ; RUN: sed 's#^;.*##' %s | %ProtectFileCheckOutput not FileCheck -check-prefix=CHECK-FAIL3 -implicit-check-not=warning: %s 2>&1 | FileCheck %s -check-prefix CHECK-ERROR3
Index: llvm/lib/Support/FileCheck.cpp
===
--- llvm/lib/Support/FileCheck.cpp
+++ llvm/lib/Support/FileCheck.cpp
@@ -1305,6 +1305,7 @@
   // found.
   unsigned LineNumber = 1;
 
+  bool FoundUsedPrefix = false;
   while (1) {
 Check::FileCheckType CheckTy;
 
@@ -1315,6 +1316,8 @@
 FindFirstMatchingPrefix(PrefixRE, Buffer, LineNumber, CheckTy);
 if (UsedPrefix.empty())
   break;
+FoundUsedPrefix = true;
+
 assert(UsedPrefix.data() == Buffer.data() &&
"Failed to move Buffer's start forward, or pointed prefix outside "
"of the buffer!");
@@ -1398,16 +1401,10 @@
 DagNotMatches = ImplicitNegativeChecks;
   }
 
-  // Add an EOF pattern for any trailing --implicit-check-not/CHECK-DAG/-NOTs,
-  // and use the first prefix as a filler for the error message.
-  if (!DagNotMatches.empty()) {
-CheckStrings->emplace_back(
-Pattern(Check::CheckEOF, PatternContext.get(), LineNumber + 1),
-*Req.CheckPrefixes.begin(), SMLoc::getFromPointer(Buffer.data()));
-std::swap(DagNotMatches, CheckStrings->back().DagNotStrings);
-  }
-
-  if (CheckStrings->empty()) {
+  // When there are no used prefixes we report an error except in the case that
+  // no prefix is specified explicitly but -implicit-check-not is specified.
+  if (!FoundUsedPrefix &&
+  (ImplicitNegativeChecks.empty() || !Req.IsDefaultCheckPrefix)) {
 errs() << "error: no check strings found with prefix"
<< (Req.CheckPrefixes.size() > 1 ? "es " : " ");
 auto I = Req.CheckPrefixes.begin();
@@ -1423,6 +1420,15 @@
 return true;
   }
 
+  // Add an EOF pattern for any trailing --implicit-check-not/CHECK-DAG/-NOTs,
+  // and use the first prefix as a filler for the error message.
+  if (!DagNotMatches.empty()) {
+   

[PATCH] D78024: [FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix.

[George Rimar via Phabricator via cfe-commits]

grimar marked an inline comment as done.
grimar added inline comments.



Comment at: llvm/lib/Support/FileCheck.cpp:1375-1377
+  // We do not allow using -implicit-check-not when an explicitly specified
+  // check prefix is not present in the input buffer.
+  if ((Req.IsDefaultCheckPrefix || FoundUsedPrefix) && !DagNotMatches.empty()) 
{

jdenny wrote:
> grimar wrote:
> > jdenny wrote:
> > > I find this logic confusing.  Its goal appears to be to constrain when 
> > > `DagNotMatches` are added to `CheckStrings`.  However, the real goal is 
> > > to constrain when FileCheck complains that there are no used prefixes.  
> > > Would the following logic work?
> > > 
> > > ```
> > > if (!FoundUsedPrefix && (ImplicitNegativeChecks.empty() || 
> > > !Req.IsDefaultCheckPrefix)) {
> > >   errs() << "error: no check strings found with prefix"
> > >   . . .
> > > }
> > > if (!DagNotMatches.empty()) {
> > >   CheckStrings->emplace_back(
> > >   . . .
> > > }
> > > ```
> > This looks better and works, thanks!
> > 
> > (The code above has `DagNotMatches = ImplicitNegativeChecks;` line which is 
> > also a bit confusing probably)
> > This looks better and works, thanks!
> 
> Thanks for making that change.
> 
> > (The code above has DagNotMatches = ImplicitNegativeChecks; line which is 
> > also a bit confusing probably)
> 
> I'm fine with that one.  DagNotMatches starts out as ImplicitNegativeChecks, 
> and CHECK-NOTs might be added later.
> 
> I think the second sentence in the following comment still reflects the old 
> logic:
> 
> ```
>   // When there are no used prefixes we report an error.
>   // We do not allow using -implicit-check-not when an explicitly specified
>   // check prefix is not present in the input buffer.
> ```
> 
> Something like the following seems better to me:
> 
> ```
>   // When there are no used prefixes we report an error except in the case 
> that
>   // no prefix is specified explicitly but -implicit-check-not is specified.
> ```
> 
> the real goal is to constrain when FileCheck complains that there are no used 
> prefixes

btw, do you know why FileCheck seems intentionally allows the case when 
`--check-prefixes=KNOWN,UNKNOWN`?
I've mentioned in the description of D78110 that there are about 1000 tests 
that have this. but is it a feature or a something that we might want to fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78024



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

[PATCH] D78024: [FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix.

[George Rimar via Phabricator via cfe-commits]

grimar marked an inline comment as done.
grimar added inline comments.



Comment at: llvm/lib/Support/FileCheck.cpp:1375-1377
+  // We do not allow using -implicit-check-not when an explicitly specified
+  // check prefix is not present in the input buffer.
+  if ((Req.IsDefaultCheckPrefix || FoundUsedPrefix) && !DagNotMatches.empty()) 
{

jdenny wrote:
> grimar wrote:
> > jdenny wrote:
> > > grimar wrote:
> > > > jdenny wrote:
> > > > > I find this logic confusing.  Its goal appears to be to constrain 
> > > > > when `DagNotMatches` are added to `CheckStrings`.  However, the real 
> > > > > goal is to constrain when FileCheck complains that there are no used 
> > > > > prefixes.  Would the following logic work?
> > > > > 
> > > > > ```
> > > > > if (!FoundUsedPrefix && (ImplicitNegativeChecks.empty() || 
> > > > > !Req.IsDefaultCheckPrefix)) {
> > > > >   errs() << "error: no check strings found with prefix"
> > > > >   . . .
> > > > > }
> > > > > if (!DagNotMatches.empty()) {
> > > > >   CheckStrings->emplace_back(
> > > > >   . . .
> > > > > }
> > > > > ```
> > > > This looks better and works, thanks!
> > > > 
> > > > (The code above has `DagNotMatches = ImplicitNegativeChecks;` line 
> > > > which is also a bit confusing probably)
> > > > This looks better and works, thanks!
> > > 
> > > Thanks for making that change.
> > > 
> > > > (The code above has DagNotMatches = ImplicitNegativeChecks; line which 
> > > > is also a bit confusing probably)
> > > 
> > > I'm fine with that one.  DagNotMatches starts out as 
> > > ImplicitNegativeChecks, and CHECK-NOTs might be added later.
> > > 
> > > I think the second sentence in the following comment still reflects the 
> > > old logic:
> > > 
> > > ```
> > >   // When there are no used prefixes we report an error.
> > >   // We do not allow using -implicit-check-not when an explicitly 
> > > specified
> > >   // check prefix is not present in the input buffer.
> > > ```
> > > 
> > > Something like the following seems better to me:
> > > 
> > > ```
> > >   // When there are no used prefixes we report an error except in the 
> > > case that
> > >   // no prefix is specified explicitly but -implicit-check-not is 
> > > specified.
> > > ```
> > > 
> > > the real goal is to constrain when FileCheck complains that there are no 
> > > used prefixes
> > 
> > btw, do you know why FileCheck seems intentionally allows the case when 
> > `--check-prefixes=KNOWN,UNKNOWN`?
> > I've mentioned in the description of D78110 that there are about 1000 tests 
> > that have this. but is it a feature or a something that we might want to 
> > fix?
> There was a long discussion about diagnostics like that over a year ago.  I 
> believe we described that case as prefixes that are defined but not used.
> 
> In some of my downstream work, I often create FileCheck prefix schemes 
> reflecting various combinations of features I want to test.  For example: 
> FOO, BAR, FOO-AND-BAR, FOO-NOT-BAR, etc.  It's far easier to maintain those 
> tests if I list all appropriate prefixes for each FileCheck command even if 
> some prefixes are not currently used in the test file.  I don't know if this 
> use case exists upstream right now.
> 
> Also, when developing or debugging tests involving many prefixes and many 
> FileCheck commands, I think it could be painful to have to update all 
> FileCheck commands every time you temporarily remove the only uses of some 
> prefix.
> 
> If you pursue this diagnostic, please make it optional, even if on by 
> default.  At one time, we discussed a warning system for such diagnostics.  
> Warnings could be configurable via command-line options, possibly passed via 
> the FILECHECK_OPTS env var so that it's easy to relax them during debugging 
> or based on a test suite's specific needs.
> 
> @probinson and @SjoerdMeijer might want to chime in as they were also part of 
> previous discussions.
I see, thanks for details.

D78110 and this patch (which revealed rG09331fd7) showed that we had test cases 
committed which
had unknown prefixes placed by mistake and hence false positives.
I remember how accidentally pushed on review the patch with the same issue and 
nobody noticed..
I just think that making FileCheck stricter could prevent such issues.

If we'll have a consensus about making `--check-prefixes` stricter I'll be 
happy to work on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78024



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

[PATCH] D78024: [FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In D78024#1989287 , @probinson wrote:

> I'm just reading this review for the first time, and my thought was, why is 
> this interacting with implicit-check-not?


It was interacting (this patch was committed and fixed the issue), because the 
code used the logic to add an EOF pattern
for "any trailing --implicit-check-not/CHECK-DAG/-NOTs" before the error about 
"no check strings found with prefix" was reported.
I.e. this logic suppressed the error, because `CheckStrings` was not empty 
anymore:

if (CheckStrings->empty()) {
  errs() << "error: no check strings found with prefix"
  ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78024



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

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:16079
+  auto Style9 = getStyle("file", "/d/.clang-format", "LLVM", "", &FS, true);
+  ASSERT_TRUE((bool)Style9);
 }

It looks like it is very similar to "Test 7:" and can be a part of it?
E.g:


```
...
llvm::consumeError(Style7a.takeError());

// 
auto Style7b = getStyle("file", "/d/.clang-format", "LLVM", "", &FS, true);
ASSERT_TRUE((bool)Style8);
```



Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:39
+
+TEST(ObjectYAML, UnkownOption) {
+  std::string InputYAML = "Binary: \n"

Unk**n**ownOption



Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:40
+TEST(ObjectYAML, UnkownOption) {
+  std::string InputYAML = "Binary: \n"
+  "InvalidKey: InvalidValue";

`std::string`->`StringRef`?



Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:41
+  std::string InputYAML = "Binary: \n"
+  "InvalidKey: InvalidValue";
+  BinaryHolder BH;

Will this work if we switch the order of `InvalidKey` and `Binary`?

```
  std::string InputYAML = "InvalidKey: InvalidValue\n"
  "Binary: ";
```

I expect that yes and I think it is reasonable to do it to show that we don't 
stop parsing an YAML
when there is an unknown key.



Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:44
+  llvm::yaml::Input Input(InputYAML);
+  // test 1: default in trying to parse invalid key is an error case
+  Input >> BH;

No full stop at the end.



Comment at: llvm/unittests/ObjectYAML/YAMLTest.cpp:57
+  EXPECT_EQ(BH2.Binary, BH_GT.Binary);
+  EXPECT_EQ(Input2.error().value(), 0);
+}

Do you need `BH_GT`? Can it be just:

```
  // test 2: only warn about invalid key if actively set.
  llvm::yaml::Input Input2(InputYAML);
  BinaryHolder BH2;
  Input2.setAllowUnknownKeys(true);
  Input2 >> BH2;
  EXPECT_EQ(Input2.error().value(), 0);
  EXPECT_EQ(BH2.Binary, yaml::BinaryRef(""));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86137

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

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

[George Rimar via Phabricator via cfe-commits]

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

LGTM. It worth wainting for a second approvement and/or other comments to 
verify that people are happy with doing this for `clang-format`.




Comment at: clang/unittests/Format/FormatTest.cpp:16062
   FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int 
i;")));
   auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", &FS);
   ASSERT_FALSE((bool)Style7);

nit: since you have `Style7b`, this perhaps should be `Style7a`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86137

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

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

I am not familar with `clang-format`, but have a few comments inlined about the 
rest.
I think the new `setIgnoreUnknown` YAMLlib API is probably OK generally.
I'd perhaps call it differently, e.g. `setAllowUnknownKeys` though.

Also, I think you need to add a unit testing for the new functionality.
It seems appropriate places are: `clang\unittests\Format\FormatTest.cpp` (to 
test clang-format)
and `llvm\unittests\ObjectYAML\YAMLTest.cpp` (to test new YAML 
`Input::setIgnoreUnknown` API)

I'll add people who might have something useful to say too.




Comment at: llvm/include/llvm/Support/YAMLTraits.h:1508
 
+  void setIgnoreUnknown(bool) override;
+

I'd add a parameter name here. Seems the style is mixed in this file,
but it is a public member, and having no names for arguments actually
reads bad I think. Not sure why it was done initially.

Probably the same applies for `setIgnoreUnknown` in the base class.



Comment at: llvm/lib/Support/YAMLTraits.cpp:199
+  if (IgnoreUnkown)
+return;
   for (const auto &NN : MN->Mapping) {

fodinabor wrote:
> MyDeveloperDay wrote:
> > do we want to flat out ignore or just report but not fatally. (just a 
> > thought) silent failures are hard to diagnose
> true.. don't know what's the best option?
> 
> keep it as a printed out error and just don't return an error code on exit? 
> This option would make it a clang-format only change, but feels really dirty.
> 
> Otherwise I'd have to dig my way through to `llvm::yaml::Stream::printError` 
> (or maybe rather add a `printWarning`) to conditionally change the message 
> type for the ignore case.
Yes, I think we might want to introduce a method, like `Input::reportWarning`
which will call ` Strm->printError(node, message);`, but will not set a `EC`.
Also, it should print "warning: ..." instead of "error: ..." prefix somehow.




Comment at: llvm/lib/Support/YAMLTraits.cpp:742
 
+void Output::setIgnoreUnknown(bool Value) {}
+

You don't actually need it for `Output`, right?
I think instead you can have a default implementation in the base class, which 
should call `llvm_unreachable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86137

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

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: llvm/lib/Support/YAMLTraits.cpp:204
 if (!is_contained(MN->ValidKeys, NN.first())) {
-  setError(NN.second.get(), Twine("unknown key '") + NN.first() + "'");
-  break;
+  auto ReportNode = NN.second.get();
+  auto ReportMessage = Twine("unknown key '") + NN.first() + "'";

Please don't use `auto` when the variable type is not obvious.
Use the real type name instead.



Comment at: llvm/lib/Support/YAMLTraits.cpp:205
+  auto ReportNode = NN.second.get();
+  auto ReportMessage = Twine("unknown key '") + NN.first() + "'";
+  if (!AllowUnknownKeys) {

The same here, but also the result type here is `Twine`, and you shouldn't use 
it like this. Documentation says:
"A Twine is not intended for use directly and should not be stored, its 
implementation relies on the ability to store pointers to temporary stack 
objects which may be deallocated at the end of a statement. Twines should only 
be used accepted as const references in arguments, when an API wishes to accept 
possibly-concatenated strings."
(https://llvm.org/doxygen/classllvm_1_1Twine.html#details)

You can probably inline it or use `std::string`, since this is a error path 
only code.



Comment at: llvm/lib/Support/YAMLTraits.cpp:387
+
+void Input::reportWarning(Node *node, const Twine &message) {
+  Strm->printError(node, message, SourceMgr::DK_Warning);

Why do you need both `reportWarning` methods? I think you can have only one for 
now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86137

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

As far I understand, looking on the description of D94239 
, the message on z/OS looks like "EDC5129I No 
such file or directory.".
I guess the `EDC5129I` is a stable error code? So why not to check for a 
possible `EDC5129I` prefix word instead of `.*`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

I like this approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95246

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

[PATCH] D68049: Propeller: Clang options for basic block sections

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:120
 
+  std::string BasicBlockSections;
+

MaskRay wrote:
> Comment its allowed values ("all", "labels", "none")
I'd suggest to rewrite it somehow. This set of values did not help me to 
understand what is this field for. The comment could probably be (for example): 
"This is a field for Allowed values are:"



Comment at: clang/lib/CodeGen/BackendUtil.cpp:444
+  while ((std::getline(fin, line)).good()) {
+StringRef S(line);
+// Lines beginning with @, # are not useful here.

Something is wrong with the namings (I am not an expert in lib/CodeGen), but 
you are mixing lower vs upper case styles: "fin", "line", "S", "R". Seems the 
code around prefers upper case.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:450
+  break;
+if (S.consume_front("!")) {
+  if (fi != Options.BBSectionsList.end())

```
if (S.empty() || S[0] == '@' || S[0] == '#')
  continue;
if (!S.consume_front("!") || S.empty())
  break;
if (S.consume_front("!")) {
```

It is looks a bit strange. See: you are testing `S.empty()` condition twice and 
you are checking `S.consume_front("!")` twice.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:461
+  fi = R.first;
+  assert(R.second);
+}

It seems this assert can be triggered for a wrong user input?



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:964
+<< Opts.BBSections;
+  }
+

Doesn't seem you need "{}" around this lines. (Its a single call and looks 
inconsistent with the code around).
(The same for the change above)


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

https://reviews.llvm.org/D68049



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

[PATCH] D72829: Implement -fsemantic-interposition

[George Rimar via Phabricator via cfe-commits]

grimar added inline comments.



Comment at: llvm/lib/IR/Module.cpp:558
+bool Module::getSemanticInterposition() const {
+  auto *Val =
+  cast_or_null(getModuleFlag("SemanticInterposition"));

A minor nit about style:
This probably would be nicer as

```
  if (auto *Val = cast_or_null(
  getModuleFlag("SemanticInterposition")))
return cast(Val->getValue())->getZExtValue();
  return false;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72829



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

[PATCH] D28807: [Clang] - Update code to match upcoming llvm::zlib API.

[George Rimar via Phabricator via cfe-commits]

grimar created this revision.

https://reviews.llvm.org/D28684 changed llvm::zlib to return Error instead of 
Status.
It was accepted and committed in r292214, but then reverted in r292217
because I missed that clang code also needs to be updated.

Patch do that.


https://reviews.llvm.org/D28807

Files:
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -73,6 +73,7 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/EndianStream.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/OnDiskHashTable.h"
@@ -1986,6 +1987,30 @@
 free(const_cast(SavedStrings[I]));
 }
 
+static void emitBlob(llvm::BitstreamWriter &Stream, StringRef Blob,
+ unsigned SLocBufferBlobCompressedAbbrv,
+ unsigned SLocBufferBlobAbbrv) {
+  typedef ASTWriter::RecordData::value_type RecordDataType;
+
+  // Compress the buffer if possible. We expect that almost all PCM
+  // consumers will not want its contents.
+  SmallString<0> CompressedBuffer;
+  if (llvm::zlib::isAvailable()) {
+llvm::Error E = llvm::zlib::compress(Blob.drop_back(1), CompressedBuffer);
+if (!E) {
+  RecordDataType Record[] = {SM_SLOC_BUFFER_BLOB_COMPRESSED,
+ Blob.size() - 1};
+  Stream.EmitRecordWithBlob(SLocBufferBlobCompressedAbbrv, Record,
+CompressedBuffer);
+  return;
+}
+llvm::consumeError(std::move(E));
+  }
+
+  RecordDataType Record[] = {SM_SLOC_BUFFER_BLOB};
+  Stream.EmitRecordWithBlob(SLocBufferBlobAbbrv, Record, Blob);
+}
+
 /// \brief Writes the block containing the serialized form of the
 /// source manager.
 ///
@@ -2094,20 +2119,8 @@
 const llvm::MemoryBuffer *Buffer =
 Content->getBuffer(PP.getDiagnostics(), PP.getSourceManager());
 StringRef Blob(Buffer->getBufferStart(), Buffer->getBufferSize() + 1);
-
-// Compress the buffer if possible. We expect that almost all PCM
-// consumers will not want its contents.
-SmallString<0> CompressedBuffer;
-if (llvm::zlib::compress(Blob.drop_back(1), CompressedBuffer) ==
-llvm::zlib::StatusOK) {
-  RecordData::value_type Record[] = {SM_SLOC_BUFFER_BLOB_COMPRESSED,
- Blob.size() - 1};
-  Stream.EmitRecordWithBlob(SLocBufferBlobCompressedAbbrv, Record,
-CompressedBuffer);
-} else {
-  RecordData::value_type Record[] = {SM_SLOC_BUFFER_BLOB};
-  Stream.EmitRecordWithBlob(SLocBufferBlobAbbrv, Record, Blob);
-}
+emitBlob(Stream, Blob, SLocBufferBlobCompressedAbbrv,
+ SLocBufferBlobAbbrv);
   }
 } else {
   // The source location entry is a macro expansion.
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -72,6 +72,7 @@
 #include "llvm/Bitcode/BitstreamReader.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -1278,10 +1279,15 @@
 unsigned RecCode = SLocEntryCursor.readRecord(Code, Record, &Blob);
 
 if (RecCode == SM_SLOC_BUFFER_BLOB_COMPRESSED) {
+  if (!llvm::zlib::isAvailable()) {
+Error("zlib is not available");
+return nullptr;
+  }
   SmallString<0> Uncompressed;
-  if (llvm::zlib::uncompress(Blob, Uncompressed, Record[0]) !=
-  llvm::zlib::StatusOK) {
-Error("could not decompress embedded file contents");
+  if (llvm::Error E =
+  llvm::zlib::uncompress(Blob, Uncompressed, Record[0])) {
+Error("could not decompress embedded file contents: " +
+  llvm::toString(std::move(E)));
 return nullptr;
   }
   return llvm::MemoryBuffer::getMemBufferCopy(Uncompressed, Name);


Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -73,6 +73,7 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/EndianStream.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/OnDiskHashTable.h"
@@ -1986,6 +1987,30 @@
 free(const_cast(SavedStrings[I]));
 }
 
+static void emitBlob(llvm::BitstreamWriter &Stream, StringRef Blob,
+ unsigned SLocBuffer

[PATCH] D28807: [Clang] - Update code to match upcoming llvm::zlib API.

[George Rimar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292227: [Clang] - Update code to match upcoming llvm::zlib 
API. (authored by grimar).

Changed prior to commit:
  https://reviews.llvm.org/D28807?vs=84669&id=84676#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28807

Files:
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp


Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -73,6 +73,7 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/EndianStream.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/OnDiskHashTable.h"
@@ -1986,6 +1987,30 @@
 free(const_cast(SavedStrings[I]));
 }
 
+static void emitBlob(llvm::BitstreamWriter &Stream, StringRef Blob,
+ unsigned SLocBufferBlobCompressedAbbrv,
+ unsigned SLocBufferBlobAbbrv) {
+  typedef ASTWriter::RecordData::value_type RecordDataType;
+
+  // Compress the buffer if possible. We expect that almost all PCM
+  // consumers will not want its contents.
+  SmallString<0> CompressedBuffer;
+  if (llvm::zlib::isAvailable()) {
+llvm::Error E = llvm::zlib::compress(Blob.drop_back(1), CompressedBuffer);
+if (!E) {
+  RecordDataType Record[] = {SM_SLOC_BUFFER_BLOB_COMPRESSED,
+ Blob.size() - 1};
+  Stream.EmitRecordWithBlob(SLocBufferBlobCompressedAbbrv, Record,
+CompressedBuffer);
+  return;
+}
+llvm::consumeError(std::move(E));
+  }
+
+  RecordDataType Record[] = {SM_SLOC_BUFFER_BLOB};
+  Stream.EmitRecordWithBlob(SLocBufferBlobAbbrv, Record, Blob);
+}
+
 /// \brief Writes the block containing the serialized form of the
 /// source manager.
 ///
@@ -2094,20 +2119,8 @@
 const llvm::MemoryBuffer *Buffer =
 Content->getBuffer(PP.getDiagnostics(), PP.getSourceManager());
 StringRef Blob(Buffer->getBufferStart(), Buffer->getBufferSize() + 1);
-
-// Compress the buffer if possible. We expect that almost all PCM
-// consumers will not want its contents.
-SmallString<0> CompressedBuffer;
-if (llvm::zlib::compress(Blob.drop_back(1), CompressedBuffer) ==
-llvm::zlib::StatusOK) {
-  RecordData::value_type Record[] = {SM_SLOC_BUFFER_BLOB_COMPRESSED,
- Blob.size() - 1};
-  Stream.EmitRecordWithBlob(SLocBufferBlobCompressedAbbrv, Record,
-CompressedBuffer);
-} else {
-  RecordData::value_type Record[] = {SM_SLOC_BUFFER_BLOB};
-  Stream.EmitRecordWithBlob(SLocBufferBlobAbbrv, Record, Blob);
-}
+emitBlob(Stream, Blob, SLocBufferBlobCompressedAbbrv,
+ SLocBufferBlobAbbrv);
   }
 } else {
   // The source location entry is a macro expansion.
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -72,6 +72,7 @@
 #include "llvm/Bitcode/BitstreamReader.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -1278,10 +1279,15 @@
 unsigned RecCode = SLocEntryCursor.readRecord(Code, Record, &Blob);
 
 if (RecCode == SM_SLOC_BUFFER_BLOB_COMPRESSED) {
+  if (!llvm::zlib::isAvailable()) {
+Error("zlib is not available");
+return nullptr;
+  }
   SmallString<0> Uncompressed;
-  if (llvm::zlib::uncompress(Blob, Uncompressed, Record[0]) !=
-  llvm::zlib::StatusOK) {
-Error("could not decompress embedded file contents");
+  if (llvm::Error E =
+  llvm::zlib::uncompress(Blob, Uncompressed, Record[0])) {
+Error("could not decompress embedded file contents: " +
+  llvm::toString(std::move(E)));
 return nullptr;
   }
   return llvm::MemoryBuffer::getMemBufferCopy(Uncompressed, Name);


Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -73,6 +73,7 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/EndianStream.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/OnDiskHashTable.h"
@@ -1986,6 +1987,30 @@
 free(const_cast(Save