MC: AsmLexer: handle multi-character CommentStrings correctly

[jannau]

Summary

As X86MCAsmInfoDarwin uses '##' as CommentString although a single '#' starts a comment a workaround for this special case is added.

Fixes divisions in constant expressions for the AArch64 assembler and other targets which use '//' as CommentString.

[compnerd]

lib/MC/MCParser/AsmLexer.cpp

465

Im not sure I like this approach. Having the special comment string being taken care of by looking at both the user supplied input and the internal pointer makes me uneasy. If we want to use the internal cursor to check the current position, then drop the argument and make check purely internal.

[jannau]

lib/MC/MCParser/AsmLexer.cpp

465

yes, I missed that it's a public function. It shouldn't use internal state.

Changing it to or adding a

bool AsmLexer::isAtStartOfComment(const char *Ptr)

seems the best option although we still have to handle the X86MCAsmInfo.cpp special case.

[jannau]

adds isAtStartOfComment(const char *Ptr) and uses it to handle multi-character CommentStrings.

[jannau]

ping

[compnerd]

The tests feel a bit insufficient for such a change. You have a special case for Darwin yet no test for it? Is there a set of existing test cases that cover the various other cases of string comment indicators?

lib/MC/MCParser/AsmLexer.cpp

460–461

Isn't this not entirely true? If you have '//' as a comment character, and you are at '/', its unclear if this is a comment starter or not. I think that we should just drop this interface and take a const char * instead.

476

I think that special casing the 1 character into the fast path makes sense here:

if (CommentString[1] == '\0')
  return *Ptr == CommentString[0];

477

Why not fix this properly rather than work around it?

[jannau]

I've removed the old isAtStartOfComment(const char) and optimized the replacement for the single char comment string case.

The workaround for "##" in X86MCAsmInfoDarwin is still there since I'm unsure how I should solve that properly. The least bad solution I found is using different CommentStrings for output and parsing. I don't feel comfortable enough to decide how to solve this.

[compnerd]

The MCParser portion of the change looks much better now. I am still a bit concerned about the testing. Can you at least add a test case for the special case of Darwin?

lib/MC/MCParser/AsmLexer.cpp

465

You have an extra space here.

468

This comment feels like it may get out of sync. There is nothing preventing a backend from claiming that @# is a comment identifier (other than sanity and the fear of a serious beating from a mob of angry developers).

[jannau]

added explicit tests with single '#' comments for x86 darwin. A couple of existing tests will catch incorrect parsing of single '#' comments too though.

[compnerd]

lib/MC/MCParser/AsmLexer.cpp

465

There is an extra space here.

[jannau]

whitespace fixes

[jannau]

Committed in r215615

include/llvm/MC/MCParser/AsmLexer.h

public:
 
   void setBuffer(StringRef Buf, const char *ptr = nullptr);
 
   StringRef LexUntilEndOfStatement() override;
   StringRef LexUntilEndOfLine();
 
   const AsmToken peekTok(bool ShouldSkipSpace = true) override;
 
-  bool isAtStartOfComment(char Char);
+  bool isAtStartOfComment(const char *Ptr);
   bool isAtStatementSeparator(const char *Ptr);
 
   const MCAsmInfo &getMAI() const { return MAI; }
 
 private:
   int getNextChar();
   AsmToken ReturnError(const char *Loc, const std::string &Msg);
 

lib/MC/MCParser/AsmLexer.cpp

AsmToken AsmLexer::LexQuote() {
   }
 
   return AsmToken(AsmToken::String, StringRef(TokStart, CurPtr - TokStart));
 }
 
 StringRef AsmLexer::LexUntilEndOfStatement() {
   TokStart = CurPtr;
 
-  while (!isAtStartOfComment(*CurPtr) &&    // Start of line comment.
+  while (!isAtStartOfComment(CurPtr) &&     // Start of line comment.
          !isAtStatementSeparator(CurPtr) && // End of statement marker.
          *CurPtr != '\n' && *CurPtr != '\r' &&
          (*CurPtr != 0 || CurPtr != CurBuf.end())) {
     ++CurPtr;
   }
   return StringRef(TokStart, CurPtr-TokStart);
 }
 
const AsmToken AsmLexer::peekTok(bool ShouldSkipSpace) {
 
   SkipSpace = SavedSkipSpace;
   isAtStartOfLine = SavedAtStartOfLine;
   CurPtr = SavedCurPtr;
   TokStart = SavedTokStart;
 
   return Token;
 }
 
-bool AsmLexer::isAtStartOfComment(char Char) {
-  // FIXME: This won't work for multi-character comment indicators like "//".
-  return Char == *MAI.getCommentString();
+bool AsmLexer::isAtStartOfComment(const char *Ptr) {
+  const char *CommentString = MAI.getCommentString();
+
+  if (CommentString[1] == '\0')
+    return CommentString[0] == Ptr[0];
+
+  // FIXME: special case for the bogus "##" comment string in X86MCAsmInfoDarwin
+  if (CommentString[1] == '#')
+    return CommentString[0] == Ptr[0];
+
+  return strncmp(Ptr, CommentString, strlen(CommentString)) == 0;
 }
 
 bool AsmLexer::isAtStatementSeparator(const char *Ptr) {
   return strncmp(Ptr, MAI.getSeparatorString(),
                  strlen(MAI.getSeparatorString())) == 0;
 }
 
 AsmToken AsmLexer::LexToken() {
   TokStart = CurPtr;
   // This always consumes at least one character.
   int CurChar = getNextChar();
 
-  if (isAtStartOfComment(CurChar)) {
+  if (isAtStartOfComment(TokStart)) {
     // If this comment starts with a '#', then return the Hash token and let
     // the assembler parser see if it can be parsed as a cpp line filename
     // comment. We do this only if we are at the start of a line.
     if (CurChar == '#' && isAtStartOfLine)
       return AsmToken(AsmToken::Hash, StringRef(TokStart, 1));
     isAtStartOfLine = true;
     return LexLineComment();
   }

test/MC/AArch64/single-slash.s

+// RUN: llvm-mc -triple aarch64-none-linux-gnu < %s | FileCheck %s
+
+// Test that a single slash is not mistaken as the start of comment.
+
+//CHECK: movz    x0, #0x10
+    movz x0, #(32 / 2)

test/MC/AsmParser/comments-x86-darwin.s

+// RUN: llvm-mc -triple x86_64-apple-darwin %s 2>&1 | FileCheck %s
+# ensure that single '#' comments are worink as expected on x86 darwin
+.align 3            # test single hash after align
+// CHECK: .align 3
+foo:                # single hash should be ignored as comment
+// CHECK-LABEL: foo:
+    movl %esp, %ebp # same after an instruction
+// CHECK: movl %esp, %ebp
+#   movl %esp, %ebp ## start of the line
+// CHECK-NOT: movl %esp, %ebp
+    # movl %esp, %ebp ## not quite start of the line
+// CHECK-NOT: movl %esp, %ebp
+bar:
+// CHECK-LABEL: bar: