[llvm-readelf] Add -e/--headers support to readobj/elf

[sidneym]

Summary

Several new options have been recently added to align with GNU. This patch adds -e/--headers.

[rupprecht]

Just a nit on the naming; actual functionality/test LGTM.

tools/llvm-readobj/llvm-readobj.cpp

62 ↗

(On Diff #176715)

nit: "JustHeaders" kind of sounds like this will only print headers and nothing else, but "llvm-readelf -e -n" should still print notes in addition to headers (for example). Maybe "AllHeaders" would be a better name, and hopefully not confusing with the "All" option above?

[sidneym]

Change JustHeaders to AllHeaders

[jhenderson]

Please remember to include the context when creating the diff with Phabricator (see the section starting "To get a full diff" here.

test/tools/llvm-readobj/justheader.test

1 ↗

(On Diff #176725)

The test needs renaming in line with the option name. I'd just suggest calling it "headers.test" since that is the option name.

I also think it would be a good idea to test that --headers produces the same headers for both llvm-readelf and llvm-readobj. In other cases (I'm thinking --program-headers specifically), the two produce different output, and not just in terms of style.

Also, rather than running FileCheck on both outputs, I personally would prefer for the second test case to compare its output to the first case to show that the --headers output is identical to -e. You should still run FileCheck on the first one, but you will need to run it using --input-file instead of stdin instead.

Finally, I'm not sure either way as to whether you should be checking more of the output (e.g. the contents of the ELF header, section headers etc). I can be persuaded either way, but I'd like to hear your justification, please.

9–10 ↗

(On Diff #176725)

You may not be aware of this, but CHECK-NOT only checks the area between the previous and next matches, so in this case, it will only check that no relocation section or symbol table are dumped at the end. They could be printed e.g. between the section headers and program headers, and you wouldn't know.

11 ↗

(On Diff #176725)

Nit: Did you mean to have a blank line at the end of this file?

tools/llvm-readobj/llvm-readobj.cpp

62 ↗

(On Diff #176715)

Why not simply "Headers", since that is what the option is called?

[sidneym]

test/tools/llvm-readobj/justheader.test

1 ↗

(On Diff #176725)

Yes, I like the name "headers.test" better.

The output from llvm-readobj vs. llvm-readelf is almost always different and that is either by design or accident. Are you suggesting syncing the output between readobj and readelf for this case, basically setting, --elf-output-style=GNU when --headers is specified. Is that what you want?

The option -e is an alias for --headers so the output will match unless the meaning of one of the options is changed.

I used "all.test" as my template reasoning that -e is "almost all".

9–10 ↗

(On Diff #176725)

I'll add a new pass for negative testing.

tools/llvm-readobj/llvm-readobj.cpp

62 ↗

(On Diff #176715)

Sounds good.

[jhenderson]

The output from llvm-readobj vs. llvm-readelf is almost always different and that is either by design or accident. Are you suggesting syncing the output between readobj and readelf for this case, basically setting, --elf-output-style=GNU when --headers is specified. Is that what you want?

We shouldn't sync the output between llvm-readobj and llvm-readelf, although both should produce the right set of headers when requested. For llvm-readelf, we should match what GNU readelf does. llvm-readobj does not need to match this.

The test should test the following:

• For llvm-readelf:
  • That the correct set of headers are dumped when specifying --headers.
  • That the -e switch produces the exact same output.
• For llvm-readobj (if desired, it could probably be argued that having both is unnecessary):
  • That the correct set of headers are dumped when specifying --headers.
  • That the -e switch produces the exact same output.

Hopefully that explains things.

P.S. Please remember to include the full context in the diff...

[sidneym]

Full context? I use git format-patch -1 and upload the file it produces.

[jhenderson]

In D55298#1321288, @sidneym wrote:

Full context? I use git format-patch -1 and upload the file it produces.

This information is in the link I previously posted:

In D55298#1319873, @jhenderson wrote:

see the section starting "To get a full diff" here.

To save you having to go there and read everything, here's a quick summary:

To get a full diff, use one of the following commands (or just use Arcanist to upload your patch):

git show HEAD -U999999 > mypatch.patch
git format-patch -U999999 @{u}
svn diff --diff-cmd=diff -x -U999999

[sidneym]

Updated the patch to include full context.

Added the contents of the header blocks to the test.

[jhenderson]

test/tools/llvm-readobj/headers.test

1 ↗

(On Diff #176979)

I've been thinking a bit more about the test for this, and I think I have a better suggestion, after all that:

1. Run llvm-readelf with --headers, store the output in a file.
2. Run llvm-readelf with -e, store the output in a file.
3. Run llvm-readelf with --file-headers, --program-headers and --section-headers, store in a different file.
4. Compare the two files from 1) and 2) against the file from 3) using cmp.

That would avoid the need for testing the detailed contents, whilst also providing what we really want, i.e. that --headers produces file, program and section header output. How does that sound?

[sidneym]

Output each option test result to different files and verify that each of the files contains identical content.

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-readobj/headers.test

+RUN: llvm-readelf -e %p/Inputs/trivial.obj.elf-i386 > %t.e
+RUN: llvm-readelf --headers %p/Inputs/trivial.obj.elf-i386 > %t.headers
+RUN: llvm-readelf -h -l -S %p/Inputs/trivial.obj.elf-i386 > %t.hlS
+RUN: cmp %t.e %t.headers
+RUN: cmp %t.e %t.hlS

llvm/trunk/tools/llvm-readobj/llvm-readobj.cpp

namespace opts {
   cl::opt<bool>
       All("all",
           cl::desc("Equivalent to setting: --file-headers, --program-headers, "
                    "--section-headers, --symbols, --relocations, "
                    "--dynamic-table, --notes, --version-info, --unwind, "
                    "--section-groups and --elf-hash-histogram."));
   cl::alias AllShort("a", cl::desc("Alias for --all"), cl::aliasopt(All));
 
+  // --headers -e
+  cl::opt<bool>
+      Headers("headers",
+          cl::desc("Equivalent to setting: --file-headers, --program-headers, "
+                   "--section-headers"));
+  cl::alias HeadersShort("e", cl::desc("Alias for --headers"),
+     cl::aliasopt(Headers));
+
   // -wide, -W
   cl::opt<bool>
       WideOutput("wide", cl::desc("Ignored for compatibility with GNU readelf"),
                  cl::Hidden);
   cl::alias WideOutputShort("W",
     cl::desc("Alias for --wide"),
     cl::aliasopt(WideOutput));
 
if (opts::All) {
     opts::DynamicTable = true;
     opts::Notes = true;
     opts::VersionInfo = true;
     opts::UnwindInfo = true;
     opts::SectionGroups = true;
     opts::HashHistogram = true;
   }
 
+  if (opts::Headers) {
+    opts::FileHeaders = true;
+    opts::ProgramHeaders = true;
+    opts::SectionHeaders = true;
+  }
+
   // Default to stdin if no filename is specified.
   if (opts::InputFilenames.size() == 0)
     opts::InputFilenames.push_back("-");
 
   llvm::for_each(opts::InputFilenames, dumpInput);
 
   if (opts::CodeViewMergedTypes) {
     ScopedPrinter W(outs());
     dumpCodeViewMergedTypes(W, CVTypes.IDTable, CVTypes.TypeTable);
   }
 
   return 0;
 }