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

[ASan][Windows] Add __cdecl to public sanitizer functions #69625

Merged
merged 4 commits into from
Oct 31, 2023
Merged

[ASan][Windows] Add __cdecl to public sanitizer functions #69625

merged 4 commits into from
Oct 31, 2023

Conversation

strega-nil
Copy link
Contributor

This is necessary for many projects which pass /Gz to their compiles, which makes their default calling convention __stdcall.

(personal note, I really wish there was a pragma for this)

@mstorsjo mstorsjo requested a review from vitalybuka October 20, 2023 21:37
mstorsjo
mstorsjo approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

Functionally, this LGTM.

There's a lot of noise in the PR though, I presume it comes from clang-format wanting to update things and the files not being quite formatted to begin with? I guess that's ok...

I'd appreciate to hear if @vitalybuka has something more to say about it, so I hope you can wait for a day or so, to let him respond in case he has something to say.

@strega-nil
Copy link
Contributor Author

There's a lot of noise in the PR though, I presume it comes from clang-format wanting to update things and the files not being quite formatted to begin with? I guess that's ok...

yeah, that's exactly what it is; clang-format is annoying sometimes :/

@strega-nil
Copy link
Contributor Author

@vitalybuka would you like to take a look at this? Otherwise, I'll merge by tomorrow (2023-10-31T18:00Z)

vitalybuka
vitalybuka approved these changes Oct 31, 2023
View reviewed changes
Copy link
Collaborator

@vitalybuka vitalybuka left a comment

Choose a reason for hiding this comment

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

We use capitalized processor definitions
How about __sanitizer_cdecl -> SANITIZER_CDECL ?

@strega-nil
[ASan][Windows] Add __cdecl to public sanitizer functions
af2a24d 
This is necessary for many projects which pass `/Gz` to their compiles,
which makes their default calling convention `__stdcall`.
@strega-nil
fix tsan_interface_atomic
a124d88
@strega-nil
__sanitizer_cdecl -> SANITIZER_CDECL
da8510c
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 31, 2023 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@strega-nil strega-nil merged commit b799080 into llvm:main Oct 31, 2023
2 checks passed
@strega-nil strega-nil deleted the cdecly branch October 31, 2023 19:59
@strega-nil strega-nil mentioned this pull request Oct 31, 2023
Merged
strega-nil-ms pushed a commit to strega-nil/llvm-project that referenced this pull request Oct 31, 2023
@strega-nil
[windows][ASan] Fix build
b576a7d 
PR llvm#69625 broke the build - I put __cdecl on the wrong side of
the `*` in function pointer declarations.

Lesson learned - run check-all!
strega-nil added a commit that referenced this pull request Oct 31, 2023
@strega-nil
[windows][ASan] Fix build (#70855)
15b0cb4 
PR #69625 broke the build - I put __cdecl on the wrong side of the `*`
in function pointer declarations.

Lesson learned - run check-all!
@AaronBallman
Copy link
Collaborator

I think this change broke one of our bots:
https://lab.llvm.org/buildbot/#/builders/258/builds/8852

Can you investigate the failure?

@AaronBallman
Copy link
Collaborator

Ping @strega-nil

@strega-nil
Copy link
Contributor Author

@AaronBallman I'm not sure how that happened - it looks like memprof_interface.h is maybe including a different <sanitizer/common_interface_defs.h> than is actually in main???

@barcharcraz barcharcraz mentioned this pull request Nov 7, 2023
Merged
barcharcraz added a commit that referenced this pull request Nov 7, 2023
@barcharcraz
Add SANITIZER_CDECL to __tsan_check_no_mutexes_held (#71471)
16a395b 
in #69625 @strega-nil added
cdecl to a huge number of sanitizer interface declarations. It looks
like she was racing against @kennyyu adding a tsan interface function. I
noticed this when merging in the latest changes from llvm/main and
corrected it.

Co-authored-by: Charlie Barto <Charles.Barto@microsoft.com>
@Enna1 Enna1 mentioned this pull request Jan 25, 2024
Merged
qihangkong pushed a commit to rvgpu/llvm that referenced this pull request Apr 18, 2024
@barcharcraz @zahiraam
Add SANITIZER_CDECL to __tsan_check_no_mutexes_held (#71471)
40374ac 
in llvm/llvm-project#69625 @strega-nil added
cdecl to a huge number of sanitizer interface declarations. It looks
like she was racing against @kennyyu adding a tsan interface function. I
noticed this when merging in the latest changes from llvm/main and
corrected it.

Co-authored-by: Charlie Barto <Charles.Barto@microsoft.com>
branh pushed a commit to branh/llvm-project that referenced this pull request May 2, 2024
@strega-nil
[ASan][Windows] Add __cdecl to public sanitizer functions (llvm#69625)
c52aed9 
This is necessary for many projects which pass `/Gz` to their compiles,
which makes their default calling convention `__stdcall`.

(personal note, I _really_ wish there was a pragma for this)

(cherry picked from commit b799080)
branh pushed a commit to branh/llvm-project that referenced this pull request May 2, 2024
@strega-nil
[windows][ASan] Fix build (llvm#70855)
68bdd94 
PR llvm#69625 broke the build - I put __cdecl on the wrong side of the `*`
in function pointer declarations.

Lesson learned - run check-all!

(cherry picked from commit 15b0cb4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitalybuka vitalybuka vitalybuka approved these changes

@mstorsjo mstorsjo mstorsjo approved these changes

@barcharcraz barcharcraz Awaiting requested review from barcharcraz

Assignees
No one assigned
Labels
compiler-rt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@strega-nil @AaronBallman @mstorsjo @vitalybuka @llvmbot