Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions compiler-rt/lib/radsan/radsan_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <sanitizer_common/sanitizer_allocator_internal.h>
#include <sanitizer_common/sanitizer_stacktrace.h>

#include <new>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transitive dependency pulled in by functional, now has to be explicit

#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
Expand All @@ -29,11 +30,6 @@ void internalFree(void *ptr) { InternalFree(ptr); }

namespace radsan {

Context::Context() : Context(createErrorActionGetter()) {}

Context::Context(std::function<OnErrorAction()> get_error_action)
: get_error_action_(get_error_action) {}

void Context::realtimePush() { realtime_depth_++; }

void Context::realtimePop() { realtime_depth_--; }
Expand All @@ -46,7 +42,7 @@ void Context::expectNotRealtime(const char *intercepted_function_name) {
if (inRealtimeContext() && !isBypassed()) {
bypassPush();
printDiagnostics(intercepted_function_name);
if (get_error_action_() == OnErrorAction::ExitWithFailure) {
if (radsan::ShouldExit()) {
exit(EXIT_FAILURE);
}
bypassPop();
Expand Down
6 changes: 0 additions & 6 deletions compiler-rt/lib/radsan/radsan_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,10 @@

#include <radsan/radsan_user_interface.h>

#include <functional>

namespace radsan {

class Context {
public:
Context();
Context(std::function<OnErrorAction()> get_error_action);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seemed to me like a way to do dependency injection. If we want to keep the ability to do something like that, I could introduce a bool shouldExitOverride or similar that would allow the unit test to inject it's own way of handling the error?

See the reworked unit test for more info


void realtimePush();
void realtimePop();

Expand All @@ -34,7 +29,6 @@ class Context {

int realtime_depth_{0};
int bypass_depth_{0};
std::function<OnErrorAction()> get_error_action_;
};

Context &getContextForThisThread();
Expand Down
32 changes: 13 additions & 19 deletions compiler-rt/lib/radsan/radsan_user_interface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,39 +17,33 @@

namespace radsan {

std::function<OnErrorAction()> createErrorActionGetter() {
auto const continue_getter = []() { return OnErrorAction::Continue; };
auto const exit_getter = []() { return OnErrorAction::ExitWithFailure; };
auto const interactive_getter = []() {
auto response = char{};
bool ShouldExit() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this function no longer returns a function, but just returns directly whether or not we should exit.


std::cout << "Continue? (Y/n): ";
std::cin >> std::noskipws >> response;
const bool defaultShouldExit = true;

if (std::toupper(response) == 'N')
return OnErrorAction::ExitWithFailure;
else
return OnErrorAction::Continue;
};

auto user_mode = __sanitizer::GetEnv("RADSAN_ERROR_MODE");
static const char* user_mode = __sanitizer::GetEnv("RADSAN_ERROR_MODE");
if (user_mode == nullptr) {
return exit_getter;
return defaultShouldExit;
}

if (std::strcmp(user_mode, "interactive") == 0) {
return interactive_getter;
auto response = char{};

std::cout << "Continue? (Y/n): ";
std::cin >> std::noskipws >> response;

return std::toupper(response) == 'N';
}

if (std::strcmp(user_mode, "continue") == 0) {
return continue_getter;
return false;
}

if (std::strcmp(user_mode, "exit") == 0) {
return exit_getter;
return true;
}

return exit_getter;
return defaultShouldExit;
}

} // namespace radsan
11 changes: 2 additions & 9 deletions compiler-rt/lib/radsan/radsan_user_interface.h
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
#pragma once

#include <functional>

namespace radsan {

enum class OnErrorAction {
Continue,
ExitWithFailure,
};

std::function<OnErrorAction()> createErrorActionGetter();
bool ShouldExit();

}
} // namespace radsan
8 changes: 4 additions & 4 deletions compiler-rt/lib/radsan/tests/radsan_test_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,14 @@ TEST(TestRadsanContext,
}

TEST(TestRadsanContext, onlyDiesIfExitWithFailureReturnedFromUser) {
auto fake_action = radsan::OnErrorAction::Continue;
auto action_getter = [&fake_action]() { return fake_action; };

auto context = radsan::Context{action_getter};
setenv("RADSAN_ERROR_MODE", "continue", 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of injecting the action_getter, we skip the middle man and change the environment variables.

This makes this test a little less unit-test, and a little more integration test, for better or for worse.


auto context = radsan::Context{};
context.realtimePush();

context.expectNotRealtime("do_some_stuff_expecting_continue");

fake_action = radsan::OnErrorAction::ExitWithFailure;
setenv("RADSAN_ERROR_MODE", "exit", 1);
EXPECT_DEATH(context.expectNotRealtime("do_some_stuff_expecting_exit"), "");
}