Page MenuHomePhabricator

[llvm-readelf] Render messages similar to that of `GNU binutils readelf` when no sections and/or no headers.
ClosedPublic

Authored by Prabhuk on Jul 27 2022, 4:14 PM.

Details

Summary

When there are no section headers section information printed by llvm-readelf is not useful and unnecessarily verbose. When there are no program headers there's a similar verbose output shown when section mapping is requested. Simplifying the message shown in these cases to match the behavior of GNU binuntils readelf.

Diff Detail

Event Timeline

Prabhuk created this revision.Jul 27 2022, 4:14 PM
Herald added a project: Restricted Project. · View Herald Transcript
Prabhuk requested review of this revision.Jul 27 2022, 4:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2022, 4:14 PM
MaskRay accepted this revision.Jul 27 2022, 4:26 PM

LGTM.

This revision is now accepted and ready to land.Jul 27 2022, 4:26 PM
jhenderson requested changes to this revision.Jul 28 2022, 12:06 AM

Behaviour change is generally fine, but there are a couple of test changes/additions required.

llvm/test/tools/llvm-readobj/ELF/many-sections.s
45

Isn't this warning still expected to be emitted?

llvm/tools/llvm-readobj/ELFDumper.cpp
4085–4088

Seems like this case isn't tested?

This revision now requires changes to proceed.Jul 28 2022, 12:06 AM
Prabhuk updated this revision to Diff 448669.EditedJul 29 2022, 10:05 AM

Added tests that will cover the printSectionDetails() function no section header use cases.
Rendering the warning e_shstrndx == SHN_XINDEX, but the section header table is empty along with the There is no sections in the file. message.

Prabhuk added inline comments.Jul 29 2022, 10:08 AM
llvm/test/tools/llvm-readobj/ELF/many-sections.s
45

Rendering the warning in this case and bringing back the warning message test to the test file.

llvm/tools/llvm-readobj/ELFDumper.cpp
4085–4088

Thank you. Added tests to cover this path.

jhenderson added inline comments.Aug 8 2022, 2:16 AM
llvm/test/tools/llvm-readobj/ELF/gnu-section-mapping-no-phdrs.test
3–6 ↗(On Diff #448669)

As we're changing this test anyway, I think it would be good to show that no other output is printed before or after, not just after the checked line. The easiest way to do this is to add --implicit-check-not={{.}} to the FileCheck command and then just get rid of the existing CHECK-NOT line.

llvm/test/tools/llvm-readobj/ELF/many-sections.s
11

I'm not sure I follow what coverage this line is adding? We need a test case with --section-details but no sections, but you've added this to no-shdrs.test.

llvm/test/tools/llvm-readobj/ELF/no-phdrs.test
15

I think we need to show that the section segment mapping is not printed in this case. I'm thinking something like GNU-NOT: {{.}} or better yet, --implicit-check-not={{.}} should do the trick.

llvm/tools/llvm-readobj/ELFDumper.cpp
3697
3701–3702

We need a test case that shows this warning being emitted.

4085
4089–4090

Ditto: this warning needs checking, I believe.

4275–4276

I feel like this function could do with some early outs. I'm thinking something along the following lines would be much cleaner:

template <class ELFT>
void GNUELFDumper<ELFT>::printProgramHeaders(
    bool PrintProgramHeaders, cl::boolOrDefault PrintSectionMapping) {
  if (!PrintProgramHeaders && PrintSectionMapping == cl::BOU_FALSE)
    return;

  const Elf_Ehdr &Header = this->Obj.getHeader();
  if (Header.e_phnum == 0) {
    OS << "\nThere are no program headers in this file.\n";
    return;
  }

  if (PrintProgramHeaders)
    printProgramHeaders();
  if (PrintSectionMapping != cl::BOU_FALSE)
    printSectionMapping();
}

Thoughts?

Prabhuk updated this revision to Diff 452862.Aug 15 2022, 5:58 PM
Prabhuk marked 7 inline comments as done.

Program header rendering cleanup.

Rendering "no program header" message should not affect readelf invocations that request for section information only. Updated the PrintProgramHeader function and the corresponding tests.

Prabhuk updated this revision to Diff 452864.Aug 15 2022, 6:15 PM

Ran clang-format to fix formatting mismatches.

Updated the printProgramHeaders section to address the review comment and to fix logic flaws. Updated corresponding tests. Please take a look.

llvm/test/tools/llvm-readobj/ELF/gnu-section-mapping-no-phdrs.test
3–6 ↗(On Diff #448669)

This change as it turns out is wrong. The code path that renders only "--section-mapping" is not originally intended to change.

llvm/test/tools/llvm-readobj/ELF/many-sections.s
11

You're right. This is a duplicate check of what is already handled in section-details.test. Removing this test.

llvm/test/tools/llvm-readobj/ELF/no-phdrs.test
15

Once again there was an error in this work flow. Clean up of "Program Headers" section was intended. But the Section to Segment mapping was not intended to be changed so bringing that part back.

llvm/tools/llvm-readobj/ELFDumper.cpp
3701–3702

test/tools/llvm-readobj/ELF/many-sections.s line 36,37 (shown below) exercises this code path.

  1. RUN: llvm-readelf --file-headers --sections %t2 2>&1 | \
  2. RUN: FileCheck %s -DFILE=%t2 --check-prefix=GNU2
4089–4090

Added a new test to cover this path:

test/tools/llvm-readobj/ELF/many-sections.s line 38,39 (shown below) :

  1. RUN: llvm-readelf --file-headers --section-details %t2 2>&1 | \
  2. RUN: FileCheck %s -DFILE=%t2 --check-prefix=GNU2
4275–4276

This comment helped me find a logical flaw in rendering. The control flow of invocations that request for segment details/mapping without the program header info should not change. I have updated this function to reflect this change and considered some of the cleanups suggested in your comment.

FWIW ELF Tool Chain readelf also emits There are no program headers in this file. or There are no sections in this file..

jhenderson added inline comments.Aug 16 2022, 12:09 AM
llvm/test/tools/llvm-readobj/ELF/no-phdrs.test
15

So just to confirm, GNU prints the (empty) section to segment mapping even if there are no program headers, when the program headers are dumped?

llvm/test/tools/llvm-readobj/ELF/no-shdrs.test
20
llvm/tools/llvm-readobj/ELFDumper.cpp
4292–4294

No need for braces in single-line if statements. Also, I'd rename IsPrintSectionMapping to ShouldPrintSectionMapping, as it is more grammatically correct.

4295

I'm not sure this and the equivalent comment below for the section mapping are really adding anything anymore, given the new code layout.

4305–4307
Prabhuk updated this revision to Diff 453046.Aug 16 2022, 9:38 AM

Addressed review comments (var name, comment removal, code style, alignment).

Prabhuk updated this revision to Diff 453047.Aug 16 2022, 9:40 AM

Removing braces for If block with single line code.

Prabhuk updated this revision to Diff 453048.Aug 16 2022, 9:42 AM

Including clang-format fixes.

Prabhuk marked 6 inline comments as done.Aug 16 2022, 9:43 AM
Prabhuk added inline comments.
llvm/test/tools/llvm-readobj/ELF/no-phdrs.test
15

That's correct! When we ask to dump program headers we dump section to segment mapping by default. Showing empty mapping offers users consistency on what to expect in the output printed to the console imo. But I could go either way (show empty mapping or not) to be honest.

jhenderson accepted this revision.Aug 16 2022, 11:55 PM

Looks good to me.

This revision is now accepted and ready to land.Aug 16 2022, 11:55 PM
This revision was landed with ongoing or failed builds.Aug 17 2022, 9:51 AM
This revision was automatically updated to reflect the committed changes.

Looks good to me.

Thank you!