This is an archive of the discontinued LLVM Phabricator instance.

[LLD] Remove global state in lld/COFF
ClosedPublic

Authored by akhuang on Sep 10 2021, 3:04 PM.

Details

Summary

This patch removes globals from the lldCOFF library, by moving globals
into a context class (COFFLinkingContext) and passing it around wherever
it's needed.

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

I also haven't moved the driver or config variables yet.

Diff Detail

Event Timeline

akhuang created this revision.Sep 10 2021, 3:04 PM
akhuang requested review of this revision.Sep 10 2021, 3:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2021, 3:04 PM
MaskRay added a comment.EditedSep 10 2021, 3:20 PM

LGTM.

For other contexts (e.g. LLVMContext, MCContext), the context argument is more commonly used as the first argument.
If we make it the last argument, we will not be able to use default arguments, though default arguments are not common in lld/.

Minor suggestions, but otherwise LGTM. I assume you've coordinated with @aganea to ensure you aren't duplicating effort.

lld/COFF/COFFLinkerContext.cpp
27

I understand the need for clearGHashes, but clearing the other containers appears unnecessary. I wouldn't bother to explicitly clear them. Their destructors are going to do that.

lld/COFF/DebugTypes.cpp
934–935

It looks like you're passing the entire vector in. I think you meant to pass it by reference (probably even const reference):

const std::vector<TpiSource *> &instances

lld/COFF/Driver.cpp
72–73

Consider deleting the memset rather than commenting it out.

950

Heh. I know why. But it's unimportant nowadays.

akhuang marked an inline comment as done.Sep 10 2021, 4:42 PM
akhuang added inline comments.
lld/COFF/DebugTypes.cpp
934–935

not sure why I decided to pass the vector here instead of just &ctx like I did everywhere else? Might as well do that here, too

lld/COFF/Driver.cpp
72–73

whoops,

akhuang updated this revision to Diff 372036.Sep 10 2021, 4:43 PM

address comments, move order of COFFLinkerContext in argument lists

MaskRay accepted this revision.Sep 10 2021, 4:44 PM
This revision is now accepted and ready to land.Sep 10 2021, 4:44 PM
amccarth accepted this revision.Sep 13 2021, 10:44 AM

Thanks for the fixes. LGTM.

Thank you Amy for all this!

lld/COFF/CMakeLists.txt
4

Can the following be added here? What would be missing for this to work?

# Enable errors for any global constructors.
add_flag_if_supported("-Werror=global-constructors" WERROR_GLOBAL_CONSTRUCTOR)
lld/COFF/ICF.cpp
39–40

Same question here, move to locals if possible.

lld/COFF/SymbolTable.cpp
37–38

Can this be moved to SymbolTable?

lld/COFF/Writer.cpp
83–84

These commented out bits should probably be removed.

305–306

Can these Timer globals be moved to non-static members of the Writer class?

MaskRay added inline comments.Sep 13 2021, 4:12 PM
lld/COFF/CMakeLists.txt
4

i think the flag switch should be added separately. this can very easily break some builds for some platforms if there is some platform-specific #ifdef.

akhuang updated this revision to Diff 372511.Sep 14 2021, 10:26 AM
akhuang marked an inline comment as done.

Move Timers into local/class scope; modify the Timer class so the root timer
isn't a static class member.

akhuang updated this revision to Diff 372525.Sep 14 2021, 11:51 AM

move a timer to local scope instead of class member

aganea added inline comments.Sep 14 2021, 1:49 PM
lld/COFF/DebugTypes.cpp
1018

Timers cannot be function-local otherwise rootTimer will end up with a list of dangling pointers in lld::Timer::children. The best would be to keep the Timers alive in other structures until Timer::root().print() is called (at the end of LinkerDriver::linkerMain(). The comment applies to all others Timer instances below.

Could you possibly ensure that the output of link.exe @something.rsp /time before & after this patch is the same please? (with inputs that exercise all Timers)

lld/COFF/ICF.cpp
249

Same here, move this Timer into class ICF.

@akhuang: I realize that we don't ever cover usage of /time in tests. That would be a nice addition if you're willing to, but it doesn't have to be in this patch.

akhuang added inline comments.Sep 14 2021, 6:32 PM
lld/COFF/DebugTypes.cpp
1018

ohh right, hmm, I think a lot of the classes also go out of scope before we print the final timers, guess I'll put them all in the Context class...

akhuang updated this revision to Diff 372611.Sep 14 2021, 6:36 PM

move all timers into COFFLinkerContext class,
modify Timer class and print function so it no longer uses a static root timer

aganea accepted this revision.Sep 15 2021, 2:43 PM

LGTM, thanks Amy!

This revision was landed with ongoing or failed builds.Sep 16 2021, 11:02 AM
This revision was automatically updated to reflect the committed changes.

The timers.ll test is failing intermittently, can you look into this? See here for example: https://lab.llvm.org/buildbot/#/builders/139/builds/10288

Seems like the right revision is https://reviews.llvm.org/D109904 ; I'll comment there.