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

Add -Wold-style-cast and fix related issues #682

Merged
merged 9 commits into from
Aug 21, 2024

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Aug 16, 2024

This PR enables -Wold-style-cast. TBB unfortunately has lots of these, and some of the examples binaries include TBB headers. To solve this, the builtin TBB target is modified to include directories with -isystem.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

With my recommendation you'd also need to change fewer files. 😉

cmake/traccc-functions.cmake Outdated Show resolved Hide resolved
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

For better or worse, as long as this hides all the warnings from the TBB code, this is the setup that I prefer. (Since this modification of the tbb library target per definition doesn't change anything about the compilation commands of the TBB library itself.)

@paulgessinger
Copy link
Member Author

Ok, so this fails in the SYCL build, because in that case we take TBB from the global installation in /opt/intel, so bypass the SYSTEM fix I added, and therefore compile without -isystem.

In file included from /__w/traccc/traccc/examples/run/cpu/throughput_mt.cpp:9:
In file included from /__w/traccc/traccc/examples/run/cpu/../common/throughput_mt.hpp:37:
In file included from /__w/traccc/traccc/examples/run/cpu/../common/throughput_mt.ipp:40:
In file included from /opt/intel/oneapi/tbb/2021.13/env/../include/tbb/global_control.h:17:
/opt/intel/oneapi/tbb/2021.13/env/../include/tbb/../oneapi/tbb/global_control.h:92:48: error: use of old-style cast [-Werror,-Wold-style-cast]
   92 |         return r1::global_control_active_value((int)p);
      |                                                ^    ~
1 error generated.
gmake[2]: *** [examples/run/cpu/CMakeFiles/traccc_throughput_mt.dir/build.make:76: examples/run/cpu/CMakeFiles/traccc_throughput_mt.dir/throughput_mt.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:4455: examples/run/cpu/CMakeFiles/traccc_throughput_mt.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs...

Any ideas @krasznaa?

@krasznaa
Copy link
Member

This is a pretty evil error. 😦

Since we now give the local TBB headers as system headers to the oneAPI compiler, it decides that heck, it has "better system headers" for TBB than what we have. And since oneAPI's headers are now included "implicitly", the whole -isystem thing doesn't mean anything for it. 😦 Since we only declare the directory inside the build directory with -isystem.

Let me think a bit. 🤔 I may need to call on some Intel developers about this in the end...

@stephenswat
Copy link
Member

What I'd try is this (not sure if I can suggest patches for unchanged files). In .github/ci_setup.sh change:

# Set up the correct environment for the SYCL tests.
if [ "${PLATFORM_NAME}" = "SYCL" ]; then
   if [ -f "/opt/intel/oneapi/setvars.sh" ]; then
      source /opt/intel/oneapi/setvars.sh --include-intel-llvm
   fi
fi

to

# Set up the correct environment for the SYCL tests.
if [ "${PLATFORM_NAME}" = "SYCL" ]; then
   if [ -f "/opt/intel/oneapi/setvars.sh" ]; then
      source /opt/intel/oneapi/setvars.sh --include-intel-llvm
      export CPATH=$(echo $CPATH | sed -e 's/\(^\|:\)[^:]*oneapi\/tbb[^:]*\($\|:\)/:/')
   fi
fi

That should prevent Intel's code from explicitly picking up its built-in headers.

@krasznaa
Copy link
Member

That's a good catch that it's the $CPATH environment variable! As long as the compiler is "only" picking up TBB through that, we should just unset $CPATH completely. 🤔 Our build doesn't rely on implicitly picking up any of those things after all.

[bash][celeborn]:~ > source ~/software/intel/oneapi-2024.2.1/setvars.sh 
 
:: initializing oneAPI environment ...
   bash: BASH_VERSION = 5.1.16(1)-release
   args: Using "$@" for setvars.sh arguments: 
:: advisor -- latest
:: compiler -- latest
:: debugger -- latest
:: dev-utilities -- latest
:: dpl -- latest
:: ipp -- latest
:: mkl -- latest
:: tbb -- latest
:: vtune -- latest
:: oneAPI environment initialized ::
 
[bash][celeborn]:~ > echo $CPATH | sed "s/:/\n/g"
/home/krasznaa/software/intel/oneapi-2024.2.1/tbb/2021.13/env/../include
/home/krasznaa/software/intel/oneapi-2024.2.1/mkl/2024.2/include
/home/krasznaa/software/intel/oneapi-2024.2.1/ipp/2021.12/include
/home/krasznaa/software/intel/oneapi-2024.2.1/dpl/2022.6/include
/home/krasznaa/software/intel/oneapi-2024.2.1/dev-utilities/2024.2/include
[bash][celeborn]:~ > 

@stephenswat
Copy link
Member

I thought about this, but I'm not sure how important dev-utilities is... 🤔

@krasznaa
Copy link
Member

I think we're okay without it.

[bash][celeborn]:~ > ls -l /home/krasznaa/software/intel/oneapi-2024.2.1/dev-utilities/2024.2/include
total 12
-rw-r--r-- 1 krasznaa krasznaa 1102 May 24 13:04 dpc_common.hpp
-rw-r--r-- 1 krasznaa krasznaa  581 May 24 13:04 dpc_common_README.md
drwxr-xr-x 2 krasznaa krasznaa 4096 Aug  8 17:02 stb
[bash][celeborn]:~ > more /home/krasznaa/software/intel/oneapi-2024.2.1/dev-utilities/2024.2/include/dpc_common_README.md 
# oneAPI-samples/Common/


## `dpc_common.hpp`

Many oneAPI code samples share common include files.  These include files are installed locally with the product installation and 
can be located at `%ONEAPI_ROOT%\dev-utilities\latest\include` on your development system. These same include files are provided h
ere for your reference.

| Common files/directory            | Description
|:---                               |:---
| dpc_common.hpp                    | Common macros and functions for oneAPI samples
| STB                               | Image rendering library
[bash][celeborn]:~ >

Also: https://github-wiki-see.page/m/oneapi-src/oneAPI-samples/wiki/dpc_common.hpp

@stephenswat
Copy link
Member

stephenswat commented Aug 20, 2024

Right, then I'd do something like this:

# Set up the correct environment for the SYCL tests.
if [ "${PLATFORM_NAME}" = "SYCL" ]; then
   if [ -f "/opt/intel/oneapi/setvars.sh" ]; then
      OLD_CPATH=${CPATH}
      source /opt/intel/oneapi/setvars.sh --include-intel-llvm
      export CPATH=${OLD_CPATH}
   fi
fi

Copy link
Member

@stephenswat stephenswat left a comment

Choose a reason for hiding this comment

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

Unfortunate that we had to add this semi-ugly CI fix, but that's Intel's fault not ours.

@stephenswat stephenswat enabled auto-merge (squash) August 21, 2024 09:15
@stephenswat stephenswat merged commit 72b421f into acts-project:main Aug 21, 2024
18 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants