LowerTypeTests: Drop function type metadata only if we're going to replace it.

[pcc]

Summary

Previously we were (mis)handling jump table members with a prevailing
definition in a full LTO module and a non-prevailing definition in a
ThinLTO module by dropping type metadata on those functions entirely,
which would cause type tests involving such functions to fail.

This patch causes us to drop metadata only if we are about to replace
it with metadata from cfi.functions.

We also want to replace metadata for available_externally functions,
which can arise in the opposite scenario (prevailing ThinLTO
definition, non-prevailing full LTO definition). The simplest way
to handle that is to remove the definition; there's little value in
keeping it around at this point (i.e. after most optimization passes
have already run) and later code will try to use the function's linkage
to create an alias, which would result in invalid IR if the function
is available_externally.

Fixes PR33832.

[tejohnson]

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

1485 ↗

(On Diff #107237)

ExportSummary is the full LTO case? Can you note that somewhere? I forget where that is documented (it isn't where ExportSummary and ImportSummary are declared in LowerTypeTestsModule and LowerTypeTests).

1515 ↗

(On Diff #107237)

I'm assuming this change is to handle your first case:
"jump table members with a prevailing
definition in a full LTO module and a non-prevailing definition in a
ThinLTO module...This patch causes us to drop metadata only if we are about to replace
it with metadata from cfi.functions."

I'm a bit confused on where here indicates that the LTO version is prevailing and there is a non-prevailing def in ThinLTO? Also, is M the full LTO module or the ThinLTO Module? I was assuming the latter but since F is a declaration below, it wouldn't be the case where F was prevailing in this module.

1515 ↗

(On Diff #107237)

Needs comment.

llvm/test/Transforms/LowerTypeTests/export-icall.ll

1 ↗

(On Diff #107237)

Are the changes in this test case testing for both parts of the fix in this patch?

41 ↗

(On Diff #107237)

Without the fix in this patch I guess this would try to create an alias with available_externally linkage?

[pcc]

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

1485 ↗

(On Diff #107237)

Yes, ExportSummary is for full LTO. It's documented here: http://llvm-cs.pcc.me.uk/include/llvm/Transforms/IPO/PassManagerBuilder.h#138

In a separate change I'll add a reference to that where those fields are defined.

1515 ↗

(On Diff #107237)

I'm assuming this change is to handle your first case:

Yes.

Also, is M the full LTO module or the ThinLTO Module?

It is the full LTO module.

I'm a bit confused on where here indicates that the LTO version is prevailing and there is a non-prevailing def in ThinLTO?

If the LTO def is prevailing and the ThinLTO def is non-prevailing, we will see a function definition here (because it will have been copied into the combined module by the IRMover) as well as a definition entry in cfi.functions for that function (because we will have added it to the full LTO module in the ThinLTO bitcode writer -- we know it is non-prevailing because we can expect to see max 1 prevailing definition per symbol).

It looks like the original code assumed that if cfi.functions contained a definition entry for a function, there must not be a definition in the full LTO module (and therefore we can go ahead and drop the metadata and expect to enter the if statement below to re-add it). But that wasn't accounting for the ThinLTO def potentially being non-prevailing.

1515 ↗

(On Diff #107237)

Will do.

llvm/test/Transforms/LowerTypeTests/export-icall.ll

1 ↗

(On Diff #107237)

Hmm, not quite. There ought to also be a test with a linker definition here. I'll add it.

41 ↗

(On Diff #107237)

Right.

[pcc]

• Address review comments

[tejohnson]

LGTM

llvm/trunk/lib/Transforms/IPO/LowerTypeTests.cpp

if (CfiFunctionsMD) {
         CfiFunctionLinkage Linkage = P.second.Linkage;
         MDNode *FuncMD = P.second.FuncMD;
         Function *F = M.getFunction(FunctionName);
         if (!F)
           F = Function::Create(
               FunctionType::get(Type::getVoidTy(M.getContext()), false),
               GlobalVariable::ExternalLinkage, FunctionName, &M);
 
-        if (Linkage == CFL_Definition)
-          F->eraseMetadata(LLVMContext::MD_type);
+        // If the function is available_externally, remove its definition so
+        // that it is handled the same way as a declaration. Later we will try
+        // to create an alias using this function's linkage, which will fail if
+        // the linkage is available_externally. This will also result in us
+        // following the code path below to replace the type metadata.
+        if (F->hasAvailableExternallyLinkage()) {
+          F->setLinkage(GlobalValue::ExternalLinkage);
+          F->deleteBody();
+          F->setComdat(nullptr);
+          F->clearMetadata();
+        }
 
+        // If the function in the full LTO module is a declaration, replace its
+        // type metadata with the type metadata we found in cfi.functions. That
+        // metadata is presumed to be more accurate than the metadata attached
+        // to the declaration.
         if (F->isDeclaration()) {
           if (Linkage == CFL_WeakDeclaration)
             F->setLinkage(GlobalValue::ExternalWeakLinkage);
 
-          SmallVector<MDNode *, 2> Types;
+          F->eraseMetadata(LLVMContext::MD_type);
           for (unsigned I = 2; I < FuncMD->getNumOperands(); ++I)
             F->addMetadata(LLVMContext::MD_type,
                            *cast<MDNode>(FuncMD->getOperand(I).get()));
         }
       }
     }
   }
 

llvm/trunk/test/Transforms/LowerTypeTests/export-icall.ll

 ; RUN: opt -S -lowertypetests -lowertypetests-summary-action=export -lowertypetests-read-summary=%S/Inputs/use-typeid1-typeid2.yaml -lowertypetests-write-summary=%t < %s | FileCheck %s
 ; RUN: FileCheck --check-prefix=SUMMARY %s < %t
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @h(i8 %x) !type !2 {
   ret void
 }
 
 declare !type !8 void @f(i32 %x)
+define available_externally void @f2(i32 %x) !type !8 {
+  ret void
+}
+define void @f3(i32 %x) !type !8 {
+  ret void
+}
 
-!cfi.functions = !{!0, !1, !3, !4, !5, !6}
+!cfi.functions = !{!0, !1, !3, !9, !10, !4, !5, !6}
 
 ; declaration of @h with a different type is ignored
 !0 = !{!"h", i8 1, !7}
 
 ; extern_weak declaration of @h with a different type is ignored as well
 !1 = !{!"h", i8 2, !8}
 !2 = !{i64 0, !"typeid1"}
 
-; definition of @f replaces types on the IR declaration above
+; definitions of @f and @f2 replace types on the IR declarations above
 !3 = !{!"f", i8 0, !2}
+!9 = !{!"f2", i8 0, !2}
+!10 = !{!"f3", i8 0, !2}
 !4 = !{!"external", i8 1, !2}
 !5 = !{!"external_weak", i8 2, !2}
 !6 = !{!"g", i8 0, !7}
 !7 = !{i64 0, !"typeid2"}
 !8 = !{i64 0, !"typeid3"}
 
 
 ; CHECK-DAG: @__typeid_typeid1_global_addr = hidden alias i8, bitcast (void ()* [[JT1:.*]] to i8*)
 ; CHECK-DAG: @__typeid_typeid1_align = hidden alias i8, inttoptr (i8 3 to i8*)
-; CHECK-DAG: @__typeid_typeid1_size_m1 = hidden alias i8, inttoptr (i64 3 to i8*)
+; CHECK-DAG: @__typeid_typeid1_size_m1 = hidden alias i8, inttoptr (i64 4 to i8*)
 
 ; CHECK-DAG: @h                    = alias void (i8), bitcast (void ()* [[JT1]] to void (i8)*)
 ; CHECK-DAG: @f                    = alias void (i32), {{.*}}getelementptr {{.*}}void ()* [[JT1]]
+; CHECK-DAG: @f2                   = alias void (i32), {{.*}}getelementptr {{.*}}void ()* [[JT1]]
 ; CHECK-DAG: @external.cfi_jt      = hidden alias void (), {{.*}}getelementptr {{.*}}void ()* [[JT1]]
 ; CHECK-DAG: @external_weak.cfi_jt = hidden alias void (), {{.*}}getelementptr {{.*}}void ()* [[JT1]]
 
 ; CHECK-DAG: @__typeid_typeid2_global_addr = hidden alias i8, bitcast (void ()* [[JT2:.*]] to i8*)
 
 ; CHECK-DAG: @g                    = alias void (), void ()* [[JT2]]
 
 ; CHECK-DAG: define internal void @h.cfi(i8 {{.*}}) !type !{{.*}}
 ; CHECK-DAG: declare !type !{{.*}} void @external()
 ; CHECK-DAG: declare !type !{{.*}} void @external_weak()
 ; CHECK-DAG: declare !type !{{.*}} void @f.cfi(i32)
+; CHECK-DAG: declare !type !{{.*}} void @f2.cfi(i32)
+; CHECK-DAG: define void @f3(i32 {{.*}}) !type !3
+; CHECK-DAG: !3 = !{i64 0, !"typeid3"}
 ; CHECK-DAG: declare !type !{{.*}} void @g.cfi()
 
 
 ; SUMMARY:      TypeIdMap:
 ; SUMMARY-NEXT:   typeid1:
 ; SUMMARY-NEXT:     TTRes:
 ; SUMMARY-NEXT:       Kind:            AllOnes
 ; SUMMARY-NEXT:       SizeM1BitWidth:  7
 ; SUMMARY-NEXT:     WPDRes:
 ; SUMMARY-NEXT:   typeid2:
 ; SUMMARY-NEXT:     TTRes:
 ; SUMMARY-NEXT:       Kind:            Single
 ; SUMMARY-NEXT:       SizeM1BitWidth:  0
 ; SUMMARY-NEXT:     WPDRes:
 
 ; SUMMARY:      CfiFunctionDefs:
 ; SUMMARY-NEXT:   - f
+; SUMMARY-NEXT:   - f2
 ; SUMMARY-NEXT:   - g
 ; SUMMARY-NEXT:   - h
 ; SUMMARY-NEXT: CfiFunctionDecls:
 ; SUMMARY-NEXT:   - external
 ; SUMMARY-NEXT:   - external_weak
 ; SUMMARY-NEXT: ...