[ubsan-minimal] Switch to weak symbols for callbacks to allow overriding in client code #119242

kstoimenov · 2024-12-09T18:21:11Z

No description provided.

llvmbot · 2024-12-09T18:21:48Z

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Kirill Stoimenov (kstoimenov)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/119242.diff

1 Files Affected:

(modified) compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp (+3-2)

diff --git a/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp b/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
index 53a3273e4e6980..aa91a66cb6d32e 100644
--- a/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
+++ b/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp
@@ -97,8 +97,9 @@ constexpr unsigned kAddrBuf = SANITIZER_WORDSIZE / 4;
 #define MSG_BUF_LEN(msg) (sizeof(MSG_TMPL(msg)) + kAddrBuf + 1)
 
 #define HANDLER_RECOVER(name, msg)                               \
-  INTERFACE void __ubsan_handle_##name##_minimal() {             \
-    uintptr_t caller = GET_CALLER_PC();                  \
+  SANITIZER_INTERFACE_WEAK_DEF(                                  \
+    void, __ubsan_handle_##name##_minimal, void) {               \
+    uintptr_t caller = GET_CALLER_PC();                          \
     if (!report_this_error(caller)) return;                      \
     char msg_buf[MSG_BUF_LEN(msg)] = MSG_TMPL(msg);              \
     decorate_msg(MSG_TMPL_END(msg_buf, msg), caller);            \

github-actions · 2024-12-09T18:25:11Z

✅ With the latest revision this PR passed the C/C++ code formatter.

vitalybuka · 2024-12-09T23:16:46Z

compiler-rt/test/ubsan_minimal/TestCases/override-callback.c

+#include <stdio.h>
+#include <stdint.h>
+
+void __ubsan_handle_implicit_conversion_minimal() {


I think it's going to be hard to maintain on client size, when we add more types of checks

I believe it should be single function with simple interface.

Maybe just existing:
void SANITIZER_CDECL __sanitizer_report_error_summary(const char *error_summary);

With exactly the same format of output.
"SUMMARY: ubsan: morestuff"

I renamed report_this_error to __sanitizer_report_ubsan_error so we have a single handler now. PTAL.

There is function declared as:

void SANITIZER_CDECL __sanitizer_report_error_summary(const char *error_summary);

if we abandon callerpc we can use that one
Do you see good usecase of it?

Maybe just

INTERFACE void __ubsan_handle_##name##_minimal() { \ uintptr_t caller = GET_CALLER_PC(); \ if (!report_this_error(caller)) \ return; \ char msg_buf[MSG_BUF_LEN(msg)] = MSG_TMPL(msg); \ decorate_msg(MSG_TMPL_END(msg_buf, msg), caller); \ message(msg_buf); \ __sanitizer_report_error_summary(msg_buf); \ }

Actually just

NTERFACE void __ubsan_handle_##name##_minimal() { \ uintptr_t caller = GET_CALLER_PC(); \ if (!report_this_error(caller)) \ return; \ char msg_buf[MSG_BUF_LEN(msg)] = MSG_TMPL(msg); \ decorate_msg(MSG_TMPL_END(msg_buf, msg), caller); \ __sanitizer_report_error_summary(msg_buf); \ }

With default implementation of __sanitizer_report_error_summary as write

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

vitalybuka · 2024-12-12T20:45:32Z

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

-    uintptr_t caller = GET_CALLER_PC();                  \
-    if (!report_this_error(caller)) return;                      \
+    uintptr_t caller = GET_CALLER_PC();                          \
+    if (!__sanitizer_report_ubsan_error(caller, #name))          \


we want msg, not name

We will need the name if we want to increment a counter. The way I am thinking about this is that we will have a 2 dimensional hash map and the logic will be to increment error_count[caller][name]. We additionally could pass the message also.

vitalybuka · 2024-12-12T23:52:44Z

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

@@ -20,20 +20,21 @@ static __sanitizer::atomic_uintptr_t caller_pcs[kMaxCallerPcs];
 // that "too many errors" has already been reported.
 static __sanitizer::atomic_uint32_t caller_pcs_sz;

-__attribute__((noinline)) static bool report_this_error(uintptr_t caller) {
+SANITIZER_INTERFACE_WEAK_DEF(void, __ubsan_report_error, uintptr_t caller,


maybe sinc into __ubsan_report_error

char msg_buf[MSG_BUF_LEN(msg)] = MSG_TMPL(msg); \ decorate_msg(MSG_TMPL_END(msg_buf, msg), caller);

and we can remove decorated_msg

We can't because it uses compile time string lengths: MSG_BUF_LEN(msg) is a constant.

and lets flip orcer
__ubsan_report_error(caller, msg); -> __ubsan_report_error(kind, caller);

vitalybuka · 2024-12-13T19:56:09Z

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp

-}
+    char msg_buf[128] = MSG_PREFIX;
+    const char *end = msg_buf + sizeof(msg_buf);
+    char *p = append_str(msg, msg_buf + sizeof(MSG_PREFIX) - 1, end);


insted of sizeof(MSG_PREFIX) - 1, making

char msg_buf[128]; p = msg_buf; p = append_str(PREFIX, p, end); p = append_str(msg, p, end);

let's keep decorate_msg all printing tu
decorate_msg(buff, kind ,caller)

and just call decorate_msg twice for android, so we cant remove abort parameter

#119920) This refactoring will allow to make this function weak later on so that it could be overloaded by a client. See #119242.

in client code.

llvmbot added compiler-rt compiler-rt:ubsan Undefined behavior sanitizer compiler-rt:sanitizer labels Dec 9, 2024

kstoimenov requested a review from vitalybuka December 9, 2024 18:23

vitalybuka reviewed Dec 9, 2024

View reviewed changes

vitalybuka reviewed Dec 12, 2024

View reviewed changes

compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp Show resolved Hide resolved

vitalybuka reviewed Dec 12, 2024

View reviewed changes

vitalybuka approved these changes Dec 13, 2024

View reviewed changes

vitalybuka reviewed Dec 13, 2024

View reviewed changes

kstoimenov mentioned this pull request Dec 13, 2024

[nfc][ubsan-minimal] Refactor error reporting to use a single function #119920

Merged

kstoimenov added a commit that referenced this pull request Dec 13, 2024

[nfc][ubsan-minimal] Refactor error reporting to use a single function (

e5e0f23

#119920) This refactoring will allow to make this function weak later on so that it could be overloaded by a client. See #119242.

kstoimenov closed this Dec 13, 2024

kstoimenov force-pushed the minrt-callbacks branch from 66e7129 to 3273d0b Compare December 13, 2024 21:48

[UBSAN-MINRT] Switch to weak symbols for callbacks to allow overriding

6043590

in client code.

kstoimenov reopened this Dec 13, 2024

kstoimenov added 3 commits December 13, 2024 22:01

Repatch.

8885f82

Added a test.

189941c

Fixed the test.

67a7658

vitalybuka approved these changes Dec 13, 2024

View reviewed changes

kstoimenov added 2 commits December 13, 2024 22:59

clang-format

ce65b1c

msg -> kind

c10a299

kstoimenov merged commit 71d2fa7 into llvm:main Dec 13, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ubsan-minimal] Switch to weak symbols for callbacks to allow overriding in client code #119242

[ubsan-minimal] Switch to weak symbols for callbacks to allow overriding in client code #119242

kstoimenov commented Dec 9, 2024

llvmbot commented Dec 9, 2024

github-actions bot commented Dec 9, 2024 •

edited

Loading

vitalybuka Dec 9, 2024 •

edited

Loading

kstoimenov Dec 11, 2024

vitalybuka Dec 12, 2024

vitalybuka Dec 12, 2024 •

edited

Loading

vitalybuka Dec 12, 2024

vitalybuka Dec 12, 2024

kstoimenov Dec 12, 2024

vitalybuka Dec 12, 2024

vitalybuka Dec 12, 2024

kstoimenov Dec 12, 2024

vitalybuka Dec 12, 2024

vitalybuka Dec 13, 2024 •

edited

Loading

vitalybuka Dec 13, 2024

[ubsan-minimal] Switch to weak symbols for callbacks to allow overriding in client code #119242

[ubsan-minimal] Switch to weak symbols for callbacks to allow overriding in client code #119242

Conversation

kstoimenov commented Dec 9, 2024

llvmbot commented Dec 9, 2024

github-actions bot commented Dec 9, 2024 • edited Loading

vitalybuka Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitalybuka Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitalybuka Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 9, 2024 •

edited

Loading

vitalybuka Dec 9, 2024 •

edited

Loading

vitalybuka Dec 12, 2024 •

edited

Loading

vitalybuka Dec 13, 2024 •

edited

Loading