DataFlowSanitizer: wrappers of functions with local linkage should have the same linkage as the function being wrapped

[skerner]

Summary

This patch resolves link errors when the address of a static function is taken, and that function is uninstrumented by DFSan.

This change resolves bug 36314.

[skerner]

Revert a file which was mistakenly included.

[pcc]

It seems like a better fix would be to give the dfsw$ function internal linkage if the wrapped function has internal linkage. That way, if two translation units have static uninstrumented functions with the same name, the references to the dfsw$ functions will be correct.

[skerner]

Address review comment: Wrappers of local functions should have the same linkage as the function being wrapped.

[skerner]

In D44784#1045790, @pcc wrote:

It seems like a better fix would be to give the dfsw$ function internal linkage if the wrapped function has internal linkage. That way, if two translation units have static uninstrumented functions with the same name, the references to the dfsw$ functions will be correct.

Good point. Done.

[skerner]

Peter, friendly ping.

[pcc]

LGTM

I will commit.

include/llvm/IR/GlobalValue.h

protected:
   /// If true then there is a definition within the same linkage unit and that
   /// definition cannot be runtime preempted.
   unsigned IsDSOLocal : 1;
 
 private:
   friend class Constant;
 
   void maybeSetDsoLocal() {
+    // hasLocalLinkage => false
     if (hasLocalLinkage() ||
         (!hasDefaultVisibility() && !hasExternalWeakLinkage()))
       setDSOLocal(true);
   }
 
   // Give subclasses access to what otherwise would be wasted padding.
   // (17 + 4 + 2 + 2 + 2 + 3 + 1 + 1) == 32.
   unsigned SubClassData : GlobalValueSubClassDataBits;

lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

DataFlowSanitizer::buildWrapperFunction(Function *F, StringRef NewFName,
   FunctionType *FT = F->getFunctionType();
   Function *NewF = Function::Create(NewFT, NewFLink, NewFName,
                                     F->getParent());
   NewF->copyAttributesFrom(F);
   NewF->removeAttributes(
       AttributeList::ReturnIndex,
       AttributeFuncs::typeIncompatible(NewFT->getReturnType()));
 
+  // If the function being created has non-local linkage, it should not have the
+  // dso_local attribute.  Explicitly remove dso_local, in case it was set when
+  // attributes were copied from F.
+  if (!NewF->hasLocalLinkage()) {
+    NewF->setDSOLocal(false);
+  }
+
   BasicBlock *BB = BasicBlock::Create(*Ctx, "entry", NewF);
   if (F->isVarArg()) {
     NewF->removeAttributes(AttributeList::FunctionIndex,
                            AttrBuilder().addAttribute("split-stack"));
     CallInst::Create(DFSanVarargWrapperFn,
                      IRBuilder<>(BB).CreateGlobalStringPtr(F->getName()), "",
                      BB);
     new UnreachableInst(*Ctx, BB);

test/Instrumentation/DataFlowSanitizer/Inputs/abilist.txt

 fun:discard*=uninstrumented
 fun:discard*=discard
 
 fun:functional=uninstrumented
 fun:functional=functional
 
 fun:custom*=uninstrumented
 fun:custom*=custom
+
+fun:uninstrumented*=uninstrumented

test/Instrumentation/DataFlowSanitizer/uninstrumented_internal_function.ll

+; RUN: opt < %s -dfsan -dfsan-args-abi -dfsan-abilist=%S/Inputs/abilist.txt -S | FileCheck %s
+; RUN: opt < %s -dfsan                 -dfsan-abilist=%S/Inputs/abilist.txt -S | FileCheck %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define internal i8 @uninstrumented_internal_fun(i8 %in) {
+  ret i8 %in
+}
+
+define i8 @call_uninstrumented_fun(i8 %in) {
+  %call = call i8 @uninstrumented_internal_fun(i8 %in)
+  ret i8 %call
+}
+
+; CHECK: define linkonce_odr {{(i8|{ i8, i16 })}} @"dfsw$uninstrumented_internal_fun"
+; CHECK-NOT: dso_local