Page MenuHomePhabricator

[yaml2obj][obj2yaml] - Add support for dumping/parsing .dynamic sections.
ClosedPublic

Authored by grimar on Feb 4 2019, 7:58 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Feb 4 2019, 7:58 AM
jhenderson added a subscriber: amontanez.EditedFeb 4 2019, 8:22 AM

@amontanez has already put something similar up for review here as D56569.

There was a corresponding llvm-dev email thread to discuss it further here:
http://lists.llvm.org/pipermail/llvm-dev/2019-January/129231.html

which didn't get much traction.

@amontanez has already put something similar up for review here as D56569.

There was a corresponding llvm-dev email thread to discuss it further here:
http://lists.llvm.org/pipermail/llvm-dev/2019-January/129231.html

which didn't get much traction.

Interesting. I did not see it. D56569 seems to do a lot of different things.

FTR, my main intentions were the following:

  1. I looked how to dump/parse ELF versioning sections and found we sometimes might want to access

the dynamic tags values. For example:

DT_VERDEFNUM The number of entries in DT_VERDEF.
DT_VERNEEDNUM
The number of entries in DT_VERNEED.

  1. I noticed that some of our tests (For example, "binary-read-bad-vaddr.test" and lots of others improved by this patch)

want to see the particular tags and values and there was no way to do that (without editing the raw binary section content).

So I decided to implement the simple solution that would allow accessing and changing the dynamic entries from yaml and the code.

grimar added a reviewer: ruiu.Feb 5 2019, 12:09 AM
ruiu added a comment.Feb 7 2019, 4:42 PM

I think this is a good change -- it seems like it does the right thing and works as a good starter to add more fields. James, are you okay with this change?

This change basically looks fine in itself. From what I remember of the discussion around D56569, there isn't anything here in the behaviour that conflicts with that change, I believe.

lib/ObjectYAML/ELFYAML.cpp
677 ↗(On Diff #185055)

I may be missing something, but why is this line different to the equivalent for e.g. the relocations above?

test/tools/llvm-readobj/gnu-hash-symbols.test
77 ↗(On Diff #185055)

Not that it really matters, but any particular reason you've moved this to before the Link field?

grimar marked 2 inline comments as done.Feb 8 2019, 2:26 AM
grimar added inline comments.
lib/ObjectYAML/ELFYAML.cpp
677 ↗(On Diff #185055)

See how relocations are declared. For example in ELFRelocs/RISCV.def:

ELF_RELOC(R_RISCV_NONE,               0)
ELF_RELOC(R_RISCV_32,                 1)
...

But dynamic tags are refined without prefix DT_*:

DYNAMIC_TAG(NULL, 0)        // Marks end of dynamic array.
DYNAMIC_TAG(NEEDED, 1)      // String table offset of needed library.

ELF.h declares an enum like that:

enum {
#define DYNAMIC_TAG(name, value) DT_##name = value,
#include "DynamicTags.def"
#undef DYNAMIC_TAG
};

(https://github.com/llvm-mirror/llvm/blob/master/include/llvm/BinaryFormat/ELF.h#L1242)

Do I had to use ELF::DT_##X.

The reason I used STRINGIFY and STRINGIFY(DT_##X) is that I think we should dump
tags as DT_TAGNAME and not just TAGNAME because:

  1. That is consistent with how we dump relocations (we dump them with R_* prefix.
  2. Dumping just a tag name does not look clear to me. DT_NEEDED looks better than NEEDED IMO.

DT_NULL is more clear than just NULL and so on.

test/tools/llvm-readobj/gnu-hash-symbols.test
77 ↗(On Diff #185055)

No. I had to use yaml2obj with the old YAMLs to convert then objects to a new YAML format (to update the tests),
so seems it just the natural order of the fields now (i.e. obj2yaml writes them in this order). I did not try to swap the lines intentionally and/or manually.

jhenderson accepted this revision.Feb 8 2019, 2:28 AM

Okay, LGTM.

This revision is now accepted and ready to land.Feb 8 2019, 2:28 AM

Oof, my email filters somehow didn't send the discussion here to my inbox. Sorry! I'm fine with this landing as-is, I can just rebase on top of it. I think the only significant deviance from the RFC is future improvements require that DynamicEntry::Val is a StringRef instead of llvm::yaml::Hex64, but I can address that later.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2019, 3:38 AM