[AsmParser][SystemZ][z/OS] Introducing HLASM Comment Syntax

[anirudhp]

Summary

• This patch adds in support for the ordinary HLASM comment syntax asm statements (Reference - Chapter 7, Comment Statements, Ordinary Comment Statements)
• In brief, the ordinary comment syntax if used, must begin with the "*" character
• To achieve this, this patch makes use of the CommentString attribute provided in the base MCAsmInfo class
• In the SystemZMCAsmInfo class, the CommentString attribute was set to "*" based on the assembler dialect
• Furthermore, a new attribute RestrictCommentString, is provided to only treat a string as a comment if it appears at the start of the asm statement. Example: "jo *-4" is valid in HLASM (jump back 4 bytes from current point - similar to jo -4 in gnu asm) and we don't want "*-4" to be treated as a comment.

General Comments:

• We are ongoing in our work to provide support for the z/OS target to LLVM. The z/OS target is still missing MCStreamer support, and hence there are no tests (for the z/OS target) provided as part of this patch (as it is impossible to generate code for z/OS currently). When the MCStreamer support is enabled, I will put up relevant test(s) as part of a separate patch.

• RFC for HLASM Parser support implementation

[scott.linder]

Can you add a test? It seems like you already have the jo *-4 example so just a test that the following does what you expect would be enough for me:

* comment
jo *-4

llvm/include/llvm/MC/MCAsmInfo.h

125

This seems to be stale? It seems like this should read "the comment string", unless the strncmp in AsmLexer::isAtStartOfComment is just dead code?

129

Ditto here, the new option should apply for every valid CommentString, which appears to be any non-zero-length string (with some weird special case when the second character is #)

131

I would prefer a name that describes how the comment string is restricted, maybe RestrictCommentStringToStartOfStatement? It is a bit long, but we already have e.g. ZeroDirectiveSupportsNonZeroValue, and these options are only used in a couple well-defined places where a descriptive name seems worth the extra characters.

llvm/lib/MC/MCParser/AsmLexer.cpp

667

To implement the comment below, can this be replaced with another if at the start of the function:

if (RestrictCommentString && !IsAtStartOfStatement)
  return false;

673

It may not be needed for HLASM, but if we are adding an option which appears to be orthogonal to CommentString then I think it ought to handle the non-single-character-string case. I.e. if CommentString == "//" then your current RestrictCommentString has an unexpected effect (i.e. it no longer applies).

[anirudhp]

In D97703#2600429, @scott.linder wrote:

Can you add a test? It seems like you already have the jo *-4 example so just a test that the following does what you expect would be enough for me:

* comment
jo *-4

Thank you for your feedback! I will address the other inline comments shortly. However, for the test, as mentioned in the description, we don't have MCStreamer support yet for the z/OS target, so any test would just fail outright (as mentioned above, we are working on putting it up, but I can't provide a fixed timeframe). I understand that this is problematic, since its best practice to put up a test along with any changes to implementation. Do you have any other recommendations as to how to go about this?

[scott.linder]

In D97703#2600474, @anirudhp wrote:

In D97703#2600429, @scott.linder wrote:

Can you add a test? It seems like you already have the jo *-4 example so just a test that the following does what you expect would be enough for me:

* comment
jo *-4

Thank you for your feedback! I will address the other inline comments shortly. However, for the test, as mentioned in the description, we don't have MCStreamer support yet for the z/OS target, so any test would just fail outright (as mentioned above, we are working on putting it up, but I can't provide a fixed timeframe). I understand that this is problematic, since its best practice to put up a test along with any changes to implementation. Do you have any other recommendations as to how to go about this?

Thank you for the patch! And I'm sorry, I thought I had read the whole description before I hit submit, but evidently not :^)

I don't know the best route, but in similar contexts I have seen the advice in this case (when tests are impossible) to be waiting and including this as part of the first patch for which at least some testing would be possible. I imagine you would like to land this now to simplify the future work, but I would defer to someone with more experience as to whether that is OK in this case.

[anirudhp]

• Few minor changes addressing comments.

[anirudhp]

Thank you for the patch! And I'm sorry, I thought I had read the whole description before I hit submit, but evidently not :^)

I don't know the best route, but in similar contexts I have seen the advice in this case (when tests are impossible) to be waiting and including this as part of the first patch for which at least some testing would be possible. I imagine you would like to land this now to simplify the future work, but I would defer to someone with more experience as to whether that is OK in this case.

Yup, the intention behind the early submission of this patch was to simplify the future work, and attempt to get things done in parallel (if possible). I'll wait for some more comments to see if there are any recommendations on the testing issue.

[scott.linder]

Thank you for making the changes, this LGTM modulo feedback from others on the testing

[anirudhp]

Ping :)

[anirudhp]

• Added a unittest in lieu of a generic lit test (which cannot be put in due to missing MCStreamer and Object file support for the z/OS target in LLVM)

[anirudhp]

In D97703#2606944, @scott.linder wrote:

Thank you for making the changes, this LGTM modulo feedback from others on the testing

Hi @scott.linder ,
Would you please be able to re-review this patch (or just the added test case :) )?

I was able to come up with a unit test instead. The unit test checks the behaviour the RestrictCommentStringToStartOfStatement attribute. Hopefully this should answer the testing issue, and should be sufficient as a "test" for the patch, until more lit tests can be added (once the necessary MCStreamer and Object file support for the z/OS target is added in)

[anirudhp]

• Addressing clang-tidy warnings + errors
• Rebasing

[scott.linder]

Thank you for coming up with a way to test the change! It LGTM.

Couple minor nits, but if you are OK with the requested changes feel free to make them and commit without updating the review

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

52 ↗

(On Diff #329663)

Can be static

69 ↗

(On Diff #329663)

I think these should be ASSERT_NE, because the rest of the test relies on this

[anirudhp]

• minor change to unit test

[anirudhp]

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

69 ↗

(On Diff #329663)

I do see your point, because if the returned pointers are null, it doesn't make sense to run the rest of the test.

In terms of the ASSERT_* macros, I don't believe you can use them in a constructor. I could create a separate function where these are checked and call this for each individual test fixture.

But looking at some of the other tests that currently create an AsmParser, MCAsmInfo instances, either the returned pointer isn't checked, or its checked using the EXPECT_* macro. So I'm leaning towards just keeping it like this (for now anyway).

[anirudhp]

• Added the missing copyright to the newly created unittest.

[scott.linder]

llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp

69 ↗

(On Diff #329663)

I didn't realize the ASSERT_* macros weren't available; I am fine with the EXPECT_* macros, as they will still correctly flag the test as a failure, there just may be additional noise in the output when the test fails. If we are already doing this elsewhere I am happy to say LGTM!

[Kai]

LGTM.
It's nice to have this test!

[thakis]

llvm/unittests/MC/SystemZ/CMakeLists.txt

11 ↗

(On Diff #330263)

All other unit test binaries have a name that ends in "Tests". Could this one too?

Actually, does this need to be in its own binary? Can't this just be part of MCTests?

[thakis]

llvm/unittests/MC/SystemZ/CMakeLists.txt

11 ↗

(On Diff #330263)

All other unit test binaries have a name that ends in "Tests". Could this one too?

Actually, does this need to be in its own binary? Can't this just be part of MCTests?

I retract the second question – I guess it's due to the dep on SystemZ. First question still stands, though :)

[anirudhp]

llvm/unittests/MC/SystemZ/CMakeLists.txt

11 ↗

(On Diff #330263)

Ahhh, this commit was just merged in :(

How about I do this?

I'm working on another patch, and that patch is going to need changes to this test again. At that point, I will suffix the name of the test with "Tests". Does that sound okay? (I'll make sure to add you as the reviewer! :) )

[fhahn]

I'm seeing the following error when doing a shared-library build. Could this be caused by the patch?

Undefined symbols for architecture x86_64:
  "llvm::createMCAsmParser(llvm::SourceMgr&, llvm::MCContext&, llvm::MCStreamer&, llvm::MCAsmInfo const&, unsigned int)", referenced from:
      (anonymous namespace)::SystemZAsmLexerTest::setupCallToAsmParser(llvm::StringRef) in SystemZAsmLexerTest.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[thakis]

llvm/unittests/MC/SystemZ/CMakeLists.txt

11 ↗

(On Diff #330263)

Sure, sounds fine. You come also send a patch that does nothing but rename the target -- but there's no big rush, so either is fine :) thanks!

[anirudhp]

In D97703#2622720, @fhahn wrote:

I'm seeing the following error when doing a shared-library build. Could this be caused by the patch?

Undefined symbols for architecture x86_64:
  "llvm::createMCAsmParser(llvm::SourceMgr&, llvm::MCContext&, llvm::MCStreamer&, llvm::MCAsmInfo const&, unsigned int)", referenced from:
      (anonymous namespace)::SystemZAsmLexerTest::setupCallToAsmParser(llvm::StringRef) in SystemZAsmLexerTest.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Sorry about that. From the trace, it does seem to be because of this patch.

Strange though, the remote builds didn't seem to indicate any issues (https://reviews.llvm.org/harbormaster/build/129916/ and https://buildkite.com/llvm-project/diff-checks/builds/33061)

[anirudhp]

@fhahn
Do you have a bot link? I'm trying to track if it fails on any others bots as well.

[fhahn]

In D97703#2622835, @anirudhp wrote:

@fhahn
Do you have a bot link? I'm trying to track if it fails on any others bots as well.

No, unfortunately I don't, but I think there definitely are bots that build with shared libraries on. To reproduce, you should be able to just build with -DBUILD_SHARED_LIBS=On

[anirudhp]

In D97703#2622952, @fhahn wrote:

In D97703#2622835, @anirudhp wrote:

@fhahn
Do you have a bot link? I'm trying to track if it fails on any others bots as well.

No, unfortunately I don't, but I think there definitely are bots that build with shared libraries on. To reproduce, you should be able to just build with -DBUILD_SHARED_LIBS=On

I was able to reproduce it on a ppc64le instance using the -DBUILD_SHARED_LIBS=On (We were building with BUILD_SHARED_LIBS=OFF by default). My sincere apologies @fhahn, I will put up a patch shortly to revert it.

[anirudhp]

In D97703#2622952, @fhahn wrote:

In D97703#2622835, @anirudhp wrote:

@fhahn
Do you have a bot link? I'm trying to track if it fails on any others bots as well.

No, unfortunately I don't, but I think there definitely are bots that build with shared libraries on. To reproduce, you should be able to just build with -DBUILD_SHARED_LIBS=On

@fhahn, this has been reverted (https://github.com/llvm/llvm-project/commit/4f9cc15). Once again, my apologies for bringing down your build.

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
   /// "#"
   StringRef CommentString;
 
+  /// This indicates whether the comment character is only accepted as a comment
+  /// at the beginning of statements. Defaults to false.
+  bool RestrictCommentString = 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 getRestrictCommentString() const { return RestrictCommentString; }
   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

size_t AsmLexer::peekTokens(MutableArrayRef<AsmToken> Buf,
   }
 
   SetError(SavedErrLoc, SavedErr);
   return ReadCount;
 }
 
 bool AsmLexer::isAtStartOfComment(const char *Ptr) {
   StringRef CommentString = MAI.getCommentString();
+  bool RestrictCommentString = MAI.getRestrictCommentString();
 
   if (CommentString.size() == 1)
-    return CommentString[0] == Ptr[0];
+    return CommentString[0] == Ptr[0] &&
+           (!RestrictCommentString || IsAtStartOfStatement);
 
   // Allow # preprocessor commments also be counted as comments for "##" cases
   if (CommentString[1] == '#')
     return CommentString[0] == Ptr[0];
 
   return strncmp(Ptr, CommentString.data(), CommentString.size()) == 0;
 }
 
 bool AsmLexer::isAtStatementSeparator(const char *Ptr) {
   return strncmp(Ptr, MAI.getSeparatorString(),
                  strlen(MAI.getSeparatorString())) == 0;
 }
 
 AsmToken AsmLexer::LexToken() {

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 ? "*" : "#";
+  RestrictCommentString = (AssemblerDialect == AD_HLASM);
   ZeroDirective = "\t.space\t";
   Data64bitsDirective = "\t.quad\t";
   UsesELFSectionDirectiveForBSS = true;
   SupportsDebugInformation = true;
   ExceptionsType = ExceptionHandling::DwarfCFI;
 }