This is an archive of the discontinued LLVM Phabricator instance.

ARM: generation of .ARM.exidx/.ARM.extab sections
ClosedPublic

Authored by lenykholodov on Apr 28 2015, 8:46 AM.

Details

Summary

This patch provides generation of .ARM.exidx & .ARM.extab sections which are used for unwinding. The patch adds new content type typeARMExidx for atoms from .ARM.exidx section and integration of atoms with such type to the ELF ReaderWriter. Most of changes were done for only ARM component. However, some changes have to be implemented in common headers.

Diff Detail

Repository
rL LLVM

Event Timeline

lenykholodov retitled this revision from to ARM: generation of .ARM.exidx/.ARM.extab sections.
lenykholodov updated this object.
lenykholodov edited the test plan for this revision. (Show Details)
lenykholodov set the repository for this revision to rL LLVM.
lenykholodov added a project: lld.
lenykholodov added a subscriber: Unknown Object (MLST).
ruiu accepted this revision.Apr 28 2015, 12:37 PM
ruiu edited edge metadata.

LGTM with nits

lib/ReaderWriter/ELF/ARM/ARMELFFile.h
29 ↗(On Diff #24552)

nit: I'd remove this blank line (and other two lines below after dangling if).

lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
70 ↗(On Diff #24552)

Can you keep and update the comment, so that it's clear what we are doing here?

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
23 ↗(On Diff #24552)

A brief comment about what this class is for would be needed.

81 ↗(On Diff #24552)

Does just "TargetLayout" (without <>) work?

90 ↗(On Diff #24552)

Ditto

This revision is now accepted and ready to land.Apr 28 2015, 12:37 PM
lib/Core/DefinedAtom.cpp
46 ↗(On Diff #24552)

Just a general note to think about:

Maybe it's a good time to introduce doPermissions method for descendants and handle specific types there?

lib/ReaderWriter/ELF/ARM/ARMELFFile.h
33 ↗(On Diff #24552)

This should work as ELFDefinedAtom::permissions(). Please, check it.

41 ↗(On Diff #24552)

Ditto.

lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
74 ↗(On Diff #24552)

There's startEnd lambda function in the ExecutableWriter::finalizeDefaultAtomValues() that does *almost* the same thing, you may adapt it to be a proper function and use here.

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
38 ↗(On Diff #24552)

Remove this blank line.

39 ↗(On Diff #24552)

asserts should have description of a failure provided.

42 ↗(On Diff #24552)

This variable is not used in this context. Please, move it to the place where it's actually needed.

71 ↗(On Diff #24552)

TargetLayout without <> should work here as well.

lenykholodov added inline comments.May 5 2015, 8:34 AM
lib/Core/DefinedAtom.cpp
46 ↗(On Diff #24552)

I agree but I believe it should be implemented as separate patch.

lib/ReaderWriter/ELF/ARM/ARMELFFile.h
33 ↗(On Diff #24552)

I'll move the implementation to ELFDefinedAtom.

lenykholodov edited edge metadata.
  • implementations of permissions() and doContentType() were moved to base class ELFDefinedAtom;
  • ARMELFDefinedAtomBase was removed;
  • assert message was added;
  • minor code changes, comments and formatting after review.
lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
76 ↗(On Diff #24952)

That's now copy'n'pasting of the functionality. I actually wanted you to convert base class's lambda into member method and call it from here. Please, remove code duplication.

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
23 ↗(On Diff #24952)

It should be a doxygen-style comment. See here: http://llvm.org/docs/CodingStandards.html#commenting

  • startEnd as a separate method in ExecutableWriter;
  • comment style changes.
denis-protivensky accepted this revision.May 8 2015, 1:03 AM
denis-protivensky edited edge metadata.

LGTM with nits.

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
23 ↗(On Diff #25144)

/// \brief ARM ...

lib/ReaderWriter/ELF/ExecutableWriter.h
29 ↗(On Diff #25144)

Please, consider more meaningful name.

This revision was automatically updated to reflect the committed changes.