Make DWARF debug flag behavior match Clang. #17087

allevato · 2018-06-10T03:55:51Z

Only write the compilation flags to debug info for Mach-O targets, and only
if the RC_DEBUG_OPTIONS environment variable is set.

This change improves hermeticity/reproducibility of Swift binaries/dSYMs by not encoding command line flags that might contain file system paths, since LLDB no longer needs them (except for -private-discriminator, which I've left in; it's invariant because it's computed only from the basename anyway).

@adrian-prantl

Only write the compilation flags to debug info for Mach-O targets, and only if the RC_DEBUG_OPTIONS environment variable is set.

adrian-prantl

This is going into the right direction, I added a suggestion for an alternate wording. Let's also hear if @jrose-apple has any opinions.

adrian-prantl · 2018-06-10T04:06:54Z

include/swift/Option/FrontendOptions.td

@@ -486,4 +486,7 @@ def disable_verify_exclusivity : Flag<["-"], "disable-verify-exclusivity">,
 def enable_key_path_resilience : Flag<["-"], "enable-key-path-resilience">,
  HelpText<"Enable key path resilience.">;

+def use_dwarf_debug_flags : Flag<["-"], "use-dwarf-debug-flags">,
+  HelpText<"Write the compilation flags into the DWARF debug info.">;


Perhaps: -emit-debug-options: Emit the compiler invocation in the debug info.

That alternative seems fine; my only concern would be whether it would be easily confused with -serialize-debugging-options. Or do you think the symmetry between "serialize" and "emit" would intentionally work well and make it clear which one goes where?

I don't love either name; it's not the "debug flags" or "debug options" that are being written. (It also doesn't seem DWARF-specific.) I'll toss another contribution in, -enable-invocation-in-debug-info, but that's getting fairly wordy.

I think it would be nice if the debug-info part came first for grouping and sorting.
-debug-info-(emit|store)-invocation, -debug-info-invocation=yes?

If we're making it a driver flag based on the conversation below, should we prefix it with -g then (but not add it to the g_Group)? How do any of these sound:

-gcompiler-flags

-gcompiler-options

-gcompiler-invocation

-grecord-compiler-flags

-grecord-compiler-options

-grecord-compiler-invocation

Sorry, I'm fine with -debug-info; it's grouping a bunch of things under single letters that I don't like. To compare with GCC/Clang flags:

-f* - something to do with the language? Why -f?

-m* - something to do with code generation? Why -m?

-d* - something to do with dependencies? Or "defines"?

-i* - "includes", okay…but it led us to the horrible -iframework, meaning "system framework include path"

-g is at least consistently used for debugging things, but there's still no obvious link between "-g" and "debug info"; you just have to memorize it. -O, -W, and -X are pretty much the only flag families that stayed consistent, and -W is still weird around the edges.

Also we do have a man page! It just tells you to use -help. :-)

Alright, given the feedback so far, if we exclude prefixed flags, I think -debug-info-store-invocation is the one that best balances conciseness while still getting across what the flag actually does. -enable-invocation-in-debug-info came close, but I agree that it's a bit verbose.

If that sounds good, I'll go ahead and update the PR to use that.

@jrose-apple the prefixes actually have some level of consistency

-f: feature flags
-m: machine flags
-d: dump flags (used for the C Preprocessor)
-i: includes

Except that "feature flag" vs. "machine flag" doesn't make any sense to a normal user, and "-i", while consistent, has led to badness elsewhere.

compnerd · 2018-06-10T19:56:24Z

Please don't limit this to Darwin. This is a spelling issue. -grecord-gcc-switches accomplishes the same thing (it records all of the CC1 flags).

allevato · 2018-06-10T20:22:29Z

@compnerd This PR only changes the default behavior of the driver to match that of Clang, where on non-Darwin platforms it never emits the flags, and on Darwin it only emits them if that environment variable is set. The user can still force the frontend to emit them on other platforms by passing -Xfrontend -use-dwarf-debug-flags (or whatever we end up renaming it to).

Is that sufficient, or do you think we need a driver-level flag for this?

jrose-apple · 2018-06-11T16:06:27Z

Definitely in favor of this direction! I've been after Adrian to remove/disable this feature for a while. :-) I'm inclined to make this a driver-level flag too; if it's useful on Darwin it could easily be useful elsewhere, and no one's supposed to use -Xfrontend options in production code (they're not supported or stable). And environment variables are terrible for controlling things like this.

adrian-prantl · 2018-06-11T16:36:52Z

And environment variables are terrible for controlling things like this.

The environment variable is (good design or not) necessary for uniformity with clang on Apple platforms.

I just wanted to point out that the intent of this feature is to help debugging build systems.
(Historically the flags were also used by LLDB during Swift bringup, but that use-case has since been superseded by information serialized in the .swiftmodule files.)

compnerd · 2018-06-13T00:54:08Z

@allevato - I think as @jrose-apple points out, -Xfrontend options aren't as attractive

allevato · 2018-06-13T01:34:06Z

I've updated the PR to use -debug-info-store-invocation, and renamed the toolchain method that lets it override the behavior if the flag isn't present (to preserve the Clang behavior under Darwin, as discussed above). PTAL.

jrose-apple

Looks reasonable to me! Adrian?

jrose-apple · 2018-06-13T16:29:35Z

test/DebugInfo/compiler-flags-macosx.swift

+// Check that the sdk and resource dirs end up in the debug info if we build for
+// a Darwin target and set the RC_DEBUG_OPTIONS environment variable. This
+// matches the behavior found in Clang.
+// RUN: %target-swiftc_driver %s -emit-ir -g -target x86_64-apple-macosx10.10 -o - | %FileCheck %s


These can just be %swiftc_driver, since you're overriding the target anyway.

jrose-apple · 2018-06-13T16:30:36Z

@swift-ci Please test

swift-ci · 2018-06-13T16:32:20Z

Build failed
Swift Test Linux Platform
Git Sha - 22ac700

adrian-prantl · 2018-06-13T16:33:28Z

lib/FrontendTool/FrontendTool.cpp

-  if (!PD.empty())
-    IRGenOpts.DWARFDebugFlags += (" -private-discriminator " + PD.str()).str();
+  if (!PD.empty()) {
+    if (!IRGenOpts.DWARFDebugFlags.empty()) IRGenOpts.DWARFDebugFlags += " ";


clang-format would format this as:

if (!IRGenOpts.DWARFDebugFlags.empty()) IRGenOpts.DWARFDebugFlags += " ";

swift-ci · 2018-06-13T16:34:08Z

Build failed
Swift Test OS X Platform
Git Sha - 22ac700

allevato · 2018-06-13T17:24:21Z

Thanks all! Latest requested changes are pushed.

adrian-prantl · 2018-06-13T17:28:32Z

@swift-ci test

swift-ci · 2018-06-13T17:30:13Z

Build failed
Swift Test Linux Platform
Git Sha - 0acc73a

swift-ci · 2018-06-13T17:31:42Z

Build failed
Swift Test OS X Platform
Git Sha - 0acc73a

jrose-apple · 2018-06-13T19:50:09Z

Oops, of course. By default, we can't compile for macOS on a non-macOS platform. Adding -parse-stdlib to that test should get around that by not trying to import Swift.

allevato · 2018-06-13T20:01:03Z

Ah-ha, thanks for the tip! Updated.

jrose-apple · 2018-06-13T20:58:09Z

@swift-ci Please test

swift-ci · 2018-06-13T21:00:12Z

Build failed
Swift Test Linux Platform
Git Sha - 88cffc1

swift-ci · 2018-06-13T21:09:37Z

Build failed
Swift Test OS X Platform
Git Sha - 88cffc1

allevato · 2018-06-13T23:03:45Z

Hmm, I'm not sure why the test is crashing when emitting the debug info on Linux after the change adding -parse-stdlib. It looks similar to this IRGen/nominal-type-section.swift test, which works; the only meaningful difference I see is that it's invoking the frontend directly instead of the driver. Could that be the important difference here? (I only have a macOS machine to test locally with.)

jrose-apple · 2018-06-15T15:24:19Z

All right, #17226 should have cleared it up. Let's try again.

@swift-ci Please test Linux

jrose-apple · 2018-06-15T15:24:35Z

And to be sure nothing's changed since a few days ago.

@swift-ci Please smoke test macOS

allevato · 2018-06-15T16:39:12Z

Excellent! Thanks for the related fix on the side.

Make DWARF debug flag behavior match Clang.

22ac700

Only write the compilation flags to debug info for Mach-O targets, and only if the RC_DEBUG_OPTIONS environment variable is set.

adrian-prantl self-assigned this Jun 10, 2018

adrian-prantl requested a review from jrose-apple June 10, 2018 03:57

adrian-prantl reviewed Jun 10, 2018

View reviewed changes

Update flag name to -debug-info-store-invocation.

0acc73a

jrose-apple reviewed Jun 13, 2018

View reviewed changes

adrian-prantl approved these changes Jun 13, 2018

View reviewed changes

Update indentation and %swiftc_driver usage.

88cffc1

Add -parse-stdlib to macOS target test so it builds on Linux.

50c8942

jrose-apple self-assigned this Jun 13, 2018

jrose-apple mentioned this pull request Jun 14, 2018

[DebugInfo] Don't record that the standard library imports itself #17226

Merged

jrose-apple merged commit 8d43ec3 into swiftlang:master Jun 15, 2018

allevato deleted the dwarf-command-line-flags branch June 15, 2018 16:53

Make DWARF debug flag behavior match Clang. #17087

Make DWARF debug flag behavior match Clang. #17087

Uh oh!

Conversation

allevato commented Jun 10, 2018

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Jun 10, 2018

Uh oh!

allevato commented Jun 10, 2018

Uh oh!

jrose-apple commented Jun 11, 2018

Uh oh!

adrian-prantl commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Jun 13, 2018

Uh oh!

allevato commented Jun 13, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jun 13, 2018

Uh oh!

swift-ci commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Jun 13, 2018

Uh oh!

allevato commented Jun 13, 2018

Uh oh!

adrian-prantl commented Jun 13, 2018

Uh oh!

swift-ci commented Jun 13, 2018

Uh oh!

swift-ci commented Jun 13, 2018

Uh oh!

jrose-apple commented Jun 13, 2018

Uh oh!

allevato commented Jun 13, 2018

Uh oh!

jrose-apple commented Jun 13, 2018

Uh oh!

swift-ci commented Jun 13, 2018

Uh oh!

swift-ci commented Jun 13, 2018

Uh oh!

allevato commented Jun 13, 2018

compnerd Jun 13, 2018 •

edited

Loading

adrian-prantl commented Jun 11, 2018 •

edited

Loading