This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf] - Change letters used for SHF_ARM_PURECODE and SHF_X86_64_LARGE flags.
ClosedPublic

Authored by grimar on Dec 13 2019, 6:34 AM.

Details

Summary

GNU uses l for SHF_X86_64_LARGE and y for SHF_ARM_PURECODE.
Lets follow.

To do this I had to refactor and refine how we print the help flags description.
It was too generic and inconsistent with GNU readelf.

Depends on https://reviews.llvm.org/D71462

Diff Detail

Event Timeline

grimar created this revision.Dec 13 2019, 6:34 AM
grimar planned changes to this revision.Dec 13 2019, 6:42 AM

I've found that we also seems need to update "Key to Flags:" printed

For example. when SHF_ARM_PURECODE is used, GNU readelf prints:

Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
  L (link order), O (extra OS processing required), G (group), T (TLS),
  C (compressed), x (unknown), o (OS specific), E (exclude),
  y (purecode), p (processor specific)

When SHF_X86_64_LARGE is used, it prints:

Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
  L (link order), O (extra OS processing required), G (group), T (TLS),
  C (compressed), x (unknown), o (OS specific), E (exclude),
  l (large), p (processor specific)

I am doing to update the patch with this change.

MaskRay added inline comments.Dec 15 2019, 8:50 PM
llvm/tools/llvm-readobj/ELFDumper.cpp
1504

Str += 'l'

1508

SHF_ARM_PURECODE

grimar updated this revision to Diff 234034.Dec 16 2019, 5:25 AM
grimar marked 3 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Addressed comments.
  • Fixed the flags description printed.
llvm/tools/llvm-readobj/ELFDumper.cpp
1508

I've removed these lines. They are not needed actually because of Flags &= ~Flag.

jhenderson added inline comments.Dec 16 2019, 5:43 AM
llvm/test/tools/llvm-readobj/ELF/gnu-sections.test
1

section and flag descriptions

4

I'd probably delete the phrase "help text" here as it could be confused with the output of --help, and is unnecessary ("key" already implies its a descriptive piece of help text).

I'd also rephrase slightly:
"to show how the default flag key is printed."

7

--check-prefix here and below.

61

a EM_X86_64 -> an EM_X86_64
for the SHF_X86_64_LARGE

62

Check we mention it in the flag key.

127

a EM_ARM -> an EM_ARM
for the SHF_ARM_PURECODE

128

Same comment as above.

llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test
101

Why does it look strange? (Also "Despite the fact...")

grimar marked an inline comment as done.Dec 16 2019, 5:49 AM
grimar added inline comments.
llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test
101

Because we print o, p and x last, so one might think that the order is UPPER CASE letters and lower case after them.
But lE violates this.

jhenderson added inline comments.Dec 16 2019, 5:59 AM
llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test
101

Okay, I'd change this statement to simply say something like:

"GNU prints 'l' before 'E', despite otherwise printing upper-case letters first".

grimar updated this revision to Diff 234057.Dec 16 2019, 6:59 AM
grimar marked 9 inline comments as done.
  • Addressed review comments.
jhenderson accepted this revision.Dec 16 2019, 7:20 AM

LGTM, except for one comment.

llvm/test/tools/llvm-readobj/ELF/gnu-sections.test
2

If this should be interpreted as "section descriptions and flag descriptions", then sections -> section.

This revision is now accepted and ready to land.Dec 16 2019, 7:20 AM
MaskRay accepted this revision.Dec 16 2019, 8:38 AM
This revision was automatically updated to reflect the committed changes.