[AsmParser][SystemZ][z/OS] Re-introduce HLASM comment syntax

[anirudhp]

Summary

• https://reviews.llvm.org/rGb605cfb336989705f391d255b7628062d3dfe9c3 was reverted due to sanitizer bugs in the introduced unit-test (specifically in the Address sanitizer https://lab.llvm.org/buildbot/#/builders/5/builds/5697)
• This patch attempts to rectify that, as well as re-factor parts of the test
• The issue was previously, within the setupCallToAsmParser function in the unit-test, SrcMgr was declared as a local variable. SrcMgr owns a unique pointer. Since the variable goes out of scope at the end of the function, the unique pointer is released.
• This patch, moves the declaration of the SrcMgr variable to a class field, since the scope will remain until the class's destructor is invoked (which in this case is at the end of the unit test)
• Furthermore, this patch also moves the MCContext Ctx declaration from a local variable instance inside a function, to a unique pointer class field. This ensures the instantiation of the MCContext remains until the tear down of the test.

[abhina.sreeskantharajan]

The new changes LGTM

llvm/include/llvm/MC/MCAsmInfo.h

protected:
   /// The '$' token, when not referencing an identifier or constant, refers to
   /// the current PC.  Defaults to false.
   bool DollarIsPC = false;
 
   /// This string, if specified, is used to separate instructions from each
   /// other when on the same line.  Defaults to ';'
   const char *SeparatorString;
 
-  /// This indicates the comment character used by the assembler.  Defaults to
+  /// 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 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:
   bool getDollarIsPC() const { return DollarIsPC; }
   const char *getSeparatorString() const { return SeparatorString; }
 
   /// 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;
+  }
   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 (Token.is(AsmToken::Eof))
       break;
   }
 
   SetError(SavedErrLoc, SavedErr);
   return ReadCount;
 }
 
 bool AsmLexer::isAtStartOfComment(const char *Ptr) {
+  if (MAI.getRestrictCommentStringToStartOfStatement() && !IsAtStartOfStatement)
+    return false;
+
   StringRef CommentString = MAI.getCommentString();
 
   if (CommentString.size() == 1)
     return CommentString[0] == Ptr[0];
 
   // Allow # preprocessor commments also be counted as comments for "##" cases
   if (CommentString[1] == '#')
     return CommentString[0] == Ptr[0];

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

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

llvm/unittests/MC/SystemZ/CMakeLists.txt

+include_directories(
+  ${LLVM_MAIN_SRC_DIR}/lib/Target/SystemZ
+  )
+
+set(LLVM_LINK_COMPONENTS
+  SystemZ
+  MCParser
+  MC
+  Support
+  )
+
+add_llvm_unittest(SystemZAsmLexerTests
+  SystemZAsmLexerTest.cpp
+  )

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

+//===- llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp ----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--------------------------------------------------------------------===//
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCObjectFileInfo.h"
+#include "llvm/MC/MCParser/MCTargetAsmParser.h"
+#include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCStreamer.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+// Come up with our hacked version of MCAsmInfo.
+// This hacked version derives from the main MCAsmInfo instance.
+// 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; }
+};
+
+// Setup a testing class that the GTest framework can call.
+class SystemZAsmLexerTest : public ::testing::Test {
+protected:
+  static void SetUpTestCase() {
+    LLVMInitializeSystemZTargetInfo();
+    LLVMInitializeSystemZTargetMC();
+  }
+
+  std::unique_ptr<MCRegisterInfo> MRI;
+  std::unique_ptr<MockedUpMCAsmInfo> MUPMAI;
+  std::unique_ptr<const MCInstrInfo> MII;
+  std::unique_ptr<MCStreamer> Str;
+  std::unique_ptr<MCAsmParser> Parser;
+  std::unique_ptr<MCContext> Ctx;
+
+  SourceMgr SrcMgr;
+  std::string TripleName;
+  llvm::Triple Triple;
+  const Target *TheTarget;
+
+  const MCTargetOptions MCOptions;
+  MCObjectFileInfo MOFI;
+
+  SystemZAsmLexerTest() {
+    // We will use the SystemZ triple, because of missing
+    // Object File and Streamer support for the z/OS target.
+    TripleName = "s390x-ibm-linux";
+    Triple = llvm::Triple(TripleName);
+
+    std::string Error;
+    TheTarget = TargetRegistry::lookupTarget(TripleName, Error);
+    EXPECT_NE(TheTarget, nullptr);
+
+    MRI.reset(TheTarget->createMCRegInfo(TripleName));
+    EXPECT_NE(MRI, nullptr);
+
+    std::unique_ptr<MCAsmInfo> MAI;
+    MAI.reset(TheTarget->createMCAsmInfo(*MRI, TripleName, MCOptions));
+    EXPECT_NE(MAI, nullptr);
+
+    // Now we cast to our mocked up version of MCAsmInfo.
+    MUPMAI.reset(static_cast<MockedUpMCAsmInfo *>(MAI.release()));
+    // MUPMAI should "hold" MAI.
+    EXPECT_NE(MUPMAI, nullptr);
+    // After releasing, MAI should now be null.
+    EXPECT_EQ(MAI, nullptr);
+  }
+
+  void setupCallToAsmParser(StringRef AsmStr) {
+    std::unique_ptr<MemoryBuffer> Buffer(MemoryBuffer::getMemBuffer(AsmStr));
+    SrcMgr.AddNewSourceBuffer(std::move(Buffer), SMLoc());
+    EXPECT_EQ(Buffer, nullptr);
+
+    Ctx.reset(
+        new MCContext(MUPMAI.get(), MRI.get(), &MOFI, &SrcMgr, &MCOptions));
+    MOFI.InitMCObjectFileInfo(Triple, false, *Ctx, false);
+
+    Str.reset(TheTarget->createNullStreamer(*Ctx));
+
+    Parser.reset(createMCAsmParser(SrcMgr, *Ctx, *Str, *MUPMAI));
+    // Lex initially to get the string.
+    Parser->getLexer().Lex();
+  }
+
+  void lexAndCheckTokens(StringRef AsmStr,
+                         SmallVector<AsmToken::TokenKind> ExpectedTokens) {
+    // Get reference to AsmLexer.
+    MCAsmLexer &Lexer = Parser->getLexer();
+    // Loop through all expected tokens checking one by one.
+    for (size_t I = 0; I < ExpectedTokens.size(); ++I) {
+      EXPECT_EQ(Lexer.getTok().getKind(), ExpectedTokens[I]);
+      Lexer.Lex();
+    }
+  }
+};
+
+TEST_F(SystemZAsmLexerTest, CheckDontRestrictCommentStringToStartOfStatement) {
+  StringRef AsmStr = "jne #-4";
+
+  // Setup.
+  setupCallToAsmParser(AsmStr);
+
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::Identifier, AsmToken::EndOfStatement});
+  lexAndCheckTokens(AsmStr /* "jne #-4" */, ExpectedTokens);
+}
+
+// Testing MCAsmInfo's RestrictCommentStringToStartOfStatement attribute.
+TEST_F(SystemZAsmLexerTest, CheckRestrictCommentStringToStartOfStatement) {
+  StringRef AsmStr = "jne #-4";
+
+  // Setup.
+  MUPMAI->setRestrictCommentStringToStartOfStatement(true);
+  setupCallToAsmParser(AsmStr);
+
+  // When we are restricting the comment string to only the start of the
+  // statement, The sequence of tokens we are expecting are: Identifier - "jne"
+  // Hash - '#'
+  // Minus - '-'
+  // Integer - '4'
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::Identifier, AsmToken::Hash, AsmToken::Minus,
+       AsmToken::Integer});
+  lexAndCheckTokens(AsmStr /* "jne #-4" */, ExpectedTokens);
+}
+
+// Test HLASM Comment Syntax ('*')
+TEST_F(SystemZAsmLexerTest, CheckHLASMComment) {
+  StringRef AsmStr = "* lhi 1,10";
+
+  // Setup.
+  MUPMAI->setCommentString("*");
+  setupCallToAsmParser(AsmStr);
+
+  SmallVector<AsmToken::TokenKind> ExpectedTokens(
+      {AsmToken::EndOfStatement, AsmToken::Eof});
+  lexAndCheckTokens(AsmStr /* "* lhi 1,10" */, ExpectedTokens);
+}
+} // end anonymous namespace