[PATCH] Prevent MDNode's RAUW from introducing a reference to a void Value.

Thu Oct 16 18:22:33 PDT 2014

> On 16 Oct 2014, at 18:08, Eric Christopher <echristo at gmail.com> wrote:
> 
> On Thu, Oct 16, 2014 at 6:06 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>> 
>>> On Oct 16, 2014, at 3:54 PM, Frederic Riss <friss at apple.com> wrote:
>>> 
>>> If you look at the added testcase, it cast a void (*func)() to a int (*func)(). The int typed result is referenced in a debug info metadata node (an argument to the dbg.value intrinsic). Then instcombine would remove the bitcast and turn the int returning function into a void returning function that gets RAUWd over the value in the MDNode. And at this point in the current code you have invalid IR.
>>> 
>>> The code in the testcase is reduced from a huge LTO link that encountered this situation in a much more complicated setup.
>>> 
>>> http://reviews.llvm.org/D5828
>> 
>> Weird.  This LGTM (after fixing a few super minor whitespace changes);
>> not sure if Eric is still looking.
> 
> Enh, I was trying to figure out if there was some way to avoid the
> void metadata node being create rather than avoiding RAUW'ing it.

When the MDNode is created it is perfectly legal (of type int) in this case. What we could do is special case the optimisation to call RAUW with a null value instead of the replacing void value. I’ve seen at least one place doing that. But I thought that preventing the illegal IR from being created altogether was a better choice.

Fred

> It's
> probably not a huge distinction at the moment.
> -eric
> 
>> 
>>> Index: lib/IR/Metadata.cpp
>>> ===================================================================
>>> --- lib/IR/Metadata.cpp
>>> +++ lib/IR/Metadata.cpp
>>> @@ -90,7 +90,11 @@
>>> }
>>> 
>>> void MDNodeOperand::allUsesReplacedWith(Value *NV) {
>>> -  getParent()->replaceOperand(this, NV);
>>> +  // Void type operands aren't allowed anyway.
>> 
>> "anyway" is redundant.  I'd just say:
>> 
>>    // Void type operands aren't allowed.
>> 
>>> +  if (NV->getType()->isVoidTy())
>>> +    deleted();
>>> +  else
>>> +    getParent()->replaceOperand(this, NV);
>>> }
>>> 
>>> //===----------------------------------------------------------------------===//
>>> Index: test/DebugInfo/voidMDNode.ll
>>> ===================================================================
>>> --- /dev/null
>>> +++ test/DebugInfo/voidMDNode.ll
>>> @@ -0,0 +1,65 @@
>>> +; RUN: opt -sroa -instcombine < %s | FileCheck %s
>>> +
>>> +; IR generated from (the obvisouly wrong):
>> 
>> s/obvisouly/obviously/
>> 
>>> +; void foo(void) {}
>>> +; void baz() {
>>> +;    int deadvar = ((int (*)(void))&foo)();
>>> +; }
>>> +
>>> +; Function Attrs: nounwind ssp uwtable
>>> +define void @foo() #0 {
>>> +entry:
>>> +  ret void, !dbg !17
>>> +}
>>> +
>>> +; Function Attrs: nounwind ssp uwtable
>>> +define void @baz() #0 {
>>> +entry:
>>> +  %deadvar = alloca i32, align 4
>>> +; This dbg.declare will be turned in a dbg.value describing the bellow
>>> +; store by sroa. The created dbg.value first argument will be the %call
>>> +; value.
>>> +  call void @llvm.dbg.declare(metadata !{i32* %deadvar}, metadata !18, metadata !19), !dbg !20
>>> +  %call = call i32 bitcast (void ()* @foo to i32 ()*)(), !dbg !21
>>> +; When instcombine removes the bitcast in the call instruction, it will
>>> +; RAUW the %call value with the replacement instruction that is of void
>>> +; type. Check that the dbg.value get's nullified rather that having a
>>> +; bad reference to a void Value.
>>> +;CHECK-NOT: <badref>
>> 
>> Usually we have a space after the `;` for CHECKs.
>> 
>>    ; CHECK-NOT: <badref>
>> 
>>> +  store i32 %call, i32* %deadvar, align 4, !dbg !21
>>> +  ret void, !dbg !22
>>> +}
>>> +
>>> +; Function Attrs: nounwind readnone
>>> +declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
>>> +
>>> +attributes #0 = { nounwind ssp uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
>>> +attributes #1 = { nounwind readnone }
>>> +
>>> +!llvm.dbg.cu = !{!0}
>>> +!llvm.module.flags = !{!14, !15}
>>> +!llvm.ident = !{!16}
>>> +
>>> +!0 = metadata !{metadata !"0x11\0012\00clang version 3.6.0 \000\00\000\00\001", metadata !1, metadata !2, metadata !3, metadata !8, metadata !2, metadata !2} ; [ DW_TAG_compile_unit ] [/tmp/voidmetadata.c] [DW_LANG_C99]
>>> +!1 = metadata !{metadata !"voidmetadata.c", metadata !"/tmp"}
>>> +!2 = metadata !{}
>>> +!3 = metadata !{metadata !4}
>>> +!4 = metadata !{metadata !"0xf\00\000\0064\0064\000\000", null, null, metadata !5} ; [ DW_TAG_pointer_type ] [line 0, size 64, align 64, offset 0] [from ]
>>> +!5 = metadata !{metadata !"0x15\00\000\000\000\000\000\000", null, null, null, metadata !6, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
>>> +!6 = metadata !{metadata !7}
>>> +!7 = metadata !{metadata !"0x24\00int\000\0032\0032\000\000\005", null, null} ; [ DW_TAG_base_type ] [int] [line 0, size 32, align 32, offset 0, enc DW_ATE_signed]
>>> +!8 = metadata !{metadata !9, metadata !13}
>>> +!9 = metadata !{metadata !"0x2e\00foo\00foo\00\002\000\001\000\000\00256\000\002", metadata !1, metadata !10, metadata !11, null, void ()* @foo, null, null, metadata !2} ; [ DW_TAG_subprogram ] [line 2] [def] [foo]
>>> +!10 = metadata !{metadata !"0x29", metadata !1}   ; [ DW_TAG_file_type ] [/tmp/voidmetadata.c]
>>> +!11 = metadata !{metadata !"0x15\00\000\000\000\000\000\000", null, null, null, metadata !12, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
>>> +!12 = metadata !{null}
>>> +!13 = metadata !{metadata !"0x2e\00baz\00baz\00\004\000\001\000\000\000\000\004", metadata !1, metadata !10, metadata !11, null, void ()* @baz, null, null, metadata !2} ; [ DW_TAG_subprogram ] [line 4] [def] [baz]
>>> +!14 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}
>>> +!15 = metadata !{i32 2, metadata !"Debug Info Version", i32 2}
>>> +!16 = metadata !{metadata !"clang version 3.6.0 "}
>>> +!17 = metadata !{i32 2, i32 17, metadata !9, null}
>>> +!18 = metadata !{metadata !"0x100\00deadvar\005\000", metadata !13, metadata !10, metadata !7} ; [ DW_TAG_auto_variable ] [deadvar] [line 5]
>>> +!19 = metadata !{metadata !"0x102"}               ; [ DW_TAG_expression ]
>>> +!20 = metadata !{i32 5, i32 6, metadata !13, null}
>>> +!21 = metadata !{i32 5, i32 16, metadata !13, null}
>>> +!22 = metadata !{i32 6, i32 1, metadata !13, null}

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141016/b7f94fc0/attachment.html>