Reapply "Allow "[[FLAGS=<none>]]" value in the ELF Fileheader Flags field (#143845)" #151094
Conversation
…ield (llvm#143845)" This fixes the issues with 0b054e2 This reverts commit b80ce05.
|
@llvm/pr-subscribers-objectyaml Author: Aakanksha Patil (aakanksha555) ChangesThis fixes the issues with 0b054e2 This reverts commit b80ce05. Full diff: https://github.com/llvm/llvm-project/pull/151094.diff 6 Files Affected:
diff --git a/llvm/include/llvm/ObjectYAML/ELFYAML.h b/llvm/include/llvm/ObjectYAML/ELFYAML.h
index e883f2f3e1447..3bf8c29c99581 100644
--- a/llvm/include/llvm/ObjectYAML/ELFYAML.h
+++ b/llvm/include/llvm/ObjectYAML/ELFYAML.h
@@ -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;
diff --git a/llvm/lib/ObjectYAML/ELFEmitter.cpp b/llvm/lib/ObjectYAML/ELFEmitter.cpp
index 6de87a88d0060..bc5c68d08d11f 100644
--- a/llvm/lib/ObjectYAML/ELFEmitter.cpp
+++ b/llvm/lib/ObjectYAML/ELFEmitter.cpp
@@ -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)
diff --git a/llvm/lib/ObjectYAML/ELFYAML.cpp b/llvm/lib/ObjectYAML/ELFYAML.cpp
index 7fcabb684e87f..c27339de67efd 100644
--- a/llvm/lib/ObjectYAML/ELFYAML.cpp
+++ b/llvm/lib/ObjectYAML/ELFYAML.cpp
@@ -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);
IO.mapOptional("Entry", FileHdr.Entry, Hex64(0));
IO.mapOptional("SectionHeaderStringTable", FileHdr.SectionHeaderStringTable);
diff --git a/llvm/test/tools/obj2yaml/ELF/eflags.yaml b/llvm/test/tools/obj2yaml/ELF/eflags.yaml
new file mode 100644
index 0000000000000..da16a62c04a5b
--- /dev/null
+++ b/llvm/test/tools/obj2yaml/ELF/eflags.yaml
@@ -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 ]
diff --git a/llvm/test/tools/yaml2obj/file-header-flags.yaml b/llvm/test/tools/yaml2obj/file-header-flags.yaml
new file mode 100644
index 0000000000000..baa101af013c9
--- /dev/null
+++ b/llvm/test/tools/yaml2obj/file-header-flags.yaml
@@ -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: ]
diff --git a/llvm/tools/obj2yaml/elf2yaml.cpp b/llvm/tools/obj2yaml/elf2yaml.cpp
index 53455b8c7580a..ab15553df1e84 100644
--- a/llvm/tools/obj2yaml/elf2yaml.cpp
+++ b/llvm/tools/obj2yaml/elf2yaml.cpp
@@ -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);
Y->Header.Entry = Obj.getHeader().e_entry;
// Dump sections
|
|
Ping. |
| IO.mapRequired("Type", FileHdr.Type); | ||
| IO.mapOptional("Machine", FileHdr.Machine); | ||
| IO.mapOptional("Flags", FileHdr.Flags, ELFYAML::ELF_EF(0)); | ||
| IO.mapOptional("Flags", FileHdr.Flags); |
There was a problem hiding this comment.
This is an addition from the original PR applied right? How does this fix the test failures?
There was a problem hiding this comment.
We're removing the default value and making it truly optional.
| 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); |
There was a problem hiding this comment.
This line changed too it seems. We need a cast here now?
There was a problem hiding this comment.
This cast will ensure we generate a Flags output everytime.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Both of the above changes helped fix the assertion crashes being encountered. |
4 participants
This fixes the issues with 0b054e2
This reverts commit b80ce05.