[ms] [llvm-ml] Add support for the (many) optional SEGMENT parameters

[epastor]

Summary

Add support for many parameters to the SEGMENT directive

[thakis]

Looks great, thanks :)

llvm/lib/MC/MCParser/COFFMasmParser.cpp

190

If you use git clang-format main or git clang-format HEAD~, it'll only format lines you touched, not unrelated lines.

If you do want to reformat these lines, land this in an unrelated change.

261

nit: add tailing .

349

Should we warn on unknown classes?

[epastor]

Addressing feedback

[epastor]

Removing another accidental reformat

llvm/lib/MC/MCParser/COFFMasmParser.cpp

349

Strangely, I don't think so - my testing suggests that ML64.EXE treats any unknown class as equivalent to DATA.

[thakis]

Ship it :)

llvm/lib/MC/MCParser/COFFMasmParser.cpp

349

Ah cool, even covered by tests.

[thakis]

llvm/lib/MC/MCParser/COFFMasmParser.cpp

333

My bot remarks:

../../llvm/lib/MC/MCParser/COFFMasmParser.cpp(333,28): warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
        if (Characteristic == -1) {
            ~~~~~~~~~~~~~~ ^  ~~

Either use static_cast<unsigned>(-1) or UINT_MAX.

llvm/lib/MC/MCParser/COFFMasmParser.cpp

 //===- COFFMasmParser.cpp - COFF MASM Assembly Parser ---------------------===//
 //
 // 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/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/COFF.h"
+#include "llvm/MC/MCAsmMacro.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
 #include "llvm/MC/MCParser/MCAsmParserExtension.h"
+#include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #include "llvm/MC/MCSectionCOFF.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSymbolCOFF.h"
 #include "llvm/MC/SectionKind.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/SMLoc.h"
 #include <cstdint>
 #include <utility>
 
 using namespace llvm;
 
 namespace {
 
 class COFFMasmParser : public MCAsmParserExtension {
   template <bool (COFFMasmParser::*HandlerMethod)(StringRef, SMLoc)>
   void addDirectiveHandler(StringRef Directive) {
     MCAsmParser::ExtensionDirectiveHandler Handler =
         std::make_pair(this, HandleDirective<COFFMasmParser, HandlerMethod>);
     getParser().addDirectiveHandler(Directive, Handler);
   }
 
-  bool ParseSectionSwitch(StringRef Section, unsigned Characteristics,
+  bool ParseSectionSwitch(StringRef SectionName, unsigned Characteristics,
                           SectionKind Kind);
 
-  bool ParseSectionSwitch(StringRef Section, unsigned Characteristics,
+  bool ParseSectionSwitch(StringRef SectionName, unsigned Characteristics,
                           SectionKind Kind, StringRef COMDATSymName,
-                          COFF::COMDATType Type);
+                          COFF::COMDATType Type, Align Alignment);
 
   bool ParseDirectiveProc(StringRef, SMLoc);
   bool ParseDirectiveEndProc(StringRef, SMLoc);
   bool ParseDirectiveSegment(StringRef, SMLoc);
   bool ParseDirectiveSegmentEnd(StringRef, SMLoc);
   bool ParseDirectiveIncludelib(StringRef, SMLoc);
 
   bool ParseDirectiveAlias(StringRef, SMLoc);
void Initialize(MCAsmParser &Parser) override {
     // .seq (32-bit only, order segments sequentially)
     addDirectiveHandler<&COFFMasmParser::ParseDirectiveSegmentEnd>("ends");
     // group (32-bit only)
     addDirectiveHandler<&COFFMasmParser::ParseDirectiveSegment>("segment");
 
     // Simplified segment directives
     addDirectiveHandler<&COFFMasmParser::ParseSectionDirectiveCode>(".code");
     // .const
-    addDirectiveHandler<
-        &COFFMasmParser::ParseSectionDirectiveInitializedData>(".data");
+    addDirectiveHandler<&COFFMasmParser::ParseSectionDirectiveInitializedData>(
+        ".data");
     addDirectiveHandler<
         &COFFMasmParser::ParseSectionDirectiveUninitializedData>(".data?");
     // .exit
     // .fardata
     // .fardata?
     addDirectiveHandler<&COFFMasmParser::IgnoreDirective>(".model");
     // .stack
     // .startup
void Initialize(MCAsmParser &Parser) override {
     // typedef
   }
 
   bool ParseSectionDirectiveCode(StringRef, SMLoc) {
     return ParseSectionSwitch(".text",
                               COFF::IMAGE_SCN_CNT_CODE
                             | COFF::IMAGE_SCN_MEM_EXECUTE
                             | COFF::IMAGE_SCN_MEM_READ,
                               SectionKind::getText());
   }
 
   bool ParseSectionDirectiveInitializedData(StringRef, SMLoc) {
     return ParseSectionSwitch(".data",
                               COFF::IMAGE_SCN_CNT_INITIALIZED_DATA
                             | COFF::IMAGE_SCN_MEM_READ
                             | COFF::IMAGE_SCN_MEM_WRITE,
                               SectionKind::getData());
class COFFMasmParser : public MCAsmParserExtension {
   bool CurrentProcedureFramed;
 
 public:
   COFFMasmParser() = default;
 };
 
 } // end anonymous namespace.
 
-static SectionKind computeSectionKind(unsigned Flags) {
-  if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
-    return SectionKind::getText();
-  if (Flags & COFF::IMAGE_SCN_MEM_READ &&
-      (Flags & COFF::IMAGE_SCN_MEM_WRITE) == 0)
-    return SectionKind::getReadOnly();
-  return SectionKind::getData();
-}
-
-bool COFFMasmParser::ParseSectionSwitch(StringRef Section,
+bool COFFMasmParser::ParseSectionSwitch(StringRef SectionName,
                                         unsigned Characteristics,
                                         SectionKind Kind) {
-  return ParseSectionSwitch(Section, Characteristics, Kind, "",
-                            (COFF::COMDATType)0);
+  return ParseSectionSwitch(SectionName, Characteristics, Kind, "",
+                            (COFF::COMDATType)0, Align(16));
 }
 
-bool COFFMasmParser::ParseSectionSwitch(StringRef Section,
-                                        unsigned Characteristics,
-                                        SectionKind Kind,
-                                        StringRef COMDATSymName,
-                                        COFF::COMDATType Type) {
+bool COFFMasmParser::ParseSectionSwitch(
+    StringRef SectionName, unsigned Characteristics, SectionKind Kind,
+    StringRef COMDATSymName, COFF::COMDATType Type, Align Alignment) {
   if (getLexer().isNot(AsmToken::EndOfStatement))
     return TokError("unexpected token in section switching directive");
   Lex();
 
-  getStreamer().switchSection(getContext().getCOFFSection(
-      Section, Characteristics, Kind, COMDATSymName, Type));
+  MCSection *Section = getContext().getCOFFSection(SectionName, Characteristics,
+                                                   Kind, COMDATSymName, Type);
+  Section->setAlignment(Alignment);
+  getStreamer().switchSection(Section);
 
   return false;
 }
 
 bool COFFMasmParser::ParseDirectiveSegment(StringRef Directive, SMLoc Loc) {
   StringRef SegmentName;
   if (!getLexer().is(AsmToken::Identifier))
     return TokError("expected identifier in directive");
   SegmentName = getTok().getIdentifier();
   Lex();
 
   StringRef SectionName = SegmentName;
   SmallVector<char, 247> SectionNameVector;
-  unsigned Flags = COFF::IMAGE_SCN_CNT_INITIALIZED_DATA |
-                   COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_MEM_WRITE;
+
+  StringRef Class;
   if (SegmentName == "_TEXT" || SegmentName.startswith("_TEXT$")) {
     if (SegmentName.size() == 5) {
       SectionName = ".text";
     } else {
       SectionName =
           (".text$" + SegmentName.substr(6)).toStringRef(SectionNameVector);
     }
-    Flags = COFF::IMAGE_SCN_CNT_CODE | COFF::IMAGE_SCN_MEM_EXECUTE |
-            COFF::IMAGE_SCN_MEM_READ;
+    Class = "CODE";
   }
-  SectionKind Kind = computeSectionKind(Flags);
-  getStreamer().switchSection(getContext().getCOFFSection(
-      SectionName, Flags, Kind, "", (COFF::COMDATType)(0)));
+
+  // Parse all options to end of statement.
+  // Alignment defaults to PARA if unspecified.
+  int64_t Alignment = 16;
+  // Default flags are used only if no characteristics are set.
+  bool DefaultCharacteristics = true;
+  unsigned Flags = 0;
+  // "obsolete" according to the documentation, but still supported.
+  bool Readonly = false;
+  while (getLexer().isNot(AsmToken::EndOfStatement)) {
+    switch (getTok().getKind()) {
+    default:
+      break;
+    case AsmToken::String: {
+      // Class identifier; overrides Kind.
+      Class = getTok().getStringContents();
+      Lex();
+      break;
+    }
+    case AsmToken::Identifier: {
+      SMLoc KeywordLoc = getTok().getLoc();
+      StringRef Keyword;
+      if (getParser().parseIdentifier(Keyword)) {
+        llvm_unreachable("failed to parse identifier at an identifier token");
+      }
+      if (Keyword.equals_insensitive("byte")) {
+        Alignment = 1;
+      } else if (Keyword.equals_insensitive("word")) {
+        Alignment = 2;
+      } else if (Keyword.equals_insensitive("dword")) {
+        Alignment = 4;
+      } else if (Keyword.equals_insensitive("para")) {
+        Alignment = 16;
+      } else if (Keyword.equals_insensitive("page")) {
+        Alignment = 256;
+      } else if (Keyword.equals_insensitive("align")) {
+        if (getParser().parseToken(AsmToken::LParen) ||
+            getParser().parseIntToken(Alignment,
+                                      "Expected integer alignment") ||
+            getParser().parseToken(AsmToken::RParen)) {
+          return Error(getTok().getLoc(),
+                       "Expected (n) following ALIGN in SEGMENT directive");
+        }
+        if (!isPowerOf2_64(Alignment) || Alignment > 8192) {
+          return Error(KeywordLoc,
+                       "ALIGN argument must be a power of 2 from 1 to 8192");
+        }
+      } else if (Keyword.equals_insensitive("alias")) {
+        if (getParser().parseToken(AsmToken::LParen) ||
+            !getTok().is(AsmToken::String))
+          return Error(
+              getTok().getLoc(),
+              "Expected (string) following ALIAS in SEGMENT directive");
+        SectionName = getTok().getStringContents();
+        Lex();
+        if (getParser().parseToken(AsmToken::RParen))
+          return Error(
+              getTok().getLoc(),
+              "Expected (string) following ALIAS in SEGMENT directive");
+      } else if (Keyword.equals_insensitive("readonly")) {
+        Readonly = true;
+      } else {
+        unsigned Characteristic =
+            StringSwitch<unsigned>(Keyword)
+                .CaseLower("info", COFF::IMAGE_SCN_LNK_INFO)
+                .CaseLower("read", COFF::IMAGE_SCN_MEM_READ)
+                .CaseLower("write", COFF::IMAGE_SCN_MEM_WRITE)
+                .CaseLower("execute", COFF::IMAGE_SCN_MEM_EXECUTE)
+                .CaseLower("shared", COFF::IMAGE_SCN_MEM_SHARED)
+                .CaseLower("nopage", COFF::IMAGE_SCN_MEM_NOT_PAGED)
+                .CaseLower("nocache", COFF::IMAGE_SCN_MEM_NOT_CACHED)
+                .CaseLower("discard", COFF::IMAGE_SCN_MEM_DISCARDABLE)
+                .Default(-1);
+        if (Characteristic == -1) {
+          return Error(KeywordLoc,
+                       "Expected characteristic in SEGMENT directive; found '" +
+                           Keyword + "'");
+        }
+        Flags |= Characteristic;
+        DefaultCharacteristics = false;
+      }
+    }
+    }
+  }
+
+  SectionKind Kind = StringSwitch<SectionKind>(Class)
+                         .CaseLower("data", SectionKind::getData())
+                         .CaseLower("code", SectionKind::getText())
+                         .CaseLower("const", SectionKind::getReadOnly())
+                         .Default(SectionKind::getData());
+  if (Kind.isText()) {
+    if (DefaultCharacteristics) {
+      Flags |= COFF::IMAGE_SCN_MEM_EXECUTE | COFF::IMAGE_SCN_MEM_READ;
+    }
+    Flags |= COFF::IMAGE_SCN_CNT_CODE;
+  } else {
+    if (DefaultCharacteristics) {
+      Flags |= COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_MEM_WRITE;
+    }
+    Flags |= COFF::IMAGE_SCN_CNT_INITIALIZED_DATA;
+  }
+  if (Readonly) {
+    Flags &= ~COFF::IMAGE_SCN_MEM_WRITE;
+  }
+
+  MCSection *Section = getContext().getCOFFSection(SectionName, Flags, Kind, "",
+                                                   (COFF::COMDATType)(0));
+  if (Alignment != 0) {
+    Section->setAlignment(Align(Alignment));
+  }
+  getStreamer().switchSection(Section);
   return false;
 }
 
 /// ParseDirectiveSegmentEnd
 ///  ::= identifier "ends"
 bool COFFMasmParser::ParseDirectiveSegmentEnd(StringRef Directive, SMLoc Loc) {
   StringRef SegmentName;
   if (!getLexer().is(AsmToken::Identifier))
     return TokError("expected identifier in directive");
   SegmentName = getTok().getIdentifier();
 
   // Ignore; no action necessary.
   Lex();
   return false;
 }
 
 /// ParseDirectiveIncludelib
 ///  ::= "includelib" identifier
 bool COFFMasmParser::ParseDirectiveIncludelib(StringRef Directive, SMLoc Loc) {
   StringRef Lib;
   if (getParser().parseIdentifier(Lib))
     return TokError("expected identifier in includelib directive");
 
   unsigned Flags = COFF::IMAGE_SCN_MEM_PRELOAD | COFF::IMAGE_SCN_MEM_16BIT;
-  SectionKind Kind = computeSectionKind(Flags);
+  SectionKind Kind = SectionKind::getData();
   getStreamer().pushSection();
   getStreamer().switchSection(getContext().getCOFFSection(
       ".drectve", Flags, Kind, "", (COFF::COMDATType)(0)));
   getStreamer().emitBytes("/DEFAULTLIB:");
   getStreamer().emitBytes(Lib);
   getStreamer().emitBytes(" ");
   getStreamer().popSection();
   return false;

llvm/test/tools/llvm-ml/segment.asm

+; RUN: llvm-ml %s /Fo - | llvm-readobj --section-headers - | FileCheck %s
+
+t1 SEGMENT BYTE
+t1 ENDS
+; CHECK-LABEL: Name: t1
+; CHECK-NOT: }
+; CHECK: IMAGE_SCN_ALIGN_1BYTES
+
+t2 SEGMENT WORD
+t2 ENDS
+; CHECK-LABEL: Name: t2
+; CHECK-NOT: }
+; CHECK: IMAGE_SCN_ALIGN_2BYTES
+
+t3 SEGMENT DWORD
+t3 ENDS
+; CHECK-LABEL: Name: t3
+; CHECK-NOT: }
+; CHECK: IMAGE_SCN_ALIGN_4BYTES
+
+t4 SEGMENT PARA
+t4 ENDS
+; CHECK-LABEL: Name: t4
+; CHECK-NOT: }
+; CHECK: IMAGE_SCN_ALIGN_16BYTES
+
+t5 SEGMENT PAGE
+t5 ENDS
+; CHECK-LABEL: Name: t5
+; CHECK-NOT: }
+; CHECK: IMAGE_SCN_ALIGN_256BYTES
+
+; Default alignment (should match PARA)
+t6 SEGMENT
+t6 ENDS
+; CHECK-LABEL: Name: t6
+; CHECK-NOT: }
+; CHECK: IMAGE_SCN_ALIGN_16BYTES
+
+; Unnamed alignment
+t7 SEGMENT ALIGN(32)
+t7 ENDS
+; CHECK-LABEL: Name: t7
+; CHECK-NOT: }
+; CHECK: IMAGE_SCN_ALIGN_32BYTES
+
+t8 SEGMENT ALIAS('t8_alias')
+t8 ENDS
+; CHECK-LABEL: Name: t8_alias
+
+t9 SEGMENT 'DATA'
+t9 ENDS
+; CHECK-LABEL: Name: t9
+; CHECK-NOT: }
+; CHECK-DAG: IMAGE_SCN_CNT_INITIALIZED_DATA
+; CHECK-DAG: IMAGE_SCN_MEM_READ
+; CHECK-DAG: IMAGE_SCN_MEM_WRITE
+; CHECK: }
+
+t10 SEGMENT 'CODE'
+t10 ENDS
+; CHECK-LABEL: Name: t10
+; CHECK-NOT: }
+; CHECK-NOT: IMAGE_SCN_MEM_WRITE
+; CHECK-DAG: IMAGE_SCN_CNT_CODE
+; CHECK-DAG: IMAGE_SCN_MEM_EXECUTE
+; CHECK-DAG: IMAGE_SCN_MEM_READ
+; CHECK-NOT: IMAGE_SCN_MEM_WRITE
+; CHECK: }
+
+t11 SEGMENT 'CONST'
+t11 ENDS
+; CHECK-LABEL: Name: t11
+; CHECK-NOT: }
+; CHECK-DAG: IMAGE_SCN_CNT_INITIALIZED_DATA
+; CHECK-DAG: IMAGE_SCN_MEM_READ
+; CHECK-DAG: IMAGE_SCN_MEM_WRITE
+; CHECK: }
+
+t12 SEGMENT 'STACK'
+t12 ENDS
+; CHECK-LABEL: Name: t12
+; CHECK-NOT: }
+; CHECK-DAG: IMAGE_SCN_CNT_INITIALIZED_DATA
+; CHECK-DAG: IMAGE_SCN_MEM_READ
+; CHECK-DAG: IMAGE_SCN_MEM_WRITE
+; CHECK: }
+
+t13 SEGMENT 'DOESNTEXIST'
+t13 ENDS
+; CHECK-LABEL: Name: t13
+; CHECK-NOT: }
+; CHECK-DAG: IMAGE_SCN_CNT_INITIALIZED_DATA
+; CHECK-DAG: IMAGE_SCN_MEM_READ
+; CHECK-DAG: IMAGE_SCN_MEM_WRITE
+; CHECK: }
+
+t14 SEGMENT READONLY 'CONST'
+t14 ENDS
+; CHECK-LABEL: Name: t14
+; CHECK-NOT: }
+; CHECK-NOT: IMAGE_SCN_MEM_WRITE
+; CHECK-DAG: IMAGE_SCN_CNT_INITIALIZED_DATA
+; CHECK-DAG: IMAGE_SCN_MEM_READ
+; CHECK-NOT: IMAGE_SCN_MEM_WRITE
+; CHECK: }
+
+t15 SEGMENT WRITE
+t15 ENDS
+; CHECK-LABEL: Name: t15
+; CHECK-NOT: }
+; CHECK-NOT: IMAGE_SCN_MEM_READ
+; CHECK-DAG: IMAGE_SCN_CNT_INITIALIZED_DATA
+; CHECK-DAG: IMAGE_SCN_MEM_WRITE
+; CHECK-NOT: IMAGE_SCN_MEM_READ
+; CHECK: }
+
+t16 SEGMENT SHARED NOPAGE NOCACHE INFO READ WRITE EXECUTE DISCARD
+t16 ENDS
+; CHECK-LABEL: Name: t16
+; CHECK-NOT: }
+; CHECK-DAG: IMAGE_SCN_LNK_INFO
+; CHECK-DAG: IMAGE_SCN_MEM_READ
+; CHECK-DAG: IMAGE_SCN_MEM_WRITE
+; CHECK-DAG: IMAGE_SCN_MEM_EXECUTE
+; CHECK-DAG: IMAGE_SCN_MEM_SHARED
+; CHECK-DAG: IMAGE_SCN_MEM_NOT_PAGED
+; CHECK-DAG: IMAGE_SCN_MEM_NOT_CACHED
+; CHECK-DAG: IMAGE_SCN_MEM_DISCARDABLE
+; CHECK: }
+
+_TEXT SEGMENT
+_TEXT ENDS
+; CHECK-LABEL: Name: .text
+; CHECK-NOT: }
+; CHECK-NOT: IMAGE_SCN_MEM_WRITE
+; CHECK-DAG: IMAGE_SCN_CNT_CODE
+; CHECK-DAG: IMAGE_SCN_MEM_EXECUTE
+; CHECK-DAG: IMAGE_SCN_MEM_READ
+; CHECK-NOT: IMAGE_SCN_MEM_WRITE
+; CHECK: }
+
+END