Ast dump ignore wmo #20596

vinivendra · 2018-11-15T12:19:14Z

The current Dump AST methods dump the AST for primary files or, if there are no primary files, the main file. This behavior is weird on WMO builds, which don't have the concept of primary files. In that case, the compiler currently dumps the main file and ignores all other files.

Since it's uncommon to have the -dump-ast argument in an invocation, it is assumed to be a priority over -wmo. Therefore, the decision is to ignore -wmo when -dump-ast is also present, printing a warning for the user and dumping all files normally.

This was briefly discussed on PR-20392, where it was decided that a FIXME would be added to lib/FrontendTool/FrontendTool.cpp and the change would be delayed to a separate pull request.

This pull request executes the necessary changes on Driver.cpp and DiagnosticsDriver.def and removes the FIXME from FrontendTool.cpp. It also fixes a bit of the code style in DiagnosticsDriver.def.

vinivendra · 2018-11-15T12:21:00Z

Cc @jrose-apple, who's been following these changes.

jrose-apple · 2018-11-15T21:13:25Z

include/swift/AST/DiagnosticsDriver.def

@@ -42,91 +42,92 @@
 DIAG(REMARK,ID,Options,Text,Signature)
 #endif

-WARNING(warning_parallel_execution_not_supported,none,
+WARNING(warning_parallel_execution_not_supported, none,


Please don't include formatting changes for code you're not touching.

Oh, ok. I thought it'd be useful to do this as I was adding a new warning, but yeah, maybe not.

jrose-apple · 2018-11-15T21:14:10Z

lib/Driver/Driver.cpp

+  // AST dump doesn't work in WMO. Since it's not common want to dump the AST,
+  // we assume that's the priority and ignore `-wmo`, but we warn the user about
+  // this decision.
+  if (Args.hasArg(options::OPT_whole_module_optimization) &&


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

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.

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

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

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

jrose-apple · 2018-11-15T21:14:18Z

lib/Driver/Driver.cpp

@@ -1653,6 +1653,15 @@ Driver::computeCompilerMode(const DerivedArgList &Args,
  if (!ArgRequiringSingleCompile)
    return OutputInfo::Mode::StandardCompile;

+  // AST dump doesn't work in WMO. Since it's not common want to dump the AST,


Typo: "not common want to".

vinivendra · 2018-11-15T21:49:38Z

Should I do a rebase on master every time I update the pull request? It seems like the right thing to do, but at the same time it seems to make github act a little weird.

jrose-apple · 2018-11-15T21:51:30Z

Should I do a rebase on master every time I update the pull request? It seems like the right thing to do, but at the same time it seems to make github act a little weird.

I think most of us do do force pushes, which is 90% of GitHub's weirdness, but you don't need to rebase every time if there's not actually a conflict.

jrose-apple · 2018-11-16T17:46:31Z

Thanks, this looks good. Can you add a test case for it in test/Driver/ somewhere?

vinivendra · 2018-11-16T19:06:11Z

Heheh, adding tests reminded me why I hadn't ignored -index-file in the first place: if I ignore it the same way as -wmo in the code but then pass -index-file after -dump-ast in the command line, my option gets overwritten and un-ignored (inDriver.cpp, around line 1410).

This seems to happen because the compiler checks for the last argument (Args.getLastArg(options::OPT_modes_Group)), and I didn't find a good way to fix this yet. So I'm just gonna revert it back to only ignoring -wmo, add a FIXME for -index-file, write the tests, and leave that for a future PR.

jrose-apple · 2018-11-26T18:40:51Z

include/swift/AST/DiagnosticsDriver.def

-WARNING(warn_ignoring_single_compile, none,
-        "ignoring '%0' because '-dump-ast' was also specified", (StringRef))
+WARNING(warn_ignoring_wmo, none,
+        "ignoring '-wmo' because '-dump-ast' was also specified", ())


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

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.

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

CodaFi · 2019-11-18T21:18:28Z

@vinivendra Are you still interested in pursuing this change?

vinivendra · 2019-11-21T17:36:21Z

@CodaFi Yes!
I’ve been busy with other things and unfortunately this got left behind. I suppose by now it’d be better to start again from scratch since the code is probably very different.
That said, I’m more interested in using this feature than implementing it myself, so if anyone else wants to take a crack at it they’re welcome.

CodaFi · 2019-11-21T18:38:43Z

It looks like this diff still applies just fine. You should probably rebase and retest to make sure, but we don't have to start over if you don't want to. Jordan still has unaddressed feedback, though, which is the most important thing.

If you would like, I can close this and file an SR to let somebody else try their hand at this.

CodaFi · 2019-11-22T15:55:54Z

I spoke too soon. This needs a quick rebase.

vinivendra · 2019-11-24T15:38:45Z

You're right, these changes still apply well. I just performed a rebase and everything looks OK.

As for Jordan's feedback (where he suggested using ArgRequiringSingleCompile->getSpelling()), I just tried it but it prints '-whole-module-optimization' even when I pass '-wmo'. I also tried this (to make sure I'm printing the wmo argument and not the index-file argument), but the same thing happens:

const Arg *ArgWMO = Args.getLastArg(options::OPT_whole_module_optimization);
Diags.diagnose(SourceLoc(), diag::warn_ignoring_wmo, ArgWMO->getSpelling());

And I tried this, but the program crashes:

const Arg *ArgWMO = Args.getLastArg(options::OPT_wmo);
Diags.diagnose(SourceLoc(), diag::warn_ignoring_wmo, ArgWMO->getSpelling());

Any ideas?

jrose-apple · 2019-12-05T23:25:55Z

Weird. I thought that was the point of getSpelling(). There's also getAsString but that seems like overkill.

I'm no longer at Apple at this point, but there's not too much here, and this diagnostic bit shouldn't hold up the rest of the patch if it's not coming together easily. If you rebase, @CodaFi or @brentdax can do the final bit of shepherding it in.

Ignoring both `-wmo` and `-index-file` will be harder than just `-wmo`. This is because when calling the compiler and passing `-index-file` after `-dump-ast`, the option gets un-ignored by `Driver::buildOutputInfo`. Therefore, we will just ignore `-wmo` for now.

vinivendra · 2019-12-06T23:00:42Z

Ok then! It's rebased and should be ready to merge.

CodaFi · 2019-12-07T00:25:39Z

Will manually squash this down when it passes

@swift-ci please smoke test

vinivendra · 2019-12-19T18:30:32Z

I don't mean to pressure anyone, but it seems the tests have passed - and I'm afraid if we wait much longer we'll need a new rebase and new tests. Is this still OK to merge?

CodaFi · 2019-12-19T19:47:40Z

⛵️

jrose-apple reviewed Nov 15, 2018

View reviewed changes

jrose-apple reviewed Nov 26, 2018

View reviewed changes

vinivendra added 7 commits December 6, 2019 19:17

Ignore -wmo when passing -dump-ast

21debbc

Cleanup on driver diagnostics

8d8b1ed

Remove the FIXME.

d31e473

Add support for ignoring -index-file

3618e63

Revert unrelated formatting changes

7088ff7

Add tests, inspired by Driver/batch_mode_with_WMO_or_index.swift

b515a65

vinivendra force-pushed the ast-dump-ignore-wmo branch from 3f3ec79 to b515a65 Compare December 6, 2019 22:59

beccadax assigned CodaFi Dec 12, 2019

CodaFi merged commit aebfb53 into swiftlang:master Dec 19, 2019

vinivendra deleted the ast-dump-ignore-wmo branch December 20, 2019 18:19

Ast dump ignore wmo #20596

Ast dump ignore wmo #20596

Uh oh!

Conversation

vinivendra commented Nov 15, 2018

Uh oh!

vinivendra commented Nov 15, 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!

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!

vinivendra commented Nov 15, 2018

Uh oh!

jrose-apple commented Nov 15, 2018

Uh oh!

jrose-apple commented Nov 16, 2018

Uh oh!

vinivendra commented Nov 16, 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!

CodaFi commented Nov 18, 2019

Uh oh!

vinivendra commented Nov 21, 2019

Uh oh!

CodaFi commented Nov 21, 2019

Uh oh!

CodaFi commented Nov 22, 2019

Uh oh!

vinivendra commented Nov 24, 2019

Uh oh!

jrose-apple commented Dec 5, 2019

Uh oh!

vinivendra commented Dec 6, 2019

Uh oh!

CodaFi commented Dec 7, 2019

Uh oh!

vinivendra commented Dec 19, 2019

Uh oh!

CodaFi commented Dec 19, 2019

Uh oh!

Uh oh!