[LLDB] Fix missing return value in SBBreakpointLocation::GetQueueName()

[fixathon]

Summary

Seems like a typo in the function that never returns a significant value

[clayborg]

There must be a test for this, obviously we don't have one, but check the blame log and ask the original author how this should be tested.

[jingham]

So far as I can tell, we don't have any significant tests for the "queue specific breakpoint or breakpoint location" feature. There's a pretty extensive test for identifying queues & getting more detail from them in test/API/macosx/queues/TestQueues.py, but it doesn't test the breakpoint feature.

Both SetQueueName and GetQueueName are called in the api/default_constructor test, but those tests don't assert anything about values, since they are on default-constructed objects. We also test that we can set a queue name on a breakpoint, serialize the breakpoint, deserialize it and get the queue name back out. But we can't reuse that for locations since we don't serialize breakpoint locations.

SBBreakpointLocation.{Get,Set}QueueName can be applied to any breakpoint, it doesn't require any queue support from the underlying OS. Get just gets back whatever string Set put in. So for coverage of this bug it should be enough to find some test that makes a breakpoint, then set a queue, make sure you get it back again.

But we really should add some tests that show this actually works.

Jim

[clayborg]

Thanks for the info! Should be easy to test this function at least like you mentioned

[fixathon]

Working on adding unit tests

[fixathon]

Added unit tests, and verified the fix is working.

[fixathon]

Fix duplicate name of the test function

[clayborg]

Looks good. I would just inline the contents of shadowed_bkpt_locations_setters_getters_test into test_shadowed_location_getters_setters since it isn't use anywhere else.

lldb/test/API/functionalities/breakpoint/breakpoint_locations/TestBreakpointLocations.py

29

unless you plan on re-using shadowed_bkpt_locations_setters_getters_test() I would just inline the function's contents here.

[fixathon]

Address the review comments and inline the tests into an existing test function

[fixathon]

Address the review comments

[clayborg]

As long as all values being set on the location differ from any that might be set on the breakpoint itself for any get/set tests you added this is good to go!

lldb/source/API/SBBreakpointLocation.cpp

 
 const char *SBBreakpointLocation::GetQueueName() const {
   LLDB_INSTRUMENT_VA(this);
 
   BreakpointLocationSP loc_sp = GetSP();
   if (loc_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         loc_sp->GetTarget().GetAPIMutex());
-    loc_sp->GetQueueName();
+    return loc_sp->GetQueueName();
   }
   return nullptr;
 }
 
 bool SBBreakpointLocation::IsResolved() {
   LLDB_INSTRUMENT_VA(this);
 
   BreakpointLocationSP loc_sp = GetSP();

lldb/test/API/functionalities/breakpoint/breakpoint_locations/TestBreakpointLocations.py

class BreakpointLocationsTestCase(TestBase):
     def test_shadowed_cond_options(self):
         """Test that options set on the breakpoint and location behave correctly."""
         self.build()
         self.shadowed_bkpt_cond_test()
 
     def test_shadowed_command_options(self):
         """Test that options set on the breakpoint and location behave correctly."""
         self.build()
         self.shadowed_bkpt_command_test()
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to break inside main().
         self.line = line_number('main.c', '// Set break point at this line.')
 
     def set_breakpoint (self):
def shadowed_bkpt_cond_test(self):
         self.assertEqual(bkpt.location[0].GetIgnoreCount(), 10, "Location doesn't track top-level bkpt.")
 
         # Now make sure resetting the condition to "" resets the tracking:
         bkpt.location[0].SetCondition("")
         bkpt_new_cond = "1 == 3"
         bkpt.SetCondition(bkpt_new_cond)
         self.assertEqual(bkpt.location[0].GetCondition(), bkpt_new_cond, "Didn't go back to tracking condition")
 
+        # Test that set/get accessor methods on BreakpointLocation behave correctly.
+        bkpt_loc = bkpt.GetLocationAtIndex(0)
+
+        value = "MyQueue"
+        bkpt_loc.SetQueueName(value)
+        self.assertEqual(bkpt_loc.GetQueueName(), value,"Successfully set/get bp location QueueName")
+
+        value = 5
+        bkpt_loc.SetThreadID(value)
+        self.assertEqual(bkpt_loc.GetThreadID(), value,"Successfully set/get bp location ThreadID")
+
+        value = "1 == 0"
+        bkpt_loc.SetCondition(value)
+        self.assertEqual(bkpt_loc.GetCondition(), value,"Successfully set/get bp location Condition")
+
+        value = 6
+        bkpt_loc.SetThreadIndex(value)
+        self.assertEqual(bkpt_loc.GetThreadIndex(), value,"Successfully set/get bp location ThreadIndex")
+
+        value = "MyThread"
+        bkpt_loc.SetThreadName(value)
+        self.assertEqual(bkpt_loc.GetThreadName(), value,"Successfully set/get bp location ThreadName")
+
+        value = 5
+        bkpt_loc.SetIgnoreCount(value)
+        self.assertEqual(bkpt_loc.GetIgnoreCount(), value,"Successfully set/get bp location IgnoreCount")
+
+        for value in [True,False]:
+            bkpt_loc.SetAutoContinue(value)
+            self.assertEqual(bkpt_loc.GetAutoContinue(), value,"Successfully set/get bp location AutoContinue")
+
+        for value in [True,False]:
+            bkpt_loc.SetEnabled(value)
+            self.assertEqual(bkpt_loc.IsEnabled(), value,"Successfully set/get bp location SetEnabled")
+
+        # test set/get CommandLineCommands
+        set_cmds = lldb.SBStringList()
+        set_cmds.AppendString("frame var")
+        set_cmds.AppendString("bt")
+        bkpt_loc.SetCommandLineCommands(set_cmds)
+
+        get_cmds = lldb.SBStringList()
+        bkpt_loc.GetCommandLineCommands(get_cmds)
+        self.assertEqual(set_cmds.GetSize(), get_cmds.GetSize(), "Size of command line commands")
+        for idx, _ in  enumerate(set_cmds):
+            self.assertEqual(set_cmds.GetStringAtIndex(idx), get_cmds.GetStringAtIndex(idx), "Command %d"%(idx))
+
     def shadowed_bkpt_command_test(self):
         """Test that options set on the breakpoint and location behave correctly."""
         # Breakpoint option propagation from bkpt to loc used to be done the first time
         # a breakpoint location option was specifically set.  After that the other options
         # on that location would stop tracking the breakpoint.  That got fixed, and this test
         # makes sure only the option touched is affected.
 
         bkpt = self.set_breakpoint()