Attribute to disable RADSan

[cjappl]

I apologize in advance for the ravings of a madman.

This PR successfully introduces a way to mark a function as "ignored" by the realtime sanitizer. Taking on the job of radsan_on and radsan_off but without having to link a library in user code.

High level problem (as described realtime-sanitizer/rtsan#7)

You have no way of disabling RADSan without linking in the RADSan library, this is more complicated than disabling any other sanitizer.

Gold standard would be disabling inspection of a function/nested functions with an attribute:

__attribute__((no_sanitize("realtime")))
void* realtime_safe_malloc (size_t size)
{
    return malloc (size);
}

[[clang::realtime]] float* process (float* data)
{
    float *y = (float*) realtime_safe_maloc (sizeof (float));
    … 
}


This program would NOT throw an error when run. The user knows that for some reason this called function should be safe in a realtime context.

How I fixed it:

I added an additional attribute, realtime_bypass. When applied to a function, it adds one to the bypass stack, thus the function is ignored.

[[clang::realtime_bypass]] void* realtime_safe_malloc (size_t size)
{
    return malloc (size);
}

[[clang::realtime]] float* process (float* data)
{
    float *y = (float*) realtime_safe_maloc (sizeof (float));
    … 
}

The above program does not throw a sanitizer error when it is run

Additionally, with a little hackery this also gets us the intended behavior:

__attribute__((no_sanitize("realtime")))
void* realtime_safe_malloc (size_t size)
{
    return malloc (size);
}

[[clang::realtime]] float* process (float* data)
{
    float *y = (float*) realtime_safe_maloc (sizeof (float));
    … 
}

The above program does not throw a sanitizer error when it is run

An example program to try out on your own machine can be found at this gist:
https://gist.github.com/cjappl/9a6f6dffcd36d5264816670afd39c6c0

Try deleting or introducing the new attributes.

Why did you not just use the “no_sanitize” syntax, as above? Why introduce an attribute?

There is a chicken and egg problem. In our code, to set the bounds of the realtime sanitizer, we have a block that looks like this:

  if (SanOpts.has(SanitizerKind::Realtime)) {       // # 1 
    if (Fn->hasFnAttribute(llvm::Attribute::AttrKind::Realtime)) {
       InsertRadsanExit(Fn); 
    }
  }

“When realtime sanitizer is running for this function, put an extra function call to push/pop the realtime sanitizer stack."

We would like to be able to say:
"If the realtime sanitizer is in scope (either me, or one of my ancestors has the realtime attribute), but I have it disabled, push a bypass into the stack"

If we disable the sanitizer with the attribute, it ensures that we never get in if #1. We will never get the opportunity to put a bypass in the code. We cannot put the bypass as an else case to if #1, as this would insert it for every single function clang generates for, which would be horrible for performance of compiling all programs.

To get around this, I added the check to see if the sanitizer was disabled. If that case is true, I add the bypass attribute to the function, and re-enable the sanitizer.

    if (no_sanitize_mask && SanitizerKind::Realtime)
    {
      // If we have turned off the sanitizer for this method, what we really want is to bypass
      // this function. Turn on the sanitizer, but add the attribute to bypass it.
      SanOpts.set(SanitizerKind::Realtime, true);
      Fn->addFnAttr(llvm::Attribute::RealtimeBypass);
    }

This gets us to a point that we can properly bypass when the function is generated later in the file.

To do this without the realtime_bypass attribute, we would need to have some way of telling if the current function or any of its ancestors are in a [[clang::realtime]] context. If that is the case, and we get the nosanitize attribute, then we can insert the radsan_off and radsan_on calls.

I DO NOT KNOW IF THIS IS POSSIBLE AT THIS POINT, the answer is eluding me.

[cjappl]

Some questions:

• Is behavior like this necessary for v1.0? If it is not, an approach like this definitely makes the eventual upstream PR more muddy
• The "nosanitize" tag is (in this approach) syntactic sugar. Is that necessary to include at all? We could get away with just the realtime_bypass tag. Nosanitize is probably the most expected behavior for users, but it comes at a cost of a little more complicated code here.
• is realtime_bypass the right name for this? realtime_safe?

[cjappl]

Alright, so there is definitely a lot of infrastructure for the suppressions file, but it will take a little doing.

In general, for the suppression file we would want to support an environment variable like TSAN (drawn from this page):

RADSAN_OPTIONS="suppressions=/my/suppressions/file" ./myprogram

This file would look like this:

suppression_type:suppression_string

Where type is a demarcation of what kind it is, and suppression string is an identifier like a function or a class method

From TSAN:

# ThreadSanitizer suppressions file for project X.

# Library foobar is full of races.
# Filed bug 123, but do not want to deal with it now.
race:foobar

# The function turns to be racy. Bug 345.
race:NuclearRocket::Launch

# The race is introduced in patch 456. Bug 567.
race:src/surgery/laser_scalpel.cc

# Global var global_var is racy. Bug 568.
race:global_var

# short() function is racy, but not match any other functions containing "short". Bug 569.
race:^short$

# The following thread leaks. Bug 678.
thread:MonitoringThread

# Uninstrumented library.
called_from_lib:libzmq.so

We could start with just one suppression type, or have a few like synchronization, allocation, file_io, socket_io, sleep.

I would suspect starting with just one would simplify the problem statement a little, then we could easily add the specific categories.

As for the code, (notes for myself or anyone else curious). The main suppression mechanism is a SuppressionContext in files sanitizer_suppresions.cpp/h. Test file lib/sanitizer_common/tests/sanitizer_suppressions_test.cpp

If you grep the code for the used class, you'll see it in a few of the sanitizers already:

lib/tsan/rtl/tsan_suppressions.cpp
45:ALIGNED(64) static char suppression_placeholder[sizeof(SuppressionContext)];
46:static SuppressionContext *suppression_ctx = nullptr;
55:      SuppressionContext(kSuppressionTypes, ARRAY_SIZE(kSuppressionTypes));
63:SuppressionContext *Suppressions() {

lib/ubsan/ubsan_diag.cpp
402:ALIGNED(64) static char suppression_placeholder[sizeof(SuppressionContext)];
403:static SuppressionContext *suppression_ctx = nullptr;
415:      SuppressionContext(kSuppressionTypes, ARRAY_SIZE(kSuppressionTypes));

lib/tsan/rtl/tsan_interceptors_posix.cpp
230:  const SuppressionContext &supp = *Suppressions();

LSAN seems to have a pretty simple concept of this (lsan_common.cpp), and may be a good template to borrow from.

We additionally need to extend lit tests to make sure this works.

In summary:

1. Write our own extension to the SuppressionContext class
2. Integrate them into our radsan_interceptors file
3. Write lit tests to make them go.

Seeing as I'm totally in the dark on a lot of this stuff, it seems doable but it would be a lot of stumbling around. Likely a weeks worth of work? Maybe less once you get in there and grok what's going on.

[davidtrevelyan]

Great investigations @cjappl - thanks for taking a look into it! Let's chat about it on a call, maybe? Lots to weigh up here!

[cjappl]

@davidtrevelyan left you comments in the code as to where to look specifically. I would also recommend reviewing my previous comments for some unhinged rambling on the subject.

[cjappl]

Do we like this name as "Realtime Bypass"? What other name should we use if not?

[davidtrevelyan]

As discussed on Discord, we like NoSanitizeRealtime, to match the existing NoSanitizeBounds and NoSanitizeCoverage

[cjappl]

Alright, this feels like a kind of hack here.

Basically the idea is what I described in the original description of this PR. On line 1573, if the realtime sanitizer is disabled, it is turned off. That sounds stupid to type out, but it's important.

If the realtime sanitizer is turned off, the block on line 1573 is never tripped, and we never get the opportunity to add the bypass call in.

Our options:

1. Don't respect the __nosanitize attribute, which would mean we just remove this entire change in this function
2. do this some other way?? Not sure how...

The psuedocode (implemented here) feels hacky:

This feels a little hacky because it's essentially saying "Do not ACTUALLY turn off the RADSan if the flag tells you to". This means that while SanOpts will be accurate for all other sanitizers, it will not be accurate for RADSan.

[davidtrevelyan]

After having a think, this doesn't feel that bad to me. Fundamentally what "no sanitize" means for a RADSan-sanitized function is different to what "no sanitize" means in other sanitizers. For RADSan, it's not really "turning it off" - it just means that we have to do something different: inserting the radsan_off() and radsan_on() calls! I'm not sure there's another way, as you imply as well. With a clear enough comment, I feel like this is a reasonable approach!

[davidtrevelyan]

This approach seems reasonable to me @cjappl! I think it'll be ready to go once we're happy with the naming and comments :)

[cjappl]

Why did I do 51 instead of 52? These orderings are very specific, it seems. I could not get them to play nicely if I appended this, so I put it at 51.

It seems like it compiles a bunch of little functions and checks the output is sane. All of the rest of them seem to be copy/paste but builtin is different somehow. This may be the reason it is last and should remain last

[cjappl]

@davidtrevelyan I recommend looking at my most recent commit to grok what the differences were. I will call out a couple.

1. Matching other nosanitize_*

I went through and grepped the rest of the source files for nosanitize_bounds etc. I went through and added nosanitize_realtime to match all these. This means updating some documentation, some tests, some emacs and vim helper files

2. Making this attribute "Private"

You'll note that I removed the definition of nosanitize_realtime from:

clang/include/clang/Basic/Attr.td

And instead, put it entirely in:

llvm/include/llvm/IR/Attributes.td

This is to match the other nosanitize_* options. You'll see they do not exist in "Basic" but do in "IR". However, this changed things for us a bit.

It is my understanding (could be wrong, this is the best conclusion I have) that the attributes in "Basic" are public facing attributes. This is the reason that clang::realtime exists in there. They are things that end end users can put in their code.

The stuff in "IR" are internal attributes in the "back end". They are not advertised or accessible to a user. The basic "nosanitize" option already exists in "Basic" but we need an internal attribute here to decorate the function for when it is codegen.

I am reasonably confident this is a good solution, as it matches the way the other ones (_bounds and _coverage) work. This was all to match this existing approach

As a consequence, can no longer mark a function as [[clang::realtime_bypass]] (or [[nosanitize_realtime]] because these are only defined in the back end. The ONLY option to the user is to use the attribute((nosanitize("realtime)) decorator as seen in the lit test. I have deleted the other test that used the other attribute, as it no longer compiles.

[cjappl]

I believe this solution to be good and complete! Take a look and see if you agree, happy to address any confusion or thoughts, otherwise ship it 🚢

[davidtrevelyan]

Wonderful - thanks @cjappl ! This looks really good to me. I have one suggested naming improvement, namely changing:

1. insertCallAtBeginning and
2. insertCallAtReturn

to something a tiny bit more descriptive, like

1. insertCallAtFunctionEntryPoint, and
2. insertCallAtAllFunctionExitPoints

With these tiny changes, I'm happy for this to be merged! Also, if you have other suggestions for the naming, very happy to discuss those :)

[cjappl]

Wonderful - thanks @cjappl ! This looks really good to me. I have one suggested naming improvement, namely changing:

1. `insertCallAtBeginning` and

2. `insertCallAtReturn`

to something a tiny bit more descriptive, like

1. `insertCallAtFunctionEntryPoint`, and

2. `insertCallAtAllFunctionExitPoints`

With these tiny changes, I'm happy for this to be merged! Also, if you have other suggestions for the naming, very happy to discuss those :)

Makes sense to me! Addressed.

[davidtrevelyan]

Wonderful, thanks again for this excellent contribution! Great work!