Skip to content

[scudo] Add __scudo_get_info symbol to export stats to a buffer. #130273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
7 changes: 7 additions & 0 deletions compiler-rt/lib/scudo/standalone/combined.h
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,7 @@ class Allocator {
// sizing purposes.
uptr getStats(char *Buffer, uptr Size) {
ScopedString Str;
// TODO: Use Str.copyToBuffer and fail when Buffer is NULL.
const uptr Length = getStats(&Str) + 1;
if (Length < Size)
Size = Length;
Expand All @@ -654,6 +655,12 @@ class Allocator {
Str.output();
}

uptr getFragmentationInfo(char *Buffer, uptr Size) {
ScopedString Str;
Primary.getFragmentationInfo(&Str);
return Str.copyToBuffer(Buffer, Size);
}

void printFragmentationInfo() {
ScopedString Str;
Primary.getFragmentationInfo(&Str);
Expand Down
17 changes: 17 additions & 0 deletions compiler-rt/lib/scudo/standalone/include/scudo/interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,23 @@ __attribute__((weak)) void __scudo_realloc_deallocate_hook(void *old_ptr);

void __scudo_print_stats(void);

// Reports all allocators configuration and general statistics as a null
// terminated text string.
#ifndef M_INFO_TOPIC_STATS
#define M_INFO_TOPIC_STATS 1
#endif

// Reports fragmentation statistics of the primary allocation as a null
// terminated text string.
#ifndef M_INFO_TOPIC_FRAGMENTATION
#define M_INFO_TOPIC_FRAGMENTATION 2
#endif

// Writes allocator statistics to the buffer, truncating to the specified size
// if necessary. Returns the full report size (before truncation) for buffer
// sizing purpose, or zero if the topic is not supported.
size_t __scudo_get_info(uint32_t topic, void *buffer, size_t size);
Copy link
Contributor

Choose a reason for hiding this comment

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

All parameters are capitalized.

Copy link
Author

Choose a reason for hiding this comment

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

It looks like parameters use snake_case in the interface.h. Could you please confirm that we want to Capitalize?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, we have so many different cases. We should probably change this in the future, but your interface is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought I had a comment about this, but I may have missed your reply.

Is there a reason to have this as one function and not two different functions that do different things? For example, are you planning to add more info types in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I want to allow for future topic additions without altering the binary interface. This approach enables us to experiment with new topics and validate their usefulness before formally documenting them. It also provides a graceful way to deprecate topics without breaking existing callers or cluttering the symbol table.


typedef void (*iterate_callback)(uintptr_t base, size_t size, void *arg);

// Determine the likely cause of a tag check fault or other memory protection
Expand Down
10 changes: 10 additions & 0 deletions compiler-rt/lib/scudo/standalone/string_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,16 @@ void ScopedString::append(const char *Format, ...) {
va_end(Args);
}

size_t ScopedString::copyToBuffer(char *OutputBase, size_t OutputLength) {
DCHECK(OutputBase);
if (OutputLength) {
const size_t Written = Min(length(), OutputLength - 1);
memcpy(OutputBase, data(), Written);
OutputBase[Written] = '\0';
}
return length() + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should return the length of data copied?

Copy link
Author

Choose a reason for hiding this comment

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

Here I tried to conform to existing APIs, they are a bit surprising, but it makes sense.

In combined.h, getStats uses that protocol, where the caller can provide a buffer, and if it is too small, the value gets truncated, and the caller knows it from the returned size. I reuse the same principle here.

}

void Printf(const char *Format, ...) {
va_list Args;
va_start(Args, Format);
Expand Down
9 changes: 7 additions & 2 deletions compiler-rt/lib/scudo/standalone/string_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ namespace scudo {
class ScopedString {
public:
explicit ScopedString() { String.push_back('\0'); }
uptr length() { return String.size() - 1; }
const char *data() { return String.data(); }
uptr length() const { return String.size() - 1; }
const char *data() const { return String.data(); }
void clear() {
String.clear();
String.push_back('\0');
Expand All @@ -31,6 +31,11 @@ class ScopedString {
void reserve(size_t Size) { String.reserve(Size + 1); }
uptr capacity() { return String.capacity() - 1; }

// Copies the string to the buffer, truncating if necessary.
// Null-terminates the output if output_length is greater than zero.
// Returns the original string's size (including null).
size_t copyToBuffer(char *OutputBase, size_t OutputLength);

private:
void appendNumber(u64 AbsoluteValue, u8 Base, u8 MinNumberLength,
bool PadWithZero, bool Negative, bool Upper);
Expand Down
6 changes: 6 additions & 0 deletions compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,12 @@ void ScudoCombinedTest<Config>::BasicTest(scudo::uptr SizeLog) {

Allocator->printStats();
Allocator->printFragmentationInfo();

{
char buffer[256] = {0};
EXPECT_LE(0, Allocator->getStats(buffer, sizeof(buffer)));
EXPECT_LE(0, Allocator->getFragmentationInfo(buffer, sizeof(buffer)));
Comment on lines +339 to +340
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change these to EXPECT_GT(Allocator->..., 0);

Same as the uses in the tests

Copy link
Author

Choose a reason for hiding this comment

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

Done.

}
}

#define SCUDO_MAKE_BASIC_TEST(SizeLog) \
Expand Down
43 changes: 43 additions & 0 deletions compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,49 @@ TEST(ScudoStringsTest, Padding) {
testAgainstLibc<int>("%03d - %03d", -12, -1234);
}

TEST(ScudoStringsTest, CopyFromAnEmptyString) {
scudo::ScopedString Str;
char buf[256] = {'0', '1'};
EXPECT_EQ(1U, Str.copyToBuffer(buf, sizeof(buf)));
EXPECT_STREQ("", buf);
EXPECT_EQ(0, buf[0]); // Rest of the buffer remains unchanged.
}

TEST(ScudoStringsTest, CopyFromAnEmptyStringIntoZeroSizeBuffer) {
scudo::ScopedString Str;
char buf[256] = {'0', '1'};
EXPECT_EQ(1U, Str.copyToBuffer(buf, 0));
EXPECT_EQ('0', buf[0]); // Nothing changed because provided size is 0.
}

TEST(ScudoStringsTest, CopyIntoLargeEnoughBuffer) {
scudo::ScopedString Str;
Str.append("abc");
char buf[256] = {'0', '1', '2', '3', '4', '5'};
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that we don't need to init the buf with string nubmers

Copy link
Author

Choose a reason for hiding this comment

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

I forgot to check for overflow. Fixed.

// Size includes terminal null.
EXPECT_EQ(4U, Str.copyToBuffer(buf, sizeof(buf)));
EXPECT_STREQ("abc", buf);
EXPECT_EQ(buf[4], '4');
}

TEST(ScudoStringsTest, CopyWithTextOverflow) {
scudo::ScopedString Str;
Str.append("abc");
char buf[256] = {'0', '1', '2', '3', '4', '5'};
EXPECT_EQ(4U, Str.copyToBuffer(buf, 3));
EXPECT_STREQ("ab", buf);
EXPECT_EQ(buf[3], '3');
}

TEST(ScudoStringsTest, CopyIntoExactFit) {
scudo::ScopedString Str;
Str.append("abc");
char buf[256] = {'0', '1', '2', '3', '4', '5'};
EXPECT_EQ(4U, Str.copyToBuffer(buf, 4));
EXPECT_STREQ("abc", buf);
EXPECT_EQ(buf[4], '4');
}

#if defined(__linux__)

#include <sys/resource.h>
Expand Down
16 changes: 16 additions & 0 deletions compiler-rt/lib/scudo/standalone/wrappers_c.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "internal_defs.h"
#include "platform.h"
#include "scudo/interface.h"
#include "string_utils.h"
#include "wrappers_c.h"
#include "wrappers_c_checks.h"

Expand All @@ -37,4 +38,19 @@ scudo::Allocator<scudo::Config, SCUDO_PREFIX(malloc_postinit)> SCUDO_ALLOCATOR;

extern "C" INTERFACE void __scudo_print_stats(void) { Allocator.printStats(); }

extern "C" INTERFACE size_t __scudo_get_info(uint32_t topic, void *buffer,
size_t size) {
switch (topic) {
case M_INFO_TOPIC_STATS:
return Allocator.getStats(reinterpret_cast<char *>(buffer), size);
case M_INFO_TOPIC_FRAGMENTATION:
return Allocator.getFragmentationInfo(reinterpret_cast<char *>(buffer),
size);
default:
scudo::Printf("Scudo WARNING: unrecognized __scudo_get_info topic: %d\n",
topic);
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want a default case here and below?

Copy link
Author

Choose a reason for hiding this comment

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

I can rewrite with a default case. Is it better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think it's better to warn any invalid options than silent it

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Now it prints a warning message.

}

#endif // !SCUDO_ANDROID || !_BIONIC
15 changes: 15 additions & 0 deletions compiler-rt/lib/scudo/standalone/wrappers_c_bionic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "internal_defs.h"
#include "platform.h"
#include "scudo/interface.h"
#include "string_utils.h"
#include "wrappers_c.h"
#include "wrappers_c_checks.h"

Expand All @@ -38,6 +39,20 @@ static scudo::Allocator<scudo::Config, SCUDO_PREFIX(malloc_postinit)>
// TODO(kostyak): support both allocators.
INTERFACE void __scudo_print_stats(void) { Allocator.printStats(); }

INTERFACE size_t __scudo_get_info(uint32_t topic, void *buffer, size_t size) {
switch (topic) {
case M_INFO_TOPIC_STATS:
return Allocator.getStats(reinterpret_cast<char *>(buffer), size);
case M_INFO_TOPIC_FRAGMENTATION:
return Allocator.getFragmentationInfo(reinterpret_cast<char *>(buffer),
size);
default:
scudo::Printf("Scudo WARNING: unrecognized __scudo_get_info topic: %d\n",
topic);
return 0;
}
}

INTERFACE void __scudo_get_error_info(
struct scudo_error_info *error_info, uintptr_t fault_addr,
const char *stack_depot, size_t stack_depot_size, const char *region_info,
Expand Down