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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
Thanks for the change.
llvm/include/llvm/ProfileData/SampleProf.h | ||
---|---|---|
383 ↗ | (On Diff #214173) | 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. |
llvm/include/llvm/ProfileData/SampleProf.h | ||
---|---|---|
383 ↗ | (On Diff #214173) | 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. |
LGTM.
llvm/include/llvm/ProfileData/SampleProf.h | ||
---|---|---|
451 ↗ | (On Diff #214290) | Please add some assertion message. |