[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 246162.
kwk added a comment.

- Clear formatting
- Make list private again
- Remove open from CommandObjectBreakpoint.cpp
- Remove change in unrelated file


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136

Files:
  lldb/include/lldb/Core/SearchFilter.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointResolverName.cpp
  lldb/source/Core/SearchFilter.cpp
  lldb/source/Target/Target.cpp
  lldb/test/Shell/Breakpoint/Inputs/search-support-files.cpp
  lldb/test/Shell/Breakpoint/Inputs/search-support-files.h
  lldb/test/Shell/Breakpoint/Inputs/search-support-files2.h
  lldb/test/Shell/Breakpoint/search-support-files.test

Index: lldb/test/Shell/Breakpoint/search-support-files.test
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/search-support-files.test
@@ -0,0 +1,51 @@
+# In these tests we will set breakpoints on a function by name. That function
+# is defined in a header file (search-support-files.h) and will therefore be
+# inlined into the file that includes it (search-support-files.cpp).
+#
+# TODO(kwk): Check that we can also do the same with C++ methods in files?
+#(See https://lldb.llvm.org/use/tutorial.html and look for --method.)
+
+# RUN: mkdir -p %t
+# RUN: cd %t
+# RUN: %build %p/Inputs/search-support-files.cpp -o dummy.out
+# RUN: %lldb -b -s %s dummy.out | FileCheck --color --dump-input=fail %s 
+
+#---
+# Set breakpoint by function name.
+
+breakpoint set -n inlined_42
+# CHECK: (lldb) breakpoint set -n inlined_42
+# CHECK-NEXT: Breakpoint 1: where = dummy.out`inlined_42() + 4 at search-support-files.h:1:20, address = 0x0{{.*}}
+
+breakpoint set -n main
+# CHECK: (lldb) breakpoint set -n main
+# CHECK-NEXT: Breakpoint 2: where = dummy.out`main + 22 at search-support-files.cpp:5:11, address = 0x0{{.*}}
+
+#---
+# Set breakpoint by function name and filename (the one in which the function is
+# inlined, aka the compilation unit).
+
+breakpoint set -n inlined_42 -f search-support-files.cpp
+# CHECK: (lldb) breakpoint set -n inlined_42 -f search-support-files.cpp
+# CHECK-NEXT: Breakpoint 3: no locations (pending).
+
+#---
+# Set breakpoint by function name and source filename (the file in which the
+# function is defined).
+#
+# NOTE: This test is the really interesting one as it shows that we can now
+#   search by source files that are themselves no compulation units.
+
+breakpoint set -n inlined_42 -f search-support-files.h
+# CHECK: (lldb) breakpoint set -n inlined_42 -f search-support-files.h
+# CHECK-NEXT: Breakpoint 4: where = dummy.out`inlined_42() + 4 at search-support-files.h:1:20, address = 0x0{{.*}}
+
+#---
+# Set breakpoint by function name and source filename. This time the file
+# doesn't exist to prove that we haven't widen the search space too much. When
+# we search for a function in file that doesn't exist, we should get no results.
+
+breakpoint set -n inlined_42 -f file-not-existing.h
+# CHECK: (lldb) breakpoint set -n inlined_42 -f file-not-existing.h
+# CHECK-NEXT: Breakpoint 5: no locations (pending).
+# CHECK-NEXT: WARNING: Unable to resolve breakpoint to any actual locations.
Index: lldb/test/Shell/Breakpoint/Inputs/search-support-files2.h
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/search-support-files2.h
@@ -0,0 +1 @@
+int return_zero() { return 0; }
\ No newline at end of file
Index: lldb/test/Shell/Breakpoint/Inputs/search-support-files.h
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/search-support-files.h
@@ -0,0 +1 @@
+int inlined_42() { return 42; }
Index: lldb/test/Shell/Breakpoint/Inputs/search-support-files.cpp
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/search-support-files.cpp
@@ -0,0 +1,7 @@
+#include "search-support-files.h"
+#include "search-support-files2.h"
+
+int main(int argc, char *argv[]) {
+  int a = inlined_42();
+  return return_zero() + a;
+}
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -324,7 +324,7 @@
   LazyBool check_inlines,
   LazyBool skip_prologue, bool internal,
   bool hardware,
-  LazyBool move_to_nearest_code) {
+ 

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 246160.
kwk added a comment.

Updated tests and code to remove the --search-source-files flag and make it the 
default behavior to also search source files.

TODO: rename class SearchFilterByModuleListAndCU to something more meaningful 
when an agreement on the behavior was made.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136

Files:
  lldb/include/lldb/Core/SearchFilter.h
  lldb/source/Breakpoint/Breakpoint.cpp
  lldb/source/Breakpoint/BreakpointResolverName.cpp
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  lldb/source/Core/SearchFilter.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  lldb/source/Target/Target.cpp
  lldb/test/Shell/Breakpoint/Inputs/search-support-files.cpp
  lldb/test/Shell/Breakpoint/Inputs/search-support-files.h
  lldb/test/Shell/Breakpoint/Inputs/search-support-files2.h
  lldb/test/Shell/Breakpoint/search-support-files.test

Index: lldb/test/Shell/Breakpoint/search-support-files.test
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/search-support-files.test
@@ -0,0 +1,51 @@
+# In these tests we will set breakpoints on a function by name. That function
+# is defined in a header file (search-support-files.h) and will therefore be
+# inlined into the file that includes it (search-support-files.cpp).
+#
+# TODO(kwk): Check that we can also do the same with C++ methods in files?
+#(See https://lldb.llvm.org/use/tutorial.html and look for --method.)
+
+# RUN: mkdir -p %t
+# RUN: cd %t
+# RUN: %build %p/Inputs/search-support-files.cpp -o dummy.out
+# RUN: %lldb -b -s %s dummy.out | FileCheck --color --dump-input=fail %s 
+
+#---
+# Set breakpoint by function name.
+
+breakpoint set -n inlined_42
+# CHECK: (lldb) breakpoint set -n inlined_42
+# CHECK-NEXT: Breakpoint 1: where = dummy.out`inlined_42() + 4 at search-support-files.h:1:20, address = 0x0{{.*}}
+
+breakpoint set -n main
+# CHECK: (lldb) breakpoint set -n main
+# CHECK-NEXT: Breakpoint 2: where = dummy.out`main + 22 at search-support-files.cpp:5:11, address = 0x0{{.*}}
+
+#---
+# Set breakpoint by function name and filename (the one in which the function is
+# inlined, aka the compilation unit).
+
+breakpoint set -n inlined_42 -f search-support-files.cpp
+# CHECK: (lldb) breakpoint set -n inlined_42 -f search-support-files.cpp
+# CHECK-NEXT: Breakpoint 3: no locations (pending).
+
+#---
+# Set breakpoint by function name and source filename (the file in which the
+# function is defined).
+#
+# NOTE: This test is the really interesting one as it shows that we can now
+#   search by source files that are themselves no compulation units.
+
+breakpoint set -n inlined_42 -f search-support-files.h
+# CHECK: (lldb) breakpoint set -n inlined_42 -f search-support-files.h
+# CHECK-NEXT: Breakpoint 4: where = dummy.out`inlined_42() + 4 at search-support-files.h:1:20, address = 0x0{{.*}}
+
+#---
+# Set breakpoint by function name and source filename. This time the file
+# doesn't exist to prove that we haven't widen the search space too much. When
+# we search for a function in file that doesn't exist, we should get no results.
+
+breakpoint set -n inlined_42 -f file-not-existing.h
+# CHECK: (lldb) breakpoint set -n inlined_42 -f file-not-existing.h
+# CHECK-NEXT: Breakpoint 5: no locations (pending).
+# CHECK-NEXT: WARNING: Unable to resolve breakpoint to any actual locations.
Index: lldb/test/Shell/Breakpoint/Inputs/search-support-files2.h
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/search-support-files2.h
@@ -0,0 +1 @@
+int return_zero() { return 0; }
\ No newline at end of file
Index: lldb/test/Shell/Breakpoint/Inputs/search-support-files.h
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/search-support-files.h
@@ -0,0 +1 @@
+int inlined_42() { return 42; }
Index: lldb/test/Shell/Breakpoint/Inputs/search-support-files.cpp
===
--- /dev/null
+++ lldb/test/Shell/Breakpoint/Inputs/search-support-files.cpp
@@ -0,0 +1,7 @@
+#include "search-support-files.h"
+#include "search-support-files2.h"
+
+int main(int argc, char *argv[]) {
+  int a = inlined_42();
+  return return_zero() + a;
+}
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -324,7 +324,7 @@
   

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D74136#1871751 , @labath wrote:

> In D74136#1870029 , @jingham wrote:
>
> > In D74136#1869622 , @kwk wrote:
> >
> > > @labath @jingham to summarize from what I read here and what I chatted 
> > > about with @labath , the following  is a possible way to go for now, 
> > > right?
> > >
> > > 1. We're not going to introduce my flag.
> > > 2. You're both not perfectly happy with the way things are documented at 
> > > the moment and dislike some of the implementation as in in LLDB but 
> > > chaning it should not be part of this patch.
> > > 3. @jingham wouldn't want to introduce `--compile-unit` as a flag that 
> > > @labath proposed.
> > > 4. You want `breakpoint set --file` to search everything, that is to say 
> > > compilation units and files referenced in `DW_AT_decl_file`.
> > >
> > >   If you can confirm that this is correct, then I can refactor this patch 
> > > to remove the switch and change the default behavior for `breakpoint set 
> > > --file`. Especially point 4. is important I guess.
> >
> >
> > There's a point (5) which is that the search in 4 should be conditioned on 
> > the setting of the "target.inline-breakpoint-strategy".  That way if people 
> > have big projects but don't ever #include source files, and don't build 
> > with LTO, they can turn off these extra searches, which might end up being 
> > costly and to no purpose for them.
>
>
> I think we're on the same page, but I'll just try to rephrase this in my own 
> words.
>
> Basically, I'm hoping that we could get the `--file+--function` combo to 
> behave the same way `--file+--line`. I.e., that the --file argument should 
> specify the file that the function is defined in (i.e. the DW_AT_decl_file 
> attribute of the function), and not the compile unit (DW_AT_name of the 
> compile unit containing the function). The way the 
> `inline-breakpoint-strategy` setting comes into play is that if the setting 
> value is "headers" and the user specifies a `.cpp` file, then we will assume 
> that the .cpp file also matches the name of one of the compile units, and we 
> will not attempt to search compile units with different names (which is the 
> same thing we do for line breakpoints, I believe).


That is my understanding as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This way the only bit of the matrix of "restricting function name breakpoints 
by containing source file" that we don't support is "set a breakpoint on this 
function, but ONLY when it is inlined into another CU, NOT when it is in its 
defining CU."  That seems the least useful of the options, and I'm happy to 
trade off not adding another option to the already overstuffed "break set" list 
of options.  And remember, if you actually needed this it would be 
straightforward to write a scripted breakpoint resolver to do that job.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D74136#1870029 , @jingham wrote:

> In D74136#1869622 , @kwk wrote:
>
> > @labath @jingham to summarize from what I read here and what I chatted 
> > about with @labath , the following  is a possible way to go for now, right?
> >
> > 1. We're not going to introduce my flag.
> > 2. You're both not perfectly happy with the way things are documented at 
> > the moment and dislike some of the implementation as in in LLDB but chaning 
> > it should not be part of this patch.
> > 3. @jingham wouldn't want to introduce `--compile-unit` as a flag that 
> > @labath proposed.
> > 4. You want `breakpoint set --file` to search everything, that is to say 
> > compilation units and files referenced in `DW_AT_decl_file`.
> >
> >   If you can confirm that this is correct, then I can refactor this patch 
> > to remove the switch and change the default behavior for `breakpoint set 
> > --file`. Especially point 4. is important I guess.
>
>
> There's a point (5) which is that the search in 4 should be conditioned on 
> the setting of the "target.inline-breakpoint-strategy".  That way if people 
> have big projects but don't ever #include source files, and don't build with 
> LTO, they can turn off these extra searches, which might end up being costly 
> and to no purpose for them.


I think we're on the same page, but I'll just try to rephrase this in my own 
words.

Basically, I'm hoping that we could get the `--file+--function` combo to behave 
the same way `--file+--line`. I.e., that the --file argument should specify the 
file that the function is defined in (i.e. the DW_AT_decl_file attribute of the 
function), and not the compile unit (DW_AT_name of the compile unit containing 
the function). The way the `inline-breakpoint-strategy` setting comes into play 
is that if the setting value is "headers" and the user specifies a `.cpp` file, 
then we will assume that the .cpp file also matches the name of one of the 
compile units, and we will not attempt to search compile units with different 
names (which is the same thing we do for line breakpoints, I believe).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D74136#1869622 , @kwk wrote:

> @labath @jingham to summarize from what I read here and what I chatted about 
> with @labath , the following  is a possible way to go for now, right?
>
> 1. We're not going to introduce my flag.
> 2. You're both not perfectly happy with the way things are documented at the 
> moment and dislike some of the implementation as in in LLDB but chaning it 
> should not be part of this patch.
> 3. @jingham wouldn't want to introduce `--compile-unit` as a flag that 
> @labath proposed.
> 4. You want `breakpoint set --file` to search everything, that is to say 
> compilation units and files referenced in `DW_AT_decl_file`.
>
>   If you can confirm that this is correct, then I can refactor this patch to 
> remove the switch and change the default behavior for `breakpoint set 
> --file`. Especially point 4. is important I guess.


There's a point (5) which is that the search in 4 should be conditioned on the 
setting of the "target.inline-breakpoint-strategy".  That way if people have 
big projects but don't ever #include source files, and don't build with LTO, 
they can turn off these extra searches, which might end up being costly and to 
no purpose for them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

@labath @jingham to summarize from what I read here and what I chatted about 
with @labath , the following  is a possible way to go for now, right?

1. We're not going to introduce my flag.
2. You're both not perfectly happy with the way things are documented at the 
moment and dislike some of the implementation as in in LLDB but chaning it 
should not be part of this patch.
3. @jingham wouldn't want to introduce `--compile-unit` as a flag that @labath 
proposed.
4. You want `breakpoint set --file` to search everything, that is to say 
compilation units and files referenced in `DW_AT_decl_file`.

If you can confirm that this is correct, then I can refactor this patch to 
remove the switch and change the default behavior for `breakpoint set --file`. 
Especially point 4. is important I guess.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D74136#1862246 , @labath wrote:

> In D74136#1862200 , @jingham wrote:
>
> > Is there ever a reason other than performance why you would want NOT to 
> > consult both the Compile Unit name and also look for DW_AT_decl_file?  That 
> > doesn't seem clear to me.
> >
> > If the only worry is performance, and except for that you would really 
> > always want the full search, then I don't see what we gain by adding 
> > --compile-unit as distinct from --file.  We should use 
> > target.inline-breakpoint-strategy to allow people to turn on or off the 
> > searching for DW_AT_decl_file for folks whose projects are complicated 
> > enough AND who aren't doing a lot of inlining.  "break set" is already 
> > over-stuffed, I don't want to add to it unless we get a positive benefit.
>
>
> Well, conceptually these are two different things. --compile-unit would 
> restrict the search to the given compile unit no matter what is the file 
> where the function was defined in. And --file would consider the defining 
> file, and not care which compile unit hosts that. In theory, both can be 
> useful, but I'm not sure if people would really use that flexibility.
>
> My proposal was based on the fact that we want to preserve the ability to 
> filter according to the defining cu. If we don't need to do that, then simply 
> just redefining the meaning of the file+function combo is fine by me..


Yeah, I'm not convinced the flexibility is worth adding another slot, 
especially since now --file needs to explain how it is different from 
--compile-unit, but ONLY when --name is specified.  When --file is the primary 
(for source breakpoints) that won't be true.  And why is there a --compile-unit 
that DOESN'T set file & line breakpoints, etc...  The fact that --file has 
different meanings depending on what the primary specification is is one of the 
reasons why it's a shame that they all share one common pool of documentable 
options.

> 
> 
>> On a side note, I really wish I had made "break set" be a multi-word 
>> command, so you could specify the main search criteria, and then the 
>> qualifiers could be used more flexibly and be better documented and and you 
>> wouldn't see options that weren't relevant.  Like:
>> 
>> break set name
>>  break set source
>>  break set address
>> 
>> That wouldn't make the command any harder to type, it would even be easier 
>> since:
>> 
>> break set name foo
>> 
>> would shorten to:
>> 
>> br s n foo
>> 
>> instead of
>> 
>> br s -n foo
>> 
>> I wonder if it is too late to make this change?
> 
> That would be a bigger change than what I proposed because it would affect 
> every breakpoint command, instead of just the (rarely used, I think) 
> file+function combo, though I'm not necessarily against that. However, if we 
> redefine the meaning of the file+function combo, then I think the need for 
> this will be diminished because the option will become less context sensitive 
> and thus more predictable and more combinable.

Yes, I wasn't suggesting as blocking this change.  I just feel like at some 
point we're going to tip over into too much complexity as things are currently 
constituted, so someday we'll probably go crazy if we don't rework this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D74136#1862200 , @jingham wrote:

> Is there ever a reason other than performance why you would want NOT to 
> consult both the Compile Unit name and also look for DW_AT_decl_file?  That 
> doesn't seem clear to me.
>
> If the only worry is performance, and except for that you would really always 
> want the full search, then I don't see what we gain by adding --compile-unit 
> as distinct from --file.  We should use target.inline-breakpoint-strategy to 
> allow people to turn on or off the searching for DW_AT_decl_file for folks 
> whose projects are complicated enough AND who aren't doing a lot of inlining. 
>  "break set" is already over-stuffed, I don't want to add to it unless we get 
> a positive benefit.


Well, conceptually these are two different things. --compile-unit would 
restrict the search to the given compile unit no matter what is the file where 
the function was defined in. And --file would consider the defining file, and 
not care which compile unit hosts that. In theory, both can be useful, but I'm 
not sure if people would really use that flexibility.

My proposal was based on the fact that we want to preserve the ability to 
filter according to the defining cu. If we don't need to do that, then simply 
just redefining the meaning of the file+function combo is fine by me..

> On a side note, I really wish I had made "break set" be a multi-word command, 
> so you could specify the main search criteria, and then the qualifiers could 
> be used more flexibly and be better documented and and you wouldn't see 
> options that weren't relevant.  Like:
> 
> break set name
>  break set source
>  break set address
> 
> That wouldn't make the command any harder to type, it would even be easier 
> since:
> 
> break set name foo
> 
> would shorten to:
> 
> br s n foo
> 
> instead of
> 
> br s -n foo
> 
> I wonder if it is too late to make this change?

That would be a bigger change than what I proposed because it would affect 
every breakpoint command, instead of just the (rarely used, I think) 
file+function combo, though I'm not necessarily against that. However, if we 
redefine the meaning of the file+function combo, then I think the need for this 
will be diminished because the option will become less context sensitive and 
thus more predictable and more combinable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Is there ever a reason other than performance why you would want NOT to consult 
both the Compile Unit name and also look for DW_AT_decl_file is performance?  
That doesn't seem clear to me.

If the only worry is performance, and except for that you would really always 
want the full search, then I don't see what we gain by adding --compile-unit as 
distinct from --file.  We should use target.inline-breakpoint-strategy to allow 
people to turn on or off the searching for DW_AT_decl_file for folks whose 
projects are complicated enough AND who aren't doing a lot of inlining.  "break 
set" is already over-stuffed, I don't want to add to it unless we get a 
positive benefit.

On a side note, I really wish I had made "break set" be a multi-word command, 
so you could specify the main search criteria, and then the qualifiers could be 
used more flexibly and be better documented and and you wouldn't see options 
that weren't relevant.  Like:

break set name
break set source
break set address

That wouldn't make the command any harder to type, it would even be easier 
since:

break set name foo

would shorten to:

br s n foo

instead of

br s -n foo

I wonder if it is too late to make this change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think that having this behavior key off of a flag is really confusing. I 
think the most sensible behavior here would be to introduce a new option like 
`--compile-unit` and have that behave like the current --file+--function mode. 
This would free up the --file argument for the behavior that you want to 
implement here.

I.e.:

- --compile-unit+--function: search for the occurence of the given function in 
the given compile unit (ignoring DW_AT_decl_file)
- --file+--function: search for the given function defined in the given file 
(no matter which compile unit it comes from). This would be consistent with the 
--file+--line scenario. For maximal consistence this should also respect the 
`target.inline-breakpoint-strategy` setting and only do an exhaustive file 
search when the value of that setting is "always". If the value is "headers" 
then we should assume that the name of the .cpp file refers to a compile unit 
and only search the unit with that name.

The change in behavior is unfortunate, but I don't think this is something that 
a lot of people will depend on because the current behavior is quite confusing. 
And I think it's better to change it now instead of painting ourselves into an 
even smaller corner. To reduce surprises we can issue a warning about the 
behavior change when the user uses this flag combination. And making these 
things separate paves the way for introducing new combinations of flags 
(--compile-unit+--file+--name, or --compile-unit+--file+--line) with a 
reasonably predictable behavior...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk created this revision.
Herald added subscribers: lldb-commits, dexonsmith, aprantl, mehdi_amini, 
mgorny.
Herald added a reviewer: jdoerfert.
Herald added a project: LLDB.
kwk planned changes to this revision.
kwk added a comment.

I plan to refactor some of this patch.


To set a breakpoint on a function called `YOURFUNCTION` you do this:

  (lldb) breakpoint set -n YOURFUNCTION

The output then could look like this:

  Breakpoint 1: where = YOURTARGET`YOURFUNCTION() + 4 at YOURFILE:1:20, address 
= 0x01234

LLDB does a good job in finding `YOURFUNCTION` because, indeed,
`YOURFILE` contains the definition of `YOURFUNCTION`.

Let's say, you not only want to find `YOURFUNCTION` anywhere in
`YOURTARGET`. Say, you want to find `YOURFUNCTION` in the file that LLDB
found it in.

This is how you'd do that:

  (lldb) breakpoint set -n YOURFUNCTION -f YOURFILE

This is where things get a bit tricky. If `YOURFILE` points to a header
file in which `YOURFUNCTION` is not only declared but also defined, then
chances are that it might be inlined into `YOURTARGET` and LLDB cannot
find it. *Why?* You might ask and the answer is that LLDB uses only the
`DW_TAG_compile_unit`'s name that it finds under the `DW_AT_name`
attribute.

Suppose this is a snippet of `llvm-darfdump YOURTARGET`

  0x000b: DW_TAG_compile_unit
DW_AT_name ("main.c")
  
  0x002a: DW_TAG_subprogram
DW_AT_name ("YOURFUNCTION")
DW_AT_decl_file ("YOURFILE")

Then LLDB only looks at the file called `main.c` and ignores the
`DW_AT_decl_file`.

With this change I introduce a switch that teaches LLDB to optionally
also respect the `DW_AT_decl_file` all you have to do is pass an option
to the previously mentioned breakpoint command.

  (lldb) breakpoint set -n YOURFUNCTION -f YOURFILE
  --search-source-files

I'm happy to change the name of the flag from `--search-source-files` or
`-Q` to something else. Maybe `--respect-decl-file` or
`--follow-decl-file` are better names, but for now this is about the
working of this patch and not about names.

If you're not convinced that this change has any purpose, think about
LTO and how it might inline your functions with logic that is hard to
reason about when you just have the source code and a binary.

For reference, here's the definition for `DW_AT_decl_file` from the DWARF
spec in chapter 2.14:

> It is sometimes useful in a debugger to be able to associate a
>  declaration with its occurrence in the program source.
>  [...]
>  The value of the DW_AT_decl_file attribute corresponds to a file
>  number from the line number information table for the compilation unit
>  containing the debugging information entry and represents the source
>  file in which the declaration appeared (see Section 6.2 on page 148).
>  The value 0 indicates that no source file has been specified.



Reference to lldb-dev mailing list
==

I once brought this topic up on the lldb-dev mailing list
(http://lists.llvm.org/pipermail/lldb-dev/2019-November/015707.html) and
since then worked on it.

Performance considerations
--

That is where Jim Ingham wrote:

> You probably also want to take some care not to regress the
>  performance of the case where the function is defined in a CU, since
>  that's the more common usage.

Since I didn't change the default behavior in LLDB, I think the
performance is not in danger.

Documentation issue
---

Jim also brought up an idea of documenting and querying options of
commands:

> Another thing I've thought about doing is adding the ability to have
>  help include one of the non-optional, non-overlapping options to the
> command, so you could say:
> 
>   (lldb) help break set -n
> 
> and that would tell you that this is a "by function name" breakpoint,
>  and in that case -n means...  That might help reduce the information
>  overload, and give a better sense of what these complex commands do.

I haven't implemented this yet but I wanted to bubble this idea up here
so it receives more attention.

Thoughts on LTO
---

Jim's thoughts on LTO:

> Back to your original query...  If the function is defined in a .h
>  file, or gets inlined by LTO, this filtering is trickier, and I
>  didn't implement that behavior when I implemented this breakpoint
>  type.  So in that case, and in the case where LTO inlines a function,
>  the feature isn't implemented correctly.  The -n searche always looks
>  for out of line and inline instances when doing the search.  So we
>  already get the searcher to all the instances.
>  You would just have to widen the search beyond "Does the CU match" to
>  try to figure out where the inlined instance was defined.

That is essentially what I do in this change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74136

Files:
  lldb/include/lldb/Core/SearchFilter.h
  lldb/include/lldb/Target/Target.h
  

[Lldb-commits] [PATCH] D74136: [LLDB] WIP: Optionally follow DW_AT_decl_file when setting breakpoint

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk planned changes to this revision.
kwk added a comment.

I plan to refactor some of this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74136/new/

https://reviews.llvm.org/D74136



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits