rocdbgapi: allow attaching with no forward progress requirement - #7657
rocdbgapi: allow attaching with no forward progress requirement#7657lancesix wants to merge 1 commit into
Conversation
|
Looks like debug agent tripped on build: |
c9914c8 to
e846754
Compare
| @@ -1078,7 +1078,7 @@ process_dbgapi_events (amd_dbgapi_process_id_t process_id, bool all_wavefronts, | |||
| /* TODO, we should have a RAII object to handle forward progress wave | |||
| creation mode override. */ | |||
| DBGAPI_CHECK (amd_dbgapi_process_set_progress ( | |||
| process_id, AMD_DBGAPI_PROGRESS_NO_FORWARD)); | |||
| process_id, AMD_DBGAPI_PROCESS_ATTACH_FLAGS_NO_FORWARD_PROGRESS)); | |||
There was a problem hiding this comment.
This change looks wrong -- AMD_DBGAPI_PROCESS_ATTACH_FLAGS_NO_FORWARD_PROGRESS is for the amd_dbgapi_process_attach function, but here we're still calling amd_dbgapi_process_set_progress, so the old code was right.
There was a problem hiding this comment.
yes, this is clearly wrong, should be AMD_DBGAPI_PROCESS_ATTACH_FLAGS_NONE .
There was a problem hiding this comment.
but this is a amd_dbgapi_process_set_progress call, not amd_dbgapi_process_attach.
| @@ -1253,7 +1253,7 @@ dbgapi_worker (int listen_fd, bool all_wavefronts, bool precise_memory, | |||
|
|
|||
| DBGAPI_CHECK (amd_dbgapi_process_attach ( | |||
| reinterpret_cast<amd_dbgapi_client_process_id_t> (&self_mem_fd), | |||
| &process_id)); | |||
| &process_id, PROCESS_ATTACH_FLAGS_NONE)); | |||
There was a problem hiding this comment.
Shouldn't this be AMD_DBGAPI_PROCESS_ATTACH_FLAGS_NONE?
| @@ -1944,6 +1950,17 @@ amd_dbgapi_status_t AMD_DBGAPI amd_dbgapi_process_get_info ( | |||
| amd_dbgapi_process_id_t process_id, amd_dbgapi_process_info_t query, | |||
| size_t value_size, void *value) AMD_DBGAPI_VERSION_0_80; | |||
|
|
|||
| /** Flags influencing the how the library attaches to a process. | |||
There was a problem hiding this comment.
"influencing the how the library" => "influencing how the library"
|
|
||
| return str; | ||
| } | ||
|
|
There was a problem hiding this comment.
Seeing yet another instance of this pattern made me want to refactor things so we can avoid it. See:
|
This pull request has been inactive for 25 days and will be marked as stale. If you would like to keep this PR open, please:
This PR will be automatically closed in 5 days if no further activity occurs. |
|
This pull request has been automatically closed due to inactivity (30 days with no updates). If you'd like to continue working on this, feel free to reopen the PR or create a new one. |
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
When attaching to a process, we currently implicitly have forward progress requirement disabled. As a consequence, during the attach sequence, we have a few suspend-restore cycles, which are not necessary. To fix this inefficiency, allow the client to attach with process requirement disabled. We could have made this the new default, but to maintain compatibility with existing debuggers, we make this behavior an opt-in and not a new default. Adjust the debug agent to use the updated API. Suggested-by: Laurent Morichetti <laurent.morichetti@amd.com>
e846754 to
cda12a5
Compare
4 participants
We could have made this the new default, but to maintain compatibility with existing debuggers, we make this behavior an opt-in and not a new default.
Suggested-by: Laurent Morichetti laurent.morichetti@amd.com
Motivation
When attaching to a process, we currently implicitly have forward progress requirement disabled. As a consequence, during the attach sequence, we have a few suspend-restore cycles, which are not necessary.
Technical Details
To fix this inefficiency, allow the client to attach with process requirement disabled.
Test Plan
Tested using ROCm/ROCgdb#182, as well as debug agent tests.
Test Result
Pass.
Submission Checklist