Re: [PATCH 12/12] gdb: initial support for ROCm platform (AMDGPU) debugging

[Simon Marchi <simark@simark.ca>]

>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -7021,6 +7021,8 @@ signal happened.  @value{GDBN} alerts you to the context switch with a
>>  message such as @samp{[Switching to Thread @var{n}]} to identify the
>>  thread.  
>>  
>> +@node set scheduler-locking
> 
> This @node is without any @chapter/@section, and does not appear in any
> @menu.  That doesn't look right; did you actually succeed in building the
> manual with these changes?
Yes, it builds.  It is used as a destination for a @pxref{set
scheduler-locking} later in the patch, if I remove it doesn't build:

    gdb.texinfo:26469: @pxref reference to nonexistent node `set scheduler-locking'

But I see the problem, in the HTML it introduces a new page starting at
that point, we don't want that.  Perhaps we should use @anchor instead?

>> +The following @acronym{AMD GPU} architectures are supported:
>> +
>> +@table @emph
>> +
>> +@item @samp{gfx900}
>> +AMD Vega 10 devices, displayed as @samp{vega10} by @value{GDBN}.
>> +
>> +@item @samp{gfx906}
>> +AMD Vega 7nm devices, displayed as @samp{vega20} by @value{GDBN}.
> 
> Do we really need this long list of architectures in the GDB manual?  It
> sounds like an ad for AMD...

We found it useful, as people often ask which devices / models the
debugger supports.  AMD produces a lot of GPU models.  A subset of that
can run ROCm programs.  And a subset of that support debugging.  I think
it's useful to tell users which devices GDB is expected to work with.

And it would not be a very good ad either, as most of these devices are
far from the latest and greatest :).

>> +@smallexample
>> +hipcc -O0 -g --offload-arch=gfx900 --offload-arch=gfx906 bit_extract.cpp -o bit_extract
>> +@end smallexample
> 
> This and other @smallexamples in the patch have too long lines; please break
> them into two or more, to avoid problems with the printed format of the
> manual.

What is the maximum line length for this?

I'll try, but it's a bit difficult when quoting actual GDB output.  For
instance, how would you do this one?

@smallexample
(@value{GDBP}) info sharedlibrary
From                To                  Syms Read   Shared Object Library
0x00007fd120664ac0  0x00007fd120682790  Yes (*)     /lib64/ld-linux-x86-64.so.2
...
0x00007fd0125d8ec0  0x00007fd015f21630  Yes (*)     /opt/rocm-3.5.0/hip/lib/../../lib/libamd_comgr.so
0x00007fd11d74e870  0x00007fd11d75a868  Yes (*)     /lib/x86_64-linux-gnu/libtinfo.so.5
0x00007fd11d001000  0x00007fd11d00173c  Yes         file:///home/rocm/examples/bit_extract#offset=6477&size=10832
0x00007fd11d008000  0x00007fd11d00adc0  Yes (*)     memory://95557/mem#offset=0x7fd0083e7f60&size=41416
(*): Shared library is missing debugging information.
(@value{GDBP})
@end smallexample

> 
>> +@subsubsection @acronym{AMD GPU} Wavefronts
> 
> I think a @cindex about wavefronts would be useful here.

I just add a `@cindex Wavefronts` under the line quoted above?

> 
>> +@item file_path
>> +The file's path specified as a URI encoded UTF-8 string.  In URI
>> +encoding, every character that is not:
>> +
>> +@itemize
>> +@item In the @samp{a-z}, @samp{A-Z}, @samp{0-9} ranges
>> +@item @samp{/}, @samp{_}, @samp{.}, @samp{~} or @samp{-}
>> +@end itemize
>> +
>> +is encoded as two uppercase hexadecimal digits proceeded by @samp{%}.
> 
> You want @noindent before the last line.

I can add it, but I don't see the difference (at least in the HTML
and PDF outputs).

>> +Directories in the path are separated by @samp{/}.
> 
> GNU coding standards frown on using "path" for anything that is not
> PATH-style lists of directories.  Please use "file name" instead.

Done.

>> +The @acronym{AMD GPU} entities have the following target identifier formats:
>> +
>> +@table @var
> 
> @var is wrong here, since the @item text is not a meta-syntactic variable:
> it doesn't stand for something else.  I'd use @asis instead.

Done.

>> +Make sure the container user is a member of the @var{render} group for
>> +Ubuntu 20.04 onward and the @var{video} group for all other
> 
> ??? Why are we talking about specific Linux distros in the manual?

Hmm, indeed, that probably doesn't belong here.  The downstream ROCm GDB
manual contains a lot of information about how it integrates in the rest
of the ROCm ecosystem.  Some of this information is probably not
relevant to upstream GDB, where we should stick to information
specifically about using GDB itself.  I removed a bunch of it already,
but it looks like there are still things we should remove.

> Finally: maybe it's just me, but isn't this documentation way too detailed?
> It weighs in at 800 lines, and includes many details that seem to be more
> related to AMD, GPU, and HIP than to GDB.  Would it be reasonable to make
> this section shorter by omitting too low-level and unrelated details?

I went over the page, and while I agree it's very thorough and detailed,
my impression is that it's all information that is one way or another
related to how GDB interacts with ROCm / HIP / AMD GPUs.  So, all
information that could be useful to someone with good knowledge of ROCm
/ HIP / AMD GPUs, if they wanted to use GDB to debug their program.  For
instance, the description of when GDB reports a SIGABRT is useful, as
the mapping between target debug events and Unix signals in GDB is kind
of arbitrary.

If you can point out specific parts that you think are not relevant, we
can discuss them specifically.

Simon