[lldb] Fix broken bad-address-breakpoint test

[hawkinsw]

Summary

After changing the "fallback" behavior when a user sets a breakpoint
without specifying a module the bad-address-breakpoint test case failed
incorrectly. This patch updates that test case in order to more
thoroughly discover an illegal address and use that as the means for
testing whether a breakpoint set at an illegal address fails to resolve.

[DavidSpickett]

lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py

38

I'm confused as to what the logic is here now.

You:

• read from address 0x0 to fill in error
• walk the memory regions until the highest one, making the end of that the illegal address
• assume that the error value from reading 0x0 is the same as you'd get from this new illegal address

...

Or am I missing something and the ptr was just left in accidentally.

(I would suggest you could jump to the last region in the list straight away but I don't think we actually require them to be sorted, plus sometimes you get multiple regions with the same base)

[DavidSpickett]

lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py

38

(same base but different end addresses I mean)

[hawkinsw]

lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py

38

Hello! Thank you for the review!

I was initializing illegal_address to 0 as a way to, well, initialize the value. I suppose that I could have initialized it to None and then used that a special value in the loop. In fact, now that I think about it, that's the tact that I will take in a second version of this patch.

But, yes, you basically have the rest correct: I walk through all the memory regions and find the highest address. Then, when I have that address (which I know is not in a valid memory region), I will use that as the address to set for the illegal breakpoint.

Does that make sense?

[hawkinsw]

lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py

38

And, woah, I now realize what you meant! So sorry!

Yes, ptr was leftover. I am so sorry!

Standby for the next version of this patch!

[hawkinsw]

Updating based on DavidSpickett helpful feedback.

[hawkinsw]

@DavidSpickett Thank you for the review! I hope that this 2nd version of the patch addresses your helpful comments!

Will

[DavidSpickett]

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

lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py

42

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

[hawkinsw]

@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!

lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py

42

Yes! I always make that error!

[JDevlieghere]

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.

[hawkinsw]

Updating the revision to consider feedback (!= None vs is not None)

[hawkinsw]

In D126109#3535931, @JDevlieghere wrote:

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.

Thanks @JDevlieghere ! I have updated the patch according to his feedback and I am running a full test on my desktop. I think that this is ready to land but I still haven't gotten myself in order and requested committer access so I will need your help again.

Thanks again for your patience and help getting me acclimated! Contributing is lots of fun!

lldb/test/API/functionalities/breakpoint/address_breakpoints/TestBadAddressBreakpoints.py

class BadAddressBreakpointTestCase(TestBase):
 
     def address_breakpoints(self):
         """Test that breakpoints set on a bad address say they are bad."""
         target, process, thread, bkpt = \
             lldbutil.run_to_source_breakpoint(self,
                                               "Set a breakpoint here",
                                               lldb.SBFileSpec("main.c"))
 
-        # Now see if we can read from 0.  If I can't do that, I don't
-        # have a good way to know what an illegal address is...
-        error = lldb.SBError()
 
-        ptr = process.ReadPointerFromMemory(0x0, error)
 
-        if not error.Success():
-            bkpt = target.BreakpointCreateByAddress(0x0)
+        # illegal_address will hold (optionally) an address that, if
+        # used as a breakpoint, will generate an unresolved breakpoint.
+        illegal_address = None
+
+        # Walk through all the memory regions in the process and
+        # find an address that is invalid.
+        regions = process.GetMemoryRegions()
+        for region_idx in range(regions.GetSize()):
+            region = lldb.SBMemoryRegionInfo()
+            regions.GetMemoryRegionAtIndex(region_idx, region)
+            if illegal_address == None or \
+                region.GetRegionEnd() > illegal_address:
+                illegal_address = region.GetRegionEnd()
+
+        if illegal_address is not None:
+            # Now, set a breakpoint at the address we know is illegal.
+            bkpt = target.BreakpointCreateByAddress(illegal_address)
+            # Verify that breakpoint is not resolved.
             for bp_loc in bkpt:
                 self.assertEquals(bp_loc.IsResolved(), False)
         else:
             self.fail(
                 "Could not find an illegal address at which to set a bad breakpoint.")