[safestack] Require TargetMachine to be provided.

[mlemay-intel]

Summary

This simplifies control flow and enables future refactoring to
eliminate some redundancies.

[eugenis]

This crashes opt -safe-stack when the input file does not specify a target triple. This is probably fine... Is it?

[pcc]

We probably want to call report_fatal_error if we were unable to get a TargetMachine.

[mlemay-intel]

Convert assert to report_fatal_error.

[pcc]

LGTM

[mlemay-intel]

The hexagon-build-02 buildbot detected failures caused by this patch: http://lab.llvm.org:8011/builders/llvm-hexagon-elf/builds/34095 The "Target machine is required" error is being reported. I was able to reproduce those failures by building a version of LLVM that only targets Hexagon. I'm surprised by these failures, since I would expect such a toolchain to simply report that the test is unsupported given that X86 triples are specified on the opt command lines. I don't yet know of a good way to revise this patch to resolve those failures while still avoiding duplicating code in D19852.

[pcc]

You could add REQUIRES: x86-registered-target to each of the failing tests

[mlemay-intel]

In D24896#562745, @pcc wrote:

You could add REQUIRES: x86-registered-target to each of the failing tests

I see that none of the tests in test/Transforms/SafeStack/ currently specify any non-X86 target. The only tests that do are in arch-specific subdirectories. So, adding the REQUIRES: x86-registered-target directive could work. However, I wonder whether it will be necessary in the future to add non-X86 targets to the tests in the base directory.

[pcc]

Ah, I didn't see the arch specific directories. In that case I think the simplest thing to do would be to move the existing tests to the X86 directory. If we ever need to run one of those tests on multiple architectures we can always move it back with a REQUIRES directive.

[andrew.w.kaylor]

It looks like the explicit triples are being used in the tests in the non-arch-specific directory simply to force the tests to be run for both x86-64 and i386 targets. This is great for x86, but it means that these tests aren't run at all against other targets, even when another target is the default for the build. That seems bad.

I would be inclined to remove the explicit triple from these tests and look for another way to exercise the 32-bit x86 targets. A quick and dirty way to do this would be to duplicate the tests as they are now in the X86 directory and leave a version without an explicit triple in the non-arch directory. That's not a good long-term solution but I'm not sure what other options are available. There must be other tests that have this same dilemma though.

Is it possible that a much smaller set of tests could verify whatever i386-specific behavior needs to be tested?

[pcc]

It looks like the explicit triples are being used in the tests in the non-arch-specific directory simply to force the tests to be run for both x86-64 and i386 targets. This is great for x86, but it means that these tests aren't run at all against other targets, even when another target is the default for the build. That seems bad.

I would be inclined to remove the explicit triple from these tests and look for another way to exercise the 32-bit x86 targets. A quick and dirty way to do this would be to duplicate the tests as they are now in the X86 directory and leave a version without an explicit triple in the non-arch directory. That's not a good long-term solution but I'm not sure what other options are available. There must be other tests that have this same dilemma though.

Ultimately we do want these tests to run against as many targets as possible, but I think the best way to do that is with a test with multiple RUN lines with different triples, rather than depending on the host triple. The latter seems like a good way to have missing test coverage depending on which platform you happen to be running on.

Moving the existing tests to X86 is a good first step because we don't currently have any tests that target more than one architecture. As we accumulate such tests (either by changing the tests in X86 or by adding new tests) we can move them up one directory.

Is it possible that a much smaller set of tests could verify whatever i386-specific behavior needs to be tested?

That may certainly be true, but it's orthogonal to fixing our present build issue.

[andrew.w.kaylor]

The problem with adding more run lines is that if I understand this failure correctly, the test would fail if any of the run targets was missing from the build configuration. Long term, I think it would be nice if LIT had a way to selectively disable individual run lines based on the build configuration.

[pcc]

The problem with adding more run lines is that if I understand this failure correctly, the test would fail if any of the run targets was missing from the build configuration.

Not necessarily, you could have something like REQUIRES: foo-registered-target, bar-registered-target (but would agree this is suboptimal)

Long term, I think it would be nice if LIT had a way to selectively disable individual run lines based on the build configuration.

Agree

lib/CodeGen/SafeStack.cpp

bool SafeStack::IsSafeStackAlloca(const Value *AllocaPtr, uint64_t AllocaSize) {
   }
 
   // All uses of the alloca are safe, we can place it on the safe stack.
   return true;
 }
 
 Value *SafeStack::getOrCreateUnsafeStackPtr(IRBuilder<> &IRB, Function &F) {
   // Check if there is a target-specific location for the unsafe stack pointer.
-  if (TL)
-    if (Value *V = TL->getSafeStackPointerLocation(IRB))
-      return V;
 
   // Otherwise, assume the target links with compiler-rt, which provides a
   // thread-local variable with a magic name.
   Module &M = *F.getParent();
   const char *UnsafeStackPtrVar = "__safestack_unsafe_stack_ptr";
   auto UnsafeStackPtr =
       dyn_cast_or_null<GlobalVariable>(M.getNamedValue(UnsafeStackPtrVar));
 
if (!UnsafeStackPtr) {
     if (UseTLS != UnsafeStackPtr->isThreadLocal())
       report_fatal_error(Twine(UnsafeStackPtrVar) + " must " +
                          (UseTLS ? "" : "not ") + "be thread-local");
   }
   return UnsafeStackPtr;
 }
 
 Value *SafeStack::getStackGuard(IRBuilder<> &IRB, Function &F) {
-  Value *StackGuardVar = nullptr;
-  if (TL)
-    StackGuardVar = TL->getIRStackGuard(IRB);
+  Value *StackGuardVar = TL->getIRStackGuard(IRB);
   if (!StackGuardVar)
     StackGuardVar =
         F.getParent()->getOrInsertGlobal("__stack_chk_guard", StackPtrTy);
   return IRB.CreateLoad(StackGuardVar, "StackGuard");
 }
 
 void SafeStack::findInsts(Function &F,
                           SmallVectorImpl<AllocaInst *> &StaticAllocas,
bool SafeStack::runOnFunction(Function &F) {
   }
 
   if (F.isDeclaration()) {
     DEBUG(dbgs() << "[SafeStack]     function definition"
                     " is not available\n");
     return false;
   }
 
-  TL = TM ? TM->getSubtargetImpl(F)->getTargetLowering() : nullptr;
+  if (!TM)
+    report_fatal_error("Target machine is required");
+  TL = TM->getSubtargetImpl(F)->getTargetLowering();
   SE = &getAnalysis<ScalarEvolutionWrapperPass>().getSE();
 
   ++NumFunctions;
 
   SmallVector<AllocaInst *, 16> StaticAllocas;
   SmallVector<AllocaInst *, 4> DynamicAllocas;
   SmallVector<Argument *, 4> ByValArguments;
   SmallVector<ReturnInst *, 4> Returns;