[PATCH] D77893: [lld] Merge Mach-O input sections
Kellie Medlin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 27 15:39:44 PDT 2020
Ktwu marked 9 inline comments as done.
Ktwu added inline comments.
================
Comment at: lld/MachO/OutputSection.h:41
+ // Some sections may allow coalescing other raw input sections.
+ virtual void mergeInput(InputSection *input);
+
----------------
smeenai wrote:
> Ktwu wrote:
> > smeenai wrote:
> > > Is this needed for anything other than MergedOutputSection?
> > No, it's not needed except for MergedOutputSection. Should I dynamically cast each output section before trying to merge an input section into it instead (I wanted to avoid the runtime hit doing that).
> Yeah, dynamically casting wouldn't be ideal. We could leave this as-is, or make `getOrCreateOutputSection` return a `MergedOutputSection *` instead of an `OutputSection *` (idk if that'd cause other complications).
I believe that would require having an assert that the output class being returned, if already created, is in fact a mergeable section. I think a static_cast would be what we'd want, but I like having the explicit assert here in case something goes wrong instead of the undefined behavior of a static_cast gone wrong.
================
Comment at: lld/MachO/Writer.cpp:328-330
+ getOrCreateOutputSegment(isec->segname)
+ ->getOrCreateOutputSection(isec->name)
+ ->mergeInput(isec);
----------------
smeenai wrote:
> Nit: might be nicer to assign these to variables instead of chaining.
I personally prefer chaining :D
================
Comment at: lld/MachO/Writer.cpp:376
// dyld requires __LINKEDIT segment to always exist (even if empty).
- getOrCreateOutputSegment(segment_names::linkEdit);
- // No more segments can be created after this method runs.
+ auto *linkEditSegment = getOrCreateOutputSegment(segment_names::linkEdit);
+
----------------
smeenai wrote:
> Nit: use an explicit type instead of auto
What's up with `auto`? It's not like it's forbidden from use in the style guide:
https://llvm.org/docs/CodingStandards.html#id27
I'm curious about folks' reasoning behind its usage (or discouragement of).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77893/new/
https://reviews.llvm.org/D77893
More information about the llvm-commits
mailing list