Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SystemZ][z/OS] yaml2obj GOFF symbols #75971

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

ysyeda
Copy link
Contributor

@ysyeda ysyeda commented Dec 19, 2023

This PR implements part 2 of yaml2obj for the GOFF Object File Format - it adds support for the symbols.

Copy link

github-actions bot commented Dec 19, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@ysyeda ysyeda changed the title [SystemZ][z/OS] yaml2obj part 2, adding sections & symbols [SystemZ][z/OS] yaml2obj GOFF sections & symbols Dec 19, 2023
@ysyeda ysyeda marked this pull request as ready for review December 19, 2023 21:42
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 19, 2023

@llvm/pr-subscribers-backend-systemz

@llvm/pr-subscribers-objectyaml

Author: Yusra Syeda (ysyeda)

Changes

This PR implements part 2 of yaml2obj for the GOFF Object File Format. It adds support for the sections and symbols.


Patch is 32.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75971.diff

10 Files Affected:

  • (modified) llvm/include/llvm/BinaryFormat/GOFF.h (+6)
  • (modified) llvm/include/llvm/ObjectYAML/GOFFYAML.h (+143)
  • (modified) llvm/lib/ObjectYAML/GOFFEmitter.cpp (+105-1)
  • (modified) llvm/lib/ObjectYAML/GOFFYAML.cpp (+254)
  • (added) llvm/test/tools/yaml2obj/GOFF/GOFF-TXTSection.yaml (+49)
  • (added) llvm/test/tools/yaml2obj/GOFF/GOFF-basic.yaml (+59)
  • (modified) llvm/test/tools/yaml2obj/GOFF/GOFF-header-end.yaml (+1)
  • (modified) llvm/test/tools/yaml2obj/GOFF/GOFF-header-settings.yaml (+2-1)
  • (added) llvm/test/tools/yaml2obj/GOFF/GOFF-longSymbol.yaml (+91)
  • (added) llvm/test/tools/yaml2obj/GOFF/GOFF-oneSymb.yaml (+40)
diff --git a/llvm/include/llvm/BinaryFormat/GOFF.h b/llvm/include/llvm/BinaryFormat/GOFF.h
index 443bcfc9479a8b..be6850216dbe2a 100644
--- a/llvm/include/llvm/BinaryFormat/GOFF.h
+++ b/llvm/include/llvm/BinaryFormat/GOFF.h
@@ -157,6 +157,12 @@ enum ESDAlignment : uint8_t {
   ESD_ALIGN_4Kpage = 12,
 };
 
+enum TXTRecordStyle : uint8_t {
+  TXT_RS_Byte = 0,
+  TXT_RS_Structured = 1,
+  TXT_RS_Unstructured = 2,
+};
+
 enum ENDEntryPointRequest : uint8_t {
   END_EPR_None = 0,
   END_EPR_EsdidOffset = 1,
diff --git a/llvm/include/llvm/ObjectYAML/GOFFYAML.h b/llvm/include/llvm/ObjectYAML/GOFFYAML.h
index f9bf45e95bd3a4..c17b4b605e258e 100644
--- a/llvm/include/llvm/ObjectYAML/GOFFYAML.h
+++ b/llvm/include/llvm/ObjectYAML/GOFFYAML.h
@@ -21,10 +21,120 @@
 
 namespace llvm {
 
+namespace GOFF {
+
+enum ESDFlags {
+  ESD_FillByteValuePresent = 1 << 7,
+  ESD_SymbolDisplayFlag = 1 << 6,
+  ESD_SymbolRenamingFlag = 1 << 5,
+  ESD_RemovableClass = 1 << 4
+};
+
+enum {
+  ESD_Mask_ERST = 0x07,
+  ESD_Mask_RQW = 0x07,
+  ESD_Mask_TextStyle = 0xf0,
+  ESD_Mask_BindingAlgorithm = 0x0f,
+};
+
+enum ESDBAFlags {
+  ESD_BA_Movable = 0x01,
+  ESD_BA_ReadOnly = 0x2,
+  ESD_BA_NoPrime = 0x4,
+  ESD_BA_COMMON = 0x8,
+  ESD_BA_Indirect = 0x10,
+};
+} // end namespace GOFF
+
 // The structure of the yaml files is not an exact 1:1 match to GOFF. In order
 // to use yaml::IO, we use these structures which are closer to the source.
 namespace GOFFYAML {
 
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDSYMBOLTYPE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDNAMESPACEID)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDFlags)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDAMODE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDRMODE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDTEXTSTYLE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDBINDINGALGORITHM)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDTASKINGBEHAVIOR)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDEXECUTABLE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDLINKAGETYPE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDBINDINGSTRENGTH)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDLOADINGBEHAVIOR)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDBINDINGSCOPE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDALIGNMENT)
+LLVM_YAML_STRONG_TYPEDEF(uint64_t, GOFF_BAFLAGS)
+
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_TEXTRECORDSTYLE)
+
+struct RecordBase {
+  enum RecordBaseKind { RBK_Symbol, RBK_Section };
+
+private:
+  const RecordBaseKind Kind;
+
+protected:
+  RecordBase(RecordBaseKind Kind) : Kind(Kind) {}
+
+public:
+  RecordBaseKind getKind() const { return Kind; }
+};
+typedef std::unique_ptr<RecordBase> RecordPtr;
+
+struct Section : public RecordBase {
+  Section() : RecordBase(RBK_Section) {}
+
+  StringRef SymbolName;
+  uint32_t SymbolID;
+  uint32_t Offset;
+  uint32_t TrueLength;
+  uint16_t TextEncoding;
+  uint16_t DataLength;
+  GOFF_TEXTRECORDSTYLE TextStyle;
+
+  std::optional<yaml::BinaryRef> Data;
+
+  static bool classof(const RecordBase *Rec) {
+    return Rec->getKind() == RBK_Section;
+  }
+};
+
+struct Symbol : public RecordBase {
+  Symbol() : RecordBase(RBK_Symbol) {}
+
+  StringRef Name;
+  GOFF_ESDSYMBOLTYPE Type;
+  uint32_t ID;
+  uint32_t OwnerID;
+  uint32_t Address;
+  uint32_t Length;
+  uint32_t ExtAttrID;
+  uint32_t ExtAttrOffset;
+  GOFF_ESDNAMESPACEID NameSpace;
+  GOFF_ESDFlags Flags;
+  uint8_t FillByteValue;
+  uint32_t PSectID;
+  uint32_t Priority;
+  std::optional<llvm::yaml::Hex64> Signature;
+  GOFF_ESDAMODE Amode;
+  GOFF_ESDRMODE Rmode;
+  GOFF_ESDTEXTSTYLE TextStyle;
+  GOFF_ESDBINDINGALGORITHM BindingAlgorithm;
+  GOFF_ESDTASKINGBEHAVIOR TaskingBehavior;
+  GOFF_ESDEXECUTABLE Executable;
+  GOFF_ESDLINKAGETYPE LinkageType;
+  GOFF_ESDBINDINGSTRENGTH BindingStrength;
+  GOFF_ESDLOADINGBEHAVIOR LoadingBehavior;
+  GOFF_ESDBINDINGSCOPE BindingScope;
+  GOFF_ESDALIGNMENT Alignment;
+  GOFF_BAFLAGS BAFlags;
+
+  static bool classof(const RecordBase *Rec) {
+    return Rec->getKind() == RBK_Symbol;
+  }
+};
+
 struct FileHeader {
   uint32_t TargetEnvironment = 0;
   uint32_t TargetOperatingSystem = 0;
@@ -38,12 +148,45 @@ struct FileHeader {
 
 struct Object {
   FileHeader Header;
+  std::vector<RecordPtr> Records;
   Object();
 };
 } // end namespace GOFFYAML
 } // end namespace llvm
 
+LLVM_YAML_IS_SEQUENCE_VECTOR(GOFFYAML::RecordPtr)
+
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDSYMBOLTYPE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDNAMESPACEID)
+LLVM_YAML_DECLARE_BITSET_TRAITS(GOFFYAML::GOFF_ESDFlags)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDAMODE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDRMODE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDTEXTSTYLE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDBINDINGALGORITHM)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDTASKINGBEHAVIOR)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDEXECUTABLE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDLINKAGETYPE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDBINDINGSTRENGTH)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDLOADINGBEHAVIOR)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDBINDINGSCOPE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDALIGNMENT)
+LLVM_YAML_DECLARE_BITSET_TRAITS(GOFFYAML::GOFF_BAFLAGS)
+
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_TEXTRECORDSTYLE)
+
+LLVM_YAML_DECLARE_MAPPING_TRAITS(GOFFYAML::Section)
+LLVM_YAML_DECLARE_MAPPING_TRAITS(GOFFYAML::Symbol)
 LLVM_YAML_DECLARE_MAPPING_TRAITS(GOFFYAML::FileHeader)
 LLVM_YAML_DECLARE_MAPPING_TRAITS(GOFFYAML::Object)
 
+namespace llvm {
+namespace yaml {
+
+template <> struct CustomMappingTraits<GOFFYAML::RecordPtr> {
+  static void inputOne(IO &IO, StringRef Key, GOFFYAML::RecordPtr &Elem);
+  static void output(IO &IO, GOFFYAML::RecordPtr &Elem);
+};
+
+} // end namespace yaml
+} // end namespace llvm
 #endif // LLVM_OBJECTYAML_GOFFYAML_H
diff --git a/llvm/lib/ObjectYAML/GOFFEmitter.cpp b/llvm/lib/ObjectYAML/GOFFEmitter.cpp
index 345904407e1d24..e7db0736993444 100644
--- a/llvm/lib/ObjectYAML/GOFFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/GOFFEmitter.cpp
@@ -22,6 +22,8 @@ using namespace llvm;
 
 namespace {
 
+static const uint8_t TXTMaxDataLength = 56;
+
 // Common flag values on records.
 enum {
   // Flag: This record is continued.
@@ -60,6 +62,11 @@ raw_ostream &operator<<(raw_ostream &OS, const ZerosImpl &Z) {
 
 ZerosImpl zeros(const size_t NumBytes) { return ZerosImpl{NumBytes}; }
 
+raw_ostream &operator<<(raw_ostream &OS, const yaml::BinaryRef &Data) {
+  Data.writeAsBinary(OS);
+  return OS;
+}
+
 // The GOFFOstream is responsible to write the data into the fixed physical
 // records of the format. A user of this class announces the start of a new
 // logical record and the size of its payload. While writing the payload, the
@@ -175,6 +182,8 @@ class GOFFOstream : public raw_ostream {
 class GOFFState {
   void writeHeader(GOFFYAML::FileHeader &FileHdr);
   void writeEnd();
+  void writeSymbol(GOFFYAML::Symbol Sym);
+  void writeSection(GOFFYAML::Section Sec);
 
   void reportError(const Twine &Msg) {
     ErrHandler(Msg);
@@ -183,7 +192,7 @@ class GOFFState {
 
   GOFFState(raw_ostream &OS, GOFFYAML::Object &Doc,
             yaml::ErrorHandler ErrHandler)
-      : GW(OS), Doc(Doc), ErrHandler(ErrHandler), HasError(false) {}
+      : GW(OS), Doc(Doc), ErrHandler(ErrHandler), SymbolID(0), HasError(false) {}
 
   ~GOFFState() { GW.finalize(); }
 
@@ -197,6 +206,7 @@ class GOFFState {
   GOFFOstream GW;
   GOFFYAML::Object &Doc;
   yaml::ErrorHandler ErrHandler;
+  uint16_t SymbolID;
   bool HasError;
 };
 
@@ -245,6 +255,90 @@ void GOFFState::writeHeader(GOFFYAML::FileHeader &FileHdr) {
   }
 }
 
+void GOFFState::writeSymbol(GOFFYAML::Symbol Sym) {
+  if (Sym.ID != SymbolID + 1)
+    reportError("symbol IDs not monotonic " + Sym.Name);
+  else
+    ++SymbolID;
+  if (Sym.OwnerID >= SymbolID)
+    reportError("owner ID not defined " + Sym.Name);
+  SmallString<80> SymName;
+  if (std::error_code EC = ConverterEBCDIC::convertToEBCDIC(Sym.Name, SymName))
+    reportError("conversion error on " + Sym.Name);
+  size_t SymLength = SymName.size();
+  if (SymLength > GOFF::MaxDataLength)
+    reportError("symbol name is too long: " + Twine(SymLength));
+
+  GW.makeNewRecord(GOFF::RT_ESD, 69 + SymLength);
+  GW << binaryBe(Sym.Type)          // Symbol type
+     << binaryBe(Sym.ID)            // ESDID
+     << binaryBe(Sym.OwnerID)       // Owner ESDID
+     << binaryBe(uint32_t(0))       // Reserved
+     << binaryBe(Sym.Address)       // Offset/Address
+     << binaryBe(uint32_t(0))       // Reserved
+     << binaryBe(Sym.Length)        // Length
+     << binaryBe(Sym.ExtAttrID)     // Extended attributes
+     << binaryBe(Sym.ExtAttrOffset) // Extended attributes data offset
+     << binaryBe(uint32_t(0))       // Reserved
+     << binaryBe(Sym.NameSpace)     // Namespace ID
+     << binaryBe(Sym.Flags)         // Flags
+     << binaryBe(Sym.FillByteValue) // Fill byte value
+     << binaryBe(uint8_t(0))        // Reserved
+     << binaryBe(Sym.PSectID)       // PSECT ID
+     << binaryBe(Sym.Priority);     // Priority
+  if (Sym.Signature)
+    GW << *Sym.Signature; // Signature
+  else
+    GW << zeros(8);
+#define BIT(E, N) (Sym.BAFlags & GOFF::E ? 1 << (7 - N) : 0)
+  GW << binaryBe(Sym.Amode) // Behavioral attributes - Amode
+     << binaryBe(Sym.Rmode) // Behavioral attributes - Rmode
+     << binaryBe(uint8_t(Sym.TextStyle << 4 | Sym.BindingAlgorithm))
+     << binaryBe(uint8_t(Sym.TaskingBehavior << 5 | BIT(ESD_BA_Movable, 3) |
+                         BIT(ESD_BA_ReadOnly, 4) | Sym.Executable))
+     << binaryBe(uint8_t(BIT(ESD_BA_NoPrime, 1) | Sym.BindingStrength))
+     << binaryBe(uint8_t(Sym.LoadingBehavior << 6 | BIT(ESD_BA_COMMON, 2) |
+                         BIT(ESD_BA_Indirect, 3) | Sym.BindingScope))
+     << binaryBe(uint8_t(Sym.LinkageType << 5 | Sym.Alignment))
+     << zeros(3) // Behavioral attributes - Reserved
+     << binaryBe(static_cast<uint16_t>(SymLength)) // Name length
+     << SymName.str();
+#undef BIT
+}
+
+void GOFFState::writeSection(GOFFYAML::Section Sec) {
+  if (Sec.SymbolID == 0 || Sec.SymbolID > SymbolID)
+    reportError("section symbol not defined: " + Twine(Sec.SymbolID));
+
+  size_t Size = 0;
+  if (Sec.Data) {
+    Size = Sec.Data->binary_size();
+    if (Size > GOFF::MaxDataLength) {
+      reportError("section content is too long: " + Twine(Size));
+      return;
+    }
+    if (Sec.DataLength && Sec.DataLength != Size) {
+      reportError("Section content length " + Twine(Size) +
+                  " does not match data length " + Twine(Sec.DataLength));
+      return;
+    }
+  } else
+    Size = Sec.DataLength;
+
+  GW.makeNewRecord(GOFF::RT_TXT, GOFF::PayloadLength - TXTMaxDataLength + Size);
+  GW << binaryBe(Sec.TextStyle)                // Text Record Style
+     << binaryBe(Sec.SymbolID)                 // Element ESDID
+     << binaryBe(uint32_t(0))                  // Reserved
+     << binaryBe(Sec.Offset)                   // Offset
+     << binaryBe(Sec.TrueLength)               // Text Field True Length
+     << binaryBe(Sec.TextEncoding)             // Text Encoding
+     << binaryBe(static_cast<uint16_t>(Size)); // Data Length
+  if (Sec.Data)
+    GW << *Sec.Data; // Data
+  else
+    GW << zeros(Size);
+}
+
 void GOFFState::writeEnd() {
   GW.makeNewRecord(GOFF::RT_END, GOFF::PayloadLength);
   GW << binaryBe(uint8_t(0)) // No entry point
@@ -259,6 +353,16 @@ bool GOFFState::writeObject() {
   writeHeader(Doc.Header);
   if (HasError)
     return false;
+  // Iterate over all records.
+  for (auto &Rec : Doc.Records) {
+    if (auto *Sec = dyn_cast<GOFFYAML::Section>(Rec.get())) {
+      writeSection(*Sec);
+    } else if (auto *Sym = dyn_cast<GOFFYAML::Symbol>(Rec.get())) {
+      writeSymbol(*Sym);
+    } else {
+      reportError("Unknown record type");
+    }
+  }
   writeEnd();
   return true;
 }
diff --git a/llvm/lib/ObjectYAML/GOFFYAML.cpp b/llvm/lib/ObjectYAML/GOFFYAML.cpp
index ae857980a521b0..4d10c8352219cf 100644
--- a/llvm/lib/ObjectYAML/GOFFYAML.cpp
+++ b/llvm/lib/ObjectYAML/GOFFYAML.cpp
@@ -23,6 +23,235 @@ Object::Object() {}
 
 namespace yaml {
 
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDSYMBOLTYPE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDSYMBOLTYPE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_ST_SectionDefinition);
+  ECase(ESD_ST_ElementDefinition);
+  ECase(ESD_ST_LabelDefinition);
+  ECase(ESD_ST_PartReference);
+  ECase(ESD_ST_ExternalReference);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDNAMESPACEID>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDNAMESPACEID &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_NS_ProgramManagementBinder);
+  ECase(ESD_NS_NormalName);
+  ECase(ESD_NS_PseudoRegister);
+  ECase(ESD_NS_Parts);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarBitSetTraits<GOFFYAML::GOFF_ESDFlags>::bitset(
+    IO &IO, GOFFYAML::GOFF_ESDFlags &Value) {
+#define BCase(X) IO.bitSetCase(Value, #X, GOFF::X)
+#define BCaseMask(X, M) IO.maskedBitSetCase(Value, #X, GOFF::X, GOFF::M)
+  BCase(ESD_FillByteValuePresent);
+  BCase(ESD_SymbolDisplayFlag);
+  BCase(ESD_SymbolRenamingFlag);
+  BCase(ESD_RemovableClass);
+  BCaseMask(ESD_RQ_0, ESD_Mask_RQW);
+  BCaseMask(ESD_RQ_1, ESD_Mask_RQW);
+  BCaseMask(ESD_RQ_2, ESD_Mask_RQW);
+  BCaseMask(ESD_RQ_3, ESD_Mask_RQW);
+#undef BCase
+#undef BCaseMask
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_TEXTRECORDSTYLE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_TEXTRECORDSTYLE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(TXT_RS_Byte);
+  ECase(TXT_RS_Structured);
+  ECase(TXT_RS_Unstructured);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDAMODE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDAMODE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_AMODE_None);
+  ECase(ESD_AMODE_24);
+  ECase(ESD_AMODE_31);
+  ECase(ESD_AMODE_ANY);
+  ECase(ESD_AMODE_64);
+  ECase(ESD_AMODE_MIN);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDRMODE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDRMODE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_RMODE_None);
+  ECase(ESD_RMODE_24);
+  ECase(ESD_RMODE_31);
+  ECase(ESD_RMODE_64);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDTEXTSTYLE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDTEXTSTYLE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_TS_ByteOriented);
+  ECase(ESD_TS_Structured);
+  ECase(ESD_TS_Unstructured);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDBINDINGALGORITHM>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDBINDINGALGORITHM &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_BA_Concatenate);
+  ECase(ESD_BA_Merge);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDTASKINGBEHAVIOR>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDTASKINGBEHAVIOR &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_TA_Unspecified);
+  ECase(ESD_TA_NonReus);
+  ECase(ESD_TA_Reus);
+  ECase(ESD_TA_Rent);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDEXECUTABLE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDEXECUTABLE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_EXE_Unspecified);
+  ECase(ESD_EXE_DATA);
+  ECase(ESD_EXE_CODE);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDLINKAGETYPE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDLINKAGETYPE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_LT_OS);
+  ECase(ESD_LT_XPLink);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDBINDINGSTRENGTH>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDBINDINGSTRENGTH &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_BST_Strong);
+  ECase(ESD_BST_Weak);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDLOADINGBEHAVIOR>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDLOADINGBEHAVIOR &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_LB_Initial);
+  ECase(ESD_LB_Deferred);
+  ECase(ESD_LB_NoLoad);
+  ECase(ESD_LB_Reserved);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDBINDINGSCOPE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDBINDINGSCOPE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_BSC_Unspecified);
+  ECase(ESD_BSC_Section);
+  ECase(ESD_BSC_Module);
+  ECase(ESD_BSC_Library);
+  ECase(ESD_BSC_ImportExport);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDALIGNMENT>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDALIGNMENT &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_ALIGN_Byte);
+  ECase(ESD_ALIGN_Halfword);
+  ECase(ESD_ALIGN_Fullword);
+  ECase(ESD_ALIGN_Doubleword);
+  ECase(ESD_ALIGN_Quadword);
+  ECase(ESD_ALIGN_32byte);
+  ECase(ESD_ALIGN_64byte);
+  ECase(ESD_ALIGN_128byte);
+  ECase(ESD_ALIGN_256byte);
+  ECase(ESD_ALIGN_512byte);
+  ECase(ESD_ALIGN_1024byte);
+  ECase(ESD_ALIGN_2Kpage);
+  ECase(ESD_ALIGN_4Kpage);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarBitSetTraits<GOFFYAML::GOFF_BAFLAGS>::bitset(
+    IO &IO, GOFFYAML::GOFF_BAFLAGS &Value) {
+#define BCase(X) IO.bitSetCase(Value, #X, GOFF::X)
+#define BCaseMask(X, M) IO.maskedBitSetCase(Value, #X, GOFF::X, GOFF::M)
+  BCase(ESD_BA_Movable);
+  BCase(ESD_BA_ReadOnly);
+  BCase(ESD_BA_NoPrime);
+  BCase(ESD_BA_COMMON);
+  BCase(ESD_BA_Indirect);
+#undef BCase
+#undef BCaseMask
+}
+
+void MappingTraits<GOFFYAML::Section>::mapping(IO &IO, GOFFYAML::Section &Sec) {
+  IO.mapRequired("SymbolName", Sec.SymbolName);
+  IO.mapRequired("SymbolID", Sec.SymbolID);
+  IO.mapOptional("Offset", Sec.Offset, 0);
+  IO.mapOptional("TrueLength", Sec.TrueLength, 0);
+  IO.mapOptional("TextEncoding", Sec.TextEncoding, 0);
+  IO.mapOptional("DataLength", Sec.DataLength, 0);
+  IO.mapOptional("TextStyle", Sec.TextStyle, GOFF::TXT_RS_Byte);
+  IO.mapOptional("Data", Sec.Data);
+}
+
+void MappingTraits<GOFFYAML::Symbol>::mapping(IO &IO, GOFFYAML::Symbol &Sym) {
+  IO.mapRequired("Name", Sym.Name);
+  IO.mapRequired("Type", Sym.Type);
+  IO.mapRequired("ID", Sym.ID);
+  IO.mapOptional("OwnerID", Sym.OwnerID, 0);
+  IO.mapOptional("Address", Sym.Address, 0);
+  IO.mapOptional("Length", Sym.Length, 0);
+  IO.mapOptional("ExtAttrID", Sym.ExtAttrID, 0);
+  IO.mapOptional("ExtAttrOffset", Sym.ExtAttrOffset, 0);
+  IO.mapRequired("NameSpace", Sym.NameSpace);
+  IO.mapOptional("Flags", Sym.Flags, GOFFYAML::GOFF_ESDFlags(0));
+  IO.mapOptional("FillByteValue", Sym.FillByteValue, 0);
+  IO.mapOptional("PSectID", Sym.PSectID, 0);
+  IO.mapOptional("Priority", Sym.Priority, 0);
+  IO.mapOptional("Signature", Sym.Signature);
+  IO.mapOptional("Amode", Sym.Amode, GOFF::ESD_AMODE_None);
+  IO.mapOptional("Rmode", Sym.Rmode, GOFF::ESD_RMODE_None);
+  IO.mapOptional("TextStyle", Sym.TextStyle, GOFF::ESD_TS_ByteOriented);
+  IO.mapOptional("BindingAlgorithm", Sym.BindingAlgorithm,
+                 GOFF::ESD_BA_Concatenate);
+  IO.mapOptional("TaskingBehavior", Sym.TaskingBehavior,
+                 GOFF::ESD_TA_Unspecified);
+  IO.mapOptional("Executable", Sym.Executable, GOFF::ESD_EXE_Unspecified);
+  IO.mapOptional("LinkageType", Sym.LinkageType, GOFF::ESD_LT_OS);
+  IO.mapOptional("BindingStrength", Sym.BindingStrength, GOFF::ESD_BST_Strong);
+  IO.mapOptional("LoadingBehavior", Sym.LoadingBehavior, GOFF::ESD_LB_Initial);
+  IO.mapOptional("BindingScope", Sym.BindingScope, GOFF::ESD_BSC_Unspecified);
+  IO.mapOptional("Alignment", Sym.Alignment, GOFF::ESD_ALIGN_Byte);
+  IO.mapOptional("BAFlags", Sym.BAFlags, 0);
+}
+
 void MappingTraits<GOFFYAML::FileHeader>::mapping(
     IO &IO, GOFFYAML::FileHeader &FileHdr) {
   IO.mapOptional("TargetEnvironment", FileHdr.TargetEnvironment, 0);
@@ -37,9 +266,34 @@ void MappingTraits<GOFFYAML::FileHeader>::mapping(
                  FileHdr.TargetSoftwareEnvironment);
 }
 
+void Cus...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 19, 2023

@llvm/pr-subscribers-llvm-binary-utilities

Author: Yusra Syeda (ysyeda)

Changes

This PR implements part 2 of yaml2obj for the GOFF Object File Format. It adds support for the sections and symbols.


Patch is 32.36 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75971.diff

10 Files Affected:

  • (modified) llvm/include/llvm/BinaryFormat/GOFF.h (+6)
  • (modified) llvm/include/llvm/ObjectYAML/GOFFYAML.h (+143)
  • (modified) llvm/lib/ObjectYAML/GOFFEmitter.cpp (+105-1)
  • (modified) llvm/lib/ObjectYAML/GOFFYAML.cpp (+254)
  • (added) llvm/test/tools/yaml2obj/GOFF/GOFF-TXTSection.yaml (+49)
  • (added) llvm/test/tools/yaml2obj/GOFF/GOFF-basic.yaml (+59)
  • (modified) llvm/test/tools/yaml2obj/GOFF/GOFF-header-end.yaml (+1)
  • (modified) llvm/test/tools/yaml2obj/GOFF/GOFF-header-settings.yaml (+2-1)
  • (added) llvm/test/tools/yaml2obj/GOFF/GOFF-longSymbol.yaml (+91)
  • (added) llvm/test/tools/yaml2obj/GOFF/GOFF-oneSymb.yaml (+40)
diff --git a/llvm/include/llvm/BinaryFormat/GOFF.h b/llvm/include/llvm/BinaryFormat/GOFF.h
index 443bcfc9479a8b..be6850216dbe2a 100644
--- a/llvm/include/llvm/BinaryFormat/GOFF.h
+++ b/llvm/include/llvm/BinaryFormat/GOFF.h
@@ -157,6 +157,12 @@ enum ESDAlignment : uint8_t {
   ESD_ALIGN_4Kpage = 12,
 };
 
+enum TXTRecordStyle : uint8_t {
+  TXT_RS_Byte = 0,
+  TXT_RS_Structured = 1,
+  TXT_RS_Unstructured = 2,
+};
+
 enum ENDEntryPointRequest : uint8_t {
   END_EPR_None = 0,
   END_EPR_EsdidOffset = 1,
diff --git a/llvm/include/llvm/ObjectYAML/GOFFYAML.h b/llvm/include/llvm/ObjectYAML/GOFFYAML.h
index f9bf45e95bd3a4..c17b4b605e258e 100644
--- a/llvm/include/llvm/ObjectYAML/GOFFYAML.h
+++ b/llvm/include/llvm/ObjectYAML/GOFFYAML.h
@@ -21,10 +21,120 @@
 
 namespace llvm {
 
+namespace GOFF {
+
+enum ESDFlags {
+  ESD_FillByteValuePresent = 1 << 7,
+  ESD_SymbolDisplayFlag = 1 << 6,
+  ESD_SymbolRenamingFlag = 1 << 5,
+  ESD_RemovableClass = 1 << 4
+};
+
+enum {
+  ESD_Mask_ERST = 0x07,
+  ESD_Mask_RQW = 0x07,
+  ESD_Mask_TextStyle = 0xf0,
+  ESD_Mask_BindingAlgorithm = 0x0f,
+};
+
+enum ESDBAFlags {
+  ESD_BA_Movable = 0x01,
+  ESD_BA_ReadOnly = 0x2,
+  ESD_BA_NoPrime = 0x4,
+  ESD_BA_COMMON = 0x8,
+  ESD_BA_Indirect = 0x10,
+};
+} // end namespace GOFF
+
 // The structure of the yaml files is not an exact 1:1 match to GOFF. In order
 // to use yaml::IO, we use these structures which are closer to the source.
 namespace GOFFYAML {
 
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDSYMBOLTYPE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDNAMESPACEID)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDFlags)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDAMODE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDRMODE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDTEXTSTYLE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDBINDINGALGORITHM)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDTASKINGBEHAVIOR)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDEXECUTABLE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDLINKAGETYPE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDBINDINGSTRENGTH)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDLOADINGBEHAVIOR)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDBINDINGSCOPE)
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_ESDALIGNMENT)
+LLVM_YAML_STRONG_TYPEDEF(uint64_t, GOFF_BAFLAGS)
+
+LLVM_YAML_STRONG_TYPEDEF(uint8_t, GOFF_TEXTRECORDSTYLE)
+
+struct RecordBase {
+  enum RecordBaseKind { RBK_Symbol, RBK_Section };
+
+private:
+  const RecordBaseKind Kind;
+
+protected:
+  RecordBase(RecordBaseKind Kind) : Kind(Kind) {}
+
+public:
+  RecordBaseKind getKind() const { return Kind; }
+};
+typedef std::unique_ptr<RecordBase> RecordPtr;
+
+struct Section : public RecordBase {
+  Section() : RecordBase(RBK_Section) {}
+
+  StringRef SymbolName;
+  uint32_t SymbolID;
+  uint32_t Offset;
+  uint32_t TrueLength;
+  uint16_t TextEncoding;
+  uint16_t DataLength;
+  GOFF_TEXTRECORDSTYLE TextStyle;
+
+  std::optional<yaml::BinaryRef> Data;
+
+  static bool classof(const RecordBase *Rec) {
+    return Rec->getKind() == RBK_Section;
+  }
+};
+
+struct Symbol : public RecordBase {
+  Symbol() : RecordBase(RBK_Symbol) {}
+
+  StringRef Name;
+  GOFF_ESDSYMBOLTYPE Type;
+  uint32_t ID;
+  uint32_t OwnerID;
+  uint32_t Address;
+  uint32_t Length;
+  uint32_t ExtAttrID;
+  uint32_t ExtAttrOffset;
+  GOFF_ESDNAMESPACEID NameSpace;
+  GOFF_ESDFlags Flags;
+  uint8_t FillByteValue;
+  uint32_t PSectID;
+  uint32_t Priority;
+  std::optional<llvm::yaml::Hex64> Signature;
+  GOFF_ESDAMODE Amode;
+  GOFF_ESDRMODE Rmode;
+  GOFF_ESDTEXTSTYLE TextStyle;
+  GOFF_ESDBINDINGALGORITHM BindingAlgorithm;
+  GOFF_ESDTASKINGBEHAVIOR TaskingBehavior;
+  GOFF_ESDEXECUTABLE Executable;
+  GOFF_ESDLINKAGETYPE LinkageType;
+  GOFF_ESDBINDINGSTRENGTH BindingStrength;
+  GOFF_ESDLOADINGBEHAVIOR LoadingBehavior;
+  GOFF_ESDBINDINGSCOPE BindingScope;
+  GOFF_ESDALIGNMENT Alignment;
+  GOFF_BAFLAGS BAFlags;
+
+  static bool classof(const RecordBase *Rec) {
+    return Rec->getKind() == RBK_Symbol;
+  }
+};
+
 struct FileHeader {
   uint32_t TargetEnvironment = 0;
   uint32_t TargetOperatingSystem = 0;
@@ -38,12 +148,45 @@ struct FileHeader {
 
 struct Object {
   FileHeader Header;
+  std::vector<RecordPtr> Records;
   Object();
 };
 } // end namespace GOFFYAML
 } // end namespace llvm
 
+LLVM_YAML_IS_SEQUENCE_VECTOR(GOFFYAML::RecordPtr)
+
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDSYMBOLTYPE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDNAMESPACEID)
+LLVM_YAML_DECLARE_BITSET_TRAITS(GOFFYAML::GOFF_ESDFlags)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDAMODE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDRMODE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDTEXTSTYLE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDBINDINGALGORITHM)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDTASKINGBEHAVIOR)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDEXECUTABLE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDLINKAGETYPE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDBINDINGSTRENGTH)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDLOADINGBEHAVIOR)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDBINDINGSCOPE)
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_ESDALIGNMENT)
+LLVM_YAML_DECLARE_BITSET_TRAITS(GOFFYAML::GOFF_BAFLAGS)
+
+LLVM_YAML_DECLARE_ENUM_TRAITS(GOFFYAML::GOFF_TEXTRECORDSTYLE)
+
+LLVM_YAML_DECLARE_MAPPING_TRAITS(GOFFYAML::Section)
+LLVM_YAML_DECLARE_MAPPING_TRAITS(GOFFYAML::Symbol)
 LLVM_YAML_DECLARE_MAPPING_TRAITS(GOFFYAML::FileHeader)
 LLVM_YAML_DECLARE_MAPPING_TRAITS(GOFFYAML::Object)
 
+namespace llvm {
+namespace yaml {
+
+template <> struct CustomMappingTraits<GOFFYAML::RecordPtr> {
+  static void inputOne(IO &IO, StringRef Key, GOFFYAML::RecordPtr &Elem);
+  static void output(IO &IO, GOFFYAML::RecordPtr &Elem);
+};
+
+} // end namespace yaml
+} // end namespace llvm
 #endif // LLVM_OBJECTYAML_GOFFYAML_H
diff --git a/llvm/lib/ObjectYAML/GOFFEmitter.cpp b/llvm/lib/ObjectYAML/GOFFEmitter.cpp
index 345904407e1d24..e7db0736993444 100644
--- a/llvm/lib/ObjectYAML/GOFFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/GOFFEmitter.cpp
@@ -22,6 +22,8 @@ using namespace llvm;
 
 namespace {
 
+static const uint8_t TXTMaxDataLength = 56;
+
 // Common flag values on records.
 enum {
   // Flag: This record is continued.
@@ -60,6 +62,11 @@ raw_ostream &operator<<(raw_ostream &OS, const ZerosImpl &Z) {
 
 ZerosImpl zeros(const size_t NumBytes) { return ZerosImpl{NumBytes}; }
 
+raw_ostream &operator<<(raw_ostream &OS, const yaml::BinaryRef &Data) {
+  Data.writeAsBinary(OS);
+  return OS;
+}
+
 // The GOFFOstream is responsible to write the data into the fixed physical
 // records of the format. A user of this class announces the start of a new
 // logical record and the size of its payload. While writing the payload, the
@@ -175,6 +182,8 @@ class GOFFOstream : public raw_ostream {
 class GOFFState {
   void writeHeader(GOFFYAML::FileHeader &FileHdr);
   void writeEnd();
+  void writeSymbol(GOFFYAML::Symbol Sym);
+  void writeSection(GOFFYAML::Section Sec);
 
   void reportError(const Twine &Msg) {
     ErrHandler(Msg);
@@ -183,7 +192,7 @@ class GOFFState {
 
   GOFFState(raw_ostream &OS, GOFFYAML::Object &Doc,
             yaml::ErrorHandler ErrHandler)
-      : GW(OS), Doc(Doc), ErrHandler(ErrHandler), HasError(false) {}
+      : GW(OS), Doc(Doc), ErrHandler(ErrHandler), SymbolID(0), HasError(false) {}
 
   ~GOFFState() { GW.finalize(); }
 
@@ -197,6 +206,7 @@ class GOFFState {
   GOFFOstream GW;
   GOFFYAML::Object &Doc;
   yaml::ErrorHandler ErrHandler;
+  uint16_t SymbolID;
   bool HasError;
 };
 
@@ -245,6 +255,90 @@ void GOFFState::writeHeader(GOFFYAML::FileHeader &FileHdr) {
   }
 }
 
+void GOFFState::writeSymbol(GOFFYAML::Symbol Sym) {
+  if (Sym.ID != SymbolID + 1)
+    reportError("symbol IDs not monotonic " + Sym.Name);
+  else
+    ++SymbolID;
+  if (Sym.OwnerID >= SymbolID)
+    reportError("owner ID not defined " + Sym.Name);
+  SmallString<80> SymName;
+  if (std::error_code EC = ConverterEBCDIC::convertToEBCDIC(Sym.Name, SymName))
+    reportError("conversion error on " + Sym.Name);
+  size_t SymLength = SymName.size();
+  if (SymLength > GOFF::MaxDataLength)
+    reportError("symbol name is too long: " + Twine(SymLength));
+
+  GW.makeNewRecord(GOFF::RT_ESD, 69 + SymLength);
+  GW << binaryBe(Sym.Type)          // Symbol type
+     << binaryBe(Sym.ID)            // ESDID
+     << binaryBe(Sym.OwnerID)       // Owner ESDID
+     << binaryBe(uint32_t(0))       // Reserved
+     << binaryBe(Sym.Address)       // Offset/Address
+     << binaryBe(uint32_t(0))       // Reserved
+     << binaryBe(Sym.Length)        // Length
+     << binaryBe(Sym.ExtAttrID)     // Extended attributes
+     << binaryBe(Sym.ExtAttrOffset) // Extended attributes data offset
+     << binaryBe(uint32_t(0))       // Reserved
+     << binaryBe(Sym.NameSpace)     // Namespace ID
+     << binaryBe(Sym.Flags)         // Flags
+     << binaryBe(Sym.FillByteValue) // Fill byte value
+     << binaryBe(uint8_t(0))        // Reserved
+     << binaryBe(Sym.PSectID)       // PSECT ID
+     << binaryBe(Sym.Priority);     // Priority
+  if (Sym.Signature)
+    GW << *Sym.Signature; // Signature
+  else
+    GW << zeros(8);
+#define BIT(E, N) (Sym.BAFlags & GOFF::E ? 1 << (7 - N) : 0)
+  GW << binaryBe(Sym.Amode) // Behavioral attributes - Amode
+     << binaryBe(Sym.Rmode) // Behavioral attributes - Rmode
+     << binaryBe(uint8_t(Sym.TextStyle << 4 | Sym.BindingAlgorithm))
+     << binaryBe(uint8_t(Sym.TaskingBehavior << 5 | BIT(ESD_BA_Movable, 3) |
+                         BIT(ESD_BA_ReadOnly, 4) | Sym.Executable))
+     << binaryBe(uint8_t(BIT(ESD_BA_NoPrime, 1) | Sym.BindingStrength))
+     << binaryBe(uint8_t(Sym.LoadingBehavior << 6 | BIT(ESD_BA_COMMON, 2) |
+                         BIT(ESD_BA_Indirect, 3) | Sym.BindingScope))
+     << binaryBe(uint8_t(Sym.LinkageType << 5 | Sym.Alignment))
+     << zeros(3) // Behavioral attributes - Reserved
+     << binaryBe(static_cast<uint16_t>(SymLength)) // Name length
+     << SymName.str();
+#undef BIT
+}
+
+void GOFFState::writeSection(GOFFYAML::Section Sec) {
+  if (Sec.SymbolID == 0 || Sec.SymbolID > SymbolID)
+    reportError("section symbol not defined: " + Twine(Sec.SymbolID));
+
+  size_t Size = 0;
+  if (Sec.Data) {
+    Size = Sec.Data->binary_size();
+    if (Size > GOFF::MaxDataLength) {
+      reportError("section content is too long: " + Twine(Size));
+      return;
+    }
+    if (Sec.DataLength && Sec.DataLength != Size) {
+      reportError("Section content length " + Twine(Size) +
+                  " does not match data length " + Twine(Sec.DataLength));
+      return;
+    }
+  } else
+    Size = Sec.DataLength;
+
+  GW.makeNewRecord(GOFF::RT_TXT, GOFF::PayloadLength - TXTMaxDataLength + Size);
+  GW << binaryBe(Sec.TextStyle)                // Text Record Style
+     << binaryBe(Sec.SymbolID)                 // Element ESDID
+     << binaryBe(uint32_t(0))                  // Reserved
+     << binaryBe(Sec.Offset)                   // Offset
+     << binaryBe(Sec.TrueLength)               // Text Field True Length
+     << binaryBe(Sec.TextEncoding)             // Text Encoding
+     << binaryBe(static_cast<uint16_t>(Size)); // Data Length
+  if (Sec.Data)
+    GW << *Sec.Data; // Data
+  else
+    GW << zeros(Size);
+}
+
 void GOFFState::writeEnd() {
   GW.makeNewRecord(GOFF::RT_END, GOFF::PayloadLength);
   GW << binaryBe(uint8_t(0)) // No entry point
@@ -259,6 +353,16 @@ bool GOFFState::writeObject() {
   writeHeader(Doc.Header);
   if (HasError)
     return false;
+  // Iterate over all records.
+  for (auto &Rec : Doc.Records) {
+    if (auto *Sec = dyn_cast<GOFFYAML::Section>(Rec.get())) {
+      writeSection(*Sec);
+    } else if (auto *Sym = dyn_cast<GOFFYAML::Symbol>(Rec.get())) {
+      writeSymbol(*Sym);
+    } else {
+      reportError("Unknown record type");
+    }
+  }
   writeEnd();
   return true;
 }
diff --git a/llvm/lib/ObjectYAML/GOFFYAML.cpp b/llvm/lib/ObjectYAML/GOFFYAML.cpp
index ae857980a521b0..4d10c8352219cf 100644
--- a/llvm/lib/ObjectYAML/GOFFYAML.cpp
+++ b/llvm/lib/ObjectYAML/GOFFYAML.cpp
@@ -23,6 +23,235 @@ Object::Object() {}
 
 namespace yaml {
 
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDSYMBOLTYPE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDSYMBOLTYPE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_ST_SectionDefinition);
+  ECase(ESD_ST_ElementDefinition);
+  ECase(ESD_ST_LabelDefinition);
+  ECase(ESD_ST_PartReference);
+  ECase(ESD_ST_ExternalReference);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDNAMESPACEID>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDNAMESPACEID &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_NS_ProgramManagementBinder);
+  ECase(ESD_NS_NormalName);
+  ECase(ESD_NS_PseudoRegister);
+  ECase(ESD_NS_Parts);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarBitSetTraits<GOFFYAML::GOFF_ESDFlags>::bitset(
+    IO &IO, GOFFYAML::GOFF_ESDFlags &Value) {
+#define BCase(X) IO.bitSetCase(Value, #X, GOFF::X)
+#define BCaseMask(X, M) IO.maskedBitSetCase(Value, #X, GOFF::X, GOFF::M)
+  BCase(ESD_FillByteValuePresent);
+  BCase(ESD_SymbolDisplayFlag);
+  BCase(ESD_SymbolRenamingFlag);
+  BCase(ESD_RemovableClass);
+  BCaseMask(ESD_RQ_0, ESD_Mask_RQW);
+  BCaseMask(ESD_RQ_1, ESD_Mask_RQW);
+  BCaseMask(ESD_RQ_2, ESD_Mask_RQW);
+  BCaseMask(ESD_RQ_3, ESD_Mask_RQW);
+#undef BCase
+#undef BCaseMask
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_TEXTRECORDSTYLE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_TEXTRECORDSTYLE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(TXT_RS_Byte);
+  ECase(TXT_RS_Structured);
+  ECase(TXT_RS_Unstructured);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDAMODE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDAMODE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_AMODE_None);
+  ECase(ESD_AMODE_24);
+  ECase(ESD_AMODE_31);
+  ECase(ESD_AMODE_ANY);
+  ECase(ESD_AMODE_64);
+  ECase(ESD_AMODE_MIN);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDRMODE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDRMODE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_RMODE_None);
+  ECase(ESD_RMODE_24);
+  ECase(ESD_RMODE_31);
+  ECase(ESD_RMODE_64);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDTEXTSTYLE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDTEXTSTYLE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_TS_ByteOriented);
+  ECase(ESD_TS_Structured);
+  ECase(ESD_TS_Unstructured);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDBINDINGALGORITHM>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDBINDINGALGORITHM &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_BA_Concatenate);
+  ECase(ESD_BA_Merge);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDTASKINGBEHAVIOR>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDTASKINGBEHAVIOR &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_TA_Unspecified);
+  ECase(ESD_TA_NonReus);
+  ECase(ESD_TA_Reus);
+  ECase(ESD_TA_Rent);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDEXECUTABLE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDEXECUTABLE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_EXE_Unspecified);
+  ECase(ESD_EXE_DATA);
+  ECase(ESD_EXE_CODE);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDLINKAGETYPE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDLINKAGETYPE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_LT_OS);
+  ECase(ESD_LT_XPLink);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDBINDINGSTRENGTH>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDBINDINGSTRENGTH &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_BST_Strong);
+  ECase(ESD_BST_Weak);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDLOADINGBEHAVIOR>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDLOADINGBEHAVIOR &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_LB_Initial);
+  ECase(ESD_LB_Deferred);
+  ECase(ESD_LB_NoLoad);
+  ECase(ESD_LB_Reserved);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDBINDINGSCOPE>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDBINDINGSCOPE &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_BSC_Unspecified);
+  ECase(ESD_BSC_Section);
+  ECase(ESD_BSC_Module);
+  ECase(ESD_BSC_Library);
+  ECase(ESD_BSC_ImportExport);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarEnumerationTraits<GOFFYAML::GOFF_ESDALIGNMENT>::enumeration(
+    IO &IO, GOFFYAML::GOFF_ESDALIGNMENT &Value) {
+#define ECase(X) IO.enumCase(Value, #X, GOFF::X)
+  ECase(ESD_ALIGN_Byte);
+  ECase(ESD_ALIGN_Halfword);
+  ECase(ESD_ALIGN_Fullword);
+  ECase(ESD_ALIGN_Doubleword);
+  ECase(ESD_ALIGN_Quadword);
+  ECase(ESD_ALIGN_32byte);
+  ECase(ESD_ALIGN_64byte);
+  ECase(ESD_ALIGN_128byte);
+  ECase(ESD_ALIGN_256byte);
+  ECase(ESD_ALIGN_512byte);
+  ECase(ESD_ALIGN_1024byte);
+  ECase(ESD_ALIGN_2Kpage);
+  ECase(ESD_ALIGN_4Kpage);
+#undef ECase
+  IO.enumFallback<Hex8>(Value);
+}
+
+void ScalarBitSetTraits<GOFFYAML::GOFF_BAFLAGS>::bitset(
+    IO &IO, GOFFYAML::GOFF_BAFLAGS &Value) {
+#define BCase(X) IO.bitSetCase(Value, #X, GOFF::X)
+#define BCaseMask(X, M) IO.maskedBitSetCase(Value, #X, GOFF::X, GOFF::M)
+  BCase(ESD_BA_Movable);
+  BCase(ESD_BA_ReadOnly);
+  BCase(ESD_BA_NoPrime);
+  BCase(ESD_BA_COMMON);
+  BCase(ESD_BA_Indirect);
+#undef BCase
+#undef BCaseMask
+}
+
+void MappingTraits<GOFFYAML::Section>::mapping(IO &IO, GOFFYAML::Section &Sec) {
+  IO.mapRequired("SymbolName", Sec.SymbolName);
+  IO.mapRequired("SymbolID", Sec.SymbolID);
+  IO.mapOptional("Offset", Sec.Offset, 0);
+  IO.mapOptional("TrueLength", Sec.TrueLength, 0);
+  IO.mapOptional("TextEncoding", Sec.TextEncoding, 0);
+  IO.mapOptional("DataLength", Sec.DataLength, 0);
+  IO.mapOptional("TextStyle", Sec.TextStyle, GOFF::TXT_RS_Byte);
+  IO.mapOptional("Data", Sec.Data);
+}
+
+void MappingTraits<GOFFYAML::Symbol>::mapping(IO &IO, GOFFYAML::Symbol &Sym) {
+  IO.mapRequired("Name", Sym.Name);
+  IO.mapRequired("Type", Sym.Type);
+  IO.mapRequired("ID", Sym.ID);
+  IO.mapOptional("OwnerID", Sym.OwnerID, 0);
+  IO.mapOptional("Address", Sym.Address, 0);
+  IO.mapOptional("Length", Sym.Length, 0);
+  IO.mapOptional("ExtAttrID", Sym.ExtAttrID, 0);
+  IO.mapOptional("ExtAttrOffset", Sym.ExtAttrOffset, 0);
+  IO.mapRequired("NameSpace", Sym.NameSpace);
+  IO.mapOptional("Flags", Sym.Flags, GOFFYAML::GOFF_ESDFlags(0));
+  IO.mapOptional("FillByteValue", Sym.FillByteValue, 0);
+  IO.mapOptional("PSectID", Sym.PSectID, 0);
+  IO.mapOptional("Priority", Sym.Priority, 0);
+  IO.mapOptional("Signature", Sym.Signature);
+  IO.mapOptional("Amode", Sym.Amode, GOFF::ESD_AMODE_None);
+  IO.mapOptional("Rmode", Sym.Rmode, GOFF::ESD_RMODE_None);
+  IO.mapOptional("TextStyle", Sym.TextStyle, GOFF::ESD_TS_ByteOriented);
+  IO.mapOptional("BindingAlgorithm", Sym.BindingAlgorithm,
+                 GOFF::ESD_BA_Concatenate);
+  IO.mapOptional("TaskingBehavior", Sym.TaskingBehavior,
+                 GOFF::ESD_TA_Unspecified);
+  IO.mapOptional("Executable", Sym.Executable, GOFF::ESD_EXE_Unspecified);
+  IO.mapOptional("LinkageType", Sym.LinkageType, GOFF::ESD_LT_OS);
+  IO.mapOptional("BindingStrength", Sym.BindingStrength, GOFF::ESD_BST_Strong);
+  IO.mapOptional("LoadingBehavior", Sym.LoadingBehavior, GOFF::ESD_LB_Initial);
+  IO.mapOptional("BindingScope", Sym.BindingScope, GOFF::ESD_BSC_Unspecified);
+  IO.mapOptional("Alignment", Sym.Alignment, GOFF::ESD_ALIGN_Byte);
+  IO.mapOptional("BAFlags", Sym.BAFlags, 0);
+}
+
 void MappingTraits<GOFFYAML::FileHeader>::mapping(
     IO &IO, GOFFYAML::FileHeader &FileHdr) {
   IO.mapOptional("TargetEnvironment", FileHdr.TargetEnvironment, 0);
@@ -37,9 +266,34 @@ void MappingTraits<GOFFYAML::FileHeader>::mapping(
                  FileHdr.TargetSoftwareEnvironment);
 }
 
+void Cus...
[truncated]

@jh7370
Copy link
Collaborator

jh7370 commented Dec 20, 2023

I'm not going to have time to review this until the new year, I'm afraid. Is it possible to split this into separate PRs, one for sections and one for symbols? If there's more than one variety of section, those could be separate PRs too.

@ysyeda ysyeda changed the title [SystemZ][z/OS] yaml2obj GOFF sections & symbols [SystemZ][z/OS] yaml2obj GOFF symbols Jan 2, 2024
@ysyeda
Copy link
Contributor Author

ysyeda commented Jan 2, 2024

I'm not going to have time to review this until the new year, I'm afraid. Is it possible to split this into separate PRs, one for sections and one for symbols? If there's more than one variety of section, those could be separate PRs too.

@jh7370 I split this PR to just add the functionality for symbols. I'll create another PR for sections after this one is merged.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to revisit you're tests somewhat. In particular, it would be clearest if you had one test case where all symbol fields are set and another where none of the optional ones are. You should also have:

  • tests for all error cases
  • tests for each enum field showing that you can use raw integer values

llvm/include/llvm/ObjectYAML/GOFFYAML.h Show resolved Hide resolved
llvm/include/llvm/ObjectYAML/GOFFYAML.h Outdated Show resolved Hide resolved
llvm/include/llvm/ObjectYAML/GOFFYAML.h Outdated Show resolved Hide resolved
@@ -245,6 +248,57 @@ void GOFFState::writeHeader(GOFFYAML::FileHeader &FileHdr) {
}
}

void GOFFState::writeSymbol(GOFFYAML::Symbol Sym) {
if (Sym.ID != SymbolID + 1)
reportError("symbol IDs not monotonic " + Sym.Name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I raised this point before: in general, yaml2obj should be permissive in what it accepts as input, as one of the main purposes of yaml2obj is to craft inputs for tests. To test edge cases in file format handling, you need to be able to craft inputs that don't follow the rules of the file format. In other words, if you don't need symbol IDs (in this case) to be monotonic to be able to physically write things, don't enforce it.

else
++SymbolID;
if (Sym.OwnerID >= SymbolID)
reportError("owner ID not defined " + Sym.Name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above: is this absolutely necessary? Could you (in a malformed input object) have an owner ID field that isn't valid?

llvm/test/tools/yaml2obj/GOFF/GOFF-longSymbol.yaml Outdated Show resolved Hide resolved
llvm/test/tools/yaml2obj/GOFF/GOFF-longSymbol.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,40 @@
# RUN: yaml2obj %s | od -v -An -tx1 | FileCheck --ignore-case %s

# Verify that GOFF Header is correct.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd focus on the parts of the header that might vary, rather than testing the whole thing.

@@ -0,0 +1,40 @@
# RUN: yaml2obj %s | od -v -An -tx1 | FileCheck --ignore-case %s
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test name should use this-name-style.yaml. Also, don't abbreviate "symbol" unnecessarily.

Why is one symbol an interesting case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short Answer:
The way a symbol is handled differs slightly depending on the name's length.

Long Answer:
(Might need a bit of background info to explain this in detail...)

GOFF is made up of 80-byte physical records. Both symbols (represented by a type of record called the ESD Record) and sections (represented by a tuple of ESD Records + another type of record, the TXT record) can span multiple physical records, depending on the length of their names (in the case of symbols), or their contents (in the case of a section). The physical records that represent the same symbol/section collectively make up a "logical record".

This testcase tests a symbol that does not span multiple physical records, as opposed to GOFF-longSymbol.yaml, which tests symbols that do span multiple physical records.

Maybe the best way to handle this is that the record in this yaml file could just be copied to GOFF-longSymbol.yaml, that file could have its name changed to suggest that we are testing the proper emitting of symbols of varying name lengths, and then this file could be deleted?

llvm/test/tools/yaml2obj/GOFF/GOFF-oneSymb.yaml Outdated Show resolved Hide resolved
@jh7370
Copy link
Collaborator

jh7370 commented Mar 21, 2024

@ysyeda, do you believe you have addressed all of my points? I see at least one that haven't been addressed.

@jh7370
Copy link
Collaborator

jh7370 commented Apr 9, 2024

@ysyeda, is this ready to review now? Please post to say it is, as I don't have time to go through and check whether you've addressed all my points or to guess that you think you have!

@ysyeda
Copy link
Contributor Author

ysyeda commented Apr 9, 2024

@jh7370 This should be ready for review. I believe your comments should all be addressed.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've been quite busy. I've done a quick skim of the latest update so that you can address these points. It's likely I'll have more at a later date. I'd prefer it if you push all your changes at once, rather than drip-feed them as you do them, as it makes it clearer that you've addressed everything.

llvm/lib/ObjectYAML/GOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/GOFFEmitter.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/GOFFYAML.cpp Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/GOFFEmitter.cpp Outdated Show resolved Hide resolved
@ysyeda
Copy link
Contributor Author

ysyeda commented Apr 16, 2024

Sorry, I've been quite busy. I've done a quick skim of the latest update so that you can address these points. It's likely I'll have more at a later date. I'd prefer it if you push all your changes at once, rather than drip-feed them as you do them, as it makes it clearer that you've addressed everything.

Thanks, I pushed all my changes to address your new set of comments.

@@ -308,11 +308,14 @@ bool GOFFState::writeObject() {
if (HasError)
return false;
// Iterate over all records.
for (const std::unique_ptr<llvm::GOFFYAML::RecordBase> &Rec : Doc.Records)
unsigned RecordNum = 0;
for (const std::unique_ptr<llvm::GOFFYAML::RecordBase> &Rec : Doc.Records) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should look up the enumerate function to allow iterating over the range whilst also producing an index.

if (const auto *Sym = dyn_cast<GOFFYAML::Symbol>(Rec.get()))
writeSymbol(*Sym);
else
reportError("unknown record type");
reportError("unknown record type on record index" + Twine(RecordNum));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reportError("unknown record type on record index" + Twine(RecordNum));
reportError("record with index " + Twine(RecordNum) + " has unknown record type " + Twine(Rec.Type));

This is clearer, I think. Rec.Type may not be the right thing - I just used this as a placeholder so that I didn't have to look up the actual member that is used to determine what kind of Record it is.

You should also have a test case for this error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is running in the wrong direction. The if is over the discriminator used for the inheritance hierarchy. There is no user input involved. The error message made no sense in the first place. The whole loop should be:

  for (const auto &Rec: Doc.Records) {
    GOFFYAML::RecordBase *RecBase = Rec.get();
    switch (RecBase->getKind()) {
      case GOFFYAML::RecordBase::RBK_Symbol:
        writeSymbol(*static_cast<GOFFYAML::Symbol *>(RecBase));
    }
  }

There is no default case because the switch covers all cases. I put this into the updated PR.

@@ -1,5 +1,7 @@
# RUN: yaml2obj %s | od -v -An -tx1 | FileCheck --ignore-case %s

## This tests the long symbol name case requiring continuation records.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally the order in tests is:

## Top-level comment
# RUN commands

# CHECK patterns

--- YAML

If there are multiple cases within the same test file, things vary a little depending on what typically changes between the test cases.

@@ -1,5 +1,7 @@
# RUN: yaml2obj %s | od -v -An -tx1 | FileCheck --ignore-case %s

## Test for 1 symbol case.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What coverage does this case give us that e.g. symbol-all-fields.yaml doesn't give?

@@ -1,5 +1,7 @@
# RUN: yaml2obj %s | od -v -An -tx1 | FileCheck --ignore-case %s

## This tests contains a symbol where none of the optional fields are set.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## This tests contains a symbol where none of the optional fields are set.
## This tests a symbol where none of the optional fields are set.

void GOFFState::writeSymbol(GOFFYAML::Symbol Sym) {
SmallString<80> SymName;
if (std::error_code EC = ConverterEBCDIC::convertToEBCDIC(Sym.Name, SymName))
reportError("conversion error on " + Sym.Name + ": " + EC.message());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to this fresh, I think we can reword this:

Suggested change
reportError("conversion error on " + Sym.Name + ": " + EC.message());
reportError("cannot convert '" + Sym.Name + "' to EBCDIC: " + EC.message());

You also should have a test case which triggers this error.

reportError("conversion error on " + Sym.Name + ": " + EC.message());
size_t SymNameLength = SymName.size();
if (SymNameLength > GOFF::MaxDataLength)
reportError("symbol name is too long: " + Twine(SymNameLength) +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test case for this error, please.

ECase(ESD_ST_LabelDefinition);
ECase(ESD_ST_PartReference);
ECase(ESD_ST_ExternalReference);
IO.enumFallback<Hex8>(Value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not checked each of these enums, but in each case, you should have at least one test case that uses the actual named value, and another case that uses the hex fallback. Please make sure to add them all.

if (auto *Sym = dyn_cast<GOFFYAML::Symbol>(Elem.get())) {
IO.mapRequired("Symbol", *Sym);
} else {
IO.setError("Unknown record type");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error still doesn't conform to the coding standards. Please go and read and update accordingly.

Also, all error cases need tests.

@ysyeda
Copy link
Contributor Author

ysyeda commented Apr 26, 2024

@jh7370 Thank you for your comments. FYI I won't be available to address these and my colleague @redstar will take over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants