Skip to content

[XCOFF][obj2yaml] support parsing auxiliary symbols for XCOFF #70642

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

Merged
merged 8 commits into from
Dec 7, 2023
Merged

[XCOFF][obj2yaml] support parsing auxiliary symbols for XCOFF #70642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7ba6a65
Add support for parsing auxiliary symbols of XCOFF object file for ob…
Oct 30, 2023
724fb01
Address James's comments.
Nov 7, 2023
1f12c62
Address James's comments; move case bodies to separate functions.
Nov 8, 2023
70f8325
Address part of Digger's comments.
Nov 8, 2023
d9f2abd
Address Digger's comments: 1) keep the order of AUX_CSECT symbol; 2) …
Nov 21, 2023
be93262
Enrich error messages and test them.
Nov 22, 2023
96aff59
printf symbol index info for error message.
Nov 23, 2023
bfc5696
Merge branch 'llvm:main' into xcoff2yaml_auxsym
EsmeYi Dec 5, 2023
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
2 changes: 1 addition & 1 deletion llvm/include/llvm/Object/XCOFFObjectFile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,13 +411,13 @@ class XCOFFCsectAuxRef {
return Entry64->AuxType;
}

private:
uint8_t getSymbolAlignmentAndType() const {
return GETVALUE(SymbolAlignmentAndType);
}

#undef GETVALUE

private:
const XCOFFCsectAuxEnt32 *Entry32 = nullptr;
const XCOFFCsectAuxEnt64 *Entry64 = nullptr;
};
Expand Down
35 changes: 23 additions & 12 deletions llvm/lib/ObjectYAML/XCOFFYAML.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,45 +282,57 @@ static void auxSymMapping(IO &IO, XCOFFYAML::SectAuxEntForStat &AuxSym) {

void MappingTraits<std::unique_ptr<XCOFFYAML::AuxSymbolEnt>>::mapping(
IO &IO, std::unique_ptr<XCOFFYAML::AuxSymbolEnt> &AuxSym) {
assert(!IO.outputting() && "We don't dump aux symbols currently.");

auto ResetAuxSym = [&](auto *AuxEnt) {
if (!IO.outputting())
AuxSym.reset(AuxEnt);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if IO.outputting() is true, will AuxEnt be deleted?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might use a templated helper function to replace this lambda.

template<typename AuxEntT>
static void ResetAuxSym(IO &IO, std::unique_ptr<XCOFFYAML::AuxSymbolEnt> &AuxSym) {
  if (!IO.outputting()) AuxSym.reset(new AuxEntT);
}

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, this seems cause a sanitizer error in https://lab.llvm.org/buildbot/#/builders/5/builds/39023
I gonna fix it.

};

const bool Is64 =
static_cast<XCOFFYAML::Object *>(IO.getContext())->Header.Magic ==
(llvm::yaml::Hex16)XCOFF::XCOFF64;

XCOFFYAML::AuxSymbolType AuxType;
if (IO.outputting())
AuxType = AuxSym.get()->Type;
IO.mapRequired("Type", AuxType);
switch (AuxType) {
case XCOFFYAML::AUX_EXCEPT:
if (!Is64)
if (!Is64) {
IO.setError("an auxiliary symbol of type AUX_EXCEPT cannot be defined in "
"XCOFF32");
AuxSym.reset(new XCOFFYAML::ExcpetionAuxEnt());
return;
}
ResetAuxSym(new XCOFFYAML::ExcpetionAuxEnt());
Copy link
Contributor

Choose a reason for hiding this comment

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

in the code, for 32bit, after you set IO.setError m it will still tun into this line, do you forget else ?

Copy link
Author

Choose a reason for hiding this comment

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

The yaml2obj fails to parse immediately once the IO.setError() triggered and the error message is printed directly, therefore we don't need an else here.

Copy link
Contributor

@diggerlin diggerlin Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

after the error print out, but the it do not return function immediately. it go to the ResetAuxSym(new XCOFFYAML::ExcpetionAuxEnt()) , since the ExcpetionAuxEnt is not support in the 32bit, do not need to run the `ResetAuxSym(new XCOFFYAML::ExcpetionAuxEnt());' is it correct ?

Copy link
Author

Choose a reason for hiding this comment

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

OK, I see. Thanks!

auxSymMapping(IO, *cast<XCOFFYAML::ExcpetionAuxEnt>(AuxSym.get()));
break;
case XCOFFYAML::AUX_FCN:
AuxSym.reset(new XCOFFYAML::FunctionAuxEnt());
ResetAuxSym(new XCOFFYAML::FunctionAuxEnt());
auxSymMapping(IO, *cast<XCOFFYAML::FunctionAuxEnt>(AuxSym.get()), Is64);
break;
case XCOFFYAML::AUX_SYM:
AuxSym.reset(new XCOFFYAML::BlockAuxEnt());
ResetAuxSym(new XCOFFYAML::BlockAuxEnt());
auxSymMapping(IO, *cast<XCOFFYAML::BlockAuxEnt>(AuxSym.get()), Is64);
break;
case XCOFFYAML::AUX_FILE:
AuxSym.reset(new XCOFFYAML::FileAuxEnt());
ResetAuxSym(new XCOFFYAML::FileAuxEnt());
auxSymMapping(IO, *cast<XCOFFYAML::FileAuxEnt>(AuxSym.get()));
break;
case XCOFFYAML::AUX_CSECT:
AuxSym.reset(new XCOFFYAML::CsectAuxEnt());
ResetAuxSym(new XCOFFYAML::CsectAuxEnt());
auxSymMapping(IO, *cast<XCOFFYAML::CsectAuxEnt>(AuxSym.get()), Is64);
break;
case XCOFFYAML::AUX_SECT:
AuxSym.reset(new XCOFFYAML::SectAuxEntForDWARF());
ResetAuxSym(new XCOFFYAML::SectAuxEntForDWARF());
auxSymMapping(IO, *cast<XCOFFYAML::SectAuxEntForDWARF>(AuxSym.get()));
break;
case XCOFFYAML::AUX_STAT:
if (Is64)
if (Is64) {
IO.setError(
"an auxiliary symbol of type AUX_STAT cannot be defined in XCOFF64");
AuxSym.reset(new XCOFFYAML::SectAuxEntForStat());
return;
}
ResetAuxSym(new XCOFFYAML::SectAuxEntForStat());
auxSymMapping(IO, *cast<XCOFFYAML::SectAuxEntForStat>(AuxSym.get()));
break;
}
Expand All @@ -334,8 +346,7 @@ void MappingTraits<XCOFFYAML::Symbol>::mapping(IO &IO, XCOFFYAML::Symbol &S) {
IO.mapOptional("Type", S.Type);
IO.mapOptional("StorageClass", S.StorageClass);
IO.mapOptional("NumberOfAuxEntries", S.NumberOfAuxEntries);
if (!IO.outputting())
IO.mapOptional("AuxEntries", S.AuxEntries);
IO.mapOptional("AuxEntries", S.AuxEntries);
}

void MappingTraits<XCOFFYAML::StringTable>::mapping(IO &IO, XCOFFYAML::StringTable &Str) {
Expand Down
38 changes: 38 additions & 0 deletions llvm/test/tools/obj2yaml/XCOFF/aix.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,30 @@
# CHECK32-NEXT: Type: 0x0
# CHECK32-NEXT: StorageClass: C_EXT
# CHECK32-NEXT: NumberOfAuxEntries: 1
# CHECK32-NEXT: AuxEntries:
Copy link
Contributor

Choose a reason for hiding this comment

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

just curiosity, you have AUX_CSECT test in aux0symbols,yaml, why you add test case here ?

Copy link
Author

Choose a reason for hiding this comment

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

NumberOfAuxEntries was set to 1 but non auxiliary symbol is defined in this case, which was allowed before but will be illegal after this PR. And aix.yaml is a basic test for general behavior of xcoff2yaml and I think it would be better to add such test instead of remove the set of NumberOfAuxEntries: 1 here.

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 explain.

# CHECK32-NEXT: - Type: AUX_CSECT
# CHECK32-NEXT: ParameterHashIndex: 0
# CHECK32-NEXT: TypeChkSectNum: 0
# CHECK32-NEXT: SymbolAlignmentAndType: 0
# CHECK32-NEXT: StorageMappingClass: XMC_PR
# CHECK32-NEXT: SectionOrLength: 0
# CHECK32-NEXT: StabInfoIndex: 0
# CHECK32-NEXT: StabSectNum: 0
# CHECK32-NEXT: - Name: .data
# CHECK32-NEXT: Value: 0x70
# CHECK32-NEXT: Section: .data
# CHECK32-NEXT: Type: 0x0
# CHECK32-NEXT: StorageClass: C_HIDEXT
# CHECK32-NEXT: NumberOfAuxEntries: 1
# CHECK32-NEXT: AuxEntries:
# CHECK32-NEXT: - Type: AUX_CSECT
# CHECK32-NEXT: ParameterHashIndex: 0
# CHECK32-NEXT: TypeChkSectNum: 0
# CHECK32-NEXT: SymbolAlignmentAndType: 0
# CHECK32-NEXT: StorageMappingClass: XMC_PR
# CHECK32-NEXT: SectionOrLength: 0
# CHECK32-NEXT: StabInfoIndex: 0
# CHECK32-NEXT: StabSectNum: 0

# CHECK64: --- !XCOFF
# CHECK64-NEXT: FileHeader:
Expand Down Expand Up @@ -106,12 +124,28 @@
# CHECK64-NEXT: Type: 0x0
# CHECK64-NEXT: StorageClass: C_EXT
# CHECK64-NEXT: NumberOfAuxEntries: 1
# CHECK64-NEXT: AuxEntries:
# CHECK64-NEXT: - Type: AUX_CSECT
# CHECK64-NEXT: ParameterHashIndex: 0
# CHECK64-NEXT: TypeChkSectNum: 0
# CHECK64-NEXT: SymbolAlignmentAndType: 0
# CHECK64-NEXT: StorageMappingClass: XMC_PR
# CHECK64-NEXT: SectionOrLengthLo: 0
# CHECK64-NEXT: SectionOrLengthHi: 0
# CHECK64-NEXT: - Name: .data
# CHECK64-NEXT: Value: 0x70
# CHECK64-NEXT: Section: .data
# CHECK64-NEXT: Type: 0x0
# CHECK64-NEXT: StorageClass: C_HIDEXT
# CHECK64-NEXT: NumberOfAuxEntries: 1
# CHECK64-NEXT: AuxEntries:
# CHECK64-NEXT: - Type: AUX_CSECT
# CHECK64-NEXT: ParameterHashIndex: 0
# CHECK64-NEXT: TypeChkSectNum: 0
# CHECK64-NEXT: SymbolAlignmentAndType: 0
# CHECK64-NEXT: StorageMappingClass: XMC_PR
# CHECK64-NEXT: SectionOrLengthLo: 0
# CHECK64-NEXT: SectionOrLengthHi: 0

--- !XCOFF
FileHeader:
Expand Down Expand Up @@ -140,9 +174,13 @@ Symbols:
Type: 0x0
StorageClass: C_EXT
NumberOfAuxEntries: 1
AuxEntries:
- Type: AUX_CSECT
- Name: .data
Value: 0x70
Section: .data
Type: 0x0
StorageClass: C_HIDEXT
NumberOfAuxEntries: 1
AuxEntries:
- Type: AUX_CSECT
Loading