Skip to content

Reapply "Allow "[[FLAGS=<none>]]" value in the ELF Fileheader Flags field (#143845)" #151094

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 1 commit into from
Jul 31, 2025
Merged

Reapply "Allow "[[FLAGS=<none>]]" value in the ELF Fileheader Flags field (#143845)" #151094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/ObjectYAML/ELFYAML.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ struct FileHeader {
llvm::yaml::Hex8 ABIVersion;
ELF_ET Type;
std::optional<ELF_EM> Machine;
ELF_EF Flags;
std::optional<ELF_EF> Flags;
llvm::yaml::Hex64 Entry;
std::optional<StringRef> SectionHeaderStringTable;

Expand Down
6 changes: 5 additions & 1 deletion llvm/lib/ObjectYAML/ELFEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,11 @@ void ELFState<ELFT>::writeELFHeader(raw_ostream &OS) {

Header.e_version = EV_CURRENT;
Header.e_entry = Doc.Header.Entry;
Header.e_flags = Doc.Header.Flags;
if (Doc.Header.Flags)
Header.e_flags = *Doc.Header.Flags;
else
Header.e_flags = 0;

Header.e_ehsize = sizeof(Elf_Ehdr);

if (Doc.Header.EPhOff)
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/ObjectYAML/ELFYAML.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,7 +1160,7 @@ void MappingTraits<ELFYAML::FileHeader>::mapping(IO &IO,
IO.mapOptional("ABIVersion", FileHdr.ABIVersion, Hex8(0));
IO.mapRequired("Type", FileHdr.Type);
IO.mapOptional("Machine", FileHdr.Machine);
IO.mapOptional("Flags", FileHdr.Flags, ELFYAML::ELF_EF(0));
IO.mapOptional("Flags", FileHdr.Flags);
Copy link
Contributor

@lamb-j lamb-j Jul 31, 2025

Choose a reason for hiding this comment

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

This is an addition from the original PR applied right? How does this fix the test failures?

Copy link
Contributor Author

@aakanksha555 aakanksha555 Jul 31, 2025

Choose a reason for hiding this comment

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

We're removing the default value and making it truly optional.

IO.mapOptional("Entry", FileHdr.Entry, Hex64(0));
IO.mapOptional("SectionHeaderStringTable", FileHdr.SectionHeaderStringTable);

Expand Down
31 changes: 31 additions & 0 deletions llvm/test/tools/obj2yaml/ELF/eflags.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
## Check how obj2yaml dumps e_flags field.

--- !ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2MSB
Type: ET_EXEC
Machine: EM_SPARC32PLUS
Flags: [ [[FLAGS]] ]

# RUN: yaml2obj -DFLAGS="EF_SPARC_32PLUS " %s -o %t2
# RUN: obj2yaml %t2 | FileCheck %s --check-prefix=FLAG

# FLAG: --- !ELF
# FLAG-NEXT: FileHeader:
# FLAG-NEXT: Class: ELFCLASS64
# FLAG-NEXT: Data: ELFDATA2MSB
# FLAG-NEXT: Type: ET_EXEC
# FLAG-NEXT: Machine: EM_SPARC32PLUS
# FLAG-NEXT: Flags: [ EF_SPARC_32PLUS ]

# RUN: yaml2obj -DFLAGS="EF_SPARC_HAL_R1 " %s -o %t3
# RUN: obj2yaml %t3 | FileCheck %s --check-prefix=FLAG2

# FLAG2: --- !ELF
# FLAG2-NEXT: FileHeader:
# FLAG2-NEXT: Class: ELFCLASS64
# FLAG2-NEXT: Data: ELFDATA2MSB
# FLAG2-NEXT: Type: ET_EXEC
# FLAG2-NEXT: Machine: EM_SPARC32PLUS
# FLAG2-NEXT: Flags: [ EF_SPARC_HAL_R1 ]
25 changes: 25 additions & 0 deletions llvm/test/tools/yaml2obj/file-header-flags.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
## Test for FileHeader Flags.

## When FLAGS variable isn't defined, the e_flags value is 0.
## Otherwise, it's the specified value.

# RUN: yaml2obj %s -o %t
# RUN: llvm-readobj -h %t | FileCheck %s --check-prefixes=NO-FLAG

# RUN: yaml2obj %s -o %t -DFLAGS=[EF_SPARC_32PLUS]
# RUN: llvm-readobj -h %t | FileCheck %s --check-prefixes=FLAG

!ELF
FileHeader:
Class: ELFCLASS32
Data: ELFDATA2LSB
Type: ET_EXEC
Machine: EM_SPARC32PLUS
Flags: [[FLAGS=<none>]]

# NO-FLAG: Flags [ (0x0)
# NO-FLAG-NEXT: ]

# FLAG: Flags [ (0x100)
# FLAG-NEXT: EF_SPARC_32PLUS (0x100)
# FLAG-NEXT: ]
3 changes: 2 additions & 1 deletion llvm/tools/obj2yaml/elf2yaml.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ template <class ELFT> Expected<ELFYAML::Object *> ELFDumper<ELFT>::dump() {
Y->Header.Type = Obj.getHeader().e_type;
if (Obj.getHeader().e_machine != 0)
Y->Header.Machine = ELFYAML::ELF_EM(Obj.getHeader().e_machine);
Y->Header.Flags = Obj.getHeader().e_flags;
if (Obj.getHeader().e_flags != 0)
Y->Header.Flags = ELFYAML::ELF_EF(Obj.getHeader().e_flags);
Copy link
Contributor

@lamb-j lamb-j Jul 31, 2025

Choose a reason for hiding this comment

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

This line changed too it seems. We need a cast here now?

Copy link
Contributor Author

@aakanksha555 aakanksha555 Jul 31, 2025

Choose a reason for hiding this comment

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

This cast will ensure we generate a Flags output everytime.

Copy link
Collaborator

@jh7370 jh7370 Aug 4, 2025

Choose a reason for hiding this comment

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

This cast will ensure we generate a Flags output everytime.

This doesn't seem to make much sense to me. The line above it (e_flags != 0) will ensure we don't generate a Flags field if the value is 0 (and that is the behaviour we want).

So, what is the cast doing and could you give an example where the behaviour has changed with this case, please?

Copy link
Contributor Author

@aakanksha555 aakanksha555 Aug 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

With the old patch on a release build I found all lit tests passing but with assertions enabled it was causing a lot of tests to crash.
This is the assertion I saw -

void llvm::yaml::IO::processKeyWithDefault(const char*, ELFYAML::ELF_EF; Context = llvm::yaml::EmptyContext]: Assertion '!DefaultValue && "std::optional<T> shouldn't have a value!"' failed.
So, I made the Flags field fully optional like the Machine field by removing the default value in llvm/lib/ObjectYAML/ELFYAML.cpp
The asserts still fired even with this change.
Adding the ELFYAML::ELF_EF cast in elf2yaml.cpp was able to fix the asserts.

Y->Header.Entry = Obj.getHeader().e_entry;

// Dump sections
Expand Down