[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
https://github.com/teresajohnson approved this pull request. lgtm https://github.com/llvm/llvm-project/pull/94264 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -20,25 +20,25 @@ CHECK-NEXT: - CHECK: Records: CHECK-NEXT: - -CHECK-NEXT:FunctionGUID: 15505678318020221912 +CHECK-NEXT:FunctionGUID: 3873612792189045660 CHECK-NEXT:AllocSites: CHECK-NEXT:- CHECK-NEXT: Callstack: CHECK-NEXT: - -CHECK-NEXT:Function: 15505678318020221912 -CHECK-NEXT:SymbolName: qux +CHECK-NEXT:Function: 3873612792189045660 +CHECK-NEXT:SymbolName: _Z3quxi teresajohnson wrote: Not a problem, just struck me as odd since your patch shouldn't change. But like you said, probably some prior change was made that didn't require updating this test to get it to pass, and now those changes are showing up. https://github.com/llvm/llvm-project/pull/94264 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -96,19 +102,63 @@ llvm::SmallVector readSegmentEntries(const
char *Ptr) {
}
llvm::SmallVector>
-readMemInfoBlocks(const char *Ptr) {
+readMemInfoBlocksV3(const char *Ptr) {
using namespace support;
const uint64_t NumItemsToRead =
- endian::readNext(Ptr);
+ endian::readNext(Ptr);
+
llvm::SmallVector> Items;
for (uint64_t I = 0; I < NumItemsToRead; I++) {
const uint64_t Id =
-endian::readNext(Ptr);
-const MemInfoBlock MIB = *reinterpret_cast(Ptr);
+endian::readNext(Ptr);
+
+// We cheat a bit here and remove the const from cast to set the
+// Histogram Pointer to newly allocated buffer. We also cheat, since V3 and
+// V4 do not have the same fields. V3 is missing AccessHistogramSize and
+// AccessHistogram. This means we read "dirty" data in here, but it should
+// not segfault, since there will be callstack data placed after this in
the
+// binary format.
+MemInfoBlock MIB = *reinterpret_cast(Ptr);
+// Overwrite dirty data.
teresajohnson wrote:
Oh yes you are right, I missed the first `*` which means it is making a copy of
what was in the memory pointed to by Ptr.
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -216,6 +228,35 @@ u64 GetShadowCount(uptr p, u32 size) {
return count;
}
+// Accumulates the access count from the shadow for the given pointer and size.
+// See memprof_mapping.h for an overview on histogram counters.
+u64 GetShadowCountHistogram(uptr p, u32 size) {
+ u8 *shadow = (u8 *)HISTOGRAM_MEM_TO_SHADOW(p);
+ u8 *shadow_end = (u8 *)HISTOGRAM_MEM_TO_SHADOW(p + size);
+ u64 count = 0;
+ for (; shadow <= shadow_end; shadow++)
+count += *shadow;
+ return count;
+}
+
+// If we use the normal approach from clearCountersWithoutHistogram, the
+// histogram will clear too much data and may overwrite shadow counters that
are
+// in use. Likely because of rounding up the shadow_end pointer.
+// See memprof_mapping.h for an overview on histogram counters.
+void clearCountersHistogram(uptr addr, uptr size) {
+ u8 *shadow_8 = (u8 *)HISTOGRAM_MEM_TO_SHADOW(addr);
+ u8 *shadow_end_8 = (u8 *)HISTOGRAM_MEM_TO_SHADOW(addr + size);
+ for (; shadow_8 < shadow_end_8; shadow_8++) {
+*shadow_8 = 0;
+ }
+}
+
+void clearCountersWithoutHistogram(uptr addr, uptr size) {
+ uptr shadow_beg = MEM_TO_SHADOW(addr);
+ uptr shadow_end = MEM_TO_SHADOW(addr + size - SHADOW_GRANULARITY) + 1;
+ REAL(memset)((void *)shadow_beg, 0, shadow_end - shadow_beg);
+}
+
// Clears the shadow counters (when memory is allocated).
void ClearShadow(uptr addr, uptr size) {
teresajohnson wrote:
I guess it works, because the shadow granularities are less than the page size,
and because they are both scaling by the same 8 to 1 scale, and also I guess
because the clear_shadow_mmap_threshold is an optimization and doesn't need to
be exact. However, it still feels a little wonky to me (and also means that we
have to do extra mapping operations here and again in `clearCounters*`). I do
think I would prefer to have it be something like:
```
uptr shadow_beg;
uptr shadow_end;
if (__memprof_histogram) {
shadow_beg = HISTOGRAM_MEM_TO_SHADOW(addr);
shadow_end = HISTOGRAM_MEM_TO_SHADOW(addr + size);
} else {
shadow_beg = MEM_TO_SHADOW(addr);
shadow_end = MEM_TO_SHADOW(addr + size - SHADOW_GRANULARITY) + 1;
}
if (shadow_end - shadow_beg < common_flags()->clear_shadow_mmap_threshold) {
REAL(memset)((void *)shadow_beg, 0, shadow_end - shadow_beg);
} else {
...
```
I.e. set shadow_beg/end based on whether we are doing the histogramming or not,
leave the rest as-is. Would that work?
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -0,0 +1,101 @@ +REQUIRES: x86_64-linux + +This is a copy of memprof-basict.test with slight changes to check that we can still read v3 of memprofraw. + +To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang teresajohnson wrote: I think this comment is wrong - we can't update the v3 version, is that correct? Nor would we want to. https://github.com/llvm/llvm-project/pull/94264 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -38,4 +38,5 @@ MEMPROF_FLAG(bool, allocator_frees_and_returns_null_on_realloc_zero, true, MEMPROF_FLAG(bool, print_text, false, "If set, prints the heap profile in text format. Else use the raw binary serialization format.") MEMPROF_FLAG(bool, print_terse, false, - "If set, prints memory profile in a terse format. Only applicable if print_text = true.") + "If set, prints memory profile in a terse format. Only applicable " teresajohnson wrote: Formatting change only, remove from patch https://github.com/llvm/llvm-project/pull/94264 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -205,8 +205,14 @@ class RawMemProfReader final : public MemProfReader {
object::SectionedAddress getModuleOffset(uint64_t VirtualAddress);
+ llvm::SmallVector>
+ readMemInfoBlocks(const char *Ptr);
+
// The profiled binary.
object::OwningBinary Binary;
+ // Version of raw memprof binary currently being read. Defaults to most
update
teresajohnson wrote:
typo: s/update to date/up to date/
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -610,13 +670,33 @@ RawMemProfReader::peekBuildIds(MemoryBuffer *DataBuffer) {
return BuildIds.takeVector();
}
+// FIXME: Add a schema for serializing similiar to IndexedMemprofReader. This
+// will help being able to deserialize different versions raw memprof versions
+// more easily.
+llvm::SmallVector>
+RawMemProfReader::readMemInfoBlocks(const char *Ptr) {
+ if (MemprofRawVersion == 3ULL) {
+errs() << "Reading V3\n";
teresajohnson wrote:
Once you do that, the braces can be removed from all of the if else conditions
here which have single statement bodies
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -610,13 +670,33 @@ RawMemProfReader::peekBuildIds(MemoryBuffer *DataBuffer) {
return BuildIds.takeVector();
}
+// FIXME: Add a schema for serializing similiar to IndexedMemprofReader. This
+// will help being able to deserialize different versions raw memprof versions
+// more easily.
+llvm::SmallVector>
+RawMemProfReader::readMemInfoBlocks(const char *Ptr) {
+ if (MemprofRawVersion == 3ULL) {
+errs() << "Reading V3\n";
teresajohnson wrote:
Leftover debugging message that should be removed?
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -0,0 +1,101 @@ +REQUIRES: x86_64-linux + +This is a copy of memprof-basict.test with slight changes to check that we can still read v3 of memprofraw. teresajohnson wrote: typo: basict https://github.com/llvm/llvm-project/pull/94264 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -216,6 +228,35 @@ u64 GetShadowCount(uptr p, u32 size) {
return count;
}
+// Accumulates the access count from the shadow for the given pointer and size.
+// See memprof_mapping.h for an overview on histogram counters.
+u64 GetShadowCountHistogram(uptr p, u32 size) {
+ u8 *shadow = (u8 *)HISTOGRAM_MEM_TO_SHADOW(p);
+ u8 *shadow_end = (u8 *)HISTOGRAM_MEM_TO_SHADOW(p + size);
+ u64 count = 0;
+ for (; shadow <= shadow_end; shadow++)
+count += *shadow;
+ return count;
+}
+
+// If we use the normal approach from clearCountersWithoutHistogram, the
+// histogram will clear too much data and may overwrite shadow counters that
are
+// in use. Likely because of rounding up the shadow_end pointer.
+// See memprof_mapping.h for an overview on histogram counters.
+void clearCountersHistogram(uptr addr, uptr size) {
+ u8 *shadow_8 = (u8 *)HISTOGRAM_MEM_TO_SHADOW(addr);
+ u8 *shadow_end_8 = (u8 *)HISTOGRAM_MEM_TO_SHADOW(addr + size);
+ for (; shadow_8 < shadow_end_8; shadow_8++) {
+*shadow_8 = 0;
+ }
+}
+
+void clearCountersWithoutHistogram(uptr addr, uptr size) {
+ uptr shadow_beg = MEM_TO_SHADOW(addr);
+ uptr shadow_end = MEM_TO_SHADOW(addr + size - SHADOW_GRANULARITY) + 1;
+ REAL(memset)((void *)shadow_beg, 0, shadow_end - shadow_beg);
+}
+
// Clears the shadow counters (when memory is allocated).
void ClearShadow(uptr addr, uptr size) {
teresajohnson wrote:
Looking at this function more, there are a lot of uses of the MEM_TO_SHADOW
computed values, which is not the right mapping function to use with the
smaller granularity of the histogram case. I think you probably need to version
the calls to MEM_TO_SHADOW here, then all of the rest of the code can work as
is? I.e. you wouldn't need the 2 versions of `clearCounters*`
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -124,6 +124,13 @@ struct PortableMemInfoBlock {
OS << "" << #Name << ": " << Name << "\n";
#include "llvm/ProfileData/MIBEntryDef.inc"
#undef MIBEntryDef
+if (AccessHistogramSize > 0) {
+ OS << "" << "AccessHistogramValues" << ":";
+ for (uint32_t I = 0; I < AccessHistogramSize; ++I) {
+OS << " -" << ((uint64_t *)AccessHistogram)[I];
teresajohnson wrote:
Why the " -" in the outputs? I noticed when looking at the text that they look
like negative values as a result. Any reason not to just delimit with the space?
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -508,7 +519,26 @@ void createProfileFileNameVar(Module ) {
}
}
+// Set MemprofHistogramFlag as a Global veriable in IR. This makes it
accessible
+// to
teresajohnson wrote:
nit: fix line wrapping
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -96,19 +102,63 @@ llvm::SmallVector readSegmentEntries(const
char *Ptr) {
}
llvm::SmallVector>
-readMemInfoBlocks(const char *Ptr) {
+readMemInfoBlocksV3(const char *Ptr) {
using namespace support;
const uint64_t NumItemsToRead =
- endian::readNext(Ptr);
+ endian::readNext(Ptr);
+
llvm::SmallVector> Items;
for (uint64_t I = 0; I < NumItemsToRead; I++) {
const uint64_t Id =
-endian::readNext(Ptr);
-const MemInfoBlock MIB = *reinterpret_cast(Ptr);
+endian::readNext(Ptr);
+
+// We cheat a bit here and remove the const from cast to set the
+// Histogram Pointer to newly allocated buffer. We also cheat, since V3 and
+// V4 do not have the same fields. V3 is missing AccessHistogramSize and
+// AccessHistogram. This means we read "dirty" data in here, but it should
+// not segfault, since there will be callstack data placed after this in
the
+// binary format.
+MemInfoBlock MIB = *reinterpret_cast(Ptr);
+// Overwrite dirty data.
teresajohnson wrote:
Isn't this going to overwrite some callstack data?
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -20,25 +20,25 @@ CHECK-NEXT: - CHECK: Records: CHECK-NEXT: - -CHECK-NEXT:FunctionGUID: 15505678318020221912 +CHECK-NEXT:FunctionGUID: 3873612792189045660 CHECK-NEXT:AllocSites: CHECK-NEXT:- CHECK-NEXT: Callstack: CHECK-NEXT: - -CHECK-NEXT:Function: 15505678318020221912 -CHECK-NEXT:SymbolName: qux +CHECK-NEXT:Function: 3873612792189045660 +CHECK-NEXT:SymbolName: _Z3quxi teresajohnson wrote: Why did the function symbol names change with your patch? https://github.com/llvm/llvm-project/pull/94264 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -508,7 +519,26 @@ void createProfileFileNameVar(Module ) {
}
}
+// Set MemprofHistogramFlag as a Global veriable in IR. This makes it
accessible
+// to
+// the runtime, changing shadow count behavior.
+void createMemprofHistogramFlagVar(Module ) {
+ const StringRef VarName(MemProfHistogramFlagVar);
+ Type *IntTy1 = Type::getInt1Ty(M.getContext());
+ auto MemprofHistogramFlag = new GlobalVariable(
+ M, IntTy1, true, GlobalValue::WeakAnyLinkage,
+ Constant::getIntegerValue(IntTy1, APInt(1, ClHistogram)), VarName);
+ // MemprofHistogramFlag->setVisibility(GlobalValue::HiddenVisibility);
teresajohnson wrote:
remove?
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)
@@ -216,6 +228,35 @@ u64 GetShadowCount(uptr p, u32 size) {
return count;
}
+// Accumulates the access count from the shadow for the given pointer and size.
+// See memprof_mapping.h for an overview on histogram counters.
+u64 GetShadowCountHistogram(uptr p, u32 size) {
+ u8 *shadow = (u8 *)HISTOGRAM_MEM_TO_SHADOW(p);
+ u8 *shadow_end = (u8 *)HISTOGRAM_MEM_TO_SHADOW(p + size);
+ u64 count = 0;
+ for (; shadow <= shadow_end; shadow++)
+count += *shadow;
+ return count;
+}
+
+// If we use the normal approach from clearCountersWithoutHistogram, the
+// histogram will clear too much data and may overwrite shadow counters that
are
+// in use. Likely because of rounding up the shadow_end pointer.
+// See memprof_mapping.h for an overview on histogram counters.
+void clearCountersHistogram(uptr addr, uptr size) {
+ u8 *shadow_8 = (u8 *)HISTOGRAM_MEM_TO_SHADOW(addr);
+ u8 *shadow_end_8 = (u8 *)HISTOGRAM_MEM_TO_SHADOW(addr + size);
+ for (; shadow_8 < shadow_end_8; shadow_8++) {
teresajohnson wrote:
Why not use REAL(memset) like the non-histogram version below?
https://github.com/llvm/llvm-project/pull/94264
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [mlir] [openmp] [polly] fix(python): fix comparison to None (PR #91857)
https://github.com/teresajohnson approved this pull request. compiler-rt changes lgtm https://github.com/llvm/llvm-project/pull/91857 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
