[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[David Spickett via Phabricator via lldb-commits]

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

Much clearer now, thanks. LGTM with the one nit fixed.




Comment at: 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py:42
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if illegal_address == None or \
+region.GetRegionEnd() > illegal_address:

General rule is to use `is None` instead of `== None`, though the result is the 
same here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

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

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Will Hawkins via Phabricator via lldb-commits]

hawkinsw added a comment.

@DavidSpickett Thanks (again) for the feedback. Once I fix the nit, is the 
proper protocol for you to review again? I am sorry to ask but I don't know the 
right procedure and don't want to do the wrong thing!




Comment at: 
lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py:42
+regions.GetMemoryRegionAtIndex(region_idx, region)
+if illegal_address == None or \
+region.GetRegionEnd() > illegal_address:

DavidSpickett wrote:
> General rule is to use `is None` instead of `== None`, though the result is 
> the same here.
Yes! I always make that error!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

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

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 3 inline comments as done.
jingham added a comment.

This patch treats the Signal structure held in the target differently from 
UnixSignals::Signal.  The latter always has to have values for all three 
actions and needs to have a signal number.  The former doesn't actually claim 
to know a signal number, it only works on strings, since it might get set 
before we can know what signal number a given signal string has (signals with 
the same name do have different numbers on different platforms).  Also, it 
doesn't record actual values, it records user intents, so it needs to be 
tri-state - thus the LazyBools.

I think the two are sufficiently different in use that it's cleaner to have 
separate types for them.




Comment at: lldb/include/lldb/Target/Target.h:1421
+protected:
+  struct DummySignalValues {
+LazyBool pass = eLazyBoolCalculate;

clayborg wrote:
> We should make UnixSignals::Signal a public class and then just save a 
> std::vector of those inside this class. That class doesn't contain a signal 
> number, so we should be able to re-use it here and avoid creating a new class 
> that mimic what is contains. 
The data structure held by the target is different from the one that represents 
actual signals because the Target one needs to record not just "true or false" 
but "whether set by the user."  That's why I used LazyBools not bools in the 
structure.  That's important because if you are setting a signal handling in 
the .lldbinit you don't yet know what the default value is, so you need to be 
able to say "change print to false, and leave the others at their default 
values".

So I don't think that the UnixSignals::Signal is the right data structure of 
rthis.



Comment at: lldb/include/lldb/Target/Target.h:1429
+  };
+  using DummySignalElement = std::pair;
+  static bool UpdateSignalFromDummy(lldb::UnixSignalsSP signals_sp, 

clayborg wrote:
> Maybe store a std::vector objects filled out as much as 
> possible here? Then we don't need the DummySignalElement type at all.
See the comment above.



Comment at: lldb/include/lldb/Target/Target.h:1533-1536
+  std::map m_dummy_signals;/// These are used 
to
+  /// set the signal state when you don't have a process and more usefully 
+  /// in the Dummy target where you can't know exactly what signals you 
+  /// will have.

clayborg wrote:
> Switch to std::vector< UnixSignal::Signal> here and then we can prime the 
> UnixSignals with the vector?
See the first comment.



Comment at: lldb/include/lldb/Target/UnixSignals.h:120
 bool m_suppress : 1, m_stop : 1, m_notify : 1;
+bool m_default_suppress : 1, m_default_stop : 1, m_default_notify : 1;
 

clayborg wrote:
> If we store a vector in the target, can we reset the 
> default values from another "Signal" struct instead of saving it here? Not a 
> big deal if not, but just wondering.
I wondered about that, but having to make another version of Signals and hope 
it's constructed the same way seems a fallible way to get the default value the 
Signal object was constructed with.  This is just 3 bytes, and won't actually 
change the size of the structure (plus these are not structures we have lots 
and lots of).  So I think doing the straightforward thing is better.



Comment at: lldb/include/lldb/Target/UnixSignals.h:126
 ~Signal() = default;
+void Reset(bool reset_stop, bool reset_notify, bool reset_suppress);
   };

clayborg wrote:
> To follow up with he above comment, this _could_ become:
> ```
> void Reset(const UnixSignals::Signal &signal);
> ```
> we can find the UnixSignals::Signal object for this signal first in the 
> target and pass it down into here?
The code that's resetting this doesn't really need to know what values the 
signal currently has.  It knows that it changed "pass" but not "notify" or 
"suppress" and just wants them reset to the default values.  Having to look up 
some other Signal to do this seems needlessly complex.



Comment at: lldb/source/Commands/Options.td:754
+  def process_handle_dummy : Option<"dummy", "d">, Group<2>,
+Desc<"Also clear the values in the dummy target so they won't be inherited 
by new targets.">;
 }

clayborg wrote:
> Do we need the "Also" in the documentation here? Is this option only 
> available when used with another option?
Right now, you can either clear the values in the current Target or in the 
Current target and the Dummy Target.  So the "also" is correct.  If you think 
it's useful to only clear the dummy target's values, I can change the code to 
do that easily.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

___
lldb-commits mailing list
lldb-commits@lists.llvm

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So the "process handle" command allows us to set signals by signal number as 
well. Does this patch support this? It seems like it wouldn't be too hard to do 
if we wanted to handle this. Lemme know what you think, other than that LGTM.




Comment at: lldb/include/lldb/Target/Target.h:1421
+protected:
+  struct DummySignalValues {
+LazyBool pass = eLazyBoolCalculate;

jingham wrote:
> clayborg wrote:
> > We should make UnixSignals::Signal a public class and then just save a 
> > std::vector of those inside this class. That class doesn't contain a signal 
> > number, so we should be able to re-use it here and avoid creating a new 
> > class that mimic what is contains. 
> The data structure held by the target is different from the one that 
> represents actual signals because the Target one needs to record not just 
> "true or false" but "whether set by the user."  That's why I used LazyBools 
> not bools in the structure.  That's important because if you are setting a 
> signal handling in the .lldbinit you don't yet know what the default value 
> is, so you need to be able to say "change print to false, and leave the 
> others at their default values".
> 
> So I don't think that the UnixSignals::Signal is the right data structure of 
> rthis.
Makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

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

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 3 inline comments as done.
jingham added a comment.

In D126259#3534729 , @clayborg wrote:

> So the "process handle" command allows us to set signals by signal number as 
> well. Does this patch support this? It seems like it wouldn't be too hard to 
> do if we wanted to handle this. Lemme know what you think, other than that 
> LGTM.

Ah, I forgot about specifying the signal by number.  Before you have a process 
I don't think we should allow signals by number.  The mapping signal number -> 
"signal name for any platform" is not 1-1 so we couldn't guarantee we were 
doing the right thing here.  I'll put in a check for "specified by number with 
no process" and error out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

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

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D126259#3534919 , @jingham wrote:

> In D126259#3534729 , @clayborg 
> wrote:
>
>> So the "process handle" command allows us to set signals by signal number as 
>> well. Does this patch support this? It seems like it wouldn't be too hard to 
>> do if we wanted to handle this. Lemme know what you think, other than that 
>> LGTM.
>
> Ah, I forgot about specifying the signal by number.  Before you have a 
> process I don't think we should allow signals by number.  The mapping signal 
> number -> "signal name for any platform" is not 1-1 so we couldn't guarantee 
> we were doing the right thing here.  I'll put in a check for "specified by 
> number with no process" and error out.

I would add a test for this and make sure it fails gracefully in that case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

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

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 431745.
jingham added a comment.

Don't allow setting signal actions by signal number before you have a process.

We don't know what signal 20 is going to end up being till we have a process, 
so allowing this by number doesn't make sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

Files:
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/UnixSignals.h
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/Options.td
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/source/Target/UnixSignals.cpp
  lldb/test/API/commands/process/handle/Makefile
  lldb/test/API/commands/process/handle/TestProcessHandle.py
  lldb/test/API/commands/process/handle/main.cpp
  lldb/test/API/functionalities/signal/raise/TestRaise.py
  lldb/unittests/Signals/UnixSignalsTest.cpp

Index: lldb/unittests/Signals/UnixSignalsTest.cpp
===
--- lldb/unittests/Signals/UnixSignalsTest.cpp
+++ lldb/unittests/Signals/UnixSignalsTest.cpp
@@ -53,6 +53,29 @@
   EXPECT_EQ(LLDB_INVALID_SIGNAL_NUMBER, signals.GetNextSignalNumber(16));
 }
 
+TEST(UnixSignalsTest, Reset) {
+  TestSignals signals;
+  bool stop_val = signals.GetShouldStop(2);
+  bool notify_val   = signals.GetShouldNotify(2);
+  bool suppress_val = signals.GetShouldSuppress(2);
+  
+  // Change two, then reset one and make sure only that one was reset:
+  EXPECT_EQ(true, signals.SetShouldNotify(2, !notify_val));
+  EXPECT_EQ(true, signals.SetShouldSuppress(2, !suppress_val));
+  EXPECT_EQ(true, signals.ResetSignal(2, false, true, false));
+  EXPECT_EQ(stop_val, signals.GetShouldStop(2));
+  EXPECT_EQ(notify_val, signals.GetShouldStop(2));
+  EXPECT_EQ(!suppress_val, signals.GetShouldNotify(2));
+  
+  // Make sure reset with no arguments resets them all:
+  EXPECT_EQ(true, signals.SetShouldSuppress(2, !suppress_val));
+  EXPECT_EQ(true, signals.SetShouldNotify(2, !notify_val));
+  EXPECT_EQ(true, signals.ResetSignal(2));
+  EXPECT_EQ(stop_val, signals.GetShouldStop(2));
+  EXPECT_EQ(notify_val, signals.GetShouldNotify(2));
+  EXPECT_EQ(suppress_val, signals.GetShouldSuppress(2));
+}
+
 TEST(UnixSignalsTest, GetInfo) {
   TestSignals signals;
 
Index: lldb/test/API/functionalities/signal/raise/TestRaise.py
===
--- lldb/test/API/functionalities/signal/raise/TestRaise.py
+++ lldb/test/API/functionalities/signal/raise/TestRaise.py
@@ -36,6 +36,10 @@
 
 def launch(self, target, signal):
 # launch the process, do not stop at entry point.
+# If we have gotten the default for this signal, reset that as well.
+if len(self.default_pass) != 0:
+lldbutil.set_actions_for_signal(self, signal, self.default_pass, self.default_stop, self.default_notify)
+
 process = target.LaunchSimple(
 [signal], None, self.get_process_working_directory())
 self.assertTrue(process, PROCESS_IS_VALID)
@@ -64,27 +68,19 @@
 target = self.dbg.CreateTarget(exe)
 self.assertTrue(target, VALID_TARGET)
 lldbutil.run_break_set_by_symbol(self, "main")
+self.default_pass = ""
+self.default_stop = ""
+self.default_notify = ""
 
 # launch
 process = self.launch(target, signal)
 signo = process.GetUnixSignals().GetSignalNumberFromName(signal)
 
 # retrieve default signal disposition
-return_obj = lldb.SBCommandReturnObject()
-self.dbg.GetCommandInterpreter().HandleCommand(
-"process handle %s " % signal, return_obj)
-match = re.match(
-'NAME *PASS *STOP *NOTIFY.*(false|true) *(false|true) *(false|true)',
-return_obj.GetOutput(),
-re.IGNORECASE | re.DOTALL)
-if not match:
-self.fail('Unable to retrieve default signal disposition.')
-default_pass = match.group(1)
-default_stop = match.group(2)
-default_notify = match.group(3)
+(self.default_pass, self.default_stop, self.default_notify) = lldbutil.get_actions_for_signal(self, signal)
 
 # Make sure we stop at the signal
-self.set_handle(signal, "false", "true", "true")
+lldbutil.set_actions_for_signal(self, signal, "false", "true", "true")
 process.Continue()
 self.assertEqual(process.GetState(), lldb.eStateStopped)
 thread = lldbutil.get_stopped_thread(process, lldb.eStopReasonSignal)
@@ -102,12 +98,11 @@
 self.assertEqual(process.GetState(), lldb.eStateExited)
 self.assertEqual(process.GetExitStatus(), 0)
 
-# launch again
 process = self.launch(target, signal)
 
 # Make sure we do not stop at the signal. We should still get the
 # notification.
-self.set

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D126259#3534944 , @clayborg wrote:

> In D126259#3534919 , @jingham wrote:
>
>> In D126259#3534729 , @clayborg 
>> wrote:
>>
>>> So the "process handle" command allows us to set signals by signal number 
>>> as well. Does this patch support this? It seems like it wouldn't be too 
>>> hard to do if we wanted to handle this. Lemme know what you think, other 
>>> than that LGTM.
>>
>> Ah, I forgot about specifying the signal by number.  Before you have a 
>> process I don't think we should allow signals by number.  The mapping signal 
>> number -> "signal name for any platform" is not 1-1 so we couldn't guarantee 
>> we were doing the right thing here.  I'll put in a check for "specified by 
>> number with no process" and error out.
>
> I would add a test for this and make sure it fails gracefully in that case

I added that.  You get an error from the command saying you can't set signal 
actions by number w/o a process.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

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

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D126259#3534997 , @jingham wrote:

> Don't allow setting signal actions by signal number before you have a process.

I understand

> We don't know what signal 20 is going to end up being till we have a process, 
> so allowing this by number doesn't make sense.

I am saying that it would be a good idea to make sure an error is returned when 
you do try and set a signal by number before a process exists and make sure 
there is a test that covers this if it isn't already in the current patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

> It doesn't make sense to require a stable hash algorithm for an internal 
> cache file.

It's at least a stronger stability requirement than may be provided before - 
like: stable across process boundaries, at least, by the sounds of it? yeah?
& then still raises the question for me what about minor version updates, is it 
expected to be stable across those?

It'd be a bit subtle to have to know when to go and update the lldb cache 
version number because this hash function has changed, for instance. It might 
be more suitable to have lldb explicitly request a known hash function rather 
than the generic one (even if they're identical at the moment) so updates to 
LLVM's core libraries don't subtly break the hashing/cache system here. (I 
would guess there's no cross-version hash testing? So it seems like such a 
change could produce fairly subtle breakage that would slip under the radar 
fairly easily?)


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

https://reviews.llvm.org/D122974

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

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D126259#3535004 , @clayborg wrote:

> In D126259#3534997 , @jingham wrote:
>
>> Don't allow setting signal actions by signal number before you have a 
>> process.
>
> I understand
>
>> We don't know what signal 20 is going to end up being till we have a 
>> process, so allowing this by number doesn't make sense.
>
> I am saying that it would be a good idea to make sure an error is returned 
> when you do try and set a signal by number before a process exists and make 
> sure there is a test that covers this if it isn't already in the current 
> patch.

I think we crossed paths.  I added that in the last update of the diff.  The 
test is TestHandleProcess.py:20, and the code to enforce it starts around 1671 
of CommandObjectProcess.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

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

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Greg Clayton via Phabricator via lldb-commits]

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

In D126259#3535688 , @jingham wrote:

> In D126259#3535004 , @clayborg 
> wrote:
>
>> In D126259#3534997 , @jingham 
>> wrote:
>>
>>> Don't allow setting signal actions by signal number before you have a 
>>> process.
>>
>> I understand
>>
>>> We don't know what signal 20 is going to end up being till we have a 
>>> process, so allowing this by number doesn't make sense.
>>
>> I am saying that it would be a good idea to make sure an error is returned 
>> when you do try and set a signal by number before a process exists and make 
>> sure there is a test that covers this if it isn't already in the current 
>> patch.
>
> I think we crossed paths.  I added that in the last update of the diff.  The 
> test is TestHandleProcess.py:20, and the code to enforce it starts around 
> 1671 of CommandObjectProcess.cpp.

I see it, it is hard to tell what got updated. Thanks for the info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

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

[Lldb-commits] [PATCH] D125148: Add an example command that runs to one of a set of breakpoints

[Jim Ingham via Phabricator via lldb-commits]

jingham updated this revision to Diff 431837.
jingham added a comment.

Added support for breakpoint names as well as ID's.
Addressed other review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125148

Files:
  lldb/examples/python/cont_to_bkpt.py


Index: lldb/examples/python/cont_to_bkpt.py
===
--- /dev/null
+++ lldb/examples/python/cont_to_bkpt.py
@@ -0,0 +1,78 @@
+import lldb
+
+class ContinueToBreakpoint:
+def __init__(self, debugger, unused):
+return
+
+def __call__(self, debugger, command, exe_ctx, result):
+
+target = exe_ctx.target
+process = exe_ctx.process
+
+if  not target.IsValid():
+result.SetError("Need a valid target")
+return
+if not process.IsValid():
+result.SetError("Need a valid process")
+return
+
+bkpt_strs = command.split()
+bkpt_ids = []
+for str in bkpt_strs:
+is_a_number = True
+bkpt_id = lldb.LLDB_INVALID_BREAK_ID
+try:
+bkpt_id = int(str)
+except:
+is_a_number = False
+
+if is_a_number:
+bkpt = target.FindBreakpointByID(bkpt_id)
+if not bkpt.IsValid():
+result.SetError("Input is a number, but not a known 
breakpoint ID: {0}".format(str))
+return
+
+bkpt_ids.append(bkpt_id)
+else:
+bkpts_for_name = lldb.SBBreakpointList(target)
+if target.FindBreakpointsByName(str, bkpts_for_name):
+for idx in range(0, bkpts_for_name.GetSize()):
+
bkpt_ids.append(bkpts_for_name.GetBreakpointAtIndex(idx).GetID())
+else:
+result.SetError("Input must be breakpoint id's or 
breakpoint names: {0}".format(str))
+return
+
+if len(bkpt_ids) == 0:
+result.SetError("No breakpoint to run to")
+result.SetStatus(lldb.eReturnStatusFailed)
+return
+
+disabled_bkpts = []
+for idx in range(0, target.num_breakpoints):
+bkpt = target.GetBreakpointAtIndex(idx)
+bkpt_id = bkpt.GetID()
+if bkpt_id not in bkpt_ids:
+if bkpt.enabled:
+disabled_bkpts.append(bkpt)
+bkpt.enabled = False
+old_async = debugger.GetAsync()
+debugger.SetAsync(False)
+process.Continue()
+strm = lldb.SBStream()
+if process.state == lldb.eStateExited:
+result.PutCString("process exited with state: 
{0}".format(process.exit_state))
+else:
+thread = process.GetSelectedThread()
+thread.GetStatus(strm)
+result.PutCString(strm.GetData())
+
+result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
+for bkpt in disabled_bkpts:
+bkpt.enabled = True;
+debugger.SetAsync(old_async)
+
+def get_short_help(self):
+return "takes a list of breakpoint ID's and continues the process 
until one of the breakpoint IDs passed in is hit"
+
+def __lldb_init_module(debugger, unused):
+debugger.HandleCommand("command script add -c {0}.ContinueToBreakpoint 
continue_to_bkpts".format(__name__))


Index: lldb/examples/python/cont_to_bkpt.py
===
--- /dev/null
+++ lldb/examples/python/cont_to_bkpt.py
@@ -0,0 +1,78 @@
+import lldb
+
+class ContinueToBreakpoint:
+def __init__(self, debugger, unused):
+return
+
+def __call__(self, debugger, command, exe_ctx, result):
+
+target = exe_ctx.target
+process = exe_ctx.process
+
+if  not target.IsValid():
+result.SetError("Need a valid target")
+return
+if not process.IsValid():
+result.SetError("Need a valid process")
+return
+
+bkpt_strs = command.split()
+bkpt_ids = []
+for str in bkpt_strs:
+is_a_number = True
+bkpt_id = lldb.LLDB_INVALID_BREAK_ID
+try:
+bkpt_id = int(str)
+except:
+is_a_number = False
+
+if is_a_number:
+bkpt = target.FindBreakpointByID(bkpt_id)
+if not bkpt.IsValid():
+result.SetError("Input is a number, but not a known breakpoint ID: {0}".format(str))
+return
+
+bkpt_ids.append(bkpt_id)
+else:
+bkpts_for_name = lldb.SBBreakpointList(target)
+if target.FindBreakpointsByName(str, bkpts_for_name):
+for idx in range(0, bkpts_for_name.GetSize()):
+bkpt_ids.append(bkpts_for

[Lldb-commits] [PATCH] D125148: Add an example command that runs to one of a set of breakpoints

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 7 inline comments as done.
jingham added a comment.

The patch gets a little hard to read with all the no-longer in the right places 
comments, but I think I addressed everything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125148

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

[Lldb-commits] [PATCH] D125148: Add an example command that runs to one of a set of breakpoints

[Dave Lee via Phabricator via lldb-commits]

kastiglione added a comment.

> The patch gets a little hard to read with all the no-longer in the right 
> places comments

I just learned there's a keyboard shortcut (`A`) that hides them all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125148

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

[Lldb-commits] [lldb] f179f40 - [lldb] Disable modules in Apple-lldb-base

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-05-24T17:17:14-07:00
New Revision: f179f403c8579c939e09d27c383b6263cf2523aa

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

LOG: [lldb] Disable modules in Apple-lldb-base

The LLDB website recommends using the CMake caches to build on macOS.
Although modules result in a faster build, this configuration tends to
break occasionally because it's specific to our platform. I don't expect
newcomers to be able to deal with those kind of breakages so don't
enable them by default.

Added: 


Modified: 
lldb/cmake/caches/Apple-lldb-base.cmake

Removed: 




diff  --git a/lldb/cmake/caches/Apple-lldb-base.cmake 
b/lldb/cmake/caches/Apple-lldb-base.cmake
index 76ab843c4b6b7..4d4f02bfae95b 100644
--- a/lldb/cmake/caches/Apple-lldb-base.cmake
+++ b/lldb/cmake/caches/Apple-lldb-base.cmake
@@ -3,7 +3,6 @@ set(CMAKE_EXPORT_COMPILE_COMMANDS ON CACHE BOOL "")
 
 set(LLVM_TARGETS_TO_BUILD X86;ARM;AArch64 CACHE STRING "")
 set(LLVM_ENABLE_ASSERTIONS ON CACHE BOOL "")
-set(LLVM_ENABLE_MODULES ON CACHE BOOL "")
 
 set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
 set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")



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

[Lldb-commits] [lldb] 436eaf8 - [lldb] Improve TestAppleSimulatorOSType.py error message

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2022-05-24T17:17:26-07:00
New Revision: 436eaf8d32fa8d5db6782f4da30d0b5b7c2690e8

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

LOG: [lldb] Improve TestAppleSimulatorOSType.py error message

This was inspired by D109336 which got reverted because we didn't want
the test to fail silently. This patch prints a more informative error
message when we fail to parse the simctl output while still failing the
test.

Differential revision: https://reviews.llvm.org/D126217

Added: 


Modified: 
lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py

Removed: 




diff  --git a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py 
b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
index 31fe282c7d910..840e970410121 100644
--- a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
+++ b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
@@ -17,9 +17,13 @@ class 
TestAppleSimulatorOSType(gdbremote_testcase.GdbRemoteTestCaseBase):
 
 def check_simulator_ostype(self, sdk, platform_name, 
arch=platform.machine()):
 cmd = ['xcrun', 'simctl', 'list', '-j', 'devices']
-self.trace(' '.join(cmd))
+cmd_str = ' '.join(cmd)
+self.trace(cmd_str)
 sim_devices_str = subprocess.check_output(cmd).decode("utf-8")
-sim_devices = json.loads(sim_devices_str)['devices']
+try:
+sim_devices = json.loads(sim_devices_str)['devices']
+except json.decoder.JSONDecodeError:
+self.fail("Could not parse '{}' output. Authorization 
denied?".format(cmd_str))
 # Find an available simulator for the requested platform
 deviceUDID = None
 deviceRuntime = None



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

[Lldb-commits] [PATCH] D126217: [lldb] Improve TestAppleSimulatorOSType.py error message

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG436eaf8d32fa: [lldb] Improve TestAppleSimulatorOSType.py 
error message (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126217

Files:
  lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py


Index: lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
===
--- lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
+++ lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
@@ -17,9 +17,13 @@
 
 def check_simulator_ostype(self, sdk, platform_name, 
arch=platform.machine()):
 cmd = ['xcrun', 'simctl', 'list', '-j', 'devices']
-self.trace(' '.join(cmd))
+cmd_str = ' '.join(cmd)
+self.trace(cmd_str)
 sim_devices_str = subprocess.check_output(cmd).decode("utf-8")
-sim_devices = json.loads(sim_devices_str)['devices']
+try:
+sim_devices = json.loads(sim_devices_str)['devices']
+except json.decoder.JSONDecodeError:
+self.fail("Could not parse '{}' output. Authorization 
denied?".format(cmd_str))
 # Find an available simulator for the requested platform
 deviceUDID = None
 deviceRuntime = None


Index: lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
===
--- lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
+++ lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
@@ -17,9 +17,13 @@
 
 def check_simulator_ostype(self, sdk, platform_name, arch=platform.machine()):
 cmd = ['xcrun', 'simctl', 'list', '-j', 'devices']
-self.trace(' '.join(cmd))
+cmd_str = ' '.join(cmd)
+self.trace(cmd_str)
 sim_devices_str = subprocess.check_output(cmd).decode("utf-8")
-sim_devices = json.loads(sim_devices_str)['devices']
+try:
+sim_devices = json.loads(sim_devices_str)['devices']
+except json.decoder.JSONDecodeError:
+self.fail("Could not parse '{}' output. Authorization denied?".format(cmd_str))
 # Find an available simulator for the requested platform
 deviceUDID = None
 deviceRuntime = None
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D126109: [lldb] Fix broken bad-address-breakpoint test

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D126109#3534522 , @hawkinsw wrote:

> @DavidSpickett Thanks (again) for the feedback. Once I fix the nit, is the 
> proper protocol for you to review again? I am sorry to ask but I don't know 
> the right procedure and don't want to do the wrong thing!

If the patch is accepted with a comment you can land the patch with the comment 
addressed. Unless you disagree in which case you can continue discussing in the 
code review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126109

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

[Lldb-commits] [PATCH] D125148: Add an example command that runs to one of a set of breakpoints

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/examples/python/cont_to_bkpt.py:5
+def __init__(self, debugger, unused):
+return
+

Nit: The "Pythonic" way of doing is using `pass`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125148

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

[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/include/lldb/Target/Target.h:1451
+  /// Print all the signals set in this target.
+  void PrintDummySignals(Stream &strm, Args signals, size_t num_signals);
+

Was `Args` supposed to be a reference here? If not why do we need the copy?

I didn't look at the implementation yet, but why do we need `num_signals`? 
Can't we count the number of args?



Comment at: lldb/include/lldb/Target/Target.h:1533
   lldb::StackFrameRecognizerManagerUP m_frame_recognizer_manager_up;
+  std::map m_dummy_signals;/// These are used 
to
+  /// set the signal state when you don't have a process and more usefully 

Does it matter that these are ordered? Can this use a `llvm::StringMap`?



Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1477-1479
+bool only_target_values;
+bool do_clear;
+bool dummy;

Let's initialize these to the same values as `Clear`.



Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1582
   bool DoExecute(Args &signal_args, CommandReturnObject &result) override {
-Target *target_sp = &GetSelectedTarget();
+Target *target_sp = &GetSelectedOrDummyTarget();
 

Not your change but why `Target *` and not `Target &`?



Comment at: lldb/source/Target/Process.cpp:2931-2934
+if (m_unix_signals_sp) {
+  StreamSP warning_strm = GetTarget().GetDebugger().GetAsyncErrorStream();
+  GetTarget().UpdateSignalsFromDummy(m_unix_signals_sp, warning_strm);
+}

Clang format



Comment at: lldb/source/Target/Target.cpp:293-294
   m_suppress_stop_hooks = false;
+  Args signal_args;
+  ClearDummySignals(signal_args);
 }

Instead of having to pass an empty Args here, can we make the input argument an 
`Optional = {}`? Then we can just call `ClearDummySignals` here. This 
looks like `signal_args` is an output argument. 



Comment at: lldb/source/Target/Target.cpp:3357
 
+void Target::AddDummySignal(const char *name, LazyBool pass, LazyBool notify, 
+LazyBool stop) {

It seems like this can take a `std::string`and `std::move` it into the pair 
below. Or a `StringRef` if you turn this into a StringMap as per the other 
suggestion.



Comment at: lldb/source/Target/Target.cpp:3366-3374
+auto elem = m_dummy_signals.find(name);
+if (elem != m_dummy_signals.end()) {
+  (*elem).second.pass = pass;
+  (*elem).second.notify = notify;
+  (*elem).second.stop = stop;
+  return;
+}

With a StringMap you can simplify all of this into:

```
auto& entry = m_dummy_signals[name];
entry.pass = pass;
entry.notify = notify;
...
```



Comment at: lldb/source/Target/Target.cpp:3386-3389
+  if (elem.second.pass == eLazyBoolYes)
+signals_sp->SetShouldSuppress(signo, false);
+  else if (elem.second.pass == eLazyBoolNo)
+signals_sp->SetShouldSuppress(signo, true);

I'm wondering if we can simplify this with a little utility function. Something 
like this:

```
static void helper(LazyBool b, std::function f) {
  switch(b) {
case eLazyBoolYes:
  return f(true);
case eLazyBookFalse:
  return f(false);
case eLazyBoolCalculate:
  return;
  }
  llvm_unreachable("all cases handled");
}
```

That way we can simplify this:

```
helper(elem.second.pass, [](bool b) { signals_sp->SetShouldSuppress(signo, b); 
});
```



Comment at: lldb/source/Target/Target.cpp:3436-3438
+  UnixSignalsSP signals_sp;
+  if (process_sp)
+process_sp->GetUnixSignals();

Should this have been 

```
if (process_sp)
signals_sp = process_sp->GetUnixSignals();
```

Now `signals_sp` is never initialized. 



Comment at: lldb/source/Target/Target.cpp:3458-3464
+auto str_for_lazy = [] (LazyBool lazy) -> const char * {
+  switch (lazy) {
+case eLazyBoolCalculate: return "not set";
+case eLazyBoolYes: return "true   ";
+case eLazyBoolNo: return "false  ";
+  }
+};

This seems super useful. Maybe this function, together with the other utility I 
suggested, can go in a LazyBoolUtils or something in Utility. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259

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

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[Luboš Luňák via Phabricator via lldb-commits]

llunak added a comment.

In D122974#3535669 , @dblaikie wrote:

>> It doesn't make sense to require a stable hash algorithm for an internal 
>> cache file.
>
> It's at least a stronger stability requirement than may be provided before - 
> like: stable across process boundaries, at least, by the sounds of it? yeah?

It's meant to be the same for the same library binary.

> & then still raises the question for me what about minor version updates, is 
> it expected to be stable across those?

Is anything expected to be stable across those? If so, that doesn't seem to be 
the reality of it (a random quick check finds two ABI changes just between 
14.0.4 and 14.0.5, e6de9ed37308e46560243229dd78e84542f37ead 
 and 
53433dd0b5034681e1866e74651e8527a29e9364 
).

> It'd be a bit subtle to have to know when to go and update the lldb cache 
> version number because this hash function has changed, for instance. It might 
> be more suitable to have lldb explicitly request a known hash function rather 
> than the generic one (even if they're identical at the moment) so updates to 
> LLVM's core libraries don't subtly break the hashing/cache system here. (I 
> would guess there's no cross-version hash testing? So it seems like such a 
> change could produce fairly subtle breakage that would slip under the radar 
> fairly easily?)

D124704  adds a unittest that compares 
StringMap::hash() to a known hardcoded value, so whenever the hash 
implementation changes, it won't be possible to unittest LLDB with that change, 
and that will be the time to change the lldb cache version. If the theory is 
that this should keep working even with the library changing without LLDB 
rebuild, then as I wrote above that theory needs double-checking first. And 
additionally a good question to ask would be if it's really a good idea to do 
incompatible implementation changes to a class that has half of its 
functionality inline. Finally, there have been already attempts to change the 
hash function to use the better non-zero seed (D97396 
), and they were reverted because something 
depended on the hash function not changing, so I don't see it that likely for 
the hash function to change just like that in a minor update.

But if after all this that's still the theory, I guess StringMap could get an 
additional template parameter specifying the hash function, if it's actually 
worth the effort.

@clayborg : BTW, this is exactly the reason why I wanted the cache files header 
to include a hash value for a known string.


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

https://reviews.llvm.org/D122974

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