Skip to content
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

rocmPackages.rocgdb: Ensure build with AMDPGU support, plus various improvements #309654

Merged
merged 2 commits into from
May 22, 2024

Conversation

lsix
Copy link
Member

@lsix lsix commented May 6, 2024

Description of changes

The current ROCgdb project is built without AMDGPU support. The current patch mostly aim at fixing this, plus other various improvements:

  • Review dependencies ROCgdb is built against (I mostly use those recommended here)
  • Add the --program-prefix configure option so the gdb is installed as rocgdb.
  • The source repo contains GDB + various other GNU toolchain projects (binutils. ,,,). This package is only interested in installing ROCgdb, so I use make install-gdb to avoid installing other unrelated components.
  • Disable building many components that we do not need (binutils, as, ld), or which are not compatible with the amdgpu target (gdbserver, gprofng, ...).
  • Those patches also adjust the ROCgdb license to GPLv3+.

I have tested the resulting result/bin/rocgdb against a gfx1031 card.

I am aware there are quite a few changes here, I am happy splitting those patch further if needed.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

lsix added 2 commits May 6, 2024 22:10
The currnt rocgdb package is built without amdgpu uspport (which
requires the rocdbgapi library).  Fix this, and do other improvements
over the default configuration.
Fix the lincense information for ROCgdb.  The appropriate license is
GPL3 or above.
@mschwaig
Copy link
Member

mschwaig commented May 6, 2024

Thanks for the PR.

I would appreciate if someone who is a user of / knows more about rocgdb could also review this, but I from my side I am inclined to approve this PR once I understand the changed in license information better.

build results

Result of nixpkgs-review pr 309654 run on x86_64-linux 1

1 package built:
  • rocmPackages.rocgdb (rocmPackages_6.rocgdb)

code review

I am not a user of rocgdb, so I cannot really evaluate the merit of these changes, but I am inclined to trust you on that.
The code itself especially together with the docs linked above about dependencies, looks totally fine.

I cannot really tell why rocgdb depends on mpfr and sourceHighlight, on the other hand those packages seem relatively harmless, so I am unsure if the question is worth asking.

Some due diligence questions that come to mind would be:
Are you aware of any functionality we are losing by not building other components?
What functionality does the AMDGPU target for GDB that gets added here enable?

About the license information

I see that you have corrected the license for the package to GPLv3+.

  • Those patches also adjust the ROCgdb license to GPLv3+.

What do you mean by that?

Is the applicable license GPLv3+, because that is how GDB and the extensions by AMD are licensed, and the GPLv2 and BSD3 licenses are irrelevant because they apply to other components that live in the same repo, but are not built and therefore are not part of the build output?

@lsix
Copy link
Member Author

lsix commented May 7, 2024

Hi, thanks for the review.

I cannot really tell why rocgdb depends on mpfr and sourceHighlight, on the other hand those packages seem relatively harmless, so I am unsure if the question is worth asking.

Those are valid questions! Here are some elements of response:

  • Regarding `mprf, GDB can use that when evaluating user expression. This is not a strict requirement of rocgdb-6.0 (based on GDB-13), it is just an optional dependency at this point. However, starting rocgdb-6.1 (based on GDB-14) mpfr became a required dependency, so I have added it to help future upgrades. I could remove that part from the patch / move it to a dedicated patch if you prefer.
  • sourceHighlight is an optional dependency GDB can used to do syntax highlighting when showing source code. Similarly, it can be moved to a dedicated patch.

Some due diligence questions that come to mind would be:
Are you aware of any functionality we are losing by not building other components?

Basically: none. The other components that would be installed are usual GNU toolchain components (the ld linker, the as assembler, various binutils such as strings objcopy objdump, profiling tools such as gprofng…). They happen to live in the same source repository as GDB, so they are present in the ROCgdb downstream tree, but are not used in the ROCm toolchain. If required, all of those tools can be available via dedicated packages. The current package for rocgdb contains:

ls /nix/store/xpfdz5lw0x9bhwzi8v58605y9gyvl6qr-rocgdb-6.0.2/bin/
addr2line  gcore          gp-collect-app   gprofng  objdump  strip
ar         gdb            gp-display-html  ld       ranlib
as         gdb-add-index  gp-display-src   ld.bfd   readelf
c++filt    gdbserver      gp-display-text  nm       size
elfedit    gp-archive     gprof            objcopy  strings

Most of those tools can’t really deal with rocm object codes anyway (no disassembler support for amdgcn), one would need to use lld, llvm-objdump, llvm-dwarfdump to handle the binaries. So 1) those are not "GDB tools" and 2) do not provide much for rocm debugging.

The last element to discuss would be gdbserver. This is a GDB related tool used for remote debugging, but as of today it is not able to do GPU debugging. That might come in the future, but is not supported yet.

What functionality does the AMDGPU target for GDB that gets added here enable?

It allows to debug code running on AMDGPU devices. Without that, ROCgdb can only debug code running on the CPU, so it is basically pkgs.gdb.

About the license information

I see that you have corrected the license for the package to GPLv3+.

  Those patches also adjust the ROCgdb license to GPLv3+.

What do you mean by that?

Is the applicable license GPLv3+, because that is how GDB and the extensions by AMD are licensed, and the GPLv2 and BSD3 > licenses are irrelevant because they apply to other components that live in the same repo, but are not built and therefore are not > part of the build output?

I am not 100% of the licensing for all the code living in the binutils-gdb repo, but the GDB part (as well as the AMD extensions) are GPL3 or above:

$ ~/nixpkgs/result/bin/rocgdb -version
GNU gdb (GDB) 13.2
Copyright (C) 2023 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

This is consistent with the licensing information for pkgs.gdb (and pkgs.binutils):

Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

Only glanced over the changes (never used rocgdb myself). That looks good to me

@lsix lsix merged commit 41b39e4 into NixOS:master May 22, 2024
28 checks passed
@lsix lsix deleted the fix-rocgdb branch May 22, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants