Fix llvm-link assert failure in BitCodeWriter

[sanwou01]

Summary

In some cases, the attribute group table contains a reference to a type
that was not enumerated. Enumerate the type parameters of byval and
sret during the construction of ValueEnumerator. In the case of sret,
its type was not enumerated at all.

This bug was recently introduced by D88241.

[efriedma]

I'm not sure I understand the reason behind moving the EnumerateType call from incorporateFunction to the ValueEnumerator constructor. We don't walk the attributes before that, do we?

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

408

getPointeeInMemoryValueType()?

[dblaikie]

Given the code has explicit cases for byval and sret separately - could you include test coverage for both as well? (maybe rename teh test case and both could be tested in the same file - on different parameters or different functions, etc)

[sanwou01]

In D88423#2298681, @efriedma wrote:

I'm not sure I understand the reason behind moving the EnumerateType call from incorporateFunction to the ValueEnumerator constructor. We don't walk the attributes before that, do we?

In ModuleBitCodeWrite::write(), we call writeAttributeGroupTable() (which looks up the type of the attribute in VE's TypeMap) before writeFunction() (which calls VE::incorporateFunction()).

In D88423#2298842, @dblaikie wrote:

Given the code has explicit cases for byval and sret separately - could you include test coverage for both as well? (maybe rename teh test case and both could be tested in the same file - on different parameters or different functions, etc)

Sure, I'll test both, but I think Eli's suggestion can get rid of the explicit byval and sret cases.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

408

I figured there must be one that works for both, just had no idea which one. Thanks!

[sanwou01]

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

408

Hmm. I can't quite get this to work. getPointeeInMemoryValueType() seems to return a different type than getParamStructRetType()

(gdb) p A.hasAttribute(Attribute::StructRet)
$15 = true
(gdb) p A.getPointeeInMemoryValueType()
$12 = (llvm::Type *) 0xec7860
(gdb) p A.getParamStructRetType()
$14 = (llvm::Type *) 0xec7900
(gdb) p A.getParamStructRetType()->dump()
%"type 0xec7900" = type { i32 }
(gdb) p A.getPointeeInMemoryValueType()->dump()
%v = type { i32 }

I'm not sure what's going on there...

[arsenm]

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

410

This would also be a problem for preallocated (but I think inalloca is still missing the type)

974–975

Why does this move?

[rsmith]

llvm/test/Linker/sret-types.ll

1

Simpler testcase (from https://reviews.llvm.org/D88241#2299465):

$ cat c.ll
define void @f(i8* sret({i64})) { ret void }
$ ./build/bin/opt - -o /tmp/tmp.ir # or anything that converts text IR to bitcode

There appear to be two issues here: the linker is not properly linking types inside attributes, and the value enumerator doesn't number types inside attributes. (I'm not sure if the linker relies on the value enumerator; maybe this patch fixes both issues, maybe not.)

[dblaikie]

llvm/test/Linker/sret-types.ll

1

If you have a chance, could you try this patch with both the test cases you posted over there? See if it does address both issues?

[rsmith]

This comment has been deleted.

llvm/test/Linker/sret-types.ll

1

This patch does *not* fix the failure of llvm-link to link together the modules properly. Indeed, this test fails if we:

-; RUN: llvm-link %s %p/Inputs/sret-types-1.ll | llvm-dis | FileCheck %s
+; RUN: llvm-link %s %p/Inputs/sret-types-1.ll -S -o - | FileCheck %s

... because it generates incorrect output that looks like:

; ModuleID = 'llvm-link'
source_filename = "llvm-link"

%v = type { i32 }

define void @g(%v* %0) {
  ret void
}

define void @f(%v* sret(%"type 0x2c18b30") %0) {
  ret void
}

So I guess there are actually *three* different bugs here: fixing the issue for the bitcode writer (as I can confirm this patch does) does not fix the corresponding issue for the textual IR printing.

[dblaikie]

llvm/test/Linker/sret-types.ll

1

Sounds like enough for me to be on board with reverting the original patch for now.

[tpopp]

llvm/test/Linker/sret-types.ll

1

I have gone ahead and reverted the original patch for now. I apologize for any difficulties this causes.

[sanwou01]

llvm/test/Linker/sret-types.ll

1

Thanks, I was about to do the same.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp

for (const auto &I : MDs)
       // FIXME: Pass GV to EnumerateMetadata and arrange for the bitcode writer
       // to write metadata to the global variable's own metadata block
       // (PR28134).
       EnumerateMetadata(nullptr, I.second);
   }
 
   // Enumerate types used by function bodies and argument lists.
   for (const Function &F : M) {
-    for (const Argument &A : F.args())
+    for (const Argument &A : F.args()) {
       EnumerateType(A.getType());
 
+      if (A.hasAttribute(Attribute::ByVal))
+        EnumerateType(A.getParamByValType());
+
+      if (A.hasAttribute(Attribute::StructRet))
+        EnumerateType(A.getParamStructRetType());
+    }
+
     // Enumerate metadata attached to this function.
     MDs.clear();
     F.getAllMetadata(MDs);
     for (const auto &I : MDs)
       EnumerateMetadata(F.isDeclaration() ? nullptr : &F, I.second);
 
     for (const BasicBlock &BB : F)
       for (const Instruction &I : BB) {
void ValueEnumerator::incorporateFunction(const Function &F) {
 
   // Add global metadata to the function block.  This doesn't include
   // LocalAsMetadata.
   incorporateFunctionMetadata(F);
 
   // Adding function arguments to the value table.
   for (const auto &I : F.args()) {
     EnumerateValue(&I);
-    if (I.hasAttribute(Attribute::ByVal))
-      EnumerateType(I.getParamByValType());
   }
   FirstFuncConstantID = Values.size();
 
   // Add all function-level constants to the value table.
   for (const BasicBlock &BB : F) {
     for (const Instruction &I : BB) {
       for (const Use &OI : I.operands()) {
         if ((isa<Constant>(OI) && !isa<GlobalValue>(OI)) || isa<InlineAsm>(OI))

llvm/test/Linker/Inputs/sret-types-1.ll

+%v = type { i32 }
+
+define void @f(%v* sret(%v) %0) {
+  ret void
+}

llvm/test/Linker/sret-types.ll

+; RUN: llvm-link %s %p/Inputs/sret-types-1.ll | llvm-dis | FileCheck %s
+
+%v = type { i32 }
+
+define void @g(%v* %0) {
+  ret void
+}
+
+; CHECK: define void @g(%v* %0)
+; CHECK: define void @f(%v* sret(%v) %0)