[AsmParser][SystemZ][z/OS] Add in support to allow use of additional comment strings.

[anirudhp]

Summary

• Currently, MCAsmInfo provides a CommentString attribute, that various targets can set, so that the AsmLexer can appropriately lex a string as a comment based on the set value of the attribute.
• However, AsmLexer also supports a few additional comment syntaxes, in addition to what's specified as a CommentString attribute. This includes regular C-style block comments (/* ... */), regular C-style line comments (// .... ) and #. While I'm not sure as to why this behaviour exists, I am assuming it does to maintain backward compatibility with GNU AS (see https://sourceware.org/binutils/docs/as/Comments.html#Comments for reference)

For example:
Consider a target which sets the CommentString attribute to '*'.
The following strings are all lexed as comments.

"# abc" -> comment
"// abc" -> comment
"/* abc */ -> comment
"* abc" -> comment

• In HLASM however, only "*" is accepted as a comment string, and nothing else.
• To achieve this, an additional attribute (AllowAdditionalComments) has been added to MCAsmInfo. If this attribute is set to false, then only the string specified by the CommentString attribute is used as a possible comment string to be lexed by the AsmLexer. The regular C-style block comments, line comments and "#" are disabled. As a final note, "#" will still be treated as a comment, if the CommentString attribute is set to "#".

Depends on https://reviews.llvm.org/D99277

[anirudhp]

• Few formatting changes + making the unittest a bit more robust in the tokens it was checking

[anirudhp]

Ping :)

[anirudhp]

• Rebasing on latest master.

[MaskRay]

If the C and BCPL style comments don't cause conflict with HLASM, I'd rather we don't add any code. We don't necessarily reject syntax which are unlikely used incorrectly by the user.
If such distinction is really needed, maybe name the variable with a positive meaning, e.g. AllowCCommentMarker, instead of Only* (Disallow*).

[anirudhp]

In D99286#2667966, @MaskRay wrote:

If the C and BCPL style comments don't cause conflict with HLASM, I'd rather we don't add any code. We don't necessarily reject syntax which are unlikely used incorrectly by the user.

They do cause "conflict". HLASM currently does not treat the additional comment strings as "comments".

If such distinction is really needed, maybe name the variable with a positive meaning, e.g. AllowCCommentMarker, instead of Only* (Disallow*).

How about a new field called AllowAdditionalComments instead? This will encapsulate allowing the C-style line and block comments, and the "#" string as additional comment strings in addition to the CommentString attribute.

[MaskRay]

In D99286#2668227, @anirudhp wrote:

In D99286#2667966, @MaskRay wrote:

If the C and BCPL style comments don't cause conflict with HLASM, I'd rather we don't add any code. We don't necessarily reject syntax which are unlikely used incorrectly by the user.

They do cause "conflict". HLASM currently does not treat the additional comment strings as "comments".

This does not necessarily mean a conflict. As an concrete example, ld.lld treats # as an additional comment marker for linker scripts. GNU ld don't do this.
The ld.lld way can be seen as an extension. Since there is no meaningful syntax starting with # in GNU ld linker scripts, reserving # for an additional comment marker is totally fine.

My question here is similar. If there is no meaningful syntax starting with /* or //, we don't necessarily detect such cases.

If such distinction is really needed, maybe name the variable with a positive meaning, e.g. AllowCCommentMarker, instead of Only* (Disallow*).

How about a new field called AllowAdditionalComments instead? This will encapsulate allowing the C-style line and block comments, and the "#" string as additional comment strings in addition to the CommentString attribute.

Looks good.

Note that # is special - some dialects don't use it for comment markers at an arbitrary column but allows it at the line beginning.
If HLASM does not use # at line beginning as useful syntax, we don't necessarily detect it.

[anirudhp]

This does not necessarily mean a conflict. As an concrete example, ld.lld treats # as an additional comment marker for linker scripts. GNU ld don't do this.
The ld.lld way can be seen as an extension. Since there is no meaningful syntax starting with # in GNU ld linker scripts, reserving # for an additional comment marker is totally fine.
My question here is similar. If there is no meaningful syntax starting with /* or //, we don't necessarily detect such cases.

Ahh I see. I think I understand what you're saying. With respect to the extent of the support we are providing for HLASM (only for 1st param of inline asm block only concerned with Z machine instructions), I don't believe there would be any meaningful syntax starting with // or /* .. */, or # (there shouldn't be a reason for a user to use it). However, we also want to ensure that for the z/OS target, for the HLASM syntax support we are providing, we don't leak any accepted gnu syntax.

i.e. Let's say as a usecase, a user for z/OS using the HLASM syntax, does use the // .... , /* ... */ , # ..... in an inline asm statement (even though it might be not anything meaningful).

Under the current behaviour, llvm's assembler/asm lexer will lex it as a line comment. And since we want to reject any accepted gnu syntax, we want to ensure that those "additional comment" syntaxes are not accepted, i.e. they are lexed as regular tokens (non line comments in this case).

[anirudhp]

• Addressed Comments
• Renamed MCAsmInfo attribute with a more positive meaning
• Updated tests and AsmLexer with new attribute name.

[abhina.sreeskantharajan]

Changes LGTM. You will need to update the title and description to reflect the new approach you took to allow additional comment strings, rather than disable them.

[anirudhp]

@MaskRay Would you happen to have any other review comments on this patch that I can address?

[myiwanch]

The changes to MCAsmInfo are pretty small, so it shouldn't affect others too much.
Looks good

[anirudhp]

• Updating repo to rg Github monorepo (no functional changes)

[anirudhp]

• Rebase on latest master

llvm/include/llvm/MC/MCAsmInfo.h

protected:
   /// This indicates the comment string used by the assembler.  Defaults to
   /// "#"
   StringRef CommentString;
 
   /// This indicates whether the comment string is only accepted as a comment
   /// at the beginning of statements. Defaults to false.
   bool RestrictCommentStringToStartOfStatement = false;
 
+  /// This indicates whether to only treat the string specified by the
+  /// CommentString attribute as a possible comment in AsmLexer.
+  /// Currently, regular C-style block level comments and line level
+  /// comments are accepted.
+  /// Furthermore, if a string starts with "#", it is treated as a possible
+  /// comment, even if the CommentString attribute is specified to be something
+  /// other than "#".
+  bool OnlyUseCommentStringForComment = false;
+
   /// This is appended to emitted labels.  Defaults to ":"
   const char *LabelSuffix;
 
   // Print the EH begin symbol with an assignment. Defaults to false.
   bool UseAssignmentForEHBegin = false;
 
   // Do we need to create a local symbol for .size?
   bool NeedsLocalForSize = false;
public:
   /// This indicates the column (zero-based) at which asm comments should be
   /// printed.
   unsigned getCommentColumn() const { return 40; }
 
   StringRef getCommentString() const { return CommentString; }
   bool getRestrictCommentStringToStartOfStatement() const {
     return RestrictCommentStringToStartOfStatement;
   }
+  bool shouldOnlyUseCommentStringForComment() const {
+    return OnlyUseCommentStringForComment;
+  }
   const char *getLabelSuffix() const { return LabelSuffix; }
 
   bool useAssignmentForEHBegin() const { return UseAssignmentForEHBegin; }
   bool needsLocalForSize() const { return NeedsLocalForSize; }
   StringRef getPrivateGlobalPrefix() const { return PrivateGlobalPrefix; }
   StringRef getPrivateLabelPrefix() const { return PrivateLabelPrefix; }
 
   bool hasLinkerPrivateGlobalPrefix() const {

llvm/lib/MC/MCParser/AsmLexer.cpp

if (CurPtr == TokStart+1 && TokStart[0] == '.')
     return AsmToken(AsmToken::Dot, StringRef(TokStart, 1));
 
   return AsmToken(AsmToken::Identifier, StringRef(TokStart, CurPtr - TokStart));
 }
 
 /// LexSlash: Slash: /
 ///           C-Style Comment: /* ... */
 AsmToken AsmLexer::LexSlash() {
+  if (MAI.shouldOnlyUseCommentStringForComment()) {
+    IsAtStartOfStatement = false;
+    return AsmToken(AsmToken::Slash, StringRef(TokStart, 1));
+  }
+
   switch (*CurPtr) {
   case '*':
     IsAtStartOfStatement = false;
     break; // C style comment.
   case '/':
     ++CurPtr;
     return LexLineComment();
   default:
if (!IsPeeking && CurChar == '#' && IsAtStartOfStatement) {
     if (IsAtStartOfLine && num == 2 && TokenBuf[0].is(AsmToken::Integer) &&
         TokenBuf[1].is(AsmToken::String)) {
       CurPtr = TokStart; // reset curPtr;
       StringRef s = LexUntilEndOfLine();
       UnLex(TokenBuf[1]);
       UnLex(TokenBuf[0]);
       return AsmToken(AsmToken::HashDirective, s);
     }
+
+    if (!MAI.shouldOnlyUseCommentStringForComment())
       return LexLineComment();
   }
 
   if (isAtStartOfComment(TokStart))
     return LexLineComment();
 
   if (isAtStatementSeparator(TokStart)) {
     CurPtr += strlen(MAI.getSeparatorString()) - 1;
     IsAtStartOfLine = true;

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmInfo.cpp

SystemZMCAsmInfo::SystemZMCAsmInfo(const Triple &TT) {
   IsLittleEndian = false;
 
   AssemblerDialect = TT.isOSzOS() ? AD_HLASM : AD_ATT;
 
   MaxInstLength = 6;
 
   CommentString = AssemblerDialect == AD_HLASM ? "*" : "#";
   RestrictCommentStringToStartOfStatement = (AssemblerDialect == AD_HLASM);
+  OnlyUseCommentStringForComment = (AssemblerDialect == AD_HLASM);
   ZeroDirective = "\t.space\t";
   Data64bitsDirective = "\t.quad\t";
   UsesELFSectionDirectiveForBSS = true;
   SupportsDebugInformation = true;
   ExceptionsType = ExceptionHandling::DwarfCFI;
 }

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

 // Here, we're free to override whatever we want, without polluting
 // the main MCAsmInfo interface.
 class MockedUpMCAsmInfo : public MCAsmInfo {
 public:
   void setRestrictCommentStringToStartOfStatement(bool Value) {
     RestrictCommentStringToStartOfStatement = Value;
   }
   void setCommentString(StringRef Value) { CommentString = Value; }
+  void setOnlyUseCommentStringForComment(bool Value) {
+    OnlyUseCommentStringForComment = Value;
+  }
 };
 
 // Setup a testing class that the GTest framework can call.
 class SystemZAsmLexerTest : public ::testing::Test {
 protected:
   static void SetUpTestCase() {
     LLVMInitializeSystemZTargetInfo();
     LLVMInitializeSystemZTargetMC();
TEST_F(SystemZAsmLexerTest, CheckAllowHashInIdentifier2) {
   // "lh#12" -> Identifier
   // "*" -> Star
   // "3" -> Integer
   SmallVector<AsmToken::TokenKind> ExpectedTokens(
       {AsmToken::Identifier, AsmToken::Star, AsmToken::Integer,
        AsmToken::EndOfStatement, AsmToken::Eof});
   lexAndCheckTokens(AsmStr, ExpectedTokens);
 }
+
+TEST_F(SystemZAsmLexerTest, DontCheckStrictCommentString) {
+  StringRef AsmStr = "# abc\n/* def *///  xyz";
+
+  // Setup.
+  setupCallToAsmParser(AsmStr);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::EndOfStatement, AsmToken::Comment, AsmToken::EndOfStatement,
+       AsmToken::Eof});
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
+
+TEST_F(SystemZAsmLexerTest, DontCheckStrictCommentString2) {
+  StringRef AsmStr = "# abc\n/* def *///  xyz\n* rst";
+
+  // Setup.
+  MUPMAI->setCommentString("*");
+  setupCallToAsmParser(AsmStr);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::EndOfStatement, AsmToken::Comment, AsmToken::EndOfStatement,
+       AsmToken::EndOfStatement, AsmToken::Eof});
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
+
+TEST_F(SystemZAsmLexerTest, CheckStrictCommentString) {
+  StringRef AsmStr = "# abc\n/* def *///  xyz";
+
+  // Setup.
+  MUPMAI->setOnlyUseCommentStringForComment(true);
+  setupCallToAsmParser(AsmStr);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  // "# abc" -> still treated as a comment, since CommentString
+  //            is set to "#"
+  SmallVector<AsmToken::TokenKind> ExpectedTokens;
+  ExpectedTokens.push_back(AsmToken::EndOfStatement); // "# abc\n"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::Star);           // "*"
+  ExpectedTokens.push_back(AsmToken::Identifier);     // "def"
+  ExpectedTokens.push_back(AsmToken::Star);           // "*"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::Identifier);     // "xyz"
+  ExpectedTokens.push_back(AsmToken::EndOfStatement);
+  ExpectedTokens.push_back(AsmToken::Eof);
+
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
+
+TEST_F(SystemZAsmLexerTest, CheckStrictCommentString2) {
+  StringRef AsmStr = "// abc";
+
+  // Setup.
+  MUPMAI->setOnlyUseCommentStringForComment(true);
+  MUPMAI->setCommentString("//");
+  setupCallToAsmParser(AsmStr);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  // "// abc" -> will still be treated as a comment because "//" is the
+  //             CommentString
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::EndOfStatement, AsmToken::Eof});
+  lexAndCheckTokens(AsmStr /* "// abc" */, ExpectedTokens);
+}
+
+TEST_F(SystemZAsmLexerTest, CheckStrictCommentString3) {
+  StringRef AsmStr = "/* abc */";
+
+  // Setup.
+  MUPMAI->setOnlyUseCommentStringForComment(true);
+  setupCallToAsmParser(AsmStr);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  SmallVector<AsmToken::TokenKind> ExpectedTokens;
+  ExpectedTokens.push_back(AsmToken::Slash);
+  ExpectedTokens.push_back(AsmToken::Star);
+  ExpectedTokens.push_back(AsmToken::Identifier);
+  ExpectedTokens.push_back(AsmToken::Star);
+  ExpectedTokens.push_back(AsmToken::Slash);
+  ExpectedTokens.push_back(AsmToken::EndOfStatement);
+  ExpectedTokens.push_back(AsmToken::Eof);
+
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
+
+TEST_F(SystemZAsmLexerTest, CheckStrictCommentString4) {
+  StringRef AsmStr = "# abc\n/* def *///  xyz";
+
+  // Setup.
+  MUPMAI->setCommentString("*");
+  MUPMAI->setOnlyUseCommentStringForComment(true);
+  MUPMAI->setRestrictCommentStringToStartOfStatement(true);
+  setupCallToAsmParser(AsmStr);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  SmallVector<AsmToken::TokenKind> ExpectedTokens;
+  ExpectedTokens.push_back(AsmToken::Hash);           // "#"
+  ExpectedTokens.push_back(AsmToken::Identifier);     // "abc"
+  ExpectedTokens.push_back(AsmToken::EndOfStatement); // "\n"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::Star);           // "*"
+  ExpectedTokens.push_back(AsmToken::Identifier);     // "def"
+  ExpectedTokens.push_back(AsmToken::Star);           // "*"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::Identifier);     // "xyz"
+  ExpectedTokens.push_back(AsmToken::EndOfStatement);
+  ExpectedTokens.push_back(AsmToken::Eof);
+
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
+
+TEST_F(SystemZAsmLexerTest, CheckStrictCommentString5) {
+  StringRef AsmStr = "#abc\n/* def */// xyz";
+
+  // Setup.
+  MUPMAI->setCommentString("*");
+  MUPMAI->setOnlyUseCommentStringForComment(true);
+  setupCallToAsmParser(AsmStr);
+
+  // Lex initially to get the string.
+  Parser->getLexer().Lex();
+
+  SmallVector<AsmToken::TokenKind> ExpectedTokens;
+  ExpectedTokens.push_back(AsmToken::Hash);           // "#"
+  ExpectedTokens.push_back(AsmToken::Identifier);     // "abc"
+  ExpectedTokens.push_back(AsmToken::EndOfStatement); // "\n"
+  ExpectedTokens.push_back(AsmToken::Slash);          // "/"
+  ExpectedTokens.push_back(AsmToken::EndOfStatement); // "* def */// xyz"
+  ExpectedTokens.push_back(AsmToken::Eof);
+
+  lexAndCheckTokens(AsmStr, ExpectedTokens);
+}
 } // end anonymous namespace