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

[Eric Christopher]

On Thu, Oct 16, 2014 at 6:06 PM, Duncan P. N. Exon Smith
<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. 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}
>>
>