This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf][llvm-readobj] - Reimplement the logic of section flags dumping.
ClosedPublic

Authored by grimar on Dec 13 2019, 5:23 AM.

Details

Summary

Our logic that dumped the flags was buggy.

For LLVM style it dumped SHF_MASKPROC/SHF_MASKOS named constants, though
they are not flags, but masks.

For GNU style it was just very inconsistent with GNU which has logic
that is not straightforward. Imagine we have sh_flags == 0x90000000.
SHF_EXCLUDE ("E") has a value of 0x80000000 and SHF_MASKPROC is 0xf0000000.
GNU readelf will not print "E" or "Ep" in this case, but will print just
"p". It only will print "E" when no other processor flag is set.
I had to investigate the GNU source to find the algorithm and now our logic should
match it.

Diff Detail

Event Timeline

grimar created this revision.Dec 13 2019, 5:23 AM

I don't see any testing for the 'x' flag printing. I think it makes sense to add it here, since the behaviour could be impacted by this patch.

llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test
128–129

This comment probably belongs with the YAML below.

I'd change the last part of the sentence as follows:

"when have this bits set." -> "when bits in the OS and processor specific ranges are set."

133–134

I'd change as follows:
"GNU readelf does not print "E", it drops this bits, because handles SHF_MASKPROC mask first" -> "GNU readelf does not necessarily print "E", because it handles the SHF_MASKPROC mask first"

148

What is this test case trying to show?

160

Same as above. What is this case trying to show?

179

Same as above. What is this case actually trying to show?

200

What should GNU output do for a MIPS object with SHF_EXCLUDE set, since that value is also a used processor-specific one?

llvm/tools/llvm-readobj/ELFDumper.cpp
1497

It's entirely possible I'm misreading the code (I hate trying to follow bit-wise arithmetic), but isn't this line just a duplicate of the Flags &= ~Flag; above?

1502

a OS -> an OS

1509

It took me quite a long time to identify how this algorithm achieves the stated aim in the comment, and I think it's because this block doesn't make clear in comments that the SHF_EXCLUDE flag is masked off by the masking below. Perhaps a comment here would be a good idea to clarify. Something along the lines of "Mask off all the processor-specific bits. This removes the SHF_EXCLUDE bit if set so that it doesn't also get printed."

grimar updated this revision to Diff 234024.Dec 16 2019, 4:01 AM
grimar marked 13 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test
148

I wanted to show how we handle the value that is (SHF_MASKOS - 1).
I supposed it is reasonable, because we might have some logic (and we actually have it)
that works with SHF_MASKOS in the code. Hence I tried to check the bounds: a minimal possible value,
the mask itself and something that is almost large as SHF_MASKOS, but != SHF_MASKOS.

Do you think it is excessive?

160

The same, it is SHF_MASKPROC - 1.

179

It's just both SHF_MASKPROC - 1 and SHF_MASKOS - 1 together.
for consistency of .both.flags* and .proc.flags*/.os.flags* tests.

This might test we drop the bits that belong to the right mask probably.

200

If we have:

--- !ELF
FileHeader:
  Class:   ELFCLASS32
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_MIPS
Sections:
  - Name:  .mips
    Type:  SHT_PROGBITS
    Flags: [ SHF_MIPS_STRING ]

Then both GNU readelf and llvm-readelf print "E". I've added a test case to demonstrate.

llvm/tools/llvm-readobj/ELFDumper.cpp
1497

Right! I was expermenting with this code alot and seems forgot to remove this line.

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

an MIPS -> a MIPS
has the SHF_MIPS_STRING

132

proccessor -> processor

148

I think it's okay to have one not on the boundaries, but I'd change the naming of this and the similar issues elsewhere, since "max" implies it is trying to be the maximum OS-specific flag value, which is not the case (that would be 0xff00000).

183–186

This should probably be in the regular section-flags.test.

I'm not actually sure if any of this test case belongs in this test, as it's not about the architecture really. How about moving it into a new test (e.g. section-flags-os-proc.test)?

203

arbitraty -> arbitrary

jhenderson added inline comments.Dec 16 2019, 5:42 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
1502

proccessor -> processor

grimar updated this revision to Diff 234049.Dec 16 2019, 6:32 AM
grimar marked 7 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/section-arch-flags.test
148

I've used "low" and "high" suffixes instead.

183–186

This should probably be in the regular section-flags.test.

OK.

I'm not actually sure if any of this test case belongs in this test, as it's not about the architecture really. How about moving it into a new test (e.g. section-flags-os-proc.test)?

OK, I've also added one more test (.all.possible) there too.

jhenderson added inline comments.Dec 16 2019, 7:18 AM
llvm/test/tools/llvm-readobj/ELF/section-flags-os-proc.test
1 ↗(On Diff #234049)

Perhaps a top-level comment explaining that this tests the dumping of flags in the respective regions would be a good idea.

110–118 ↗(On Diff #234049)

I'd rearrange and rename these to be consistent with the previous os and proc ones above:

- Name: .both.flags.low
- Name: .both.flags.high
- Name: .both.flags.mask
llvm/test/tools/llvm-readobj/ELF/section-flags.test
175 ↗(On Diff #234049)

.unknown -> unknown for consistency with the other sections.

MaskRay added inline comments.Dec 16 2019, 10:28 PM
llvm/test/tools/llvm-readobj/ELF/section-flags-os-proc.test
90 ↗(On Diff #234049)

Or use EM_NONE

llvm/tools/llvm-readobj/ELFDumper.cpp
1490

Flags -= Flag;

grimar updated this revision to Diff 234232.Dec 17 2019, 12:44 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/section-flags-os-proc.test
90 ↗(On Diff #234049)

Nice! I've forgot about EM_NONE.

jhenderson accepted this revision.Dec 17 2019, 1:20 AM

LGTM, with one remaining nit.

llvm/test/tools/llvm-readobj/ELF/section-flags-os-proc.test
5 ↗(On Diff #234232)

Shouldn't this be above the corresponding RUNs?

This revision is now accepted and ready to land.Dec 17 2019, 1:20 AM
This revision was automatically updated to reflect the committed changes.