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

[CI] Add CI configurations for assertions, clang, and sanitizers #634

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

kuhar
Copy link
Contributor

@kuhar kuhar commented Apr 27, 2020

This extends the current public CI infrastructure with 5 new build
configurations. Each of configuration has its own base image cached
in GCR. This way each incremental CI build doesn't have to know about
the precise CMake invocation and resumes from whatever configuration
its base imgage used.

GitHub actions offer 10 free concurrent CI jobs, so this should
significanly slow down the public CI, with 4 workers still unused.

@amdvlk-admin
Copy link
Collaborator

Can one of the admins verify this patch?

@JaxLinAMD
Copy link
Contributor

@kuhar , how about always enable assertion and sanitzers? then we only have two build variants:
gcc, clang

@Flakebi
Copy link
Member

Flakebi commented Apr 28, 2020

Looks good in general, thanks for working on this.

I have a few notes on the sanitizers:

  • I need to get some patches into the xgl cmake configuration before the driver compiles with sanitizers enabled, so I think it makes sense to enable this CI configuration in a later PR.
  • libclang-common-6.0-dev (or libclang-common-9.0-dev) is needed for the asan runtime libraries. We need to use the shared asan version, the statically linked version is not supported for shared libraries as far as I know.
  • To use it, we need to set LD_PRELOAD=/usr/lib/llvm-6.0/lib/clang/6.0.0/lib/linux/libclang_rt.asan-x86_64.so (or corresponding 9 version).
  • Using tablegen in debug+sanitizer mode is really slow (it might be better in release mode), -DLLVM_OPTIMIZED_TABLEGEN=1 improves that but it needs D78913 in LLVM.
  • The flag is called -DLLVM_USE_SANITIZER=… (note the missing s) which cmake warns about hidden in the 5000 line log ;)

@kuhar
Copy link
Contributor Author

kuhar commented Apr 28, 2020

@kuhar , how about always enable assertion and sanitzers? then we only have two build variants:
gcc, clang

Having assertions always on is not a good idea. They affect ABI and can hide bugs -- a simple example is code being correct only when assertions are executed, or vice versa. When it comes to sanitizers, I think that in practice they can complain about some issues that are pretty low-priority to fix, and it would be good to know that even though they complain, regular builds continue working.

@kuhar
Copy link
Contributor Author

kuhar commented Apr 28, 2020

Looks good in general, thanks for working on this.

I have a few notes on the sanitizers:

Thanks for pointing the issues out.

If you are happy with the general direction of this PR, perhaps it would be best to commit only the clang part now and add builds with assertions and sanitizers later, once the CMake build properly support them?

This extends the current public CI infrastructure with 5 new build
configurations. Each of configuration has its own base image cached
in GCR. This way each incremental CI build doesn't have to know about
the precise CMake invocation and resumes from whatever configuration
its base imgage used.

GitHub actions offer 10 free concurrent CI jobs, so this should
significanly slow down the public CI, with 4 workers still unused.
@kuhar
Copy link
Contributor Author

kuhar commented Apr 28, 2020

I updated the PR to run without sanitizers for now. The most important thing for me is that we have a mechanism to support these in the future and that there's a clear way to add more 'features' as we need.

@Flakebi Can we introduce 2 new flags:

  1. XYZ_ENABLE_ASSERTIONS that enables assertions also in release mode, and probably forwards to LLVM_ENABLE_ASSERTIONS.
  2. XYZ_ENABLE_SANITIZER that works like the LLVM one but sets everything up like you described.

Where XYZ is XGL or LLPC, whichever is more appropriate. I'm not very familiar with how the CMake builds are setup for amdvlk and would appreciate if someone else took over that part of the CI setup.

@Flakebi
Copy link
Member

Flakebi commented Apr 28, 2020

I updated the PR to run without sanitizers for now. The most important thing for me is that we have a mechanism to support these in the future and that there's a clear way to add more 'features' as we need.

Looks good!

  1. XYZ_ENABLE_ASSERTIONS: Sounds good, enabling LLVM_ENABLE_ASSERTIONS, PAL_ENABLE_PRINTS_ASSERTS and removing NDEBUG from the release definitions somehow should do this. (Although PAL assertions shouldn’t change anything as we cannot test PAL code on GitHub anyway)
  2. XYZ_ENABLE_SANITIZER: I’m not quite sure how to integrate that into the cmake declarations because I don’t want to duplicate all the code from LLVM for LLVM_ENABLE_SANITIZER, which already sets global flags. So far I was just reusing LLVM_ENABLE_SANITIZER.

I’ll try to get these flags into xgl.

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.

LGTM

@kuhar
Copy link
Contributor Author

kuhar commented Apr 28, 2020

2. XYZ_ENABLE_SANITIZER: I’m not quite sure how to integrate that into the cmake declarations because I don’t want to duplicate all the code from LLVM for LLVM_ENABLE_SANITIZER, which already sets global flags. So far I was just reusing LLVM_ENABLE_SANITIZER.

You mentioned something about having to set LD_PRELOAD -- I thought that this can be setup in CMake, in addition to forwarding the flag to the llvm one. Is that right?

I’ll try to get these flags into xgl.

Sounds good to me.

@Flakebi
Copy link
Member

Flakebi commented Apr 28, 2020

You mentioned something about having to set LD_PRELOAD -- I thought that this can be setup in CMake, in addition to forwarding the flag to the llvm one. Is that right?

Actually, ignore that one, I think it only plays a role when using a shared library (amdvlk64.so), for example when running the CTS, not when running unit tests.

@JaxLinAMD JaxLinAMD merged commit 3dd0495 into GPUOpen-Drivers:dev Apr 29, 2020
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.

5 participants