Page MenuHomePhabricator

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

Authored by grimar on Oct 2 2019, 6:37 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Oct 2 2019, 6:37 AM
MaskRay added inline comments.Oct 2 2019, 7:57 AM
include/llvm/ObjectYAML/ELFYAML.h
260 ↗(On Diff #222818)

{}; -> {}

test/tools/yaml2obj/elf-llvm-addrsig-section.yaml
199 ↗(On Diff #222818)

Right shift 123{{$}}

jhenderson added inline comments.Oct 2 2019, 8:19 AM
include/llvm/ObjectYAML/ELFYAML.h
260–262 ↗(On Diff #222818)

clang-format?

263 ↗(On Diff #222818)

I'd find it easier to read if there were a blank line between the methods and member variables.

test/tools/obj2yaml/elf-llvm-addrsig-section.yaml
4 ↗(On Diff #222818)

dumps -> dumping

5 ↗(On Diff #222818)

when it can't match

I'd delete "found"

56 ↗(On Diff #222818)

Delete "malformed"

57–58 ↗(On Diff #222818)

broken, e.g. because the entry contains a malformed

61 ↗(On Diff #222818)

I might use the prefix "INVALID-ENTRY"

79 ↗(On Diff #222818)

obj2yaml produces a "Symbols" tag when describing an ...

test/tools/yaml2obj/elf-llvm-addrsig-section.yaml
3 ↗(On Diff #222818)

using the "Symbols" tag

117 ↗(On Diff #222818)

I'm not really sure what "Symbol"->"Index" is trying to say. Perhaps it could be written as follows: "Check that the maximum symbol index size is 32 bits."

This actually shows that an error is produced if the value is out of range. You should have a separate case without the error, using the maximum possible value.

135 ↗(On Diff #222818)

the "Content" tag

158 ↗(On Diff #222818)

"for SHT_LLVM_ADDRSIG sections." here and elsewhere.

tools/obj2yaml/elf2yaml.cpp
865 ↗(On Diff #222818)

Whilst you're moving code around, I'd improve this comment slightly:
"Get symbol with index sh_info. This symbol's name is the signature of the group.

grimar updated this revision to Diff 222988.Oct 3 2019, 5:25 AM
grimar marked 17 inline comments as done.
  • Addressed review comments.
include/llvm/ObjectYAML/ELFYAML.h
260–262 ↗(On Diff #222818)

(It was formatted.. but because of excessive ';' which I've removed, formatting did't add a space between ')' and '{').

test/tools/yaml2obj/elf-llvm-addrsig-section.yaml
117 ↗(On Diff #222818)

This actually shows that an error is produced if the value is out of range. You should have a separate case without the error, using the maximum possible value.

I've included it to SYMBOL-INDEX test.

This revision is now accepted and ready to land.Oct 3 2019, 6:05 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 6:55 AM
RKSimon added a subscriber: RKSimon.Oct 3 2019, 8:12 AM

@grimar elf-llvm-addrsig-section.yaml is failing on EXPENSIVE_CHECKS builds please can you take a look?

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/19989/

grimar added a comment.Oct 3 2019, 8:15 AM

@grimar elf-llvm-addrsig-section.yaml is failing on EXPENSIVE_CHECKS builds please can you take a look?

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/19989/

Yep, sorry for this. I`ve reverted it and recommitted with a fix in rL373606 recently. Should be fixed now.