[PATCH] D49016: [llvm-objdump] Add dynamic section printing to private-headers option

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

(moving this comment out of line, because the comments got completely out of sync with the code):
Regarding the VirtBase calculation, yeah, you're right, there was a mistake on my part - I made an incorrect assumption regarding program layout.

But you definitely cannot rely on PT_PHDR, because it isn't always present.

There might be a better way than this, but I think one way we can rely on, is to loop through the program headers until finding one whose address range (i.e. p_vaddr to p_vaddr + p_memsz) includes the value of DT_STRTAB, then subtract that segment's p_vaddr from the DT_STRTAB value (giving us the relative offset within the segment of the section), before finally adding the p_offset value.

Aside: since you are now touching the libObject code, you might want to see if there are any people you should add to the reviewers from that area.



================
Comment at: include/llvm/Object/ELF.h:451
+template <class ELFT>
+Expected<typename ELFT::DynRange> ELFFile<ELFT>::dynamicEntries() const {
+  const Elf_Dyn *Dyn = nullptr;
----------------
I think this should use the dynamic program header, rather than the sections, because a runnable program doesn't need section headers (but it does not program headers). It'll be faster, because there are generally fewer program headers than sections too. Maybe it could fall back to using the section headers if there is no PT_DYNAMIC segment.

Are the functions in this file unit-tested at all? If so, we should add them. If not, we still might want to add some, but it would depend on how straightforward it is to do.


================
Comment at: include/llvm/Object/ELF.h:472
+  if (!numEntry)
+    return createError("invalid dynamic number of entries");
+
----------------
I'd rephrase this as "invalid empty dynamic section" (or segment if using the program headers), or similar. I also have a personal preference to be explicit with checks (i.e. do `numEntry == 0`), though I'm not too fussed if you prefer it this way.


================
Comment at: include/llvm/Object/ELF.h:478
+
+  return makeArrayRef(Dyn, numEntry);
+}
----------------
I wonder if we should cache this result somehow, so that the majority of this function is only run once per file?


================
Comment at: test/tools/llvm-objdump/private-headers-dynamic-section.test:1
+# RUN: llvm-objdump -p %p/Inputs/eh_frame.elf-mipsel | FileCheck %s
+
----------------
paulsemel wrote:
> jhenderson wrote:
> > As with other tests, I'd rather not use a pre-canned binary here, especially one that isn't directly associated with this test. Please use yaml2obj.
> Ok.. I gave it a try. The problem is not about building the dynamic section (though it might lead to confusing tests), but more about all the needed parts we need to have to get a relevant test (Program headers, string tables etc..)
> What do you think ?
Does yaml2obj not allow you to pass in arbitrary values to the dynamic tags then?

Regardless, that complexity is there already, it's just hiding away in a pre-canned binary, which will make it harder to work out what to do with this test, if that binary is ever updated and/or this test ever starts to fail. I'd rather we be explicit, and not tie ourselves to that binary, if at all possible.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:28
+                                     uint64_t VirtBase) {
+  for (; Dyn->d_tag != ELF::DT_NULL; Dyn++) {
+    if (Dyn->d_tag == ELF::DT_STRTAB)
----------------
paulsemel wrote:
> jhenderson wrote:
> > What happens if you have a malformed segment with no DT_NULL tag?
> > 
> > You should probably use the program header's p_filesz field as an additional check to avoid overrunning the end of the array. If we hit p_filesz and not DT_NULL, we should probably warn. Assuming you can get yaml2obj to create such a section, I'd also expect a test case for that.
> Ok.. I finally added a helper function in libObject, I think it is the right way to do it.
> Moreover, DT_NULL is needed by the ELF specification (it must end the Dynamic section), so let's issue an error.
Normally, I'd agree with you, but objdump's job is not to manipulate things, only to give information about things. That means that we should try to give as much information as possible, especially as in this case, a missing DT_NULL does not prevent us doing so. Emitting an error will presumably stop us doing any further dumps the user has requested. So I think a warning would be more appropriate.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:143
+    case ELF::DT_LOOS:
+      Str = "LOOS";
+      break;
----------------
jhenderson wrote:
> DT_LOOS shouldn't be printed like this. It's a boundary marker, so would actually be some platform specific value (e.g. DT_GNU_MY_MAGIC_TAG).
> 
> Similar comments go for DT_HIGHOS, DT_LOPROC, and DT_HIGHPROC, although I see you haven't got them in the list.
Does your new method address this comment?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:145
+      break;
+    case ELF::DT_GNU_HASH:
+      Str = "GNU_HASH";
----------------
paulsemel wrote:
> paulsemel wrote:
> > jhenderson wrote:
> > > What made you pick just this set of platform specific values?
> > > 
> > > I'd actually suggest that you should have printing for all dynamic tags listed in llvm/BinaryFormat/DynamicTags.def.
> > This was just to initiate this feature, I didn't really know which one to handle. But I will just take every tags in DynamicTags.def
> Ok I'll answer here for this comment and the one above. I decided to take get all the non architecture specific tags for the moment.
I think having the non-architecture-specific tags only is sensible for the first instance. A later change can add support for them (we should only decode them when using the appropriate machine type).


================
Comment at: tools/llvm-objdump/ELFDump.cpp:21
 
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(*(a)))
+
----------------
I'd be surprised if this doesn't already exist in the LLVM support library somewhere...


================
Comment at: tools/llvm-objdump/ELFDump.cpp:51
+
+  return createError("DT_STRTAB entry not present");
+}
----------------
Similar to my earlier comments, maybe it would be better to warn for this case, and fall back to just printing the hex value, like you suggested.

This probably goes for all errors that result in us being unable to load the dynamic string table. Also, where possible, we should test each of those errors.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:59
+  if (!ProgramHeaderOrError)
+    report_fatal_error(
+        errorToErrorCode(ProgramHeaderOrError.takeError()).message());
----------------
This and others should be "report_error" available in llvm-objdump.h, not report_fatal_error. They also just take an Error, so you don't have to mess about with errorToErrorCode.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:89
+    else {
+      // We want to fit with GNU objdump out
+      outs() << format("  %-21s", Str.data());
----------------
Why? Also, I think this comment should be a bit more verbose, so that it is clearer. Depending on the reason, perhaps something like:

"We use "-21" in order to match GNU objdump's output. <explain reason briefly>"

(also, don't forget the trailing full stop).


================
Comment at: tools/llvm-objdump/ELFDump.cpp:94
+    const char *Fmt =
+        ELFT::Is64Bits ? "0x%016" PRIx64 "\n" : "0x%08" PRIx64 "\n";
+    if (Dyn.d_tag == ELF::DT_NEEDED) {
----------------
Why not make the second one PRIx32, and then get rid of the cast when formatting later?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:101
+        continue;
+      } else
+        consumeError(StrTabOrErr.takeError());
----------------
No need for this else following the continue.


Repository:
  rL LLVM

https://reviews.llvm.org/D49016