Page MenuHomePhabricator

[llvm-objdump] Add dynamic section printing to private-headers option
ClosedPublic

Authored by paulsemel on Jul 6 2018, 3:19 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Jul 11 2018, 2:34 AM
tools/llvm-objdump/ELFDump.cpp
49

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.

57

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.

143

Does your new method address this comment?

145

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).

paulsemel marked 3 inline comments as done.Jul 16 2018, 2:04 AM

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 ?

include/llvm/Object/ELF.h
478 ↗(On Diff #154945)

It's up to you. For the moment, I've just mimicked the behavior of program_headers and sections :)

test/tools/llvm-objdump/private-headers-dynamic-section.test
1 ↗(On Diff #154393)

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

tools/llvm-objdump/ELFDump.cpp
21

Tried to grep but was there. Do you know where this kind of macros are in LLVM ?

28

Is it even possible to issue warnings from libObject ? (as now this code has moved to the libobj)

57

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

87

Is there a rational reason apart from "We want to be GNU command line compliant" ? Not sure we might try to explain smth.

92

Seems that a static assert prevent us from doing it. See what's being done below for program header.

143

Yes, to have the markers, you need to define another macro

paulsemel updated this revision to Diff 155631.Jul 16 2018, 2:04 AM

Added Jame's suggestions

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 ?

I think this would be a reasonable way forward.

I'm still concerned by the amount of duplication between llvm-readobj's handling of this and llvm-objdump's. My impression is that only the printing and error handling sections should be distinct. The methods for looking up values etc should be in libObject.

I'm out of time now, so haven't looked at everything yet, but hopefully this will help you continue moving.

include/llvm/Object/ELF.h
478 ↗(On Diff #154945)

Okay, sure. I guess it's unlikely a program will need to do this multiple times, and it could be the thing to do the caching if its important. So no need to do it, I think.

459 ↗(On Diff #155631)

Too much auto. Please use the explicit type here.

474 ↗(On Diff #155631)

Similar to above, please use the explicit type here.

476 ↗(On Diff #155631)

You can use getSectionContentsAsArray here, I think.

488 ↗(On Diff #155631)

What about if DynSecSize % sizeof(Elf_Dyn) is non-zero? That should probably be an error too.

491 ↗(On Diff #155631)

I would call this "Last" or similar, since End implies one past the end in iterator terminology. That being said, it might just be easier to do the makeArrayRef before this and use the methods on the ArrayRef instead to query the last element (specifically back).

test/tools/llvm-objdump/private-headers-dynamic-section.test
1 ↗(On Diff #154393)

Okay, those are all fair comments. I'm happy for a pre-canned binary, but I'd prefer it if it is dedicated to this feature.

tools/llvm-objdump/ELFDump.cpp
21

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?

28

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).

49

Why not just propagate the Error back up the stack? You are already returning an Expected.

57

I'm not sure which one this lined up with before, but it looks to me that you have two options in most cases, 1) return Error/Expected up the stack, so that the call site handles it, or 2) Pass the file name into the function.

paulsemel marked 6 inline comments as done and an inline comment as not done.Jul 16 2018, 1:18 PM
paulsemel added inline comments.
include/llvm/Object/ELF.h
493 ↗(On Diff #155631)

I don't think we need to issue an error here actually

493 ↗(On Diff #155631)

My bad.. old comment

tools/llvm-objdump/ELFDump.cpp
28

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 ?

paulsemel updated this revision to Diff 155745.Jul 16 2018, 1:19 PM

Added Jame's suggestions.

jhenderson added inline comments.Jul 17 2018, 2:31 AM
include/llvm/ADT/STLExtras.h
1041–1045 ↗(On Diff #155745)

Be careful that this doesn't get submitted with everything else...

include/llvm/Object/ELF.h
451 ↗(On Diff #154945)

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.

test/tools/llvm-objdump/private-headers-dynamic-section.test
27 ↗(On Diff #155745)

Could you also add SHT_RELR cases here, please?

tools/llvm-objdump/ELFDump.cpp
21

Can you remove this now?

28

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!

42

o -> O (or probably better just Elf or similar, since it's not an object file if its got a dynamic section).

48–49

Is this a mistake here? Because it doesn't look like you meant to write this...

66

This should be report_error, like below.

70–72

I don't think you've addressed my earlier comments about not using VirtBase, derived from a PT_PHDR?

81–87

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.

paulsemel updated this revision to Diff 155879.Jul 17 2018, 7:14 AM
paulsemel marked 5 inline comments as done.

Alright.. Again, there was a lot of changes I wanted to do, so sorry if I messed smth @jhenderson.
I eventually moved some code from llvm-readobj to libObject, and I will refactor llvm-readobj similar code in another PR !

paulsemel added inline comments.Jul 17 2018, 7:17 AM
include/llvm/ADT/STLExtras.h
1041–1045 ↗(On Diff #155745)

Sure, I will wait for the other patch to be accepted, and then I'll rebase

include/llvm/Object/ELF.h
451 ↗(On Diff #154945)

No there is basically two features unit tested in the libObject. Do you really want me to do one ?

test/tools/llvm-objdump/private-headers-dynamic-section.test
27 ↗(On Diff #155745)

Are you talking about PLT REL entries ? I've added some anyway !

tools/llvm-objdump/ELFDump.cpp
70–72

I have eventually changed the way I'm getting the Dynamic String Table, to reuse at most as possible the libObject.

81–87

Well, I would just need to replace this to a switch actually

Hmm... I don't like using the dynamic symbol table to lookup the dynamic string table. There is a reason for a specific DT_STRTAB tag. We should use that. Also, this latest version won't work if the section headers have been stripped, but we should be able to dump the dynamic table in this instance, by looking at the DT_* tags in the PT_DYNAMIC segment (if present). Try adding a test for this case (i.e. section headers stripped), and you'll see an error, I suspect... Also, please make sure you have a test that has no section headers or PT_DYNAMIC segment.

Alright.. Again, there was a lot of changes I wanted to do, so sorry if I messed smth @jhenderson.
I eventually moved some code from llvm-readobj to libObject, and I will refactor llvm-readobj similar code in another PR !

Could you do this separate PR first, please? Otherwise, we end up with (temporarily) duplicate code, which might be an issue if somebody modifies llvm-readobj in the meantime.

include/llvm/ADT/STLExtras.h
1041–1045 ↗(On Diff #155745)

Actually, since it's been pointed out, you might as well use array_lengthof in this patch, to remove any dependency.

include/llvm/Object/ELF.h
533 ↗(On Diff #155879)

It occurs to me that this half of the code is currently untested. Could you add a test that shows that if a dynamic segment doesn't exist, we look for and use the .dynamic section.

You should also add a test that shows that if neither exist, we just don't print anything.

451 ↗(On Diff #154945)

Is it possible to easily construct an ELFFile in memory? If not, I suppose not, as we might as well use the lit tests to achieve this.

test/tools/llvm-objdump/private-headers-dynamic-section.test
27 ↗(On Diff #155745)

No, I was referring to SHT_RELR, added recently to LLD (rL336594) and llvm-readobj (rL335922). See also the proposal at https://groups.google.com/forum/#!topic/generic-abi/bX460iggiKg. The important bit is that there are some additional DT_* tags for these sections: DT_RELR, DT_RELRENT, and DT_RELRSZ, and I'd like it if these were tested too.

tools/llvm-objdump/ELFDump.cpp
42

Um... This hasn't been done?

paulsemel updated this revision to Diff 156622.Jul 20 2018, 3:33 PM
paulsemel marked 4 inline comments as done.

Added Jame's suggestions.
I managed to do one of the two real test cases with only yaml2obj. The first one definitely needs too much things to be a yaml2obj test.

Ok, I think the way I'm now getting the dynamic string table is good !

Hmm no.. I'm pretty sure it is a bad idea to do a PR that is completely relying on this one. I just duplicated the code, so I don't see any problem even if someone modifies this area in llvm-readobj. I will just refactor no worries :)

tools/llvm-objdump/ELFDump.cpp
42

Was for the function above, but I should have done this one too in the meantime !

Hmm no.. I'm pretty sure it is a bad idea to do a PR that is completely relying on this one. I just duplicated the code, so I don't see any problem even if someone modifies this area in llvm-readobj. I will just refactor no worries :)

Okay. Please make sure in the commit comment to explain that this is a copy of the llvm-readobj code, and that a subsequent refactor is still to come.

include/llvm/Object/ELF.h
421–422 ↗(On Diff #156622)

This strikes me as a rather large function to have in a header file. What do you think about moving the body into ELF.cpp source file? It looks like there is precedence for this already. I also noticed that the source provides a "STRINGIFY_ENUM_CASE" macro that does what you want.

513 ↗(On Diff #156622)

Again, this should be moved to the source file.

test/tools/llvm-objdump/private-headers-dynamic-section.test
1 ↗(On Diff #156622)

One more test suggestion: use llvm-objcopy to remove the section headers for this test input and then run the same dump. It should produce the same output.

test/tools/llvm-objdump/private-headers-no-dynamic-segment.test
14 ↗(On Diff #156622)

Now that you've done this, feel free to keep it, but I think you can simplify it down a bit and not test all the tags here.

test/tools/llvm-objdump/private-headers-no-dynamic.test
18 ↗(On Diff #156622)

Maybe worth doing a CHECK-NOT: DT_ or similar to show that the tool isn't trying to dump garbage in this cae.

tools/llvm-objdump/ELFDump.cpp
32–35

I don't think we want this loop as the first choice. Making it the fallback would match the behaviour earlier of dynamicEntries() (i.e. use program headers first, and only fallback on sections if that doesn't work.

42

I just realised that you're reinventing the wheel again here. llvm-readobj has the same function to do the same sort of thing. Take a look at toMappedAddr in parseDynamicTable in the ElfDumper class. It probably is worth sharing this again, so as to not repeat the code, and risk getting it slightly wrong.

paulsemel updated this revision to Diff 156804.Jul 23 2018, 9:18 AM
paulsemel marked 4 inline comments as done.

Added Jame's suggestions

I haven't copied the whole thing, most of the code is from me actually.. Why can't I just land this one and refactor the other one right after ?
I'm not sure this is that much a big deal to do this way, is it ?

Anyway, this patch is for the moment relying on an other patch I made for yaml2obj, so I'll wait until it is accepted !

NB: fun fact, it seems, that we are doing better in finding the dynamic section than GNU objdump.

include/llvm/Object/ELF.h
421–422 ↗(On Diff #156622)

No, I can't, because I don't want to output "DT_NEEDED", but "NEEDED"...

test/tools/llvm-objdump/private-headers-no-dynamic.test
18 ↗(On Diff #156622)

I though about it, but there is no pattern I can check.. (as I am not printing the DT_ thing)

tools/llvm-objdump/ELFDump.cpp
42

I move this toMappedAddr function to libObj, as I see a lot of use cases for it :)

jhenderson added inline comments.Jul 23 2018, 9:40 AM
include/llvm/Object/ELF.h
513 ↗(On Diff #156622)

You've marked this as done but dynamicEntries is still in the header file...

test/tools/llvm-objdump/private-headers-dynamic-section.test
1 ↗(On Diff #156622)

Sorry, I guess I wasn't clear. I was expecting these to be in the same test file. So the run commands would look something like:

# RUN: llvm-objdump -p %p/Inputs/private-headers-x86_64.elf | FileCheck %s
# RUN: llvm-objcopy <insert appropriate arguments here> %p/Inputs/private-headers-x86_64.elf %t-stripped.elf
# RUN: llvm-objdump -p %t-stripped.elf | FileCheck %s

This means that you can show that the output is identical with and without the section headers.

test/tools/llvm-objdump/private-headers-no-dynamic.test
18 ↗(On Diff #156622)

This is the last bit of the output right? If so, you can do:

CHECK-NOT: {{.}} or similar.

tools/llvm-objdump/ELFDump.cpp
42

Yes, I agree with this.

paulsemel marked 2 inline comments as done.

Added Jame's suggestions.

include/llvm/Object/ELF.h
513 ↗(On Diff #156622)

Oops..

test/tools/llvm-objdump/private-headers-dynamic-section.test
1 ↗(On Diff #156622)

You can't just strip the section header entry and not the whole section with llvm-objcopy (which make sense). I had to craft the binary by hand.. So this is not really possible

jhenderson added inline comments.Jul 24 2018, 2:31 AM
include/llvm/Object/ELF.h
513 ↗(On Diff #156622)

How about moving this into the source file too!?

test/tools/llvm-objdump/private-headers-dynamic-section.test
1 ↗(On Diff #156622)

You can strip all section headers. Use llvm-objcopy --strip-sections to do it. This will preserve the section contents, but remove the section headers (and the section header string table, if I'm not mistaken).

paulsemel updated this revision to Diff 156992.Jul 24 2018, 3:05 AM
This revision is now accepted and ready to land.Jul 24 2018, 3:12 AM
This revision was automatically updated to reflect the committed changes.