[Lldb-commits] [PATCH] D97114: [lldb] [docs] Update platform support status

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, krytarowski, emaste.
Herald added subscribers: arphaman, arichardson.
mgorny requested review of this revision.

Update supported features on FreeBSD, and supported platform list
on FreeBSD, Linux and NetBSD.


https://reviews.llvm.org/D97114

Files:
  lldb/docs/index.rst
  lldb/docs/status/status.rst


Index: lldb/docs/status/status.rst
===
--- lldb/docs/status/status.rst
+++ lldb/docs/status/status.rst
@@ -43,26 +43,26 @@
 
 Features Matrix
 ---
-+---++-+---++--+
-| Feature   | FreeBSD| Linux   | macOS 
| NetBSD | Windows  |
-+===++=+===++==+
-| Backtracing   | YES| YES | YES   
| YES| YES  |
-+---++-+---++--+
-| Breakpoints   | YES| YES | YES   
| YES| YES  |
-+---++-+---++--+
-| C++11:| YES| YES | YES   
| YES| Unknown  |
-+---++-+---++--+
-| Commandline tool  | YES| YES | YES   
| YES| YES  |
-+---++-+---++--+
-| Core file debugging   | YES (ELF)  | YES (ELF)   | YES (MachO)   
| YES (ELF)  | YES (Minidump)   |
-+---++-+---++--+
-| Remote debugging  | NO | YES (lldb-server)   | YES 
(debugserver) | YES (lldb-server)  | NO   |
-+---++-+---++--+
-| Disassembly   | YES| YES | YES   
| YES| YES  |
-+---++-+---++--+
-| Expression evaluation | Unknown| YES (known issues)  | YES   
| YES (known issues) | YES (known issues)   |
-+---++-+---++--+
-| JIT debugging | Unknown| Symbolic debugging only | Untested  
| Work In Progress   | NO   |
-+---++-+---++--+
-| Objective-C 2.0:  | Unknown| N/A | YES   
| Unknown| N/A  |
-+---++-+---++--+
++---++-+---++--+
+| Feature   | FreeBSD| Linux   | macOS 
| NetBSD | Windows  |
++===++=+===++==+
+| Backtracing   | YES| YES | YES   
| YES| YES  |
++---++-+---++--+
+| Breakpoints   | YES| YES | YES   
| YES| YES  |
++---++-+---++--+
+| C++11:| YES| YES | YES   
| YES| Unknown  |
++---++-+---++--+
+| Commandline tool  | YES| YES | YES   
| YES| YES  |
++-

[Lldb-commits] [PATCH] D97114: [lldb] [docs] Update platform support status

[Ed Maste via Phabricator via lldb-commits]

emaste accepted this revision.
emaste added a comment.
This revision is now accepted and ready to land.

FreeBSD changes LGTM


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

https://reviews.llvm.org/D97114

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

[Lldb-commits] [PATCH] D97114: [lldb] [docs] Update platform support status

[Kamil Rytarowski via Phabricator via lldb-commits]

krytarowski accepted this revision.
krytarowski added a comment.

NetBSD OK!


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

https://reviews.llvm.org/D97114

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

[Lldb-commits] [PATCH] D96637: Make sure the interpreter module was loaded before making checks against it

[António Afonso via Phabricator via lldb-commits]

aadsm added inline comments.



Comment at: 
lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py:28
+def test_x(self):
+process = self.build_launch_and_attach()
+thread = process.GetSelectedThread()

clayborg wrote:
> Is this racy? What happens on a really slow system? Can we fail to attach? If 
> we do attach, are we guaranteed to be at a place where we can set 
> "flip_to_1_to_continue = 1"? The nice thing is it is a global variable that 
> we should be able to set no matter where we stop. 
> Is this racy?

I don't think so because we already have a pid at that point in time, so we 
should always be able to attach.

> If we do attach, are we guaranteed to be at a place where we can set 
> "flip_to_1_to_continue = 1"?

yeah, that's exactly why I made it global. I could also wait until there's a 
`flip_to_1_to_continue` in the scope if you think it's worthwhile.



Comment at: 
lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py:32-33
+# Continue so that dlopen is called.
+breakpoint = self.target().BreakpointCreateBySourceRegex(
+"// break after dlopen", lldb.SBFileSpec("main.c"))
+self.assertNotEqual(breakpoint.GetNumResolvedLocations(), 0)

clayborg wrote:
> Don't we need to break before the dlopen and make sure we don't have a 
> libfeature.so in our module list, then run over the dlopen and verify we do 
> see it afterwards? Wasn't this bug that we will see shared libraries 
> correctly one time when we attach, but just not get any updates after this??
that was a completely different bug and I have a different test for that 
situation as well.
Something that I could test though, is that before we got an update for an 
unresolved breakpoint to make sure we did indeed transitioned from unresolved 
-> resolved.

I'll add that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96637

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

[Lldb-commits] [PATCH] D96637: Make sure the interpreter module was loaded before making checks against it

[António Afonso via Phabricator via lldb-commits]

aadsm added inline comments.



Comment at: 
lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py:32-33
+# Continue so that dlopen is called.
+breakpoint = self.target().BreakpointCreateBySourceRegex(
+"// break after dlopen", lldb.SBFileSpec("main.c"))
+self.assertNotEqual(breakpoint.GetNumResolvedLocations(), 0)

aadsm wrote:
> clayborg wrote:
> > Don't we need to break before the dlopen and make sure we don't have a 
> > libfeature.so in our module list, then run over the dlopen and verify we do 
> > see it afterwards? Wasn't this bug that we will see shared libraries 
> > correctly one time when we attach, but just not get any updates after this??
> that was a completely different bug and I have a different test for that 
> situation as well.
> Something that I could test though, is that before we got an update for an 
> unresolved breakpoint to make sure we did indeed transitioned from unresolved 
> -> resolved.
> 
> I'll add that.
Forget this, I shouldn't be answering comments in the morning.

> that was a completely different bug and I have a different test for that 
> situation as well.
> Something that I could test though, is that before we got an update for an 
> unresolved breakpoint to make sure we did indeed transitioned from unresolved 
> -> resolved.
> 
> I'll add that.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96637

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

[Lldb-commits] [PATCH] D96637: Make sure the interpreter module was loaded before making checks against it

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 325221.
aadsm added a comment.

Checks the module is not loaded right after we attach


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96637

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  lldb/test/API/functionalities/module_load_attach/Makefile
  lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py
  lldb/test/API/functionalities/module_load_attach/feature.c
  lldb/test/API/functionalities/module_load_attach/main.c

Index: lldb/test/API/functionalities/module_load_attach/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/module_load_attach/main.c
@@ -0,0 +1,15 @@
+#include 
+#include 
+#include 
+
+volatile int flip_to_1_to_continue = 0;
+
+int main() {
+  lldb_enable_attach();
+  while (! flip_to_1_to_continue) // Wait for debugger to attach
+sleep(1);
+  // dlopen the feature
+  void *feature = dlopen("libfeature.so", RTLD_NOW);
+  assert(feature && "dlopen failed?");
+  return 0; // break after dlopen
+}
Index: lldb/test/API/functionalities/module_load_attach/feature.c
===
--- /dev/null
+++ lldb/test/API/functionalities/module_load_attach/feature.c
@@ -0,0 +1 @@
+extern void feature() {}
Index: lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py
===
--- /dev/null
+++ lldb/test/API/functionalities/module_load_attach/TestModuleLoadAttach.py
@@ -0,0 +1,49 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def build_launch_and_attach(self):
+self.build()
+# launch
+exe = self.getBuildArtifact("a.out")
+popen = self.spawnSubprocess(exe)
+# attach
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+listener = lldb.SBListener("my.attach.listener")
+error = lldb.SBError()
+process = target.AttachToProcessWithID(listener, popen.pid, error)
+self.assertTrue(error.Success() and process, PROCESS_IS_VALID)
+return process
+
+def assertModuleIsLoaded(self, module_name):
+feature_module = self.dbg.GetSelectedTarget().FindModule(lldb.SBFileSpec(module_name))
+self.assertTrue(feature_module.IsValid(), f"Module {module_name} should be loaded")
+
+def assertModuleIsNotLoaded(self, module_name):
+feature_module = self.dbg.GetSelectedTarget().FindModule(lldb.SBFileSpec(module_name))
+self.assertFalse(feature_module.IsValid(), f"Module {module_name} should not be loaded")
+
+@skipIfRemote
+@skipUnlessLinux
+@no_debug_info_test
+def test(self):
+'''
+This test makes sure that after attach lldb still gets notifications
+about new modules being loaded by the process
+'''
+process = self.build_launch_and_attach()
+thread = process.GetSelectedThread()
+self.assertModuleIsNotLoaded("libfeature.so")
+thread.GetSelectedFrame().EvaluateExpression("flip_to_1_to_continue = 1")
+# Continue so that dlopen is called.
+breakpoint = self.target().BreakpointCreateBySourceRegex(
+"// break after dlopen", lldb.SBFileSpec("main.c"))
+self.assertNotEqual(breakpoint.GetNumResolvedLocations(), 0)
+stopped_threads = lldbutil.continue_to_breakpoint(self.process(), breakpoint)
+self.assertModuleIsLoaded("libfeature.so")
Index: lldb/test/API/functionalities/module_load_attach/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/module_load_attach/Makefile
@@ -0,0 +1,10 @@
+C_SOURCES := main.c
+LD_EXTRAS := -Wl,-rpath "-Wl,$(shell pwd)"
+USE_LIBDL := 1
+
+feature:
+	$(MAKE) -f $(MAKEFILE_RULES) \
+		DYLIB_ONLY=YES DYLIB_NAME=feature DYLIB_C_SOURCES=feature.c
+all: feature
+
+include Makefile.rules
Index: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===
--- lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -441,6 +441,7 @@
   if (module_sp.get()) {
 if (module_sp->GetObjectFile()->GetBaseAddress().GetLoadAddress(
 &m_process->GetTarget()) == m_interpreter_base &&
+m_interpreter_module.lock() &&
 module_sp != m_interpreter_module.lock()) {
   // If this is a duplicate instance of ld.so, unload it.  We may end up
   // with i

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2573076 , @jankratochvil 
wrote:

> In D96778#2572816 , @dblaikie wrote:
>
>> rolls this into one file with two CUs - bit easier to deal with.
>
> Then one could not use the `.file` directives and one would need to code also 
> `.debug_line` by hand.

Ah, right, makes sense.

>> Could you provide the source code for this - I wouldn't mind trying it out 
>> and seeing what might be different/why DWZ doesn't understand this.
>
> dwz will merge it without -flto but not with -flto: 
> http://people.redhat.com/jkratoch/inlinevar.d.tar.xz

Ah, yeah, so I see - thanks for the repro!

If I had to guess, it'd be because with LTO clang emits the abstract origin 
lexically after the inlined instance of "var", whereas with non-LTO clang emits 
the abstract origin before the inlined instance.

(& yeah, clang should get this right with lto without the need for dwz - LLVM 
correctly deduplicates types when merging for LTO, but doesn't do so for 
already-inlined functions, could be done with the same sort of logic, though)

> Personally I am not interested in DWZ, I am implementing it only as a 
> compatibility with existing file format, IMNSHO DWZ should be dropped.

Not sure I follow. dwz does use the existing DWARF file format. you mean you're 
only interested in compatibility with the DWARF directly produced by compilers, 
not post-processed by dwz? Fair enough. I guess @werat has some interest in 
supporting dwz for their use cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D96778#2576881 , @dblaikie wrote:

> In D96778#2573076 , @jankratochvil 
> wrote:
>
>> Personally I am not interested in DWZ, I am implementing it only as a 
>> compatibility with existing file format, IMNSHO DWZ should be dropped.
>
> Not sure I follow. dwz does use the existing DWARF file format. you mean 
> you're only interested in compatibility with the DWARF directly produced by 
> compilers, not post-processed by dwz? Fair enough. I guess @werat has some 
> interest in supporting dwz for their use cases.

No, I need compatibility of LLDB with DWZ (or rather LLDB needs it if it should 
get used on Red Hat systems). DWZ is specified by DWARF-5 but there are various 
DWARF tools still not supporting DWZ. Also the only DWZ format currently in use 
is a non-standard GNU extension of DWARF-4. The DWZ format has been designed 
for GDB making it difficult to parse it by LLDB. LLDB does per-DIE DWARF->IR 
conversion compared to GDB doing per-CU DWARF->IR conversion, that means that 
LLDB needs to keep context of the DWZ parent DIE (as DWZ recursively imports 
DW_TAG_partial_unit) for each its reference of DIE in its various indices which 
is a PITA. One cannot parse a PU (partial unit) without knowing which CU did 
import it. `-fdebug-types-section` brings most of the size savings of DWZ but 
unfortunately not all of it. I tried to drop DWZ for Fedora in exchange of 
`-fdebug-types-section` but I failed, there is too strong push for DWZ. From 
the mail thread one can see they try hard to find any little benefits of DWZ 
ignoring any its disadvantages: Fedora change proposal 
 + its 
fedora-devel mail thread 

DWZ format could be fixed for better consumption by intelligent consumers like 
LLDB but then Red Hat wants compatibility with existing flawed DWZ format being 
already present in RHEL-7/8/9 anyway. That means I would have to implement two 
different DWZ file formats LLDB compatibility so I rather implemented only the 
only existing DWZ format (D96236  D96237 
 D96238  
D96239  D96240 
 D96241  
D96242  D96243 
) and hopefully Fedora+RHEL will rather drop 
DWZ in the future.
(Currently I am blocked by some clang assertion for testsuite running in DWZ 
mode with libc++ - the assertion does not happen with libc++ without DWZ mode 
nor with libstdc++ in DWZ mode.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2576913 , @jankratochvil 
wrote:

> In D96778#2576881 , @dblaikie wrote:
>
>> In D96778#2573076 , @jankratochvil 
>> wrote:
>>
>>> Personally I am not interested in DWZ, I am implementing it only as a 
>>> compatibility with existing file format, IMNSHO DWZ should be dropped.
>>
>> Not sure I follow. dwz does use the existing DWARF file format. you mean 
>> you're only interested in compatibility with the DWARF directly produced by 
>> compilers, not post-processed by dwz? Fair enough. I guess @werat has some 
>> interest in supporting dwz for their use cases.
>
> No, I need compatibility of LLDB with DWZ (or rather LLDB needs it if it 
> should get used on Red Hat systems).

Ah, OK. You want RH compatibility. RH uses DWZ. You'd rather they didn't, but 
working with it, given they still do use it.

> DWZ is specified by DWARF-5

What part of DWZ is specified by DWARFv5? My understanding is that DWZ uses 
existing standard features/didn't need anything in particular standardized in 
any DWARF version (perhaps the original creation of partial units was motivated 
by DWZ? But partial units have been around for a few versions - seems like it 
was introduced in DWARFv3)

> but there are various DWARF tools still not supporting DWZ.

DWARF is a very broad (as they like to say, "permissive") standard. Lots of 
valid DWARF won't work in lots of places, because the full spectrum of 
interesting ways one could use DWARF features is too broad to be uniformly 
implemented - generally consumers and produces implement enough to work with 
each other and that's about it.

(for instance I recently started dabbling with a feature that would use 
DW_AT_ranges on DW_TAG_subprograms - totally valid DWARF, but unsurprisingly, 
DWARF consumers that'd never had to deal with DWARF like this before didn't 
have support for it (lldb, in particular - had partial support, hopefully it's 
got full support for at least single-length DW_AT_ranges, but I think last I 
checked there were still issues with multi-element ranges))

> Also the only DWZ format currently in use is a non-standard GNU extension of 
> DWARF-4.

Which extensions would those be? At a glance when looking at the small dwz 
example earlier I didn't notice any DWARF extensions. (though, in any case, we 
deal with extensions on a pretty regular basis - see all the DWARFv4 + Split 
DWARF work, for instance (& then the call_site/call_site_parameter stuff, etc))

> The DWZ format has been designed for GDB making it difficult to parse it by 
> LLDB. LLDB does per-DIE DWARF->IR conversion compared to GDB doing per-CU 
> DWARF->IR conversion, that means that LLDB needs to keep context of the DWZ 
> parent DIE (as DWZ recursively imports DW_TAG_partial_unit) for each its 
> reference of DIE in its various indices which is a PITA.

(probably best to leave out the 'PITA'/swear/colourful language)

I'm not sure I follow the description here - when generating Clang ASTs from 
some DIE in LLDB, one would still have to potentially pass through multiple CUs 
to retrieve all the relevant types, especially under any form of LTO - are 
there particular features of DWZ that make this more/differently problematic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D96778#2576927 , @dblaikie wrote:

> What part of DWZ is specified by DWARFv5?

`DW_*_sup` being used by DWZ with DWARF-4 as `DW_*_GNU_*_alt` (and dwarfstd.org 
proposal 

 having it as `DW_*_alt`) for DWZ common files located in 
`/usr/lib/debug/.dwz/` where each file is for one rpm package sharing DIEs to 
multiple *.debug files of the same rpm package.
That is probably all but I cannot check more as www.dwarfstd.org 
 is down now.

> (perhaps the original creation of partial units was motivated by DWZ?

Rather DWZ was motivated by partial units IMO.

> Which extensions would those be?

Those `DW_*_alt`/`DW_*_sup` ones.

> At a glance when looking at the small dwz example earlier I didn't notice any 
> DWARF extensions.

This example is not using the DWZ common file (that makes sense only for >=2 
`*.debug` files). Still `DW_TAG_partial_unit`, `DW_AT_import` etc. is used only 
by DWZ and AFAIK no other tool so it needs extra implementation in all the 
tools incl. LLDB.

> I'm not sure I follow the description here - when generating Clang ASTs from 
> some DIE in LLDB, one would still have to potentially pass through multiple 
> CUs to retrieve all the relevant types, especially under any form of LTO - 
> are there particular features of DWZ that make this more/differently 
> problematic?

If you have a reference to DIE then LLDB tries to ask for `DW_AT_language` of 
its CU. But if the DIE is in `DW_TAG_partial_unit` (PU) there is no 
`DW_AT_language` and you do not know which CU did use 
`DW_TAG_imported_unit->DW_AT_import` for that PU. Moreover the 
`DW_TAG_partial_unit` can be imported by two different CUs one having 
`DW_AT_language` as C and the other as C++. That is IMO a bug, each PU should 
have its `DW_AT_language` and then PU with different language would be separate.

Still such format change would not help much as `DWARFASTParser`, 
`CompilerDeclContext` and similar stuff needs to come from the parent CU as 
otherwise LLDB all breaks down when some DIEs (from `DW_AT_partial_unit` 
possibly even from DWZ common file) come from a different AST.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96778#2576980 , @jankratochvil 
wrote:

> In D96778#2576927 , @dblaikie wrote:
>
>> What part of DWZ is specified by DWARFv5?
>
> Only `DW_*_sup` - being used by DWZ with DWARF-4 as `DW_*_GNU_*_alt` (and 
> dwarfstd.org proposal  
> having it as `DW_*_alt`) for DWZ common files located in 
> `/usr/lib/debug/.dwz/` where each file is for one rpm package sharing DIEs to 
> multiple *.debug files of the same rpm package.

Ah, thanks for the pointers! I didn't know about the cross-file feature of dwz.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96778

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

[Lldb-commits] [lldb] b0186c2 - [lldb] Refine ThreadPlan::ShouldAutoContinue

[Dave Lee via lldb-commits]

Author: Dave Lee
Date: 2021-02-20T17:25:31-08:00
New Revision: b0186c25c62e79869d37b81ad819c67a3e97b0cd

URL: 
https://github.com/llvm/llvm-project/commit/b0186c25c62e79869d37b81ad819c67a3e97b0cd
DIFF: 
https://github.com/llvm/llvm-project/commit/b0186c25c62e79869d37b81ad819c67a3e97b0cd.diff

LOG: [lldb] Refine ThreadPlan::ShouldAutoContinue

Adjust `ShouldAutoContinue` to be available to any thread plan previous to the 
plan that
explains a stop, not limited to the parent to the plan that explains the stop.

Before this change, `Thread::ShouldStop` did the following:

1. find the plan that explains the stop
2. if it's not a master plan, continue processing previous (aka parent) plans
3. first, call `ShouldAutoContinue` on the immediate parent of the explaining 
plan
4. then loop over previous plans, calling `ShouldStop` and `MischiefManaged`

Of note, the iteration in step 4 does not call `ShouldAutoContinue`, so again 
only the
plan just prior to the explaining plan is given the opportunity to override 
whether to
continue or stop.

This commit changes the loop call `ShouldAutoContinue`, giving each plan the 
opportunity
to override `ShouldStop` of previous plans.

Why? This allows a plan to do the following:

1. mark itself done and be popped off the stack
2. allow parent plans to finish their work, and to also be popped off the stack
3. and finally, have the thread continue, not stop

This is useful for stepping into async functions. A plan will would step far 
enough
enough to set a breakpoint on the async target, and then use 
`ShouldAutoContinue` to
unwind the necessary stepping, and then have the calling thread continue.

Differential Revision: https://reviews.llvm.org/D97076

Added: 


Modified: 
lldb/include/lldb/Target/ThreadPlan.h
lldb/source/Target/Thread.cpp

Removed: 




diff  --git a/lldb/include/lldb/Target/ThreadPlan.h 
b/lldb/include/lldb/Target/ThreadPlan.h
index 30e81f6e3050..007e56d4babe 100644
--- a/lldb/include/lldb/Target/ThreadPlan.h
+++ b/lldb/include/lldb/Target/ThreadPlan.h
@@ -361,6 +361,12 @@ class ThreadPlan : public 
std::enable_shared_from_this,
 
   virtual bool ShouldStop(Event *event_ptr) = 0;
 
+  /// Returns whether this thread plan overrides the `ShouldStop` of
+  /// subsequently processed plans.
+  ///
+  /// When processing the thread plan stack, this function gives plans the
+  /// ability to continue - even when subsequent plans return true from
+  /// `ShouldStop`. \see Thread::ShouldStop
   virtual bool ShouldAutoContinue(Event *event_ptr) { return false; }
 
   // Whether a "stop class" event should be reported to the "outside world".

diff  --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index 6670caa54392..854f7e81153e 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -829,6 +829,8 @@ bool Thread::ShouldStop(Event *event_ptr) {
   ThreadPlan *plan_ptr = current_plan;
   while ((plan_ptr = GetPreviousPlan(plan_ptr)) != nullptr) {
 if (plan_ptr->PlanExplainsStop(event_ptr)) {
+  LLDB_LOGF(log, "Plan %s explains stop.", plan_ptr->GetName());
+
   should_stop = plan_ptr->ShouldStop(event_ptr);
 
   // plan_ptr explains the stop, next check whether plan_ptr is done,
@@ -860,10 +862,7 @@ bool Thread::ShouldStop(Event *event_ptr) {
   }
 
   if (!done_processing_current_plan) {
-bool over_ride_stop = current_plan->ShouldAutoContinue(event_ptr);
-
-LLDB_LOGF(log, "Plan %s explains stop, auto-continue %i.",
-  current_plan->GetName(), over_ride_stop);
+bool override_stop = false;
 
 // We're starting from the base plan, so just let it decide;
 if (current_plan->IsBasePlan()) {
@@ -884,20 +883,24 @@ bool Thread::ShouldStop(Event *event_ptr) {
   if (should_stop)
 current_plan->WillStop();
 
-  // If a Master Plan wants to stop, and wants to stick on the stack,
-  // we let it. Otherwise, see if the plan's parent wants to stop.
+  if (current_plan->ShouldAutoContinue(event_ptr)) {
+override_stop = true;
+LLDB_LOGF(log, "Plan %s auto-continue: true.",
+  current_plan->GetName());
+  }
+
+  // If a Master Plan wants to stop, we let it. Otherwise, see if the
+  // plan's parent wants to stop.
 
+  PopPlan();
   if (should_stop && current_plan->IsMasterPlan() &&
   !current_plan->OkayToDiscard()) {
-PopPlan();
 break;
-  } else {
-PopPlan();
+  }
 
-current_plan = GetCurrentPlan();
-if (current_plan == nullptr) {
-  break;
-}
+  current_plan = GetCurrentPlan();
+  if (current_plan == nullptr) {
+break;
   }
 } else {
   break;
@@ -905,7 +908,7 @@ bool Thread::Should

[Lldb-commits] [PATCH] D97076: [lldb] Refine ThreadPlan::ShouldAutoContinue

[Dave Lee via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb0186c25c62e: [lldb] Refine ThreadPlan::ShouldAutoContinue 
(authored by kastiglione).

Changed prior to commit:
  https://reviews.llvm.org/D97076?vs=325055&id=325263#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97076

Files:
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/source/Target/Thread.cpp


Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -829,6 +829,8 @@
   ThreadPlan *plan_ptr = current_plan;
   while ((plan_ptr = GetPreviousPlan(plan_ptr)) != nullptr) {
 if (plan_ptr->PlanExplainsStop(event_ptr)) {
+  LLDB_LOGF(log, "Plan %s explains stop.", plan_ptr->GetName());
+
   should_stop = plan_ptr->ShouldStop(event_ptr);
 
   // plan_ptr explains the stop, next check whether plan_ptr is done,
@@ -860,10 +862,7 @@
   }
 
   if (!done_processing_current_plan) {
-bool over_ride_stop = current_plan->ShouldAutoContinue(event_ptr);
-
-LLDB_LOGF(log, "Plan %s explains stop, auto-continue %i.",
-  current_plan->GetName(), over_ride_stop);
+bool override_stop = false;
 
 // We're starting from the base plan, so just let it decide;
 if (current_plan->IsBasePlan()) {
@@ -884,20 +883,24 @@
   if (should_stop)
 current_plan->WillStop();
 
-  // If a Master Plan wants to stop, and wants to stick on the stack,
-  // we let it. Otherwise, see if the plan's parent wants to stop.
+  if (current_plan->ShouldAutoContinue(event_ptr)) {
+override_stop = true;
+LLDB_LOGF(log, "Plan %s auto-continue: true.",
+  current_plan->GetName());
+  }
+
+  // If a Master Plan wants to stop, we let it. Otherwise, see if the
+  // plan's parent wants to stop.
 
+  PopPlan();
   if (should_stop && current_plan->IsMasterPlan() &&
   !current_plan->OkayToDiscard()) {
-PopPlan();
 break;
-  } else {
-PopPlan();
+  }
 
-current_plan = GetCurrentPlan();
-if (current_plan == nullptr) {
-  break;
-}
+  current_plan = GetCurrentPlan();
+  if (current_plan == nullptr) {
+break;
   }
 } else {
   break;
@@ -905,7 +908,7 @@
   }
 }
 
-if (over_ride_stop)
+if (override_stop)
   should_stop = false;
   }
 
Index: lldb/include/lldb/Target/ThreadPlan.h
===
--- lldb/include/lldb/Target/ThreadPlan.h
+++ lldb/include/lldb/Target/ThreadPlan.h
@@ -361,6 +361,12 @@
 
   virtual bool ShouldStop(Event *event_ptr) = 0;
 
+  /// Returns whether this thread plan overrides the `ShouldStop` of
+  /// subsequently processed plans.
+  ///
+  /// When processing the thread plan stack, this function gives plans the
+  /// ability to continue - even when subsequent plans return true from
+  /// `ShouldStop`. \see Thread::ShouldStop
   virtual bool ShouldAutoContinue(Event *event_ptr) { return false; }
 
   // Whether a "stop class" event should be reported to the "outside world".


Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -829,6 +829,8 @@
   ThreadPlan *plan_ptr = current_plan;
   while ((plan_ptr = GetPreviousPlan(plan_ptr)) != nullptr) {
 if (plan_ptr->PlanExplainsStop(event_ptr)) {
+  LLDB_LOGF(log, "Plan %s explains stop.", plan_ptr->GetName());
+
   should_stop = plan_ptr->ShouldStop(event_ptr);
 
   // plan_ptr explains the stop, next check whether plan_ptr is done,
@@ -860,10 +862,7 @@
   }
 
   if (!done_processing_current_plan) {
-bool over_ride_stop = current_plan->ShouldAutoContinue(event_ptr);
-
-LLDB_LOGF(log, "Plan %s explains stop, auto-continue %i.",
-  current_plan->GetName(), over_ride_stop);
+bool override_stop = false;
 
 // We're starting from the base plan, so just let it decide;
 if (current_plan->IsBasePlan()) {
@@ -884,20 +883,24 @@
   if (should_stop)
 current_plan->WillStop();
 
-  // If a Master Plan wants to stop, and wants to stick on the stack,
-  // we let it. Otherwise, see if the plan's parent wants to stop.
+  if (current_plan->ShouldAutoContinue(event_ptr)) {
+override_stop = true;
+LLDB_LOGF(log, "Plan %s auto-continue: true.",
+  current_plan->GetName());
+  }
+
+  // If a Master Plan wants to stop, we let it. Otherwise, see if t