Fix line numbers for code inlined from __nodebug__ functions.

[eugenis]

Summary

This is a fix for PR19001.

Instructions from nodebug functions don't have file:line information even when inlined into no-nodebug functions.
As a result, intrinsics (SSE and other) from <*intrin.h> clang headers _never_ have file:line information.

With this change, an instruction without !dbg metadata gets one from the call instruction when inlined.

[dblaikie]

What's the user experience today that this is fixing? Is this any different/worse than a whole (non-inlined) function having no line information?

Any idea what GCC does here?

[dblaikie]

test/Transforms/Inline/inline-line-numbers.ll

2 ↗

(On Diff #10010)

Do you have the original source for this test? I imagine these functions could be a little simpler, maybe? (do they need parameters/return values? It's unlikely the outer function (test2) does, I imagine - the inner function might be slightly simpler, while still having an un-elidable instruction, if it just assigned a constant to a global? Maybe there's some other simple operation it could perform)

[chandlerc]

I'll defer to Dave for getting the actual test case into a form he's happy with, but I think this is the *exact* right change to behavior. always_inlin,nodebug functions should behave as-if their contents were written in the place they are called, and this gives us that behavior.

Among other things this should fix is stepping through a long sequence of _mm_...() calls.

lib/Transforms/Utils/InlineFunction.cpp

472–473

Rather than mentioning *intrin.h functions, I would just say that this is important for ((always_inline, nodebug)) functions to at least have *some* location after inlining.

[eugenis]

I've also included updates to tests in clang and compiler-rt that were earlier sent as a separate CL.

Btw, is there any reason not to commit CLs spanning multiple llvm projects as one? This seems to work perfectly well, but for some reason people often do multiple commits with inconsistent state in between.

lib/Transforms/Utils/InlineFunction.cpp

472–473

done

test/Transforms/Inline/inline-line-numbers.ll

2 ↗

(On Diff #10010)

I've simplified the test and included the original source to simplify future modifications.

[dblaikie]

test/Transforms/Inline/inline-line-numbers.ll

9 ↗

(On Diff #10044)

This seems a strange way to test this - I would've expected just a single assignment in "callee" and the nodebug attribute on it?

Why two assignments and a manual modification to the metadata?

tools/clang/test/CodeGen/sse-builtins.c

9

Does this need to be tested here (and for /every/ intrinsic)? Seems fair to just test this once. (I guess somewhere we test that the intrinsics have nodebug on them in some way already?)

[eugenis]

test/Transforms/Inline/inline-line-numbers.ll

9 ↗

(On Diff #10044)

Because this way I'm testing a function that has instructions both with and without debug metadata. This is not the exact case we are interested in, but a more general one.

tools/clang/test/CodeGen/sse-builtins.c

9

Sounds reasonable, I'll add a separate test for this.

[eugenis]

PTAL

I've also moved the LLVM test under test/DebugInfo - that's where other inliner+!dbg tests are in.

[dblaikie]

test/Transforms/Inline/inline-line-numbers.ll

9 ↗

(On Diff #10044)

Why is that a scenario we need to test? When do we end up with a function both with and without debug metadata?

[eugenis]

Updated the test to avoid manual IR editing.

[dblaikie]

Thanks - looks great. One optional comment.

test/DebugInfo/inline-no-debug-info.ll

23

It's probably worth having a similar check line for the value of A to demonstrate that they're both located at line 3, not line 2? (and if you like, you can omit everything after the field you care about (the first field, in this instance: "CHECK: [[X]] = metadata !{i32 42,") - but I don't feel strongly about it))

[eugenis]

Thanks, committed as r210459.

test/DebugInfo/inline-no-debug-info.ll

23

Done.
I've left all metadata fields to verify that B is an inlined location, and A is not.

[edward-san]

I suppose this can be closed safely, right?

lib/Transforms/Utils/InlineFunction.cpp

static void fixupLineNumbers(Function *Fn, Function::iterator FI,
   DebugLoc TheCallDL = TheCall->getDebugLoc();
   if (TheCallDL.isUnknown())
     return;
 
   for (; FI != Fn->end(); ++FI) {
     for (BasicBlock::iterator BI = FI->begin(), BE = FI->end();
          BI != BE; ++BI) {
       DebugLoc DL = BI->getDebugLoc();
-      if (!DL.isUnknown()) {
+      if (DL.isUnknown()) {
+        // If the inlined instruction has no line number, make it look as if it
+        // originates from the call location. This is important for
+        // ((__always_inline__, __nodebug__)) functions which must use caller
+        // location for all instructions in their function body.
+        BI->setDebugLoc(TheCallDL);
+      } else {
         BI->setDebugLoc(updateInlinedAtInfo(DL, TheCallDL, BI->getContext()));
         if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(BI)) {
           LLVMContext &Ctx = BI->getContext();
           MDNode *InlinedAt = BI->getDebugLoc().getInlinedAt(Ctx);
           DVI->setOperand(2, createInlinedVariable(DVI->getVariable(), 
                                                    InlinedAt, Ctx));
         }
       }

projects/compiler-rt/test/msan/vector_cvt.cc

 // RUN: %clangxx_msan -m64 -O0 %s -o %t && %run %t
 // RUN: %clangxx_msan -DPOSITIVE -m64 -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
 
 #include <emmintrin.h>
 
 int to_int(double v) {
   __m128d t = _mm_set_sd(v);
   int x = _mm_cvtsd_si32(t);
   return x;
   // CHECK: WARNING: MemorySanitizer: use-of-uninitialized-value
-  // CHECK: #{{.*}} in to_int{{.*}}vector_cvt.cc:[[@LINE-4]]
+  // CHECK: #{{.*}} in to_int{{.*}}vector_cvt.cc:[[@LINE-3]]
 }
 
 int main() {
 #ifdef POSITIVE
   double v;
 #else
   double v = 1.1;
 #endif
   double* volatile p = &v;
   int x = to_int(*p);
   return !x;
 }

test/DebugInfo/inline-no-debug-info.ll

+; RUN: opt < %s -inline -S | FileCheck %s
+
+; This was generated from the following source:
+; int a, b;
+; __attribute__((__always_inline__)) static void callee2() { b = 2; }
+; __attribute__((__nodebug__)) void callee() { a = 1; callee2(); }
+; void caller() { callee(); }
+; by running
+;   clang -S test.c -emit-llvm -O1 -gline-tables-only -fno-strict-aliasing
+
+; CHECK-LABEL: @caller(
+
+; This instruction did not have a !dbg metadata in the callee.
+; CHECK: store i32 1, {{.*}}, !dbg [[A:!.*]]
+
+; This instruction came from callee with a !dbg metadata.
+; CHECK: store i32 2, {{.*}}, !dbg [[B:!.*]]
+
+; The remaining instruction from the caller.
+; CHECK: ret void, !dbg [[A]]
+
+; Debug location of the inlined code.
+; CHECK-DAG: [[B]] = metadata !{i32 2, i32 0, metadata !{{[01-9]+}}, metadata !{{[01-9]+}}}
+
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@a = common global i32 0, align 4
+@b = common global i32 0, align 4
+
+; Function Attrs: nounwind uwtable
+define void @callee() #0 {
+entry:
+  store i32 1, i32* @a, align 4
+  store i32 2, i32* @b, align 4, !dbg !11
+  ret void
+}
+
+; Function Attrs: nounwind uwtable
+define void @caller() #0 {
+entry:
+  tail call void @callee(), !dbg !12
+  ret void, !dbg !12
+}
+
+attributes #0 = { nounwind uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!8, !9}
+!llvm.ident = !{!10}
+
+!0 = metadata !{i32 786449, metadata !1, i32 12, metadata !"clang version 3.5.0 (210174)", i1 true, metadata !"", i32 0, metadata !2, metadata !2, metadata !3, metadata !2, metadata !2, metadata !"", i32 2} ; [ DW_TAG_compile_unit ] [/code/llvm/build0/test.c] [DW_LANG_C99]
+!1 = metadata !{metadata !"test.c", metadata !"/code/llvm/build0"}
+!2 = metadata !{}
+!3 = metadata !{metadata !4, metadata !7}
+!4 = metadata !{i32 786478, metadata !1, metadata !5, metadata !"caller", metadata !"caller", metadata !"", i32 4, metadata !6, i1 false, i1 true, i32 0, i32 0, null, i32 0, i1 true, void ()* @caller, null, null, metadata !2, i32 4} ; [ DW_TAG_subprogram ] [line 4] [def] [caller]
+!5 = metadata !{i32 786473, metadata !1}          ; [ DW_TAG_file_type ] [/code/llvm/build0/test.c]
+!6 = metadata !{i32 786453, i32 0, null, metadata !"", i32 0, i64 0, i64 0, i64 0, i32 0, null, metadata !2, i32 0, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
+!7 = metadata !{i32 786478, metadata !1, metadata !5, metadata !"callee2", metadata !"callee2", metadata !"", i32 2, metadata !6, i1 true, i1 true, i32 0, i32 0, null, i32 0, i1 true, null, null, null, metadata !2, i32 2} ; [ DW_TAG_subprogram ] [line 2] [local] [def] [callee2]
+!8 = metadata !{i32 2, metadata !"Dwarf Version", i32 4}
+!9 = metadata !{i32 2, metadata !"Debug Info Version", i32 1}
+!10 = metadata !{metadata !"clang version 3.5.0 (210174)"}
+!11 = metadata !{i32 2, i32 0, metadata !7, null}
+!12 = metadata !{i32 4, i32 0, metadata !4, null}

tools/clang/test/CodeGen/sse-builtins-dbg.c

+// RUN: %clang_cc1 -ffreestanding -triple x86_64-apple-macosx10.8.0 -target-feature +sse4.1 -g -emit-llvm %s -o - | FileCheck %s
+
+// Test that intrinsic calls inlined from _mm_* wrappers have debug metadata.
+
+#include <xmmintrin.h>
+
+__m128 test_rsqrt_ss(__m128 x) {
+  // CHECK: define {{.*}} @test_rsqrt_ss
+  // CHECK: call <4 x float> @llvm.x86.sse.rsqrt.ss({{.*}}, !dbg !{{.*}}
+  // CHECK: ret <4 x float>
+  return _mm_rsqrt_ss(x);
+}

tools/clang/test/CodeGen/sse-builtins.c

-// RUN: %clang_cc1 -ffreestanding -triple x86_64-apple-macosx10.8.0 -target-feature +sse4.1 -g -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -triple x86_64-apple-macosx10.8.0 -target-feature +sse4.1 -emit-llvm %s -o - | FileCheck %s
 
 #include <xmmintrin.h>
 #include <emmintrin.h>
 #include <smmintrin.h>
 
 __m128 test_rsqrt_ss(__m128 x) {
   // CHECK: define {{.*}} @test_rsqrt_ss
   // CHECK: call <4 x float> @llvm.x86.sse.rsqrt.ss
   // CHECK: extractelement <4 x float> {{.*}}, i32 0
   // CHECK: extractelement <4 x float> {{.*}}, i32 1
   // CHECK: extractelement <4 x float> {{.*}}, i32 2
   // CHECK: extractelement <4 x float> {{.*}}, i32 3
   return _mm_rsqrt_ss(x);
 }
 
 __m128 test_rcp_ss(__m128 x) {

 __m128 test_load1_ps(void* y) {
   // CHECK: define {{.*}} @test_load1_ps
   // CHECK: load float* {{.*}}, align 1{{$}}
   return _mm_load1_ps(y);
 }
 
 void test_store_ss(__m128 x, void* y) {
   // CHECK-LABEL: define void @test_store_ss
-  // CHECK: store {{.*}} float* {{.*}}, align 1,
+  // CHECK: store {{.*}} float* {{.*}}, align 1{{$}}
   _mm_store_ss(y, x);
 }
 
 __m128d test_load1_pd(__m128 x, void* y) {
   // CHECK: define {{.*}} @test_load1_pd
   // CHECK: load double* {{.*}}, align 1{{$}}
   return _mm_load1_pd(y);
 }