[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas updated this revision to Diff 150357.
gramanas added a comment.
Make more elaborate comment.
Repository:
rC Clang
https://reviews.llvm.org/D47097
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGen/debug-info-preserve-scope.c
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+// RUN: %clang_cc1 -O1 -debug-info-kind=limited -emit-llvm -mllvm \
+// RUN: -opt-bisect-limit=2 -o - %s 2> /dev/null | FileCheck %s \
+// RUN: --check-prefix PHI
+
+extern int map[];
+// PHI-LABEL: define void @test1
+void test1(int a, int n) {
+ for (int i = 0; i < n; ++i)
+a = map[a];
+}
+
+// PHI: for.cond:
+// PHI-NEXT: {{.*}} = phi i32 {{.*}} !dbg ![[test1DbgLoc:[0-9]+]]
+
+// PHI: ![[test1DbgLoc]] = !DILocation(line: 0
+
+
+static int i;
+// CHECK-LABEL: define void @test2
+void test2(int b) {
+ i = b;
+}
+
+// CHECK: store i32 {{.*}} !dbg ![[test2DbgLoc:[0-9]+]]
+
+// CHECK: ![[test2DbgLoc]] = !DILocation(line: 0
+
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,10 @@
}
}
+ // Set artificial debug location to preserve the scope. This is later
+ // used by mem2reg to assign DL at the phi's it generates.
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+// RUN: %clang_cc1 -O1 -debug-info-kind=limited -emit-llvm -mllvm \
+// RUN: -opt-bisect-limit=2 -o - %s 2> /dev/null | FileCheck %s \
+// RUN: --check-prefix PHI
+
+extern int map[];
+// PHI-LABEL: define void @test1
+void test1(int a, int n) {
+ for (int i = 0; i < n; ++i)
+a = map[a];
+}
+
+// PHI: for.cond:
+// PHI-NEXT: {{.*}} = phi i32 {{.*}} !dbg ![[test1DbgLoc:[0-9]+]]
+
+// PHI: ![[test1DbgLoc]] = !DILocation(line: 0
+
+
+static int i;
+// CHECK-LABEL: define void @test2
+void test2(int b) {
+ i = b;
+}
+
+// CHECK: store i32 {{.*}} !dbg ![[test2DbgLoc:[0-9]+]]
+
+// CHECK: ![[test2DbgLoc]] = !DILocation(line: 0
+
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,10 @@
}
}
+ // Set artificial debug location to preserve the scope. This is later
+ // used by mem2reg to assign DL at the phi's it generates.
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
aprantl added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1949 + // Set artificial debug location in order to preserve the scope + auto DL = ApplyDebugLocation::CreateArtificial(*this); Can you make that comment elaborate more about why this is being done? For example, you could add an "otherwise mem2reg will ..." Also please note that all comments in LLVM need to be full sentences with a "." at the end :-) Repository: rC Clang https://reviews.llvm.org/D47097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas added a comment. What about this? Ping! Repository: rC Clang https://reviews.llvm.org/D47097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas updated this revision to Diff 148588.
gramanas added a comment.
I added a test that illustrates what @vsk is talking about.
Sorry for not providing the relevant information but I thought this would
be a simple one liner like https://reviews.llvm.org/rL327800. I will update the
revision's
description to include everything I know about this.
Repository:
rC Clang
https://reviews.llvm.org/D47097
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGen/debug-info-preserve-scope.c
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+// RUN: %clang_cc1 -O1 -debug-info-kind=limited -emit-llvm -mllvm \
+// RUN: -opt-bisect-limit=2 -o - %s 2> /dev/null | FileCheck %s \
+// RUN: --check-prefix PHI
+
+extern int map[];
+// PHI-LABEL: define void @test1
+void test1(int a, int n) {
+ for (int i = 0; i < n; ++i)
+a = map[a];
+}
+
+// PHI: for.cond:
+// PHI-NEXT: {{.*}} = phi i32 {{.*}} !dbg ![[test1DbgLoc:[0-9]+]]
+
+// PHI: ![[test1DbgLoc]] = !DILocation(line: 0
+
+
+static int i;
+// CHECK-LABEL: define void @test2
+void test2(int b) {
+ i = b;
+}
+
+// CHECK: store i32 {{.*}} !dbg ![[test2DbgLoc:[0-9]+]]
+
+// CHECK: ![[test2DbgLoc]] = !DILocation(line: 0
+
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+// RUN: %clang_cc1 -O1 -debug-info-kind=limited -emit-llvm -mllvm \
+// RUN: -opt-bisect-limit=2 -o - %s 2> /dev/null | FileCheck %s \
+// RUN: --check-prefix PHI
+
+extern int map[];
+// PHI-LABEL: define void @test1
+void test1(int a, int n) {
+ for (int i = 0; i < n; ++i)
+a = map[a];
+}
+
+// PHI: for.cond:
+// PHI-NEXT: {{.*}} = phi i32 {{.*}} !dbg ![[test1DbgLoc:[0-9]+]]
+
+// PHI: ![[test1DbgLoc]] = !DILocation(line: 0
+
+
+static int i;
+// CHECK-LABEL: define void @test2
+void test2(int b) {
+ i = b;
+}
+
+// CHECK: store i32 {{.*}} !dbg ![[test2DbgLoc:[0-9]+]]
+
+// CHECK: ![[test2DbgLoc]] = !DILocation(line: 0
+
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added a comment.
In https://reviews.llvm.org/D47097#248, @aprantl wrote:
> In https://reviews.llvm.org/D47097#223, @gramanas wrote:
>
> > In https://reviews.llvm.org/D47097#149, @probinson wrote:
> >
> > > Can we step back a second and better explain what the problem is? With
> > > current Clang the debug info for this function looks okay to me.
> > > The store that is "missing" a debug location is homing the formal
> > > parameter to its local stack location; this is part of prolog setup, not
> > > "real" code.
> >
>
The problem @gramanas is trying to address appears after SROA. SROA invokes
mem2reg, which tries to assign scope information to the phis it creates (see
https://reviews.llvm.org/D45397). Subsequent passes which touch these phis can
use these debug locations. This makes it easier for the debugger to find the
right scope when handling a machine exception.
Store instructions in the prolog without scope information cause mem2reg to
create phis without scope info. E.g:
// foo.c
extern int map[];
void f(int a, int n) {
for (int i = 0; i < n; ++i)
a = map[a];
}
$ clang foo.c -S -emit-llvm -o - -g -O1 -mllvm -opt-bisect-limit=2
..
%a.addr.0 = phi i32 [ %a, %entry ], [ %0, %for.body ]
(Side note: @gramanas, it would help to have the full context/motivation for
changes in the patch summary.)
>> Isn't this the reason the artificial debug loc exists? The store in the
>> prolog might not be real code but it should still have the scope that the
>> rest of the function has.
>
> Instructions that are part of the function prologue are supposed to have no
> debug location, not an artificial one. The funciton prologue ends at the
> first instruction with a nonempty location.
I've been reading through PEI but I'm having a hard time verifying this. How
does llvm determine where the function prologue ends when there isn't any debug
info? What would go wrong if llvm started to behave as if the prologue ended
sooner than it should? Also, why isn't this a problem for the swift compiler,
which appears to always assign debug locations to each instruction?
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
aprantl added a comment. In https://reviews.llvm.org/D47097#223, @gramanas wrote: > In https://reviews.llvm.org/D47097#149, @probinson wrote: > > > Can we step back a second and better explain what the problem is? With > > current Clang the debug info for this function looks okay to me. > > The store that is "missing" a debug location is homing the formal > > parameter to its local stack location; this is part of prolog setup, not > > "real" code. > > > Isn't this the reason the artificial debug loc exists? The store in the > prolog might not be real code but it should still have the scope that the > rest of the function has. Instructions that are part of the function prologue are supposed to have no debug location, not an artificial one. The funciton prologue ends at the first instruction with a nonempty location. Repository: rC Clang https://reviews.llvm.org/D47097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas added a comment. In https://reviews.llvm.org/D47097#149, @probinson wrote: > Can we step back a second and better explain what the problem is? With > current Clang the debug info for this function looks okay to me. > The store that is "missing" a debug location is homing the formal parameter > to its local stack location; this is part of prolog setup, not "real" code. Isn't this the reason the artificial debug loc exists? The store in the prolog might not be real code but it should still have the scope that the rest of the function has. Repository: rC Clang https://reviews.llvm.org/D47097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
probinson added a comment. Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me. The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setup, not "real" code. Repository: rC Clang https://reviews.llvm.org/D47097 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas updated this revision to Diff 148414.
gramanas added a comment.
Adress review comments.
Limit changes to the storeInst
Repository:
rC Clang
https://reviews.llvm.org/D47097
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGen/debug-info-preserve-scope.c
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+
+// CHECK: ![[artificialDbgLoc]] = !DILocation(line: 0
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+
+// CHECK: ![[artificialDbgLoc]] = !DILocation(line: 0
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
aprantl added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
vsk wrote:
> aprantl wrote:
> > vsk wrote:
> > > aprantl wrote:
> > > > vsk wrote:
> > > > > I think we need to be a bit more careful here. The current debug
> > > > > location stored in the builder may not be an artificial 0-location.
> > > > > This may cause non-linear single-stepping behavior. Consider this
> > > > > example:
> > > > >
> > > > > ```
> > > > > void foo() {
> > > > > bar();
> > > > > if (...) {
> > > > > int var = ...; //< Clang emits an alloca for "var".
> > > > > }
> > > > > ...
> > > > > ```
> > > > >
> > > > > The current debug location at the line "int var = ..." would be at
> > > > > line 4. But the alloca is emitted in the entry block of the function.
> > > > > In the debugger, this may result in strange single-stepping behavior
> > > > > when stepping into foo(). You could step to line 4, then line 2, then
> > > > > line 3, then line 4 again.
> > > > >
> > > > > I think we can avoid that by setting an artificial location on
> > > > > allocas.
> > > > > I think we can avoid that by setting an artificial location on
> > > > > allocas.
> > > > An alloca doesn't really generate any code (or rather.. the code it
> > > > generates is in the function prologue). In what situation would the
> > > > debug location on an alloca influence stepping? Are you thinking about
> > > > the alloca() function?
> > > An alloca instruction can lower to a subtraction (off the stack pointer)
> > > though: https://godbolt.org/g/U4nGzJ.
> > >
> > > `dwarfdump` shows that this subtraction instruction is actually assigned
> > > a location -- it currently happens to be the first location in the body
> > > of the function. I thought that assigning an artificial location to the
> > > alloca would be a first step towards fixing this.
> > >
> > > Also, using an artificial location could mitigate possible bad
> > > interactions between code motion passes and IRBuilder. If, say, we were
> > > to set the insertion point right after an alloca, we might infer some
> > > arbitrary debug location. So long as this inference happens, it seems
> > > safer to infer an artificial location.
> > >
> > >
> > This may have unintended side-effects: By assigning a debug location to an
> > alloca you are moving the end of the function prolog to before the alloca
> > instructions, since LLVM computes the end of the function prologue as the
> > first instruction with a non-empty debug location. Moving the end of the
> > function prologue to before that stack pointer is adjusted is wrong, since
> > that's the whole point of the prologue_end marker.
> >
> > To me it looks more like a bug in a much later stage. With the exception of
> > shrink-wrapped code, the prologue_end should always be after the stack
> > pointer adjustment, I think.
> Thanks for explaining, I didn't realize that's how the end of the function
> prologue is computed! Should we leave out the any debug location changes for
> allocas in this patch, then?
I think that would be better. You might want to double-check
PrologueEpilogueInserter and how the FrameSetup attribute is attached to
MachineInstrs in case my knowledge is out-of-date.
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
aprantl wrote:
> vsk wrote:
> > aprantl wrote:
> > > vsk wrote:
> > > > I think we need to be a bit more careful here. The current debug
> > > > location stored in the builder may not be an artificial 0-location.
> > > > This may cause non-linear single-stepping behavior. Consider this
> > > > example:
> > > >
> > > > ```
> > > > void foo() {
> > > > bar();
> > > > if (...) {
> > > > int var = ...; //< Clang emits an alloca for "var".
> > > > }
> > > > ...
> > > > ```
> > > >
> > > > The current debug location at the line "int var = ..." would be at line
> > > > 4. But the alloca is emitted in the entry block of the function. In the
> > > > debugger, this may result in strange single-stepping behavior when
> > > > stepping into foo(). You could step to line 4, then line 2, then line
> > > > 3, then line 4 again.
> > > >
> > > > I think we can avoid that by setting an artificial location on allocas.
> > > > I think we can avoid that by setting an artificial location on allocas.
> > > An alloca doesn't really generate any code (or rather.. the code it
> > > generates is in the function prologue). In what situation would the debug
> > > location on an alloca influence stepping? Are you thinking about the
> > > alloca() function?
> > An alloca instruction can lower to a subtraction (off the stack pointer)
> > though: https://godbolt.org/g/U4nGzJ.
> >
> > `dwarfdump` shows that this subtraction instruction is actually assigned a
> > location -- it currently happens to be the first location in the body of
> > the function. I thought that assigning an artificial location to the alloca
> > would be a first step towards fixing this.
> >
> > Also, using an artificial location could mitigate possible bad interactions
> > between code motion passes and IRBuilder. If, say, we were to set the
> > insertion point right after an alloca, we might infer some arbitrary debug
> > location. So long as this inference happens, it seems safer to infer an
> > artificial location.
> >
> >
> This may have unintended side-effects: By assigning a debug location to an
> alloca you are moving the end of the function prolog to before the alloca
> instructions, since LLVM computes the end of the function prologue as the
> first instruction with a non-empty debug location. Moving the end of the
> function prologue to before that stack pointer is adjusted is wrong, since
> that's the whole point of the prologue_end marker.
>
> To me it looks more like a bug in a much later stage. With the exception of
> shrink-wrapped code, the prologue_end should always be after the stack
> pointer adjustment, I think.
Thanks for explaining, I didn't realize that's how the end of the function
prologue is computed! Should we leave out the any debug location changes for
allocas in this patch, then?
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
aprantl added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
vsk wrote:
> aprantl wrote:
> > vsk wrote:
> > > I think we need to be a bit more careful here. The current debug location
> > > stored in the builder may not be an artificial 0-location. This may cause
> > > non-linear single-stepping behavior. Consider this example:
> > >
> > > ```
> > > void foo() {
> > > bar();
> > > if (...) {
> > > int var = ...; //< Clang emits an alloca for "var".
> > > }
> > > ...
> > > ```
> > >
> > > The current debug location at the line "int var = ..." would be at line
> > > 4. But the alloca is emitted in the entry block of the function. In the
> > > debugger, this may result in strange single-stepping behavior when
> > > stepping into foo(). You could step to line 4, then line 2, then line 3,
> > > then line 4 again.
> > >
> > > I think we can avoid that by setting an artificial location on allocas.
> > > I think we can avoid that by setting an artificial location on allocas.
> > An alloca doesn't really generate any code (or rather.. the code it
> > generates is in the function prologue). In what situation would the debug
> > location on an alloca influence stepping? Are you thinking about the
> > alloca() function?
> An alloca instruction can lower to a subtraction (off the stack pointer)
> though: https://godbolt.org/g/U4nGzJ.
>
> `dwarfdump` shows that this subtraction instruction is actually assigned a
> location -- it currently happens to be the first location in the body of the
> function. I thought that assigning an artificial location to the alloca would
> be a first step towards fixing this.
>
> Also, using an artificial location could mitigate possible bad interactions
> between code motion passes and IRBuilder. If, say, we were to set the
> insertion point right after an alloca, we might infer some arbitrary debug
> location. So long as this inference happens, it seems safer to infer an
> artificial location.
>
>
This may have unintended side-effects: By assigning a debug location to an
alloca you are moving the end of the function prolog to before the alloca
instructions, since LLVM computes the end of the function prologue as the first
instruction with a non-empty debug location. Moving the end of the function
prologue to before that stack pointer is adjusted is wrong, since that's the
whole point of the prologue_end marker.
To me it looks more like a bug in a much later stage. With the exception of
shrink-wrapped code, the prologue_end should always be after the stack pointer
adjustment, I think.
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
aprantl wrote:
> vsk wrote:
> > I think we need to be a bit more careful here. The current debug location
> > stored in the builder may not be an artificial 0-location. This may cause
> > non-linear single-stepping behavior. Consider this example:
> >
> > ```
> > void foo() {
> > bar();
> > if (...) {
> > int var = ...; //< Clang emits an alloca for "var".
> > }
> > ...
> > ```
> >
> > The current debug location at the line "int var = ..." would be at line 4.
> > But the alloca is emitted in the entry block of the function. In the
> > debugger, this may result in strange single-stepping behavior when stepping
> > into foo(). You could step to line 4, then line 2, then line 3, then line 4
> > again.
> >
> > I think we can avoid that by setting an artificial location on allocas.
> > I think we can avoid that by setting an artificial location on allocas.
> An alloca doesn't really generate any code (or rather.. the code it generates
> is in the function prologue). In what situation would the debug location on
> an alloca influence stepping? Are you thinking about the alloca() function?
An alloca instruction can lower to a subtraction (off the stack pointer)
though: https://godbolt.org/g/U4nGzJ.
`dwarfdump` shows that this subtraction instruction is actually assigned a
location -- it currently happens to be the first location in the body of the
function. I thought that assigning an artificial location to the alloca would
be a first step towards fixing this.
Also, using an artificial location could mitigate possible bad interactions
between code motion passes and IRBuilder. If, say, we were to set the insertion
point right after an alloca, we might infer some arbitrary debug location. So
long as this inference happens, it seems safer to infer an artificial location.
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
aprantl added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
vsk wrote:
> I think we need to be a bit more careful here. The current debug location
> stored in the builder may not be an artificial 0-location. This may cause
> non-linear single-stepping behavior. Consider this example:
>
> ```
> void foo() {
> bar();
> if (...) {
> int var = ...; //< Clang emits an alloca for "var".
> }
> ...
> ```
>
> The current debug location at the line "int var = ..." would be at line 4.
> But the alloca is emitted in the entry block of the function. In the
> debugger, this may result in strange single-stepping behavior when stepping
> into foo(). You could step to line 4, then line 2, then line 3, then line 4
> again.
>
> I think we can avoid that by setting an artificial location on allocas.
> I think we can avoid that by setting an artificial location on allocas.
An alloca doesn't really generate any code (or rather.. the code it generates
is in the function prologue). In what situation would the debug location on an
alloca influence stepping? Are you thinking about the alloca() function?
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: lib/CodeGen/CGExpr.cpp:72
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
I think we need to be a bit more careful here. The current debug location
stored in the builder may not be an artificial 0-location. This may cause
non-linear single-stepping behavior. Consider this example:
```
void foo() {
bar();
if (...) {
int var = ...; //< Clang emits an alloca for "var".
}
...
```
The current debug location at the line "int var = ..." would be at line 4. But
the alloca is emitted in the entry block of the function. In the debugger, this
may result in strange single-stepping behavior when stepping into foo(). You
could step to line 4, then line 2, then line 3, then line 4 again.
I think we can avoid that by setting an artificial location on allocas.
Comment at: lib/CodeGen/CGExpr.cpp:105
return new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(),
ArraySize, Name, AllocaInsertPt);
}
Why not apply the location here to cover more cases?
Comment at: test/CodeGen/debug-info-preserve-scope.c:11
+
+// CHECK: [[B:%.*]] = alloca i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc]]
Why is "B" captured?
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas updated this revision to Diff 148200.
gramanas marked an inline comment as done.
gramanas added a comment.
Remove redundant `this`
Repository:
rC Clang
https://reviews.llvm.org/D47097
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExpr.cpp
test/CodeGen/debug-info-preserve-scope.c
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+
+// CHECK: [[B:%.*]] = alloca i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc]]
+
+// CHECK: ![[artificialDbgLoc]] = !DILocation(line: 0
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -68,6 +68,8 @@
bool CastToDefaultAddrSpace) {
auto Alloca = CreateTempAlloca(Ty, Name, ArraySize);
Alloca->setAlignment(Align.getQuantity());
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
*AllocaAddr = Address(Alloca, Align);
llvm::Value *V = Alloca;
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
@@ -2071,7 +2074,7 @@
// Store the initial value into the alloca.
if (DoStore)
-EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
setAddrOfLocalVar(, DeclPtr);
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+
+// CHECK: [[B:%.*]] = alloca i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc]]
+
+// CHECK: ![[artificialDbgLoc]] = !DILocation(line: 0
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -68,6 +68,8 @@
bool CastToDefaultAddrSpace) {
auto Alloca = CreateTempAlloca(Ty, Name, ArraySize);
Alloca->setAlignment(Align.getQuantity());
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(Builder.getCurrentDebugLocation());
if (AllocaAddr)
*AllocaAddr = Address(Alloca, Align);
llvm::Value *V = Alloca;
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
@@ -2071,7 +2074,7 @@
// Store the initial value into the alloca.
if (DoStore)
-EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
setAddrOfLocalVar(, DeclPtr);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas updated this revision to Diff 148198.
gramanas added a comment.
Set debug location to the entry block alloca
Repository:
rC Clang
https://reviews.llvm.org/D47097
Files:
lib/CodeGen/CGDecl.cpp
lib/CodeGen/CGExpr.cpp
test/CodeGen/debug-info-preserve-scope.c
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+
+// CHECK: [[B:%.*]] = alloca i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc]]
+
+// CHECK: ![[artificialDbgLoc]] = !DILocation(line: 0
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -68,6 +68,8 @@
bool CastToDefaultAddrSpace) {
auto Alloca = CreateTempAlloca(Ty, Name, ArraySize);
Alloca->setAlignment(Align.getQuantity());
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(this->Builder.getCurrentDebugLocation());
if (AllocaAddr)
*AllocaAddr = Address(Alloca, Align);
llvm::Value *V = Alloca;
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
@@ -2071,7 +2074,7 @@
// Store the initial value into the alloca.
if (DoStore)
-EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
setAddrOfLocalVar(, DeclPtr);
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+
+// CHECK: [[B:%.*]] = alloca i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc]]
+
+// CHECK: ![[artificialDbgLoc]] = !DILocation(line: 0
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -68,6 +68,8 @@
bool CastToDefaultAddrSpace) {
auto Alloca = CreateTempAlloca(Ty, Name, ArraySize);
Alloca->setAlignment(Align.getQuantity());
+ // Set debug location in order to preserve the scope
+ Alloca->setDebugLoc(this->Builder.getCurrentDebugLocation());
if (AllocaAddr)
*AllocaAddr = Address(Alloca, Align);
llvm::Value *V = Alloca;
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
@@ -2071,7 +2074,7 @@
// Store the initial value into the alloca.
if (DoStore)
-EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
setAddrOfLocalVar(, DeclPtr);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: test/CodeGen/debug-info-preserve-scope.c:10
+
+// CHECK: alloca i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
In these two check lines, you're capturing the variable dbgLocForStore twice.
That means that if the !dbg location on the alloca were different from the
location on the store, this test would still pass.
To fix that, just capture the dbgLocForStore variable once, the first time you
see it on the alloca. In the second check line, you can simply refer to the
captured variable with `[[dbgLocForStore]]`.
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas updated this revision to Diff 148034.
gramanas added a comment.
Move ApplyDebugLocation before CreateMemTemp
Repository:
rC Clang
https://reviews.llvm.org/D47097
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGen/debug-info-preserve-scope.c
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+// CHECK: alloca i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
+
+// CHECK: ![[dbgLocForStore]] = !DILocation(line: 0
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
@@ -2071,7 +2074,7 @@
// Store the initial value into the alloca.
if (DoStore)
-EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
setAddrOfLocalVar(, DeclPtr);
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+// CHECK: alloca i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
+// CHECK: store i32 {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
+
+// CHECK: ![[dbgLocForStore]] = !DILocation(line: 0
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1946,6 +1946,9 @@
}
}
+ // Set artificial debug location in order to preserve the scope
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
Address DeclPtr = Address::invalid();
bool DoStore = false;
bool IsScalar = hasScalarEvaluationKind(Ty);
@@ -2071,7 +2074,7 @@
// Store the initial value into the alloca.
if (DoStore)
-EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
setAddrOfLocalVar(, DeclPtr);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
vsk added inline comments.
Comment at: lib/CodeGen/CGDecl.cpp:2074
+ if (DoStore) {
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
Ideally this would precede the calls to CreateMemTemp which set up the allocas.
Comment at: test/CodeGen/debug-info-preserve-scope.c:10
+
+// CHECK: store i32 %b, i32* %b.addr, align 4, !dbg ![[dbgLocForStore:[0-9]+]]
+
Can you check that the alloca gets the same location as well? You can do this
with:
```
CHECK: alloca {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
CHECK: store i32 {{.*}} !dbg ![[dbgLocForStore]]
```
Repository:
rC Clang
https://reviews.llvm.org/D47097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst
gramanas updated this revision to Diff 147876.
gramanas added a comment.
Update diff
Repository:
rC Clang
https://reviews.llvm.org/D47097
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGen/debug-info-preserve-scope.c
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+// CHECK: store i32 %b, i32* %b.addr, align 4, !dbg ![[dbgLocForStore:[0-9]+]]
+
+// CHECK: ![[dbgLocForStore]] = !DILocation(line: 0
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -2070,8 +2070,10 @@
}
// Store the initial value into the alloca.
- if (DoStore)
-EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ if (DoStore) {
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ }
setAddrOfLocalVar(, DeclPtr);
Index: test/CodeGen/debug-info-preserve-scope.c
===
--- /dev/null
+++ test/CodeGen/debug-info-preserve-scope.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+ a = b;
+}
+
+// CHECK: store i32 %b, i32* %b.addr, align 4, !dbg ![[dbgLocForStore:[0-9]+]]
+
+// CHECK: ![[dbgLocForStore]] = !DILocation(line: 0
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -2070,8 +2070,10 @@
}
// Store the initial value into the alloca.
- if (DoStore)
-EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ if (DoStore) {
+ auto DL = ApplyDebugLocation::CreateArtificial(*this);
+ EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
+ }
setAddrOfLocalVar(, DeclPtr);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
