[llvm] Support forward-referenced globals with dso_local_equivalent

[leonardchan]

Summary

See https://github.com/llvm/llvm-project/issues/57815.

dso_local_equivalent would fail with an assertion on forward-referenced globals. This is an issue that only comes up in textual IR, which is why we've never seen this assertion with clang.

[MaskRay]

llvm/lib/AsmParser/LLParser.cpp

220

Use a range-based for loop and clear ForwardRefDSOLocalEquivalents?

227

This line has no test coverage.

230

The error is not tested.

234

The error is not tested.

[leonardchan]

llvm/lib/AsmParser/LLParser.cpp

220

Right. Had this but forgot to update the diff.

[MaskRay]

llvm/lib/AsmParser/LLParser.cpp

3459

This may use try_emplace with structured bindings since we can use C++17.

llvm/test/CodeGen/X86/dso_local_equivalent_undefined_func.ll

1 ↗

(On Diff #461732)

Instead of having 3 test files, if merging improves readability, check out split-file. I think at the least the two invalid tests should be combined.

[leonardchan]

llvm/test/CodeGen/X86/dso_local_equivalent_undefined_func.ll

1 ↗

(On Diff #461732)

First time learning about this. Very useful!

[nikic]

These tests should be in llvm/test/Assembler and preferably test round-trip of llvm-as and llvm-dis, as well as verify-uselistorder.

[leonardchan]

In D134234#3806864, @nikic wrote:

These tests should be in llvm/test/Assembler and preferably test round-trip of llvm-as and llvm-dis, as well as verify-uselistorder.

Added llvm-as/dis tests and verify-uselistorder. I still think the codegen tests are necessary though since we still want to test the lowered calls are properly calling the right thing with/without plts.

llvm/include/llvm/AsmParser/LLParser.h

private:
     // a list of references to blocks in that function.
     std::map<ValID, std::map<ValID, GlobalValue *>> ForwardRefBlockAddresses;
     class PerFunctionState;
     /// Reference to per-function state to allow basic blocks to be
     /// forward-referenced by blockaddress instructions within the same
     /// function.
     PerFunctionState *BlockAddressPFS;
 
+    // References to dso_local_equivalent. The key is the global's ValID, the
+    // value is a placeholder value that will be replaced.
+    std::map<ValID, GlobalValue *> ForwardRefDSOLocalEquivalents;
+
     // Attribute builder reference information.
     std::map<Value*, std::vector<unsigned> > ForwardRefAttrGroups;
     std::map<unsigned, AttrBuilder> NumberedAttrBuilders;
 
     // Summary global value reference information.
     std::map<unsigned, std::vector<std::pair<ValueInfo *, LocTy>>>
         ForwardRefValueInfos;
     std::map<unsigned, std::vector<std::pair<AliasSummary *, LocTy>>>

llvm/lib/AsmParser/LLParser.cpp

bool LLParser::validateEndOfModule(bool UpgradeDebugInfo) {
   }
 
   // If there are entries in ForwardRefBlockAddresses at this point, the
   // function was never defined.
   if (!ForwardRefBlockAddresses.empty())
     return error(ForwardRefBlockAddresses.begin()->first.Loc,
                  "expected function name in blockaddress");
 
+  // If there are entries in ForwardRefDSOLocalEquivalents at this point, they
+  // are references after the function was defined.  Resolve those now.
+  for (auto &Iter : ForwardRefDSOLocalEquivalents) {
+    GlobalValue *GV = nullptr;
+    const ValID &GVRef = Iter.first;
+    if (GVRef.Kind == ValID::t_GlobalName)
+      GV = M->getNamedValue(GVRef.StrVal);
+    else if (GVRef.UIntVal < NumberedVals.size())
+      GV = dyn_cast<GlobalValue>(NumberedVals[GVRef.UIntVal]);
+
+    if (!GV)
+      return error(GVRef.Loc, "unknown function '" + GVRef.StrVal +
+                                  "' referenced by dso_local_equivalent");
+
+    if (!GV->getValueType()->isFunctionTy())
+      return error(GVRef.Loc,
+                   "expected a function, alias to function, or ifunc "
+                   "in dso_local_equivalent");
+
+    auto *Equiv = DSOLocalEquivalent::get(GV);
+    Iter.second->replaceAllUsesWith(Equiv);
+    Iter.second->eraseFromParent();
+  }
+  ForwardRefDSOLocalEquivalents.clear();
+
   for (const auto &NT : NumberedTypes)
     if (NT.second.second.isValid())
       return error(NT.second.second,
                    "use of undefined type '%" + Twine(NT.first) + "'");
 
   for (StringMap<std::pair<Type*, LocTy> >::iterator I =
        NamedTypes.begin(), E = NamedTypes.end(); I != E; ++I)
     if (I->second.second.isValid())
case lltok::kw_dso_local_equivalent: {
     GlobalValue *GV = nullptr;
     if (Fn.Kind == ValID::t_GlobalID) {
       if (Fn.UIntVal < NumberedVals.size())
         GV = NumberedVals[Fn.UIntVal];
     } else if (!ForwardRefVals.count(Fn.StrVal)) {
       GV = M->getNamedValue(Fn.StrVal);
     }
 
-    assert(GV && "Could not find a corresponding global variable");
+    if (!GV) {
+      // Make a placeholder global variable as a placeholder for this reference.
+      GlobalValue *&FwdRef = ForwardRefDSOLocalEquivalents
+                                 .try_emplace(std::move(Fn), nullptr)
+                                 .first->second;
+      if (!FwdRef) {
+        FwdRef = new GlobalVariable(*M, Type::getInt8Ty(Context), false,
+                                    GlobalValue::InternalLinkage, nullptr, "",
+                                    nullptr, GlobalValue::NotThreadLocal);
+      }
+
+      ID.ConstantVal = FwdRef;
+      ID.Kind = ValID::t_Constant;
+      return false;
+    }
 
     if (!GV->getValueType()->isFunctionTy())
       return error(Fn.Loc, "expected a function, alias to function, or ifunc "
                            "in dso_local_equivalent");
 
     ID.ConstantVal = DSOLocalEquivalent::get(GV);
     ID.Kind = ValID::t_Constant;
     return false;

llvm/test/CodeGen/X86/dso_local_equivalent.ll

 }
 
 ; CHECK: call_dso_local_ifunc_func:
 ; CHECK:   callq dso_local_ifunc_func{{$}}
 define void @call_dso_local_ifunc_func() {
   call void dso_local_equivalent @dso_local_ifunc_func()
   ret void
 }
+
+;; PR57815
+;; Ensure dso_local_equivalent works the exact same way as the previous
+;; examples but with forward-referenced symbols.
+
+; CHECK: call_forward_func:
+; CHECK:   callq forward_extern_func@PLT
+define void @call_forward_func() {
+  call void dso_local_equivalent @forward_extern_func()
+  ret void
+}
+
+; CHECK: call_forward_hidden_func:
+; CHECK:   callq forward_hidden_func{{$}}
+define void @call_forward_hidden_func() {
+  call void dso_local_equivalent @forward_hidden_func()
+  ret void
+}
+
+; CHECK: call_forward_protected_func:
+; CHECK:   callq forward_protected_func{{$}}
+define void @call_forward_protected_func() {
+  call void dso_local_equivalent @forward_protected_func()
+  ret void
+}
+
+; CHECK: call_forward_dso_local_func:
+; CHECK:   callq forward_dso_local_func{{$}}
+define void @call_forward_dso_local_func() {
+  call void dso_local_equivalent @forward_dso_local_func()
+  ret void
+}
+
+; CHECK: call_forward_internal_func:
+; CHECK:   callq forward_internal_func{{$}}
+define void @call_forward_internal_func() {
+  call void dso_local_equivalent @forward_internal_func()
+  ret void
+}
+
+declare hidden void @forward_hidden_func()
+declare protected void @forward_protected_func()
+declare dso_local void @forward_dso_local_func()
+define internal void @forward_internal_func() {
+entry:
+  ret void
+}
+define private void @forward_private_func() {
+entry:
+  ret void
+}
+
+; CHECK: call_forward_alias_func:
+; CHECK:   callq forward_alias_func@PLT
+define void @call_forward_alias_func() {
+  call void dso_local_equivalent @forward_alias_func()
+  ret void
+}
+
+; CHECK: call_forward_dso_local_alias_func:
+; CHECK:   callq .Lforward_dso_local_alias_func$local{{$}}
+define void @call_forward_dso_local_alias_func() {
+  call void dso_local_equivalent @forward_dso_local_alias_func()
+  ret void
+}
+
+define void @forward_aliasee_func() {
+entry:
+  ret void
+}
+
+@forward_alias_func = alias void (), ptr @forward_aliasee_func
+@forward_dso_local_alias_func = dso_local alias void (), ptr @forward_aliasee_func
+
+; If an ifunc is not dso_local already, then we should still emit a stub for it
+; to ensure it will be dso_local.
+; CHECK: call_forward_ifunc_func:
+; CHECK:   callq forward_ifunc_func@PLT
+define void @call_forward_ifunc_func() {
+  call void dso_local_equivalent @forward_ifunc_func()
+  ret void
+}
+
+; CHECK: call_forward_dso_local_ifunc_func:
+; CHECK:   callq forward_dso_local_ifunc_func{{$}}
+define void @call_forward_dso_local_ifunc_func() {
+  call void dso_local_equivalent @forward_dso_local_ifunc_func()
+  ret void
+}
+
+@forward_ifunc_func = ifunc void (), ptr @resolver
+@forward_dso_local_ifunc_func = dso_local ifunc void (), ptr @resolver
+
+;; Test "no-named" variables
+; CHECK: call_no_name_hidden:
+; CHECK:   callq __unnamed_{{[0-9]+}}{{$}}
+define void @call_no_name_hidden() {
+  call void dso_local_equivalent @0()
+  ret void
+}
+
+; CHECK: call_no_name_extern:
+; CHECK:   callq __unnamed_{{[0-9]+}}@PLT
+define void @call_no_name_extern() {
+  call void dso_local_equivalent @1()
+  ret void
+}
+
+declare hidden void @0()
+declare void @1()
+
+;; Note that we keep this at the very end because llc emits this after all the
+;; functions.
+; CHECK: const:
+; CHECK:   .long   forward_extern_func@PLT
+@const = constant i32 trunc (i64 ptrtoint (ptr dso_local_equivalent @forward_extern_func to i64) to i32)
+
+declare void @forward_extern_func()

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
+
+;--- undefined_func.ll
+; UNDEFINED: error: unknown function 'undefined_func' referenced by dso_local_equivalent
+define void @call_undefined() {
+  call void dso_local_equivalent @undefined_func()
+  ret void
+}
+
+;--- invalid_arg.ll
+; INVALID: error: expected a function, alias to function, or ifunc in dso_local_equivalent
+define void @call_global_var() {
+  call void dso_local_equivalent @glob()
+  ret void
+}
+
+@glob = constant i32 1