This is an archive of the discontinued LLVM Phabricator instance.

ObjectFileMachO: split CreateSections mega-function into more manageable chunks
ClosedPublic

Authored by labath on Mar 3 2018, 5:31 PM.

Details

Summary

In an effort to understand the function's operation, I've split it into logical
pieces. Parsing of a single segment is moved to a separate function (and the
parsing state that is carried from one segment to another is explicitly
captured in the SegmentParsingContext object). I've also extracted some pieces
of code which were already standalone (validation of the segment load command,
determining the section type, determining segment permissions) into
separate functions.

Parsing of a single section within the segment should probably also be a
separate function, but I've left that for a separate patch.

This patch is intended to be NFC.

Event Timeline

labath created this revision.Mar 3 2018, 5:31 PM
clayborg accepted this revision.Mar 5 2018, 7:46 AM

Looks fine. Would just move stuff from header into .cpp file for the SegmentParsingContext class. I'll leave that up to you.

source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
191–202

Forward declare this and remove from header file?

This revision is now accepted and ready to land.Mar 5 2018, 7:46 AM
davide accepted this revision.Mar 5 2018, 8:12 AM

LGTM

source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
191–202

Why?

clayborg added inline comments.Mar 5 2018, 8:34 AM
source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
202

Because it is only used in internal functions... I doesn't have to move, just a suggestion. All this code used to be in the section parsing functions.

davide added a comment.Mar 5 2018, 8:51 AM

Fair, I don't have a strong opinion on whether this should be in an header or not. Probably Greg is right though, if this is not used anywhere else, we could make it somehow private.
Thanks!

labath added a comment.Mar 6 2018, 5:28 AM

I agree we should move things to the cpp file when possible, and this is possible. I'm going to update the patch before committing. Thanks for the review.

This revision was automatically updated to reflect the committed changes.