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.

Sign up for GitHub

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

Jump to bottom

Enable -flto and -fsanitize=cfi in clang #2222

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Enable -flto and -fsanitize=cfi in clang #2222

merged 1 commit into from
Nov 15, 2024
+71 −20

Conversation

RossBrunton
Copy link
Contributor

@RossBrunton RossBrunton commented Oct 18, 2024 •
edited
Loading

@github-actions github-actions bot added the conformance Conformance test suite issues. label Oct 18, 2024
@RossBrunton RossBrunton force-pushed the ross/cfi branch 2 times, most recently from 99bf458 to fee753b Compare October 24, 2024 10:42
@RossBrunton
Copy link
Contributor Author

@PatKamin is there any particular reason the fuzz testing explicitly sets "-g"? It seems to trigger a weird edge case when used in release mode when -lto is also enabled, which crashes the linker.

@PatKamin
Copy link
Contributor

@PatKamin is there any particular reason the fuzz testing explicitly sets "-g"? It seems to trigger a weird edge case when used in release mode when -lto is also enabled, which crashes the linker.

I've added it after libFuzzer examples: https://llvm.org/docs/LibFuzzer.html#fuzzer-usage
I think that it is safe to remove it as mixing Release flags prepared by CMake with this debug flag might cause issues like the one you have.

@RossBrunton RossBrunton force-pushed the ross/cfi branch 4 times, most recently from bd54953 to 3782c5f Compare October 25, 2024 16:31
@RossBrunton RossBrunton force-pushed the ross/cfi branch 13 times, most recently from 25983b0 to abcb72b Compare November 7, 2024 12:16
@github-actions github-actions bot added the common Changes or additions to common utilities label Nov 7, 2024
@RossBrunton RossBrunton force-pushed the ross/cfi branch 7 times, most recently from 4132a6e to 49770db Compare November 7, 2024 17:04
RossBrunton added a commit to RossBrunton/intel-llvm that referenced this pull request Nov 11, 2024
@RossBrunton
[UR] Bump UR with cfi sanitizer
3243cba 
Pre-commit MR for:
oneapi-src/unified-runtime#2222
@RossBrunton RossBrunton mentioned this pull request Nov 11, 2024
Merged
RossBrunton added a commit to RossBrunton/intel-llvm that referenced this pull request Nov 11, 2024
@RossBrunton
[UR] Bump UR with cfi sanitizer
4beeefd 
Pre-commit MR for:
oneapi-src/unified-runtime#2222
@RossBrunton RossBrunton marked this pull request as ready for review November 11, 2024 13:00
@RossBrunton RossBrunton requested review from a team as code owners November 11, 2024 13:00
aarongreig
aarongreig reviewed Nov 11, 2024
View reviewed changes
test/fuzz/CMakeLists.txt
@@ -51,7 +51,9 @@ target_link_libraries(fuzztest-base
${PROJECT_NAME}::headers
${PROJECT_NAME}::common
-fsanitize=fuzzer)
target_compile_options(fuzztest-base PRIVATE -g -fsanitize=fuzzer)
# When built with -g and -flto (which is required by some hardening flags), this causes a segfault in (upstream)
# LLVM 14-15 while linking when CMAKE_BUILD_TYPE is Release
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. It feels like an upstream compiler bug, and we don't need to build with -g here anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe an upstream issue is appropriate then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, this is fixed in LLVM 16+, so either they knew about it and fixed it or it happened to be fixed anyway with another change.

RossBrunton added a commit to RossBrunton/intel-llvm that referenced this pull request Nov 11, 2024
@RossBrunton
[UR] Bump UR with cfi sanitizer
1a64ee8 
Pre-commit MR for:
oneapi-src/unified-runtime#2222
@RossBrunton RossBrunton requested a review from a team as a code owner November 12, 2024 12:23
@RossBrunton RossBrunton marked this pull request as draft November 12, 2024 14:54
@RossBrunton RossBrunton force-pushed the ross/cfi branch 5 times, most recently from 96708bb to 15aaebe Compare November 13, 2024 17:28
@RossBrunton RossBrunton marked this pull request as ready for review November 13, 2024 17:32
@RossBrunton RossBrunton mentioned this pull request Nov 14, 2024
Closed
RossBrunton added a commit to RossBrunton/intel-llvm that referenced this pull request Nov 14, 2024
@RossBrunton
[UR] Bump UR with cfi sanitizer
1dcc534 
Pre-commit MR for:
oneapi-src/unified-runtime#2222
@RossBrunton
Enable -flto and -fsanitize=cfi in clang
58dabfe 
Note that this flagged up a few issues, for which followup tickets
were created:
* oneapi-src#2323
* oneapi-src#2309
* oneapi-src#2324
EwanC
EwanC approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

command-buffer match file changes LGTM, I've created an internal ticket for our team to look into the fails further.

@RossBrunton
Copy link
Contributor Author

@oneapi-src/unified-runtime-level-zero-v2-write This flags up an issue with the event pool tests, so I've disabled it for now if CFI is to be enabled.

Could you look and see if this is something you're okay with me doing?

@RossBrunton RossBrunton added the ready to merge Added to PR's which are ready to merge label Nov 15, 2024
@callumfare callumfare merged commit 30391c6 into oneapi-src:main Nov 15, 2024
86 checks passed
sarnex pushed a commit to intel/llvm that referenced this pull request Nov 15, 2024
@RossBrunton @callumfare
[UR] Bump UR with cfi sanitizer (#16040)
27dab62 
Pre-commit MR for:
oneapi-src/unified-runtime#2222

---------

Co-authored-by: Callum Fare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isaacault isaacault isaacault left review comments

@aarongreig aarongreig aarongreig approved these changes

@martygrant martygrant martygrant approved these changes

@pbalcer pbalcer pbalcer approved these changes

@EwanC EwanC EwanC approved these changes

Assignees
No one assigned
Labels
ci/cd Continuous integration/devliery common Changes or additions to common utilities conformance Conformance test suite issues. ready to merge Added to PR's which are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@RossBrunton @PatKamin @EwanC @pbalcer @martygrant @isaacault @aarongreig @callumfare