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

[Paul Semel via Phabricator via llvm-commits]

paulsemel marked 6 inline comments as done and an inline comment as not done.
paulsemel added inline comments.


================
Comment at: include/llvm/Object/ELF.h:493
+  if (End->d_tag != ELF::DT_NULL)
+    return createError("dynamic sections must be DT_NULL terminated");
+
----------------
I don't think we need to issue an error here actually


================
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)
----------------
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 ?


Repository:
  rL LLVM

https://reviews.llvm.org/D49016