scudo-standalone: Add GetRSS method on Linux

[1c3t3a]

Summary

This change adds a GetRSS method on Linux that parses
the number from /proc/self/statm. This change is part
of splitting up https://reviews.llvm.org/D126752.

[1c3t3a]

Fix clang-format error

[1c3t3a]

Add implementation for GetRSS and GetRSSFromBuffer for different platforms.

[1c3t3a]

Only run tests on Linux

[vitalybuka]

compiler-rt/lib/scudo/standalone/linux.cpp

171

It looks unrelated

184–188

I don't see scudo uses internal_ names

184–192

192

206

looks wrong
and please use static_cast

compiler-rt/lib/scudo/standalone/tests/common_test.cpp

72

is this going to work on Android?
I assume SCUDO_LINUX is android as well?

[1c3t3a]

Add requested changes

[1c3t3a]

Thanks for the review! @vitalybuka :)

compiler-rt/lib/scudo/standalone/tests/common_test.cpp

72

Let me check that.

[1c3t3a]

Include check for procfile in test

[vitalybuka]

compiler-rt/lib/scudo/standalone/linux.cpp

184–192

static is not done?

198

It's correct, but () are redundant
I would prefer *(pos++) for better precendence visibility

or even just make it for loop

for (;*pos >= '0' && *pos <= '9'; ++pos)
   rss = rss * 10 + static_cast<u64>(*pos) - '0'

compiler-rt/lib/scudo/standalone/tests/common_test.cpp

74–83

can you please hide everything as static
and only test GetRSS()

you can e.g. new int[10000000], memset, and see that RSS before and after is ~10M

[1c3t3a]

Add requested changes

[1c3t3a]

This comment was removed by 1c3t3a.

[cryptoad]

Can we use the LLVM style for this change?
eg: no k* constants, casing for pos, fd etc.

[1c3t3a]

Adopt to LLVM naming rules

[1c3t3a]

In D139430#3981622, @cryptoad wrote:

Can we use the LLVM style for this change?
eg: no k* constants, casing for pos, fd etc.

Done :) I don't have commit access to LLVM, can you or someone else commit the patch for me?

[cryptoad]

Thanks! Missed one :)

compiler-rt/lib/scudo/standalone/internal_defs.h

93 ↗

(On Diff #481297)

LLVM naming + no C-cast please

[1c3t3a]

Fix missing C-cast

[vitalybuka]

remove unneded inlcudes and declarations
improve the test

[saghir]

Just a heads up that this seems to be breaking one of our bots: https://lab.llvm.org/staging/#/builders/14/builds/1305.

I am building LLVM to confirm this, will update in a bit.

[1c3t3a]

Oh. I think I know where this comes from - for fixing this: Would I submit to this changelist or open a new one?

[saghir]

In D139430#3984689, @1c3t3a wrote:

Oh. I think I know where this comes from - for fixing this: Would I submit to this changelist or open a new one?

Thanks for taking a look at this!
If it's a minor one line fix then you can just add another patch. If it needs significant changes to fix, then you can revert the patch, fix it and then commit it again...
If you were asking about using this phabricator review, I think this one is closed and a new one would be needed.
Let me know if you need any help.
Thanks again!

[vitalybuka]

I'll fix it in a few min.

In D139430#3984711, @saghir wrote:

In D139430#3984689, @1c3t3a wrote:

Oh. I think I know where this comes from - for fixing this: Would I submit to this changelist or open a new one?

Thanks for taking a look at this!
If it's a minor one line fix then you can just add another patch. If it needs significant changes to fix, then you can revert the patch, fix it and then commit it again...
If you were asking about using this phabricator review, I think this one is closed and a new one would be needed.
Let me know if you need any help.
Thanks again!

[amyk]

Hi there @vitalybuka @1c3t3a! I am now realizing that we are experiencing some intermittent failures with the GetRssFromBuffer test case (sometimes it fails, and sometimes it passes) on PPC.

Specifically, the EXPECT_LE() failure (when it does fail) will look like this, where the Rss - AllocSize - Prev can succeed the Error (3000000):

/home/amyk/community/llvm-project/compiler-rt/lib/scudo/standalone/tests/common_test.cpp:93: Failure
Expected: (std::abs(Rss - AllocSize - Prev)) <= (Error), actual: 3708544 vs 3000000

I am noticing a couple things with respect to this test case:

• This test can can fail on any iteration of the for loop (sometimes it can fail on the first iteration, other times the ninth, etc.)
• The amount that it fails with is always 3708544. It is always 708544 more than the expected error.
• I could be mistaken, but the Rss prior to the EXPECT_LE() check always appears to be less than Prev + AllocSize. Perhaps something is being deallocated here?

Basically, I'd like to try to understand this test case a bit more and was wondering if any of you have any thoughts with regards to this issue I am seeing on this test case.
Moreover, I was wondering if there was a specific justification behind having Error be 3000000, as we can see in my example, it is possible to exceed this value.

Any assistance and/or insight would be greatly appreciated! Thanks in advance.

[vitalybuka]

Anther way to test this is instead of failure on the first EXPECT_LE, we rather count mismatches and after the loop we check that count is e.g. <=3

[amyk]

In D139430#4127024, @vitalybuka wrote:

Anther way to test this is instead of failure on the first EXPECT_LE, we rather count mismatches and after the loop we check that count is e.g. <=3

That could be possible. Just curious, is 3 just an arbitrary value that we can pick, or is there any particular reason that we would pick that?

[vitalybuka]

it's totally made up: 3 out of 10

[amyk]

In D139430#4127089, @vitalybuka wrote:

it's totally made up: 3 out of 10

Thanks @vitalybuka for the explanation. Were we still planning to update this test case in the future?

[vitalybuka]

In D139430#4141870, @amyk wrote:

In D139430#4127089, @vitalybuka wrote:

it's totally made up: 3 out of 10

Thanks @vitalybuka for the explanation. Were we still planning to update this test case in the future?

I am not looking into that. Would you be interested to send a patch to improves the test for your platform?

[amyk]

In D139430#4143314, @vitalybuka wrote:

In D139430#4141870, @amyk wrote:

In D139430#4127089, @vitalybuka wrote:

it's totally made up: 3 out of 10

Thanks @vitalybuka for the explanation. Were we still planning to update this test case in the future?

I am not looking into that. Would you be interested to send a patch to improves the test for your platform?

Thanks for getting back to me, @vitalybuka! I've discussed this offline with a few others from our backend and we are deciding to disable this test on Power for the time being. I will follow up with a commit shortly for this.

[amyk]

In D139430#4147344, @amyk wrote:

In D139430#4143314, @vitalybuka wrote:

In D139430#4141870, @amyk wrote:

In D139430#4127089, @vitalybuka wrote:

it's totally made up: 3 out of 10

Thanks @vitalybuka for the explanation. Were we still planning to update this test case in the future?

I am not looking into that. Would you be interested to send a patch to improves the test for your platform?

Thanks for getting back to me, @vitalybuka! I've discussed this offline with a few others from our backend and we are deciding to disable this test on Power for the time being. I will follow up with a commit shortly for this.

I have committed this at 0e3ef5f89749a599c652c546214a8307e3495526.

compiler-rt/lib/scudo/standalone/common.h

inline uptr getPageSizeCached() {
   return getPageSizeSlow();
 }
 
 // Returns 0 if the number of CPUs could not be determined.
 u32 getNumberOfCPUs();
 
 const char *getEnv(const char *Name);
 
+u64 GetRSS();
+
 u64 getMonotonicTime();
 
 u32 getThreadID();
 
 // Our randomness gathering function is limited to 256 bytes to ensure we get
 // as many bytes as requested, and avoid interruptions (on Linux).
 constexpr uptr MaxRandomLength = 256U;
 bool getRandom(void *Buffer, uptr Length, bool Blocking = false);

compiler-rt/lib/scudo/standalone/common.cpp

formatString(
         "Scudo ERROR: internal map failure (NO MEMORY) requesting %zuKB\n",
         SizeIfOOM >> 10);
   }
   outputRaw(Error);
   setAbortMessage(Error);
   die();
 }
 
+#if !SCUDO_LINUX
+u64 GetRSS() { return 0; }
+#endif
+
 } // namespace scudo

compiler-rt/lib/scudo/standalone/linux.cpp

 #include "linux.h"
 #include "mutex.h"
 #include "string_utils.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <linux/futex.h>
 #include <sched.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/syscall.h>
 #include <sys/time.h>
 #include <time.h>
 #include <unistd.h>

 #endif
   // Up to 256 bytes, getrandom will not be interrupted.
   ReadBytes =
       syscall(SYS_getrandom, Buffer, Length, Blocking ? 0 : GRND_NONBLOCK);
   if (ReadBytes == static_cast<ssize_t>(Length))
     return true;
 #endif // defined(SYS_getrandom)
   // Up to 256 bytes, a read off /dev/urandom will not be interrupted.
   // Blocking is moot here, O_NONBLOCK has no effect when opening /dev/urandom.
   const int FileDesc = open("/dev/urandom", O_RDONLY);
   if (FileDesc == -1)
     return false;
   ReadBytes = read(FileDesc, Buffer, Length);
   close(FileDesc);
   return (ReadBytes == static_cast<ssize_t>(Length));
 }
 
 // Allocation free syslog-like API.
 extern "C" WEAK int async_safe_write_log(int pri, const char *tag,
                                          const char *msg);
 
+static u64 GetRSSFromBuffer(const char *Buf) {
+  // The format of the file is:
+  // 1084 89 69 11 0 79 0
+  // We need the second number which is RSS in pages.
+  const char *Pos = Buf;
+  // Skip the first number.
+  while (*Pos >= '0' && *Pos <= '9')
+    Pos++;
+  // Skip whitespaces.
+  while (!(*Pos >= '0' && *Pos <= '9') && *Pos != 0)
+    Pos++;
+  // Read the number.
+  u64 Rss = 0;
+  for (; *Pos >= '0' && *Pos <= '9'; Pos++)
+    Rss = Rss * 10 + static_cast<u64>(*Pos) - '0';
+  return Rss * getPageSizeCached();
+}
+
+u64 GetRSS() {
+  auto Fd = open("/proc/self/statm", O_RDONLY);
+  char Buf[64];
+  s64 Len = read(Fd, Buf, sizeof(Buf) - 1);
+  close(Fd);
+  if (Len <= 0)
+    return 0;
+  Buf[Len] = 0;
+
+  return GetRSSFromBuffer(Buf);
+}
+
 void outputRaw(const char *Buffer) {
   if (&async_safe_write_log) {
     constexpr s32 AndroidLogInfo = 4;
     constexpr uptr MaxLength = 1024U;
     char LocalBuffer[MaxLength];
     while (strlen(Buffer) > MaxLength) {
       uptr P;
       for (P = MaxLength - 1; P > 0; P--) {

compiler-rt/lib/scudo/standalone/tests/common_test.cpp

TEST(ScudoCommonTest, Zeros) {
   EXPECT_EQ(std::count(P, P + N, 0), 0);
 
   releasePagesToOS((uptr)P, 0, Size, &Data);
   EXPECT_EQ(std::count(P, P + N, 0), N);
 
   unmap(P, Size, 0, &Data);
 }
 
+#if SCUDO_LINUX
+TEST(ScudoCommonTest, GetRssFromBuffer) {
+  constexpr size_t AllocSize = 10000000;
+  constexpr u64 Error = 3000000;
+  constexpr size_t Runs = 10;
+
+  u64 Rss = scudo::GetRSS();
+  EXPECT_GT(Rss, 0);
+
+  std::vector<std::unique_ptr<char[]>> Allocs(Runs);
+  for (auto &Alloc : Allocs) {
+    Alloc.reset(new char[AllocSize]());
+    u64 Prev = Rss;
+    Rss = scudo::GetRSS();
+    EXPECT_LE(std::abs(static_cast<int64_t>(Rss - AllocSize - Prev)), Error);
+  }
+}
+#endif // SCUDO_LINUX
+
 } // namespace scudo