[PATCH] D71872: [yaml2obj/obj2yaml] - Add support for SHT_RELR sections.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 2 04:52:18 PST 2020
jhenderson added inline comments.
================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:444
+struct RelocationOffsetsSection : Section {
+ Optional<std::vector<llvm::yaml::Hex64>> Entries;
----------------
grimar wrote:
> MaskRay wrote:
> > Why not `RelrSection`? Introducing a new term `RelocationOffsets` may be more confusing.
> I've renamed to `RelrSection`. I guess we might want to rename `RelocationSection` to `RelSection` too for consistency.
`RelocationSection` refers to both SHT_REL and SHT_RELA sections, right? Renaming to `RelSection` implies it's just for the former, to me.
================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:113
typedef typename ELFT::Dyn Elf_Dyn;
+ using uintX_t = typename ELFT::uint;
----------------
Maybe you should just use typedef for consistency with the code above. I actually would find this harder to read as is if I read the whole block at once.
================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1455-1456
+ if (const auto *ROS = dyn_cast<ELFYAML::RelrSection>(C.get())) {
+ if (ROS->Entries && ROS->Content)
+ return "\"Entries\" and \"Content\" can't be used together";
----------------
`ROS` -> `RS`
(what does the 'O' stand for...?)
================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:4
+## Test how we create SHT_RELR sections for 64-bits LE targets.
+## Test that sh_entsize is 8.
+# RUN: yaml2obj --docnum=1 %s -o %t.le64
----------------
I don't think this comment line here is necessary. It's implicit, since you actually check it below. (Why not also say, "that the sh_link and sh_info is 0"?)
================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:34
+ Type: SHT_RELR
+ Flags: [ SHF_ALLOC ]
+ Entries: [ 0x00000000AABBCCDD, 0x00000000EEFF1122,
----------------
Is this important to the test?
================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:38
+
+## Test how we create SHT_RELR sections for 64-bits BE targets.
+# RUN: yaml2obj --docnum=2 %s -o %t.be64
----------------
Perhaps worth changing this comment to say "Test that the content of SHT_RELR sections for 64-bits BE targets is correct".
I might also avoid abbreviating LE/BE in the comments (i.e. use "little endian" and "big endian").
================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:61
+
+## Test how we create SHT_RELR sections for 32-bit LE targets.
+## Test that sh_entsize is 4.
----------------
Same comments as above for the 32-bit cases.
================
Comment at: llvm/test/tools/yaml2obj/ELF/relr-section.yaml:173
+ Flags: [ SHF_ALLOC ]
+ Entries: [ 0xAABBCCDDEEFF0011 ]
+
----------------
Maybe worth making this explicitly 1 greater than the max (and adding a similar test for the 32-bit max case).
================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:748
+ } else {
+ // Ignore. We are going to dump the data as a raw "Content" below.
+ consumeError(Relrs.takeError());
----------------
I'd write "We are going to dump the data as raw content below."
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71872/new/
https://reviews.llvm.org/D71872
More information about the llvm-commits
mailing list