Skip to content

gdb.rocm/hip-builtin-completions: consider multi choices for completions - #284

Merged
amd-shahab merged 1 commit into
amd-stagingfrom
users/shvahedi/hip-completion-narrow
Aug 20, 2026
Merged

gdb.rocm/hip-builtin-completions: consider multi choices for completions#284
amd-shahab merged 1 commit into
amd-stagingfrom
users/shvahedi/hip-completion-narrow

Conversation

@amd-shahab

@amd-shahab amd-shahab commented Aug 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor 
Current implementation of the test assumes one "suggestion" per
completion.  This can go awry if there are symbols that have
commonality in their names with the symbols that the test cares
about, like "threadIdx" and "threadIndex" [1].

This change adjust the checks as such to look for the desired
symbol name in the output of "complete" command, assuming that
there can be one or more entries.

[1]
Due to the missing step of stripping ".symtab" from libLLVM.so, a
"threadIndex" variable comes into GDB's scope.  This collides with
the expected tab-completion of "threadI" to become "threadIdx".
The symbol from libLLVM.so is loaded through these shared objects
dependencies:

  test
    libamdhip64.so
      libamd_comgr.so
        libclang-cpp.so
          libLLVM.so

Bug: LCOMPILER-2585

@amd-shahab amd-shahab self-assigned this Aug 17, 2026
@amd-shahab
amd-shahab requested a review from a team as a code owner August 17, 2026 20:45
@amd-shahab amd-shahab changed the title gdb.rocm/hip-builtin-completions: narrow down some tests gdb.rocm/hip-builtin-completions: narrow down some checks Aug 17, 2026
@amd-shahab
amd-shahab force-pushed the users/shvahedi/hip-completion-narrow branch from dee67b7 to d0094b6 Compare August 17, 2026 20:50
lancesix
lancesix reviewed Aug 17, 2026
View reviewed changes
Comment thread gdb/testsuite/gdb.rocm/hip-builtin-completions.exp Outdated
# Look for completion of "threadId" into "threadIdx".
# Unlike the above tests, this one is narrower, becasue it can
# collide with libLLVM's parallel::threadIndex
# (llvm/lib/Support/Parallel.cpp).

@lancesix lancesix Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we discuss having the test check that print threadIdx was one of the proposed completion (and implicitly accept having other symbols show up)?

Also, it is really not clear to someone without context why libLLVM would come here. Might be worth mentionning that this is a rocr -> comgr -> libllvm dependency.

@lumachad lumachad Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

becasue -> because

I don't think we should mention the build/llvm bug here. We're just protecting ourselves from potential deviations from symbol availability.

@amd-shahab amd-shahab Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should mention the build/llvm bug here. We're just protecting ourselves from potential deviations from symbol availability.

With this "solution", the bug is the sole reason of this change. We're not protecting ourselves from future issues this way. "Narrow"ing down is not the way to go. What if tomorrow there's another variable added, this time called threadIdintity!?

@amd-shahab

amd-shahab commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Didn't we discuss having the test check that print threadIdx was one of the proposed completion (and implicitly accept having other symbols show up)?

We did, but I don't know how to do it. Otherwise, it could be a final solution (not being affected by the environment).

UPDATE: Now it is done as we've discussed.

@ronlieb
ronlieb self-requested a review August 18, 2026 15:45
@amd-shahab
amd-shahab force-pushed the users/shvahedi/hip-completion-narrow branch from d0094b6 to 1cbc00b Compare August 19, 2026 12:39
@amd-shahab

amd-shahab commented Aug 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author
  • Use gdb_test_multiple to look for our symbol-of-interest amongst a likely multi-line suggestion.
  • Update the commit message to reflect the latest changes and the shared object dependencies.

@amd-shahab amd-shahab changed the title gdb.rocm/hip-builtin-completions: narrow down some checks gdb.rocm/hip-builtin-completions: consider multi choices for completions Aug 19, 2026
@amd-shahab
amd-shahab requested a review from lancesix August 19, 2026 12:42
@amd-shahab
amd-shahab force-pushed the users/shvahedi/hip-completion-narrow branch from 1cbc00b to b42e30f Compare August 19, 2026 12:45
@amd-shahab
gdb.rocm/hip-builtin-completions: consider multi choices for completions
bd29607 
Current implementation of the test assumes one "suggestion" per
completion.  This can go awry if there are symbols that have
commonality in their names with the symbols that the test cares
about, like "threadIdx" and "threadIndex" [1].

This change adjust the checks as such to look for the desired
symbol name in the output of "complete" command, assuming that
there can be one or more entries.

[1]
Due to the missing step of stripping ".symtab" from libLLVM.so, a
"threadIndex" variable comes into GDB's scope.  This collides with
the expected tab-completion of "threadI" to become "threadIdx".
The symbol from libLLVM.so is loaded through these shared objects
dependencies:

  test
    libamdhip64.so
      libamd_comgr.so
        libclang-cpp.so
          libLLVM.so

Bug: LCOMPILER-2585
@amd-shahab
amd-shahab force-pushed the users/shvahedi/hip-completion-narrow branch from b42e30f to bd29607 Compare August 19, 2026 12:47
lancesix
lancesix approved these changes Aug 20, 2026
View reviewed changes

@lancesix lancesix left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@amd-shahab
amd-shahab merged commit b5151b0 into amd-staging Aug 20, 2026
9 checks passed
@amd-shahab
amd-shahab deleted the users/shvahedi/hip-completion-narrow branch August 20, 2026 13:15
@ronlieb

ronlieb commented Aug 21, 2026

Copy link
Copy Markdown

@amd-shahab has this been CP'ed to develop ?

@ronlieb
ronlieb requested a review from lajagapp August 21, 2026 10:41
@lumachad

lumachad commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

@amd-shahab has this been CP'ed to develop ?

No. It needs cherry-picking to amd-staging-rocgdb-* first then a rocgdb bump to reach TheRock main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lancesix lancesix lancesix approved these changes
@lumachad lumachad Awaiting requested review from lumachad
@ronlieb ronlieb Awaiting requested review from ronlieb
@lajagapp lajagapp Awaiting requested review from lajagapp

Assignees

@amd-shahab amd-shahab

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@amd-shahab @ronlieb @lumachad @lancesix