Skip to content

Ast dump ignore wmo #20596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Dec 19, 2019
Merged

Ast dump ignore wmo #20596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
21debbc
Ignore -wmo when passing -dump-ast
vinivendra Nov 14, 2018
8d8b1ed
Cleanup on driver diagnostics
vinivendra Nov 14, 2018
d31e473
Remove the FIXME.
vinivendra Nov 15, 2018
3618e63
Add support for ignoring `-index-file`
vinivendra Nov 15, 2018
7088ff7
Revert unrelated formatting changes
vinivendra Nov 15, 2018
9aa8bd0
Revert back to only ignoring `-wmo`
vinivendra Nov 16, 2018
b515a65
Add tests, inspired by `Driver/batch_mode_with_WMO_or_index.swift`
vinivendra Nov 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/swift/AST/DiagnosticsDriver.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ WARNING(warn_opt_remark_disabled, none,
WARNING(warn_ignoring_batch_mode,none,
"ignoring '-enable-batch-mode' because '%0' was also specified", (StringRef))

WARNING(warn_ignoring_wmo, none,
"ignoring '-wmo' because '-dump-ast' was also specified", ())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was better the way it was before, since there are several ways to spell the flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The place from which I copied this code (Driver.cpp at line 1672, just below my changes) used ArgRequiringSingleCompile->getOption().getPrefixedName(). As far as I can tell, this will always print "-whole-module-optimization", even if you pass the shortened -wmo version.

The snippet I copied uses this method to distinguish between printing -whole-module-optimization or -index-file, but I don't think it'll have the desired effect here unless we change the way we get the flag name too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, then we should probably be using ArgRequiringSingleCompile->getSpelling() instead, probably in both places.


WARNING(warn_ignoring_source_range_dependencies,none,
"ignoring '-enable-source-range-dependencies' because '%0' was also specified", (StringRef))

Expand Down
11 changes: 11 additions & 0 deletions lib/Driver/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1758,6 +1758,17 @@ Driver::computeCompilerMode(const DerivedArgList &Args,
const Arg *ArgRequiringSinglePrimaryCompile =
Args.getLastArg(options::OPT_enable_source_range_dependencies);

// AST dump doesn't work with `-wmo`. Since it's not common to want to dump
// the AST, we assume that's the priority and ignore `-wmo`, but we warn the
// user about this decision.
// FIXME: AST dump also doesn't work with `-index-file`, but that fix is a bit
// more complicated than this.
if (Args.hasArg(options::OPT_whole_module_optimization) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're ignoring OPT_index_file here, compared to the logic above. If that's intentional, it deserves a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about ignoring OPT_index_file too, but that seems to be a bit more complicated than just adding it in this check, and it's probably not as important (no one seems to have complained about it or tried to fix it so far).

I'll add a FIXME saying OPT_index_file should probably also be ignored in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh never mind, adding -index-file to the check seems to have worked after all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just reuse the boolean from above, so that it's always consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm reusing the boolean, but I think a separate if statement makes the code easier to understand... do you agree?

Args.hasArg(options::OPT_dump_ast)) {
Diags.diagnose(SourceLoc(), diag::warn_ignoring_wmo);
return OutputInfo::Mode::StandardCompile;
}

// Override batch mode if given -wmo or -index-file.
if (ArgRequiringSingleCompile) {
if (BatchModeOut) {
Expand Down
4 changes: 0 additions & 4 deletions lib/FrontendTool/FrontendTool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,10 +893,6 @@ static SourceFile *getPrimaryOrMainSourceFile(CompilerInvocation &Invocation,
/// files were specified, use them; otherwise, dump the AST to stdout.
static void dumpAST(CompilerInvocation &Invocation,
CompilerInstance &Instance) {
// FIXME: WMO doesn't use the notion of primary files, so this doesn't do the
// right thing. Perhaps it'd be best to ignore WMO when dumping the AST, just
// like WMO ignores `-incremental`.

auto primaryFiles = Instance.getPrimarySourceFiles();
if (!primaryFiles.empty()) {
for (SourceFile *sourceFile: primaryFiles) {
Expand Down
38 changes: 38 additions & 0 deletions test/Driver/ast_dump_with_WMO.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// RUN: %empty-directory(%t)


// Check that -wmo is ignored when -dump-ast is present and that the AST gets
// dumped to stdout, no matter the order of the options

// RUN: %swiftc_driver -whole-module-optimization -dump-ast -module-name=main %S/../Inputs/empty.swift -### 2>%t/stderr_WMO_dump | %FileCheck -check-prefix CHECK-STDOUT %s
// RUN: %swiftc_driver -dump-ast -whole-module-optimization -module-name=main %S/../Inputs/empty.swift -### 2>%t/stderr_dump_WMO | %FileCheck -check-prefix CHECK-STDOUT %s
// RUN: %FileCheck -check-prefix CHECK-WMO %s <%t/stderr_WMO_dump
// RUN: %FileCheck -check-prefix CHECK-WMO %s <%t/stderr_dump_WMO


// Check that ignoring -wmo doesn't affect the output file paths for the AST
// dumps

// RUN: cd %t
// RUN: echo ' ' > a.swift
// RUN: echo ' ' > main.swift
// RUN: echo '{ "a.swift": { "ast-dump": "a.ast" }, "main.swift": { "ast-dump": "main.ast" }}' > outputFileMap.json

// RUN: %swiftc_driver -whole-module-optimization -dump-ast -module-name=main -output-file-map=outputFileMap.json main.swift a.swift -### 2>%t/stderr_WMO_OFM | %FileCheck -check-prefix CHECK-OFM %s
// RUN: %FileCheck -check-prefix CHECK-WMO %s <%t/stderr_WMO_OFM



// CHECK-WMO: warning: ignoring '-wmo' because '-dump-ast' was also specified

// CHECK-STDOUT-NOT: -whole-module-optimization
// CHECK-STDOUT-NOT: -wmo
// CHECK-STDOUT: -dump-ast
// CHECK-STDOUT: -o -

// CHECK-OFM-NOT: -whole-module-optimization
// CHECK-OFM-NOT: -wmo
// CHECK-OFM: -dump-ast
// CHECK-OFM-SAME: -o main.ast
// CHECK-OFM-NEXT: -dump-ast
// CHECK-OFM-SAME: -o a.ast