From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 383A3382CE0C for ; Wed, 7 Dec 2022 02:17:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 383A3382CE0C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 443D61E0CB; Tue, 6 Dec 2022 21:17:26 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1670379446; bh=MQOxzaD+Zzj8OujhUhhhPXJmeADhJOE2+b/KOcfxQKQ=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=DBWgPDS3MdV2XeXODx4CybkyMQUvNARakhpZKl3q/G2NBQGkVoZLiH3F4EVObrkmV HRQzbsAvKbsfC0d1UL5pQx4347W+foKDbnGv7nTLEBxYa5V+JMmBm7MgzI01x1ppbY y4SVRuN4J27fMVo9/8gkox9BQtevAI9VLHZ2jqXw= Message-ID: Date: Tue, 6 Dec 2022 21:17:25 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH 12/12] gdb: initial support for ROCm platform (AMDGPU) debugging Content-Language: en-US To: Eli Zaretskii , Simon Marchi Cc: gdb-patches@sourceware.org, zoran.zaric@amd.com, laurent.morichetti@amd.com, Tony.Tye@amd.com, lancelot.six@amd.com, pedro@palves.net References: <20221206135729.3937767-1-simon.marchi@efficios.com> <20221206135729.3937767-13-simon.marchi@efficios.com> <835yeo7d3i.fsf@gnu.org> From: Simon Marchi In-Reply-To: <835yeo7d3i.fsf@gnu.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: >> --- 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