Skip to content

Conversation

@malfet
Copy link
Contributor

@malfet malfet commented Jan 11, 2023

And rebase the patches

And rebase the patches
@vercel
Copy link

vercel bot commented Jan 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
torchci ⬜️ Ignored (Inspect) Jan 13, 2023 at 3:44AM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 11, 2023
@malfet
Copy link
Contributor Author

malfet commented Jan 12, 2023

Ok, I glad it has testing, as clang-tidy binary build with static linking simply fails to initialize. Need to dig further

@malfet
Copy link
Contributor Author

malfet commented Jan 12, 2023

@Skylion007 what version of clang-tidy do you need? I can easily update to 12, but 14 and 15 for some reason crash at the start

@Skylion007
Copy link

Skylion007 commented Jan 12, 2023

@Skylion007 what version of clang-tidy do you need? I can easily update to 12, but 14 and 15 for some reason crash at the start

Actually, turns out clang-tidy 11 has the bare minimum of what I need after all (the feature was actually added in clang-tidy 11 https://reviews.llvm.org/D75184). In that case clang-tidy 11 onwards should be good enough. Is 13 broken as well out of curiosity?

@malfet
Copy link
Contributor Author

malfet commented Jan 13, 2023

@Skylion007 what version of clang-tidy do you need? I can easily update to 12, but 14 and 15 for some reason crash at the start

Actually, turns out clang-tidy 11 has the bare minimum of what I need after all (the feature was actually added in clang-tidy 11 https://reviews.llvm.org/D75184). In that case clang-tidy 11 onwards should be good enough. Is 13 broken as well out of curiosity?

We already have clang-tidy 11, don't we?
And clang-13 for whatever reason simply fails to compile in a static mode (and clang-tidy must be static otherwise it can't be installed on any linux machine and used by linter)

@malfet
Copy link
Contributor Author

malfet commented Jan 13, 2023

Ok, after a little bit of debugging, find out the problem : pthread_rwlock_wrlock isn't really usable from statically linked app. But following little patch helps:

diff --git a/llvm/include/llvm/Support/RWMutex.h b/llvm/include/llvm/Support/RWMutex.h
index 3dd962586..1e055e628 100644
--- a/llvm/include/llvm/Support/RWMutex.h
+++ b/llvm/include/llvm/Support/RWMutex.h
@@ -20,11 +20,7 @@
 #include <shared_mutex>
 
 // std::shared_timed_mutex is only availble on macOS 10.12 and later.
-#if defined(__APPLE__) && defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)
-#if __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 101200
 #define LLVM_USE_RW_MUTEX_IMPL
-#endif
-#endif
 
 namespace llvm {
 namespace sys {

@malfet malfet merged commit 813fdc0 into main Jan 13, 2023
@malfet malfet deleted the malfet/move-clang-tidy-to-15 branch January 13, 2023 04:24
malfet added a commit to pytorch/pytorch that referenced this pull request Jan 14, 2023
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Jan 18, 2023
Based on results from pytorch/test-infra#1382

Fixes #ISSUE_NUMBER

Pull Request resolved: #92195
Approved by: https://github.com/Skylion007
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants