[PATCH] D97748: [SystemZ][z/OS] Add support to validate a HLASM Label.

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added a comment.

In D97748#2597782 <https://reviews.llvm.org/D97748#2597782>, @anirudhp wrote:

> An additional point:
> I checked the V1R6 Reference manual again and I seem to have forgotten to include a check for "National Characters" ($,@ and #). I would like to remedy this by adding two private inline functions `isHLASMAlpha` and `isHLASMAlnum` which encapsulates the checks for what is an "HLASM" alphabet, and use them in the overridden `isLabel` routine. Is this acceptable?

This raises an issue having to do with the encoding used for string literals. It seems that the XL compiler processes the string like it would for emission into the object file and then the assembly is parsed as IBM-1047.

For example:

  %:pragma filetag("IBM-1143")
  %:include <stdio.h>
  int main(void) ??<
    unsigned a = 0;
    %:pragma convert("IBM-1026")
    __asm__(
    "LAB??/x5B??/x7B??/x7C AFI %0,C'A' "
    : "+r"(a)
    :
    );
    printf("%02x??/n", a);
  ??>

parses okay because 0x5B, 0x7B, and 0x7C are the encodings for the "National Characters" in IBM-1047.



================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1605
+    // Rules:
+    // 1. Starts with an alphabetic character, followed by 62-alphanumberic
+    // characters
----------------
anirudhp wrote:
> hubert.reinterpretcast wrote:
> > Please fix the spelling for "alphanumeric". Also, I don't think the hyphen should be used here. If the "62" is a reference to the possible length, then the sentence is missing "up to".
> > 
> > Also, by "alphabetic", you mean (aside from `_`) 'considered alphabetic in the "C" locale'?
> Yes. By "alphabetic", aside from "_" it is what is considered alphabetic in the "C" locale (a character in the range 'a' to 'z' or 'A' to 'Z')
> 
I think adding `(in the "C" locale)` to the comment would help clarify that.


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1621
+    // '_' is treated as an alphabetic character.
+    if (!isalpha(RawLabel[0]) && RawLabel[0] != '_')
+      return !Error(Loc, "HLASM Label has to start with an alphabetic "
----------------
anirudhp wrote:
> hubert.reinterpretcast wrote:
> > Is this supposed to be a call to the locale-sensitive check? LLVM has an `isAlpha` function.
> As mentioned above, the intent of the call to `isalpha` is to check whether the character is alphabetic according to the c-locale.
> 
> If it is preferred to use the isAlpha function provided in `StringExtras.h` I can use it.
I believe `isAlpha` is preferred (because `isalpha` will give an answer based on the current locale).


================
Comment at: llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:1629
+    for (unsigned I = 1; I < RawLabel.size(); I++)
+      if (!isalnum(RawLabel[I]) && RawLabel[I] != '_')
+        return !Error(Loc, "HLASM Label has to be alphanumeric");
----------------
anirudhp wrote:
> hubert.reinterpretcast wrote:
> > See my comment about `isalpha` versus `isAlpha`.
> Same comment as above. If it is preferred to use the LLVM `isAlnum` I can use that.
Same response: `isAlnum` is a better match in terms of intent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97748/new/

https://reviews.llvm.org/D97748