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

Use GCC 13 in CUDA 12 conda builds. #811

Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jan 13, 2025

Description

conda-forge is using GCC 13 for CUDA 12 builds. This PR updates CUDA 12 conda builds to use GCC 13, for alignment.

These PRs should be merged in a specific order, see rapidsai/build-planning#129 for details.

@bdice bdice added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Jan 13, 2025
Copy link

copy-pr-bot bot commented Jan 13, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@bdice bdice marked this pull request as ready for review January 13, 2025 16:59
@bdice bdice requested a review from a team as a code owner January 13, 2025 16:59
@bdice bdice requested a review from gforsyth January 13, 2025 16:59
@bdice bdice force-pushed the use-gcc-13-with-cuda-12-conda-builds branch from 11830a2 to f3709cf Compare January 13, 2025 17:06
@bdice bdice self-assigned this Jan 13, 2025
redundant move in return statement [-Werror=redundant-move]
@bdice bdice requested a review from a team as a code owner January 13, 2025 17:56
@jameslamb jameslamb requested review from jameslamb and removed request for gforsyth January 13, 2025 19:59
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Approving the packaging changes, assuming the fix for this build issue will be small and/or reviewed by C++ codeowners:

[ 44%] Building CXX object _deps/deps-googlebenchmark-build/src/CMakeFiles/benchmark.dir/counter.cc.o
In file included from /opt/conda/conda-bld/work/benchmarks/primitives.cpp:23:
In function 'void benchmark::DoNotOptimize(Tp&) [with Tp = char*]',
    inlined from 'void string_strcpy(benchmark::State&)' at /opt/conda/conda-bld/work/benchmarks/primitives.cpp:110:33:
/opt/conda/conda-bld/work/build-release/_deps/deps-googlebenchmark-src/src/../include/benchmark/benchmark.h:322:3: error: pointer used after 'void free(void*)' [-Werror=use-after-free]
  322 |   asm volatile("" : "+m,r"(value) : : "memory");
      |   ^~~

(build link)

@jakirkham
Copy link
Member

This looks like a bug in the Google Benchmark library: google/benchmark#1675

We may need to disable the relevant benchmarks in cuCIM until upstream can resolve

@jakirkham
Copy link
Member

^ @gigony would you be able to help with this?

@gigony
Copy link
Contributor

gigony commented Jan 15, 2025

Thanks @bdice and @jakirkham for this PR.

Please refer to #812, which is a prerequisite for merging this PR.

rapids-bot bot pushed a commit that referenced this pull request Jan 16, 2025
The pointer variable is used after releasing memory with `free()`, which is a bug that needs to be addressed.

Without this fix, the benchmark code may fail to build or run correctly, resulting in the following error ([link](https://github.com/rapidsai/cucim/actions/runs/12753155354/job/35544254443?pr=811))
```
In function 'void benchmark::DoNotOptimize(Tp&) [with Tp = char*]',
    inlined from 'void string_strcpy(benchmark::State&)' at /opt/conda/conda-bld/work/benchmarks/primitives.cpp:110:33:
/opt/conda/conda-bld/work/build-release/_deps/deps-googlebenchmark-src/src/../include/benchmark/benchmark.h:322:3: error: pointer used after 'void free(void*)' [-Werror=use-after-free]
  322 |   asm volatile("" : "+m,r"(value) : : "memory");
      |   ^~~
```
It seems that the updated version of GCC is now capable of detecting this potential issue.  

This commit resolves the issue by ensuring the memory is released only after `benchmark::DoNotOptimize()` is called.

This PR handles a build issue related to #811
- #811

Authors:
  - Gigon Bae (https://github.com/gigony)

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - https://github.com/jakirkham

URL: #812
@jakirkham
Copy link
Member

Thanks Gigon! 🙏

Bradley and I both reviewed and approved that PR. Since it passed, have also merged it

Updated this PR to pull in those changes. Let's see how it goes

@jakirkham
Copy link
Member

A couple CI jobs are failing during package install in tests

Likely we need to consolidate these installs as has been done for a few other RAPIDS repos: rapidsai/build-planning#22

Will work on putting something together

@jakirkham
Copy link
Member

Looks like this is now passing 🥳

@bdice
Copy link
Contributor Author

bdice commented Jan 16, 2025

CI is now passing with a rerun. I'll mark this repo as ready in the tracking issue.

@jakirkham
Copy link
Member

Already done :)

@bdice
Copy link
Contributor Author

bdice commented Jan 17, 2025

/merge

@rapids-bot rapids-bot bot merged commit a0fed4e into rapidsai:branch-25.02 Jan 17, 2025
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants