[PATCH] [InstCombine] Fix a crash while generating debug output for replacements with debug info

[Duncan P. N. Exon Smith via llvm-commits]

(resending to the right llvm-commits)

> On 2015-Jul-27, at 16:02, Bjorn Steinbrink <bsteinbr at gmail.com> wrote:
> 
> 
> Add() tries to produce debug output for the new instruction but is
> currently called before the instruction is inserted into the function,
> so it doesn't get a Module reference and goes on to dereference a null
> pointer.

Why does it dereference a null pointer?  This seems worth fixing.

> Moving the call to Add() to a slightly later point avoid this
> problem.
> 
> While at it, also adjust the other debug output that prints the new
> value so that the value is fully setup and has a name instead of being
> output as <badref>.

Is this a separable change?

> ---
> .../InstCombine/InstructionCombining.cpp           | 13 +++++++------
> test/Transforms/InstCombine/debug-debuginfo.ll     | 22 ++++++++++++++++++++++
> 2 files changed, 29 insertions(+), 6 deletions(-)
> create mode 100644 test/Transforms/InstCombine/debug-debuginfo.ll
> 
> <0001-InstCombine-Fix-a-crash-while-generating-debug-outpu.patch>_______________________________________________

> diff --git a/test/Transforms/InstCombine/debug-debuginfo.ll b/test/Transforms/InstCombine/debug-debuginfo.ll
> new file mode 100644
> index 0000000..e894958
> --- /dev/null
> +++ b/test/Transforms/InstCombine/debug-debuginfo.ll
> @@ -0,0 +1,22 @@
> +; RUN: opt -instcombine -debug -disable-output < %s >& /dev/null

Does the `-debug` flag get accepted in non-asserts builds?  Maybe you
need `REQUIRES: asserts` here; I'm not sure.

Also, please add some positive tests via `FileCheck` for the debug
output you're looking for.

> +; This used to crash when it tried to produce debug output for the bitcast
> +; replacement
> +define i32* @test(i8*) {
> +entry:
> +  %1 = bitcast i8* %0 to i16*
> +  %2 = bitcast i16* %1 to i32*, !dbg !2
> +  ret i32* %2
> +}
> +
> +!llvm.module.flags = !{!0, !1}
> +
> +!0 = !{i32 2, !"Dwarf Version", i32 4}
> +!1 = !{i32 2, !"Debug Info Version", i32 3}
> +!2 = !DILocation(line: 2, column: 9, scope: !3)
> +!3 = !DISubprogram(name: "foo", scope: null, line: 1, type: !4)
> +!4 = !DISubroutineType(types: !5)
> +!5 = !{!6, !8}
> +!6 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 64, align: 64)
> +!7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
> +!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !9, size: 64, align: 64)
> +!9 = !DIBasicType(name: "char", size: 8, align: 8, encoding: DW_ATE_signed_char)

Is this debug info relevant?  It doesn't really look valid, since
there's no compile unit or anything.

I wonder if you just need some metadata attachment here, or if you
really need !dbg.  If you do, does this reproduce if generated with
-gline-tables-only?  That at least would get rid of the types.