[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-09 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

In D149193#4331015 , @dyung wrote:

> Hi @MaskRay, the test you modified clang/test/Driver/split-debug.c is failing 
> on the PS5 Windows bot, can you take a look?
>
> https://lab.llvm.org/buildbot/#/builders/216/builds/20981
>
>   # command stderr:
>   
> Z:\b\llvm-clang-x86_64-sie-win\llvm-project\clang\test\Driver\split-debug.c:72:21:
>  error: SPLIT_LINK_NULL: expected string not found in input
>   // SPLIT_LINK_NULL: "-dumpdir" "/dev/null-"
>   ^
>   :1:1: note: scanning from here
>   clang version 17.0.0 (https://github.com/llvm/llvm-project.git 
> 16a0a69aadf1ad04efeaab9073bebdfb4b4fd34f)
>   ^
>   :5:130: note: possible intended match here
>"z:\\b\\llvm-clang-x86_64-sie-win\\build\\bin\\clang.exe" "-cc1" "-triple" 
> "x86_64-unknown-linux-gnu" "-emit-obj" "-mrelax-all" "-dumpdir" 
> "C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-" 
> "-disable-free" "-clear-ast-before-backend" "-main-file-name" "split-debug.c" 
> "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" 
> "-mframe-pointer=all" "-fmath-errno" "-ffp-contract=on" "-fno-rounding-math" 
> "-mconstructor-aliases" "-funwind-tables=2" "-target-cpu" "x86-64" 
> "-tune-cpu" "generic" "-debug-info-kind=constructor" "-dwarf-version=5" 
> "-debugger-tuning=gdb" "-ggnu-pubnames" "-split-dwarf-file" 
> "C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-split-debug.dwo"
>  "-split-dwarf-output" 
> "C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-split-debug.dwo"
>  
> "-fcoverage-compilation-dir=Z:\\b\\llvm-clang-x86_64-sie-win\\build\\tools\\clang\\test\\Driver"
>  "-resource-dir" "z:\\b\\llvm-clang-x86_64-sie-win\\build\\lib\\clang\\17" 
> "-internal-isystem" 
> "z:\\b\\llvm-clang-x86_64-sie-win\\build\\lib\\clang\\17\\include" 
> "-internal-isystem" "/usr/local/include" "-internal-externc-isystem" 
> "/include" "-internal-externc-isystem" "/usr/include" 
> "-fdebug-compilation-dir=Z:\\b\\llvm-clang-x86_64-sie-win\\build\\tools\\clang\\test\\Driver"
>  "-ferror-limit" "19" "-fgnuc-version=4.2.1" "-faddrsig" 
> "-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o" 
> "C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\split-debug-c67aff.o"
>  "-x" "c" 
> "Z:\\b\\llvm-clang-x86_64-sie-win\\llvm-project\\clang\\test\\Driver\\split-debug.c"
>   
>  ^
>   Input file: 
>   Check file: 
> Z:\b\llvm-clang-x86_64-sie-win\llvm-project\clang\test\Driver\split-debug.c
>   -dump-input=help explains the following input dump.
>   Input was:
>   <<
>   1: clang version 17.0.0 
> (https://github.com/llvm/llvm-project.git 
> 16a0a69aadf1ad04efeaab9073bebdfb4b4fd34f) 
>   check:72'0 
> X
>  error: no match found
>   2: Target: x86_64-unknown-linux-gnu 
>   check:72'0 ~
>   3: Thread model: posix 
>   check:72'0 
>   4: InstalledDir: z:\b\llvm-clang-x86_64-sie-win\build\bin 
>   check:72'0 ~~~
>   5:  "z:\\b\\llvm-clang-x86_64-sie-win\\build\\bin\\clang.exe" 
> "-cc1" "-triple" "x86_64-unknown-linux-gnu" "-emit-obj" "-mrelax-all" 
> "-dumpdir" 
> "C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-" 
> "-disable-free" "-clear-ast-before-backend" "-main-file-name" "split-debug.c" 
> "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" 
> "-mframe-pointer=all" "-fmath-errno" "-ffp-contract=on" "-fno-rounding-math" 
> "-mconstructor-aliases" "-funwind-tables=2" "-target-cpu" "x86-64" 
> "-tune-cpu" "generic" "-debug-info-kind=constructor" "-dwarf-version=5" 
> "-debugger-tuning=gdb" "-ggnu-pubnames" "-split-dwarf-file" 
> "C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-split-debug.dwo"
>  "-split-dwarf-output" 
> "C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-split-debug.dwo"
>  
> "-fcoverage-compilation-dir=Z:\\b\\llvm-clang-x86_64-sie-win\\build\\tools\\clang\\test\\Driver"
>  "-resource-dir" "z:\\b\\llvm-clang-x86_64-sie-win\\build\\lib\\clang\\17" 
> "-internal-isystem" 
> "z:\\b\\llvm-clang-x86_64-sie-win\\build\\lib\\clang\\17\\include" 
> "-internal-isystem" "/usr/local/include" "-internal-externc-isystem" 
> "/include" "-internal-externc-isystem" "/usr/include" 
> "-fdebug-compilation-dir=Z:\\b\\llvm-clang-x86_64-sie-win\\build\\tools\\clang\\test\\Driver"
>  "-ferror-limit" "19" "-fgnuc-version=4.2.1" "-faddrsig" 
> "-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o" 
> "C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\split-debug-c67aff.o"
>  "-x" "c" 
>

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-09 Thread Douglas Yung via Phabricator via cfe-commits 
dyung added a comment.

Hi @MaskRay, the test you modified clang/test/Driver/split-debug.c is failing 
on the PS5 Windows bot, can you take a look?

https://lab.llvm.org/buildbot/#/builders/216/builds/20981

  # command stderr:
  
Z:\b\llvm-clang-x86_64-sie-win\llvm-project\clang\test\Driver\split-debug.c:72:21:
 error: SPLIT_LINK_NULL: expected string not found in input
  // SPLIT_LINK_NULL: "-dumpdir" "/dev/null-"
  ^
  :1:1: note: scanning from here
  clang version 17.0.0 (https://github.com/llvm/llvm-project.git 
16a0a69aadf1ad04efeaab9073bebdfb4b4fd34f)
  ^
  :5:130: note: possible intended match here
   "z:\\b\\llvm-clang-x86_64-sie-win\\build\\bin\\clang.exe" "-cc1" "-triple" 
"x86_64-unknown-linux-gnu" "-emit-obj" "-mrelax-all" "-dumpdir" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-" 
"-disable-free" "-clear-ast-before-backend" "-main-file-name" "split-debug.c" 
"-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-mframe-pointer=all" 
"-fmath-errno" "-ffp-contract=on" "-fno-rounding-math" "-mconstructor-aliases" 
"-funwind-tables=2" "-target-cpu" "x86-64" "-tune-cpu" "generic" 
"-debug-info-kind=constructor" "-dwarf-version=5" "-debugger-tuning=gdb" 
"-ggnu-pubnames" "-split-dwarf-file" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-split-debug.dwo"
 "-split-dwarf-output" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-split-debug.dwo"
 
"-fcoverage-compilation-dir=Z:\\b\\llvm-clang-x86_64-sie-win\\build\\tools\\clang\\test\\Driver"
 "-resource-dir" "z:\\b\\llvm-clang-x86_64-sie-win\\build\\lib\\clang\\17" 
"-internal-isystem" 
"z:\\b\\llvm-clang-x86_64-sie-win\\build\\lib\\clang\\17\\include" 
"-internal-isystem" "/usr/local/include" "-internal-externc-isystem" "/include" 
"-internal-externc-isystem" "/usr/include" 
"-fdebug-compilation-dir=Z:\\b\\llvm-clang-x86_64-sie-win\\build\\tools\\clang\\test\\Driver"
 "-ferror-limit" "19" "-fgnuc-version=4.2.1" "-faddrsig" 
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\split-debug-c67aff.o"
 "-x" "c" 
"Z:\\b\\llvm-clang-x86_64-sie-win\\llvm-project\\clang\\test\\Driver\\split-debug.c"

   ^
  Input file: 
  Check file: 
Z:\b\llvm-clang-x86_64-sie-win\llvm-project\clang\test\Driver\split-debug.c
  -dump-input=help explains the following input dump.
  Input was:
  <<
  1: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 
16a0a69aadf1ad04efeaab9073bebdfb4b4fd34f) 
  check:72'0 
X
 error: no match found
  2: Target: x86_64-unknown-linux-gnu 
  check:72'0 ~
  3: Thread model: posix 
  check:72'0 
  4: InstalledDir: z:\b\llvm-clang-x86_64-sie-win\build\bin 
  check:72'0 ~~~
  5:  "z:\\b\\llvm-clang-x86_64-sie-win\\build\\bin\\clang.exe" 
"-cc1" "-triple" "x86_64-unknown-linux-gnu" "-emit-obj" "-mrelax-all" 
"-dumpdir" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-" 
"-disable-free" "-clear-ast-before-backend" "-main-file-name" "split-debug.c" 
"-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-mframe-pointer=all" 
"-fmath-errno" "-ffp-contract=on" "-fno-rounding-math" "-mconstructor-aliases" 
"-funwind-tables=2" "-target-cpu" "x86-64" "-tune-cpu" "generic" 
"-debug-info-kind=constructor" "-dwarf-version=5" "-debugger-tuning=gdb" 
"-ggnu-pubnames" "-split-dwarf-file" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-split-debug.dwo"
 "-split-dwarf-output" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\tmpq2vte1gy-split-debug.dwo"
 
"-fcoverage-compilation-dir=Z:\\b\\llvm-clang-x86_64-sie-win\\build\\tools\\clang\\test\\Driver"
 "-resource-dir" "z:\\b\\llvm-clang-x86_64-sie-win\\build\\lib\\clang\\17" 
"-internal-isystem" 
"z:\\b\\llvm-clang-x86_64-sie-win\\build\\lib\\clang\\17\\include" 
"-internal-isystem" "/usr/local/include" "-internal-externc-isystem" "/include" 
"-internal-externc-isystem" "/usr/include" 
"-fdebug-compilation-dir=Z:\\b\\llvm-clang-x86_64-sie-win\\build\\tools\\clang\\test\\Driver"
 "-ferror-limit" "19" "-fgnuc-version=4.2.1" "-faddrsig" 
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o" 
"C:\\Users\\buildbot\\AppData\\Local\\Temp\\lit-tmp-zjxfa_79\\split-debug-c67aff.o"
 "-x" "c" 
"Z:\\b\\llvm-clang-x86_64-sie-win\\llvm-project\\clang\\test\\Driver\\split-debug.c"
 
  check:72'0

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-09 Thread Fangrui Song via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdbedcfdc2007: [Driver] Add -dumpdir and change -gsplit-dwarf 
.dwo names for linking (authored by MaskRay).

Changed prior to commit:
  https://reviews.llvm.org/D149193?vs=520573=520824#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/hip-gsplit-dwarf-options.hip
  clang/test/Driver/split-debug.c

Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -9,6 +9,7 @@
 
 // INLINE: "-fsplit-dwarf-inlining"
 // NOINLINE-NOT: "-fsplit-dwarf-inlining"
+// SPLIT-NOT:  "-dumpdir"
 // SPLIT:  "-debug-info-kind=constructor"
 // SPLIT-SAME: "-ggnu-pubnames"
 // SPLIT-SAME: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
@@ -54,8 +55,29 @@
 // SINGLE_WITH_FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
 // SINGLE_WITH_FILENAME-NOT: "-split-dwarf-output"
 
-/// Without -c, clang performs linking as well. The output is unchanged.
-// RUN: %clang -### -target x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o ignore.d 2>&1 | FileCheck %s --check-prefix=SPLIT
+/// If linking is the final phase, the .dwo filename is derived from -o (if specified) or "a".
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK_A
+
+// SPLIT_LINK:  "-dumpdir" "obj/out-"
+// SPLIT_LINK:  "-debug-info-kind=constructor"
+// SPLIT_LINK-SAME: "-split-dwarf-file" "obj/out-split-debug.dwo" "-split-dwarf-output" "obj/out-split-debug.dwo"
+// SPLIT_LINK_A:  "-dumpdir" "a-"
+// SPLIT_LINK_A-SAME: "-split-dwarf-file" "a-split-debug.dwo" "-split-dwarf-output" "a-split-debug.dwo"
+
+/// GCC special cases /dev/null (HOST_BIT_BUCKET) but not other special files like /dev/zero.
+/// We don't apply special rules at all.
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK_NULL
+
+// SPLIT_LINK_NULL:  "-dumpdir" "/dev/null-"
+// SPLIT_LINK_NULL-SAME: "-split-dwarf-output" "/dev/null-split-debug.dwo"
+
+/// If -dumpdir is specified, use its value to derive the .dwo filename.
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out -dumpdir pf/x -c 2>&1 | FileCheck %s --check-prefix=DUMPDIR
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out -dumpdir pf/x 2>&1 | FileCheck %s --check-prefix=DUMPDIR
+
+// DUMPDIR:  "-dumpdir" "pf/x"
+// DUMPDIR-SAME: "-split-dwarf-output" "pf/xsplit-debug.dwo"
 
 /// -fsplit-dwarf-inlining
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf=split -g -fsplit-dwarf-inlining %s 2>&1 | FileCheck %s --check-prefixes=INLINE,SPLIT
Index: clang/test/Driver/hip-gsplit-dwarf-options.hip
===
--- clang/test/Driver/hip-gsplit-dwarf-options.hip
+++ clang/test/Driver/hip-gsplit-dwarf-options.hip
@@ -13,13 +13,17 @@
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu \
 // RUN:   --offload-arch=gfx906:xnack+ %s -nogpulib -nogpuinc \
 // RUN:   --offload-arch=gfx900 \
-// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s
+// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s --check-prefix=LINK
 
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu \
 // RUN:   -fgpu-rdc --offload-arch=gfx906:xnack+ %s -nogpulib -nogpuinc \
 // RUN:   --offload-arch=gfx900 \
-// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s
+// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s --check-prefix=LINK
 
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "gfx906".* "-split-dwarf-output" "hip-gsplit-dwarf-options_gfx906:xnack\+.dwo"}}
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "gfx900".* "-split-dwarf-output" "hip-gsplit-dwarf-options_gfx900.dwo"}}
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "x86-64".* "-split-dwarf-output" "hip-gsplit-dwarf-options.dwo"}}
+
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "gfx906".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options_gfx906:xnack\+.dwo"}}
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "gfx900".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options_gfx900.dwo"}}
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "x86-64".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options.dwo"}}
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
---

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-09 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D149193#4328553 , @MaskRay wrote:

> In D149193#4328452 , @dblaikie 
> wrote:
>
>>> I agree that for most(all?) split DWARF users will not see any difference 
>>> since they always use -c -o and don't combine compilation and linking in 
>>> one command.
>>
>> Given that, I'm not sure that this is worth implementing, but if it suits 
>> you I guess.
>
> The split DWARF users are about production users.
>
> There are command line users who do `clang -g -gsplit-dwarf a.c b.c -o x` and 
> I have heard about questions that the `.dwo` files go to strange directories 
> (usually `/tmp`) quite a few times.
> Fixing `-gsplit-dwarf` helps set a baseline for other options like 
> `-ftime-trace` (https://github.com/llvm/llvm-project/issues/57285).

Fair enough


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-09 Thread Scott Linder via Phabricator via cfe-commits 
scott.linder added a comment.

In D149193#4316293 , @dblaikie wrote:

> I guess my main question is: What's the motivation for implementing this? Do 
> you have a need/use for this? (it doesn't seem to be motivated by GCC 
> compatibility - as discussed, looks like we're diverging in a bunch of ways 
> from their behavior and the argument made that these are "developer" flags, 
> so not a stable/compatible interface used across both compilers)

Even if we only align the default behavior of Clang with the default behavior 
of GCC it seems like a win. AFAIU we could implement this without `-dumpdir` at 
all, but the baseline notion of being able to specify where to put 
auxiliary/dump files seems useful.

Do we need to match every quirk and add every other knob from GCC in order to 
use the same name? Is there precedent in terms of other options where Clang is 
close but opinionated about behavior relative to GCC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-08 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay updated this revision to Diff 520573.
MaskRay added a comment.

use `llvm::sys::path::stem(getDefaultImageName())`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/hip-gsplit-dwarf-options.hip
  clang/test/Driver/split-debug.c

Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -9,6 +9,7 @@
 
 // INLINE: "-fsplit-dwarf-inlining"
 // NOINLINE-NOT: "-fsplit-dwarf-inlining"
+// SPLIT-NOT:  "-dumpdir"
 // SPLIT:  "-debug-info-kind=constructor"
 // SPLIT-SAME: "-ggnu-pubnames"
 // SPLIT-SAME: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
@@ -54,8 +55,29 @@
 // SINGLE_WITH_FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
 // SINGLE_WITH_FILENAME-NOT: "-split-dwarf-output"
 
-/// Without -c, clang performs linking as well. The output is unchanged.
-// RUN: %clang -### -target x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o ignore.d 2>&1 | FileCheck %s --check-prefix=SPLIT
+/// If linking is the final phase, the .dwo filename is derived from -o (if specified) or "a".
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK_A
+
+// SPLIT_LINK:  "-dumpdir" "obj/out-"
+// SPLIT_LINK:  "-debug-info-kind=constructor"
+// SPLIT_LINK-SAME: "-split-dwarf-file" "obj/out-split-debug.dwo" "-split-dwarf-output" "obj/out-split-debug.dwo"
+// SPLIT_LINK_A:  "-dumpdir" "a-"
+// SPLIT_LINK_A-SAME: "-split-dwarf-file" "a-split-debug.dwo" "-split-dwarf-output" "a-split-debug.dwo"
+
+/// GCC special cases /dev/null (HOST_BIT_BUCKET) but not other special files like /dev/zero.
+/// We don't apply special rules at all.
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK_NULL
+
+// SPLIT_LINK_NULL:  "-dumpdir" "/dev/null-"
+// SPLIT_LINK_NULL-SAME: "-split-dwarf-output" "/dev/null-split-debug.dwo"
+
+/// If -dumpdir is specified, use its value to derive the .dwo filename.
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out -dumpdir pf/x -c 2>&1 | FileCheck %s --check-prefix=DUMPDIR
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out -dumpdir pf/x 2>&1 | FileCheck %s --check-prefix=DUMPDIR
+
+// DUMPDIR:  "-dumpdir" "pf/x"
+// DUMPDIR-SAME: "-split-dwarf-output" "pf/xsplit-debug.dwo"
 
 /// -fsplit-dwarf-inlining
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf=split -g -fsplit-dwarf-inlining %s 2>&1 | FileCheck %s --check-prefixes=INLINE,SPLIT
Index: clang/test/Driver/hip-gsplit-dwarf-options.hip
===
--- clang/test/Driver/hip-gsplit-dwarf-options.hip
+++ clang/test/Driver/hip-gsplit-dwarf-options.hip
@@ -13,13 +13,17 @@
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu \
 // RUN:   --offload-arch=gfx906:xnack+ %s -nogpulib -nogpuinc \
 // RUN:   --offload-arch=gfx900 \
-// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s
+// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s --check-prefix=LINK
 
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu \
 // RUN:   -fgpu-rdc --offload-arch=gfx906:xnack+ %s -nogpulib -nogpuinc \
 // RUN:   --offload-arch=gfx900 \
-// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s
+// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s --check-prefix=LINK
 
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "gfx906".* "-split-dwarf-output" "hip-gsplit-dwarf-options_gfx906:xnack\+.dwo"}}
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "gfx900".* "-split-dwarf-output" "hip-gsplit-dwarf-options_gfx900.dwo"}}
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "x86-64".* "-split-dwarf-output" "hip-gsplit-dwarf-options.dwo"}}
+
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "gfx906".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options_gfx906:xnack\+.dwo"}}
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "gfx900".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options_gfx900.dwo"}}
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "x86-64".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options.dwo"}}
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1248,23 +1248,24 @@
 if (StringRef(A->getValue()) == "single")
   return Args.MakeArgString(Output.getFilename());
 
-  Arg *FinalOutput =

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-08 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay marked 3 inline comments as done.
MaskRay added a comment.

In D149193#4328452 , @dblaikie wrote:

>> I agree that for most(all?) split DWARF users will not see any difference 
>> since they always use -c -o and don't combine compilation and linking in one 
>> command.
>
> Given that, I'm not sure that this is worth implementing, but if it suits you 
> I guess.

The split DWARF users are about production users.

There are command line users who do `clang -g -gsplit-dwarf a.c b.c -o x` and I 
have heard about questions that the `.dwo` files go to strange directories 
(usually `/tmp`) quite a few times.
Fixing `-gsplit-dwarf` helps set a baseline for other options like 
`-ftime-trace` (https://github.com/llvm/llvm-project/issues/57285).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-08 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

> I agree that for most(all?) split DWARF users will not see any difference 
> since they always use -c -o and don't combine compilation and linking in one 
> command.

Given that, I'm not sure that this is worth implementing, but if it suits you I 
guess.




Comment at: clang/lib/Driver/Driver.cpp:3884
+  nullptr, getOpts().getOption(options::OPT_dumpdir),
+  Args.MakeArgString(Args.getLastArgValue(options::OPT_o, "a") + "-"));
+  Arg->claim();

MaskRay wrote:
> scott.linder wrote:
> > MaskRay wrote:
> > > dblaikie wrote:
> > > > would be nice to have this "a" derive from wherever we hardcode "a.out" 
> > > > as the default output rather than independently hardcoded here?
> > > > 
> > > > & what does GCC do when the `-o` value has a `.` in it? (if you use `-o 
> > > > a.out` do you get the same `a-x.dwo` behavior, or do you get 
> > > > `a.out-x.dwo`?)
> > > We can use `llvm::sys::path::stem(getDefaultImageName())`, but I feel 
> > > that this just complicates the code.
> > > The default is `a.out` or `a.exe`. If a downstream platform decides to 
> > > deviate and use another filename, say, `b.out`. We will use `-dumpdir b-` 
> > > on this platform and `-dumpdir a-` on everything else. I think they will 
> > > likely be fine with `a-` even if they don't use `a` as the stem name of 
> > > the default image...
> > > 
> > > GCC generally doesn't special case `.` in `-o` for linking, but the 
> > > `a.out` filename is different.
> > > 
> > > ```
> > > gcc -g -gsplit-dwarf d/a.c.c -o e/x.out  # e/x.out-a.dwo
> > > gcc -g -gsplit-dwarf d/a.c.c -o e/a.out  # e/a-a.dwo
> > > ```
> > > 
> > > I think Clang should not special case `a.out`.
> > GCC distinguishes between "dump" and "auxiliary" outputs, and in this case 
> > I think the "dump" outputs retain the basename-suffix (i.e. you get 
> > a.out) whereas "auxiliary" outputs first strip the basename-suffix 
> > (i.e. you get a). The basename-suffix itself can be specified 
> > explicitly via -dumpbase-ext, but it is inferred by default.
> > 
> > The naming for things adds to the for me:
> > 
> > * `-dumpdir` doesn't specifically/exclusively specify a "directory", it 
> > just specifies a prefix
> > * `-dumpbase-ext` only affects the output of non-dump, auxiliary files
> > 
> > I do worry that being close-but-not-quite like GCC here will cause 
> > headaches for someone, but I am also not excited about implementing the 
> > complexity of the GCC options.
> > ... I think the "dump" outputs retain the basename-suffix (i.e. you get 
> > a.out) whereas "auxiliary" outputs first strip the basename-suffix 
> > (i.e. you get a).
> 
> Confirmed.
> ```
> gcc -g -fdump-rtl-all -gsplit-dwarf d/a.c -o e/x.out
> ls e/x.out-a.c.338r.dfinish e/x.out-a.dwo
> ```
> 
> For dump output files, I think they all reside in developer options 
> (https://gcc.gnu.org/onlinedocs/gcc/Developer-Options.html) not intended to 
> be used by end users. They occasionally make incompatible changes in this 
> area as well, e.g. 
> https://gcc.gnu.org/pipermail/gcc-patches/2020-May/546009.html simplified 
> some rules.
> 
> It seems that if we just implement `-dumpdir`, we have gotten the good parts 
> and we are probably done :)
> We can use llvm::sys::path::stem(getDefaultImageName()), but I feel that this 
> just complicates the code.

I think it'd be a bit better this way - otherways "a" looks pretty arbitrary. 
Is there some other reason it would be "a"? If it is derived from "a.out" I 
think having the code reflect that is helpful to the reader.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-08 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

Ping:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-03 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

In D149193#4316293 , @dblaikie wrote:

> I guess my main question is: What's the motivation for implementing this? Do 
> you have a need/use for this? (it doesn't seem to be motivated by GCC 
> compatibility - as discussed, looks like we're diverging in a bunch of ways 
> from their behavior and the argument made that these are "developer" flags, 
> so not a stable/compatible interface used across both compilers)

This is a long-known problem that features with auxiliary output files go to 
strange directories (temporary directory, usually `/tmp`).
Among these options, the prominent ones are `-gsplit-dwarf` and `-ftime-trace` 
(`clang -ftime-trace a.c b.c` 
https://github.com/llvm/llvm-project/issues/57285).

By just implementing `-dumpdir `, we don't diverge in "a bunch ways". If GCC 
has fixed their `-o d/a.out` bug, our behavior should match theirs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-03 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

I guess my main question is: What's the motivation for implementing this? Do 
you have a need/use for this? (it doesn't seem to be motivated by GCC 
compatibility - as discussed, looks like we're diverging in a bunch of ways 
from their behavior and the argument made that these are "developer" flags, so 
not a stable/compatible interface used across both compilers)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-05-02 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

Thank you:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-28 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D149193#4302981 , @scott.linder 
wrote:

> I am OK to give LGTM, assuming the other reviewers don't still have 
> reservations?

Some - planning to get to this next week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-27 Thread Dan McGregor via Phabricator via cfe-commits 
dankm added a comment.

I certainly like the idea. I'll spend some time later looking at the 
implementation, but from a quick glance it looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-27 Thread Scott Linder via Phabricator via cfe-commits 
scott.linder added a comment.

I am OK to give LGTM, assuming the other reviewers don't still have 
reservations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-27 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay marked an inline comment as done.
MaskRay added a comment.

In D149193#4302337 , @scott.linder 
wrote:

> In D149193#4300885 , @MaskRay wrote:
>
>> I think the patch as-is implements all the useful parts of GCC's complex 
>> rules and in the absence of `-dumpbase` (we deliberately don't implement), 
>> the rule almost exactly matches GCC unless we do `gcc -g -gsplit-dwarf d/a.c 
>> -o e/a.out` (instead of other filenames).
>>
>> https://maskray.me/blog/2023-04-25-compiler-output-files records all my 
>> findings.
>
> I agree that the special-casing of `a.out` when explicitly specified is odd.
>
> Another option to avoid any confusion or debates over exact behavior relative 
> to GCC would be to just make our own option. Rather than add `-dumpdir` we 
> could add `-dump-prefix` or something. I'm also OK with just implementing a 
> simpler subset of the GCC options though (i.e. what you have already).

I have thought about this option, using a name like `--aux-prefix=` (I'd prefer 
aux to dump), but since the behaviors are nearly identical (if we ignore the 
GCC `-o d/a.out` special case), I think using `-dumpdir` is still fine, and 
users won't need to remember two options when they have the needs.

I think the GCC `-o e/a.out` special case is a bug to detect `gcc -g 
-gsplit-dwarf d/a.c` (without `-o`; `a.dwo`). If the bug is fixed, its 
`-dumpdir` behavior should be identical (for all the experiments I have done) 
to what this patch implements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-27 Thread Scott Linder via Phabricator via cfe-commits 
scott.linder added a comment.

In D149193#4300885 , @MaskRay wrote:

> I think the patch as-is implements all the useful parts of GCC's complex 
> rules and in the absence of `-dumpbase` (we deliberately don't implement), 
> the rule almost exactly matches GCC unless we do `gcc -g -gsplit-dwarf d/a.c 
> -o e/a.out` (instead of other filenames).
>
> https://maskray.me/blog/2023-04-25-compiler-output-files records all my 
> findings.

I agree that the special-casing of `a.out` when explicitly specified is odd.

Another option to avoid any confusion or debates over exact behavior relative 
to GCC would be to just make our own option. Rather than add `-dumpdir` we 
could add `-dump-prefix` or something. I'm also OK with just implementing a 
simpler subset of the GCC options though (i.e. what you have already).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-26 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay marked an inline comment as done.
MaskRay added a comment.

I think the patch as-is implements all the useful parts of GCC's complex rules 
and in the absence of `-dumpbase` (we deliberately don't implement), the rule 
almost exactly matches GCC unless we do `gcc -g -gsplit-dwarf d/a.c -o e/a.out` 
(instead of other filenames).

https://maskray.me/blog/2023-04-25-compiler-output-files records all my 
findings.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1261
-Arg *A = Args.getLastArg(options::OPT_ffile_compilation_dir_EQ,
- options::OPT_fdebug_compilation_dir_EQ);
-SmallString<128> T(A ? A->getValue() : "");

dblaikie wrote:
> I was going to suggest we shouldn't remove this functionality without 
> checking who implemented/whether they rely on it (despite it being untested)
> 
> Looks like it went in in some of the earliest patches from Google for Split 
> DWARF ( 248357f62418831c7be9d43d96860b52aae1ef56 ).
> 
> We do use `-fdebug-compilation-dir` at Google, but I think we just set it to 
> `.` anyway, so for that use case it'd be a no-op anyway, I think? Might want 
> to test this patch internally just to be sure, though.
> 
> Though perhaps we never hit this path, because we'll always be building with 
> `-o` and `-c` which should hit the `if` above?
We use the environment variable, if exists and is an absolute path, is used by 
GCC and Clang as a fallback when `-fdebug-compilation-dir=` is unspecified. As 
an example, `PWD=/proc/self/cwd clang `

I agree that for most(all?) split DWARF users will not see any difference since 
they always use `-c -o` and don't combine compilation and linking in one 
command.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-26 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

In D149193#4300501 , @phosek wrote:

> Do we have any way to check if there are projects out there that use 
> `-dumpbase`? It shouldn't be too difficult to support it, but we should find 
> out first if it's needed.

I spot checked the very few items on 
https://sourcegraph.com/search?q=context:global+-dumpbase+=standard=1=repo
 and they don't like genuine use cases.

I think the main use case for `-dumpbase` is to entirely control the auxiliary 
file for `-c` and `-S`, but the addition of `-dumpbase` doesn't seem very 
useful to me...

  gcc -c -g -gsplit-dwarf d/a.c -dumpdir f/ -dumpbase aa   # f/aa.dwo instead 
of f/a.dwo




Comment at: clang/lib/Driver/Driver.cpp:3884
+  nullptr, getOpts().getOption(options::OPT_dumpdir),
+  Args.MakeArgString(Args.getLastArgValue(options::OPT_o, "a") + "-"));
+  Arg->claim();

scott.linder wrote:
> MaskRay wrote:
> > dblaikie wrote:
> > > would be nice to have this "a" derive from wherever we hardcode "a.out" 
> > > as the default output rather than independently hardcoded here?
> > > 
> > > & what does GCC do when the `-o` value has a `.` in it? (if you use `-o 
> > > a.out` do you get the same `a-x.dwo` behavior, or do you get 
> > > `a.out-x.dwo`?)
> > We can use `llvm::sys::path::stem(getDefaultImageName())`, but I feel that 
> > this just complicates the code.
> > The default is `a.out` or `a.exe`. If a downstream platform decides to 
> > deviate and use another filename, say, `b.out`. We will use `-dumpdir b-` 
> > on this platform and `-dumpdir a-` on everything else. I think they will 
> > likely be fine with `a-` even if they don't use `a` as the stem name of the 
> > default image...
> > 
> > GCC generally doesn't special case `.` in `-o` for linking, but the `a.out` 
> > filename is different.
> > 
> > ```
> > gcc -g -gsplit-dwarf d/a.c.c -o e/x.out  # e/x.out-a.dwo
> > gcc -g -gsplit-dwarf d/a.c.c -o e/a.out  # e/a-a.dwo
> > ```
> > 
> > I think Clang should not special case `a.out`.
> GCC distinguishes between "dump" and "auxiliary" outputs, and in this case I 
> think the "dump" outputs retain the basename-suffix (i.e. you get 
> a.out) whereas "auxiliary" outputs first strip the basename-suffix 
> (i.e. you get a). The basename-suffix itself can be specified 
> explicitly via -dumpbase-ext, but it is inferred by default.
> 
> The naming for things adds to the for me:
> 
> * `-dumpdir` doesn't specifically/exclusively specify a "directory", it just 
> specifies a prefix
> * `-dumpbase-ext` only affects the output of non-dump, auxiliary files
> 
> I do worry that being close-but-not-quite like GCC here will cause headaches 
> for someone, but I am also not excited about implementing the complexity of 
> the GCC options.
> ... I think the "dump" outputs retain the basename-suffix (i.e. you get 
> a.out) whereas "auxiliary" outputs first strip the basename-suffix 
> (i.e. you get a).

Confirmed.
```
gcc -g -fdump-rtl-all -gsplit-dwarf d/a.c -o e/x.out
ls e/x.out-a.c.338r.dfinish e/x.out-a.dwo
```

For dump output files, I think they all reside in developer options 
(https://gcc.gnu.org/onlinedocs/gcc/Developer-Options.html) not intended to be 
used by end users. They occasionally make incompatible changes in this area as 
well, e.g. https://gcc.gnu.org/pipermail/gcc-patches/2020-May/546009.html 
simplified some rules.

It seems that if we just implement `-dumpdir`, we have gotten the good parts 
and we are probably done :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-26 Thread Petr Hosek via Phabricator via cfe-commits 
phosek added a comment.

Do we have any way to check if there are projects out there that use 
`-dumpbase`? It shouldn't be too difficult to support it, but we should find 
out first if it's needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-26 Thread Scott Linder via Phabricator via cfe-commits 
scott.linder added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:3884
+  nullptr, getOpts().getOption(options::OPT_dumpdir),
+  Args.MakeArgString(Args.getLastArgValue(options::OPT_o, "a") + "-"));
+  Arg->claim();

MaskRay wrote:
> dblaikie wrote:
> > would be nice to have this "a" derive from wherever we hardcode "a.out" as 
> > the default output rather than independently hardcoded here?
> > 
> > & what does GCC do when the `-o` value has a `.` in it? (if you use `-o 
> > a.out` do you get the same `a-x.dwo` behavior, or do you get `a.out-x.dwo`?)
> We can use `llvm::sys::path::stem(getDefaultImageName())`, but I feel that 
> this just complicates the code.
> The default is `a.out` or `a.exe`. If a downstream platform decides to 
> deviate and use another filename, say, `b.out`. We will use `-dumpdir b-` on 
> this platform and `-dumpdir a-` on everything else. I think they will likely 
> be fine with `a-` even if they don't use `a` as the stem name of the default 
> image...
> 
> GCC generally doesn't special case `.` in `-o` for linking, but the `a.out` 
> filename is different.
> 
> ```
> gcc -g -gsplit-dwarf d/a.c.c -o e/x.out  # e/x.out-a.dwo
> gcc -g -gsplit-dwarf d/a.c.c -o e/a.out  # e/a-a.dwo
> ```
> 
> I think Clang should not special case `a.out`.
GCC distinguishes between "dump" and "auxiliary" outputs, and in this case I 
think the "dump" outputs retain the basename-suffix (i.e. you get 
a.out) whereas "auxiliary" outputs first strip the basename-suffix 
(i.e. you get a). The basename-suffix itself can be specified 
explicitly via -dumpbase-ext, but it is inferred by default.

The naming for things adds to the for me:

* `-dumpdir` doesn't specifically/exclusively specify a "directory", it just 
specifies a prefix
* `-dumpbase-ext` only affects the output of non-dump, auxiliary files

I do worry that being close-but-not-quite like GCC here will cause headaches 
for someone, but I am also not excited about implementing the complexity of the 
GCC options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-26 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:3884
+  nullptr, getOpts().getOption(options::OPT_dumpdir),
+  Args.MakeArgString(Args.getLastArgValue(options::OPT_o, "a") + "-"));
+  Arg->claim();

dblaikie wrote:
> would be nice to have this "a" derive from wherever we hardcode "a.out" as 
> the default output rather than independently hardcoded here?
> 
> & what does GCC do when the `-o` value has a `.` in it? (if you use `-o 
> a.out` do you get the same `a-x.dwo` behavior, or do you get `a.out-x.dwo`?)
We can use `llvm::sys::path::stem(getDefaultImageName())`, but I feel that this 
just complicates the code.
The default is `a.out` or `a.exe`. If a downstream platform decides to deviate 
and use another filename, say, `b.out`. We will use `-dumpdir b-` on this 
platform and `-dumpdir a-` on everything else. I think they will likely be fine 
with `a-` even if they don't use `a` as the stem name of the default image...

GCC generally doesn't special case `.` in `-o` for linking, but the `a.out` 
filename is different.

```
gcc -g -gsplit-dwarf d/a.c.c -o e/x.out  # e/x.out-a.dwo
gcc -g -gsplit-dwarf d/a.c.c -o e/a.out  # e/a-a.dwo
```

I think Clang should not special case `a.out`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-26 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added inline comments.



Comment at: clang/lib/Driver/Driver.cpp:3884
+  nullptr, getOpts().getOption(options::OPT_dumpdir),
+  Args.MakeArgString(Args.getLastArgValue(options::OPT_o, "a") + "-"));
+  Arg->claim();

would be nice to have this "a" derive from wherever we hardcode "a.out" as the 
default output rather than independently hardcoded here?

& what does GCC do when the `-o` value has a `.` in it? (if you use `-o a.out` 
do you get the same `a-x.dwo` behavior, or do you get `a.out-x.dwo`?)



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1261
-Arg *A = Args.getLastArg(options::OPT_ffile_compilation_dir_EQ,
- options::OPT_fdebug_compilation_dir_EQ);
-SmallString<128> T(A ? A->getValue() : "");

I was going to suggest we shouldn't remove this functionality without checking 
who implemented/whether they rely on it (despite it being untested)

Looks like it went in in some of the earliest patches from Google for Split 
DWARF ( 248357f62418831c7be9d43d96860b52aae1ef56 ).

We do use `-fdebug-compilation-dir` at Google, but I think we just set it to 
`.` anyway, so for that use case it'd be a no-op anyway, I think? Might want to 
test this patch internally just to be sure, though.

Though perhaps we never hit this path, because we'll always be building with 
`-o` and `-c` which should hit the `if` above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

2023-04-25 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay updated this revision to Diff 517004.
MaskRay retitled this revision from "[Driver] -gsplit-dwarf: derive .dwo names 
from -o for link actions" to "[Driver] Add -dumpdir and change -gsplit-dwarf 
.dwo names for linking".
MaskRay edited the summary of this revision.
MaskRay added a comment.

expose -dumpdir
add release notes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149193

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/hip-gsplit-dwarf-options.hip
  clang/test/Driver/split-debug.c

Index: clang/test/Driver/split-debug.c
===
--- clang/test/Driver/split-debug.c
+++ clang/test/Driver/split-debug.c
@@ -9,6 +9,7 @@
 
 // INLINE: "-fsplit-dwarf-inlining"
 // NOINLINE-NOT: "-fsplit-dwarf-inlining"
+// SPLIT-NOT:  "-dumpdir"
 // SPLIT:  "-debug-info-kind=constructor"
 // SPLIT-SAME: "-ggnu-pubnames"
 // SPLIT-SAME: "-split-dwarf-file" "split-debug.dwo" "-split-dwarf-output" "split-debug.dwo"
@@ -54,8 +55,29 @@
 // SINGLE_WITH_FILENAME: "-split-dwarf-file" "{{.*}}foo.o"
 // SINGLE_WITH_FILENAME-NOT: "-split-dwarf-output"
 
-/// Without -c, clang performs linking as well. The output is unchanged.
-// RUN: %clang -### -target x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o ignore.d 2>&1 | FileCheck %s --check-prefix=SPLIT
+/// If linking is the final phase, the .dwo filename is derived from -o (if specified) or "a".
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK_A
+
+// SPLIT_LINK:  "-dumpdir" "obj/out-"
+// SPLIT_LINK:  "-debug-info-kind=constructor"
+// SPLIT_LINK-SAME: "-split-dwarf-file" "obj/out-split-debug.dwo" "-split-dwarf-output" "obj/out-split-debug.dwo"
+// SPLIT_LINK_A:  "-dumpdir" "a-"
+// SPLIT_LINK_A-SAME: "-split-dwarf-file" "a-split-debug.dwo" "-split-dwarf-output" "a-split-debug.dwo"
+
+/// GCC special cases /dev/null (HOST_BIT_BUCKET) but not other special files like /dev/zero.
+/// We don't apply special rules at all.
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o /dev/null 2>&1 | FileCheck %s --check-prefix=SPLIT_LINK_NULL
+
+// SPLIT_LINK_NULL:  "-dumpdir" "/dev/null-"
+// SPLIT_LINK_NULL-SAME: "-split-dwarf-output" "/dev/null-split-debug.dwo"
+
+/// If -dumpdir is specified, use its value to derive the .dwo filename.
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out -dumpdir pf/x -c 2>&1 | FileCheck %s --check-prefix=DUMPDIR
+// RUN: %clang -### --target=x86_64-unknown-linux-gnu -gsplit-dwarf -g %s -o obj/out -dumpdir pf/x 2>&1 | FileCheck %s --check-prefix=DUMPDIR
+
+// DUMPDIR:  "-dumpdir" "pf/x"
+// DUMPDIR-SAME: "-split-dwarf-output" "pf/xsplit-debug.dwo"
 
 /// -fsplit-dwarf-inlining
 // RUN: %clang -### -c -target x86_64 -gsplit-dwarf=split -g -fsplit-dwarf-inlining %s 2>&1 | FileCheck %s --check-prefixes=INLINE,SPLIT
Index: clang/test/Driver/hip-gsplit-dwarf-options.hip
===
--- clang/test/Driver/hip-gsplit-dwarf-options.hip
+++ clang/test/Driver/hip-gsplit-dwarf-options.hip
@@ -13,13 +13,17 @@
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu \
 // RUN:   --offload-arch=gfx906:xnack+ %s -nogpulib -nogpuinc \
 // RUN:   --offload-arch=gfx900 \
-// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s
+// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s --check-prefix=LINK
 
 // RUN: %clang -### --target=x86_64-unknown-linux-gnu \
 // RUN:   -fgpu-rdc --offload-arch=gfx906:xnack+ %s -nogpulib -nogpuinc \
 // RUN:   --offload-arch=gfx900 \
-// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s
+// RUN:   -ggdb -gsplit-dwarf 2>&1 | FileCheck %s --check-prefix=LINK
 
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "gfx906".* "-split-dwarf-output" "hip-gsplit-dwarf-options_gfx906:xnack\+.dwo"}}
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "gfx900".* "-split-dwarf-output" "hip-gsplit-dwarf-options_gfx900.dwo"}}
 // CHECK-DAG: {{".*clang.*".* "-target-cpu" "x86-64".* "-split-dwarf-output" "hip-gsplit-dwarf-options.dwo"}}
+
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "gfx906".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options_gfx906:xnack\+.dwo"}}
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "gfx900".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options_gfx900.dwo"}}
+// LINK-DAG: {{".*clang.*".* "-target-cpu" "x86-64".* "-split-dwarf-output" "a-hip-gsplit-dwarf-options.dwo"}}
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++