Skip to content

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

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.

Sign up for GitHub

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

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
36be167
add sections & symbols
Dec 19, 2023
ae601e2
add tests
Dec 19, 2023
e53969c
fix formtting
Dec 19, 2023
22c968e
remove section related code
Jan 2, 2024
25a8034
address comments
Jan 18, 2024
2ce0e3b
remove std::option for Signature
Mar 12, 2024
ddca557
remove checks
Mar 14, 2024
c92c5c2
more comments
Mar 15, 2024
263bef6
add maxdatalength to message, define NameLengthOffset
Mar 15, 2024
85251f6
remove Records label if there are none
Apr 4, 2024
975462f
add more testing
Apr 9, 2024
980e0d1
address new set of comments
Apr 16, 2024
2861f97
fix ordering for tests + other various comments
Apr 25, 2024
97625ca
fix coding standard for error
Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions llvm/lib/ObjectYAML/GOFFEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,13 @@ void GOFFState::writeHeader(GOFFYAML::FileHeader &FileHdr) {
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);
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.

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.

". Max length is: " + Twine(GOFF::MaxDataLength));

unsigned NameLengthOffset = 69;
const unsigned NameLengthOffset = 69;
GW.makeNewRecord(GOFF::RT_ESD, NameLengthOffset + SymNameLength);
GW << binaryBe(Sym.Type) // Symbol type
<< binaryBe(Sym.ID) // ESDID
Expand Down Expand Up @@ -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.

RecordNum++;
}
writeEnd();
return true;
}
Expand Down
3 changes: 0 additions & 3 deletions llvm/lib/ObjectYAML/GOFFYAML.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,6 @@ void ScalarBitSetTraits<GOFFYAML::GOFF_BAFLAGS>::bitset(
BCase(ESD_BA_COMMON);
BCase(ESD_BA_Indirect);
}
#undef BCase
#undef BCaseMask
#undef ECase

void MappingTraits<GOFFYAML::Symbol>::mapping(IO &IO, GOFFYAML::Symbol &Sym) {
IO.mapRequired("Name", Sym.Name);
Expand Down
2 changes: 2 additions & 0 deletions llvm/test/tools/yaml2obj/GOFF/long-symbol.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.


--- !GOFF
FileHeader:
ArchitectureLevel: 1
Expand Down
2 changes: 2 additions & 0 deletions llvm/test/tools/yaml2obj/GOFF/one-symbol.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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?


## Verify that GOFF Header is correct.
# CHECK: 03 f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00
# CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Expand Down
2 changes: 2 additions & 0 deletions llvm/test/tools/yaml2obj/GOFF/symbol-all-fields.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# RUN: yaml2obj %s | od -v -An -tx1 | FileCheck --ignore-case %s

## This tests a symbol where all required and optional fields are set.

## Verify that GOFF Header is correct.
# CHECK: 03 f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00
# CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Expand Down
2 changes: 2 additions & 0 deletions llvm/test/tools/yaml2obj/GOFF/symbol-required.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.


## Verify that GOFF Header is correct.
# CHECK: 03 f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00
# CHECK-NEXT: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Expand Down
Loading