This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho][nfc] rename parsed-section types & variables
ClosedPublic

Authored by gkm on Nov 4 2021, 9:39 PM.

Details

Summary

This is an NFC diff that prepares for pruning & relocating __eh_frame.

Along the way, I made the following changes to ...

  • clarify usage of section vs. subsection
  • remove map & vec from type names
  • disambiguate class Section from template parameter SectionHeader.

Diff Detail

Event Timeline

gkm created this revision.Nov 4 2021, 9:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2021, 9:39 PM
gkm requested review of this revision.Nov 4 2021, 9:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2021, 9:39 PM
gkm updated this revision to Diff 385187.Nov 5 2021, 2:05 PM

I missed a rename ...

  • s/Subsection/ParsedSubsection/
gkm added inline comments.Nov 5 2021, 11:40 PM
lld/MachO/Writer.cpp
506

Gotta revert this

gkm edited the summary of this revision. (Show Details)Nov 5 2021, 11:41 PM
gkm added a reviewer: int3.Nov 6 2021, 7:52 AM
int3 added a comment.Nov 6 2021, 4:23 PM

The Map instead of Vec thing was intentional... a vector of pairs is semantically a map.

Not a huge fan of the Parsed stuff either, I don't think there's much confusion about when we are working with parsed vs serialized structures...

As for the offset member... seems kind of hard to evaluate it without the context of the __eh_frame.

gkm added a comment.Nov 6 2021, 5:33 PM

The Map instead of Vec thing was intentional... a vector of pairs is semantically a map.

I was thinking of "map" operationally, i.e., used for associative lookups.

Not a huge fan of the Parsed stuff either, I don't think there's much confusion about when we are working with parsed vs serialized structures...

That sounds like mitigated speech for "veto". Correct me if I misinterpret.

As for the offset member... seems kind of hard to evaluate it without the context of the __eh_frame.

FYI, it morphed into an address member. I'll put-up a new __eh_frame diff.

gkm added a comment.Nov 7 2021, 7:13 AM

Not a huge fan of the Parsed stuff either, I don't think there's much confusion about when we are working with parsed vs serialized structures...

Prepending Parsed was a late change and I'm OK with reversing it. My concern was that Section is misleading, since it is not a base class of InputSection or OutputSection as the name might suggest. No big deal though. The more important change was s/Section/SectionHeader/ for the template arg.

I'm a big fan of the variable names switching from s/subsections/sections, s/subsectionMap/subsections. It's clearer to me that there are layers whereas the original subsections->subsectionMap layer is not as easily decipherable. Section->SectionHeader makes sense to me, but I would like it if we had some consistency, which means also renaming SegmentCommand->SegmentCommandHeader, NList->NListHeader, etc.

gkm added a comment.Nov 7 2021, 12:50 PM

I'm a big fan of the variable names switching from s/subsections/sections, s/subsectionMap/subsections. It's clearer to me that there are layers whereas the original subsections->subsectionMap layer is not as easily decipherable.

Yes, that really is the crux of this effort. It is good to hear that this change helps newcomers to the code.

Section->SectionHeader makes sense to me, but I would like it if we had some consistency, which means also renaming SegmentCommand->SegmentCommandHeader, NList->NListHeader, etc.

In InputFiles.cpp, sections are front & center, and disambiguating the several varieties of them is important. Section->SectionHeader has value because it offloads the overloaded name Section.

OTOH, SegmentCommand is not overloaded, and used only once. Nlist is not overloaded, and isn't even a header but rather an element in a vector.

OTOH, SegmentCommand is not overloaded, and used only once. Nlist is not overloaded, and isn't even a header but rather an element in a vector.

Makes sense. I guess what I was getting at is whether we can make it more obvious that Nlist or SegmentCommand are structures defined from MachOStructs.h. If this is the best we can do, then it's fine as is, but assuming we could find such a pattern, then it could also apply to Section to make it consistent.

gkm updated this revision to Diff 385404.EditedNov 7 2021, 11:46 PM

Major FUBAR revision -- ignore this one.

gkm updated this revision to Diff 385406.Nov 7 2021, 11:53 PM

Revert previous revision

oontvoo added a subscriber: oontvoo.Nov 8 2021, 6:57 AM

also +1 to the s/section/subsection and SectionHeader renaming(much clearer now)

I've no strong opinion on Map vs Vec but generally I'm not a fan of putting the type (ie., vect, list, map, etc) into the variable name anyway. Seems redundant.
Perhaps if we could make the name clear enough, then it wouldn't be necessary to add the extra clarification.

lld/MachO/InputSection.cpp
80

IMHO, it is not particularly clearer to say ParsedSection here

int3 added a comment.Nov 8 2021, 10:42 AM

That sounds like mitigated speech for "veto". Correct me if I misinterpret.

It's vote, not veto ;)

Yeah the s/subsections/sections/ and s/subsecMap/subsections/ thing looks reasonable. +1 to Vy's opinion that types should ideally be left out of the name anyway; let's drop both the Map and Vec.

I agree with Vincent's opinion that Section -> SectionHeader breaks consistency with the other MachOStructs.h types. The convention I was going for was to have all non-serialized section/segment types be prefixed by "Input" or "Output". But I don't feel super strongly about this, so if you prefer SectionHeader, go for it.

gkm updated this revision to Diff 387341.Nov 15 2021, 11:39 AM
gkm edited the summary of this revision. (Show Details)
  • revert Parsed prefix to shorten class names and lower impact of this diff
thevinster accepted this revision.Nov 15 2021, 12:34 PM

Gucci.

lld/MachO/InputSection.cpp
80

Missed this part?

This revision is now accepted and ready to land.Nov 15 2021, 12:34 PM

Gucci.

👜 ???

oontvoo accepted this revision.Nov 15 2021, 12:42 PM
oontvoo added inline comments.
lld/MachO/InputSection.cpp
80

right :) maybe just print the invalid type here

int3 accepted this revision.Nov 15 2021, 3:28 PM

Thanks!

gkm marked 2 inline comments as done.Nov 16 2021, 6:07 AM
gkm updated this revision to Diff 387597.Nov 16 2021, 6:09 AM
  • fix one remaining s/ParsedSection/Section/
This revision was landed with ongoing or failed builds.Nov 16 2021, 6:09 AM
This revision was automatically updated to reflect the committed changes.