Add option gcodeview #16888

ellishg · 2018-05-29T17:20:40Z

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

ellishg · 2018-05-29T17:21:52Z

CC @compnerd @jrose-apple @dcci

jrose-apple

I feel a little weird mixing the kind of debug info with the amount of debug info. Why is -gdwarf-types different from -g again?

Also, please undo all the whitespace changes that aren't near the code you're changing.

adrian-prantl · 2018-05-29T18:23:10Z

The debug info levels sort by amount of debug info are

-gnone (no debug info)
-gline-tables-only (line tables)
-g (line tables + variables, only type names, type definitions are in .swiftmodules)
-gdwarf-types (line tables + variables + types represented in dwarf)

Note that -g is the default and -gdwarf-types is an experimental and incomplete mode were the actual type definitions are encoded in the debug info.

adrian-prantl · 2018-05-29T18:25:23Z

What's your vision for how this is supposed to work in the end? Are you planning to hook up lldb on Windows with dwarf replaced by codeview or do you have something else in mind?

ellishg · 2018-05-29T18:34:35Z

Yes, lldb on WIndows with CodeView is the goal.

jrose-apple · 2018-05-29T18:37:16Z

Ah, the line tables mode would need to be CodeView-compatible too, right? I think that means this should be an orthogonal option, -codeview-debug-info or -debug-info-format=codeview or something.

compnerd

@jrose-apple - another option would be to support something like -g -gcodeview to specify that you want debug info and you want codeview style information rather than DWARF. -g would be equivalent to -g -gdwarf in that case. That does mean that you could specify something like -gnone -gdwarf. There is precedent for this in clang, it does make it easier to target different versions of DWARF as well. -gdwarf is the default DWARF level, while say -gdwarf-2 would generate DWARF 2.

compnerd · 2018-05-29T18:33:49Z

lib/Frontend/CompilerInvocation.cpp

@@ -764,6 +764,8 @@ static bool ParseIRGenArgs(IRGenOptions &Opts, ArgList &Args,
      Opts.DebugInfoKind = IRGenDebugInfoKind::LineTables;
    else if (A->getOption().matches(options::OPT_gdwarf_types))
      Opts.DebugInfoKind = IRGenDebugInfoKind::DwarfTypes;
+    else if (A->getOption().matches(options::OPT_gcodeview))
+      Opts.DebugInfoKind = IRGenDebugInfoKind::CodeViewTypes;
    else


Might be nice to have some sort of validation that -gdwarf-types and -gcodeview is not specified together.

adrian-prantl · 2018-05-29T18:38:55Z

In clang cc1 we have orthogonal options -gcodeview -debug-info-kind=line-tables-only for this, too.

jrose-apple · 2018-05-29T18:42:58Z

I really don't want to carry over Clang's idea that all debug options need to start with -g. Let's spell things out, please.

adrian-prantl · 2018-05-29T18:43:01Z

Internally, the codeview mode should be a flag that is orthogonal to the debug info level.
For the driver, I think we could have -gcodeview that only switches from DWARF to codeview and specify that in addition to whatever other -g options are present (e.g., -gcode-view -gline-tables-only, -gcode-view -g).

adrian-prantl · 2018-05-29T18:45:45Z

I really don't want to carry over Clang's idea that all debug options need to start with -g. Let's spell things out, please.

I'm fine with a -debug-info-format=codeview option for this, but generally I'd rather keep -g and -gline-tables-only for consistency with many, many other compilers.

jrose-apple · 2018-05-29T18:47:05Z

Yeah, the various -g options as mutually-exclusive modes bothers me less.

ellishg · 2018-05-29T19:23:27Z

So, we want to use -debug-info-format=codeview rather than -gcodeview?

adrian-prantl · 2018-05-29T19:48:43Z

-debug-info-format=[codeview|dwarf] with the default either being dwarf or depending on the host os seems reasonable to me. Let me know if anyone has a better idea for naming.

ellishg · 2018-05-29T23:10:09Z

Should -debug-info-format=codeview imply -g? Or is it reasonable to assume they will be used together?

adrian-prantl · 2018-05-29T23:10:54Z

I think it's cleaner for it not to imply -g.

compnerd

I think that you should extract the debug info kind parameter handling into a small utility function to avoid having to maintain the same list in multiple places.

compnerd · 2018-05-30T20:39:33Z

lib/Driver/Driver.cpp

+    else {
+      Diags.diagnose(SourceLoc(), diag::error_invalid_arg_value,
+                     A->getAsString(Args), A->getValue());
+      return;


I think that this would be nicer as a StringSwitch.

I don't think I can use StringSwitch after moving CodeViewTypes out of IRGenDwarfDebugInfoKind.

compnerd · 2018-05-30T20:39:41Z

lib/Frontend/CompilerInvocation.cpp

@@ -768,6 +768,18 @@ static bool ParseIRGenArgs(IRGenOptions &Opts, ArgList &Args,
      assert(A->getOption().matches(options::OPT_gnone) &&
             "unknown -g<kind> option");

+    if (const Arg *A = Args.getLastArg(options::OPT_debug_info_format)) {
+      if (A->containsValue("dwarf"))


compnerd · 2018-05-30T20:40:40Z

include/swift/AST/IRGenOptions.h

@@ -52,6 +52,7 @@ enum class IRGenDebugInfoKind : unsigned {
  LineTables, /// Line tables only.
  ASTTypes,   /// Line tables + AST type references.
  DwarfTypes, /// Line tables + AST type references + DWARF types.
+  CodeViewTypes, /// Line tables + AST type references + CodeView types.


Thinking more about what @adrian-prantl said, I think it makes sense to split this out and crease IRGenDWARFDebugInfoKind and IRGenCodeViewDebugInfoKind and a separate switch between the two formats.

I can make these changes, but rather than IRGenCodeViewDebugInfoKind I was thinking of making it IRGenCodeViewDebugInfo with only values None and Yes. Would it make more sense to make this a bool?

Nah, err on the side of enums when it's not obvious which one would be the default in the long term. It makes it easier at call sites too.

compnerd · 2018-05-31T18:29:01Z

@adrian-prantl @jrose-apple - do we really want the driver level option to be -debug-info-format? I think that having -gdwarf and -gcodeview is kinda nice to bring over those who have familiarity with gcc/clang.

compnerd · 2018-05-31T00:13:39Z

include/swift/AST/IRGenOptions.h

  None,       /// No debug info.
  LineTables, /// Line tables only.
  ASTTypes,   /// Line tables + AST type references.
  DwarfTypes, /// Line tables + AST type references + DWARF types.
  Normal = ASTTypes /// The setting LLDB prefers.
 };

+enum class IRGenCodeViewDebugInfo : bool {
+  None,
+  Yes


I think Normal would be nicer than Yes.

out of date

adrian-prantl · 2018-05-31T18:36:36Z

do we really want the driver level option to be -debug-info-format? I think that having -gdwarf and -gcodeview is kinda nice to bring over those who have familiarity with gcc/clang.

My expectation is that a normal user would never need to specify this option anyway since the driver would default to whatever format makes most sense for the specified target, and then we might as well use an unambiguous option.

jrose-apple · 2018-06-01T17:07:37Z

include/swift/AST/IRGenOptions.h

+  IRGenDwarfDebugInfoKind DwarfInfoKind : 2;
+
+  /// Whether we should emit codeview debug info.
+  IRGenCodeViewDebugInfo CodeViewInfo : 1;


I'm still not happy with this. Can't CodeView do line tables?

No, unfortunately, CodeView really has no concept of "levels". You generate full fidelity debug info. The generated information is coalesced into a PDB, which can be post-processed to reduce the amount of information.

I think this representation is causing problems elsewhere. We really do want a "level of debug info" setting and a "debug format" setting, not two different "level" settings. (And then probably a separate error saying that CodeView can't do line tables or full type info—the experimental "DWARF types" option.)

jrose-apple · 2018-06-01T17:10:29Z

lib/Driver/Driver.cpp

    else
      assert(A->getOption().matches(options::OPT_gnone) &&
             "unknown -g<kind> option");
  }

+  if (const Arg *A = Args.getLastArg(options::OPT_debug_info_format)) {
+    if (A->containsValue("dwarf"))


Nitpick: Since this isn't a multi-valued option, comparing against getValue is probably clearer.

jrose-apple · 2018-06-01T17:11:11Z

lib/Driver/Driver.cpp

+    else {
+      Diags.diagnose(SourceLoc(), diag::error_invalid_arg_value,
+                     A->getAsString(Args), A->getValue());
+      return;


No need to early-exit here. There's already an error; we might as well diagnose any other errors that come up. (I realize we're not consistent about this.)

ellishg · 2018-06-14T00:25:01Z

@jrose-apple Any other changes you would like to see?

jrose-apple

Thank you, I like this version a lot better! @adrian-prantl, any remaining concerns?

jrose-apple · 2018-06-14T16:21:55Z

lib/Driver/Driver.cpp

+    Diags.diagnose(SourceLoc(), diag::error_option_missing_required_argument,
+                   debugFormatArg->getAsString(Args), "-g");
+  } else if (OI.DebugInfoFormat == IRGenDebugInfoFormat::CodeView &&
+             OI.DebugInfoLevel != IRGenDebugInfoLevel::Normal) {


Ah, strictly speaking -gnone -debug-info-format=codeview should be allowed, right? (Could be useful for autogenerated command lines.)

I can specifically allow -g -debug-info-format=codeview and -gnone -debug-info-format=codeview, but as the code is written that would also allow just -dwarf-info-format=codeview.

Do we want any kind of warning in case someone forgets -g?

I think what you had earlier is reasonable as long as it checks for the presence of a -g* flag rather than just "oh, the debug level is None".

adrian-prantl

This looks good now (minor nitpicks inside).

adrian-prantl · 2018-06-14T16:42:22Z

lib/Frontend/CompilerInvocation.cpp

+    // If -g was specified but not -debug-info-format, DWARF is assumed.
+    Opts.DebugInfoFormat = IRGenDebugInfoFormat::DWARF;
+  }
+  if (Opts.DebugInfoFormat > IRGenDebugInfoFormat::None &&


I personally would use != here, since these values aren't ordered like the DebugInfoLevel.

adrian-prantl · 2018-06-14T16:44:34Z

include/swift/AST/IRGenOptions.h

@@ -47,14 +47,20 @@ enum class IRGenOutputKind : unsigned {
  ObjectFile
 };

-enum class IRGenDebugInfoKind : unsigned {
+enum class IRGenDebugInfoLevel : unsigned {
  None,       /// No debug info.


Not your code, but: These should be ///< comments.

ellishg · 2018-06-14T18:24:45Z

Any final nitpicks?

jrose-apple · 2018-06-14T20:52:43Z

Ah, there's conflicts now, so you'll have to rebase to resolve those.

jrose-apple · 2018-06-14T21:20:49Z

Or that, I guess.

@swift-ci Please test

swift-ci · 2018-06-14T21:22:23Z

Build failed
Swift Test OS X Platform
Git Sha - 64bf3393b2f03608ad454dc477d6621ad705227c

swift-ci · 2018-06-14T21:23:51Z

Build failed
Swift Test Linux Platform
Git Sha - 4cbdf4bb4a509143e0e1cbf5338df33f16aacbe4

jrose-apple · 2018-06-14T23:15:36Z

Looks like you'll need to do an update to LLDB too. We can then test and land them together.

ellishg · 2018-06-16T00:38:28Z

What do you mean do an update to LLDB?

jrose-apple · 2018-06-18T15:39:23Z

Since LLDB (https://github.com/apple/swift-lldb) also refers to some of the IRGenOptions fields you're changing, you'll need to submit a pull request there as well (probably just a find/replace of those names).

ellishg · 2018-06-19T03:22:46Z

@jrose-apple Can we test LLDB (apple/swift-lldb#697) and swift together?

adrian-prantl · 2018-06-19T15:33:58Z

test with apple/swift-lldb#697
@swift-ci test

swift-ci · 2018-06-19T15:35:50Z

Build failed
Swift Test Linux Platform
Git Sha - 59a7406f2e8097968052c42aee8e50f5544c8c43

swift-ci · 2018-06-19T15:37:35Z

Build failed
Swift Test OS X Platform
Git Sha - 59a7406f2e8097968052c42aee8e50f5544c8c43

jrose-apple · 2018-06-20T16:53:21Z

Thanks, Ellis!

ellishg · 2018-06-20T16:54:08Z

Thank you for all your help!

jrose-apple reviewed May 29, 2018

View reviewed changes

jrose-apple requested review from compnerd and adrian-prantl May 29, 2018 18:14

ellishg force-pushed the gcodeview branch from 7f1f1ad to 37faac6 Compare May 29, 2018 18:29

compnerd reviewed May 29, 2018

View reviewed changes

ellishg force-pushed the gcodeview branch from 37faac6 to 06a1663 Compare May 29, 2018 23:06

compnerd previously requested changes May 30, 2018

View reviewed changes

ellishg force-pushed the gcodeview branch 3 times, most recently from f79c4b8 to d7985d4 Compare May 30, 2018 22:26

compnerd reviewed May 31, 2018

View reviewed changes

ellishg force-pushed the gcodeview branch from d7985d4 to 8c658fe Compare May 31, 2018 21:40

jrose-apple reviewed Jun 1, 2018

View reviewed changes

ellishg force-pushed the gcodeview branch from c8568e2 to 4cbdf4b Compare June 13, 2018 16:39

jrose-apple reviewed Jun 14, 2018

View reviewed changes

adrian-prantl approved these changes Jun 14, 2018

View reviewed changes

ellishg force-pushed the gcodeview branch 4 times, most recently from 0294524 to 4c3920e Compare June 14, 2018 17:56

ellishg force-pushed the gcodeview branch from 4c3920e to 46f7150 Compare June 14, 2018 19:27

ellishg force-pushed the gcodeview branch from 6d337fe to 59a7406 Compare June 14, 2018 21:19

jrose-apple self-assigned this Jun 14, 2018

Add -debug-info-format=[dwarf|codeview] option

12379fe

ellishg force-pushed the gcodeview branch from c9becff to 12379fe Compare June 16, 2018 00:33

adrian-prantl mentioned this pull request Jun 19, 2018

[swift] Rename IRGenDebugInfoLevel enum to stay compatible with swift apple/swift-lldb#697

Merged

jrose-apple merged commit c93a5a5 into swiftlang:master Jun 20, 2018

ellishg deleted the gcodeview branch June 22, 2018 00:29

Add option gcodeview #16888

Add option gcodeview #16888

Uh oh!

Conversation

ellishg commented May 29, 2018

Uh oh!

ellishg commented May 29, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented May 29, 2018

Uh oh!

adrian-prantl commented May 29, 2018

Uh oh!

ellishg commented May 29, 2018

Uh oh!

jrose-apple commented May 29, 2018

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented May 29, 2018

Uh oh!

jrose-apple commented May 29, 2018

Uh oh!

adrian-prantl commented May 29, 2018

Uh oh!

adrian-prantl commented May 29, 2018

Uh oh!

jrose-apple commented May 29, 2018

Uh oh!

ellishg commented May 29, 2018

Uh oh!

adrian-prantl commented May 29, 2018

Uh oh!

ellishg commented May 29, 2018

Uh oh!

adrian-prantl commented May 29, 2018

Uh oh!

compnerd 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!

compnerd commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented May 31, 2018

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!

ellishg commented Jun 14, 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!