Page MenuHomePhabricator

[sanitizer] Add hexagon support to sanitizer-common
AcceptedPublic

Authored by bcain on Aug 14 2021, 11:38 PM.

Details

Summary

Adds build support for hexagon linux to sanitizer common.

Diff Detail

Event Timeline

bcain created this revision.Aug 14 2021, 11:38 PM
bcain requested review of this revision.Aug 14 2021, 11:39 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 14 2021, 11:39 PM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript
bcain retitled this revision from [DRAFT] add sanitizer support to hexagon to add sanitizer support to hexagon.Aug 15 2021, 2:56 PM
bcain edited the summary of this revision. (Show Details)
bcain added reviewers: vitalybuka, dvyukov.
bcain added inline comments.Aug 15 2021, 3:00 PM
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.

sidneym added inline comments.Aug 16 2021, 11:31 AM
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?

Do we have public bot for hexagon?

bcain added a comment.Aug 16 2021, 3:33 PM

Can you cut this into separate peaces, at least patch per sanitizer for "asan lsan ubsan scudo scudo_standalone cfi safestack"?

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.

bcain added a comment.Aug 16 2021, 3:36 PM

Do we have public bot for hexagon?

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?

bcain updated this revision to Diff 366766.Aug 16 2021, 5:07 PM
bcain retitled this revision from add sanitizer support to hexagon to [sanitizer] Add hexagon support to sanitizer-common (1/8).
bcain edited the summary of this revision. (Show Details)

Decomposed to just the sanitizer-common portion. Now it's an 8 patch series, this is number 1.

bcain retitled this revision from [sanitizer] Add hexagon support to sanitizer-common (1/8) to [PATCH 5/8] [sanitizer] Add hexagon support to sanitizer-common.Aug 16 2021, 5:48 PM
bcain retitled this revision from [PATCH 5/8] [sanitizer] Add hexagon support to sanitizer-common to [PATCH 1/8] [sanitizer] Add hexagon support to sanitizer-common.
vitalybuka accepted this revision.Aug 17 2021, 10:00 AM

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.

This revision is now accepted and ready to land.Aug 17 2021, 10:00 AM
vitalybuka requested changes to this revision.Aug 17 2021, 10:01 AM

Sorry, approved accidentally.

This revision now requires changes to proceed.Aug 17 2021, 10:01 AM
vitalybuka added a comment.EditedAug 17 2021, 10:14 AM

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.
If you apply it it locally make sure to use -style=file.

Actually this place is ugly. Current .clang-format will indent when the rest of the block is not.
So up to you, keep it as-is, or clang-format entire block 152-165 (ideally in a separate patch)

compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp
25–26

but please fix ones like this which do not require significant reformatting of unchanged code.

We can help create a bot for hexagon-unknown-linux-musl. What's the process?

Thank you. https://llvm.org/docs/HowToAddABuilder.html

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–26

Ok that's no problem.

bcain added inline comments.Aug 17 2021, 10:48 AM
compiler-rt/lib/sanitizer_common/sanitizer_stacktrace.cpp
25–26

but please fix ones like this which do not require significant reformatting of unchanged code.

Ok that's no problem.

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.

bcain retitled this revision from [PATCH 1/8] [sanitizer] Add hexagon support to sanitizer-common to [sanitizer] Add hexagon support to sanitizer-common.Aug 17 2021, 11:12 AM
bcain updated this revision to Diff 366995.Aug 17 2021, 12:59 PM

We can help create a bot for hexagon-unknown-linux-musl. What's the process?

Thank you. https://llvm.org/docs/HowToAddABuilder.html

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.

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–26

Goal is to avoid manual labor as much as possible even if clang-format result are a little bit inconsistent.
I guess code around was added before we started using clang-format.

The last snapshot uploaded without "arc diff", so it does not show "context" code around, which is inconvenient for review.

bcain updated this revision to Diff 367008.Aug 17 2021, 2:10 PM

added context

vitalybuka added inline comments.Aug 17 2021, 2:14 PM
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
94

please use clang-format version here.
I don't mind if you blindly clang-format entire patch.

compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc
89

should this use u64 like other __internal_syscall files?

125

Why it's not -4095 like in other versions?

bcain added inline comments.Aug 17 2021, 2:27 PM
compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc
89

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.

bcain updated this revision to Diff 367020.Aug 17 2021, 2:42 PM

applied clang-format-diff, s/long/u32/ for syscall funcs.

bcain updated this revision to Diff 367027.Aug 17 2021, 3:00 PM

fixed iserror()

compiler-rt/lib/sanitizer_common/sanitizer_syscall_linux_hexagon.inc
125

Why it's not -4095 like in other versions?

It should be, I misunderstood the intent.

vitalybuka accepted this revision.Aug 17 2021, 4:40 PM

LGTM, but maybe wait for @sidneym feedback, or add as "blocking reviewer".

This revision is now accepted and ready to land.Aug 17 2021, 4:40 PM