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

[Paul Semel via Phabricator via llvm-commits]

paulsemel marked 3 inline comments as done.
paulsemel added a comment.

Little follow-up. I'm pretty sure it would require changes to `yaml2obj` to get this test written in yaml format.
Why not creating my own binary for the moment, and wait for someone (me if I have time ?) to implement this feature ?



================
Comment at: include/llvm/Object/ELF.h:478
+
+  return makeArrayRef(Dyn, numEntry);
+}
----------------
jhenderson wrote:
> I wonder if we should cache this result somehow, so that the majority of this function is only run once per file?
It's up to you. For the moment, I've just mimicked the behavior of `program_headers` and `sections` :)


================
Comment at: test/tools/llvm-objdump/private-headers-dynamic-section.test:1
+# RUN: llvm-objdump -p %p/Inputs/eh_frame.elf-mipsel | FileCheck %s
+
----------------
jhenderson wrote:
> 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.
Ok, I'll try to be more verbose on this. Here is what I would like to have in yaml2obj :

```
Dynamic:
        DT_NEEDED            "libc.so"
        DT_WHATEVER       0x12345678
```

And here is what we have :

```
Sections:
  - Name:            .dynamic
    Type:            SHT_DYNAMIC
    Flags:           [ SHF_ALLOC, SHF_WRITE ]
    Content:         0x0000001012002030000400
```
And for each reference in the dynamic section, we need to trick to build a correct binary. (thus in our case, we need to build a correct program_headers, strtab etc..). Really, it'll just get a big mess and loss of time.

This feature might definitely be added in yaml2obj. I admit that I don't really feel like building that a complicated yaml file :)


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


================
Comment at: tools/llvm-objdump/ELFDump.cpp:143
+    case ELF::DT_LOOS:
+      Str = "LOOS";
+      break;
----------------
jhenderson wrote:
> 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?
Yes, to have the markers, you need to define another macro


================
Comment at: tools/llvm-objdump/ELFDump.cpp:21
 
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(*(a)))
+
----------------
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 ?


================
Comment at: tools/llvm-objdump/ELFDump.cpp:59
+  if (!ProgramHeaderOrError)
+    report_fatal_error(
+        errorToErrorCode(ProgramHeaderOrError.takeError()).message());
----------------
jhenderson wrote:
> 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.
Well, I can't access file name here, I just mimicked the behavior above, I think this is also the reason they did this no ?
I'm probably missing something


================
Comment at: tools/llvm-objdump/ELFDump.cpp:89
+    else {
+      // We want to fit with GNU objdump out
+      outs() << format("  %-21s", Str.data());
----------------
jhenderson wrote:
> 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).
Is there a rational reason apart from "We want to be GNU command line compliant" ? Not sure we might try to explain smth.


================
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) {
----------------
jhenderson wrote:
> Why not make the second one PRIx32, and then get rid of the cast when formatting later?
Seems that a static assert prevent us from doing it. See what's being done below for program header.


Repository:
  rL LLVM

https://reviews.llvm.org/D49016