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

[XCOFF][obj2yaml] Support SymbolAlignmentAndType as 2 separate fields in YAML. #76828

Merged
merged 9 commits into from
Feb 8, 2024

Conversation

stephenpeckham
Copy link
Contributor

XCOFF encodes a symbol type and alignment in a single 8-bit field. It is easier to read and write YAML files if the fields can be specified separately. This PR causes obj2yaml to write the fields separately and allows yaml2obj to read either the single combined field or the separate fields.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 3, 2024

@llvm/pr-subscribers-objectyaml

Author: None (stephenpeckham)

Changes

XCOFF encodes a symbol type and alignment in a single 8-bit field. It is easier to read and write YAML files if the fields can be specified separately. This PR causes obj2yaml to write the fields separately and allows yaml2obj to read either the single combined field or the separate fields.


Full diff: https://github.com/llvm/llvm-project/pull/76828.diff

6 Files Affected:

  • (modified) llvm/include/llvm/ObjectYAML/XCOFFYAML.h (+7)
  • (modified) llvm/lib/ObjectYAML/XCOFFEmitter.cpp (+11-2)
  • (modified) llvm/lib/ObjectYAML/XCOFFYAML.cpp (+14-1)
  • (modified) llvm/test/tools/obj2yaml/XCOFF/aix.yaml (+8-4)
  • (modified) llvm/test/tools/obj2yaml/XCOFF/aux-symbols.yaml (+10-5)
  • (modified) llvm/tools/obj2yaml/xcoff2yaml.cpp (+7-5)
diff --git a/llvm/include/llvm/ObjectYAML/XCOFFYAML.h b/llvm/include/llvm/ObjectYAML/XCOFFYAML.h
index f1e821fe5fa369..dd359ac8e53dd3 100644
--- a/llvm/include/llvm/ObjectYAML/XCOFFYAML.h
+++ b/llvm/include/llvm/ObjectYAML/XCOFFYAML.h
@@ -121,6 +121,9 @@ struct CsectAuxEnt : AuxSymbolEnt {
   // Common fields for both XCOFF32 and XCOFF64.
   std::optional<uint32_t> ParameterHashIndex;
   std::optional<uint16_t> TypeChkSectNum;
+  std::optional<XCOFF::SymbolType> SymbolType;
+  std::optional<uint8_t> SymbolAlignment;
+  // The two previous values can be encoded as a single value.
   std::optional<uint8_t> SymbolAlignmentAndType;
   std::optional<XCOFF::StorageMappingClass> StorageMappingClass;
 
@@ -237,6 +240,10 @@ template <> struct ScalarEnumerationTraits<XCOFF::StorageMappingClass> {
   static void enumeration(IO &IO, XCOFF::StorageMappingClass &Value);
 };
 
+template <> struct ScalarEnumerationTraits<XCOFF::SymbolType> {
+  static void enumeration(IO &IO, XCOFF::SymbolType &Value);
+};
+
 template <> struct ScalarEnumerationTraits<XCOFF::CFileStringType> {
   static void enumeration(IO &IO, XCOFF::CFileStringType &Type);
 };
diff --git a/llvm/lib/ObjectYAML/XCOFFEmitter.cpp b/llvm/lib/ObjectYAML/XCOFFEmitter.cpp
index ccf768c06aebfe..327440c6b87103 100644
--- a/llvm/lib/ObjectYAML/XCOFFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/XCOFFEmitter.cpp
@@ -23,6 +23,7 @@
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
+using namespace llvm::object;
 
 namespace {
 
@@ -525,11 +526,19 @@ bool XCOFFWriter::writeRelocations() {
 }
 
 void XCOFFWriter::writeAuxSymbol(const XCOFFYAML::CsectAuxEnt &AuxSym) {
+  uint8_t SymAlignAndType = AuxSym.SymbolAlignmentAndType.value_or(0);
+  if (AuxSym.SymbolType)
+    SymAlignAndType = (SymAlignAndType & ~XCOFFCsectAuxRef::SymbolTypeMask) |
+                      *AuxSym.SymbolType;
+  if (AuxSym.SymbolAlignment)
+    SymAlignAndType =
+        (SymAlignAndType & ~XCOFFCsectAuxRef::SymbolAlignmentMask) |
+        (*AuxSym.SymbolAlignment << XCOFFCsectAuxRef::SymbolAlignmentBitOffset);
   if (Is64Bit) {
     W.write<uint32_t>(AuxSym.SectionOrLengthLo.value_or(0));
     W.write<uint32_t>(AuxSym.ParameterHashIndex.value_or(0));
     W.write<uint16_t>(AuxSym.TypeChkSectNum.value_or(0));
-    W.write<uint8_t>(AuxSym.SymbolAlignmentAndType.value_or(0));
+    W.write<uint8_t>(SymAlignAndType);
     W.write<uint8_t>(AuxSym.StorageMappingClass.value_or(XCOFF::XMC_PR));
     W.write<uint32_t>(AuxSym.SectionOrLengthHi.value_or(0));
     W.write<uint8_t>(0);
@@ -538,7 +547,7 @@ void XCOFFWriter::writeAuxSymbol(const XCOFFYAML::CsectAuxEnt &AuxSym) {
     W.write<uint32_t>(AuxSym.SectionOrLength.value_or(0));
     W.write<uint32_t>(AuxSym.ParameterHashIndex.value_or(0));
     W.write<uint16_t>(AuxSym.TypeChkSectNum.value_or(0));
-    W.write<uint8_t>(AuxSym.SymbolAlignmentAndType.value_or(0));
+    W.write<uint8_t>(SymAlignAndType);
     W.write<uint8_t>(AuxSym.StorageMappingClass.value_or(XCOFF::XMC_PR));
     W.write<uint32_t>(AuxSym.StabInfoIndex.value_or(0));
     W.write<uint16_t>(AuxSym.StabSectNum.value_or(0));
diff --git a/llvm/lib/ObjectYAML/XCOFFYAML.cpp b/llvm/lib/ObjectYAML/XCOFFYAML.cpp
index 398b09c72170ba..a1eea621a1056e 100644
--- a/llvm/lib/ObjectYAML/XCOFFYAML.cpp
+++ b/llvm/lib/ObjectYAML/XCOFFYAML.cpp
@@ -127,6 +127,16 @@ void ScalarEnumerationTraits<XCOFF::StorageMappingClass>::enumeration(
 #undef ECase
 }
 
+void ScalarEnumerationTraits<XCOFF::SymbolType>::enumeration(
+    IO &IO, XCOFF::SymbolType &Value) {
+#define ECase(X) IO.enumCase(Value, #X, XCOFF::X)
+  ECase(XTY_ER);
+  ECase(XTY_SD);
+  ECase(XTY_LD);
+  ECase(XTY_CM);
+#undef ECase
+}
+
 void ScalarEnumerationTraits<XCOFFYAML::AuxSymbolType>::enumeration(
     IO &IO, XCOFFYAML::AuxSymbolType &Type) {
 #define ECase(X) IO.enumCase(Type, #X, XCOFFYAML::X)
@@ -229,6 +239,8 @@ static void auxSymMapping(IO &IO, XCOFFYAML::CsectAuxEnt &AuxSym, bool Is64) {
   IO.mapOptional("ParameterHashIndex", AuxSym.ParameterHashIndex);
   IO.mapOptional("TypeChkSectNum", AuxSym.TypeChkSectNum);
   IO.mapOptional("SymbolAlignmentAndType", AuxSym.SymbolAlignmentAndType);
+  IO.mapOptional("SymbolType", AuxSym.SymbolType);
+  IO.mapOptional("SymbolAlignment", AuxSym.SymbolAlignment);
   IO.mapOptional("StorageMappingClass", AuxSym.StorageMappingClass);
   if (Is64) {
     IO.mapOptional("SectionOrLengthLo", AuxSym.SectionOrLengthLo);
@@ -350,7 +362,8 @@ void MappingTraits<XCOFFYAML::Symbol>::mapping(IO &IO, XCOFFYAML::Symbol &S) {
   IO.mapOptional("AuxEntries", S.AuxEntries);
 }
 
-void MappingTraits<XCOFFYAML::StringTable>::mapping(IO &IO, XCOFFYAML::StringTable &Str) {
+void MappingTraits<XCOFFYAML::StringTable>::mapping(
+    IO &IO, XCOFFYAML::StringTable &Str) {
   IO.mapOptional("ContentSize", Str.ContentSize);
   IO.mapOptional("Length", Str.Length);
   IO.mapOptional("Strings", Str.Strings);
diff --git a/llvm/test/tools/obj2yaml/XCOFF/aix.yaml b/llvm/test/tools/obj2yaml/XCOFF/aix.yaml
index fbd5fa0629d10b..9f2f68b646b6f4 100644
--- a/llvm/test/tools/obj2yaml/XCOFF/aix.yaml
+++ b/llvm/test/tools/obj2yaml/XCOFF/aix.yaml
@@ -56,7 +56,8 @@
 # CHECK32-NEXT:       - Type:            AUX_CSECT
 # CHECK32-NEXT:         ParameterHashIndex: 0
 # CHECK32-NEXT:         TypeChkSectNum:  0
-# CHECK32-NEXT:         SymbolAlignmentAndType: 0
+# CHECK32-NEXT:         SymbolType:      XTY_ER
+# CHECK32-NEXT:         SymbolAlignment: 0
 # CHECK32-NEXT:         StorageMappingClass: XMC_PR
 # CHECK32-NEXT:         SectionOrLength: 0
 # CHECK32-NEXT:         StabInfoIndex:   0
@@ -71,7 +72,8 @@
 # CHECK32-NEXT:       - Type:            AUX_CSECT
 # CHECK32-NEXT:         ParameterHashIndex: 0
 # CHECK32-NEXT:         TypeChkSectNum:  0
-# CHECK32-NEXT:         SymbolAlignmentAndType: 0
+# CHECK32-NEXT:         SymbolType:      XTY_ER
+# CHECK32-NEXT:         SymbolAlignment: 0
 # CHECK32-NEXT:         StorageMappingClass: XMC_PR
 # CHECK32-NEXT:         SectionOrLength: 0
 # CHECK32-NEXT:         StabInfoIndex:   0
@@ -128,7 +130,8 @@
 # CHECK64-NEXT:       - Type:            AUX_CSECT
 # CHECK64-NEXT:         ParameterHashIndex: 0
 # CHECK64-NEXT:         TypeChkSectNum:  0
-# CHECK64-NEXT:         SymbolAlignmentAndType: 0
+# CHECK64-NEXT:         SymbolType:      XTY_ER
+# CHECK64-NEXT:         SymbolAlignment: 0
 # CHECK64-NEXT:         StorageMappingClass: XMC_PR
 # CHECK64-NEXT:         SectionOrLengthLo: 0
 # CHECK64-NEXT:         SectionOrLengthHi: 0
@@ -142,7 +145,8 @@
 # CHECK64-NEXT:       - Type:            AUX_CSECT
 # CHECK64-NEXT:         ParameterHashIndex: 0
 # CHECK64-NEXT:         TypeChkSectNum:  0
-# CHECK64-NEXT:         SymbolAlignmentAndType: 0
+# CHECK64-NEXT:         SymbolType:      XTY_ER
+# CHECK64-NEXT:         SymbolAlignment: 0
 # CHECK64-NEXT:         StorageMappingClass: XMC_PR
 # CHECK64-NEXT:         SectionOrLengthLo: 0
 # CHECK64-NEXT:         SectionOrLengthHi: 0
diff --git a/llvm/test/tools/obj2yaml/XCOFF/aux-symbols.yaml b/llvm/test/tools/obj2yaml/XCOFF/aux-symbols.yaml
index 7f93b8dae0ca9b..2260bb07749898 100644
--- a/llvm/test/tools/obj2yaml/XCOFF/aux-symbols.yaml
+++ b/llvm/test/tools/obj2yaml/XCOFF/aux-symbols.yaml
@@ -34,7 +34,8 @@
 # CHECK32-NEXT:       - Type:            AUX_CSECT
 # CHECK32-NEXT:         ParameterHashIndex: 1
 # CHECK32-NEXT:         TypeChkSectNum:  2
-# CHECK32-NEXT:         SymbolAlignmentAndType: 41
+# CHECK32-NEXT:         SymbolType: XTY_SD
+# CHECK32-NEXT:         SymbolAlignment: 5
 # CHECK32-NEXT:         StorageMappingClass: XMC_PR
 # CHECK32-NEXT:         SectionOrLength: 3
 # CHECK32-NEXT:         StabInfoIndex:   4
@@ -54,7 +55,8 @@
 # CHECK32-NEXT:       - Type:            AUX_CSECT
 # CHECK32-NEXT:         ParameterHashIndex: 1
 # CHECK32-NEXT:         TypeChkSectNum:  2
-# CHECK32-NEXT:         SymbolAlignmentAndType: 17
+# CHECK32-NEXT:         SymbolType: XTY_SD
+# CHECK32-NEXT:         SymbolAlignment: 2
 # CHECK32-NEXT:         StorageMappingClass: XMC_PR
 # CHECK32-NEXT:         SectionOrLength: 4
 # CHECK32-NEXT:         StabInfoIndex:   5
@@ -105,7 +107,8 @@ Symbols:
       - Type:               AUX_CSECT
         ParameterHashIndex: 1
         TypeChkSectNum:     2
-        SymbolAlignmentAndType: 41
+        SymbolAlignment: 5
+        SymbolType: XTY_SD
         SectionOrLength:    3
         StabInfoIndex:      4
         StabSectNum:        5
@@ -174,7 +177,8 @@ Symbols:
 # CHECK64-NEXT:       - Type:            AUX_CSECT
 # CHECK64-NEXT:         ParameterHashIndex: 1
 # CHECK64-NEXT:         TypeChkSectNum:  2
-# CHECK64-NEXT:         SymbolAlignmentAndType: 41
+# CHECK64-NEXT:         SymbolType: XTY_SD
+# CHECK64-NEXT:         SymbolAlignment: 5
 # CHECK64-NEXT:         StorageMappingClass: XMC_PR
 # CHECK64-NEXT:         SectionOrLengthLo: 3
 # CHECK64-NEXT:         SectionOrLengthHi: 4
@@ -196,7 +200,8 @@ Symbols:
 # CHECK64-NEXT:       - Type:            AUX_CSECT
 # CHECK64-NEXT:         ParameterHashIndex: 1
 # CHECK64-NEXT:         TypeChkSectNum:  2
-# CHECK64-NEXT:         SymbolAlignmentAndType: 17
+# CHECK64-NEXT:         SymbolType: XTY_SD
+# CHECK64-NEXT:         SymbolAlignment: 2
 # CHECK64-NEXT:         StorageMappingClass: XMC_PR
 # CHECK64-NEXT:         SectionOrLengthLo: 3
 # CHECK64-NEXT:         SectionOrLengthHi: 4
diff --git a/llvm/tools/obj2yaml/xcoff2yaml.cpp b/llvm/tools/obj2yaml/xcoff2yaml.cpp
index f7c2bae7479895..e426b645cbeff6 100644
--- a/llvm/tools/obj2yaml/xcoff2yaml.cpp
+++ b/llvm/tools/obj2yaml/xcoff2yaml.cpp
@@ -37,7 +37,7 @@ class XCOFFDumper {
   Error dumpAuxSyms(XCOFFYAML::Symbol &Sym, const XCOFFSymbolRef &SymbolEntRef);
   void dumpFuncAuxSym(XCOFFYAML::Symbol &Sym, const uintptr_t AuxAddress);
   void dumpExpAuxSym(XCOFFYAML::Symbol &Sym, const uintptr_t AuxAddress);
-  void dumpCscetAuxSym(XCOFFYAML::Symbol &Sym,
+  void dumpCsectAuxSym(XCOFFYAML::Symbol &Sym,
                        const object::XCOFFCsectAuxRef &AuxEntPtr);
 
 public:
@@ -204,12 +204,14 @@ void XCOFFDumper::dumpExpAuxSym(XCOFFYAML::Symbol &Sym,
       std::make_unique<XCOFFYAML::ExcpetionAuxEnt>(ExceptAuxSym));
 }
 
-void XCOFFDumper::dumpCscetAuxSym(XCOFFYAML::Symbol &Sym,
+void XCOFFDumper::dumpCsectAuxSym(XCOFFYAML::Symbol &Sym,
                                   const object::XCOFFCsectAuxRef &AuxEntPtr) {
   XCOFFYAML::CsectAuxEnt CsectAuxSym;
   CsectAuxSym.ParameterHashIndex = AuxEntPtr.getParameterHashIndex();
   CsectAuxSym.TypeChkSectNum = AuxEntPtr.getTypeChkSectNum();
-  CsectAuxSym.SymbolAlignmentAndType = AuxEntPtr.getSymbolAlignmentAndType();
+  CsectAuxSym.SymbolAlignment = AuxEntPtr.getAlignmentLog2();
+  CsectAuxSym.SymbolType =
+      static_cast<XCOFF::SymbolType>(AuxEntPtr.getSymbolType());
   CsectAuxSym.StorageMappingClass = AuxEntPtr.getStorageMappingClass();
 
   if (Obj.is64Bit()) {
@@ -237,7 +239,7 @@ Error XCOFFDumper::dumpAuxSyms(XCOFFYAML::Symbol &Sym,
   for (uint8_t I = 1; I <= Sym.NumberOfAuxEntries; ++I) {
 
     if (I == Sym.NumberOfAuxEntries && !Obj.is64Bit()) {
-      dumpCscetAuxSym(Sym, CsectAuxRef);
+      dumpCsectAuxSym(Sym, CsectAuxRef);
       return Error::success();
     }
 
@@ -247,7 +249,7 @@ Error XCOFFDumper::dumpAuxSyms(XCOFFYAML::Symbol &Sym,
     if (Obj.is64Bit()) {
       XCOFF::SymbolAuxType Type = *Obj.getSymbolAuxType(AuxAddress);
       if (Type == XCOFF::SymbolAuxType::AUX_CSECT)
-        dumpCscetAuxSym(Sym, CsectAuxRef);
+        dumpCsectAuxSym(Sym, CsectAuxRef);
       else if (Type == XCOFF::SymbolAuxType::AUX_FCN)
         dumpFuncAuxSym(Sym, AuxAddress);
       else if (Type == XCOFF::SymbolAuxType::AUX_EXCEPT)

Copy link
Contributor

@EsmeYi EsmeYi left a comment

Choose a reason for hiding this comment

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

Please add test case respect to yaml2obj.

@@ -37,7 +37,7 @@ class XCOFFDumper {
Error dumpAuxSyms(XCOFFYAML::Symbol &Sym, const XCOFFSymbolRef &SymbolEntRef);
void dumpFuncAuxSym(XCOFFYAML::Symbol &Sym, const uintptr_t AuxAddress);
void dumpExpAuxSym(XCOFFYAML::Symbol &Sym, const uintptr_t AuxAddress);
void dumpCscetAuxSym(XCOFFYAML::Symbol &Sym,
void dumpCsectAuxSym(XCOFFYAML::Symbol &Sym,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for correcting typo. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be fixed separately. No need for a review though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can use a separate PR. But in general, I think fixing minor typos along with functional changes makes life easier for everyone. And the typo is related to the functional change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't find an exact reference from the community guidelines, but it's strongly related to this part of the "How to submit a patch" section describing what a patch must be:

be an isolated change. Independent changes should be submitted as separate patches as this makes reviewing easier.

The norm is to do the nfc fix immediately before landing the real patch.

if (AuxSym.SymbolAlignment)
SymAlignAndType =
(SymAlignAndType & ~XCOFFCsectAuxRef::SymbolAlignmentMask) |
(*AuxSym.SymbolAlignment << XCOFFCsectAuxRef::SymbolAlignmentBitOffset);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test case in yaml stating that SymbolAlignmentAndType will be overridden if SymbolType/SymbolAlignment is defined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In similar cases in ELF yaml2obj, it is an error to specify both forms. I think the same should apply here (and be tested of course).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's harmless to allow both forms.

Copy link
Contributor

@diggerlin diggerlin Jan 12, 2024

Choose a reason for hiding this comment

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

if both forms in a YAML file, in the source code "SymbolType/SymbolAlignment" will override SymbolAlignment, but the user may not know the rule, I have same opinion as Jame to emit an error to specify both forms.

We use obj2yaml to generate a yaml file and use the yaml as input for llvm test case usually. we rarely write yaml of XCOFF in llvm test case from scratch. since you only output SymbolType/SymbolAlignment in the obj2yaml . if it is difficult to emit error/warning, I am prefer support "SymbolType/SymbolAlignment" not support SymbolAlignment.

if (AuxSym.SymbolAlignment)
SymAlignAndType =
(SymAlignAndType & ~XCOFFCsectAuxRef::SymbolAlignmentMask) |
(*AuxSym.SymbolAlignment << XCOFFCsectAuxRef::SymbolAlignmentBitOffset);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In similar cases in ELF yaml2obj, it is an error to specify both forms. I think the same should apply here (and be tested of course).

llvm/lib/ObjectYAML/XCOFFYAML.cpp Show resolved Hide resolved
llvm/test/tools/obj2yaml/XCOFF/aux-aligntype.yaml Outdated Show resolved Hide resolved
llvm/test/tools/obj2yaml/XCOFF/aux-aligntype.yaml Outdated Show resolved Hide resolved
llvm/test/tools/obj2yaml/XCOFF/aux-aligntype.yaml Outdated Show resolved Hide resolved
llvm/test/tools/obj2yaml/XCOFF/aux-aligntype.yaml Outdated Show resolved Hide resolved
@@ -37,7 +37,7 @@ class XCOFFDumper {
Error dumpAuxSyms(XCOFFYAML::Symbol &Sym, const XCOFFSymbolRef &SymbolEntRef);
void dumpFuncAuxSym(XCOFFYAML::Symbol &Sym, const uintptr_t AuxAddress);
void dumpExpAuxSym(XCOFFYAML::Symbol &Sym, const uintptr_t AuxAddress);
void dumpCscetAuxSym(XCOFFYAML::Symbol &Sym,
void dumpCsectAuxSym(XCOFFYAML::Symbol &Sym,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be fixed separately. No need for a review though.

@jh7370
Copy link
Collaborator

jh7370 commented Jan 8, 2024

I forgot to add that your testing needs to show what happens if one or both of the fields are missing.

@stephenpeckham
Copy link
Contributor Author

Code was added to validate the alignment field.

if (AuxSym.SymbolAlignment)
SymAlignAndType =
(SymAlignAndType & ~XCOFFCsectAuxRef::SymbolAlignmentMask) |
(*AuxSym.SymbolAlignment << XCOFFCsectAuxRef::SymbolAlignmentBitOffset);
Copy link
Contributor

@diggerlin diggerlin Jan 12, 2024

Choose a reason for hiding this comment

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

if both forms in a YAML file, in the source code "SymbolType/SymbolAlignment" will override SymbolAlignment, but the user may not know the rule, I have same opinion as Jame to emit an error to specify both forms.

We use obj2yaml to generate a yaml file and use the yaml as input for llvm test case usually. we rarely write yaml of XCOFF in llvm test case from scratch. since you only output SymbolType/SymbolAlignment in the obj2yaml . if it is difficult to emit error/warning, I am prefer support "SymbolType/SymbolAlignment" not support SymbolAlignment.

AuxEntries:
- Type: AUX_CSECT
SymbolAlignmentAndType: 17
SymbolAlignment: 4
Copy link
Contributor

@diggerlin diggerlin Jan 12, 2024

Choose a reason for hiding this comment

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

if user have YAML as

        SymbolAlignmentAndType: 17
        SymbolAlignment: 4

I am prefer to emit error or warning in this test scenario as James's suggestion.

- Type: AUX_CSECT
SymbolAlignment: 2
SymbolType: XTY_SD
[[SectionOrLength]]: 4
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the [[SectionOrLength]]: 4 in this test case ?

AuxEntries:
- Type: AUX_FILE
FileNameOrString: FileName
FileStringType: XFT_CD
Copy link
Contributor

@diggerlin diggerlin Jan 16, 2024

Choose a reason for hiding this comment

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

and we do not care about the StorageClass: C_FILE when we test the new functionality of the PR. Can we delete it?

AuxEntries:
- Type: AUX_CSECT
SymbolType: XTY_SD
[[SectionOrLength]]: 4
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto


## Ensure that SymbolAlignment is in range.
# RUN: not yaml2obj %s --docnum=2 -o %t 2>&1 | FileCheck %s --check-prefix=ERROR1
# ERROR1: Symbol alignment must be less than 32
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure which one is better 32 or 2^32 ?

FileHeader:
MagicNumber: 0x1F7
Symbols:
- StorageClass: C_FILE
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

FileHeader:
MagicNumber: 0x1DF
Symbols:
- StorageClass: C_FILE
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

FileHeader:
MagicNumber: 0x1DF
Symbols:
- StorageClass: C_FILE
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

@diggerlin diggerlin left a comment

Choose a reason for hiding this comment

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

LGTM, but please wait for James happy with the PR.

llvm/test/tools/obj2yaml/XCOFF/aux-aligntype.yaml Outdated Show resolved Hide resolved
llvm/lib/ObjectYAML/XCOFFYAML.cpp Show resolved Hide resolved
@@ -37,7 +37,7 @@ class XCOFFDumper {
Error dumpAuxSyms(XCOFFYAML::Symbol &Sym, const XCOFFSymbolRef &SymbolEntRef);
void dumpFuncAuxSym(XCOFFYAML::Symbol &Sym, const uintptr_t AuxAddress);
void dumpExpAuxSym(XCOFFYAML::Symbol &Sym, const uintptr_t AuxAddress);
void dumpCscetAuxSym(XCOFFYAML::Symbol &Sym,
void dumpCsectAuxSym(XCOFFYAML::Symbol &Sym,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't find an exact reference from the community guidelines, but it's strongly related to this part of the "How to submit a patch" section describing what a patch must be:

be an isolated change. Independent changes should be submitted as separate patches as this makes reviewing easier.

The norm is to do the nfc fix immediately before landing the real patch.

llvm/test/tools/yaml2obj/XCOFF/aux-symbols.yaml Outdated Show resolved Hide resolved
llvm/test/tools/obj2yaml/XCOFF/aux-aligntype.yaml Outdated Show resolved Hide resolved
llvm/test/tools/obj2yaml/XCOFF/aux-aligntype.yaml Outdated Show resolved Hide resolved
llvm/test/tools/obj2yaml/XCOFF/aux-aligntype.yaml Outdated Show resolved Hide resolved
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.

LGTM. Sorry for the delay - I was off sick for much of last week.

@jh7370
Copy link
Collaborator

jh7370 commented Feb 6, 2024

Actually, please could you check the buildkit failures: they look to be tied to this PR, so I'm concerned there might be something you've done wrong?

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.

Requesting changes until pre-merge CI issues have been addressed: if they are spurious, please say so, but I suspect they might not be.

@stephenpeckham
Copy link
Contributor Author

I fixed a problem what showed up in the Linux environment. The Windows build failed immediately.

@stephenpeckham
Copy link
Contributor Author

Linux passes. Windows fails, probably because of a buildbot error.

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.

LGTM again.

@stephenpeckham stephenpeckham merged commit 5aeabf2 into llvm:main Feb 8, 2024
3 of 4 checks passed
@stephenpeckham stephenpeckham deleted the peckham/yamlalign branch February 8, 2024 18:58
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