-
Notifications
You must be signed in to change notification settings - Fork 353
[BoundsSafety][LLDB] Implement instrumentation plugin for -fbounds-safety soft traps #11835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: swift/release/6.3
Are you sure you want to change the base?
[BoundsSafety][LLDB] Implement instrumentation plugin for -fbounds-safety soft traps #11835
Conversation
f7206d3 to
a15e413
Compare
|
@swift-ci test |
|
@swift-ci test llvm |
a15e413 to
c8af09b
Compare
|
@swift-ci test |
|
@swift-ci test llvm |
|
Windows failure looks completely unrelated to this PR: likely caused by swiftlang/swift-docc#1331. Looks like swiftlang/swift-docc#1352 is out to revert this change. |
|
@swift-ci please test windows platform |
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.h
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
lldb/test/API/lang/BoundsSafety/soft_trap/TestBoundsSafetyInstrumentationPlugin.py
Outdated
Show resolved
Hide resolved
lldb/test/API/lang/BoundsSafety/soft_trap/TestBoundsSafetyInstrumentationPlugin.py
Outdated
Show resolved
Hide resolved
lldb/test/API/lang/BoundsSafety/soft_trap/TestBoundsSafetyInstrumentationPlugin.py
Outdated
Show resolved
Hide resolved
This patch adds `LLDBLog::InstrumentationRuntime` as a log channel to provide an appropriate channel for instrumentation runtime plugins as previously one did not exist. A small use of the channel is added to illustrate its use. The logging added is not intended to be comprehensive. This is primarily motivated by an `-fbounds-safety` instrumentation plugin (swiftlang#11835). rdar://164920875
This patch adds `LLDBLog::InstrumentationRuntime` as a log channel to provide an appropriate channel for instrumentation runtime plugins as previously one did not exist. A small use of the channel is added to illustrate its use. The logging added is not intended to be comprehensive. This is primarily motivated by an `-fbounds-safety` instrumentation plugin (swiftlang#11835). rdar://164920875
This patch adds `LLDBLog::InstrumentationRuntime` as a log channel to provide an appropriate channel for instrumentation runtime plugins as previously one did not exist. A small use of the channel is added to illustrate its use. The logging added is not intended to be comprehensive. This is primarily motivated by an `-fbounds-safety` instrumentation plugin (swiftlang#11835). rdar://164920875
This patch adds `LLDBLog::InstrumentationRuntime` as a log channel to provide an appropriate channel for instrumentation runtime plugins as previously one did not exist. A small use of the channel is added to illustrate its use. The logging added is not intended to be comprehensive. This is primarily motivated by an `-fbounds-safety` instrumentation plugin (swiftlang#11835). rdar://164920875
This patch adds `LLDBLog::InstrumentationRuntime` as a log channel to provide an appropriate channel for instrumentation runtime plugins as previously one did not exist. A small use of the channel is added to illustrate its use. The logging added is not intended to be comprehensive. This is primarily motivated by an `-fbounds-safety` instrumentation plugin (swiftlang#11835). rdar://164920875 (cherry picked from commit 46565f3)
This refactors the soft trap runtime test so the soft trap runtime implementation exists in its own file. This has several advantages: * It let's the runtime be built without debug info which will be the common case uses hit * It prevents the risk of infinite recursion because it isn't safe to build the soft trap runtime with -fbounds-safety soft trap mode enabled.
…(#168508) This patch adds `LLDBLog::InstrumentationRuntime` as a log channel to provide an appropriate channel for instrumentation runtime plugins as previously one did not exist. A small use of the channel is added to illustrate its use. The logging added is not intended to be comprehensive. This is primarily motivated by an `-fbounds-safety` instrumentation plugin (swiftlang/llvm-project#11835). rdar://164920875
3c3ccb0 to
d191072
Compare
|
Changing target from |
|
@swift-ci test |
|
@jimingham @Michael137 This is ready for another round of reviews. |
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
|
There's one obsolete TODO about adding an Instrumentation Log Channel which you in fact already did. You probably do want to log the breakpoint having no locations around where the TODO is. Other than that, LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from some nits this LGTM also
High-level question though. Why couldn't the existing verbose-trap frame recognizer be made to understand the soft trap frame? Couldn't we have just added it to the recognized regex and then re-used the existing frame recognizer to display the stop reason etc.? Is it the no-debug-info cases that are special and need the separate plugin? Makes me wonder if we could still re-use the verbose trap recognizer for the debug-info case. But might be easier said than done/might missing something completely
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Show resolved
Hide resolved
lldb/source/Plugins/InstrumentationRuntime/BoundsSafety/InstrumentationRuntimeBoundsSafety.cpp
Outdated
Show resolved
Hide resolved
Good question. Technically we could extend the verbose-trap frame recognizer too but something this plugin does that the verbose trap frame recognizer doesn't do is set a breakpoint. With this plugin we automatically stop on a soft trap which is the behavior I wanted. |
Oh i see, yea we would need to stop from the frame recognizer to trigger. Technically we could make it stop at the parent frame, and then the frame recognizer kicks in? And that could be responsible for showing the stop reason? Then the frame recognizer benefits from working in the no-debug-info case too. But i'm not 100% whether this Just Works. Might be worth a try. Or if you prefer, we can merge this and then simplify/merge with the frame recognizer as a follow-up. |
The logic for getting the stop reason for
Pulling all that logic into the existing verbose stackframe recognizer doesn't make sense to me because Given that instrumentation plugins supports the exact thing I need (set a breakpoint and when its hit do some analysis and set the stop reason) I think it's cleaner to keep this PR as is. The common logic for getting the trap reason from the debug info has already been refactored so that both this PR and the verbose stackframe recognizer can use it so there's no unnecessary duplication of code.
The only use case I see for that is if the plugin was deliberately switched off and then a breakpoint is set manually. Currently the stop reason won't be set to the trap reason. I'm not sure if that's desirable or not. If we change the stop reason it's less clear that the user's breakpoint was hit, OTOH it's harder to see the trap reason. I doubt this is a common use case because the user would need to known that the plugin exists, and how to turn it off.
I would prefer to just merge. I've file a radar (rdar://165066642) about considering moving some of the logic into a stackframe recognizer. |
…fety soft traps This patch implements an instrumentation plugin for the `-fbounds-safety` soft trap mode first implemented in swiftlang#11645 (rdar://158088757). The current implementation of -fbounds-safety traps works by emitting calls to runtime functions intended to log the occurrence of a soft trap. While the user could just set a breakpoint of these functions the instrumentation plugin sets it automatically and provides several additional features: When debug info is available: * It adjusts the stop reason to be the reason for trapping. This is extracted from the artificial frame in the debug info (similar to -fbounds-safety hard traps). * It adjusts the selected frame to be the frame where the soft trap occurred. When debug info is not available: * For the `call-with-str` soft trap mode the soft trap reason is read from the first argument register. * For the `call-minimal` soft trap mode the stop reason is adjusted to note its a bounds check failure but does not give further information because none is available. * In this situation the selected frame is not adjusted because in this mode the user will be looking at assembly and adjusting the frame makes things confusing. This patch includes shell and api tests. The shell tests seemed like the best way to test behavior when debug info is missing because those tests make it easy to disable building with debug info completely. rdar://163230807
a1e85fe to
795052d
Compare
|
@swift-ci test |
|
I've squash the feedback commits into the relevant commit and I've kicked off PR testing. |
Ok that makes sense. I naively thought we'd be able to share the logic which recognizes the stop reason in the no-debug-info case. But sounds like for
Sounds good to me! |
|
@swift-ci test windows platform |
This patch implements an instrumentation plugin for the
-fbounds-safetysoft trap mode first implemented in#11645 (rdar://158088757).
The current implementation of -fbounds-safety traps works by emitting
calls to runtime functions intended to log the occurrence of a soft trap.
While the user could just set a breakpoint of these functions the
instrumentation plugin sets it automatically and provides several
additional features:
When debug info is available:
extracted from the artificial frame in the debug info (similar to
-fbounds-safety hard traps).
occurred.
When debug info is not available:
call-with-strsoft trap mode the soft trap reason isread from the first argument register.
call-minimalsoft trap mode the stop reason is adjustedto note its a bounds check failure but does not give further
information because none is available.
this mode the user will be looking at assembly and adjusting the
frame makes things confusing.
This patch includes shell and api tests. The shell tests seemed like the
best way to test behavior when debug info is missing because those tests
make it easy to disable building with debug info completely.
rdar://163230807