Skip to content

PR for llvm/llvm-project#64931 #671

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
Closed
wants to merge 4 commits into from
Closed

PR for llvm/llvm-project#64931 #671

wants to merge 4 commits into from

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Aug 31, 2023

@llvmbot
Copy link
Member Author

llvmbot commented Aug 31, 2023

@MaskRay @statham-arm What do you think about merging this PR to the release branch?

@llvmbot llvmbot mentioned this pull request Aug 31, 2023
Closed
MaggieYingYi and others added 4 commits August 31, 2023 03:38
@MaggieYingYi @llvmbot
[UBSan] Disable the function and kcfi sanitizers on an execute-only t…
fba14e4 
…arget.

An execute-only target disallows data access to code sections.
-fsanitize=function and -fsanitize=kcfi instrument indirect function
calls to load a type hash before the function label. This results in a
non-execute access to the code section and a runtime error.

To solve the issue, -fsanitize=function should not be included in any
check group (e.g. undefined) on an execute-only target. If a user passes
-fsanitize=undefined, there is no error and no warning. However, if the
user explicitly passes -fsanitize=function or -fsanitize=kcfi on an
execute-only target, an error will be emitted.

Fixes: llvm/llvm-project#64931.

Reviewed By: MaskRay, probinson, simon_tatham

Differential Revision: https://reviews.llvm.org/D158614

(cherry picked from commit 9ef536a12ea65a2b9e2511936327c7b621af38fb)
@pranavk @llvmbot
[Bazel] Fix build
9fcc8c2 
(cherry picked from commit d26dd681f9726ed7d43d7c0bdd8ee3cb2db69a2b)
@pranavk @llvmbot
[Bazel] Better fix for d26dd681
ca808f3 
Don't use glob for specific files

(cherry picked from commit 39f6a31eaa665b1a01305346092ed69f4b9f940a)
@MaskRay @llvmbot
[Driver] Adjust -fsanitize=function & -mexecute-only interop after D1…
18f8be9 
…58614

clangDriver depends on clangBasic, so clangBasic should not depend on
clangDriver, even just its header. Also remove clangBasic's dependency
on LLVMOption.

The issue can be seen through the bazel commit
d26dd681f9726ed7d43d7c0bdd8ee3cb2db69a2b which is reverted now.

Add hasFlagNoClaim and use it as we don't want to suppress
-Wunused-command-line-argument for -mexecute-only just because
-fsanitize= is specified.

(cherry picked from commit c7dce0283b5b502dacd47eb1488342a31f4ed35a)
@tru
Copy link
Contributor

tru commented Aug 31, 2023

Hmm this contains unrelated changes it seems like?

@tru
Copy link
Contributor

tru commented Aug 31, 2023

There are some test failures in this as well.

@statham-arm
Copy link
Contributor

It looks to me as if the test failure has to do with D156363, which is on main but not on the release branch. The extra RUN lines added to clang/test/Driver/fsanitize.c are all expecting clang -### to return 0 or 1 depending on whether its command line is legal, because that's what happens on main. But before D156363, clang -### returns 0 regardless. I was able to get the test to pass by removing every not from the front of a newly added RUN line.

@statham-arm
Copy link
Contributor

(I'm not very familiar with LLVM release procedure, so I don't know if it's the done thing to push an extra commit to the end of this PR branch making that change, or even if I'd have access to do it if I tried.)

@MaskRay
Copy link
Member

MaskRay commented Aug 31, 2023

(I'm not very familiar with LLVM release procedure, so I don't know if it's the done thing to push an extra commit to the end of this PR branch making that change, or even if I'd have access to do it if I tried.)

I am unfamiliar, either:)

I modified the clang/test/Driver/fsanitize.c and added a /branch MaskRay/llvm-project/17.x-mexecute-only on llvm/llvm-project#64931 (comment) , hopefully it can update this PR.

@MaskRay
Copy link
Member

MaskRay commented Aug 31, 2023

Hmm this contains unrelated changes it seems like?

There are two util/bazel/ changes that [Driver] Adjust -fsanitize=function & -mexecute-only interop after D158614 reverts. It was intended for cleaner cherr-pick.

As I now modify the patch series anyway, I have removed the two Bazel commits.

@tru
Copy link
Contributor

tru commented Sep 1, 2023

Adjusted branch was merged manually.

@tru tru closed this Sep 1, 2023
@tru tru deleted the llvm-issue64931 branch September 1, 2023 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[UBSan] Runtime Executable Only violation hit when checking the function pointer on an execute-only target.
6 participants
@llvmbot @tru @statham-arm @MaskRay @MaggieYingYi @pranavk