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

[FEA] rapids_cmake_support_conda_env should add -O0 to debug compile lines #634

Closed
robertmaynard opened this issue Jun 25, 2024 · 5 comments · Fixed by #635
Closed

[FEA] rapids_cmake_support_conda_env should add -O0 to debug compile lines #634

robertmaynard opened this issue Jun 25, 2024 · 5 comments · Fixed by #635
Labels
? - Needs Triage Need team to review and classify feature request New feature or request

Comments

@robertmaynard
Copy link
Contributor

robertmaynard commented Jun 25, 2024

Is your feature request related to a problem? Please describe.
When building in a conda env the default CXXFLAGS, CFLAGS, etc will inject -O2 on the compile line making Debug bugs significantly less useful.

Instead of having each RAPIDS project manually handle this issue, rapids-cmake shoudl be extended to do it for users

Describe the solution you'd like
rapids_cmake_support_conda_env should be updated to add:

target_compile_options(${target} INTERFACE "$<$<CONFIG:Debug>:-O0>")

That will ensure that debug builds are built without optimizations.

Additional context

When you specify CMAKE_BUILD_TYPE=Debug CMake will provide the -g flag to produce a build with debug information. The optimization level in these cases -O2 is coming from your env ( aka conda ) which globally injects the -O2 along with a host of other optimization flags into CXXFLAGS, CFLAGS. and so forth. This combines together as you have seen.

CMake presumption is that builds occurring under env variables such as CXXFLAGS are explicit requests. Since -O0 is the default optimization level passing -g is sufficient for debug builds and what you wanted was a 'releasish' build with debug info ( aka what CMAKE_BUILD_TYPE=RelWithDebInfo would have done ).

What is the right way to do this?

One argument is that the root issue is the inability to tell conda that you want to do 'debug' builds ( easily ). Therefore the activation scripts should flip the env variables, since conda does establish CXXFLAGS_DEBUG, etc. This solves the problem since it extends outside of RAPIDS. I expect this isn't a viable solution given how long it would take ( if possible at all )

Another argument is that CMake should pick CXXFLAGS etc based on the CMAKE_BUILD_TYPE. Which like the above isn't really possible due to how CMake's compiler detection works. Things like CXXFLAGS are only parsed on the first invocation and are treated as invariant afterwards, while CMAKE_BUILD_TYPE can change for each execution.

Another argument would be that CMake debug flags -g should change to -g -O0 which is more reasonable and would line up with the ongoing discussion around optimization levels in general ( https://gitlab.kitware.com/cmake/cmake/-/issues/23374 ). This again is a long term solution and wouldn't unblock us this year, but I will remember to push on this design going forward.

@robertmaynard robertmaynard added feature request New feature or request ? - Needs Triage Need team to review and classify labels Jun 25, 2024
@harrism
Copy link
Member

harrism commented Jun 26, 2024

So... what is the short-term solution?

@robertmaynard
Copy link
Contributor Author

target_compile_options(${target_name} PRIVATE $<$<CONFIG:Debug>>:-O0>

Is the recommend solution for targets that want debug info. Since C, C++, and CUDA compilers support -O0 that will map properly no matter the languages used in the target

robertmaynard added a commit to robertmaynard/rapids-cmake that referenced this issue Jun 26, 2024
robertmaynard added a commit to robertmaynard/rapids-cmake that referenced this issue Jun 26, 2024
@rapids-bot rapids-bot bot closed this as completed in #635 Jun 27, 2024
rapids-bot bot pushed a commit that referenced this issue Jun 27, 2024
When building in a conda env the default CXXFLAGS, CFLAGS, etc will inject -O2 on the compile line making Debug bugs significantly less useful. `rapids_cmake_support_conda_env` now injects a `-O0` to override the `-O2` set by conda to make debug builds usable.

Fixes #634

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #635
@harrism
Copy link
Member

harrism commented Aug 28, 2024

@robertmaynard in spite of this PR, when I build RMM with -DCMAKE_BUILD_TYPE=Debug, I get -g ... -O2 in the command lines... So I don't think it's working.

@robertmaynard
Copy link
Contributor Author

@robertmaynard in spite of this PR, when I build RMM with -DCMAKE_BUILD_TYPE=Debug, I get -g ... -O2 in the command lines... So I don't think it's working.

RMM isn't currently using rapids_cmake_support_conda_env

@harrism
Copy link
Member

harrism commented Sep 12, 2024

I see. Since my question prompted the filing of this issue, I thought the PR was a fix for my issue. :). RMM CMake doesn't mention conda at all. I'm not sure if it should or should not, as not a conda expert. I don't know if rapids_cmake_support_conda_env is something all RAPIDS repo cmake should use or not, so I'm not sure if I need to write up an RMM issue for this.

For now, I've added -O0 to the target_compile_definitions of RMM tests, which solves my problem.

rapids-bot bot pushed a commit to rapidsai/rmm that referenced this issue Oct 22, 2024
Fixes issue brought up in rapidsai/rapids-cmake#634 (comment) where rmm wasn't using rapids_cmake_support_conda_env

Authors:
  - Robert Maynard (https://github.com/robertmaynard)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Mark Harris (https://github.com/harrism)

URL: #1707
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
? - Needs Triage Need team to review and classify feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants