Page MenuHomePhabricator

[mlir] Enable passing crash reproducer stream factory method
ClosedPublic

Authored by jpienaar on Jan 16 2021, 8:58 AM.

Details

Summary

Add factory to create streams for logging the reproducer. Allows for more general logging (beyond file) and logging the configuration/module separately (logged in order, configuration before module).

Diff Detail

Event Timeline

jpienaar created this revision.Jan 16 2021, 8:58 AM
jpienaar requested review of this revision.Jan 16 2021, 8:58 AM
mehdi_amini added inline comments.Jan 16 2021, 1:04 PM
mlir/include/mlir/Pass/PassManager.h
213

Could we reduce the API surface?
I feel like like close() could be handled by the dtor.
A single stream could be handle by one method: Optional<raw_ostream> getStream().

mlir/lib/Pass/Pass.cpp
709

Could we reduce the API surface?
I feel like

752–754

(Spell the type)

757

Making this configurable is breaking the mlir-opt option to load a reproducer: the reproducer is no longer a self-contained "known" format, do you really need this hook? Can we always print to the same stream instead?

jpienaar added inline comments.Jan 19 2021, 9:21 AM
mlir/include/mlir/Pass/PassManager.h
213

close is required by ToolOutput today to differentiate between completed and not completed dumps. It is not required if one just streams without needing any cleanup - but then one needs some checking later to verify if a failure got dumped. Or are you suggesting best effort, just always assume it will complete and other parts (e.g. if file is not saved in the end, just pass the name and assume consumers are robust enough).

So would you propose that getStream opens the stream too and return none if failed? (not sure why optional)

I considered a single stream, but there seems to be 2 pieces of information being captured here: pass pipeline and module/top-level object, we mix them by using comments but that means you need to code a convention and re-extract. Which is needed if we got to file, but if stored separately then the convention doesn't help. I don't feel super strong here, as extracting the string out is easy. An option is also to not make this pure virtual and so the common case is you are reusing the same stream.

mlir/lib/Pass/Pass.cpp
757

It is self-contained if you use a filename :) it isn't if you log it to (say) a DB, but for that case you would need to do work. But you are correct: if you now implement a different file logger using the specialization, then you'd need to duplicate the same conventions.

rdzhabarov added inline comments.Jan 19 2021, 12:07 PM
mlir/include/mlir/Pass/PassManager.h
200

StringRef?

mlir/lib/Pass/Pass.cpp
705

private this and the outputFile.

746

nit: file reproducer stream.

751–752

how error is used?

mehdi_amini added inline comments.Jan 19 2021, 2:38 PM
mlir/include/mlir/Pass/PassManager.h
213

close is required by ToolOutput today to differentiate between completed and not completed dumps.

Is there a code path today where we discard the output? It seems to me that we only invoke the factory to create the ReproducerStream after we decide we want to log in the first place, i.e. close() is called unconditionally which drove my comment here.

So would you propose that getStream opens the stream too and return none if failed? (not sure why optional)

Yes, if you want to say that this can fail. Another way would be to make getStream not able to fail and rely on the factory to return nullptr.

I considered a single stream, but there seems to be 2 pieces of information being captured here: pass pipeline and module/top-level object, we mix them by using comments but that means you need to code a convention and re-extract.

Right, and we implemented this convention at this point I think with mlir-opt able to load a reproducer.
I'd just be in favor of YAGNI and KIS here :)

rriddle added inline comments.Jan 19 2021, 2:46 PM
mlir/include/mlir/Pass/PassManager.h
213

Is there a code path today where we discard the output? It seems to me that we only invoke the factory to create the ReproducerStream after we decide we want to log in the first place, i.e. close() is called unconditionally which drove my comment here.
Yes, if you want to say that this can fail. Another way would be to make getStream not able to fail and rely on the factory to return nullptr.

+1 to both of these points. I don't see a reason for an explicit close, the destructor of the stream can just handle that.

Right, and we implemented this convention at this point I think with mlir-opt able to load a reproducer.

+1 here as well. Unless there is a concrete and compelling reason, I'd rather just have one stream.

Does this struct need anything other than just:

virtual std::string description() = 0;
virtual raw_ostream &os() = 0;
jpienaar added inline comments.Jan 20 2021, 8:26 AM
mlir/include/mlir/Pass/PassManager.h
213

Is there a code path today where we discard the output?

Yes, if the process gets interrupted. So the assumption would be that no matter what we have outputted to stream successfully. And any post processing is fine (e.g., any corruption of the stream is acceptable, atomicity is assumed).

Does this struct need anything other than just:

virtual std::string description() = 0;
virtual raw_ostream &os() = 0;

Opening the stream may be expensive, but can be done in the factory. It needs no more than that if we flatten the information using a convention and assume commiting the written output on destruction is always fine.

mlir/lib/Pass/Pass.cpp
751–752

That is the error being reported.

rdzhabarov added inline comments.Jan 20 2021, 9:24 AM
mlir/lib/Pass/Pass.cpp
751–752

I do not see it used anywhere in the method =)

mehdi_amini added inline comments.Jan 20 2021, 1:27 PM
mlir/include/mlir/Pass/PassManager.h
213

Yes, if the process gets interrupted.

You mean if the process crash while printing? during the first line here:

// Output the .mlir module.
preCrashOperation->print(crashStream->operationStream());
crashStream->close();

In this case we won't call the destructor either, so it should be equivalent isn't it?
You can slightly change the client code to make sure the crashStream is destroyed right at the point you call close here instead, if you really want to be strict about it.

jpienaar updated this revision to Diff 318177.Jan 21 2021, 6:18 AM

Exposing simpler API

jpienaar updated this revision to Diff 318183.Jan 21 2021, 6:55 AM

Fix error return string not piped through.

jpienaar updated this revision to Diff 318188.Jan 21 2021, 7:00 AM
jpienaar marked 10 inline comments as done.

Spell out type & return error rather than report locally

mlir/lib/Pass/Pass.cpp
751–752

Ah, good catch thanks

nice!

mlir/lib/Pass/Pass.cpp
689

const std::string&

747

comment seems outdated.

mehdi_amini accepted this revision.Jan 21 2021, 3:20 PM

LGTM with a minor tweak on FileReproducerStream

mlir/lib/Pass/Pass.cpp
702

I'd rather patch ToolOutputFile to get access to this (it is already stored there!):

diff --git a/llvm/include/llvm/Support/ToolOutputFile.h b/llvm/include/llvm/Support/ToolOutputFile.h
index cf01b9ecefc5..441aa5881fb8 100644
--- a/llvm/include/llvm/Support/ToolOutputFile.h
+++ b/llvm/include/llvm/Support/ToolOutputFile.h
@@ -34,7 +34,7 @@ class ToolOutputFile {
   public:
     /// The flag which indicates whether we should not delete the file.
     bool Keep;
-
+    StringRef GetFilename() { return Filename; }
     explicit CleanupInstaller(StringRef Filename);
     ~CleanupInstaller();
   } Installer;
@@ -57,6 +57,9 @@ public:
   /// Return the contained raw_fd_ostream.
   raw_fd_ostream &os() { return *OS; }
 
+  /// Return the filename it was initialized with.
+  StringRef GetFilename() { return Installer.GetFilename(); }
+
   /// Indicate that the tool's job wrt this output file has been successful and
   /// the file should not be deleted.
   void keep() { Installer.Keep = true; }
This revision is now accepted and ready to land.Jan 21 2021, 3:20 PM
rriddle accepted this revision.Jan 21 2021, 3:22 PM

LGTM with Mehdi's comments addressed.

Please update the documentation on the website detailing on how users can hook into this.

jpienaar updated this revision to Diff 318357.Jan 21 2021, 4:50 PM
jpienaar marked 2 inline comments as done.

Adding short usage information and updating ToolOutputFile to query name

Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 4:50 PM
jpienaar updated this revision to Diff 318370.Jan 21 2021, 5:57 PM

Update make site and config prefix

This revision was landed with ongoing or failed builds.Jan 21 2021, 8:14 PM
This revision was automatically updated to reflect the committed changes.