Opened 7 years ago

Closed 7 years ago

#9221 closed bug (fixed)

Breakpoints in dynamically loaded add-ons don't seem to be installed correctly

Reported by: anevilyak Owned by: anevilyak
Priority: normal Milestone: R1
Component: Applications/Debugger Version: R1/Development
Keywords: Cc: bonefish
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

If one sets a breakpoint in a function located in an add-on image, then exits the debugger and reloads the same app in it again, the breakpoint doesn't appear to get reinstalled correctly when the add-on is later loaded by the team. BreakpointManager::_UpdateBreakpoints() does get called appropriately on add-on load, and stepping through it indicates that the breakpoint was both found/created and successfully installed, but it simply doesn't seem to actually get hit during program execution.

Attachments (1)

0001-Fix-9221.patch (5.3 KB ) - added by anevilyak 7 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by anevilyak, 7 years ago

The instance where I saw this by the way, the breakpoint would hypothetically have been hit almost immediately after add-on load: http://cgit.haiku-os.org/haiku/tree/src/kits/opengl/GLRendererRoster.cpp#n198 (the breakpoint was in the constructor of one of the add-ons in question, and would have been called via instantiate).

This does make me wonder if it's possible that this is simply a race between us receiving the image load notification, loading debug information and installing the breakpoints vs the thread executing past where the breakpoint would've been before we make it that far. I'm a little unclear on when execution does/doesn't get suspended when these debug events come into play though.

in reply to:  1 ; comment:2 by bonefish, 7 years ago

Replying to anevilyak:

This does make me wonder if it's possible that this is simply a race between us receiving the image load notification, loading debug information and installing the breakpoints vs the thread executing past where the breakpoint would've been before we make it that far. I'm a little unclear on when execution does/doesn't get suspended when these debug events come into play though.

This is likely the cause. Have a look at how TeamDebugger::_HandleDebuggerMessage() handles B_DEBUGGER_MESSAGE_IMAGE_CREATED. After the LoadImageDebugInfoJob is scheduled the thread is continued immediately. If there's a breakpoint in this image, we'll have to keep the thread blocked until the image debug info has been loaded and the breakpoints have been installed.

in reply to:  2 comment:3 by anevilyak, 7 years ago

Owner: changed from bonefish to anevilyak
Status: newassigned

Replying to bonefish:

This is likely the cause. Have a look at how TeamDebugger::_HandleDebuggerMessage() handles B_DEBUGGER_MESSAGE_IMAGE_CREATED. After the LoadImageDebugInfoJob is scheduled the thread is continued immediately. If there's a breakpoint in this image, we'll have to keep the thread blocked until the image debug info has been loaded and the breakpoints have been installed.

Aha...will dig into that, thanks!

comment:4 by anevilyak, 7 years ago

Status: assignedin-progress

by anevilyak, 7 years ago

Attachment: 0001-Fix-9221.patch added

comment:5 by anevilyak, 7 years ago

Has a Patch: set

comment:6 by anevilyak, 7 years ago

Cc: bonefish added

Attached a proposed fix. If that looks reasonable, let me know and I'll commit.

comment:7 by bonefish, 7 years ago

The patch looks OK, save for a few cases of somewhat unusual formatting. Neither for TeamDebugger::ImageInfoPendingThread::fNext nor for TeamDebugger::ImageInfoPendingThreadHashDefinition::ValueType there's a fixed column to be respected, so there really isn't any need to break them. If you want to align it with the other variables/type just indent those more. In case of TeamDebugger::fImageInfoPendingThreads there is a particular column to be respected, but if that isn't possible, the common solution is to keep the variable on the same line as the type with only a single space in between. If that doesn't fit the 80 columns line limit, the line can be broken (I'd indent the continuing line one additional level).

in reply to:  7 comment:8 by anevilyak, 7 years ago

Resolution: fixed
Status: in-progressclosed

Thanks for looking it over! Applied with suggested changes in hrev44927.

Note: See TracTickets for help on using tickets.