[TableGen] Make the NUL character invalid in .td files

[Paul-C-Anagnostopoulos]

Summary

This little revision makes the NUL character officially invalid in .td files.

Note that the TableGen lexer does not consistently use getNextChar() to obtain characters from the source files. So some NULs, such as those in // comments, are simply ignored.

[Paul-C-Anagnostopoulos]

Hmm. I swore I deleted all the //// lines. Hang on . . .

[Paul-C-Anagnostopoulos]

Now the //// comments should be gone.

[dblaikie]

llvm/lib/TableGen/TGLexer.cpp

421–425

So previously a null character would terminate a comment - and with this change it won't terminate the comment anymore? Maybe this would be better in a separate patch (& maybe with a test demonstrating this behavior)

[Paul-C-Anagnostopoulos]

llvm/lib/TableGen/TGLexer.cpp

421–425

It didn't terminate the comment, but rather was included in the comment. Just as the new code does.

Good idea to write a test for this. I will write one tomorrow.

[Paul-C-Anagnostopoulos]

I added a test. There are NULs in the .td file.

[dblaikie]

Hmm, guess there's no way to review the nul character test file? (I can't download the patch, can't view it in phab)

Maybe it'd make sense to separate the nul char file from the text file that has the RUN and CHECK lines? /maybe/ it could be a plain text file that gets modified to include a null character (eg: a RUN line that uses 'cat' to add a nul character at the start of the file?)?

[Paul-C-Anagnostopoulos]

There are four or five NULs in the test file, to check them in various positions. Would it be helpful to add a second nul-char.txt file that is a duplicate, except that it has an at sign (@) where every null is? I could refer to it with a comment in nul-char.td.

[dblaikie]

In D101923#2745551, @Paul-C-Anagnostopoulos wrote:

There are four or five NULs in the test file, to check them in various positions. Would it be helpful to add a second nul-char.txt file that is a duplicate, except that it has an at sign (@) where every null is? I could refer to it with a comment in nul-char.td.

Maybe? But could you look around at other tests, see if maybe there's a way to construct this test while having it still be a single text file? I'm wondering if any existing tests use the unix tr program or something to do specific replacements.

If there's no reasonable way to make this test work as a text file - maybe it'll be best to include the binary file and then a separate text file with the RUN/CHECK lines - and, yeah, maybe it'd be alright/good to include the text of the binary file with @ instead of null where the nulls would be as the text inside the RUN/CHECK file.

[Paul-C-Anagnostopoulos]

Okay, sounds good. I'll see what I can find.

[Paul-C-Anagnostopoulos]

There are test files that use sed to do replacements. I'll ask on llvm dev how to create a RUN: line that does the right thing. It has to replace @ with NUL and get rid of the sed command so it doesn't recurse forever. My Unix command knowledge is meager.

[dblaikie]

In D101923#2745590, @Paul-C-Anagnostopoulos wrote:

There are test files that use sed to do replacements. I'll ask on llvm dev how to create a RUN: line that does the right thing. It has to replace @ with NUL and get rid of the sed command so it doesn't recurse forever. My Unix command knowledge is meager.

Sorry, I missed the second step - why would it need to get rid of the sed command?

I'd expect something like:

; RUN: sed -e 's/@/NUL/g' %s > %t.input
; RUN: llvm-tblgen %t.input | FileCheck

Stuff @ more stuff

Oh, you mean the problem that the sed line itself would get replaced - then there'd be a null character in the RUN line? Maybe you can strip that off with head -n +1 or something like that?

[Paul-C-Anagnostopoulos]

No, I was confused, thinking that the modified file would go back through Lit. But of course it doesn't. I will set up something like you suggest. I'm sure there is a way to use an escape sequence to represent the NUL character in sed . . . yes, \000.

First I have to get sed for Windows.

[dblaikie]

In D101923#2745614, @Paul-C-Anagnostopoulos wrote:

No, I was confused, thinking that the modified file would go back through Lit. But of course it doesn't. I will set up something like you suggest. I'm sure there is a way to use an escape sequence to represent the NUL character in sed . . . yes, \000.

First I have to get sed for Windows.

How does the existing sed test work on your machine? Or is the test flagged as "REQUIRES: shell" and so is unsupported on your system? (in which case getting sed won't be enough to make the test run, perhaps?)

[Paul-C-Anagnostopoulos]

Hang on, there is already a TableGen test that uses sed: intrin-properties.td. So obviously I have sed in the right place.

I forgot to submit this!

[Paul-C-Anagnostopoulos]

I used David's trick to eliminate the NUL characters in the test file. They are now represented by at signs (@) and translated on the fly.

[dblaikie]

Looks good - please address the clang-format issues before submitting (there's a clang-format script that can automatically apply changes to only the things touched by your patch (from the root of the monorepo: ./clang/tools/clang-format/git-clang-format origin)

llvm/lib/TableGen/TGLexer.cpp

bool TGLexer::processEOF() {
   return false;
 }
 
 int TGLexer::getNextChar() {
   char CurChar = *CurPtr++;
   switch (CurChar) {
   default:
     return (unsigned char)CurChar;
-  case 0: {
-    // A nul character in the stream is either the end of the current buffer or
-    // a random nul in the file.  Disambiguate that here.
-    if (CurPtr-1 != CurBuf.end())
-      return 0;  // Just whitespace.
 
-    // Otherwise, return end of file.
-    --CurPtr;  // Another call to lex will return EOF again.
+  case 0: {
+    // A NUL character in the stream is either the end of the current buffer or
+    // a spurious NUL in the file.  Disambiguate that here.
+    if (CurPtr - 1 == CurBuf.end()) {
+      --CurPtr; // Arrange for another call to return EOF again.
       return EOF;
     }
+    PrintError(getLoc(),
+               "NUL character is invalid in source; treated as space");
+    return ' ';
+  }
+
   case '\n':
   case '\r':
     // Handle the newline character by ignoring it and incrementing the line
     // count.  However, be careful about 'dos style' files with \n\r in them.
     // Only treat a \n\r or \r\n as a single line.
     if ((*CurPtr == '\n' || (*CurPtr == '\r')) &&
         *CurPtr != CurChar)
       ++CurPtr;  // Eat the two char newline sequence.
if (peekNextChar(0) == '.') {
       return ReturnError(TokStart, "Invalid '..' punctuation");
     }
     return tgtok::dot;
 
   case '\r':
     PrintFatalError("getNextChar() must never return '\r'");
     return tgtok::Error;
 
-  case 0:
   case ' ':
   case '\t':
     // Ignore whitespace.
     return LexToken(FileOrLineStart);
   case '\n':
     // Ignore whitespace, and identify the new line.
     return LexToken(true);
   case '/':
bool TGLexer::LexInclude() {
   CurBuf = SrcMgr.getMemoryBuffer(CurBuffer)->getBuffer();
   CurPtr = CurBuf.begin();
 
   PrepIncludeStack.push_back(
       std::make_unique<std::vector<PreprocessorControlDesc>>());
   return false;
 }
 
+/// SkipBCPLComment - Skip over the comment by finding the next CR or LF.
+/// Or we may end up at the end of the buffer.
 void TGLexer::SkipBCPLComment() {
   ++CurPtr;  // skip the second slash.
-  while (true) {
-    switch (*CurPtr) {
-    case '\n':
-    case '\r':
-      return;  // Newline is end of comment.
-    case 0:
-      // If this is the end of the buffer, end the comment.
-      if (CurPtr == CurBuf.end())
-        return;
-      break;
-    }
-    // Otherwise, skip the character.
-    ++CurPtr;
-  }
+  auto EOLPos = CurBuf.find_first_of("\r\n", CurPtr - CurBuf.data());
+  CurPtr = (EOLPos == StringRef::npos) ? CurBuf.end() : CurBuf.data() + EOLPos;
 }
 
 /// SkipCComment - This skips C-style /**/ comments.  The only difference from C
 /// is that we allow nesting.
 bool TGLexer::SkipCComment() {
   ++CurPtr;  // skip the star.
   unsigned CommentDepth = 1;
 

llvm/test/TableGen/nul-char.td

+// RUN: sed -e 's/@/\x00/g' %s > %t
+// RUN: not llvm-tblgen -DERROR1 %t 2>&1 | FileCheck --check-prefix=ERROR1 %s
+
+// This test file checks that NUL is treated as an invalid character.
+// Each at sign is replaced with a NUL before running the test.
+
+#ifdef ERROR1
+
+// ERROR1: error: NUL character is invalid in source; treated as space
+// ERROR1: error: NUL character is invalid in source; treated as space
+// ERROR1: error: NUL character is invalid in source; treated as space
+// ERROR1: error: NUL character is invalid in source; treated as space
+// ERROR1: error: expected ';' after declaration
+
+def Foo@ {
+  int @ ID = 42;
+}
+
+@
+
+// Comment with a NUL @ there. They are ignored in comments.
+
+def Bar {
+  int Biggie = 12345@789;
+}
+
+#endif
+