[PATCH] D56569: [ObjectYAML][yaml2obj][ELF] Add basic support for dynamic entries

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 11 01:45:55 PST 2019


jhenderson added a comment.

On the hex versus decimal front, I think hex is the better way to go.



================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:94
+  ELF_DT Tag;
+  Optional<uint64_t> Val;
+  Optional<llvm::yaml::Hex64> Ptr;
----------------
Somewhat moot if you follow @ruiu's suggestion (which I support), but this should be an int64_t.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:60
+    IO &IO, ELFYAML::ELF_DT &Value) {
+#define ECase(X) IO.enumCase(Value, #X, ELF::X)
+  ECase(DT_NULL);
----------------
Where did you get this list from? It looks like you might have missed some that LLVM supports (e.g. the DT_RELR* tags). I'd recommend taking the list from the list in BinaryFormat/DynamicTags.def.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:95-98
+  ECase(DT_LOOS);
+  ECase(DT_HIOS);
+  ECase(DT_LOPROC);
+  ECase(DT_HIPROC);
----------------
I'm not sure these 4 really should be accepted, since they don't actually represent true dynamic tag values, but are rather markers.

That being said, I see that the equivalents in SHT_* are, so I'm okay with them being left in.


================
Comment at: llvm/test/tools/yaml2obj/dynamic-entry-missing-val.yaml:14
+
+# CHECK: Dynamic entry must have either Value OR Pointer specified
----------------
Is it possible to give more context to this message, e.g. Dynamic entry "DT_FLAGS"... or something similar that allows easier identification?


================
Comment at: llvm/test/tools/yaml2obj/dynamic-section.yaml:1
-# Ensures that dynamic section has sh_entsize correctly set
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-readobj -sections %t | FileCheck %s --check-prefix=SECTION
----------------
I think that there was nothing wrong with the original test. I'd like to be able to write dynamic sections with custom headers and contents, as well as with custom tag lists. You should have both tests, in my opinion.

On that note, bonus points if you can do a section definition of .dynamic (e.g. with custom sh_link field set), but with dynamic tags using this kind of format. I think relocation sections work like that?


================
Comment at: llvm/test/tools/yaml2obj/dynamic-section.yaml:15
+  - Tag: DT_DEBUG
+    Pointer: 0x1000
+# llvm-readobj requires some program headers to read .dynamic entries.
----------------
It would be good if these sort of things could be specified both with arbitrary hex values and with references to things, e.g. sections, so that they can be specified like:
```
DynamicEntries:
  - Tag: DT_STRTAB
    Pointer: .dynstr
```


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:388
+  {
+    // Ensure STN_UNDEF is present
+    Elf_Dyn NullEnt;
----------------
Copy/paste error?


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:389-391
+    Elf_Dyn NullEnt;
+    zero(NullEnt);
+    Dyn.push_back(NullEnt);
----------------
Why not create an explicit DT_NULL tag, rather than calling zero()?


================
Comment at: llvm/tools/yaml2obj/yaml2elf.cpp:740-741
+template <class ELFT> bool ELFState<ELFT>::hasDynamicEntries() const {
+  if (hasDynamicSymbols())
+    return true;
+
----------------
I'm unconvinced by this line here, unless you're going to auto-populate the dynamic section with tags such as DT_SYMTAB in this case.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56569/new/

https://reviews.llvm.org/D56569





More information about the llvm-commits mailing list