[llvm][LLParser] Fix issue with forward-referenced dso_local_equivalent globals

[leonardchan]

Summary

llc would fail with this IR:

; RUN:llc -mtriple=x86_64-linux-gnu -o - %s
@_ZTV1B_rv = constant i32 trunc (i64 sub (i64 ptrtoint (void ()* dso_local_equivalent @_ZN1B1fEi to i64), i64 ptrtoint (i32* getelementptr inbounds (i32, i32* @_ZTV1B_rv, i32 0) to i64)) to i32)

define void @_ZN1B1fEi() {
   ret void
}

bin/llc: error: bin/llc: /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-2/llvm/test/ThinLTO/X86/dso_local_equivalent.ll:6:66: error: constant expression type mismatch: got type 'i8*' but expected 'void ()*'

The issue is that the forward referenced placeholder globalvariable is being type-checked with _ZN1B1fEi once we finish parsing its name and type. This patch ensures the placeholder will only be established if we know what type it should be. In globals, this is straightforward where the expected type is given before dso_local_equivalent. For calls where we may not know the type of the forward-referenced callee, we can lazily create the placeholder until the type is known.

[nikic]

This seems to add some non-trivial complexity for something that is only relevant for typed pointers. Why do you need this at this point in time?

[leonardchan]

In D135483#3844023, @nikic wrote:

This seems to add some non-trivial complexity for something that is only relevant for typed pointers. Why do you need this at this point in time?

I'm writing tests to get [thin]lto to support relative vtables (D134320). One of the recommendations for ensuring thinlto support was writing equivalent tests to llvm/test/ThinLTO/X86/devirt.ll but using the relative vtable layout which uses dso_local_equivalent, so I'm essentially reusing a lot of what's in that file for testing RV. That is, for each test case I see for a regular vtable I'll test for a relative vtable. It just so happens that test and a lot of the whole program devirtualization tests in general still use typed pointers.

[aeubanks]

can we migrate the WPD tests to use opaque pointers? https://gist.github.com/nikic/98357b71fd67756b0f064c9517b62a34 is the script to do that

[leonardchan]

In D135483#3844223, @aeubanks wrote:

can we migrate the WPD tests to use opaque pointers? https://gist.github.com/nikic/98357b71fd67756b0f064c9517b62a34 is the script to do that

Uploaded https://reviews.llvm.org/D135611 which updates everything under llvm/test/Transforms/WholeProgramDevirt/. Needed some non-trivial manual updating though for a handful of tests. I suspect I'll run into more failures requiring manual tuning if I were to update the thinlto tests as well. What's the current plan for migrating llvm tests to unique pointers? I see on the opaque ptrs doc that by llvm 16, type pointers will no longer be supported but it looks like there's still a lot of llvm tests that still use typed pointers. Will all of them go through a mass migration using this script? One worry I have is I (or other test owners) will need to spend a non-trivial amount of time fixing tests.

[nikic]

This one can probably be abandoned per above discussion?

[leonardchan]

In D135483#3981567, @nikic wrote:

This one can probably be abandoned per above discussion?

Yup

llvm/include/llvm/AsmParser/LLParser.h

     Constant *ConstantVal;
     std::unique_ptr<Constant *[]> ConstantStructElts;
     bool NoCFI = false;
+    bool MakeDSOLocalEquivFwdRef = false;
 
     ValID() = default;
     ValID(const ValID &RHS)
         : Kind(RHS.Kind), Loc(RHS.Loc), UIntVal(RHS.UIntVal), FTy(RHS.FTy),
           StrVal(RHS.StrVal), StrVal2(RHS.StrVal2), APSIntVal(RHS.APSIntVal),
           APFloatVal(RHS.APFloatVal), ConstantVal(RHS.ConstantVal),
-          NoCFI(RHS.NoCFI) {
+          NoCFI(RHS.NoCFI), MakeDSOLocalEquivFwdRef(RHS.MakeDSOLocalEquivFwdRef) {
       assert(!RHS.ConstantStructElts);
     }
 

                              const SmallVector<ParamInfo, 16> &ArgList,
                              FunctionType *&FuncTy);
 
+    GlobalValue *getDSOLocalEquivFwdRef(ValID &Fn, PointerType *ExpectedTy);
+
     // Constant Parsing.
     bool parseValID(ValID &ID, PerFunctionState *PFS,
                     Type *ExpectedTy = nullptr);

llvm/lib/AsmParser/LLParser.cpp

                    "expected a function, alias to function, or ifunc "
                    "in dso_local_equivalent");
 
+    if (FwdRef->getType() != GV->getType())
+      return error(GVRef.Loc, "dso_local_equivalent type does not match global type: got type '" +
+                               getTypeString(GV->getType()) +
+                               "' but expected '" + getTypeString(FwdRef->getType()) + "'");
+
     auto *Equiv = DSOLocalEquivalent::get(GV);
     FwdRef->replaceAllUsesWith(Equiv);
     FwdRef->eraseFromParent();

 // GlobalValue Reference/Resolution Routines.
 //===----------------------------------------------------------------------===//
 
+GlobalValue *LLParser::getDSOLocalEquivFwdRef(ValID &Fn, PointerType *ExpectedTy) {
+  assert(Fn.Kind == ValID::t_GlobalName || Fn.Kind == ValID::t_GlobalID);
+  auto &FwdRefMap = (Fn.Kind == ValID::t_GlobalID)
+                        ? ForwardRefDSOLocalEquivalentIDs
+                        : ForwardRefDSOLocalEquivalentNames;
+  GlobalValue *&FwdRef = FwdRefMap.try_emplace(Fn, nullptr).first->second;
+  if (!FwdRef) {
+    Type *Ty;
+    if (ExpectedTy->isOpaquePointerTy()) {
+      // This is ok since all opaque pointers are treated the same.
+      Ty = Type::getInt8Ty(Context);
+    } else {
+      Ty = ExpectedTy->getNonOpaquePointerElementType();
+    }
+
+    if (auto *FuncTy = dyn_cast<FunctionType>(Ty)) {
+      FwdRef = Function::Create(FuncTy, GlobalValue::InternalLinkage, "", *M);
+    } else {
+      FwdRef = new GlobalVariable(*M, Ty, false,
+                                  GlobalValue::InternalLinkage, nullptr, "",
+                                  nullptr, GlobalValue::NotThreadLocal);
+    }
+  }
+  return FwdRef;
+}
+
 static inline GlobalValue *createGlobalFwdRef(Module *M, PointerType *PTy) {
   // For opaque pointers, the used global type does not matter. We will later
   // RAUW it with a global/function of the correct type.

     }
 
     if (!GV) {
-      // Make a placeholder global variable as a placeholder for this reference.
-      auto &FwdRefMap = (Fn.Kind == ValID::t_GlobalID)
-                            ? ForwardRefDSOLocalEquivalentIDs
-                            : ForwardRefDSOLocalEquivalentNames;
-      GlobalValue *&FwdRef = FwdRefMap.try_emplace(Fn, nullptr).first->second;
-      if (!FwdRef) {
-        FwdRef = new GlobalVariable(*M, Type::getInt8Ty(Context), false,
-                                    GlobalValue::InternalLinkage, nullptr, "",
-                                    nullptr, GlobalValue::NotThreadLocal);
+      if (ExpectedTy) {
+        // We are parsing dso_local_equivalent with a known type given
+        // beforehand (probably from parsing in a global).
+        auto *PtrTy = dyn_cast<PointerType>(ExpectedTy);
+        if (!PtrTy)
+          return error(Fn.Loc, "expected a pointer to function, alias to function, or ifunc "
+                               "in dso_local_equivalent");
+        ID.ConstantVal = getDSOLocalEquivFwdRef(Fn, PtrTy);
+        ID.Kind = ValID::t_Constant;
+        return false;
+      } else {
+        // We are parsing dso_local_equivalent with an unknown type we do not
+        // know at this point. This can happen when parsing some call-like
+        // instruction where the type beforehand is the return type. In this
+        // case, delay creating the fwd ref until we know the type.
+        ID.ConstantVal = nullptr;
+        ID.Kind = Fn.Kind;
+        if (Fn.Kind == ValID::t_GlobalID) {
+          ID.UIntVal = Fn.UIntVal;
+        } else {
+          ID.StrVal = Fn.StrVal;
+        }
+        ID.MakeDSOLocalEquivFwdRef = true;
+        return false;
       }
-
-      ID.ConstantVal = FwdRef;
-      ID.Kind = ValID::t_Constant;
-      return false;
     }
 
     if (!GV->getValueType()->isFunctionTy())

   if (Ty->isFunctionTy())
     return error(ID.Loc, "functions are not values, refer to them as pointers");
 
+  if (ID.MakeDSOLocalEquivFwdRef && !ID.ConstantVal) {
+    // Now that we know the expected type for this function, we can create the
+    // fwd ref.
+    auto *PtrTy = dyn_cast<PointerType>(Ty);
+    if (!PtrTy)
+      return error(ID.Loc, "expected a pointer to function, alias to function, or ifunc "
+                           "in dso_local_equivalent");
+    ID.ConstantVal = getDSOLocalEquivFwdRef(ID, PtrTy);
+    ID.MakeDSOLocalEquivFwdRef = false;
+    ID.Kind = ValID::t_Constant;
+  }
+
   switch (ID.Kind) {
   case ValID::t_LocalID:
     if (!PFS)

llvm/test/CodeGen/X86/dso_local_equivalent.ll

 @const = constant i32 trunc (i64 ptrtoint (ptr dso_local_equivalent @forward_extern_func to i64) to i32)
 
 declare void @forward_extern_func()
+
+; CHECK: const2:
+; CHECK:   .long   forward_func_defined@PLT
+@const2 = constant i32 trunc (i64 sub (i64 ptrtoint (void ()* dso_local_equivalent @forward_func_defined to i64), i64 ptrtoint (i32* getelementptr inbounds (i32, i32* @const2, i32 0) to i64)) to i32)
+
+define void @forward_func_defined() {
+  ret void
+}

llvm/test/CodeGen/X86/dso_local_equivalent_errors.ll

 ; RUN: split-file %s %t
 ; RUN: not llc -mtriple=x86_64-linux-gnu -o - %t/undefined_func.ll 2>&1 | FileCheck %s -check-prefix=UNDEFINED
 ; RUN: not llc -mtriple=x86_64-linux-gnu -o - %t/invalid_arg.ll 2>&1 | FileCheck %s -check-prefix=INVALID
+; RUN: not llc -mtriple=x86_64-linux-gnu -o - %t/invalid_type.ll 2>&1 | FileCheck %s -check-prefix=INVALID-TYPE
+; RUN: not llc -mtriple=x86_64-linux-gnu -o - %t/invalid_type2.ll 2>&1 | FileCheck %s -check-prefix=INVALID-TYPE2
+;; TODO(58209): Explicitly disable opaque pointers for this test to ensure it runs correctly.
+; RUN: not llc -mtriple=x86_64-linux-gnu -o - -opaque-pointers=0 %t/invalid_type3.ll 2>&1 | FileCheck %s -check-prefix=INVALID-TYPE3
+; RUN: not llc -mtriple=x86_64-linux-gnu -o - %t/invalid_type4.ll 2>&1 | FileCheck %s -check-prefix=INVALID-TYPE4
 
 ;--- undefined_func.ll
 ; UNDEFINED: error: unknown function 'undefined_func' referenced by dso_local_equivalent

 }
 
 @glob = constant i32 1
+
+;--- invalid_type.ll
+; INVALID-TYPE: error: expected a function, alias to function, or ifunc in dso_local_equivalent
+@glob = constant i32 trunc (i64 sub (i64 ptrtoint (i32* dso_local_equivalent @const to i64), i64 ptrtoint (i32* @glob to i64)) to i32)
+@const = constant i32 1
+
+;--- invalid_type2.ll
+; INVALID-TYPE2: error: expected a function, alias to function, or ifunc in dso_local_equivalent
+@glob = constant i32 trunc (i64 sub (i64 ptrtoint (i32** dso_local_equivalent @const_ptr to i64), i64 ptrtoint (i32* @glob to i64)) to i32)
+@const_ptr = constant i32* @const
+@const = constant i32 1
+
+;--- invalid_type3.ll
+; INVALID-TYPE3: error: dso_local_equivalent type does not match global type: got type 'i32 ()*' but expected 'void ()*'
+define void @call_global_var() {
+  call void dso_local_equivalent @glob()
+  ret void
+}
+
+define i32 @glob() {
+   ret i32 1
+}
+
+;--- invalid_type4.ll
+; INVALID-TYPE4: error: expected a pointer to function, alias to function, or ifunc in dso_local_equivalent
+@glob = constant i32 trunc (i64 sub (i64 ptrtoint (i32 () dso_local_equivalent @func to i64), i64 ptrtoint (i32* @glob to i64)) to i32)
+
+define i32 @func() {
+   ret i32 1
+}