Adds build support for hexagon linux to sanitizer common.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Driver/ToolChains/Hexagon.cpp | ||
---|---|---|
297–298 ↗ | (On Diff #366476) | I will try to determine which sanitizers require libunwind so that it's only added when appropriate. |
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | ||
162 | Most of the feedback from clang-format checks don't seem to really be the format we use here. We should probably apply a more specific .clang-format file if these aren't useful. | |
compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc | ||
125 | @sidneym Can you confirm that this implementation looks correct? I thought the convention was -1 for failed syscalls. |
compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc | ||
---|---|---|
125 | Maybe it should be <0 instead. |
Can you cut this into separate peaces, at least patch per sanitizer for "asan lsan ubsan scudo scudo_standalone cfi safestack"?
Some common peaces could be probably extracted as well.
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h | ||
---|---|---|
106 | please clang-format entire patch | |
compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc | ||
131 | Please fix clang-tidy which can be fixed. | |
compiler-rt/test/asan/CMakeLists.txt | ||
19 ↗ | (On Diff #366476) | why only this test changes? |
Sure, that's no problem. While I'm breaking it up, us there a guideline/preference to whether the clang changes can be combined in one of these patches, or should those be on their own as well?
Some common peaces could be probably extracted as well.
compiler-rt/test/asan/CMakeLists.txt | ||
---|---|---|
19 ↗ | (On Diff #366476) | Sorry, I shouldnt have included this, I'll omit it. I'd like to land the build changes before test ones. |
I think we have one intended to for checking hexagon-unknown-elf and perhaps not one for hexagon-unknown-linux-musl. I believe that only the Linux target uses the compiler-rt libraries, though.
We can help create a bot for hexagon-unknown-linux-musl. What's the process?
Decomposed to just the sanitizer-common portion. Now it's an 8 patch series, this is number 1.
Please correct me if I am wrong, but LLVM does not use [PATCH *] tags for series. Please remove them from title and git patches.
You can link dependent patches into Phabricator "stack" above, but most extracted patches are orthogonal, and can be summited in any order.
We can help create a bot for hexagon-unknown-linux-musl. What's the process?
Thank you. https://llvm.org/docs/HowToAddABuilder.html
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | ||
---|---|---|
162 | clang-format pre-merge check uses .clang-format files. Actually this place is ugly. Current .clang-format will indent when the rest of the block is not. | |
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp | ||
25 | but please fix ones like this which do not require significant reformatting of unchanged code. |
Oh I think I misunderstood:
Volunteers can provide their build machines to work as build workers to public LLVM Buildbot.
I was prepared to write / test / enable a build recipe but I don't have access to a public system that can be added as a worker. But I can work with our team to get the ball rolling, it just doesn't seem like it would be a quick process.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | ||
---|---|---|
162 | I did apply clang-format-diff via pre-commit hook and I don't believe it made significant changes, but this is after that formatting. I prefer to keep it as-is because the style near here seems to match. | |
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp | ||
25 | Ok that's no problem. |
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp | ||
---|---|---|
25 |
But it's a little odd -- following the format would pull the or operator onto the previous line, which would deviate from the style used in the rest of the file. Still happy to make the change, it just doesn't seem like it fits. |
Please try to do so. Even infrequent builds, like one per day, are helpful. Without that implementation will likely degrade overtime.
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp | ||
---|---|---|
25 | Goal is to avoid manual labor as much as possible even if clang-format result are a little bit inconsistent. |
The last snapshot uploaded without "arc diff", so it does not show "context" code around, which is inconvenient for review.
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp | ||
---|---|---|
94 | please use clang-format version here. | |
compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc | ||
88 | should this use u64 like other __internal_syscall files? | |
125 | Why it's not -4095 like in other versions? |
compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc | ||
---|---|---|
88 | Sorry I didn't notice this review comment before. No, u64 doesn't seem appropriate - they're 32-bit registers. I can change it to u32 if that makes more sense. |
fixed iserror()
compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc | ||
---|---|---|
125 |
It should be, I misunderstood the intent. |
clang-format: please reformat the code