This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO][AutoFDO] Fix memory corruption due to race condition from thin backends
ClosedPublic

Authored by wenlei on Aug 6 2019, 11:31 PM.

Details

Summary

This commit fixed a race condition from multi-threaded thinLTO backends that causes non-deterministic memory corruption for a data structure used only by AutoFDO with compact binary profile.
GUIDToFuncNameMap, a static data member of type DenseMap in FunctionSamples is used as a per-module mapping from function name MD5 to name string when input AutoFDO profile is in compact binary format. However with ThinLTO, we can have parallel backends modifying and accessing the class static map concurrently. The fix is to mark GUIDToFuncNameMap as thread_local.

Event Timeline

wenlei created this revision.Aug 6 2019, 11:31 PM
twoh added a subscriber: twoh.Aug 6 2019, 11:32 PM
wmi added a comment.Aug 8 2019, 8:44 AM

Thanks for working on the issue.

From reading the comment of LLVM_THREAD_LOCAL in include/llvm/Support/Compiler.h, it seems we should only use TLS for POD if we want it to be functioning as we expect on all platforms.

How about change GUIDToFuncNameMap and CurrentModule as member fields in class GUIDToFuncNameMapper so we can create them separately for each module? Add a unique_ptr of GUIDToFuncNameMapper in SampleProfileLoader and a pointer field of type "GUIDToFuncNameMapper *" for each FunctionSamples object, so FunctionSamples object can access GUIDToFuncNameMap and CurrentModule via the pointer of GUIDToFuncNameMapper. The initialization of GUIDToFuncNameMapper pointer of each FunctionSamples object can be done in GUIDToFuncNameMapper's constructor (need to pass Reader as a param to get all the FunctionSamples).

wenlei added a comment.Aug 8 2019, 8:56 AM
In D65848#1621227, @wmi wrote:

Thanks for working on the issue.

From reading the comment of LLVM_THREAD_LOCAL in include/llvm/Support/Compiler.h, it seems we should only use TLS for POD if we want it to be functioning as we expect on all platforms.

How about change GUIDToFuncNameMap and CurrentModule as member fields in class GUIDToFuncNameMapper so we can create them separately for each module? Add a unique_ptr of GUIDToFuncNameMapper in SampleProfileLoader and a pointer field of type "GUIDToFuncNameMapper *" for each FunctionSamples object, so FunctionSamples object can access GUIDToFuncNameMap and CurrentModule via the pointer of GUIDToFuncNameMapper. The initialization of GUIDToFuncNameMapper pointer of each FunctionSamples object can be done in GUIDToFuncNameMapper's constructor (need to pass Reader as a param to get all the FunctionSamples).

Thanks for review. Using thread_local is the "minimum" fix, I actually implemented something similar to what you suggested (under testing), except I let GUIDToFuncNameMap be a member of SampleProfileLoader, and keep a reference in GUIDToFuncNameMapper. That way we can avoid TLS overhead. I will update once testing is done.

wenlei updated this revision to Diff 214173.Aug 8 2019, 9:34 AM

avoid TLS and move GUIDToFuncNameMapper to class member

wmi added a comment.Aug 8 2019, 12:27 PM

Thanks for the change.

llvm/include/llvm/ProfileData/SampleProf.h
383

If we can avoid adding the param, that will be great. GUIDToFuncNameMap is only used when the format is compact. When the interface of findInlinedFunctions is used, I hope the user doesn't have to worry about how to create a GUIDToFuncNameMap. That is why I think we may pass the handle of GUIDToFuncNameMap to FunctionSamples objects in GUIDToFuncNameMapper constructor.

wenlei updated this revision to Diff 214290.Aug 8 2019, 9:10 PM

let GUIDToFuncNameMapper populate GUIDToFuncNameMap field of FunctionSamples.

wenlei marked an inline comment as done.Aug 8 2019, 9:20 PM
wenlei added inline comments.
llvm/include/llvm/ProfileData/SampleProf.h
383

Agreed it's better to avoid this extra param. Changed to populate GUIDToFuncNameMap for FunctionSamples in GUIDToFuncNameMapper's ctor now. We have to traverse all inline trees though to cover all FunctionSamples, which is a bit heavy and why I didn't do this in previous version. But this is cleaner for sure - thanks! I also moved GUIDToFuncNameMapper out of FunctionSamples.

wmi accepted this revision.Aug 12 2019, 8:38 AM

LGTM.

llvm/include/llvm/ProfileData/SampleProf.h
451

Please add some assertion message.

This revision is now accepted and ready to land.Aug 12 2019, 8:38 AM
wenlei updated this revision to Diff 214661.Aug 12 2019, 10:25 AM
wenlei marked an inline comment as done.

add message for assertion.

This revision was automatically updated to reflect the committed changes.
wenlei marked an inline comment as done.