[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 ↗

(On Diff #448457)

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();