Page MenuHomePhabricator

[Object][XCOFF] Add intial support for parsing/dumping section header table.

Authored by sfertile on Apr 16 2019, 10:29 AM.



Adds an representation of the section header table to XCOFFObjectFile, and implements enough to dump the section headers with llvm-obdump.

Diff Detail


Event Timeline

sfertile created this revision.Apr 16 2019, 10:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 10:29 AM
125 ↗(On Diff #195410)

In keeping with the ordering of overriden functions in this file, this declaration should occur between getFeatures and isRelocatableObject as that is the ordering present in the definition of class ObjectFile.

40 ↗(On Diff #195410)

There is an extra blank line here in comparison to the number of blank lines used between the next few functions below.

54 ↗(On Diff #195410)

This should be static or otherwise made to have internal linkage to avoid ODR violations.

139 ↗(On Diff #195410)

Do we want to use the version that avoids embedded NUL characters in this patch?

181 ↗(On Diff #195410)

It seems (Flags & (XCOFF::STYP_DATA | XCOFF::STYP_TDATA)) is a more common pattern in the codebase.

186 ↗(On Diff #195410)

Same comment as above.

310 ↗(On Diff #195410)

getObject (called below) takes a Size parameter. It can be used to check for the right number of sections without this call to checkSize.

sfertile updated this revision to Diff 195746.Apr 18 2019, 8:17 AM
  1. Addressed @hubert.reinterpretcast comments
  2. Added the compiler and source used to generate the object file used in the test.
  3. Added test coverage of long section names (ie names of length 8 that are not null terminated)
  4. removed the checkSize function as there were no longer any users.
DiggerLin added inline comments.Apr 18 2019, 11:10 AM
24 ↗(On Diff #195746)

ObjectFile.h already include SymbolicFile.h , I think there do need SymbolicFile.h here. and also
#include "llvm/Object/Binary.h"
#include "llvm/Object/Error.h"
#include "llvm/BinaryFormat/Magic.h"
#include "llvm/ADT/iterator_range.h"

sfertile marked an inline comment as done.Apr 18 2019, 12:16 PM
sfertile added inline comments.
24 ↗(On Diff #195746)

Good point, most of these are already included from ObjectFile.h. I'll clean up our header includes but I'll land that in a separate NFC patch.

sfertile marked 8 inline comments as done and an inline comment as not done.Apr 18 2019, 12:45 PM
sfertile added inline comments.
24 ↗(On Diff #195746)

Didn't intend to mark this done yet.

310 ↗(On Diff #195410)

I've also removed 'checkSize' as it no longer has any uses.

sfertile marked 2 inline comments as done and an inline comment as not done.Apr 18 2019, 1:16 PM
sfertile added inline comments.
24 ↗(On Diff #195746)

Posted: I plan to commit that Monday or Tuesday and then I'll rebase this patch to pick up the changes.

LGTM with minor changes (no need to post a new revision).

24 ↗(On Diff #195746)

I'd be happy with seeing this committed and having D60885 adjusted instead.

23 ↗(On Diff #195746)

Some indication of whether the source is C or C++ (e.g., the base name of the source file) would be helpful.

29 ↗(On Diff #195746)

Add a space after the # on this line (and the next few lines) to match the above lines.

This revision is now accepted and ready to land.Apr 19 2019, 8:22 PM
MaskRay added inline comments.
1 ↗(On Diff #195746)

Does it need a REQUIRES: line?

1 ↗(On Diff #195746)

No, this is dumping section headers only (not performing disassembly).

This revision was automatically updated to reflect the committed changes.