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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/ADT/STLExtras.h:1041-1045
+template <class T, std::size_t N>
+constexpr size_t size(const T (&array)[N]) noexcept {
+  return N;
+}
+
----------------
Be careful that this doesn't get submitted with everything else...


================
Comment at: include/llvm/Object/ELF.h:451
+template <class ELFT>
+Expected<typename ELFT::DynRange> ELFFile<ELFT>::dynamicEntries() const {
+  const Elf_Dyn *Dyn = nullptr;
----------------
jhenderson wrote:
> 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.
I don't think you've addressed this comment: 
>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: test/tools/llvm-objdump/private-headers-dynamic-section.test:27
+# CHECK:  VERSYM               0x00000000000003ec
+# CHECK:  RELACOUNT            0x0000000000000003
----------------
Could you also add SHT_RELR cases here, please?


================
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:
> > paulsemel wrote:
> > > jhenderson wrote:
> > > > 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.
> > > Is it even possible to issue warnings from libObject ? (as now this code has moved to the libobj)
> > I'd expect library code to emit "Error" or "Expected" returns, and then the consumer to decide what to do with them. You can see a similar thing I did earlier this year for the DWARF debug line parser, which now returns Expected/Error instances, and the consumers often convert these into warnings (see rL331971).
> Hmm.. I don't have any real arguments against this, but I don't really like doing this..
> Isn't it another way to do this ?
Well, we need to report the error back somehow from the library, and we don't want the library to do the printing, because then different consumers can't customise according to their needs. However, we're not actually in a library here, so this is all a bit moot!


================
Comment at: tools/llvm-objdump/ELFDump.cpp:21
 
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(*(a)))
+
----------------
jhenderson wrote:
> paulsemel wrote:
> > jhenderson wrote:
> > > I'd be surprised if this doesn't already exist in the LLVM support library somewhere...
> > Tried to grep but was there. Do you know where this kind of macros are in LLVM ?
> Well, I'd expect this to be an STL extension, since it's in C++17 (see https://en.cppreference.com/w/cpp/iterator/size), and I see that there is a size method in STLExtras.h. Try taking a look at that. I'm not sure it'll quite work, as I think it is implementing version 1 from the reference page only, but maybe you could try putting up a separate review for adding version 2 support?
Can you remove this now?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:44
+template <class ELFT>
+Expected<StringRef> getDynamicStrTab(const ELFFile<ELFT> *o,
+                                     const typename ELFFile<ELFT>::Elf_Dyn *Dyn,
----------------
o -> O (or probably better just Elf or similar, since it's not an object file if its got a dynamic section).


================
Comment at: tools/llvm-objdump/ELFDump.cpp:50-51
+  if (!DynamicEntriesOrError)
+    return DynamicEntriesOrError.takeError()
+        :
+
----------------
Is this a mistake here? Because it doesn't look like you meant to write this...


================
Comment at: tools/llvm-objdump/ELFDump.cpp:68
+  if (!ProgramHeaderOrError)
+    report_fatal_error(
+        errorToErrorCode(ProgramHeaderOrError.takeError()).message());
----------------
This should be report_error, like below.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:72-74
+    if (Phdr.p_type == ELF::PT_PHDR) {
+      VirtBase = Phdr.p_vaddr - Phdr.p_offset;
+      break;
----------------
I don't think you've addressed my earlier comments about not using VirtBase, derived from a PT_PHDR?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:83-89
+  for (const auto &Dyn : *DynamicEntriesOrError) {
+    if (Dyn.d_tag == ELF::DT_NULL)
+      continue;
+    StringRef Str;
+    for (size_t i = 0; i < size(DynamicTagsValues); ++i) {
+      if (DynamicTagsValues[i].Val == Dyn.d_tag) {
+        Str = StringRef(DynamicTagsValues[i].ToStr);
----------------
This sort of area is a good example of why I think we should be reusing code from llvm-readobj. Please investigate this as a priority. In particular, I note that llvm-readobj does not have to loop through the whole set of tags to get the right string.


Repository:
  rL LLVM

https://reviews.llvm.org/D49016