This is an archive of the discontinued LLVM Phabricator instance.

Fix llvm-link assert failure in BitCodeWriter
AbandonedPublic

Authored by sanwou01 on Sep 28 2020, 8:32 AM.

Details

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.

Diff Detail

Event Timeline

sanwou01 created this revision.Sep 28 2020, 8:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2020, 8:32 AM
sanwou01 requested review of this revision.Sep 28 2020, 8:32 AM

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()?

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)

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()).

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 added inline comments.Sep 28 2020, 12:28 PM
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 added inline comments.Sep 28 2020, 3:29 PM
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 added a subscriber: rsmith.Sep 28 2020, 4:55 PM
rsmith added inline comments.
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 added inline comments.Sep 28 2020, 5:06 PM
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 added a comment.EditedSep 28 2020, 6:26 PM
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 added inline comments.Sep 28 2020, 6:37 PM
llvm/test/Linker/sret-types.ll
1

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

ychen added a subscriber: ychen.Sep 28 2020, 8:19 PM
tpopp added a subscriber: tpopp.Sep 29 2020, 1:34 AM
tpopp added inline comments.
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 added inline comments.Sep 29 2020, 1:52 AM
llvm/test/Linker/sret-types.ll
1

Thanks, I was about to do the same.

sanwou01 abandoned this revision.Sep 29 2020, 2:09 AM