Page MenuHomePhabricator

[LoopIdiomRecognize] Avoid trying to create a pattern from the address of a thread local.
Needs ReviewPublic

Authored by bobsayshilol on Oct 27 2019, 4:22 PM.



The address of a thread local is technically constant however its value isn't known until the thread of execution starts, so constructing a GlobalVariable containing its address doesn't make sense unless it is also thread local and initialised at thread startup. LLVM doesn't appear to have a concept of TLS init like it does for static construction and destruction (ie something similar to llvm::appendToGlobal{C,D}tors() but for TLS) and clang's TLS init code isn't available down in LLVM, so this patch takes the easy option and avoids the issue altogether by rejecting values that are thread locals.

A better demonstration of where this code breaks is with:

#include <cstdio>

thread_local int global_var;

void test_func(int*(&ptrs)[2])
  printf("&global_var = %p\n", &global_var);
  for (auto& p : ptrs)
    p = &global_var;

int main()
  printf("test[0] = %p\n", test[0]);

which, on platforms that support memset_pattern16() and with optimisations enabled, will print something like:

&global_var = 0x7f1e51f77f3c
test[0] = 0x403de0

where any reads/writes of *test[0] would indeed crash at the mysterious address 0x403de0.

Diff Detail

Event Timeline

bobsayshilol created this revision.Oct 27 2019, 4:22 PM

I'm not the best reviewer for this, and i'm not sure who is the code owner for that pass..


What about GlobalVariable's that aren't global constants?

bobsayshilol added inline comments.Oct 28 2019, 4:27 PM

Can you give an example? GlobalVariable inherits from Constant since their address is constant, so my understanding was that all GlobalVariables are therefore (global) Constants.

lebedev.ri added inline comments.Nov 1 2019, 12:01 PM

I mean, is there any other Constant (other than this GlobalVariable->isThreadDependent())
that could lead to similar miscompiles?

bobsayshilol added a subscriber: hans.
bobsayshilol added inline comments.

Good question! Looking at other uses of isThreadDependent() brought me to ValidLookupTableConstant() in SimplifyCFG.cpp which looks like the thing we should be doing here rather than just checking for TLS. The TLS part of the check was added in D4220 for a similar reason to this patch, so I'll add @hans in case they have any words of wisdom. Assuming that is the correct approach, where should ValidLookupTableConstant() be moved to so that it can be used here? ie is it OK to simply remove the static and declare it in the header?

hans added inline comments.Nov 5 2019, 2:23 AM

It does sound like you want essentially the same as ValidLookupTableConstant() here. I guess moving that to the header file would work, but maybe we could find a better name and place for what it's trying to do?

I guess it's really trying to check if the constant can be represented as constant data (which can vary depending on the backend.) Maybe it could be turned into a utility function in Constant itself somehow?

bobsayshilol added inline comments.Wed, Nov 13, 4:00 PM

Yeah that sounds like the best approach, making it a method of Constant. But I'm terrible at naming things :P Does something like isCompileTimeConstant() or isValueKnownAtCompileTime() get across the meaning we're after?

Also I might be reading the comment in ARMTTIImpl::shouldBuildLookupTablesForConstant() wrong, but it sounds like that would be an appropriate name there too vs shouldBuildLookupTablesForConstant() since the relocation model is known at compile time so we'll know whether or not we can know the value of the Constant at compile time (and also because we'll need to call it and it's no longer specific to just lookup tables).

hans added inline comments.Thu, Nov 14, 2:40 AM

isCompileTimeConstant() is not a great improvement over Constant in general I think.. that's really what Constant mean, isn't it?

isValueKnownAtCompileTime() I'm not sure this is better. For many constants the real value is not known, and requires a run-time relocation.

I think a name that focuses on whether the value is suitable for a lookup table is maybe the easiest. The tricky part is that the decision also depends on TargetTransformInfo. I'm not sure we'd want that dependency from Constant :-/