This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by anirudhp on Mar 1 2021, 10:05 AM.

Details

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.

Diff Detail

Event Timeline

anirudhp created this revision.Mar 1 2021, 10:05 AM
anirudhp requested review of this revision.Mar 1 2021, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2021, 10:05 AM
anirudhp edited the summary of this revision. (Show Details)Mar 1 2021, 10:25 AM
anirudhp retitled this revision from [SystemZ][z/OS] Introducing HLASM Comment Syntax to [AsmParser][SystemZ][z/OS] Introducing HLASM Comment Syntax.Mar 2 2021, 10:14 AM

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
669

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

if (RestrictCommentString && !IsAtStartOfStatement)
  return false;
674

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).

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?

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 updated this revision to Diff 328004.Mar 3 2021, 8:13 PM
  • Few minor changes addressing comments.
anirudhp marked 5 inline comments as done.Mar 3 2021, 8:13 PM

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.

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

anirudhp updated this revision to Diff 329471.Mar 9 2021, 2:41 PM
  • 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 added a comment.EditedMar 9 2021, 2:44 PM

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 updated this revision to Diff 329663.Mar 10 2021, 7:36 AM
  • Addressing clang-tidy warnings + errors
  • Rebasing
scott.linder accepted this revision.Mar 10 2021, 9:08 AM

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
53

Can be static

70

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

This revision is now accepted and ready to land.Mar 10 2021, 9:08 AM
anirudhp updated this revision to Diff 329707.Mar 10 2021, 10:22 AM
  • minor change to unit test
anirudhp marked 2 inline comments as done.Mar 10 2021, 10:23 AM
anirudhp added inline comments.
llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
70

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 updated this revision to Diff 329825.Mar 10 2021, 7:24 PM
anirudhp marked an inline comment as done.
  • Added the missing copyright to the newly created unittest.
scott.linder added inline comments.Mar 11 2021, 10:31 AM
llvm/unittests/MC/SystemZ/SystemZAsmLexerTest.cpp
70

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!

scott.linder accepted this revision.Mar 11 2021, 10:31 AM
Kai accepted this revision.Mar 12 2021, 8:42 AM

LGTM.
It's nice to have this test!

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 12 2021, 9:08 AM
thakis added inline comments.
llvm/unittests/MC/SystemZ/CMakeLists.txt
11

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 added inline comments.Mar 12 2021, 9:13 AM
llvm/unittests/MC/SystemZ/CMakeLists.txt
11

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 marked an inline comment as done.Mar 12 2021, 9:19 AM
anirudhp added inline comments.
llvm/unittests/MC/SystemZ/CMakeLists.txt
11

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 added a subscriber: fhahn.Mar 12 2021, 10:30 AM

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 added inline comments.Mar 12 2021, 10:30 AM
llvm/unittests/MC/SystemZ/CMakeLists.txt
11

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 added a comment.EditedMar 12 2021, 10:35 AM

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)

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

@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 added a comment.EditedMar 12 2021, 11:26 AM

@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.

@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.