This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add -fsanitize=memtag-globals (no-op).
ClosedPublic

Authored by hctim on Jun 6 2022, 3:46 PM.

Details

Summary

Adds the -fsanitize plumbing for memtag-globals. Makes -fsanitize=memtag
imply -fsanitize=memtag-globals.

This has no effect on codegen for now.

Diff Detail

Event Timeline

hctim created this revision.Jun 6 2022, 3:46 PM
Herald added a project: Restricted Project. · View Herald Transcript
hctim requested review of this revision.Jun 6 2022, 3:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2022, 3:46 PM
eugenis added inline comments.Jun 7 2022, 12:24 PM
clang/lib/Driver/ToolChains/Linux.cpp
767 ↗(On Diff #434635)

Hmm why are all the other memtag* not here?

clang/lib/Sema/SemaDeclAttr.cpp
7892 ↗(On Diff #434635)

Maybe "applies to globals"? On the other hand, MSan "applies" to globals but does not need this logic.

isSanitizerAttributeAllowedOnGlobals?

Why do we want this as a separate -fsanitize=
Maybe better to have some modifier flag like -fsanitize-memtag-globals=1 ?

I don't have a lot of arguments either way, do you?
Ignorelist support is one for the new sanitizer type.

hctim added a comment.Jun 7 2022, 3:20 PM

We already have -fsanitize=memtag that implies -fsanitize=memtag-heap and -fsanitize=memtag-stack.

It makes the most sense IMHO in the world where we want heap MTE. -fsanitize=memtag-heap is the most reasonable instead of -fsanitize-memtag-heap, as "do i need -fsanitize=memtag as well, like MSan?" is never a question.

We already have -fsanitize=memtag that implies -fsanitize=memtag-heap and -fsanitize=memtag-stack.

It makes the most sense IMHO in the world where we want heap MTE. -fsanitize=memtag-heap is the most reasonable instead of -fsanitize-memtag-heap, as "do i need -fsanitize=memtag as well, like MSan?" is never a question.

I see, thanks for explaining.

hctim marked 2 inline comments as done.Jun 10 2022, 3:34 PM
hctim added inline comments.
clang/lib/Driver/ToolChains/Linux.cpp
767 ↗(On Diff #434635)

Yeah, just realised this isn't necessary, looks like it's covered in clang/lib/Driver/ToolChain.cpp:1088 (the entire bitset of MemTag, which is MemtagStack | MemtagGlobals | MemtagHeap).

clang/lib/Sema/SemaDeclAttr.cpp
7892 ↗(On Diff #434635)

yeah, isSanitizerAttributeAllowedOnGlobals sounds good to me. also pulled this over to a different change adding support for __attribute__((no_sanitize("hwaddress"))): D127544

hctim updated this revision to Diff 436059.Jun 10 2022, 3:34 PM
hctim marked 2 inline comments as done.

update

eugenis accepted this revision.Jun 14 2022, 4:45 PM

LGTM

This revision is now accepted and ready to land.Jun 14 2022, 4:45 PM
This revision was landed with ongoing or failed builds.Jun 15 2022, 10:08 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 15 2022, 2:44 PM

FYI: If you change serialized opts in a def file, you have to bump the pch version, else with builds that don't embed the llvm revision (LLVM_APPEND_VC_REV=OFF) won't notice they have to invalidate pchs.

(In this case, I did this in https://github.com/llvm/llvm-project/commit/307109266f6c7598dfc69b6388fa271662de9388 for this change.)

hctim added a comment.Jun 15 2022, 2:57 PM

FYI: If you change serialized opts in a def file, you have to bump the pch version, else with builds that don't embed the llvm revision (LLVM_APPEND_VC_REV=OFF) won't notice they have to invalidate pchs.

(In this case, I did this in https://github.com/llvm/llvm-project/commit/307109266f6c7598dfc69b6388fa271662de9388 for this change.)

Thanks!