Page MenuHomePhabricator

[LLD][COFF] Fix missing cache cleanup in COFF::link()
Needs ReviewPublic

Authored by blackhole12 on Nov 18 2019, 1:32 AM.

Details

Reviewers
ruiu
rnk
Group Reviewers
lld
Summary

After I provided this fix for missing global variable cleanup, two new global caches were introduced that do not get correctly cleaned up. This adds cleanup functions and calls them from COFF::link().

I am increasingly concerned about these kinds of bugs being repeatedly introduced into LLD. If programmers cannot reliably remember to clean up global caches, and there is no test that looks for these bugs, maybe LLD should stop relying on error-prone global variables and instead create a single "global cache" object that must contain all global variables?

Diff Detail

Event Timeline

blackhole12 created this revision.Nov 18 2019, 1:32 AM
smeenai added a subscriber: smeenai.

It's recommended to uploaded patches with full context (e.g. by using git diff -U9999 when uploading a patch using the web interface).

ruiu added a comment.Nov 18 2019, 9:35 PM

I share your concern and I'm sorry about leaving so many global variables as uninitialized in the second run of the linker in the same process. But I'd like to find and fix the problems without abolishing global variables in lld, as I don't actually dislike them. One thing we could do is initializing global variables with invalid data -- like -1 or {nullptr} so that you'd get an error even on the first run if you do not reset them before use. I'll create a patch and send it to you.

lld/COFF/DebugTypes.cpp
93

Instead of defining a new function, you could add TypeServerSource::instances.clear() at the beginning of loadTypeServerSource().

lld/COFF/Writer.cpp
85

Likewise, I'd add outputSections.clear() at the beginning of Writer::run().

Moved the clear commands to where @ruiu suggested. I was unable to upload a full context diff because the web interface refuses to work properly.

@blackhole12 https://llvm.org/docs/GettingStarted.html#sending-patches

apt install arcanist (clone libphutil + clone arcanist)
cd llvm; arc install-certificate
arc diff 'HEAD^' # the comment will update the revision with the full diff

Update with full context

After further testing, this actually doesn't work, but only if you are loading an associated PDB file. I have no idea what's going on, because I don't understand the internals of TypeServerSource, but it's throwing an assertion error at DebugTypes.cpp:191

assert(it != TypeServerSource::instances.end());

My previous fix, which only cleared the caches at the very end of the link() call, does work in this scenario, which suggests to me that the TypeServerSource cache is likely being cleared too often. I highly suggest wrapping this fix into https://reviews.llvm.org/D70766, because it is clear this code is simply far too entwined with itself to properly reason about.