[Lldb-commits] [PATCH] D145377: [LLDB] Show sub type of memory tagging SEGV when reading a core file

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/source/Plugins/Process/Utility/LinuxSignals.cpp:32
+  AddSignalCode(11, 8 /*SEGV_MTEAERR*/, "async tag check fault");
+  AddSignalCode(11, 9 /*SEGV_MTESERR*/, "sync tag check fault");
   AddSignal(12, "SIGUSR2",  false,true,   true,   "user defined 
signal 2");

See 
https://github.com/torvalds/linux/blob/6a98c9cae232800c319ed69e1063480d31430887/include/uapi/asm-generic/siginfo.h#L229,
 the codes apply to all architectures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145377/new/

https://reviews.llvm.org/D145377

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145629: When a ValueObjectDynamicValue fails to update, return a not valid ValueObject so the static one is used instead

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: lldb/source/Core/ValueObject.cpp:1862
   }
-  if (m_dynamic_value)
+  if (m_dynamic_value && m_dynamic_value->GetError().Success())
 return m_dynamic_value->GetSP();

mib wrote:
> It would be nice if `ValueObjectDynamicValue` had a `IsValid` method that 
> returned the result of its Status attribute. Without reading the commit 
> message, `GetError` doesn't tell us much about what we're trying to achieve 
> here.
IsValid is less restrictive than having an error.  IsValid is true if the value 
has a variable in it, even if, for instance, we couldn't currently read the 
memory underlying the value.  I want "was there any error at all", so checking 
the Error is more appropriate here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145629/new/

https://reviews.llvm.org/D145629

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] e67460c - [lldb] Add nullptr check to SymbolVendorWasm

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-03-09T10:27:18-08:00
New Revision: e67460c974e65aee83d2976226db17d8ffe17cc2

URL: 
https://github.com/llvm/llvm-project/commit/e67460c974e65aee83d2976226db17d8ffe17cc2
DIFF: 
https://github.com/llvm/llvm-project/commit/e67460c974e65aee83d2976226db17d8ffe17cc2.diff

LOG: [lldb] Add nullptr check to SymbolVendorWasm

Add the same nullptr check to SymbolVendorWasm that was added to
SymbolVendorELF.

Added: 


Modified: 
lldb/source/Plugins/SymbolVendor/wasm/SymbolVendorWasm.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolVendor/wasm/SymbolVendorWasm.cpp 
b/lldb/source/Plugins/SymbolVendor/wasm/SymbolVendorWasm.cpp
index 6290843a89872..91b10ea64535d 100644
--- a/lldb/source/Plugins/SymbolVendor/wasm/SymbolVendorWasm.cpp
+++ b/lldb/source/Plugins/SymbolVendor/wasm/SymbolVendorWasm.cpp
@@ -109,6 +109,9 @@ SymbolVendorWasm::CreateInstance(const lldb::ModuleSP 
&module_sp,
   SectionList *module_section_list = module_sp->GetSectionList();
   SectionList *objfile_section_list = sym_objfile_sp->GetSectionList();
 
+  if (!module_section_list || !objfile_section_list)
+return nullptr;
+
   static const SectionType g_sections[] = {
   eSectionTypeDWARFDebugAbbrev,   eSectionTypeDWARFDebugAddr,
   eSectionTypeDWARFDebugAranges,  eSectionTypeDWARFDebugCuIndex,



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Target/Memory.cpp:133-141
   // Check the L1 cache for a range that contain the entire memory read. If we
-  // find a range in the L1 cache that does, we use it. Else we fall back to
-  // reading memory in m_L2_cache_line_byte_size byte sized chunks. The L1
-  // cache contains chunks of memory that are not required to be
+  // find a range in the L1 cache that does, we use it. If not:
+  //  - dst_len > L2 cache line size: Attempt to read the entire memory chunk
+  //and insert whatever we get back into the L1 cache.
+  //  - dst_len <= L2 cache line size: Attempt to read it from the L2 cache, 
and
+  //if its not there, attempt to populate the cache.
+  // The L1 cache contains chunks of memory that are not required to be

JDevlieghere wrote:
> Instead of describing the algorithm here, would it make sense to break this 
> up and put it above the relevant code below? It seems like it matches pretty 
> well with the code structure. Looking at the signature of 
> `FindEntryThatContains` and the fact that it doesn't take the size, I assume 
> it's because we only check the start address? 
Good point!



Comment at: lldb/source/Target/Memory.cpp:149
+  if (m_invalid_ranges.FindEntryThatContains(addr)) {
+error.SetErrorStringWithFormat("memory read failed for 0x%" PRIx64, addr);
+return 0;

JDevlieghere wrote:
> Should the error mention that if failed due to the address overlapping with 
> an invalid range? Is an invalid range something that is meaningful to the 
> user? 
I don't think the concept of "invalid range" is meaningful to the user right 
now. I'm pretty sure we only use it to prevent us from reading __PAGEZERO on 
apple platforms.



Comment at: lldb/source/Target/Memory.cpp:181-183
+// FIXME: We should be able to wrap this into the check near the beginning
+// of this function but we don't check for overlapping ranges so we will
+// additionally check each cache line we read.

clayborg wrote:
> Not on the FIXME: We can't really check this near the beginning, because this 
> happens for each cache line we as we advance the "curr_cache_line_base_addr" 
> right? 
> 
> One thing to note about this code is that we might need to read at most 2 
> cache lines for any requests that make it to this code since we check above 
> for "if (dst_len > m_L2_cache_line_byte_size)..." and use the L1 cache if 
> that is true. So we know that we will read at most 2 cache lines depending on 
> the offset. Might be nice to read the 2 cache lines in one memory access 
> below if possible, and then make two cache entries with the result, but it 
> will be either one cache line read, or two
If I'm understanding what you mean correctly, I think we can check this near 
the beginning. We have all the information we need to do safety checks before 
we even start reading anything, I believe...?

Also, because we read at most 2 cache lines, I can probably get rid of the loop 
and just do 2 sequential reads...



Comment at: lldb/source/Target/Memory.cpp:196-198
+  // If we didn't fill out the cache line completely and the offset is
+  // bigger than what we have available, we can't do anything further
+  // here.

clayborg wrote:
> JDevlieghere wrote:
> > Why can't we read from the process? Same question below.
> Because if we did a memory request before from a valid 
> "curr_cache_line_base_addr", and we got back fewer bytes that requested, then 
> the bytes won't be available later right?
If we've hit this code path then we have previously read from the process and 
got back fewer bytes than a cache line fits. For example, maybe a cache line is 
512 bytes and when we performed the read we got back 502 bytes for some reason. 
If we're trying to read the last 10 bytes of that line, that's just not 
available, so we bail out. We could try to protect against this in a number of 
ways, like if we get back fewer bytes than we wanted initially then maybe we 
can retry the read before caching the line, or if the line isn't filled out 
maybe can try to read the inferior one more time or something.

Ultimately, I want `MemoryCache` to be prepared for reads to be incomplete and 
guard against touching memory that we don't have.



Comment at: lldb/source/Target/Memory.cpp:200
+  if (cache_offset >= cached_line_size)
+return dst_len - bytes_left;
 

mib wrote:
> Shouldn't this be the size of the cached line ?
To be honest, I'm somewhat sure that this line actually could be `return 0;`. 
I'm pretty sure that we only will hit this on the first cache line read. If we 
read a second cache line, we should always be starting at the beginning of the 
cache line... I'll probably refactor this.



Comment at: lldb/source/Target/Memory.cpp:210
+  bytes_left -=

[Lldb-commits] [lldb] 7c4e6c9 - [lldb] Skip TestSymbolFileJSON on Windows

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2023-03-09T11:33:14-08:00
New Revision: 7c4e6c97fb689f8db8ea361fe615fe39eb332610

URL: 
https://github.com/llvm/llvm-project/commit/7c4e6c97fb689f8db8ea361fe615fe39eb332610
DIFF: 
https://github.com/llvm/llvm-project/commit/7c4e6c97fb689f8db8ea361fe615fe39eb332610.diff

LOG: [lldb] Skip TestSymbolFileJSON on Windows

Disable the test as Windows (or at least the bot) doesn't have 'strip':

  'strip' is not recognized as an internal or external command

Added: 


Modified: 
lldb/test/API/macosx/symbols/TestSymbolFileJSON.py

Removed: 




diff  --git a/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py 
b/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
index 31814ba5c85e2..9c17f07420e91 100644
--- a/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
+++ b/lldb/test/API/macosx/symbols/TestSymbolFileJSON.py
@@ -12,6 +12,7 @@ def setUp(self):
 self.source = 'main.c'
 
 @no_debug_info_test
+@skipIfWindows # No 'strip'
 def test_symbol_file_json_address(self):
 """Test that 'target symbols add' can load the symbols from a JSON 
file using file addresses."""
 



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

One other optimization we can do is if we read from the process memory and it 
returns that is read zero bytes, right now we add the range we were trying to 
read into the m_invalid_ranges member variable. So lets say we were trying to 
read the range [0x1000-0x2000) on a mac. We will fail to read this due to 
__PAGEZERO, but I believe we currently add this range to the m_invalid_ranges. 
But we could ask about this memory region from the process and realize we can 
actually add [0x0-0x1) to the m_invalid_ranges. That might help avoid 
multiple bad reads from a large area that isn't mapped.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145624/new/

https://reviews.llvm.org/D145624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I would suggest checking the google stadia patch for the L1 
 and L2 caches:

https://github.com/googlestadia/vsi-lldb/blob/master/patches/llvm-project/0019-lldb-Fix-incorrect-L1-inferior-memory-cache-flushing.patch

Just to see how they did things.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145624/new/

https://reviews.llvm.org/D145624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D145624#4182424 , @clayborg wrote:

> I would suggest checking the google stadia patch for the L1 
>  and L2 caches:
>
> https://github.com/googlestadia/vsi-lldb/blob/master/patches/llvm-project/0019-lldb-Fix-incorrect-L1-inferior-memory-cache-flushing.patch
>
> Just to see how they did things.

I looked at this patch earlier. They're modifying code that I'm not touching 
and these 2 patches can be applied separately without conflict. It may be worth 
trying to apply this fix in a follow-up commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145624/new/

https://reviews.llvm.org/D145624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145568: [lldb] Add dummy field to RegisterInfo for register flags use later

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145568/new/

https://reviews.llvm.org/D145568

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145566: [lldb] Add RegisterFlags class

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

LGTM.  I'm not really advocating to changing `m_type` to be an enum, but 
curious how that might fit into the total context of this feature, and if it 
makes any sense.




Comment at: lldb/include/lldb/Target/RegisterFlags.h:70
+// We do not expect to see another flags type here.
+std::string m_type;
+  };

Should we store the type as an eFormat enum instead of a string?  Maybe it's 
just informational for logging/humans and not too important.  
ProcessGDBRemote::BuildDynamicRegisterInfo translates our own register type 
names into these values (more often seen in the old lldb-created 
`qRegisterInfo`s). 



Comment at: lldb/source/Target/RegisterFlags.cpp:49
+: m_id(id.str()), m_size(size) {
+  // We expect that the XML processor will discard anything decsribing flags 
but
+  // with no fields.

describing


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145566/new/

https://reviews.llvm.org/D145566

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 503942.
bulbazord added a comment.

- Addressed reviewer feedback
- Simplified the case where we use L2 cache lines


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145624/new/

https://reviews.llvm.org/D145624

Files:
  lldb/include/lldb/Target/Memory.h
  lldb/source/Target/Memory.cpp
  lldb/unittests/Target/CMakeLists.txt
  lldb/unittests/Target/MemoryTest.cpp

Index: lldb/unittests/Target/MemoryTest.cpp
===
--- /dev/null
+++ lldb/unittests/Target/MemoryTest.cpp
@@ -0,0 +1,228 @@
+//===-- MemoryTest.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 "lldb/Target/Memory.h"
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+#include "Plugins/Platform/MacOSX/PlatformRemoteMacOSX.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/DataBufferHeap.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+using namespace lldb;
+
+namespace {
+class MemoryTest : public ::testing::Test {
+public:
+  void SetUp() override {
+FileSystem::Initialize();
+HostInfo::Initialize();
+PlatformMacOSX::Initialize();
+  }
+  void TearDown() override {
+PlatformMacOSX::Terminate();
+HostInfo::Terminate();
+FileSystem::Terminate();
+  }
+};
+
+class DummyProcess : public Process {
+public:
+  DummyProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp)
+  : Process(target_sp, listener_sp), m_bytes_left(0) {}
+
+  // Required overrides
+  bool CanDebug(lldb::TargetSP target, bool plugin_specified_by_name) override {
+return true;
+  }
+  Status DoDestroy() override { return {}; }
+  void RefreshStateAfterStop() override {}
+  size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
+  Status &error) override {
+if (m_bytes_left == 0)
+  return 0;
+
+size_t num_bytes_to_write = size;
+if (m_bytes_left < size) {
+  num_bytes_to_write = m_bytes_left;
+  m_bytes_left = 0;
+} else {
+  m_bytes_left -= size;
+}
+
+memset(buf, 'B', num_bytes_to_write);
+return num_bytes_to_write;
+  }
+  bool DoUpdateThreadList(ThreadList &old_thread_list,
+  ThreadList &new_thread_list) override {
+return false;
+  }
+  llvm::StringRef GetPluginName() override { return "Dummy"; }
+
+  // Test-specific additions
+  size_t m_bytes_left;
+  MemoryCache &GetMemoryCache() { return m_memory_cache; }
+  void SetMaxReadSize(size_t size) { m_bytes_left = size; }
+};
+} // namespace
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
+  PlatformSP platform_sp;
+  TargetSP target_sp;
+  debugger_sp->GetTargetList().CreateTarget(
+  *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+  return target_sp;
+}
+
+TEST_F(MemoryTest, TesetMemoryCacheRead) {
+  ArchSpec arch("x86_64-apple-macosx-");
+
+  Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  TargetSP target_sp = CreateTarget(debugger_sp, arch);
+  ASSERT_TRUE(target_sp);
+
+  ListenerSP listener_sp(Listener::MakeListener("dummy"));
+  ProcessSP process_sp = std::make_shared(target_sp, listener_sp);
+  ASSERT_TRUE(process_sp);
+
+  DummyProcess *process = static_cast(process_sp.get());
+  MemoryCache &mem_cache = process->GetMemoryCache();
+  const uint64_t l2_cache_size = process->GetMemoryCacheLineSize();
+  Status error;
+  auto data_sp = std::make_shared(l2_cache_size * 2, '\0');
+  size_t bytes_read = 0;
+
+  // Cache empty, memory read fails, size > l2 cache size
+  process->SetMaxReadSize(0);
+  bytes_read = mem_cache.Read(0x1000, data_sp->GetBytes(),
+  data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == 0);
+
+  // Cache empty, memory read fails, size <= l2 cache size
+  data_sp->SetByteSize(l2_cache_size);
+  bytes_read = mem_cache.Read(0x1000, data_sp->GetBytes(),
+  data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == 0);
+
+  // Cache empty, memory read succeeds, size > l2 cache size
+  process->SetMaxReadSize(l2_cache_size * 4);
+  data_sp->SetByteSize(l2_cache_size * 2);
+  bytes_read = mem_cache.Read(0x1000, data_sp->GetBytes(),
+  data_sp->GetByteSize(), error);
+  ASSERT_TRUE(bytes_read == data_sp->GetByteSize());
+  ASSE

[Lldb-commits] [lldb] 7f25c3e - Slight refinement to a change yesterday in metadata-added binaries

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-03-09T14:56:22-08:00
New Revision: 7f25c3e25f0afa91ae47c7247cca7fabaa2a7dd5

URL: 
https://github.com/llvm/llvm-project/commit/7f25c3e25f0afa91ae47c7247cca7fabaa2a7dd5
DIFF: 
https://github.com/llvm/llvm-project/commit/7f25c3e25f0afa91ae47c7247cca7fabaa2a7dd5.diff

LOG: Slight refinement to a change yesterday in metadata-added binaries

When ObjectFileMachO::LoadCoreFileImages load a binary into the
target with a valid load address, we don't need to re-load its
segments into the Target's SectionLoadList again.  But we should
still call ModulesDidLoad on these modules so breakpoints can be
inserted etc.

Added: 


Modified: 
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index c5f04557ae611..391ed99607f77 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -7032,8 +7032,12 @@ bool 
ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
   &process, image.filename, image.uuid, image.load_address,
   false /* value_is_offset */, image.currently_executing,
   false /* notify */);
-  if (module_sp)
+  if (module_sp) {
+// We've already set the load address in the Target,
+// don't do any more processing on this module.
+added_modules.Append(module_sp, false /* notify */);
 continue;
+  }
 }
 
 // If we have a slide, we need to find the original binary
@@ -7044,8 +7048,12 @@ bool 
ObjectFileMachO::LoadCoreFileImages(lldb_private::Process &process) {
   &process, image.filename, image.uuid, image.slide,
   true /* value_is_offset */, image.currently_executing,
   false /* notify */);
-  if (module_sp)
+  if (module_sp) {
+// We've already set the load address in the Target,
+// don't do any more processing on this module.
+added_modules.Append(module_sp, false /* notify */);
 continue;
+  }
 }
 
 // Try to find the binary by UUID or filename on the local



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

[Alex Langford via Phabricator via lldb-commits]

bulbazord marked 11 inline comments as done.
bulbazord added inline comments.



Comment at: lldb/source/Target/Memory.cpp:255-256
 
-  // We need to read from the process
+  if (process_bytes_read < cache_line_byte_size)
+data_buffer_heap_up->SetByteSize(process_bytes_read);
 

bulbazord wrote:
> clayborg wrote:
> > If we don't read an entire cache line, should we populate this into the L1 
> > cache instead? It might make the logic for accessing data in the L2 cache a 
> > bit simpler?
> That might not be a bad idea actually. I'll try it and see how much it 
> simplifies the logic.
It didn't make things any simpler so I didn't change it. Good idea though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145624/new/

https://reviews.llvm.org/D145624

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

This all looks good to me.  the phab says there's a missing newline at the end 
of TestXMLRegisterFlags.py.

Given that we can construct fake debug sessions with a gdb_remote_client test, 
do you think it would be worth having a test that's says it's a big-endian 
target, and confirming that we byte swap when the test is on a little-endian 
host?  I see you're using a .yaml shell binary for the session, but I wrote a 
simple client test a while ago, TestStopPCs.py, where I don't even bother with 
that.

On the other hand, I noticed you marked these as 
`skipIfLLVMTargetMissing("AArch64")` - is this because of the clang types?  
We're not disassembling or jitting anything for the target, as long as lldb 
recognizes the triple/ArchSpec, for registers it seems like it should be fine?  
If this skip is really needed, then I don't know how many people would exercise 
an s390x test even if it was added - I don't personally build with that target 
normally.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145580/new/

https://reviews.llvm.org/D145580

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145580: [lldb] Show register fields using bitfield struct types

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

I'll mark it as accepted, I don't know if Jim or Pavel have any comments.  I 
think this is good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145580/new/

https://reviews.llvm.org/D145580

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145574: [lldb] Read register fields from target XML

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

This looks good to me.




Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4066
+"ProcessGDBRemote::ParseFlags Invalid start %d in field node, "
+"cannot be > %d",
+parsed_start, max_start_bit);

All of the bit positions are `unsigned`, `%u`'s to be proper (and further 
printf specifiers below.  not that it will matter, of course.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145574/new/

https://reviews.llvm.org/D145574

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145569: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145569/new/

https://reviews.llvm.org/D145569

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D145569: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

[Michael Buch via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGadc5168d7114: [lldb][InstrumentationRuntime] Make 
'data' struct anonymous in order to avoid… (authored by Michael137).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145569/new/

https://reviews.llvm.org/D145569

Files:
  lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
  
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
  lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp


Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, 
int *thread_id);
 size_t __asan_get_free_stack(void *addr, void **trace, size_t size, 
int *thread_id);
 }
+)";
 
-struct data {
+const char *memory_history_asan_command_format =
+R"(
+struct {
 void *alloc_trace[256];
 size_t alloc_count;
 int alloc_tid;
@@ -76,12 +79,7 @@
 void *free_trace[256];
 size_t free_count;
 int free_tid;
-};
-)";
-
-const char *memory_history_asan_command_format =
-R"(
-data t;
+} t;
 
 t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
 R"(, t.alloc_trace, 256, &t.alloc_tid);
Index: 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
===
--- 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ 
lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retrieve_report_data_command = R"(
+struct {
   const char *issue_kind;
   const char *message;
   const char *filename;
   unsigned line;
   unsigned col;
   char *memory_addr;
-};
-)";
+} t;
 
-static const char *ub_sanitizer_retrieve_report_data_command = R"(
-data t;
 __ubsan_get_current_report_data(&t.issue_kind, &t.message, &t.filename, 
&t.line,
 &t.col, &t.memory_addr);
 t;
Index: 
lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
===
--- 
lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
+++ 
lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
@@ -87,6 +87,9 @@
 void *dlsym(void* handle, const char* symbol);
 int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);
 }
+)";
+
+const char *thread_sanitizer_retrieve_report_data_command = R"(
 
 const int REPORT_TRACE_SIZE = 128;
 const int REPORT_ARRAY_SIZE = 4;
@@ -154,11 +157,7 @@
 int idx;
 int tid;
 } unique_tids[REPORT_ARRAY_SIZE];
-};
-)";
-
-const char *thread_sanitizer_retrieve_report_data_command = R"(
-data t = {0};
+} t = {0};
 
 ptr__tsan_get_report_loc_object_type = 
(typeof(ptr__tsan_get_report_loc_object_type))(void *)dlsym((void*)-2 
/*RTLD_DEFAULT*/, "__tsan_get_report_loc_object_type");
 


Index: lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
===
--- lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, int *thread_id);
 size_t __asan_get_free_stack(void *addr, void **trace, size_t size, int *thread_id);
 }
+)";
 
-struct data {
+const char *memory_history_asan_command_format =
+R"(
+struct {
 void *alloc_trace[256];
 size_t alloc_count;
 int alloc_tid;
@@ -76,12 +79,7 @@
 void *free_trace[256];
 size_t free_count;
 int free_tid;
-};
-)";
-
-const char *memory_history_asan_command_format =
-R"(
-data t;
+} t;
 
 t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
 R"(, t.alloc_trace, 256, &t.alloc_tid);
Index: lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
===
--- lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct

[Lldb-commits] [lldb] adc5168 - [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

[Michael Buch via lldb-commits]

Author: Michael Buch
Date: 2023-03-10T00:40:22Z
New Revision: adc5168d7114190ec7d931a6c346276e19b0e117

URL: 
https://github.com/llvm/llvm-project/commit/adc5168d7114190ec7d931a6c346276e19b0e117
DIFF: 
https://github.com/llvm/llvm-project/commit/adc5168d7114190ec7d931a6c346276e19b0e117.diff

LOG: [lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to 
avoid collisions with types in the debuggee

The `UBSAN`/`ASAN` plugins would previously call the internal helper
structure injected into the source expression `struct data { ... };`

This occasionally collided with user-defined types of the same (quite
common) name. In the presence of varibale with the same name LLDB would
simply fail to run the injected `InstrumentationRuntime` expressions
with:
```
warning: cannot evaluate AddressSanitizer expression:
expression failed to parse:
error: :2:5: must use 'struct' tag to refer to type 'data' 
in this scope
data t;
```

In the presence of another type called 'data', LLDB would most likely
crash with something like:
```
0x0001198de614 
clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) + 1088
0x0001198de614 
clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) + 1088
0x000119907930 clang::ASTImporter::ImportDefinition(clang::Decl*) + 596
0x0001163db928 
lldb_private::ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(clang::Decl*,
 clang::Decl*) + 100
0x0001163da070 (anonymous 
namespace)::CompleteTagDeclsScope::~CompleteTagDeclsScope() + 572
...
```
...because it got the types confused.

This patch makes these structures anonymous so there's no
chance of clashing with other types in the program. This is
already the approach taken in `UBSan/InstrumentationRuntimeABSan.cpp`.

**Testing**

- API tests still pass
- Tested manually that the `memory history` command now works in the presence 
of a local called `data`

Differential Revision: https://reviews.llvm.org/D145569

Added: 


Modified: 

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
 
b/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
index 425b0baa453f8..873704723cced 100644
--- 
a/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
+++ 
b/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
@@ -87,6 +87,9 @@ extern "C"
 void *dlsym(void* handle, const char* symbol);
 int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long 
idx, const char **object_type);
 }
+)";
+
+const char *thread_sanitizer_retrieve_report_data_command = R"(
 
 const int REPORT_TRACE_SIZE = 128;
 const int REPORT_ARRAY_SIZE = 4;
@@ -154,11 +157,7 @@ struct data {
 int idx;
 int tid;
 } unique_tids[REPORT_ARRAY_SIZE];
-};
-)";
-
-const char *thread_sanitizer_retrieve_report_data_command = R"(
-data t = {0};
+} t = {0};
 
 ptr__tsan_get_report_loc_object_type = 
(typeof(ptr__tsan_get_report_loc_object_type))(void *)dlsym((void*)-2 
/*RTLD_DEFAULT*/, "__tsan_get_report_loc_object_type");
 

diff  --git 
a/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
 
b/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
index 7ea8b4697d204..0264a58e3a931 100644
--- 
a/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ 
b/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
@@ -67,19 +67,18 @@ __ubsan_get_current_report_data(const char **OutIssueKind,
 const char **OutMessage, const char **OutFilename, unsigned *OutLine,
 unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retrieve_report_data_command = R"(
+struct {
   const char *issue_kind;
   const char *message;
   const char *filename;
   unsigned line;
   unsigned col;
   char *memory_addr;
-};
-)";
+} t;
 
-static const char *ub_sanitizer_retrieve_report_data_command = R"(
-data t;
 __ubsan_get_current_report_data(&t.issue_kind, &t.message, &t.filename, 
&t.line,
 &t.col, &t.memory_addr);
 t;

diff  --git a/lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp 
b/lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
index aaead88369b20..c1f5970badbde 100644
--- a/lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ b/lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
@@ -67,8 +67,11 @@ const char *memory_history_asan_command_prefix = R"(
 size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, 
int *t

[Lldb-commits] [PATCH] D142792: Add SBValue::GetValueAsAddress(), strip off ptrauth, TBI, MTE bits on AArch64 systems

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda planned changes to this revision.
jasonmolenda added a comment.

OK, I understand the objc testsuite failures.  ObjC has tagged pointers, where 
the address of an object is either a virtual address, or a big old bag of bits 
that can contain a small value, like instead of a pointer to an NSObject 
object, you'll have a uint64_t that can be decoded to represent a small integer 
value without existing in memory.  In `ValueObject::GetPointerValue()`, I clear 
non-addressable bits, which destroys the tagged pointer values.  In the apple 
internal version of this, we have some additional code that checks the clang 
type of the ValueObject to see if pointer auth is used for this type before we 
clear them.  But with TBI/MTE values, we need to clear it regardless of type.  
The correct fix is to audit the objc type formatters for uses of 
ValueObject::GetPointerValue and have them fetch an unsigned value if there's 
any possibility of a tagged pointer being used, this is probably going to be a 
little bit of work.

This is specifically causing fails in TestDataFormatterObjCCF.py, 
TestDataFormatterObjCNSDate.py, TestDataFormatterObjCNSNumber.py, 
TestDataFormatterNSIndexPath.py, TestNSArraySynthetic.py, 
TestPrintObjectArray.py.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142792/new/

https://reviews.llvm.org/D142792

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits