Page MenuHomePhabricator

[LLD] Remove global state in lld/COFF
Needs ReviewPublic

Authored by akhuang on Fri, Sep 24, 3:47 PM.

Details

Reviewers
aganea
MaskRay
Summary

Remove globals from the lldCOFF library, by moving globals into a context class.
This patch mostly moves the config object into COFFLinkerContext.

See https://lists.llvm.org/pipermail/llvm-dev/2021-June/151184.html for
context about removing globals from LLD.

Diff Detail

Event Timeline

akhuang created this revision.Fri, Sep 24, 3:47 PM
akhuang requested review of this revision.Fri, Sep 24, 3:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 24, 3:47 PM
aganea added inline comments.Mon, Sep 27, 10:37 AM
lld/COFF/DLL.cpp
107

I'm seeing that a lot of the *Chunk classes have this extra context now. Have you tried to measure peak RAM usage when linking large executables? Also in scenarios when /Gy is used, for example MinGW seems to use that.

My point being, if we can pass the context by argument that would be better maybe instead of storing it into these tiny objects. But I realize that isn't always practical, thus the global access proposed in D108850 - COFFLinkerContext would simply inherit from CommonLinkingContext and then could be accessed from anywhere by a call to context().

akhuang added inline comments.Tue, Sep 28, 3:05 PM
lld/COFF/DLL.cpp
107

Yeah, it feels non-ideal to store the reference in a bunch of Chunks, I'll try to see whether it's feasible to pass it by argument.

Memory usage doesn't seem to increase too much, though -- peak memory usage for linking chrome (measured with https://github.com/sgraham/tim/blob/master/tim.exe) goes up by 4mb (3972mb -> 3976mb). I think chrome builds with /Gy.