Support -file-compilation-dir #40735

pofat · 2022-01-05T12:00:44Z

This PR adds a new flag -file-compilation-dir, which does the same thing as -ffile-compilation-dir in Clang.

swiftc -g -ffile-compilation-dir=. path/to/foo.swift gives us identical debug info paths regardless of what location we compiled the file from. It's useful to debug correctly using object files built on different machines in different locations.
There's also a long-existed TODO comment.

Resolves SR-5694

kastiglione · 2022-01-05T18:36:26Z

@swift-ci test

kastiglione

seems proper to me

test/DebugInfo/debug_compilation_dir.swift

keith · 2022-01-05T19:28:20Z

Clang has quite a few flags here that imply different things, both for mappings like the $PWD=. style and ones like this. Right now swiftc has -debug-prefix-map and -coverage-prefix-map.

IMO it is worth considering introducing the equivalent of -ffile-prefix-dir . instead of the debug info only specific variant. But I'm just thinking that because we could potentially avoid the large number of related flags down the line.

swift-ci · 2022-01-05T20:17:48Z

Build failed
Swift Test Linux Platform
Git Sha - ce7377ec6b5d36d54ae6430c001f4fe2e0a6a612

adrian-prantl

Thanks, this is a nice improvement! Some comments inline.

lib/Frontend/CompilerInvocation.cpp

test/DebugInfo/debug_compilation_dir.swift

adrian-prantl · 2022-01-05T22:14:34Z

lib/Driver/ToolChains.cpp

@@ -334,6 +334,8 @@ void ToolChain::addCommonFrontendArgs(const OutputInfo &OI,
    auto OptArg = inputArgs.getLastArgNoClaim(options::OPT_O_Group);
    if (!OptArg || OptArg->getOption().matches(options::OPT_Onone))
      arguments.push_back("-enable-anonymous-context-mangled-names");
+
+    inputArgs.AddAllArgs(arguments, options::OPT_debug_compilation_dir);


Does this code need to be added to the Swift implementation of the driver, too? (I haven't looked at the Swift driver code recently so I'm not sure)

Yes, I think so. My plan is to address this in the follow up PR.

I think it may be preferable to do both at once, to keep the implementations in sync at all times.

Created a PR for Swift-implemented driver.

compnerd · 2022-01-07T17:11:38Z

Could you please add a test to ensure that the Windows path separator is handled properly? (\ vs /)

rmaz · 2022-01-07T17:14:35Z

Could you please add a test to ensure that the Windows path separator is handled properly? (\ vs /)

The existing CHECK-ABS test should handle Windows correctly. Would you like to see more tested beyond this?

compnerd · 2022-01-07T18:09:52Z

The existing CHECK-ABS test should handle Windows correctly. Would you like to see more tested beyond this?

Yes, I would. If you note the CHECK-ABS string is looking at a Unix style separator. I would like to see a test that explicitly tests for Windows style paths e.g. S:\SourceCache\swift\stdlib\public\core\Array.swift.

pofat · 2022-01-07T21:52:06Z

Yes, I would. If you note the CHECK-ABS string is looking at a Unix style separator. I would like to see a test that explicitly tests for Windows style paths e.g. S:\SourceCache\swift\stdlib\public\core\Array.swift.

Do you suggest one test for unix and another one for Windows style paths? eg:

// RUN: %target-swiftc_driver -g \
// RUN:   -c -debug-compilation-dir /path/to \
// RUN:   %s -o - -emit-ir | %FileCheck --check-prefix=CHECK-ABS-%target-os %s
// RUN: %empty-directory(%t)

func foo() {}

// CHECK-ABS-ios:       !DIFile(filename: "{{.*}}/debug_compilation_dir.swift", directory: "/path/to")
// CHECK-ABS-macosx:    !DIFile(filename: "{{.*}}/debug_compilation_dir.swift", directory: "/path/to")
// CHECK-ABS-watchos:   !DIFile(filename: "{{.*}}/debug_compilation_dir.swift", directory: "/path/to")
// CHECK-ABS-darwin:    !DIFile(filename: "{{.*}}/debug_compilation_dir.swift", directory: "/path/to")
// CHECK-ABS-objc-linux-gnu:   !DIFile(filename: "{{.*}}/debug_compilation_dir.swift", directory: "/path/to")
// CHECK-ABS-native-linux-gnu: !DIFile(filename: "{{.*}}/debug_compilation_dir.swift", directory: "/path/to")
// CHECK-ABS-objc-windows:     !DIFile(filename: "{{[a-zA-Z]:\\(?:[\\\S|*\S]+[\\\/])}}debug_compilation_dir.swift", directory: "/path/to")
// CHECK-ABS-native-windows:   !DIFile(filename: "{{[a-zA-Z]:\\(?:[\\\S|*\S]+[\\\/])}}debug_compilation_dir.swift", directory: "/path/to")

pofat · 2022-01-10T19:35:06Z

hi @compnerd , the test for windows path is added. Would you trigger the tests? Thank you.

kastiglione · 2022-01-10T21:18:05Z

IMO it is worth considering introducing the equivalent of -ffile-prefix-dir . instead of the debug info only specific variant. But I'm just thinking that because we could potentially avoid the large number of related flags down the line.

bumping this. @pofat what do you think?

kastiglione · 2022-01-10T21:19:58Z

@keith the page linked to for -ffile-prefix-dir doesn't mention that flag.

rmaz · 2022-01-10T21:41:22Z

IMO it is worth considering introducing the equivalent of -ffile-prefix-dir . instead of the debug info only specific variant. But I'm just thinking that because we could potentially avoid the large number of related flags down the line.

bumping this. @pofat what do you think?

Wouldn't we want both? Seems odd that clang would have multiple flags but Swift wouldn't.

keith · 2022-01-10T21:45:23Z

Sorry it's -ffile-compilation-dir=

Fair question for sure, I think clang's setup has gotten a bit complex related to this stuff, so it wasn't that they have many separate flags intentionally, but more because they can't ever remove any for backwards compatibility but that's just my take.

For reference clang has:

-ffile-compilation-dir=
-ffile-prefix-map=
-fcoverage-compilation-dir=
-fcoverage-prefix-map=
-fdebug-compilation-dir=
-fdebug-prefix-map=
-fmacro-prefix-map=

Where the ffile* flags imply all the others

pofat · 2022-01-11T17:38:36Z

Hi @kastiglione @keith, how about having both flags?
Based on the implantation in clang, -ffile-compilation-dir looks like a shorter syntax for both -fdebug-compilation-dir and -fcoverage-compilation-dir to me.

Personally, I would like to have this flag done in this PR, then do -coverage-compilation-dir and -ffile-compilation-dir in the follow-up PRs.

Not sure what's the best strategy to decide what flags should migrate to Swift. Probably just follow what Clang has, IMO.
What do you guys think?

keith · 2022-01-11T18:51:25Z

I will defer to the swift maintainers in general here, but my thought was just instead of doing a 1:1 copy from clang we could try to reduce the user facing complexity here, unless we can come up with compelling reasons for them to be separate.

drodriguez · 2022-01-11T20:05:33Z

@swift-ci please smoke test

2. Implement the logic: Configure DebugCompilationDir with the path specified by the flag, otherwise with current working directory. 3. Add test case

2. Use %target-swiftc_driver for test case instead 3. Add test case for absolute path 4. Code clean

2. Add a test case for path `.` 3. Use regular expression for file path separator 4. Remove std::string() constructor 5. Remove ArgumentIsPath flag

pofat · 2022-01-13T16:02:39Z

Update this PR to support coverage (given that -coverage-prefix-map exists, maybe it's not that much work, I haven't looked)

Change the name of the flag to -file-compilation-dir, but don't add support for coverage yet, leaving it as a TODO/Issue

Keep this PR as is, and add -debug-compilation-dir

Thanks for the clear explanation of all options! Took option 2 and updated this PR. Also fixed the failed windows test.

2. Fix windows test case

kastiglione · 2022-01-13T21:36:12Z

@swift-ci test

kastiglione · 2022-01-13T21:37:11Z

The tests and the PR still mention -debug-compilation-dir.

pofat · 2022-01-13T21:44:03Z

@kastiglione I am sorry, might undo some old code while pushing. All fixed.

swift-ci · 2022-01-13T23:45:05Z

Build failed
Swift Test OS X Platform
Git Sha - 70c4b38

kastiglione · 2022-01-14T00:16:05Z

@swift-ci test

kastiglione · 2022-01-14T18:25:08Z

@swift-ci test Windows Platform

kastiglione · 2022-01-14T22:34:10Z

can you update the flag for the swift-driver PR as well, in case that results in a separate set of eyes?

pofat · 2022-01-15T01:28:41Z

can you update the flag for the swift-driver PR as well, in case that results in a separate set of eyes?

Sure, I can. Working on it.

pofat · 2022-01-17T10:43:39Z

Done updating swift-driver

kastiglione · 2022-01-18T18:01:46Z

@swift-ci test Windows Platform

pofat · 2022-01-19T20:24:48Z

@swift-ci test Windows Platform

Thanks! I am working on fix the failed windows test case. Will update once I fix it.

drodriguez · 2022-01-26T23:04:47Z

@swift-ci please smoke test

pofat · 2022-01-27T07:44:52Z

@kastiglione failed windows test is fixed. Would you like to review it?

pofat · 2022-01-27T22:33:43Z

@adrian-prantl @kastiglione can you help me merger this PR? Thank you!

pofat force-pushed the support_debug_compilation_dir branch 2 times, most recently from e5e911a to ce7377e Compare January 5, 2022 18:30

kastiglione requested a review from adrian-prantl January 5, 2022 18:34

kastiglione reviewed Jan 5, 2022

View reviewed changes

test/DebugInfo/debug_compilation_dir.swift Outdated Show resolved Hide resolved

test/DebugInfo/debug_compilation_dir.swift Outdated Show resolved Hide resolved

adrian-prantl reviewed Jan 5, 2022

View reviewed changes

pofat mentioned this pull request Jan 6, 2022

Support -file-compilation-dir apple/swift-driver#968

Merged

pofat force-pushed the support_debug_compilation_dir branch from 23fba6c to dd128cc Compare January 7, 2022 16:38

pofat requested review from adrian-prantl and kastiglione January 10, 2022 19:19

pofat force-pushed the support_debug_compilation_dir branch 2 times, most recently from e303397 to e8d2e7e Compare January 10, 2022 19:30

pofat added 4 commits January 12, 2022 18:35

1. Add new flag

38e2754

2. Implement the logic: Configure DebugCompilationDir with the path specified by the flag, otherwise with current working directory. 3. Add test case

1. Forward args from driver to frontend

6244a00

2. Use %target-swiftc_driver for test case instead 3. Add test case for absolute path 4. Code clean

1. Remove -wmo flag for it's not relevant to this test

21e7bc8

2. Add a test case for path `.` 3. Use regular expression for file path separator 4. Remove std::string() constructor 5. Remove ArgumentIsPath flag

Add a test case for windows

b761e77

pofat force-pushed the support_debug_compilation_dir branch 2 times, most recently from 25fc8e5 to a76960e Compare January 13, 2022 15:55

pofat force-pushed the support_debug_compilation_dir branch 2 times, most recently from 03e6a31 to 3ec1c0b Compare January 13, 2022 21:12

1. Rename -debug-compilation-dir with -file-compilation-dir

70c4b38

2. Fix windows test case

pofat force-pushed the support_debug_compilation_dir branch from 3ec1c0b to 70c4b38 Compare January 13, 2022 21:16

Fix test case

c487eb3

pofat changed the title ~~Support -debug-compilation-dir~~ Support -file-compilation-dir Jan 13, 2022

adrian-prantl approved these changes Jan 14, 2022

View reviewed changes

Fix windows test case

5dc556f

kastiglione approved these changes Jan 27, 2022

View reviewed changes

drodriguez merged commit 96231e2 into apple:main Jan 28, 2022

This was referenced Jan 28, 2022

Support file compilation dir #41060

Closed

Support file compilation dir #41062

Merged

keith mentioned this pull request Jan 31, 2022

[SR-5694] Support -fdebug-compilation-dir #48264

Closed

Support -file-compilation-dir #40735

Support -file-compilation-dir #40735

Conversation

pofat commented Jan 5, 2022 • edited by kastiglione

kastiglione commented Jan 5, 2022

kastiglione left a comment

Choose a reason for hiding this comment

keith commented Jan 5, 2022

swift-ci commented Jan 5, 2022

adrian-prantl left a comment

Choose a reason for hiding this comment

adrian-prantl Jan 5, 2022

Choose a reason for hiding this comment

pofat Jan 5, 2022

Choose a reason for hiding this comment

adrian-prantl Jan 6, 2022

Choose a reason for hiding this comment

pofat Jan 6, 2022 • edited

Choose a reason for hiding this comment

compnerd commented Jan 7, 2022

rmaz commented Jan 7, 2022

compnerd commented Jan 7, 2022

pofat commented Jan 7, 2022

pofat commented Jan 10, 2022

kastiglione commented Jan 10, 2022 • edited

kastiglione commented Jan 10, 2022

rmaz commented Jan 10, 2022

keith commented Jan 10, 2022

pofat commented Jan 11, 2022

keith commented Jan 11, 2022

drodriguez commented Jan 11, 2022

pofat commented Jan 13, 2022 • edited

kastiglione commented Jan 13, 2022

kastiglione commented Jan 13, 2022

pofat commented Jan 13, 2022

swift-ci commented Jan 13, 2022

kastiglione commented Jan 14, 2022

kastiglione commented Jan 14, 2022

kastiglione commented Jan 14, 2022

pofat commented Jan 15, 2022

pofat commented Jan 17, 2022

kastiglione commented Jan 18, 2022

pofat commented Jan 19, 2022

drodriguez commented Jan 26, 2022

pofat commented Jan 27, 2022

pofat commented Jan 27, 2022

pofat commented Jan 5, 2022 •

edited by kastiglione

pofat Jan 6, 2022 •

edited

kastiglione commented Jan 10, 2022 •

edited

pofat commented Jan 13, 2022 •

edited