This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj/obj2yaml] - Add support for SHT_RELR sections.
ClosedPublic

Authored by grimar on Dec 25 2019, 1:32 AM.

Details

Summary

The encoded sequence of Elf*_Relr entries in a SHT_RELR section looks
like [ AAAAAAAA BBBBBBB1 BBBBBBB1 ... AAAAAAAA BBBBBB1 ... ]
i.e. start with an address, followed by any number of bitmaps. The address
entry encodes 1 relocation. The subsequent bitmap entries encode up to 63(31)
relocations each, at subsequent offsets following the last address entry.

More information is here:
https://github.com/llvm-mirror/llvm/blob/master/lib/Object/ELF.cpp#L272

This patch adds a support for these sections.

Diff Detail

Event Timeline

grimar created this revision.Dec 25 2019, 1:32 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar marked an inline comment as done.
grimar added inline comments.
llvm/lib/ObjectYAML/ELFYAML.cpp
1106

I wonder if we should call this field just "Values"?

grimar planned changes to this revision.Dec 25 2019, 11:51 PM

Going to improve test cases and add a forgotten obj2yaml test.

grimar updated this revision to Diff 235323.Dec 26 2019, 1:08 AM
  • Rename OffsetsAndBitmaps to Entries.
  • Improved, added tests.
MaskRay added inline comments.Dec 26 2019, 10:19 PM
llvm/include/llvm/ObjectYAML/ELFYAML.h
444

Why not RelrSection? Introducing a new term RelocationOffsets may be more confusing.

llvm/lib/ObjectYAML/ELFYAML.cpp
1106

I prefer Entries. 1) We already use Entries for some other sections. 2) Entries is the term used in the ELF spec proposal.

SHT_RELR: The section holds relative relocation entries without explicit
          addends or info, such as type Elf32_Relr for the 32-bit class of
          object files or type Elf64_Relr for the 64-bit class of object
          files. An object file may have multiple relocation sections. See
          ``Relocation'' below for details.
llvm/test/tools/obj2yaml/relr-section.yaml
21

To make examples more plausible: use even numbers. Odd numbers not preceded by an even number are invalid.

llvm/test/tools/yaml2obj/ELF/relr-section.yaml
2

This is a test to test -> Test

36

It seems the comments can be deleted.

The indentation of the continuation line is strange.

llvm/tools/obj2yaml/elf2yaml.cpp
748

Why can the error be swallowed?

grimar updated this revision to Diff 235399.Dec 27 2019, 2:02 AM
grimar marked 10 inline comments as done.
  • Addressed review comments.
llvm/include/llvm/ObjectYAML/ELFYAML.h
444

I've renamed to RelrSection. I guess we might want to rename RelocationSection to RelSection too for consistency.

llvm/lib/ObjectYAML/ELFYAML.cpp
1106

Yeah. I initially called this field "OffsetsAndBitmaps", suggested "Values", but then renamed to "Entries" because of the same reasons you mention.

llvm/test/tools/obj2yaml/relr-section.yaml
21

Odd numbers not preceded by an even number are invalid.

Sure, but in obj2yaml we do not need to care about such things. Moreover, we probably want to check
exactly this cases, because one of the tasks of obj2yaml is to produce the output from any given object.
Object can be valid or can be not. We only want to show that we use "Entries" when data size % 64(32) == 0.

The same applies for yaml2obj probably.

llvm/tools/obj2yaml/elf2yaml.cpp
748

If we can't read the content as array of ELF_Relr, we fall back to read it as an array of uint8_ts.
We do not need to trigger any errors, because while we are able to dump it at least with use of "Content",
we are fine. If we can't do that, the appropriate error will be reported below.

I could just read the whole content as array from the begining and then write checks, like "if (Size % sizeof(uintX_t) != 0)",
and then read values manually, but I supposed the code looks a bit simpler in this way.

MaskRay accepted this revision.Dec 27 2019, 10:06 AM

This is new code.. Perhaps give @jhenderson a chance to review if you are not in a hurry.

llvm/lib/ObjectYAML/ELFEmitter.cpp
839

the parentheses around uint64_t can be deleted.

846

Hex64 is a small, trivially copyable type. Consider dropping const &.

llvm/tools/obj2yaml/elf2yaml.cpp
745

Nit: when there is a choice between push_back and emplace_back with the same arguments, push_back may be more readable, because push_back expresses the intent more specifically.

For example, for std::vector<std::vector<int>> a, it accepts a.emplace_back(10) but not a.push_back(10). push_back makes the reader think less.

This revision is now accepted and ready to land.Dec 27 2019, 10:06 AM
grimar updated this revision to Diff 235506.Dec 29 2019, 4:13 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
llvm/tools/obj2yaml/elf2yaml.cpp
745

I can't use push_back here:

Error	C2664	'void std::vector<llvm::yaml::Hex64,std::allocator<_Ty>>::push_back(_Ty &&)':
cannot convert argument 1 from 'llvm::support::detail::packed_endian_specific_integral<uint64_t,llvm::support::little,1,1>'
to 'const _Ty &'	obj2yaml	D:\Work3\LLVM\llvm-project\llvm\tools\obj2yaml\elf2yaml.cpp	745

It happens because Entries is a std::vector<llvm::yaml::Hex64>.
I need to insert Elf_Relr, which is packed<uint>.
emplace_back is needed to use Hex64's constructor: Hex64(const uint64_t v)

MaskRay accepted this revision.Dec 29 2019, 5:36 PM
jhenderson added inline comments.Jan 2 2020, 4:50 AM
llvm/include/llvm/ObjectYAML/ELFYAML.h
444

RelocationSection refers to both SHT_REL and SHT_RELA sections, right? Renaming to RelSection implies it's just for the former, to me.

llvm/lib/ObjectYAML/ELFEmitter.cpp
113

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.

llvm/lib/ObjectYAML/ELFYAML.cpp
1455–1456

ROS -> RS

(what does the 'O' stand for...?)

llvm/test/tools/yaml2obj/ELF/relr-section.yaml
5

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"?)

35

Is this important to the test?

39

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").

62

Same comments as above for the 32-bit cases.

174

Maybe worth making this explicitly 1 greater than the max (and adding a similar test for the 32-bit max case).

llvm/tools/obj2yaml/elf2yaml.cpp
748

I'd write "We are going to dump the data as raw content below."

grimar updated this revision to Diff 237927.Jan 14 2020, 4:00 AM
grimar marked 15 inline comments as done.
  • Addressed review comments.
llvm/lib/ObjectYAML/ELFEmitter.cpp
113

OK. I think I had an oppsoite comments before (not from you). About switching to "using".
That is why I did it.
I think we can change the whole block to using at once later.

llvm/lib/ObjectYAML/ELFYAML.cpp
1455–1456

RelOcationSection I guess... fixed.

llvm/test/tools/yaml2obj/ELF/relr-section.yaml
5

Mostly it was done because I set sh_entsize in the code of this patch.
I find your comment valid though, so I've removed this line.

35

Why not? It makes sence to show we do not drop a flag specified.
Indeed it is not a full test for this bit, but it tests something and is better than nothing.
(I do not feel we want to test much more than that atm).

Your comment was usefull for other tests below though, where I've removed the Flags field.

39

Done.

FTR. I am not sure about LE/BE -> "little endian" and "big endian" conversion,
because I think that LE/BE are common abbreviations.

174

Done.

jhenderson added inline comments.Jan 14 2020, 4:37 AM
llvm/lib/ObjectYAML/ELFEmitter.cpp
113

Right. I agree that in general using is easier to read, so I'd certainly support switching over wholesale in this area at another point.

llvm/test/tools/yaml2obj/ELF/relr-section.yaml
4

64-bits -> 64-bit

(a target is described as being a 64/32-bit target - other places 64-bits would be correct).

35

Indeed it is not a full test for this bit, but it tests something and is better than nothing.

Okay. I think it needs a comment to explain why it is tested then. Perhaps something along the lines of "Show that relr section dumping maintains common section properties such as flags." (there may be a clearer or more concise phrasing)

grimar updated this revision to Diff 238187.Jan 15 2020, 1:04 AM
grimar marked 5 inline comments as done.
  • Addressed review comments.
llvm/test/tools/yaml2obj/ELF/relr-section.yaml
4

(I hope I got it right)

Fixed here and in 3 more similar places in this test.
I've kept other comments, particularry one in obj2yaml/relr-section.yaml
("Test how we dump SHT_RELR sections for 32 and 64-bits targets.") unchanged.

35

Perhaps something along the lines of "Show that relr section dumping maintains common section properties such as flags." (there may be a clearer or more concise phrasing)

It is not about a dumping probably? We are testing yaml2obj here.
I've added four "## Set an arbitrary flag to demonstrate it is not dropped." comments around.
Does it look OK for you?

jhenderson added inline comments.Jan 15 2020, 1:31 AM
llvm/test/tools/obj2yaml/relr-section.yaml
2

Please update this 64-bits -> 64-bit too.

llvm/test/tools/yaml2obj/ELF/relr-section.yaml
4

After thinking about it yesterday, I think I figured out the rule. When 32/64-bit is used as an adjective followed by a noun (e.g. "This is a 64-bit target"), it shouldn't have the 's', whereas when it is used without a following noun (e.g. "This target is 64-bits"), it should.

Aside from the obj2yaml/relr-section.yaml case (which should be updated), I couldn't find any other 64-bits usage in comments in this patch. Which were the others you were referring to?

35

I'd change "it is not dropped" -> "flags are set when requested" perhaps. Otherwise, looks good.

grimar updated this revision to Diff 238195.Jan 15 2020, 1:58 AM
grimar marked 3 inline comments as done.
  • Addressed review comments.
llvm/test/tools/yaml2obj/ELF/relr-section.yaml
4

After thinking about it yesterday, I think I figured out the rule. When 32/64-bit is used as an adjective followed by a noun (e.g. "This is a 64-bit target"), it shouldn't have the 's', whereas when it is used without a following noun (e.g. "This target is 64-bits"), it should.

Interesting. I am usually simply thinking about "s" as about suffix for a plural form of a noun. In Russian the correct spelling for a simple straightforward translation of "64-bit/64-bits" would be different for "target" and "targets" cases.
(In the "Test that the content of SHT_RELR sections for 64-bit little endian targets is correct." sentence I mean).
That is where my confuson here comes from I think.

Aside from the obj2yaml/relr-section.yaml case (which should be updated), I couldn't find any other 64-bits usage in comments in this patch. Which were the others you were referring to?

Nope. I wasn't sure what you meant by "other places" and only tried to point to places I found which I did (or did not) change :)

jhenderson accepted this revision.Jan 15 2020, 2:04 AM

LGTM!

llvm/test/tools/yaml2obj/ELF/relr-section.yaml
4

English is hard :)

This revision was automatically updated to reflect the committed changes.