Repository navigation
Conversation
|
Please feel free to add other reviewers that might have an interest in this particular change, or that have more time to review it. |
geomin12
left a comment
There was a problem hiding this comment.
lgtm, but i don't work on build system often. definitely would need a second set of eyes from @HereThereBeDragons or @amd-shiraz
Thanks for the review. I'll wait for a second opinion. |
d840399 to
89ec84c
Compare
geomin12
left a comment
There was a problem hiding this comment.
lgtm, but again, review from someone else would be lovely as I don't work on build often :)
89ec84c to
2d1fb7d
Compare
|
Rebased and fixed conflict. |
marbre
left a comment
There was a problem hiding this comment.
Left some initial comments. The CMake configuration needs a closer look though.
| # See if the compiler knows where libbacktrace.a is. It is worth mentioning | ||
| # that both gcc and llvm have the same behavior when --print-file-name is used | ||
| # but the compiler cannot locate the file anywhere. They output the filename | ||
| # passed as argument to --print-file-name unchanged. | ||
| # | ||
| # This makes it so the libbacktrace_check below will always pass. Though | ||
| # incorrect, this is somewhat harmless as we will call find_path and | ||
| # find_library later and we could end up finding includes and library | ||
| # depending on the system environment variables. | ||
| execute_process( | ||
| COMMAND ${CMAKE_CXX_COMPILER} ${CMAKE_CXX_FLAGS} --print-file-name libbacktrace.a | ||
| RESULT_VARIABLE libbacktrace_check | ||
| OUTPUT_VARIABLE libbacktrace_path | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ) | ||
| message(STATUS "[libbacktrace check] Command output: ${libbacktrace_path}") |
There was a problem hiding this comment.
We avoid calling the compiler in the configure phase. If libbacktrace_check passes always, what's the value here? Can we just drop it?
There was a problem hiding this comment.
So this cmake configuration is actually a patched up original cmake configuration from amd-dbgapi. It contains two main paths, one for building the library standalone and another to build it within TheRock.
I've tried to share as much code as possible without introducing hardcoded assumptions. Since the amd-dbgapi status as a submodule is still uncertain, further cleanups will depend on that decision.
The particular section you marked is only executed as a standalone build. We fetch the libbacktrace dependency straight from TheRock's dependencies via find_package(libbacktrace CONFIG).
There was a problem hiding this comment.
As @lumachad highlights, this part is not meant to be used when compiling with TheRock. dbgapi still needs to be buildable outside of TheRock. All of those changes will end-up in the main dbgapi CMakeLists, so better minimise the changes.
Nevertheless, this part is not meant to build anything that will end-up in dbgapi, we just poke at the compiler configuration, which is absolutely fine during the configure phase. Similarly, below we do have some calls to check_cxx_source_compiles which DO compile stuff, and configure phase is when that must be done.
| PATHS | ||
| /opt/rocm/ |
There was a problem hiding this comment.
Please don't point to /opt/rocm/ as what TheRock produces never goes there and with this you might accidentally pick up something from a previously installed ROCm.
There was a problem hiding this comment.
This is another case of keeping compatibility with a standalone build. Within TheRock find_package gets overriden, so this hint will have no effect. Please correct me if I'm mistaken here.
2d1fb7d to
3e212a1
Compare
|
Updated based on Marius' suggestions. |
HereThereBeDragons
left a comment
There was a problem hiding this comment.
as marius said, the cmake needs really a bit more looking over. so i am commenting already now to get a few questions answered.
btw i was just looking for the old pr and saw that you edited it a lot and then only merged the libbacktrace additions. i think for the future it would be nice if you have big pr that people ask you to split, to close the big pr to keep the history and make new prs with the smaller changes. so one has a chance to see what said before :)
|
|
||
| cmake_minimum_required(VERSION 3.8) | ||
|
|
||
| project(amd-dbgapi VERSION 0.78.0) |
There was a problem hiding this comment.
i guess this needs to be bumped manually each time the version in the submodule changes?
There was a problem hiding this comment.
For now at least. The tool has managed the version manually so far. This comes straight from the original cmake here: https://github.com/ROCm/ROCdbgapi/blob/amd-staging/CMakeLists.txt
There was a problem hiding this comment.
I expect in the future we will try to manage it non-manually.
There was a problem hiding this comment.
has there anything changed compared to last time i reviewed?
and is there any easy way to see what changed comparing this "therock" version vs the original version? if there are even comparable depending on the amount of changes
There was a problem hiding this comment.
Yes, there were changes, although not any critical ones. I've addressed Marius' comments and also tweaked a few minor things based on build tests.
For reference, here is the original cmake configuration for the project: https://github.com/ROCm/ROCdbgapi/blob/amd-staging/CMakeLists.txt
I've taken that one and adapted it to keep it buildable both standalone and within TheRock. You'll notice two slightly different paths in the configuration, but I've tried to share as much code as possible between those paths.
As for the changes, I've removed all the CPACK packaging code you will see in the original. We will no longer need those moving forward. I've also moved the setting of options.
Feel free to send me a message if you need clarification anywhere.
Note taken. Sorry for the confusion, I'll do that next time. |
|
Since the addition of debugger tools to TheRock is more involved in terms of having to add multiple dependencies on top of the three projects, I think it would be good if somebody from the debugger tools team also takes a pass at reviewing these pull requests. Have more than one person within that team be familiar with the work done here is good long-term. If future support is required, then we are not reliant on a single person who might be on vacation. |
3e212a1 to
c0ca9b1
Compare
|
PR updated based on previous comments. |
c0ca9b1 to
d05a748
Compare
|
I spotted a mistake where I included the debug/amd-dbgapi directory in the patch. That should be populated by the submodule and not by the patch. Fixed now and updated the PR. |
d05a748 to
7bae0c9
Compare
* Motivation Add the debug-tools feature that will serve as the home to all debug tools projects we have: ROCdbgapi, ROCgdb and ROCR-debug-agent. This commit adds the feature along with ROCdbgapi. Please note this is restricted to Linux builds for now. Windows builds will come at a later stage. * Technical Details This commit creates the minimal set of changes required to support the ROCm debug tools in TheRock build platform. It adds the following: - New debug-tools features/subdirectory (updated main CMakeLists.txt) - Adds ROCdbgapi as a submodule using github's amd-staging branch. - Adds the main debug-tools/CMakeLists.txt file with instructions on how to build the debug tools. At the moment this is just ROCdbgapi, named amd-dbgapi. - Updates build_tools/fetch_sources.py - Updates the documentation. - Adds ROCdbgapi artifacts. NOTE 1: ROCdbgapi will be added as a submodule of TheRock only temporarily until a decision is made on the topic. It may be the case we will want to migrate it to rocm-systems. But adding it as a submodule now allows us to integrate the component into TheRock without having to wait for all the rocm-systems migration steps to take place. NOTE 2: To make integration and testing faster, we have a local amd-dbgapi.cmake file that gets copied into the source directory before builds. Later on these cmake changes will be integrated into ROCdbgapi and the local file will be removed. * Test Plan - Validate that ROCdbgapi builds and pulls its libbacktrace dependency. - Validate that ROCdbgapi's shared object exists and matches the name. * Test Result All tests passed.
7bae0c9 to
0fe88de
Compare
|
@lumachad thanks for the all the work. |
|
@HereThereBeDragons I did and I asked others from our team to take a look. Unfortunately at this time the folks that can look into it are off for the end of the year. What exactly do want them to look into? The cmake changes? |
|
FYI, I'm pursuing integration of the cmake changes to amd-dbgapi. That way debug-tools/amd-dbgapi.cmake will disappear from this PR eventually. |
| type=str, | ||
| default=[ | ||
| "amd-dbgapi", | ||
| ], |
There was a problem hiding this comment.
I guess you might also want
choices=["amd-dbgapi"]so argparse knows the list of valid values (will be extended later to also cover debug-agent and gdb).
There was a problem hiding this comment.
I'm not sure about that. My original intent was to have --debug-tools pull in sources for all 3 components (amd-dbgapi, rocgdb and debug agent) and not accept individual options.
Is there a particular reason why you think that would be useful?
|
|
||
| # Profiler Trace Decoder Binary (not an artifact, just a config option) | ||
|
|
||
| # Profiler Features. |
There was a problem hiding this comment.
That bit seems unrelated to that particular PR.
| [artifacts.amd-dbgapi] | ||
| artifact_group = "debug-tools" | ||
| type = "target-neutral" | ||
| artifact_deps = ["amd-llvm", "core-runtime", "sysdeps"] |
There was a problem hiding this comment.
Few comments.
The dbgapi artifact itself does not depend on "core-runitme". We only need the rocr headers to build, but we do not need that at runtime.
And "amd-llvm" seems a quite big net. We only need amd-comgr, not the entire llvm suite (no clang/clang++ or any other tool needed at runtime).
| artifacts_group.add_argument( | ||
| "--debug-tools", | ||
| default=False, | ||
| help="Include ROCm debugging tools (amd-dbgapi, rocgdb and rocr-debug-agent) artifacts", |
There was a problem hiding this comment.
just a nit, this PR is only about dbgapi. gdb/debug agent will come later.
| # | ||
| # A library that provides support for a debugger and other tools to perform | ||
| # low-level control of the running code and inspection of the running state | ||
| # of AMD GPU's |
There was a problem hiding this comment.
missing period at the end of the sentence.
| # See if the compiler knows where libbacktrace.a is. It is worth mentioning | ||
| # that both gcc and llvm have the same behavior when --print-file-name is used | ||
| # but the compiler cannot locate the file anywhere. They output the filename | ||
| # passed as argument to --print-file-name unchanged. | ||
| # | ||
| # This makes it so the libbacktrace_check below will always pass. Though | ||
| # incorrect, this is somewhat harmless as we will call find_path and | ||
| # find_library later and we could end up finding includes and library | ||
| # depending on the system environment variables. | ||
| execute_process( | ||
| COMMAND ${CMAKE_CXX_COMPILER} ${CMAKE_CXX_FLAGS} --print-file-name libbacktrace.a | ||
| RESULT_VARIABLE libbacktrace_check | ||
| OUTPUT_VARIABLE libbacktrace_path | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ) | ||
| message(STATUS "[libbacktrace check] Command output: ${libbacktrace_path}") |
There was a problem hiding this comment.
As @lumachad highlights, this part is not meant to be used when compiling with TheRock. dbgapi still needs to be buildable outside of TheRock. All of those changes will end-up in the main dbgapi CMakeLists, so better minimise the changes.
Nevertheless, this part is not meant to build anything that will end-up in dbgapi, we just poke at the compiler configuration, which is absolutely fine during the configure phase. Similarly, below we do have some calls to check_cxx_source_compiles which DO compile stuff, and configure phase is when that must be done.
|
Aborted in favor of #2797. |
|
Thanks for all the reviews. |
Motivation
Add the debug-tools feature that will serve as the home to all debug tools projects we have: ROCdbgapi, ROCgdb and ROCR-debug-agent.
This commit adds the feature along with ROCdbgapi.
Please note this is restricted to Linux builds for now. Windows builds will come at a later stage.
Technical Details
This commit creates the minimal set of changes required to support the ROCm debug tools in TheRock build platform.
It adds the following:
NOTE 1: ROCdbgapi will be added as a submodule of TheRock only temporarily until a decision is made on the topic. It may be the case we will want to migrate it to rocm-systems. But adding it as a submodule now allows us to integrate the component into TheRock without having to wait for all the rocm-systems migration steps to take place.
NOTE 2: To make integration and testing faster, we have a local amd-dbgapi.cmake file that gets copied into the source directory before builds. Later on these cmake changes will be integrated into ROCdbgapi and the local file will be removed.
Test Plan
Test Result
All tests passed.