[Batch Mode] rdar://40167351 Replace non-specific error with truncated serialized diagnostics files. #17131

davidungar · 2018-06-12T04:57:42Z

Instead of emitting non-specific errors, emit zero-length serialized diagnostics files.

davidungar · 2018-06-12T05:08:54Z

@graydon @jrose-apple This is very preliminary; haven't tested it yet, even. Do either of you want to take a quick look to comment on the overall approach?

graydon · 2018-06-12T06:22:42Z

include/swift/AST/DiagnosticConsumer.h

+  virtual bool finishProcessing() { return false; }
+
+  /// In batch mode, any error causes failure for all primary files, but
+  /// Xcode will only see an error for a particular primary in that primary's


While in this case the motivation involves Xcode, the explanation of who-sees-what here applies to anyone consuming .dia files.

I really don't like how something intrinsic to FileSpecificDiagnosticConsumer is leaking out into the other consumers. (If it wasn't, you could just put this logic in finishProcessing, but we know we can't do that because that would affect single-file and WMO.)

Fixed (not pushed yet). Thanks!

graydon · 2018-06-12T06:33:25Z

lib/Frontend/SerializedDiagnosticConsumer.cpp


    OS->write((char *)&State->Buffer.front(), State->Buffer.size());
    OS->flush();
    return false;
  }

+  bool informDriverThatCompilationCouldNotBeCompleted() const override {
+    // Hack! When the compiler ends abnormally, but there are no contents in


Mm, doesn't seem to me like a hack; this is the intended and to-be-documented behaviour / encoding of "incomplete compilation of batch member in a batch with an erroneous primary", no?

I am happy to remove "Hack!" As you point out, it is the discarding of warnings, etc., that led me to think this is a hack. I will elaborate the comment.

graydon

I'm happy to see you moving on this! Thanks for doing this today.

I'm not 100% sure we decided / determined that this behaviour was desirable, given that it drops non-error diags -- warnings -- in the cancelled files, right?

Also unsure why you've stopped threading the sourceMgr through all the calls to finishProcessing; it looks to my eye like those arguments could remain. Maybe they were just superfluous so you were taking the opportunity to clean up?

jrose-apple · 2018-06-12T16:17:59Z

include/swift/AST/DiagnosticConsumer.h

+  virtual bool finishProcessing() { return false; }
+
+  /// In batch mode, any error causes failure for all primary files, but
+  /// Xcode will only see an error for a particular primary in that primary's


I really don't like how something intrinsic to FileSpecificDiagnosticConsumer is leaking out into the other consumers. (If it wasn't, you could just put this logic in finishProcessing, but we know we can't do that because that would affect single-file and WMO.)

jrose-apple · 2018-06-12T16:20:28Z

lib/Frontend/SerializedDiagnosticConsumer.cpp

+  bool informDriverThatCompilationCouldNotBeCompleted() const override {
+    // Hack! When the compiler ends abnormally, but there are no contents in
+    // serialized diagnostics
+    return openOutputFile() == nullptr;


It is not obvious to me-pretending-to-read-the-code-without-context why you're comparing the result here to null and then throwing it away.

Yeah, probably my and jordan's reviews here could both be addressed by replacing the comment with something like: "Open and immediately close a diagnostics output file, thereby creating a zero-sized file as a sentinel value to signal to the driver that this consumer was interrupted. Compare the resulting file to nullptr as a check for errors."

@jordan Thanks for looking at this so soon! Great catch on the "not obvious" part. I've clarified the code. Will rename informDriverThatCompilationCouldNotBeCompleted per our offline conversation.

jrose-apple · 2018-06-12T16:22:54Z

I think I'm okay with not getting the warnings because there was no guarantee those files wouldn't have had errors later.

davidungar · 2018-06-12T16:31:07Z

@graydon You are welcome! Glad I could start on it last night. Seems urgent. Thank you for looking at it so promptly.

"I'm not 100% sure we decided / determined that this behaviour was desirable, given that it drops non-error diags -- warnings -- in the cancelled files, right?"

Yes, I'm not thrilled with that aspect, either, but had thought we had decided to go that way. Feel free to suggest an alternative that meets our constraints.

"Also unsure why you've stopped threading the sourceMgr through all the calls to finishProcessing; it looks to my eye like those arguments could remain. Maybe they were just superfluous so you were taking the opportunity to clean up?"

Sorry, should have explained: that argument was only there to enable the non-specific error creation. I had put it in when I added the non-specific errors. So it seemed appropriate to delete it when no longer needed, preserving the original design more closely.

davidungar · 2018-06-12T16:36:51Z

@jordan @graydon There are two subtleties in these changes that deserve some consideration:

The iteration for FileSpecificDiagnosticConsumer::finishProcessing now uses ConsumersOrderedByRange instead of SubConsumers, and
A ConsumerPair with an empty string (i.e. primary filename if I understand it) is now an assertion violation.

Sorry, should have mentioned this last night.

graydon · 2018-06-12T16:46:31Z

If Jordan is ok dropping the warnings in the non-error primaries, I'm ok with it too; rethinking it, that's congruent with the "emulate the behaviour of single-file mode" we were aiming for in conversation yesterday (as I argued then!) I just forgot the detailed implication.

Concerning iteration order: why did it change? Just to get access to the enclosing info object?

Concerning assertion on empty string: why? Is it definitely the case that no subconsumers will be created for empty-string filenames? (isn't that what, say, a diagnostic with an invalid location produces?)

davidungar · 2018-06-12T16:58:44Z

Iteration order changed: yes, to get access to the info object with its flag.

davidungar · 2018-06-12T17:05:59Z

Per offline-conversation with @graydon , I will double-check the empty string case, which may be a relic from "whether we make a multiplexor for invalid locs, or just throw invalid-loc errors away", as you suggested.

davidungar · 2018-06-12T17:07:03Z

Per offline conversation with @jrose-apple I will rename informDriverThatCompilationCouldNotBeCompleted to include mention of BatchMode or FileSpecificDiagnosticConsumer.

davidungar · 2018-06-12T17:07:37Z

Per offline-conversation with @graydon, the difference in order in the finishProcessing loop should not be observable.

davidungar · 2018-06-12T17:39:21Z