This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Don't use multiple inheritance for OutputSection. NFC
ClosedPublic

Authored by MaskRay on Feb 28 2022, 1:36 AM.

Details

Summary

Add an OutputDesc class inheriting from SectionCommand. An OutputDesc wraps an
OutputSection.

With this change, we can inline InputSection::getParent.

My lld executable is 2.3KiB smaller.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 28 2022, 1:36 AM
MaskRay requested review of this revision.Feb 28 2022, 1:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2022, 1:36 AM

No objections for the change. I think it makes sense to logically separate OutputSection from SectionCommand. I'd like to see if anyone else has a strong opinion as there are quite a few changes and some more dynamic memory allocations. I'm wondering if there may be a way of avoiding those somehow, for example could an implicit conversion operator to OutputSection* be added?

lld/ELF/OutputSections.h
30

Given that this doesn't use anything about the representation of OutputSection, would it be better to move it to the LinkerScript.h header.

MaskRay updated this revision to Diff 411855.Feb 28 2022, 11:34 AM
MaskRay edited the summary of this revision. (Show Details)

Change OutputSection *osec to OutputSection osec

MaskRay marked an inline comment as done.Feb 28 2022, 11:44 AM

No objections for the change. I think it makes sense to logically separate OutputSection from SectionCommand. I'd like to see if anyone else has a strong opinion as there are quite a few changes and some more dynamic memory allocations. I'm wondering if there may be a way of avoiding those somehow, for example could an implicit conversion operator to OutputSection* be added?

I have switched to

struct OutputDesc final : SectionCommand {
  OutputSection osec;
...
};

make<OutputDesc> is used in replace of previous make<OutputDescription>. There is no extra allocation.

I'm wondering if there may be a way of avoiding those somehow, for example could an implicit conversion operator to OutputSection* be added?

The majority differences are due to dyn_cast<OutputSection> => dyn_cast<OutputDesc> which need changes.

An conversion operator may help a few osd->osec places but they are typically coupled with dyn_cast<OutputDesc>, so the total number of changes may not decrease a lot.
An explicit osd->osec seems clearer to me.

lld/ELF/OutputSections.h
30

OutputDesc now uses OutputSection as a non-pointer data member, so it needs to stay in this file.

MaskRay marked an inline comment as done.Feb 28 2022, 11:44 AM

Just one small comment on some function names, although that is not a strong opinion. No objections to making the changes, will wait a few days before setting approved to see if there are dissenting opinions.

ikudrin accepted this revision.Mar 7 2022, 9:22 AM

LGTM

lld/ELF/Writer.cpp
1171–1172

curSec -> curSecDesc maybe?

This revision is now accepted and ready to land.Mar 7 2022, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 9:22 AM
MaskRay updated this revision to Diff 413882.Mar 8 2022, 11:17 AM
MaskRay marked an inline comment as done.

curSec => curSecDesc

This revision was landed with ongoing or failed builds.Mar 8 2022, 11:23 AM
This revision was automatically updated to reflect the committed changes.