This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Add .eh_frame pieces to map file
AbandonedPublic

Authored by andrewng on Feb 6 2018, 6:16 AM.

Details

Reviewers
ruiu
Summary

Show the mapping of input .eh_frame pieces to the output .eh_frame section in the map file.

Diff Detail

Event Timeline

jhenderson created this revision.Feb 6 2018, 6:16 AM
grimar added a subscriber: grimar.Feb 6 2018, 7:15 AM
ruiu added a comment.Feb 22 2018, 5:24 PM

Yeah, please add a test for this for demonstration purpose. Actually, for demonstration purpose, a test is probably more useful than the actual code, because we are more interested in knowing about how a program works as a result of a change than the change itself. In particular, I'm interested in knowing the information that is contained in the new output.

Thanks for the comments. This is currently on our backburner, as we've got some other work to do first, but hopefully @andrewng or I will be able to look at this at some point in the next couple of weeks. If I get a chance later today, I'll upload a test.

andrewng commandeered this revision.Mar 1 2018, 7:37 AM
andrewng added a reviewer: jhenderson.
andrewng updated this revision to Diff 136522.Mar 1 2018, 7:43 AM
andrewng removed a reviewer: jhenderson.
andrewng added a subscriber: jhenderson.

Rebased and updated with minor change to make the output offset hex rather than decimal. Modified existing map file test to demonstrate the proposed .eh_frame output.

ruiu added a comment.Mar 2 2018, 1:31 PM

Feature-wise this is OK but the code doesn't seem to fit well to other code in lld. Please read other part of the code and make your new code similar to that. In particular, please

  • make functions shorter
  • don't use auto and
  • simplify code and logic.
andrewng retitled this revision from [ELF] DEMO: Example for adding .eh_frame pieces to map file to [ELF] Add .eh_frame pieces to map file.Mar 6 2018, 9:42 AM
andrewng edited the summary of this revision. (Show Details)
andrewng updated this revision to Diff 137220.Mar 6 2018, 9:50 AM

Updated to address comments.

Moved output of input section offset to toString as suggested by Rafael.

ruiu added inline comments.Mar 6 2018, 10:16 AM
ELF/InputSection.h
347 ↗(On Diff #137220)

I don't think this is a good idea. lld::toString(T) is an overloaded function dispatched by its parameter type. But you are now defining a different function which you only use in MapFile.cpp. I'd move this back to the original file.

ELF/MapFile.cpp
141

This is still too long. When you add a large amount of code to an existing function, please try to make it simple -- by splitting it up, refactor, etc.

ruiu added inline comments.Mar 6 2018, 10:18 AM
ELF/MapFile.cpp
143

As I said, please try to make your new code look the same as the existing code. For example, we don't define multiple local variables in one line like this in other places.

andrewng updated this revision to Diff 137366.Mar 7 2018, 5:22 AM

Updated to address review comments.

However, happy to abandon in favour of D44168 if that is the preferred implementation and output format.

ruiu added a comment.Mar 7 2018, 1:54 PM

Yeah, I think I slightly prefer my implementation because even though it is slightly inefficient (because it creates an intermediate vector), it is a bit easier to read. Or you can swap the function body with mine if you want.

There are also some slight differences in the output between this implementation and the one in D44168. This patch does not output the "+0x0" for a zero offset and outputs the input section's alignment for the "Align" column. Which style of output is preferred?

Rui Ueyama via Phabricator via llvm-commits
<llvm-commits@lists.llvm.org> writes:

ruiu added inline comments.

Comment at: ELF/InputSection.h:347

-std::string toString(const elf::InputSectionBase *);
+std::string toString(const elf::InputSectionBase *, uint64_t SecOffset = 0);

} // namespace lld

I don't think this is a good idea. lld::toString(T) is an overloaded function dispatched by its parameter type. But you are now defining a different function which you only use in MapFile.cpp. I'd move this back to the original file.

My objection to the original implementation was that it was looking at
the output of toString, effectively knowing exactly what toString does
and breaking the modularity.

Would you be OK with having

std::string lld::someName(const InputSectionBase *Sec, uint64_t Offset);

and implementing std::string lld::toString(const InputSectionBase *Sec)
as someName(Sec, 0)?

Cheers,
Rafael

ruiu added a comment.Mar 8 2018, 10:45 AM

I don't think omitting +0x<hex> only when it is +0x0 doesn't make much sense. It is better to always print out an offset regardless of its value. That also makes it easy to parse by machine.

Does alignment make sense? I think that each section has an alignment, but each piece in an .eh_frame doesn't have a notion of an alignment. So I think it isn't correct to show a section alignment as a piece alignment.

andrewng abandoned this revision.Mar 9 2018, 6:23 AM

Abandoning this revision in favour of D44168.