Fix SelectionDAG Graph Printing on Windows

[justice_adams]

Summary

Currently, when compiling to IR (presumably at the clang level) LLVM mangles symbols and sometimes they have illegal file characters including ?'s in them. This causes a problem when building a graph via llc on Windows because the code currently passes the machine function name all the way down to the Windows API which frequently returns error 123 ERROR_INVALID_NAME
https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes--0-499-

Thus, we need to remove those illegal characters from the machine function name before generating a graph, which is the purpose of this patch.
https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file

I've created a static helper function replace_illegal_filename_chars which within GraphWriter.cpp to help with replacing illegal file character names before generating a dot graph filename.

[justice_adams]

I've attached a reproducible.zip folder. To reproduce this bug, unzip the folder and compile multiply.cpp to LLVM IR and then run

llc -view-dag-combine1-dags multiply.ll

reproducible.zip1 KBDownload

I've also included the .ll file produced from my local environment, titled multiply.ll

[justice_adams]

• removed static declaration from header file
• cleaned up code according to clang-tidy specifications

[justice_adams]

Ping requesting review

[rnk]

Sorry about the delay, I find this whole feature kind of concerning. Are you telling me that LLVM lets you write a file with an arbitrary filename, if not path?

So, if I want clang to overwrite some file, all I have to do is pass clang -mllvm -view-dag-combine1-dags and I can create a file with a name from the input source? That seems dangerous.

Can you please move this code from the GraphWriter.h header to here?

// Windows can't always handle long paths, so limit the length of the name.
std::string N = Name.str();
N = N.substr(0, std::min<std::size_t>(N.size(), 140));
if (Filename.empty()) {
  Filename = createGraphFilename(N, FD);

N is only used if we call createGraphFilename anyway, and it feels related.

[rnk]

(Despite the tone of incredulity in my last comment, thanks for the fix, functionally this seems like an improvement.)

[justice_adams]

@rnk

So, if I want clang to overwrite some file, all I have to do is pass clang -mllvm -view-dag-combine1-dags and I can create a file with a name from the input source? That seems dangerous.

Currently, the dag printer will create a file with a name corresponding to the machine-function from the IR that it's representing with the graph, not the source file name. Forgive me if I misinterpreted this comment.

Can you please move this code from the GraphWriter.h header to here?

Yes that seems to make sense. Does it make sense to move that into a separate patch?

[rnk]

In D76863#1985144, @justice_adams wrote:

@rnk

So, if I want clang to overwrite some file, all I have to do is pass clang -mllvm -view-dag-combine1-dags and I can create a file with a name from the input source? That seems dangerous.

Currently, the dag printer will create a file with a name corresponding to the machine-function from the IR that it's representing with the graph, not the source file name. Forgive me if I misinterpreted this comment.

Yes, that was my interpretation. Functions can be named all kinds of evil things. It's pretty common for compiler developers to feed test cases from bug reports to the compiler, and this seems like an easy way for an attacker to write a temp file with a name of their choice. But, that is the existing functionality. I see that path separators are banned, so it's not an arbitrary file, it's a particular file in some temp directory.

Can you please move this code from the GraphWriter.h header to here?

Yes that seems to make sense. Does it make sense to move that into a separate patch?

I feel like it should be part of this patch.

[justice_adams]

• moved handling of long paths to createGraphFilename()

[justice_adams]

@rnk Sorry for the delay, I moved the file path length handling accordingly

[amccarth]

Apologies for the drive-by questions.

llvm/lib/Support/GraphWriter.cpp

100

If sys::fs::createTemporaryFile can return a too-long path here, then it could do so for anyone else, too. Should the length limit be handled there?

Is 140 arbitrary, a guestimate, or based on some actual limit?

[justice_adams]

llvm/lib/Support/GraphWriter.cpp

100

@amccarth It appears to be arbitrary https://reviews.llvm.org/D3883

I'm open to moving the length limiting to

sys::fs::createTemporaryFile

If that's preferred

[rnk]

Thanks, I think this looks good.

Do you need someone to push this? If so, let me know, I'll try to get to it.

llvm/lib/Support/GraphWriter.cpp

87–88

This can be a range-based for.

100

I don't think that's necessary. I think most callers of createTemporaryFile supply a fixed prefix. The graph writer code here is special in that it is taking non-path-like user input and using that to form a path.

[justice_adams]

• Converted to range-based for loop

[justice_adams]

@rnk Thanks for the suggestions, I've updated it. If it looks good on your end, yes, I would appreciate it if you could commit this for me, I don't have commit access.

[justice_adams]

• Fix formatting

llvm/include/llvm/Support/GraphWriter.h

 /// \return The resulting filename, or an empty string if writing
 /// failed.
 template <typename GraphType>
 std::string WriteGraph(const GraphType &G, const Twine &Name,
                        bool ShortNames = false,
                        const Twine &Title = "",
                        std::string Filename = "") {
   int FD;
-  // Windows can't always handle long paths, so limit the length of the name.
-  std::string N = Name.str();
-  N = N.substr(0, std::min<std::size_t>(N.size(), 140));
   if (Filename.empty()) {
-    Filename = createGraphFilename(N, FD);
+    Filename = createGraphFilename(Name.str(), FD);
   } else {
     std::error_code EC = sys::fs::openFileForWrite(Filename, FD);
 
     // Writing over an existing file is not considered an error.
     if (EC == std::errc::file_exists) {
       errs() << "file exists, overwriting" << "\n";
     } else if (EC) {
       errs() << "error writing into file" << "\n";

llvm/lib/Support/GraphWriter.cpp

StringRef llvm::DOT::getColorString(unsigned ColorNumber) {
   static const int NumColors = 20;
   static const char* Colors[NumColors] = {
     "aaaaaa", "aa0000", "00aa00", "aa5500", "0055ff", "aa00aa", "00aaaa",
     "555555", "ff5555", "55ff55", "ffff55", "5555ff", "ff55ff", "55ffff",
     "ffaaaa", "aaffaa", "ffffaa", "aaaaff", "ffaaff", "aaffff"};
   return Colors[ColorNumber % NumColors];
 }
 
+static std::string replaceIllegalFilenameChars(std::string Filename,
+                                               const char ReplacementChar) {
+#ifdef _WIN32
+  std::string IllegalChars = "\\/:?\"<>|";
+#else
+  std::string IllegalChars = "/";
+#endif
+
+  for (char IllegalChar : IllegalChars) {
+    std::replace(Filename.begin(), Filename.end(), IllegalChar, ReplacementChar);
+  }
+
+  return Filename;
+}
+
 std::string llvm::createGraphFilename(const Twine &Name, int &FD) {
   FD = -1;
   SmallString<128> Filename;
-  std::error_code EC = sys::fs::createTemporaryFile(Name, "dot", FD, Filename);
+
+  // Windows can't always handle long paths, so limit the length of the name.
+  std::string N = Name.str();
+  N = N.substr(0, std::min<std::size_t>(N.size(), 140));
+
+  // Replace illegal characters in graph Filename with '_' if needed
+  std::string CleansedName = replaceIllegalFilenameChars(N, '_');
+
+  std::error_code EC =
+      sys::fs::createTemporaryFile(CleansedName, "dot", FD, Filename);
   if (EC) {
     errs() << "Error: " << EC.message() << "\n";
     return "";
   }
 
   errs() << "Writing '" << Filename << "'... ";
   return std::string(Filename.str());
 }