DEMO: [rtsan] Support suppressions list #13

cjappl · 2024-09-27T16:46:04Z

No description provided.

cjappl · 2024-10-01T16:48:28Z

compiler-rt/lib/rtsan/rtsan_checks.inc

Do we like the names of these? What else would we call them

FunctionCall is "does this call exist anywhere in the stack"

Interceptor right now disables checks at the lowest level for a blocking function or an intercepted call by name (malloc or my_call_with_attr_blocking)

Do we want to have two different disabling for each of them?

If so, what are their names and strings?

Do we even want people to be able to disable [[blocking]] calls?

Yeah good questions. Here's my initial thoughts:

Q: Should we have different disabling keys for blocking, intercepted and stack functions?

A: I believe so, yes, for all three. I like to think we can improve on the names to make them more immediately understandable, even to people without a mental model of how the sanitizer works, but I'm struggling with interceptor. Here's my first iteration on the naming, but I don't think they're perfect at all, so keen to iterate more on them with you:

"improve" function-call to something more like call-stack-match

interceptor -> intercepted

add blocking

Q: Do we even want people to suppress [[blocking]] calls?

A: I think it could still be helpful, yes - I can imagine a situation where you're using a third-party library or re-using multi-threaded code in a single-threaded context (an noncontended spinlock here would always immediately grab a lock - the same reasoning here as it would apply to a normal mutex imho). I also worry that not suppressing them could potentially even be more confusing than otherwise.

Another question for you:

Q: Should we allow regex for intercepted and blocking? I can imagine a user might want to just suppress all pthread_.*, for example...

Having said that, regarding naming, if there is precedent for these sorts of names in other sanitizers, I appreciate there might be a good argument for matching those...

Q: Should we have different disabling keys for blocking, intercepted and stack functions?
A: I believe so, yes, for all three.

Sounds good, I'm in favor of this. I think the only reason to avoid any of it would be implementation/runtime complexity, but both of those are very tame. I think more atomic is better

"improve" function-call to something more like call-stack-match
interceptor -> intercepted
add blocking

I like these generally, I'll give it some thought and we can continue hashing it out on the end PR

_Q: Do we even want people to suppress [[blocking]] calls?
A: I think it could still be helpful, yes

👍

Another question for you:
Q: Should we allow regex for intercepted and blocking? I can imagine a user might want to just suppress all pthread_.*, for example...

Perhaps, but I would encourage punting on this until later on. The regex engine definitely will slow down the string matching. Before we go deeper with that I'd want to let the "basic" version prove to be inadequate for folks.

Basically "can we prove that we need it" before we implement something more complex. I think we are providing a lot of flexibility to the users already

One correction here (of myself):

Another question for you:
Q: Should we allow regex for intercepted and blocking? I can imagine a user might want to just suppress all pthread_.*, for example...

So by default (and without me knowing) we ARE using the regex engine for function matches. This is the function we use:

bool SuppressionContext::Match(const char *str, const char *type, Suppression **s) { can_parse_ = false; if (!HasSuppressionType(type)) return false; for (uptr i = 0; i < suppressions_.size(); i++) { Suppression &cur = suppressions_[i]; if (0 == internal_strcmp(cur.type, type) && TemplateMatch(cur.templ, str)) { *s = &cur; return true; } } return false; }

So early out for strcmp, then TemplateMatch, which is "regex light". It looks like it has a runtime of O(N), so it isn't more costly than just strcmp. We do both strcmp and TemplateMatch per function specified per type, so O(N_strlen*M_num_interceptor_suppressions).

Something to be aware of, that I don't think we will be able to get around -

If the user is using suppressions, the more suppressions they use the more that run-time is affected. We need to be clear that using a ScopedDisabler is the first tool they should reach for if at all possible, which has an O(1) runtime cost.

The hierarchy of speed:

ScopedDisabler (easy, early out, never compare strings, never unwind the stack) - O(1)

Function name suppressions, blocking and interceptors - O(N_strlen * M_suppressions_in_type)

Stack suppressions - O(N_strlen * M_suppressions_in_type * L_stack_frame_count)

(I think)

cjappl · 2024-10-01T16:50:08Z

compiler-rt/lib/rtsan/rtsan.cpp

-  return [info]() {
-    IncrementTotalErrorCount();
+static auto OnViolationAction(const BufferedStackTrace &stack,
+                              const DiagnosticsInfo &info) {


Note that now, when a violation occurs, ExpectNotRealtime passes in the stack and diagnostics info for more processing.

This function is probably best shown with whitespace turned off, I mostly just extracted things out of the lambda

cjappl · 2024-10-01T16:51:10Z

compiler-rt/lib/rtsan/rtsan_assertions.h

+
+private:
+  Context &context_;
+};


If we go this direction, I will split this into it's own review, likely putting this in rtsan_context.h

Could we do without this and use __rtsan::ScopedDisabler instead?

Hmm, I'm not sure if we have access to that construct here, but I'll investigate

The answer is that no compiler-rt libs include these headers, except in tests. If you do a grep such as:

> rg "ubsan_interface.h" --glob "!*test*" llvm/utils/gn/secondary/compiler-rt/include/BUILD.gn 25: "sanitizer/ubsan_interface.h", compiler-rt/include/sanitizer/ubsan_interface.h 1://===-- sanitizer/ubsan_interface.h -----------------------------*- C++ -*-===// 13:#ifndef SANITIZER_UBSAN_INTERFACE_H 14:#define SANITIZER_UBSAN_INTERFACE_H 32:#endif // SANITIZER_UBSAN_INTERFACE_H compiler-rt/include/CMakeLists.txt

This shows up blank for all interface files. Based on this (for better or worse) we are left with re-implementing anything in our "top level header" down here in the depths of the impl.

We can theorize how to improve this (perhaps for us and all sanitizers), but I'm going to keep this as is for now

cjappl · 2024-10-01T16:55:27Z

Overall, in this V2 I had two initial ways to go about it:

This way, preserving the idea of the "OnViolationAction" being separate from the question "Is this a violation" (achieved in ExpectNotRealtime, now weighing suppressions into account)
Moving back to the "old style" ExpectNotRealtime - a long imperative function call with both detection of a violation, and doing something about a violation.

The second is easy to imagine, so I opted to "show and tell" the first one. Interested to hear if you have a preference on one versus the other.

I'm wondering if there is any benefit to keeping this separate like this, or if we should compress into a big imperative function. (This is not intended to be leading, I find myself on the fence either way and would value your input)

davidtrevelyan · 2024-10-07T19:50:42Z

compiler-rt/lib/rtsan/rtsan_assertions.h

+
+private:
+  Context &context_;
+};


Could we do without this and use __rtsan::ScopedDisabler instead?

davidtrevelyan · 2024-10-07T19:57:45Z

compiler-rt/lib/rtsan/rtsan_checks.inc

Having said that, regarding naming, if there is precedent for these sorts of names in other sanitizers, I appreciate there might be a good argument for matching those...

davidtrevelyan · 2024-10-07T19:59:36Z

compiler-rt/lib/rtsan/rtsan_suppressions.cpp

+  for (uptr i = 0; i < stack.size && stack.trace[i]; i++) {
+    uptr addr = stack.trace[i];
+
+    if (suppression_ctx->HasSuppressionType(function_call_type)) {


Seems like there's quite a lot of code here that looks very similar to what's in asan's suppression implementation. Do you think it's worth abstracting anything in common into shared utility functions, or are they different enough to warrant their own implementations?

Good idea. I would recommend investigating this in a follow up review. I don't want to abstract too soon and muddy up this change in isolation.

This could be a good refactor follow-on.

davidtrevelyan · 2024-10-07T20:05:53Z

Overall, in this V2 I had two initial ways to go about it:

This way, preserving the idea of the "OnViolationAction" being separate from the question "Is this a violation" (achieved in ExpectNotRealtime, now weighing suppressions into account)

Moving back to the "old style" ExpectNotRealtime - a long imperative function call with both detection of a violation, and doing something about a violation.

The second is easy to imagine, so I opted to "show and tell" the first one. Interested to hear if you have a preference on one versus the other.

I'm wondering if there is any benefit to keeping this separate like this, or if we should compress into a big imperative function. (This is not intended to be leading, I find myself on the fence either way and would value your input)

I personally consider option 1. to have better separation of concerns and to be far easier to test (no need to EXPECT_DEATH, which forks the process, any more with a fake OnViolationAction), so I prefer that one at the moment. Flipping the question on its head - what do you see as the benefit of having the "old style" imperative function here?

cjappl · 2024-10-07T20:26:07Z

I personally consider option 1. to have better separation of concerns and to be far easier to test (no need to EXPECT_DEATH, which forks the process, any more with a fake OnViolationAction), so I prefer that one at the moment. Flipping the question on its head - what do you see as the benefit of having the "old style" imperative function here?

This implementation is good with me 👍 I think it is the better of the two regarding separation of concerns etc

davidtrevelyan · 2024-10-07T20:31:30Z

I personally consider option 1. to have better separation of concerns and to be far easier to test (no need to EXPECT_DEATH, which forks the process, any more with a fake OnViolationAction), so I prefer that one at the moment. Flipping the question on its head - what do you see as the benefit of having the "old style" imperative function here?

This implementation is good with me 👍 I think it is the better of the two regarding separation of concerns etc

Nice - agreed 👍

cjappl

Thanks for the thoughts, greatly appreciated.

I am going to close this review, and put a review up against LLVM/main where we can continue discussing etc with your feedback incorporated

cjappl · 2024-10-07T20:30:40Z

compiler-rt/lib/rtsan/rtsan_checks.inc

Q: Should we have different disabling keys for blocking, intercepted and stack functions?
A: I believe so, yes, for all three.

Sounds good, I'm in favor of this. I think the only reason to avoid any of it would be implementation/runtime complexity, but both of those are very tame. I think more atomic is better

"improve" function-call to something more like call-stack-match
interceptor -> intercepted
add blocking

I like these generally, I'll give it some thought and we can continue hashing it out on the end PR

_Q: Do we even want people to suppress [[blocking]] calls?
A: I think it could still be helpful, yes

👍

Another question for you:
Q: Should we allow regex for intercepted and blocking? I can imagine a user might want to just suppress all pthread_.*, for example...

Perhaps, but I would encourage punting on this until later on. The regex engine definitely will slow down the string matching. Before we go deeper with that I'd want to let the "basic" version prove to be inadequate for folks.

Basically "can we prove that we need it" before we implement something more complex. I think we are providing a lot of flexibility to the users already

cjappl · 2024-10-07T20:32:16Z

compiler-rt/lib/rtsan/rtsan_suppressions.cpp

+  for (uptr i = 0; i < stack.size && stack.trace[i]; i++) {
+    uptr addr = stack.trace[i];
+
+    if (suppression_ctx->HasSuppressionType(function_call_type)) {


Good idea. I would recommend investigating this in a follow up review. I don't want to abstract too soon and muddy up this change in isolation.

This could be a good refactor follow-on.

… depobj construct (llvm#114221) A codegen crash is occurring when a depend object was initialized with omp_all_memory in the depobj directive. llvm#114214 The root cause of issue looks to be the improper handling of the dependency list when omp_all_memory was specified. The change introduces the use of OMPTaskDataTy to manage dependencies. The buildDependences function is called to construct the dependency list, and the list is iterated over to emit and store the dependencies. Reduced Test Case : ``` #include <omp.h> int main() { omp_depend_t obj; #pragma omp depobj(obj) depend(inout: omp_all_memory) } ``` ``` #1 0x0000000003de6623 SignalHandler(int) Signals.cpp:0:0 #2 0x00007f8e4a6b990f (/lib64/libpthread.so.0+0x1690f) #3 0x00007f8e4a117d2a raise (/lib64/libc.so.6+0x4ad2a) #4 0x00007f8e4a1193e4 abort (/lib64/libc.so.6+0x4c3e4) #5 0x00007f8e4a10fc69 __assert_fail_base (/lib64/libc.so.6+0x42c69) #6 0x00007f8e4a10fcf1 __assert_fail (/lib64/libc.so.6+0x42cf1) #7 0x0000000004114367 clang::CodeGen::CodeGenFunction::EmitOMPDepobjDirective(clang::OMPDepobjDirective const&) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x4114367) #8 0x00000000040f8fac clang::CodeGen::CodeGenFunction::EmitStmt(clang::Stmt const*, llvm::ArrayRef<clang::Attr const*>) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x40f8fac) #9 0x00000000040ff4fb clang::CodeGen::CodeGenFunction::EmitCompoundStmtWithoutScope(clang::CompoundStmt const&, bool, clang::CodeGen::AggValueSlot) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x40ff4fb) #10 0x00000000041847b2 clang::CodeGen::CodeGenFunction::EmitFunctionBody(clang::Stmt const*) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x41847b2) #11 0x0000000004199e4a clang::CodeGen::CodeGenFunction::GenerateCode(clang::GlobalDecl, llvm::Function*, clang::CodeGen::CGFunctionInfo const&) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x4199e4a) #12 0x00000000041f7b9d clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, llvm::GlobalValue*) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x41f7b9d) #13 0x00000000041f16a3 clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x41f16a3) #14 0x00000000041fd954 clang::CodeGen::CodeGenModule::EmitDeferred() (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x41fd954) realtime-sanitizer#15 0x0000000004200277 clang::CodeGen::CodeGenModule::Release() (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x4200277) realtime-sanitizer#16 0x00000000046b6a49 (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit(clang::ASTContext&) ModuleBuilder.cpp:0:0 realtime-sanitizer#17 0x00000000046b4cb6 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x46b4cb6) realtime-sanitizer#18 0x0000000006204d5c clang::ParseAST(clang::Sema&, bool, bool) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x6204d5c) realtime-sanitizer#19 0x000000000496b278 clang::FrontendAction::Execute() (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x496b278) realtime-sanitizer#20 0x00000000048dd074 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x48dd074) realtime-sanitizer#21 0x0000000004a38092 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0x4a38092) realtime-sanitizer#22 0x0000000000fd4e9c cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0xfd4e9c) realtime-sanitizer#23 0x0000000000fcca73 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) driver.cpp:0:0 realtime-sanitizer#24 0x0000000000fd140c clang_main(int, char**, llvm::ToolContext const&) (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0xfd140c) realtime-sanitizer#25 0x0000000000ee2ef3 main (/opt/cray/pe/cce/18.0.1/cce-clang/x86_64/bin/clang-18+0xee2ef3) realtime-sanitizer#26 0x00007f8e4a10224c __libc_start_main (/lib64/libc.so.6+0x3524c) realtime-sanitizer#27 0x0000000000fcaae9 _start /home/abuild/rpmbuild/BUILD/glibc-2.31/csu/../sysdeps/x86_64/start.S:120:0 clang: error: unable to execute command: Aborted ``` --------- Co-authored-by: Chandra Ghale <ghale@pe31.hpc.amslabs.hpecorp.net>

…onger cause a crash (llvm#116569) This PR fixes a bug introduced by llvm#110199, which causes any half float argument to crash the compiler on MIPS64. Currently compiling this bit of code with `llc -mtriple=mips64`: ``` define void @half_args(half %a) nounwind { entry: ret void } ``` Crashes with the following log: ``` LLVM ERROR: unable to allocate function argument #0 PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. Stack dump: 0. Program arguments: llc -mtriple=mips64 1. Running pass 'Function Pass Manager' on module '<stdin>'. 2. Running pass 'MIPS DAG->DAG Pattern Instruction Selection' on function '@half_args' #0 0x000055a3a4013df8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x32d0df8) #1 0x000055a3a401199e llvm::sys::RunSignalHandlers() (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x32ce99e) #2 0x000055a3a40144a8 SignalHandler(int) Signals.cpp:0:0 #3 0x00007f00bde558c0 __restore_rt libc_sigaction.c:0:0 #4 0x00007f00bdea462c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76 #5 0x00007f00bde55822 gsignal ./signal/../sysdeps/posix/raise.c:27:6 #6 0x00007f00bde3e4af abort ./stdlib/abort.c:81:7 #7 0x000055a3a3f80e3c llvm::report_fatal_error(llvm::Twine const&, bool) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x323de3c) #8 0x000055a3a2e20dfa (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x20dddfa) #9 0x000055a3a2a34e20 llvm::MipsTargetLowering::LowerFormalArguments(llvm::SDValue, unsigned int, bool, llvm::SmallVectorImpl<llvm::ISD::InputArg> const&, llvm::SDLoc const&, llvm::SelectionDAG&, llvm::SmallVectorImpl<llvm::SDValue>&) const MipsISelLowering.cpp:0:0 #10 0x000055a3a3d896a9 llvm::SelectionDAGISel::LowerArguments(llvm::Function const&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30466a9) #11 0x000055a3a3e0b3ec llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c83ec) #12 0x000055a3a3e09e21 llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c6e21) #13 0x000055a3a2aae1ca llvm::MipsDAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) MipsISelDAGToDAG.cpp:0:0 #14 0x000055a3a3e07706 llvm::SelectionDAGISelLegacy::runOnMachineFunction(llvm::MachineFunction&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c4706) realtime-sanitizer#15 0x000055a3a3051ed6 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x230eed6) realtime-sanitizer#16 0x000055a3a35a3ec9 llvm::FPPassManager::runOnFunction(llvm::Function&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x2860ec9) realtime-sanitizer#17 0x000055a3a35ac3b2 llvm::FPPassManager::runOnModule(llvm::Module&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x28693b2) realtime-sanitizer#18 0x000055a3a35a499c llvm::legacy::PassManagerImpl::run(llvm::Module&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x286199c) realtime-sanitizer#19 0x000055a3a262abbb main (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x18e7bbb) realtime-sanitizer#20 0x00007f00bde3fc4c __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3 realtime-sanitizer#21 0x00007f00bde3fd05 call_init ./csu/../csu/libc-start.c:128:20 realtime-sanitizer#22 0x00007f00bde3fd05 __libc_start_main@GLIBC_2.2.5 ./csu/../csu/libc-start.c:347:5 realtime-sanitizer#23 0x000055a3a2624921 _start /builddir/glibc-2.39/csu/../sysdeps/x86_64/start.S:117:0 ``` This is caused by the fact that after the change, `f16`s are no longer lowered as `f32`s in calls. Two possible fixes are available: - Update calling conventions to properly support passing `f16` as integers. - Update `useFPRegsForHalfType()` to return `true` so that `f16` are still kept in `f32` registers, as before llvm#110199. This PR implements the first solution to not introduce any more ABI changes as llvm#110199 already did. As of what is the correct ABI for halfs, I don't think there is a correct answer. GCC doesn't support halfs on MIPS, and I couldn't find any information on old MIPS ABI manuals either.

cjappl added 2 commits September 27, 2024 09:45

[rtsan] Support suppressions list

bea3c6d

Move suppressions together

69a3f80

cjappl force-pushed the suppressions branch from 7b47971 to 69a3f80 Compare October 1, 2024 16:49

cjappl commented Oct 1, 2024

View reviewed changes

davidtrevelyan suggested changes Oct 7, 2024

View reviewed changes

cjappl commented Oct 7, 2024

View reviewed changes

cjappl closed this Oct 8, 2024

DEMO: [rtsan] Support suppressions list #13

DEMO: [rtsan] Support suppressions list #13

Uh oh!

Conversation

cjappl commented Sep 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtrevelyan Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjappl commented Oct 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtrevelyan commented Oct 7, 2024

Uh oh!

cjappl commented Oct 7, 2024

Uh oh!

davidtrevelyan commented Oct 7, 2024

Uh oh!

cjappl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davidtrevelyan Oct 7, 2024 •

edited

Loading