[Lldb-commits] [clang] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [mlir] [openmp] [polly] fix(python): fix comparison to None (PR #91857)

[Teresa Johnson via lldb-commits]

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

[Lldb-commits] [clang-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)

[Teresa Johnson via lldb-commits]

@@ -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-tools-extra] [compiler-rt] [lldb] [llvm] [Memprof] Adds the option to collect AccessCountHistograms for memprof. (PR #94264)

[Teresa Johnson via lldb-commits]

@@ -508,7 +519,26 @@ void createProfileFileNameVar(Module &M) {
   }
 }
 
+// Set MemprofHistogramFlag as a Global veriable in IR. This makes it 
accessible
+// to
+// the runtime, changing shadow count behavior.
+void createMemprofHistogramFlagVar(Module &M) {
+  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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -508,7 +519,26 @@ void createProfileFileNameVar(Module &M) {
   }
 }
 
+// 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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

@@ -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)

[Teresa Johnson via lldb-commits]

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] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #115331)

[Teresa Johnson via lldb-commits]

teresajohnson wrote:

@anjenner are you looking at the remaining reported buildbot failures? E.g. the 
unit testing failure below. Please fix or revert this change.

> LLVM Buildbot has detected a new failure on builder 
> `llvm-nvptx-nvidia-ubuntu` running on `as-builder-7` while building 
> `lldb,llvm` at step 6 "test-build-unified-tree-check-llvm".
> 
> Full details are available at: 
> https://lab.llvm.org/buildbot/#/builders/180/builds/14226
> 
> Here is the relevant piece of the build log for the reference
> ```
> Step 6 (test-build-unified-tree-check-llvm) failure: test (failure)
>  TEST 'LLVM-Unit :: Support/./SupportTests/134/175' 
> FAILED 
> Script(shard):
> --
> GTEST_OUTPUT=json:/home/buildbot/worker/as-builder-7/ramdisk/llvm-nvptx-nvidia-ubuntu/build/unittests/Support/./SupportTests-LLVM-Unit-1311025-134-175.json
>  GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=175 GTEST_SHARD_INDEX=134 
> /home/buildbot/worker/as-builder-7/ramdisk/llvm-nvptx-nvidia-ubuntu/build/unittests/Support/./SupportTests
> --
> 
> Script:
> --
> /home/buildbot/worker/as-builder-7/ramdisk/llvm-nvptx-nvidia-ubuntu/build/unittests/Support/./SupportTests
>  --gtest_filter=Caching.Normal
> --
> /home/buildbot/worker/as-builder-7/ramdisk/llvm-nvptx-nvidia-ubuntu/llvm-project/llvm/unittests/Support/Caching.cpp:55:
>  Failure
> Value of: AddStream
>   Actual: false
> Expected: true
> 
> 
> /home/buildbot/worker/as-builder-7/ramdisk/llvm-nvptx-nvidia-ubuntu/llvm-project/llvm/unittests/Support/Caching.cpp:55
> Value of: AddStream
>   Actual: false
> Expected: true
> 
> 
> 
> 
> ```



https://github.com/llvm/llvm-project/pull/115331
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #115331)

[Teresa Johnson via lldb-commits]

https://github.com/teresajohnson edited 
https://github.com/llvm/llvm-project/pull/115331
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #115331)

[Teresa Johnson via lldb-commits]

@@ -0,0 +1,116 @@
+//===- Caching.cpp 
===//

teresajohnson wrote:

Can you test the error when there is no commit?

https://github.com/llvm/llvm-project/pull/115331
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #115331)

[Teresa Johnson via lldb-commits]

@@ -0,0 +1,116 @@
+//===- Caching.cpp 
===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "llvm/Support/Caching.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+#define ASSERT_NO_ERROR(x) 
\
+  if (std::error_code ASSERT_NO_ERROR_ec = x) {
\
+SmallString<128> MessageStorage;   
\
+raw_svector_ostream Message(MessageStorage);   
\
+Message << #x ": did not return errc::success.\n"  
\
+<< "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"  
\
+<< "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";  
\
+GTEST_FATAL_FAILURE_(MessageStorage.c_str());  
\
+  } else { 
\
+  }

teresajohnson wrote:

Why the empty else?

https://github.com/llvm/llvm-project/pull/115331
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #115331)

[Teresa Johnson via lldb-commits]

@@ -30,6 +30,7 @@ class CachedFileStream {
   CachedFileStream(std::unique_ptr OS,
std::string OSPath = "")
   : OS(std::move(OS)), ObjectPathName(OSPath) {}
+  virtual Error commit() { return Error::success(); }

teresajohnson wrote:

If the expectation is that commit() should now always be called, should there 
be some error detection in the base class?

https://github.com/llvm/llvm-project/pull/115331
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #115331)

[Teresa Johnson via lldb-commits]

https://github.com/teresajohnson commented:

I'm a little concerned about the added complexity of the cache interfaces. At 
the very least, the required usage of the commit() function should be clearly 
documented probably in the main Caching.h header, however, it is a bit 
confusing right now because there is no requirement to use that in the base 
class (comment on that below). There should also be comments on the other 
commit() method declarations.

https://github.com/llvm/llvm-project/pull/115331
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #136121)

[Teresa Johnson via lldb-commits]

teresajohnson wrote:

@dyung thanks for catching this, I haven't been able to watch closely and 
didn't realize no one approved.

@anjenner No worries, misunderstandings happen. I do think we need to revert 
this, however. See PR138203 - it sounds like this is causing issues with 
lifetimes and I'm concerned that there may be other lurking issues like that 
one. Can you revert this?

https://github.com/llvm/llvm-project/pull/136121
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #136121)

[Teresa Johnson via lldb-commits]

teresajohnson wrote:

> I'm away for the long weekend but can revert on Wednesday if nobody does so 
> before then. However, I'm not sure how we can track down such lurking issues 
> other than by leaving this unreverted and fixing issues as they pop up, since 
> neither code review nor testcases caught #138194 . Do you have any 
> recommendations other than just not making this API change and leaving the 
> localCache API without proper error handling?

I've added some questions on that PR, after looking closely at the 2 changes 
I'm not really sure how this change produced or is related to the crash being 
fixed there.

https://github.com/llvm/llvm-project/pull/136121
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits