[Lldb-commits] [lldb] a53874b - [lldb] Fix modules build by adding missing include

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-04-22T09:14:09+02:00
New Revision: a53874b7e4c82416b8eb48bbd45b5cb93f1e09d3

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

LOG: [lldb] Fix modules build by adding missing include

This header is using FileSpec so we should at least include the forward header.

Added: 


Modified: 
lldb/include/lldb/Utility/XcodeSDK.h

Removed: 




diff  --git a/lldb/include/lldb/Utility/XcodeSDK.h 
b/lldb/include/lldb/Utility/XcodeSDK.h
index b186ab4a7091..552c51c36844 100644
--- a/lldb/include/lldb/Utility/XcodeSDK.h
+++ b/lldb/include/lldb/Utility/XcodeSDK.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_UTILITY_SDK_H
 #define LLDB_UTILITY_SDK_H
 
+#include "lldb/lldb-forward.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/VersionTuple.h"
 #include 



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

[Lldb-commits] [PATCH] D78396: [lldb/Dataformatter] Add support for CoreFoundation Dictionaries and Sets

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:47-62
+  DataBufferHeap buffer(size, 0);
+  m_exe_ctx_ref.GetProcessSP()->ReadMemory(addr, buffer.GetBytes(), size,
+   error);
+  if (error.Fail())
+return false;
+
+  DataExtractor data_extractor = DataExtractor(

mib wrote:
> labath wrote:
> > Why don't you just pass `m_ht.get()` to `ReadMemory` ?
> IIUC, `ReadMemory` will read memory matching the inferior endianness. That's 
> why, I'm using a `DataExtractor`, to translate, the copied bytes to the host 
> endianness.
Well, I am glad that you are thinking about non-native endianness, but I am 
afraid things just don't work that way. There's no way an API like 
`CopyData(void *, size_t) ` can automatically fix endianness problems. Why? 
Because the fix depends on the structure of the data.

Imagine this sequence of bytes: `00 01 02 03 04 05 06 07`. If this sequence 
really represents a sequence of (little-endian)  bytes (`uint8_t`s), then the 
corresponding "big-endian" sequence would be identical. If, however, it 
represents 2-byte words (uint16_t), then the big-endian representation would be 
`01 00 03 02 05 04 07 06`. For 4-byte words, it would be `03 02 01 00 07 06 05 
04`, etc.

In short, you need to know the structure of the data to translate endiannes, 
and `CopyData` does not know that. Indeed, if you look at the implementation of 
that function you'll see that it just does a `memcpy`.

My suggestion for reading directly into the object was based on the assumption 
that are fine with things working only if everything is of the same endianness. 
I wouldn't have allowed that in more cross-platform code, but I didn't feel 
like policing all corners of lldb.

However, if you do want to make this endian-correct (which I do encourage), 
then you have a couple of options:
- ditch the structs and use data extractor functions to read (GetU32, 
GetAddress, etc) the individual fields -- these know the size of the object 
they are accessing, and can adjust endianness appropriately
- compare host and target endianness and call llvm::sys::swapByteOrder on the 
individual fields if they differ
- use llvm's `packed_endian_specific_integral` instead of native types in the 
struct definitions. This would require passing the endiannes as a template 
parameter and then dispatching to the appropriate struct based on the runtime 
value similar to how you've already done for byte sizes.

Each of these options has its drawbacks, but I've seen them all places 
throughout llvm. In this particular case, I have a feeling the simplest option 
would be to go the DataExtractor route...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78396



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

[Lldb-commits] [PATCH] D77793: Fix LLDB elf core dump register access for ARM/AArch64

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid updated this revision to Diff 259182.
omjavaid added a comment.

Updated diff after implementing core file using assembly code. Also removed the 
redundant register info is null condition.

LGTM?


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

https://reviews.llvm.org/D77793

Files:
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp
  lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h
  lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
  lldb/test/API/functionalities/postmortem/elf-core/aarch64-neon.c
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64-neon.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64.core
  lldb/test/API/functionalities/postmortem/elf-core/linux-aarch64.out

Index: lldb/test/API/functionalities/postmortem/elf-core/aarch64-neon.c
===
--- /dev/null
+++ lldb/test/API/functionalities/postmortem/elf-core/aarch64-neon.c
@@ -0,0 +1,28 @@
+// compile with -march=armv8-a+sve on compatible aarch64 compiler
+// linux-aarch64-sve.core was generated by: aarch64-linux-gnu-gcc-8
+// commandline: -march=armv8-a+sve -nostdlib -static -g linux-aarch64-sve.c
+static void bar(char *boom) {
+  char F = 'b';
+  asm volatile("fmov d0,  #0.5\n\t");
+  asm volatile("fmov d1,  #1.5\n\t");
+  asm volatile("fmov d2,  #2.5\n\t");
+  asm volatile("fmov d3,  #3.5\n\t");
+  asm volatile("fmov s4,  #4.5\n\t");
+  asm volatile("fmov s5,  #5.5\n\t");
+  asm volatile("fmov s6,  #6.5\n\t");
+  asm volatile("fmov s7,  #7.5\n\t");
+  asm volatile("movi v8.16b, #0x11\n\t");
+  asm volatile("movi v31.16b, #0x30\n\t");
+
+  *boom = 47; // Frame bar
+}
+
+static void foo(char *boom, void (*boomer)(char *)) {
+  char F = 'f';
+  boomer(boom); // Frame foo
+}
+
+void _start(void) {
+  char F = '_';
+  foo(0, bar); // Frame _start
+}
Index: lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
===
--- lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
+++ lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py
@@ -19,6 +19,7 @@
 
 mydir = TestBase.compute_mydir(__file__)
 
+_aarch64_pid = 37688
 _i386_pid = 32306
 _x86_64_pid = 32259
 _s390x_pid = 1045
@@ -27,12 +28,20 @@
 _mips_o32_pid = 3532
 _ppc64le_pid = 28147
 
+_aarch64_regions = 4
 _i386_regions = 4
 _x86_64_regions = 5
 _s390x_regions = 2
 _mips_regions = 5
 _ppc64le_regions = 2
 
+
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64(self):
+"""Test that lldb can read the process information from an aarch64 linux core file."""
+self.do_test("linux-aarch64", self._aarch64_pid, self._aarch64_regions, "a.out")
+
 @skipIf(triple='^mips')
 @skipIfLLVMTargetMissing("X86")
 def test_i386(self):
@@ -247,6 +256,61 @@
 
 self.dbg.DeleteTarget(target)
 
+@skipIf(triple='^mips')
+@skipIfLLVMTargetMissing("AArch64")
+def test_aarch64_regs(self):
+# check 64 bit ARM core files
+target = self.dbg.CreateTarget(None)
+self.assertTrue(target, VALID_TARGET)
+process = target.LoadCore("linux-aarch64-neon.core")
+
+values = {}
+values["x1"] = "0x002f"
+values["w1"] = "0x002f"
+values["fp"] = "0x007fc5dd7f20"
+values["lr"] = "0x00400180"
+values["sp"] = "0x007fc5dd7f00"
+values["pc"] = "0x0040014c"
+values["v0"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0xe0 0x3f 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v1"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0xf8 0x3f 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v2"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x04 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v3"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x0c 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v4"] = "{0x00 0x00 0x90 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v5"] = "{0x00 0x00 0xb0 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v6"] = "{0x00 0x00 0xd0 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v7"] = "{0x00 0x00 0xf0 0x40 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v8"] = "{0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11 0x11}"
+values["v27"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v28"] = "{0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00}"
+values["v31"] = "{0x30 0x30 0x30 0x30 0x30 0x30 0x30 0x30 0x

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and ptrace support

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid added a comment.

Also about licensing issues of included macros I am in contact with original 
authors from ARM and I hope they will be able to help sign off this patch for 
LLVM submission.


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

https://reviews.llvm.org/D77047



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

[Lldb-commits] [PATCH] D78588: [lldb/Core] Check that ArchSpec is valid.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The presence of `llvm_unreachable` here is questionable, but I am surprised 
that this comes up in the context of reproducers. If the reproducers cause this 
function to be called with a different ArchSpec, then it sounds like there are 
bigger problems that need to be solved..

If we do want to do something about the crash, then I think we ought to just 
remove the `llvm_unreachable`. I don't think it makes sense to bail out on 
invalid ArchSpecs, but blow up on not-yet-supported architectures. If anything, 
I would say it should be the opposite -- I don't think it makes sense for 
anyone to call this function with an invalid/empty ArchSpec, but it may be 
reasonable to enable some degraded behavior for architectures which are not 
fully supported.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D78588



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

[Lldb-commits] [PATCH] D78603: remove unused function LLDBSwigPython_GetIndexOfChildWithName

[Pavel Labath via Phabricator via lldb-commits]

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

cool


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78603



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

[Lldb-commits] [PATCH] D77793: Fix LLDB elf core dump register access for ARM/AArch64

[Pavel Labath via Phabricator via lldb-commits]

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

Thanks. LGTM, assuming the checked-in core files are of a "reasonable" (~ 
several tens of KB) size.




Comment at: 
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp:51
   lldb::offset_t offset = reg_info->byte_offset;
-  uint64_t v = m_gpr.GetMaxU64(&offset, reg_info->byte_size);
-  if (offset == reg_info->byte_offset + reg_info->byte_size) {
-value = v;
-return true;
+  if (offset < GetGPRSize() && offset < m_gpr.GetByteSize()) {
+uint64_t v = m_gpr.GetMaxU64(&offset, reg_info->byte_size);

I guess all of these conditions should really be `offset + reg_info->byte_size 
<= GetGPRSize()`


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

https://reviews.llvm.org/D77793



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

[Lldb-commits] [PATCH] D77295: [lldb/Core] Fix a race in the Communication class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77295#1995077 , @JDevlieghere 
wrote:

> Hi Pavel,
>
> After this landed I started seeing a bunch of reproducers fail with 
> `SIGPIPE`. I ignored the signal and added a check for `EPIPE` to the 
> `::write` call in `PipePosix.cpp`. Below is the thread state:
>
>   * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGPIPE
> * frame #0: 0x7fff6876c55e libsystem_kernel.dylib`__ulock_wait + 10
>   frame #1: 0x7fff6882f5c2 libsystem_pthread.dylib`_pthread_join + 347
>   frame #2: 0x00010115ef62 
> liblldb.11.0.0git.dylib`lldb_private::HostThreadPosix::Join(this=0x00011e7ca490,
>  result=0x7ffeefbfe290) at HostThreadPosix.cpp:28:15
>   frame #3: 0x00010110b64f 
> liblldb.11.0.0git.dylib`lldb_private::HostThread::Join(this=0x00011e9f09a0,
>  result=0x7ffeefbfe290) at HostThread.cpp:21:27
>   frame #4: 0x0001012ea5ba 
> liblldb.11.0.0git.dylib`lldb_private::Process::ControlPrivateStateThread(this=0x00011e9f0818,
>  signal=1) at Process.cpp:3642:30
>   frame #5: 0x0001012dbc81 
> liblldb.11.0.0git.dylib`lldb_private::Process::StopPrivateStateThread(this=0x00011e9f0818)
>  at Process.cpp:3593:5
>   frame #6: 0x0001012dc514 
> liblldb.11.0.0git.dylib`lldb_private::Process::Destroy(this=0x00011e9f0818,
>  force_kill=true) at Process.cpp:3317:7
>   frame #7: 0x000100981cc1 
> liblldb.11.0.0git.dylib`lldb::SBProcess::Kill(this=0x00011e767de0) at 
> SBProcess.cpp:662:35
>  
>   * thread #2
> * frame #0: 0x7fff6876c4ce libsystem_kernel.dylib`__workq_kernreturn 
> + 10
>   frame #1: 0x7fff6882aaa1 libsystem_pthread.dylib`_pthread_wqthread 
> + 390
>   frame #2: 0x7fff68829b77 libsystem_pthread.dylib`start_wqthread + 15
>  
>   * thread #3, name = ''
> * frame #0: 0x7fff6876c55e libsystem_kernel.dylib`__ulock_wait + 10
>   frame #1: 0x7fff6882f5c2 libsystem_pthread.dylib`_pthread_join + 347
>   frame #2: 0x00010115ef62 
> liblldb.11.0.0git.dylib`lldb_private::HostThreadPosix::Join(this=0x00011e7ca510,
>  result=0x) at HostThreadPosix.cpp:28:15
>   frame #3: 0x00010110b64f 
> liblldb.11.0.0git.dylib`lldb_private::HostThread::Join(this=0x00011e9f0e28,
>  result=0x) at HostThread.cpp:21:27
>   frame #4: 0x000100f46f4e 
> liblldb.11.0.0git.dylib`lldb_private::Communication::StopReadThread(this=0x00011e9f0de8,
>  error_ptr=0x) at Communication.cpp:239:32
>   frame #5: 0x0001012e9d95 
> liblldb.11.0.0git.dylib`lldb_private::Process::ShouldBroadcastEvent(this=0x00011e9f0818,
>  event_ptr=0x00011e76aa40) at Process.cpp:3390:27
>   frame #6: 0x0001012e5e4f 
> liblldb.11.0.0git.dylib`lldb_private::Process::HandlePrivateEvent(this=0x00011e9f0818,
>  event_sp=std::__1::shared_ptr::element_type @ 
> 0x00011e76aa40 strong=2 weak=1) at Process.cpp:3697:33
>   frame #7: 0x0001012eb147 
> liblldb.11.0.0git.dylib`lldb_private::Process::RunPrivateStateThread(this=0x00011e9f0818,
>  is_secondary_thread=false) at Process.cpp:3891:7
>   frame #8: 0x0001012ea2fa 
> liblldb.11.0.0git.dylib`lldb_private::Process::PrivateStateThread(arg=0x00011e7c9a60)
>  at Process.cpp:3789:25
>   frame #9: 0x00010110b45b 
> liblldb.11.0.0git.dylib`lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(arg=0x00011e7ca4e0)
>  at HostNativeThreadBase.cpp:68:10
>   frame #10: 0x00010726ab83 
> liblldb.11.0.0git.dylib`lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(arg=0x00011e7ca4e0)
>  at HostThreadMacOSX.mm:68:10
>  
>   * thread #4, name = '', stop reason = breakpoint 
> 1.1
> * frame #0: 0x00010116071a 
> liblldb.11.0.0git.dylib`lldb_private::PipePosix::Write(this=0x00011e7ca2b0,
>  buf=43698118819080247, size=1, bytes_written=0x776908b8) at 
> PipePosix.cpp:312:7
>   frame #1: 0x000101156d15 
> liblldb.11.0.0git.dylib`lldb_private::ConnectionFileDescriptor::Disconnect(this=0x00011e7ca210,
>  error_ptr=0x) at ConnectionFileDescriptorPosix.cpp:321:30
>   frame #2: 0x000100f454e6 
> liblldb.11.0.0git.dylib`lldb_private::Communication::Disconnect(this=0x00011e9f0de8,
>  error_ptr=0x) at Communication.cpp:98:46
>   frame #3: 0x000100f46d4e 
> liblldb.11.0.0git.dylib`lldb_private::Communication::ReadThread(p=0x00011e9f0de8)
>  at Communication.cpp:377:13
>   frame #4: 0x00010110b45b 
> liblldb.11.0.0git.dylib`lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(arg=0x00011e7ca4e0)
>  at HostNativeThreadBase.cpp:68:10
>   frame #5: 0x00010726ab83 
> liblldb.11.0.0git.dylib`lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(arg=0x00011e7ca4e0)
>  at HostThreadMacOSX.mm:68:10
>   frame #6: 0x7fff6882e109 libsystem_pthread.dylib`_pthread_start + 
> 148
>

[Lldb-commits] [PATCH] D77047: AArch64 SVE register infos and ptrace support

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77047#1996198 , @omjavaid wrote:

> This update includes core file register access for SVE registers. Register 
> descriptions can now be tested with core dump.


Cool. Could you please split off the stuff necessary for making core files work 
into a separate patch, and have the "native" patch be on top of that. I want to 
get an idea of how much of new code is included in the second patch.

> @labath Should I post QEMU SVE setup instructions somewhere if you want to 
> have a go at running included test cases.?

I think that would be very valuable. Not really much for this patch (I probably 
wont get around to running it), but for any future contributors (inlcuding 
myself) whose patches break some arm stuff (be it SVE-related or not).

In D77047#1996202 , @omjavaid wrote:

> Also about licensing issues of included macros I am in contact with original 
> authors from ARM and I hope they will be able to help sign off this patch for 
> LLVM submission.


Awesome.




Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp:1191
+  size_t vq = sve_vq_from_vl(m_sve_header.vl);
+  SetRegisterInfoMode(vq);
+  m_sve_ptrace_payload.resize(SVE_PT_SIZE(vq, SVE_PT_REGS_SVE));

omjavaid wrote:
> SetRegisterInfoMode(vq); is getting called over here.
Ok, I see. That makes sense. The part that seems pretty unfortunate here is 
that you've needed to add a fairly strange function to the generic 
(`RegisterInfoInterface)`  interface even though the caller and callee are in 
arm64-specific code. Let's see if we can avoid that.

What if, instead of remangling the register infos in `RegisterInfoPOSIX_arm64` 
every time that `SetRegisterInfoMode` is called, the "mode" was a argument to 
the class constructor, and changing of the mode was implemented by creating a 
new "info" object? I.e., this code would instead of calling 
`SetRegisterInfoMode(vq);`, do something like `m_register_info_interface_up = 
std::make_unique(vq);`

Would that work?



Comment at: lldb/source/Plugins/Process/Linux/NativeThreadLinux.h:42-43
   NativeRegisterContextLinux &GetRegisterContext() override {
+if (m_reg_context_up && IsStopped(nullptr))
+  m_reg_context_up->ConfigureRegisterContext();
+

omjavaid wrote:
> labath wrote:
> > Why can't this work be done in the register context constructor? I believe 
> > we are always creating a Thread (and its reg context) while it is stopped...
> ConfigureRegisterContext makes sure that sve register offsets and sizes are 
> correctly updated and synced from ptrace on every stop. If SVE registers 
> reconfigure themselves we ll have to update that information into our dynamic 
> register infos array. Although for the context of this patch it assumes it 
> will never change its register size during execution. But I will follow up 
> patch that implements that behaviour.
Ok, that makes sense now. The thing on my mind in that case is that this 
functionality overlaps a lot with `InvalidateAllRegisters` we've added a couple 
of patches back. I think it would be nice to have just one function which 
"reinitializes" a register context. `InvalidateAllRegisters` wouldn't work 
right now, because it's called before a resume, but if we changed it to be 
called after a stop, it would be able to do both things.

Would it be possible to change it so that this `InvalidateAllRegisters` is 
called after a stop, e.g. in `NativeThreadLinux::SetStopped`. I am sorry for 
the churn, I know it was my idea to call it before a resume -- it seemed like a 
reasonable thing to do at the time, but not any more.. :(


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

https://reviews.llvm.org/D77047



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

[Lldb-commits] [PATCH] D78337: [lldb/Host] Remove TaskPool and replace its uses with llvm::ThreadPool

[Pavel Labath via Phabricator via lldb-commits]

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

Cool. I'm not sure how useful is the `TaskMapOverInt` function in this new 
world (if I was writing this from scratch, I don't think I would create a 
function for that), but it doesn't hurt that much either.


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

https://reviews.llvm.org/D78337



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

[Lldb-commits] [PATCH] D75750: [lldb] integrate debuginfod

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1993990 , @jankratochvil 
wrote:

> In D75750#1991873 , @labath wrote:
>
> > In D75750#1988694 , @jankratochvil 
> > wrote:
> >
> > > The current plan discussed with @kwk is to create the new `SymbolServer` 
> > > abstract superclass and some its inherited implementation and move there 
> > > the appropriate parts of existing 
> > > `lldb/source/Symbol/LocateSymbolFile.cpp`. Current `SymbolVendor` 
> > > implementations would then iterate new `SymbolServer`s by the existing 
> > > `LocateExecutableSymbolFile` function. That may be enough for a patch of 
> > > its own.
> >
> >
> > I'll have to see the actual patch for a definitive opinion, but I have to 
> > say that a priori I am sceptical of this direction. And yes, that should 
> > definitely be a separate patch.
>
>
> This separate `SymbolServer` is following @clayborg's comment above 
> .
>  You proposed to merge `SymbolServer` with `SymbolVendor` in @labath's 
> comment above .
>  I found more clean the separate `SymbolServer` variant as there is 
> orthogonal functionality of locating the files (on disk or from symbol server 
> - `SymbolServer`) vs. extracting the unique ID from current file (extracting 
> build-id - `SymbolVendor` functionality). So from the both proposed solutions 
> I preferred the @clayborg's comment above 
> .
>  I hope there is no misunderstanding which could lead to @kwk implementing a 
> third solution nobody wants.


I think that you two and Greg are mostly in sync, but I am yet to be convinced 
that this indeed the right solution. My reasons for that are two fold:

- the existing SymbolVendor implementations are very simple (and most 
importantly, stateless). In fact they are so simple, I was contemplating simply 
removing them and replacing by a couple of free functions. Surely, we don't 
need an entire class hierarchy to implement "extracting the unique ID from 
current file". Implementing symbol server functionality would give these 
classes a reason to exist, as there would now be an actual state that they need 
to maintain (a connection to the symbol server, or at least its url)
- I don't think the separation between "SymbolServer" and "SymbolVendor" will 
be as clear as you make it sound to be. The "searching" aspect is fairly 
trivial in SymbolVendorELF, and it does boil down to a 
`LocateExecutableSymbolFile` call. The situation is a bit fuzzier with 
SymbolVendorMacOSX, which also handles some path remapping aspects. But that is 
the job of the symbol "server" in the debuginfod world. It's not clear to me 
how you're going to align these two things without "vendors" and "servers" 
separate.

Now you can try to implement a patch and demonstrate how things will work that 
way and hope to convince me that way, or we can to talk about abstractly, and 
come up with some sort of a "design doc" for this thing. Up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750



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

[Lldb-commits] [PATCH] D77970: 2/2: [nfc] [lldb] DWARF callbacks: DIERef -> DWARFDIE

[Pavel Labath via Phabricator via lldb-commits]

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

Looks good, just please write more explanatory commit message than what's 
present in this patch description.




Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp:75-80
+  DWARFDIE die = m_dwarf.GetDIE(ref);
+  if (!die) {
+m_index.ReportInvalidDIERef(ref, m_name);
+return true;
+  }
+  return m_callback(die);

reverse the if condition
```
if (DWARFDIE die = m_dwarf.GETDIE(ref))
  return m_callback(die);
...
```



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h:93
+
+  void ReportInvalidDIERef(DIERef ref, llvm::StringRef name) const;
 };

jankratochvil wrote:
> Invalid `DIERef` is now reported for all Indexes, not just `AppleDWARFIndex`.
> 
That's fine. The error message that produces is really geared towards 
accelerator tables, but the manual index should not produce such an error 
anyway (I would have put an assert there it was convenient (but it isn't)). And 
I think I'm going to change the debug_names index to not use DIERefs once this 
lands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77970



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

[Lldb-commits] [PATCH] D78489: [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address tables

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D78489#1995837 , @clayborg wrote:

> Sounds like a win then, as long as we don't get slower I am fine with this. I 
> was guessing that line tables might be faster because it is generally very 
> compressed and compact compared to debug info, but thanks for verifying.
>
> Might be worth checking the memory consumption of LLDB quickly too with 
> DW_AT_ranges compiled out and just make sure we don't take up too much extra 
> memory.


I've done a similar benchmark to the last one, but measured memory usage 
("RssAnon" as reported by linux). One notable difference is that I

Currently (information retrieved from DW_AT_ranges) we use about ~330 MB of 
memory. If I switch to dwarf dies as the source, the memory goes all the way to 
2890 MB. This number is suspiciously large -- it either means that our die 
freeing is not working properly, or that glibc is very bad at releasing memory 
back to the OS. Given the magnitude of the increase, i think it's a little bit 
of both. With line tables as the source the memory usage is 706 MB. It's an 
increase from 330, but definitely smaller than 2.8 GB. (the number 330 is kind 
of misleading here since we're not considering removing that option -- it will 
always be used if it is available).

> We aren't doing anything to unload these line tables like we do with DIEs are 
> we? It might make sense to pares the line tables and then throw them away if 
> they were not already parsed?  With the DIEs we were throwing them away if 
> they weren't already parsed to keep memory consumption down, so might be 
> worth throwing the line tables away after running this if we are now going to 
> rely on it.

That would be possible, but I don't think it's worth the trouble. I think the 
phrase "relying on it" overemphasizes the importance of that code. In practice, 
the only time when this path will be taken is when debugging code built with 
clang<=3.3, which is seven years old and did not even fully implement c++11. It 
also seems like the switch to line tables will save memory, at least until the 
die freeing bug is fixed. And lastly, the difference i reported is pretty much 
the worst possible case, as the only thing the debugger will do is parse the 
line tables and exit. Once the debugger starts doing other stuff too, the 
difference will start to fade (e.g. running to the breakpoint in main increases 
the memory usage to 600 MB even with DW_AT_ranges).

> One other thing to verify we want to go this route is to re-enable the old 
> DIE parsing for high/low PCs, but put the results in a separate address 
> ranges class. After we parse the line tables, verify that all ranges we found 
> in the DIEs are in the line tables? It would not be great if we were going to 
> start missing some functions if a function doesn't have a line table? Not 
> sure if/how this would happen, but you never know.

I implemented a check like that. While doing it I've learned that the DIE-based 
parsing does not work since May 2018 (D47275 ) 
and nobody noticed. What that means is that in practice we were always going 
through line tables (if DW_AT_ranges were missing). It also means that my times 
reported earlier for die-based searching were incorrect as they also included 
the time to build the line tables. (However, that does not change the ordering, 
as even if we subtract the time it took to parse the line tables, the DIE 
method is still much slower).

After fixing the die-based search, I was able to verify that the die-based 
ranges are an exact match to the line table ranges (for the particular compiler 
used -- top of tree clang).

Given all of the above (die-based searching being slow, taking more memory, and 
not actually working), i think it's pretty clear that we should remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78489



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

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Gabor Marton via Phabricator via lldb-commits]

martong accepted this revision.
martong added a comment.

LGTM! Thanks!


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

https://reviews.llvm.org/D78000



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

[Lldb-commits] [lldb] d482fe2 - [nfc] [lldb] DWARF callbacks: DIERef -> DWARFDIE

[Jan Kratochvil via lldb-commits]

Author: Jan Kratochvil
Date: 2020-04-22T17:11:50+02:00
New Revision: d482fe2add95c38691edb2c91d0f5681bebcd81d

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

LOG: [nfc] [lldb] DWARF callbacks: DIERef -> DWARFDIE

Pavel Labath wrote in D73206:
The internal representation of DebugNames and Apple indexes is fixed by
the relevant (pseudo-)standards, so we can't really change it. The
question is how to efficiently (and cleanly) convert from the internal
representation to some common thing. The conversion from AppleIndex to
DIERef is trivial (which is not surprising as it was the first and the
overall design was optimized for that). With debug_names, the situation
gets more tricky. The internal representation of debug_names uses
CU-relative DIE offsets, but DIERef wants an absolute offset. That means
the index has to do more work to produce the common representation. And
it needs to do that for all results, even though a lot of the index
users are really interested only in a single entry. With the switch to
user_id_t, _all_ indexes would have to do some extra work to encode it,
only for their users to have to immediately decode it back. Having
a iterator/callback based api would allow us to minimize the impact of
that, as it would only need to happen for the entries that are really
used. And /I think/ we could make it interface returns DWARFDies
directly, and each index converts to that using the most direct approach
available.

Jan Kratochvil:
It also makes all the callers shorter as they no longer need to fetch
DWARFDIE from DIERef (and handling if not found by ReportInvalidDIERef)
but the callers are already served DWARFDIE which they need.
In some cases the DWARFDIE had to be fetched both by callee (DWARFIndex
implementation) and caller.

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
index ad34a754ad7e..33ab11a3ca40 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
@@ -53,59 +53,69 @@ std::unique_ptr AppleDWARFIndex::Create(
 }
 
 void AppleDWARFIndex::GetGlobalVariables(
-ConstString basename, llvm::function_ref callback) {
+ConstString basename, llvm::function_ref callback) {
   if (!m_apple_names_up)
 return;
-  m_apple_names_up->FindByName(basename.GetStringRef(), callback);
+  m_apple_names_up->FindByName(
+  basename.GetStringRef(),
+  DIERefCallback(callback, basename.GetStringRef()));
 }
 
 void AppleDWARFIndex::GetGlobalVariables(
 const RegularExpression ®ex,
-llvm::function_ref callback) {
+llvm::function_ref callback) {
   if (!m_apple_names_up)
 return;
 
   DWARFMappedHash::DIEInfoArray hash_data;
   m_apple_names_up->AppendAllDIEsThatMatchingRegex(regex, hash_data);
-  DWARFMappedHash::ExtractDIEArray(hash_data, callback);
+  // This is not really the DIE name.
+  DWARFMappedHash::ExtractDIEArray(hash_data,
+   DIERefCallback(callback, regex.GetText()));
 }
 
 void AppleDWARFIndex::GetGlobalVariables(
-const DWARFUnit &cu, llvm::function_ref callback) {
+const DWARFUnit &cu, llvm::function_ref callback) {
   if (!m_apple_names_up)
 return;
 
   DWARFMappedHash::DIEInfoArray hash_data;
   m_apple_names_up->AppendAllDIEsInRange(cu.GetOffset(), 
cu.GetNextUnitOffset(),
  hash_data);
-  DWARFMappedHash::ExtractDIEArray(hash_data, callback);
+  DWARFMappedHash::ExtractDIEArray(hash_data, DIERefCallback(callback));
 }
 
 void AppleDWARFIndex::GetObjCMethods(
-ConstString class_name, llvm::function_ref callback) {
+ConstString class_name, llvm::function_ref callback) {
   if (!m_apple_objc_up)
 return;
-  m_apple_objc_up->FindByName(class_name.GetStringRef(), callback);
+  m_apple_objc_up->FindByName(
+  cla

[Lldb-commits] [PATCH] D77043: Fix process gdb-remote usage of value_regs/invalidate_regs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Where does the option which I proposed fit in? It sounds like it would be 
something like 1a), where we don't actually modify the "invalidate" numbers 
stored within lldb-server, but we just do the conversion at the protocol level. 
That way the "invalidate" numbers keep their meaning as "lldb" register numbers 
in lldb-server, just like they used to. They also keep referring to "lldb" 
register numbers on the client side, because the client puts the register 
"index" into the "lldb" field. The only inconsistency is that the "lldb" 
register numbers assigned to a specific register will be different on the 
client and server side. However:

- this is not different from what happens in your patch, because you put the 
"server lldb" number into the "process plugin" field on the client
- it's not something we should actually rely on if we want to be able to 
communicate with non-matching server stubs

It seems to me like this solution has the same downsides as the current patch, 
while having the upside of not requiring protocol changes. That means it can be 
revisited if we find it to be a problem, while a protocol change is more 
permanent. That's why I'd like to get a good understanding of why it won't work 
(or be ugly) if we're not going to do it, and so far I haven't gotten that.

(BTW, if other aarch64 targets (which ones?) are reporting debug registers in 
their register lists, I don't think it would be unreasonable to do the same on 
linux)


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

https://reviews.llvm.org/D77043



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

[Lldb-commits] [PATCH] D77970: 2/2: [nfc] [lldb] DWARF callbacks: DIERef -> DWARFDIE

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

Thanks for the review and I have learned a new use case for 
`llvm::function_ref`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77970



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

[Lldb-commits] [PATCH] D77970: 2/2: [nfc] [lldb] DWARF callbacks: DIERef -> DWARFDIE

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd482fe2add95: [nfc] [lldb] DWARF callbacks: DIERef -> 
DWARFDIE (authored by jankratochvil).

Changed prior to commit:
  https://reviews.llvm.org/D77970?vs=258921&id=259303#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77970

Files:
  lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h
@@ -32,7 +32,7 @@
   DWARFCompileUnit *GetDWOCompileUnitForHash(uint64_t hash);
 
   void GetObjCMethods(lldb_private::ConstString class_name,
-  llvm::function_ref callback) override;
+  llvm::function_ref callback) override;
 
   llvm::Expected
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
@@ -97,7 +97,7 @@
 
 void SymbolFileDWARFDwo::GetObjCMethods(
 lldb_private::ConstString class_name,
-llvm::function_ref callback) {
+llvm::function_ref callback) {
   GetBaseSymbolFile().GetObjCMethods(class_name, callback);
 }
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -238,7 +238,7 @@
   GetCompUnitForDWARFCompUnit(DWARFCompileUnit &dwarf_cu);
 
   virtual void GetObjCMethods(lldb_private::ConstString class_name,
-  llvm::function_ref callback);
+  llvm::function_ref callback);
 
   bool Supports_DW_AT_APPLE_objc_complete_type(DWARFUnit *cu);
 
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -1464,7 +1464,7 @@
 }
 
 void SymbolFileDWARF::GetObjCMethods(
-ConstString class_name, llvm::function_ref callback) {
+ConstString class_name, llvm::function_ref callback) {
   m_index->GetObjCMethods(class_name, callback);
 }
 
@@ -2051,17 +2051,11 @@
   uint32_t pruned_idx = original_size;
 
   SymbolContext sc;
-  m_index->GetGlobalVariables(ConstString(basename), [&](DIERef die_ref) {
+  m_index->GetGlobalVariables(ConstString(basename), [&](DWARFDIE die) {
 if (!sc.module_sp)
   sc.module_sp = m_objfile_sp->GetModule();
 assert(sc.module_sp);
 
-DWARFDIE die = GetDIE(die_ref);
-if (!die) {
-  m_index->ReportInvalidDIERef(die_ref, name.GetStringRef());
-  return true;
-}
-
 if (die.Tag() != DW_TAG_variable)
   return true;
 
@@ -2123,17 +2117,11 @@
   const uint32_t original_size = variables.GetSize();
 
   SymbolContext sc;
-  m_index->GetGlobalVariables(regex, [&](DIERef die_ref) {
+  m_index->GetGlobalVariables(regex, [&](DWARFDIE die) {
 if (!sc.module_sp)
   sc.module_sp = m_objfile_sp->GetModule();
 assert(sc.module_sp);
 
-DWARFDIE die = GetDIE(die_ref);
-if (!die) {
-  m_index->ReportInvalidDIERef(die_ref, regex.GetText());
-  return true;
-}
-
 DWARFCompileUnit *dwarf_cu = llvm::dyn_cast(die.GetCU());
 if (!dwarf_cu)
   return true;
@@ -2292,14 +2280,8 @@
 regex.GetText().str().c_str());
   }
 
-  DWARFDebugInfo &info = DebugInfo();
   llvm::DenseSet resolved_dies;
-  m_index->GetFunctions(regex, [&](DIERef ref) {
-DWARFDIE die = info.GetDIE(ref);
-if (!die) {
-  m_index->ReportInvalidDIERef(ref, regex.GetText());
-  return true;
-}
+  m_index->GetFunctions(regex, [&](DWARFDIE die) {
 if (resolved_dies.insert(die.GetDIE()).second)
   ResolveFunction(die, include_inlines, sc_list);

[Lldb-commits] [lldb] 2dea3f1 - [SVE] Add new VectorType subclasses

[Christopher Tetreault via lldb-commits]

Author: Christopher Tetreault
Date: 2020-04-22T08:59:01-07:00
New Revision: 2dea3f129878e929e5d1f00b91a622eb1ec8be4e

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

LOG: [SVE] Add new VectorType subclasses

Summary:
Introduce new types for fixed width and scalable vectors.

Does not remove getNumElements yet so as to not break code during transition
period.

Reviewers: deadalnix, efriedma, sdesmalen, craig.topper, huntergr

Reviewed By: sdesmalen

Subscribers: jholewinski, arsenm, jvesely, nhaehnle, mehdi_amini, rriddle, 
jpienaar, burmako, shauheen, antiagainst, nicolasvasilache, csigg, 
arpith-jacob, mgester, lucyrfox, liufengdb, kerbowa, Joonsoo, grosul1, 
frgossen, lldb-commits, tschuett, hiraditya, rkruppe, psnobl, llvm-commits

Tags: #llvm, #lldb

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

Added: 


Modified: 
lldb/source/Expression/IRInterpreter.cpp
llvm/include/llvm-c/Core.h
llvm/include/llvm/IR/DataLayout.h
llvm/include/llvm/IR/DerivedTypes.h
llvm/include/llvm/IR/Type.h
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
llvm/lib/CodeGen/ValueTypes.cpp
llvm/lib/ExecutionEngine/ExecutionEngine.cpp
llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
llvm/lib/IR/AsmWriter.cpp
llvm/lib/IR/Constants.cpp
llvm/lib/IR/Core.cpp
llvm/lib/IR/DataLayout.cpp
llvm/lib/IR/Type.cpp
llvm/lib/Linker/IRMover.cpp
llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
llvm/lib/Target/Hexagon/HexagonTargetObjectFile.cpp
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
llvm/lib/Transforms/IPO/GlobalOpt.cpp
llvm/lib/Transforms/Utils/FunctionComparator.cpp
llvm/tools/llvm-c-test/echo.cpp
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

Removed: 




diff  --git a/lldb/source/Expression/IRInterpreter.cpp 
b/lldb/source/Expression/IRInterpreter.cpp
index 3ae843f69d70..ddb2d975d554 100644
--- a/lldb/source/Expression/IRInterpreter.cpp
+++ b/lldb/source/Expression/IRInterpreter.cpp
@@ -597,7 +597,8 @@ bool IRInterpreter::CanInterpret(llvm::Module &module, 
llvm::Function &function,
 switch (operand_type->getTypeID()) {
 default:
   break;
-case Type::VectorTyID: {
+case Type::FixedVectorTyID:
+case Type::ScalableVectorTyID: {
   LLDB_LOGF(log, "Unsupported operand type: %s",
 PrintType(operand_type).c_str());
   error.SetErrorString(unsupported_operand_error);

diff  --git a/llvm/include/llvm-c/Core.h b/llvm/include/llvm-c/Core.h
index 852e16400f3a..2d6490dcbcf1 100644
--- a/llvm/include/llvm-c/Core.h
+++ b/llvm/include/llvm-c/Core.h
@@ -157,10 +157,11 @@ typedef enum {
   LLVMStructTypeKind,  /**< Structures */
   LLVMArrayTypeKind,   /**< Arrays */
   LLVMPointerTypeKind, /**< Pointers */
-  LLVMVectorTypeKind,  /**< SIMD 'packed' format, or other vector type */
   LLVMMetadataTypeKind,/**< Metadata */
   LLVMX86_MMXTypeKind, /**< X86 MMX */
-  LLVMTokenTypeKind/**< Tokens */
+  LLVMTokenTypeKind,   /**< Tokens */
+  LLVMFixedVectorTypeKind, /**< Fixed width SIMD vector type */
+  LLVMScalableVectorTypeKind /**< Scalable SIMD vector type */
 } LLVMTypeKind;
 
 typedef enum {

diff  --git a/llvm/include/llvm/IR/DataLayout.h 
b/llvm/include/llvm/IR/DataLayout.h
index 98bdf30f5a46..010469c8107b 100644
--- a/llvm/include/llvm/IR/DataLayout.h
+++ b/llvm/include/llvm/IR/DataLayout.h
@@ -664,7 +664,8 @@ inline TypeSize DataLayout::getTypeSizeInBits(Type *Ty) 
const {
   // only 80 bits contain information.
   case Type::X86_FP80TyID:
 return TypeSize::Fixed(80);
-  case Type::VectorTyID: {
+  case Type::FixedVectorTyID:
+  case Type::ScalableVectorTyID: {
 VectorType *VTy = cast(Ty);
 auto EltCnt = VTy->getElementCount();
 uint64_t MinBits = EltCnt.Min *

diff  --git a/llvm/include/llvm/IR/DerivedTypes.h 
b/llvm/include/llvm/IR/DerivedTypes.h
index 306d388cb8a7..75c48f301026 100644
--- a/llvm/include/llvm/IR/DerivedTypes.h
+++ b/llvm/include/llvm/IR/DerivedTypes.h
@@ -386,7 +386,7 @@ uint64_t Type::getArrayNumElements() const {
   return cast(this)->getNumElements();
 }
 
-/// Class to represent vector types.
+/// Base class of all SIMD vector types
 class VectorType : public Type {
   /// A fully specified VectorType is of the form . 'n' is the
   /// minimum number of elements of type Ty contained within the vector, and
@@ -403,24 +403,22 @@ class VectorType : public Type {
 
   /// The element type of the vector.
   Type *ContainedType;
-  /// Minumum number of elements in the vector.
-  uint64_t NumElements;
 
-  VectorType(Type *ElType, unsigned NumEl, bool Scalable = false);
-  VectorType(Type *ElType, ElementCount EC);
+  /// The 

[Lldb-commits] [PATCH] D78588: [lldb/Core] Check that ArchSpec is valid.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D78588#1996222 , @labath wrote:

> The presence of `llvm_unreachable` here is questionable, but I am surprised 
> that this comes up in the context of reproducers. If the reproducers cause 
> this function to be called with a different ArchSpec, then it sounds like 
> there are bigger problems that need to be solved..


I only mentioned the reproducers to show that we can reach this code with an 
invalid ArchSpec and that this is not just a speculative fix. Since you're 
interested in what triggered this: TestRecognizeBreakpoint.py is a 
GDBRemoteTestBase, which starts with an empty Target (hence the invalid 
ArchSpec later on) and then connects to the test's GDB remote. It does so 
through `ConnectRemote` which for `ProcessGDBRemote` calls 
`ConnectToDebugserver`, which bypasses redirecting the connection to the GDB 
replay server. Fixing the reproducers issue is just a matter of moving that 
code down, so we connect to the replay server instead of the test's gdb server 
(which isn't there during active replay).

> If we do want to do something about the crash, then I think we ought to just 
> remove the `llvm_unreachable`. I don't think it makes sense to bail out on 
> invalid ArchSpecs, but blow up on not-yet-supported architectures. If 
> anything, I would say it should be the opposite -- I don't think it makes 
> sense for anyone to call this function with an invalid/empty ArchSpec, but it 
> may be reasonable to enable some degraded behavior for architectures which 
> are not fully supported.

Sounds reasonable to me. We can add an assert `arch.IsValid()` to enforce that 
precondition and have the default case return `0`.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D78588



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

[Lldb-commits] [lldb] e57361c - [lldb/Host] Remove TaskPool and replace its uses with llvm::ThreadPool

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-04-22T09:17:49-07:00
New Revision: e57361c055d7617ee25cdac8167625000d098ef5

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

LOG: [lldb/Host] Remove TaskPool and replace its uses with llvm::ThreadPool

Remove LLDB's TaskPool and replace its uses with LLVM's ThreadPool.

Differential revision: https://reviews.llvm.org/D78337

Added: 


Modified: 
lldb/include/lldb/module.modulemap
lldb/source/Host/CMakeLists.txt
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
lldb/unittests/Host/CMakeLists.txt

Removed: 
lldb/include/lldb/Host/TaskPool.h
lldb/source/Host/common/TaskPool.cpp
lldb/unittests/Host/TaskPoolTest.cpp



diff  --git a/lldb/include/lldb/Host/TaskPool.h 
b/lldb/include/lldb/Host/TaskPool.h
deleted file mode 100644
index bb91d807ed2c..
--- a/lldb/include/lldb/Host/TaskPool.h
+++ /dev/null
@@ -1,92 +0,0 @@
-//===- TaskPool.h ---*- C++ 
-*-===//
-//
-// 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
-//
-//===--===//
-
-#ifndef LLDB_HOST_TASKPOOL_H
-#define LLDB_HOST_TASKPOOL_H
-
-#include "llvm/ADT/STLExtras.h"
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-namespace lldb_private {
-
-// Global TaskPool class for running tasks in parallel on a set of worker
-// thread created the first time the task pool is used. The TaskPool provide no
-// guarantee about the order the task will be run and about what tasks will run
-// in parallel. None of the task added to the task pool should block on
-// something (mutex, future, condition variable) what will be set only by the
-// completion of an other task on the task pool as they may run on the same
-// thread sequentally.
-class TaskPool {
-public:
-  // Add a new task to the task pool and return a std::future belonging to the
-  // newly created task. The caller of this function has to wait on the future
-  // for this task to complete.
-  template 
-  static std::future::type>
-  AddTask(F &&f, Args &&... args);
-
-  // Run all of the specified tasks on the task pool and wait until all of them
-  // are finished before returning. This method is intended to be used for
-  // small number tasks where listing them as function arguments is acceptable.
-  // For running large number of tasks you should use AddTask for each task and
-  // then call wait() on each returned future.
-  template  static void RunTasks(T &&... tasks);
-
-private:
-  TaskPool() = delete;
-
-  template  struct RunTaskImpl;
-
-  static void AddTaskImpl(std::function &&task_fn);
-};
-
-template 
-std::future::type>
-TaskPool::AddTask(F &&f, Args &&... args) {
-  auto task_sp = std::make_shared<
-  std::packaged_task::type()>>(
-  std::bind(std::forward(f), std::forward(args)...));
-
-  AddTaskImpl([task_sp]() { (*task_sp)(); });
-
-  return task_sp->get_future();
-}
-
-template  void TaskPool::RunTasks(T &&... tasks) {
-  RunTaskImpl::Run(std::forward(tasks)...);
-}
-
-template 
-struct TaskPool::RunTaskImpl {
-  static void Run(Head &&h, Tail &&... t) {
-auto f = AddTask(std::forward(h));
-RunTaskImpl::Run(std::forward(t)...);
-f.wait();
-  }
-};
-
-template <> struct TaskPool::RunTaskImpl<> {
-  static void Run() {}
-};
-
-// Run 'func' on every value from begin .. end-1.  Each worker will grab
-// 'batch_size' numbers at a time to work on, so for very fast functions, batch
-// should be large enough to avoid too much cache line contention.
-void TaskMapOverInt(size_t begin, size_t end,
-const llvm::function_ref &func);
-
-unsigned GetHardwareConcurrencyHint();
-
-} // namespace lldb_private
-
-#endif // LLDB_HOST_TASKPOOL_H

diff  --git a/lldb/include/lldb/module.modulemap 
b/lldb/include/lldb/module.modulemap
index e668abe1c6ae..7feea8ee99c3 100644
--- a/lldb/include/lldb/module.modulemap
+++ b/lldb/include/lldb/module.modulemap
@@ -49,7 +49,6 @@ module lldb_Host {
   module SocketAddress { header "Host/SocketAddress.h" export * }
   module Socket { header "Host/Socket.h" export * }
   module StringConvert { textual header "Host/StringConvert.h" export * }
-  module TaskPool { header "Host/TaskPool.h" export * }
   module Terminal { header "Host/Terminal.h" export * }
   module ThreadLauncher { header "Host/ThreadLauncher.h" export * }
   module Time { header "Host/Time.h" export * }

diff  --git a/lldb/source/Host/CMakeLists.txt b/lldb/source/Host/CMakeLists.txt
index 357140432010..2837c0c5e3a1 100644
--- a/lldb/source/Host/CMakeLists.txt
+++ b/lldb/source/

[Lldb-commits] [PATCH] D78648: [CMake] Bump CMake minimum version to 3.13.4

[Louis Dionne via Phabricator via lldb-commits]

ldionne created this revision.
Herald added subscribers: libcxx-commits, openmp-commits, lldb-commits, 
Sanitizers, cfe-commits, frgossen, grosul1, jvesely, Joonsoo, liufengdb, 
lucyrfox, mgester, arpith-jacob, nicolasvasilache, antiagainst, shauheen, 
jpienaar, rriddle, mehdi_amini, lebedev.ri, dexonsmith, jkorous, whisperity, 
mgorny.
Herald added a reviewer: bollu.
Herald added a reviewer: lebedev.ri.
Herald added a reviewer: DavidTruby.
Herald added projects: clang, Sanitizers, LLDB, libc++, OpenMP, libc++abi, 
libunwind.
Herald added a reviewer: libc++.
Herald added a reviewer: libc++abi.
Herald added a reviewer: libunwind.
ldionne added a parent revision: D78646: [CMake] Enforce the minimum CMake 
version to be at least 3.13.4.
Herald added a reviewer: jdoerfert.
ldionne added a comment.
ldionne accepted this revision as: libc++, libc++abi, libunwind.

This review is for the upcoming CMake upgrade once we branch off the LLVM 11 
release branch.

I won't apply it until after the branch has been created, which is around July.


This upgrade should be friction-less because we've already been ensuring
that CMake >= 3.13.4 is used.

This is part of the effort discussed on llvm-dev here:

  http://lists.llvm.org/pipermail/llvm-dev/2020-April/140578.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78648

Files:
  clang/CMakeLists.txt
  clang/tools/scan-build-py/tests/functional/exec/CMakeLists.txt
  compiler-rt/CMakeLists.txt
  compiler-rt/cmake/Modules/CustomLibcxx/CMakeLists.txt
  compiler-rt/lib/builtins/CMakeLists.txt
  flang/CMakeLists.txt
  libclc/CMakeLists.txt
  libcxx/CMakeLists.txt
  libcxxabi/CMakeLists.txt
  libunwind/CMakeLists.txt
  lld/CMakeLists.txt
  lldb/CMakeLists.txt
  lldb/tools/debugserver/CMakeLists.txt
  llvm/CMakeLists.txt
  llvm/docs/CMake.rst
  llvm/docs/CMakePrimer.rst
  llvm/docs/GettingStarted.rst
  llvm/runtimes/CMakeLists.txt
  llvm/utils/benchmark/CMakeLists.txt
  mlir/examples/standalone/CMakeLists.txt
  openmp/CMakeLists.txt
  openmp/cmake/DetectTestCompiler/CMakeLists.txt
  openmp/runtime/cmake/LibompCheckLinkerFlag.cmake
  parallel-libs/CMakeLists.txt
  parallel-libs/acxxel/CMakeLists.txt
  polly/CMakeLists.txt
  pstl/CMakeLists.txt

Index: pstl/CMakeLists.txt
===
--- pstl/CMakeLists.txt
+++ pstl/CMakeLists.txt
@@ -5,7 +5,7 @@
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 #
 #===--===##
-cmake_minimum_required(VERSION 3.4.3)
+cmake_minimum_required(VERSION 3.13.4)
 
 set(PARALLELSTL_VERSION_FILE "${CMAKE_CURRENT_SOURCE_DIR}/include/pstl/internal/pstl_config.h")
 file(STRINGS "${PARALLELSTL_VERSION_FILE}" PARALLELSTL_VERSION_SOURCE REGEX "#define _PSTL_VERSION .*$")
Index: polly/CMakeLists.txt
===
--- polly/CMakeLists.txt
+++ polly/CMakeLists.txt
@@ -1,7 +1,7 @@
 # Check if this is a in tree build.
 if (NOT DEFINED LLVM_MAIN_SRC_DIR)
   project(Polly)
-  cmake_minimum_required(VERSION 3.4.3)
+  cmake_minimum_required(VERSION 3.13.4)
 
   # Where is LLVM installed?
   find_package(LLVM CONFIG REQUIRED)
Index: parallel-libs/acxxel/CMakeLists.txt
===
--- parallel-libs/acxxel/CMakeLists.txt
+++ parallel-libs/acxxel/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 3.1)
+cmake_minimum_required(VERSION 3.13.4)
 
 option(ACXXEL_ENABLE_UNIT_TESTS "enable acxxel unit tests" ON)
 option(ACXXEL_ENABLE_MULTI_DEVICE_UNIT_TESTS "enable acxxel multi-device unit tests" OFF)
Index: parallel-libs/CMakeLists.txt
===
--- parallel-libs/CMakeLists.txt
+++ parallel-libs/CMakeLists.txt
@@ -1 +1 @@
-cmake_minimum_required(VERSION 3.1)
+cmake_minimum_required(VERSION 3.13.4)
Index: openmp/runtime/cmake/LibompCheckLinkerFlag.cmake
===
--- openmp/runtime/cmake/LibompCheckLinkerFlag.cmake
+++ openmp/runtime/cmake/LibompCheckLinkerFlag.cmake
@@ -17,7 +17,7 @@
   set(library_source
 "int foo(int a) { return a*a; }")
   set(cmake_source
-"cmake_minimum_required(VERSION 2.8)
+"cmake_minimum_required(VERSION 3.13.4)
  project(foo C)
  set(CMAKE_SHARED_LINKER_FLAGS \"${flag}\")
  add_library(foo SHARED src_to_link.c)")
Index: openmp/cmake/DetectTestCompiler/CMakeLists.txt
===
--- openmp/cmake/DetectTestCompiler/CMakeLists.txt
+++ openmp/cmake/DetectTestCompiler/CMakeLists.txt
@@ -1,4 +1,4 @@
-cmake_minimum_required(VERSION 2.8)
+cmake_minimum_required(VERSION 3.13.4)
 project(DetectTestCompiler C CXX)
 
 include(CheckCCompilerFlag)
Index: openmp/CMakeLists.txt
===
--- openmp/CMakeLists.txt
+++ openmp/CMakeLists.txt
@@ -1,4 +1,4

[Lldb-commits] [PATCH] D78648: [CMake] Bump CMake minimum version to 3.13.4

[Louis Dionne via Phabricator via lldb-commits]

ldionne added a comment.

This review is for the upcoming CMake upgrade once we branch off the LLVM 11 
release branch.

I won't apply it until after the branch has been created, which is around July.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78648



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

[Lldb-commits] [PATCH] D77587: [SVE] Add new VectorType subclasses

[Christopher Tetreault via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2dea3f129878: [SVE] Add new VectorType subclasses (authored 
by ctetreau).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77587

Files:
  lldb/source/Expression/IRInterpreter.cpp
  llvm/include/llvm-c/Core.h
  llvm/include/llvm/IR/DataLayout.h
  llvm/include/llvm/IR/DerivedTypes.h
  llvm/include/llvm/IR/Type.h
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/CodeGen/ValueTypes.cpp
  llvm/lib/ExecutionEngine/ExecutionEngine.cpp
  llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
  llvm/lib/IR/AsmWriter.cpp
  llvm/lib/IR/Constants.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/IR/DataLayout.cpp
  llvm/lib/IR/Type.cpp
  llvm/lib/Linker/IRMover.cpp
  llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp
  llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp
  llvm/lib/Target/Hexagon/HexagonTargetObjectFile.cpp
  llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
  llvm/lib/Transforms/IPO/GlobalOpt.cpp
  llvm/lib/Transforms/Utils/FunctionComparator.cpp
  llvm/tools/llvm-c-test/echo.cpp
  mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

Index: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
===
--- mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
+++ mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
@@ -168,12 +168,12 @@
   return nullptr;
 return LLVMType::getArrayTy(elementType, type->getArrayNumElements());
   }
-  case llvm::Type::VectorTyID: {
-auto *typeVTy = llvm::cast(type);
-if (typeVTy->isScalable()) {
-  emitError(unknownLoc) << "scalable vector types not supported";
-  return nullptr;
-}
+  case llvm::Type::ScalableVectorTyID: {
+emitError(unknownLoc) << "scalable vector types not supported";
+return nullptr;
+  }
+  case llvm::Type::FixedVectorTyID: {
+auto *typeVTy = llvm::cast(type);
 LLVMType elementType = processType(typeVTy->getElementType());
 if (!elementType)
   return nullptr;
Index: llvm/tools/llvm-c-test/echo.cpp
===
--- llvm/tools/llvm-c-test/echo.cpp
+++ llvm/tools/llvm-c-test/echo.cpp
@@ -137,7 +137,10 @@
   Clone(LLVMGetElementType(Src)),
   LLVMGetPointerAddressSpace(Src)
 );
-  case LLVMVectorTypeKind:
+  case LLVMScalableVectorTypeKind:
+// FIXME: scalable vectors unsupported
+break;
+  case LLVMFixedVectorTypeKind:
 return LLVMVectorType(
   Clone(LLVMGetElementType(Src)),
   LLVMGetVectorSize(Src)
Index: llvm/lib/Transforms/Utils/FunctionComparator.cpp
===
--- llvm/lib/Transforms/Utils/FunctionComparator.cpp
+++ llvm/lib/Transforms/Utils/FunctionComparator.cpp
@@ -488,7 +488,8 @@
   return cmpNumbers(STyL->getNumElements(), STyR->getNumElements());
 return cmpTypes(STyL->getElementType(), STyR->getElementType());
   }
-  case Type::VectorTyID: {
+  case Type::FixedVectorTyID:
+  case Type::ScalableVectorTyID: {
 auto *STyL = cast(TyL);
 auto *STyR = cast(TyR);
 if (STyL->getElementCount().Scalable != STyR->getElementCount().Scalable)
Index: llvm/lib/Transforms/IPO/GlobalOpt.cpp
===
--- llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -129,7 +129,8 @@
   default: break;
   case Type::PointerTyID:
 return true;
-  case Type::VectorTyID:
+  case Type::FixedVectorTyID:
+  case Type::ScalableVectorTyID:
 if (cast(Ty)->getElementType()->isPointerTy())
   return true;
 break;
Index: llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
===
--- llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
+++ llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
@@ -1184,7 +1184,7 @@
 case Type::IntegerTyID: // Integers larger than 64 bits
 case Type::StructTyID:
 case Type::ArrayTyID:
-case Type::VectorTyID:
+case Type::FixedVectorTyID:
   ElementSize = DL.getTypeStoreSize(ETy);
   // Ptx allows variable initilization only for constant and
   // global state spaces.
@@ -1358,7 +1358,7 @@
   switch (ETy->getTypeID()) {
   case Type::StructTyID:
   case Type::ArrayTyID:
-  case Type::VectorTyID:
+  case Type::FixedVectorTyID:
 ElementSize = DL.getTypeStoreSize(ETy);
 O << " .b8 ";
 getSymbol(GVar)->print(O, MAI);
@@ -1892,7 +1892,7 @@
   }
 
   case Type::ArrayTyID:
-  case Type::VectorTyID:
+  case Type::FixedVectorTyID:
   case Type::StructTyID: {
 if (isa(CPV) || isa(CPV)) {
   int ElementSize = DL.getTypeAllocSize(CPV->getType());
Index: llvm/lib/Target/Hexagon/HexagonTargetObjectFile.cpp
===
--- llvm/lib/Target/Hexagon/H

[Lldb-commits] [PATCH] D78337: [lldb/Host] Remove TaskPool and replace its uses with llvm::ThreadPool

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe57361c055d7: [lldb/Host] Remove TaskPool and replace its 
uses with llvm::ThreadPool (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D78337?vs=259014&id=259323#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78337

Files:
  lldb/include/lldb/Host/TaskPool.h
  lldb/include/lldb/module.modulemap
  lldb/source/Host/CMakeLists.txt
  lldb/source/Host/common/TaskPool.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
  lldb/unittests/Host/CMakeLists.txt
  lldb/unittests/Host/TaskPoolTest.cpp

Index: lldb/unittests/Host/TaskPoolTest.cpp
===
--- lldb/unittests/Host/TaskPoolTest.cpp
+++ /dev/null
@@ -1,45 +0,0 @@
-#include "gtest/gtest.h"
-
-#include "lldb/Host/TaskPool.h"
-
-using namespace lldb_private;
-
-TEST(TaskPoolTest, AddTask) {
-  auto fn = [](int x) { return x * x + 1; };
-
-  auto f1 = TaskPool::AddTask(fn, 1);
-  auto f2 = TaskPool::AddTask(fn, 2);
-  auto f3 = TaskPool::AddTask(fn, 3);
-  auto f4 = TaskPool::AddTask(fn, 4);
-
-  ASSERT_EQ(10, f3.get());
-  ASSERT_EQ(2, f1.get());
-  ASSERT_EQ(17, f4.get());
-  ASSERT_EQ(5, f2.get());
-}
-
-TEST(TaskPoolTest, RunTasks) {
-  std::vector r(4);
-
-  auto fn = [](int x, int &y) { y = x * x + 1; };
-
-  TaskPool::RunTasks([fn, &r]() { fn(1, r[0]); }, [fn, &r]() { fn(2, r[1]); },
- [fn, &r]() { fn(3, r[2]); }, [fn, &r]() { fn(4, r[3]); });
-
-  ASSERT_EQ(2, r[0]);
-  ASSERT_EQ(5, r[1]);
-  ASSERT_EQ(10, r[2]);
-  ASSERT_EQ(17, r[3]);
-}
-
-TEST(TaskPoolTest, TaskMap) {
-  int data[4];
-  auto fn = [&data](int x) { data[x] = x * x; };
-
-  TaskMapOverInt(0, 4, fn);
-
-  ASSERT_EQ(data[0], 0);
-  ASSERT_EQ(data[1], 1);
-  ASSERT_EQ(data[2], 4);
-  ASSERT_EQ(data[3], 9);
-}
Index: lldb/unittests/Host/CMakeLists.txt
===
--- lldb/unittests/Host/CMakeLists.txt
+++ lldb/unittests/Host/CMakeLists.txt
@@ -11,7 +11,6 @@
   SocketAddressTest.cpp
   SocketTest.cpp
   SocketTestUtilities.cpp
-  TaskPoolTest.cpp
 )
 
 if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -13,10 +13,10 @@
 #include "Plugins/SymbolFile/DWARF/LogChannelDWARF.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h"
 #include "lldb/Core/Module.h"
-#include "lldb/Host/TaskPool.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
+#include "llvm/Support/ThreadPool.h"
 
 using namespace lldb_private;
 using namespace lldb;
@@ -71,20 +71,27 @@
 clear_cu_dies[cu_idx] = units_to_index[cu_idx]->ExtractDIEsScoped();
   };
 
+  // Share one thread pool across operations to avoid the overhead of
+  // recreating the threads.
+  llvm::ThreadPool pool;
+
   // Create a task runner that extracts dies for each DWARF unit in a
-  // separate thread
+  // separate thread.
   // First figure out which units didn't have their DIEs already
   // parsed and remember this.  If no DIEs were parsed prior to this index
   // function call, we are going to want to clear the CU dies after we are
   // done indexing to make sure we don't pull in all DWARF dies, but we need
   // to wait until all units have been indexed in case a DIE in one
   // unit refers to another and the indexes accesses those DIEs.
-  TaskMapOverInt(0, units_to_index.size(), extract_fn);
+  for (size_t i = 0; i < units_to_index.size(); ++i)
+pool.async(extract_fn, i);
+  pool.wait();
 
   // Now create a task runner that can index each DWARF unit in a
   // separate thread so we can index quickly.
-
-  TaskMapOverInt(0, units_to_index.size(), parser_fn);
+  for (size_t i = 0; i < units_to_index.size(); ++i)
+pool.async(parser_fn, i);
+  pool.wait();
 
   auto finalize_fn = [this, &sets](NameToDIE(IndexSet::*index)) {
 NameToDIE &result = m_set.*index;
@@ -93,14 +100,15 @@
 result.Finalize();
   };
 
-  TaskPool::RunTasks([&]() { finalize_fn(&IndexSet::function_basenames); },
- [&]() { finalize_fn(&IndexSet::function_fullnames); },
- [&]() { finalize_fn(&IndexSet::function_methods); },
- [&]() { finalize_fn(&IndexSet::function_selectors); },
- [&]() { finalize_fn(&IndexSet::objc_class_selectors); },
- [&]() { finalize_fn(&IndexSet::globals); },
- [&]() { finalize_fn(&IndexSet::types); },
- [&]() { finalize_fn(&IndexSet::namespaces); });
+  pool.async(finalize_fn, &IndexSet::function_base

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Aleksei Sidorin via Phabricator via lldb-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

Hello Shafik!
The patch looks good, I only have a few stylish nits.




Comment at: clang/unittests/AST/ASTImporterTest.cpp:5964
+  new SourceWithCompletedTagList(CompletedTags);
+  clang::ASTContext &context = FromTU->getASTContext();
+  context.setExternalSource(std::move(source));

Context (starting with capital).



Comment at: clang/unittests/AST/ASTImporterTest.cpp:5980
+  // Import the definition of the created class.
+  llvm::Error err = findFromTU(Record)->Importer->ImportDefinition(Record);
+  EXPECT_FALSE((bool)err);

Err (starting with capital).


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

https://reviews.llvm.org/D78000



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

[Lldb-commits] [PATCH] D78489: [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address tables

[Greg Clayton via Phabricator via lldb-commits]

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

In D78489#1996834 , @labath wrote:

> In D78489#1995837 , @clayborg wrote:
>
> > Sounds like a win then, as long as we don't get slower I am fine with this. 
> > I was guessing that line tables might be faster because it is generally 
> > very compressed and compact compared to debug info, but thanks for 
> > verifying.
> >
> > Might be worth checking the memory consumption of LLDB quickly too with 
> > DW_AT_ranges compiled out and just make sure we don't take up too much 
> > extra memory.
>
>
> I've done a similar benchmark to the last one, but measured memory usage 
> ("RssAnon" as reported by linux). One notable difference is that I
>
> Currently (information retrieved from DW_AT_ranges) we use about ~330 MB of 
> memory. If I switch to dwarf dies as the source, the memory goes all the way 
> to 2890 MB. This number is suspiciously large -- it either means that our die 
> freeing is not working properly, or that glibc is very bad at releasing 
> memory back to the OS. Given the magnitude of the increase, i think it's a 
> little bit of both. With line tables as the source the memory usage is 706 
> MB. It's an increase from 330, but definitely smaller than 2.8 GB. (the 
> number 330 is kind of misleading here since we're not considering removing 
> that option -- it will always be used if it is available).


Since we mmap in the entire DWARF, I am not surprised by taking up new memory 
because we touch those pages and won't get those back. If you remove the DIE 
freeing code, I will bet you see much more memory used. We definitely free the 
memory for the DIEs and give that back, so I would be willing to bet the 
increase you are seeing is from mmap loading pages in that we touch.

> 
> 
>> We aren't doing anything to unload these line tables like we do with DIEs 
>> are we? It might make sense to pares the line tables and then throw them 
>> away if they were not already parsed?  With the DIEs we were throwing them 
>> away if they weren't already parsed to keep memory consumption down, so 
>> might be worth throwing the line tables away after running this if we are 
>> now going to rely on it.
> 
> That would be possible, but I don't think it's worth the trouble. I think the 
> phrase "relying on it" overemphasizes the importance of that code. In 
> practice, the only time when this path will be taken is when debugging code 
> built with clang<=3.3, which is seven years old and did not even fully 
> implement c++11. It also seems like the switch to line tables will save 
> memory, at least until the die freeing bug is fixed. And lastly, the 
> difference i reported is pretty much the worst possible case, as the only 
> thing the debugger will do is parse the line tables and exit. Once the 
> debugger starts doing other stuff too, the difference will start to fade 
> (e.g. running to the breakpoint in main increases the memory usage to 600 MB 
> even with DW_AT_ranges).

Ok, thanks for looking into this.

> 
> 
>> One other thing to verify we want to go this route is to re-enable the old 
>> DIE parsing for high/low PCs, but put the results in a separate address 
>> ranges class. After we parse the line tables, verify that all ranges we 
>> found in the DIEs are in the line tables? It would not be great if we were 
>> going to start missing some functions if a function doesn't have a line 
>> table? Not sure if/how this would happen, but you never know.
> 
> I implemented a check like that. While doing it I've learned that the 
> DIE-based parsing does not work since May 2018 (D47275 
> ) and nobody noticed. What that means is 
> that in practice we were always going through line tables (if DW_AT_ranges 
> were missing). It also means that my times reported earlier for die-based 
> searching were incorrect as they also included the time to build the line 
> tables. (However, that does not change the ordering, as even if we subtract 
> the time it took to parse the line tables, the DIE method is still much 
> slower).
> 
> After fixing the die-based search, I was able to verify that the die-based 
> ranges are an exact match to the line table ranges (for the particular 
> compiler used -- top of tree clang).
> 
> Given all of the above (die-based searching being slow, taking more memory, 
> and not actually working), i think it's pretty clear that we should remove it.

Fine with me. Thanks for taking the time to ensure we don't regress.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78489



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

[Lldb-commits] [lldb] 478619c - Revert "get rid of PythonInteger::GetInteger()"

[Muhammad Omair Javaid via lldb-commits]

Author: Muhammad Omair Javaid
Date: 2020-04-23T04:38:32+05:00
New Revision: 478619cf9a24ad8eac806959ca6a289a9beb71ae

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

LOG: Revert "get rid of PythonInteger::GetInteger()"

This reverts commit 7375212172951d2fc283c81d03c1a8588c3280c6.

This causes multiple test failures on LLDB AArch64 Linux buildbot.
http://lab.llvm.org:8011/builders/lldb-aarch64-ubuntu/builds/3695

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

Added: 


Modified: 
lldb/bindings/python/python-typemaps.swig
lldb/bindings/python/python-wrapper.swig
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Removed: 




diff  --git a/lldb/bindings/python/python-typemaps.swig 
b/lldb/bindings/python/python-typemaps.swig
index c08aeab71f78..46dcaf611a4f 100644
--- a/lldb/bindings/python/python-typemaps.swig
+++ b/lldb/bindings/python/python-typemaps.swig
@@ -59,25 +59,37 @@
   $result = list.release();
 }
 
+
 %typemap(in) lldb::tid_t {
-  PythonObject obj = Retain($input);
-  lldb::tid_t value = unwrapOrSetPythonException(As(obj)); 
-  if (PyErr_Occurred())
+  if (PythonInteger::Check($input))
+  {
+PythonInteger py_int(PyRefType::Borrowed, $input);
+$1 = static_cast(py_int.GetInteger());
+  }
+  else
+  {
+PyErr_SetString(PyExc_ValueError, "Expecting an integer");
 return nullptr;
-  $1 = value;
+  }
 }
 
 %typemap(in) lldb::StateType {
-  PythonObject obj = Retain($input);
-  unsigned long long state_type_value =
-unwrapOrSetPythonException(As(obj));
-  if (PyErr_Occurred())
-return nullptr;
-  if (state_type_value > lldb::StateType::kLastStateType) {
-PyErr_SetString(PyExc_ValueError, "Not a valid StateType value");
+  if (PythonInteger::Check($input))
+  {
+PythonInteger py_int(PyRefType::Borrowed, $input);
+int64_t state_type_value = py_int.GetInteger() ;
+
+if (state_type_value > lldb::StateType::kLastStateType) {
+  PyErr_SetString(PyExc_ValueError, "Not a valid StateType value");
+  return nullptr;
+}
+$1 = static_cast(state_type_value);
+  }
+  else
+  {
+PyErr_SetString(PyExc_ValueError, "Expecting an integer");
 return nullptr;
   }
-  $1 = static_cast(state_type_value);
 }
 
 /* Typemap definitions to allow SWIG to properly handle char buffer. */

diff  --git a/lldb/bindings/python/python-wrapper.swig 
b/lldb/bindings/python/python-wrapper.swig
index f9e89373fe25..3a63165cf58d 100644
--- a/lldb/bindings/python/python-wrapper.swig
+++ b/lldb/bindings/python/python-wrapper.swig
@@ -444,7 +444,6 @@ LLDBSwigPythonCallBreakpointResolver
 if (PyErr_Occurred())
 {
 PyErr_Print();
-PyErr_Clear();
 return 0;
 }
 
@@ -458,13 +457,11 @@ LLDBSwigPythonCallBreakpointResolver
   return 1;
 }
 
-long long ret_val = unwrapOrSetPythonException(As(result));
-
-if (PyErr_Occurred()) {
-PyErr_Print();
-PyErr_Clear();
+PythonInteger int_result = result.AsType();
+if (!int_result.IsAllocated())
 return 0;
-}
+
+unsigned int ret_val = int_result.GetInteger();
 
 return ret_val;
 }
@@ -518,17 +515,26 @@ LLDBSwigPython_CalculateNumChildren
 return 0;
 }
 
-size_t ret_val;
+PythonObject result;
+
 if (arg_info.get().max_positional_args < 1)
-ret_val = unwrapOrSetPythonException(As(pfunc.Call()));
+result = pfunc();
 else
-ret_val = unwrapOrSetPythonException(As(pfunc.Call(PythonInteger(max;
+result = pfunc(PythonInteger(max));
 
-if (PyErr_Occurred())
+if (!result.IsAllocated())
+return 0;
+
+PythonInteger int_result = result.AsType();
+if (!int_result.IsAllocated())
+return 0;
+
+size_t ret_val = int_result.GetInteger();
+
+if (PyErr_Occurred()) //FIXME use Expected to catch python exceptions
 {
 PyErr_Print();
 PyErr_Clear();
-return 0;
 }
 
 if (arg_info.get().max_positional_args < 1)
@@ -582,15 +588,16 @@ LLDBSwigPython_GetIndexOfChildWithName
 if (!pfunc.IsAllocated())
 return UINT32_MAX;
 
-llvm::Expected result = pfunc.Call(PythonString(child_name));
+PythonObject result = pfunc(PythonString(child_name));
 
-long long retval = unwrapOrSetPythonException(As(std::move(result)));
+if (!result.IsAllocated())
+return UINT32_MAX;
 
-if (PyErr_Occurred()) { 
-PyErr_Clear(); // FIXME print this? do something else
+PythonInteger int_result = result.AsType();
+if (!int_result.IsAllocated())

[Lldb-commits] [PATCH] D78462: get rid of PythonInteger::GetInteger()

[Muhammad Omair Javaid via Phabricator via lldb-commits]

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

This causes multiple test failures on LLDB AArch64 Linux buildbot.
http://lab.llvm.org:8011/builders/lldb-aarch64-ubuntu/builds/3695


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78462



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

[Lldb-commits] [PATCH] D78462: get rid of PythonInteger::GetInteger()

[Muhammad Omair Javaid via Phabricator via lldb-commits]

omjavaid requested changes to this revision.
omjavaid added a comment.
This revision now requires changes to proceed.

I have temporarily reverted this change to turn buildbot green.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78462



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

[Lldb-commits] [PATCH] D78462: get rid of PythonInteger::GetInteger()

[Muhammad Omair Javaid via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG478619cf9a24: Revert "get rid of 
PythonInteger::GetInteger()" (authored by omjavaid).

Changed prior to commit:
  https://reviews.llvm.org/D78462?vs=259134&id=259429#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78462

Files:
  lldb/bindings/python/python-typemaps.swig
  lldb/bindings/python/python-wrapper.swig
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -123,11 +123,13 @@
   EXPECT_TRUE(major_version_field.IsAllocated());
   EXPECT_TRUE(minor_version_field.IsAllocated());
 
-  auto major_version_value = As(major_version_field);
-  auto minor_version_value = As(minor_version_field);
+  PythonInteger major_version_value =
+  major_version_field.AsType();
+  PythonInteger minor_version_value =
+  minor_version_field.AsType();
 
-  EXPECT_THAT_EXPECTED(major_version_value, llvm::HasValue(PY_MAJOR_VERSION));
-  EXPECT_THAT_EXPECTED(minor_version_value, llvm::HasValue(PY_MINOR_VERSION));
+  EXPECT_EQ(PY_MAJOR_VERSION, major_version_value.GetInteger());
+  EXPECT_EQ(PY_MINOR_VERSION, minor_version_value.GetInteger());
 }
 
 TEST_F(PythonDataObjectsTest, TestGlobalNameResolutionWithDot) {
@@ -135,14 +137,16 @@
   EXPECT_TRUE(sys_path.IsAllocated());
   EXPECT_TRUE(PythonList::Check(sys_path.get()));
 
-  auto version_major =
-  As(m_main_module.ResolveName("sys.version_info.major"));
-
-  auto version_minor =
-  As(m_main_module.ResolveName("sys.version_info.minor"));
-
-  EXPECT_THAT_EXPECTED(version_major, llvm::HasValue(PY_MAJOR_VERSION));
-  EXPECT_THAT_EXPECTED(version_minor, llvm::HasValue(PY_MINOR_VERSION));
+  PythonInteger version_major =
+  m_main_module.ResolveName("sys.version_info.major")
+  .AsType();
+  PythonInteger version_minor =
+  m_main_module.ResolveName("sys.version_info.minor")
+  .AsType();
+  EXPECT_TRUE(version_major.IsAllocated());
+  EXPECT_TRUE(version_minor.IsAllocated());
+  EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger());
+  EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger());
 }
 
 TEST_F(PythonDataObjectsTest, TestDictionaryResolutionWithDot) {
@@ -151,14 +155,14 @@
   dict.SetItemForKey(PythonString("sys"), m_sys_module);
 
   // Now use that dictionary to resolve `sys.version_info.major`
-  auto version_major = As(
-  PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict));
-
-  auto version_minor = As(
-  PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict));
-
-  EXPECT_THAT_EXPECTED(version_major, llvm::HasValue(PY_MAJOR_VERSION));
-  EXPECT_THAT_EXPECTED(version_minor, llvm::HasValue(PY_MINOR_VERSION));
+  PythonInteger version_major =
+  PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict)
+  .AsType();
+  PythonInteger version_minor =
+  PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict)
+  .AsType();
+  EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger());
+  EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger());
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonInteger) {
@@ -172,8 +176,7 @@
   PythonInteger python_int(PyRefType::Owned, py_int);
 
   EXPECT_EQ(PyObjectType::Integer, python_int.GetObjectType());
-  auto python_int_value = As(python_int);
-  EXPECT_THAT_EXPECTED(python_int_value, llvm::HasValue(12));
+  EXPECT_EQ(12, python_int.GetInteger());
 #endif
 
   // Verify that `PythonInteger` works correctly when given a PyLong object.
@@ -184,14 +187,12 @@
 
   // Verify that you can reset the value and that it is reflected properly.
   python_long.SetInteger(40);
-  auto e = As(python_long);
-  EXPECT_THAT_EXPECTED(e, llvm::HasValue(40));
+  EXPECT_EQ(40, python_long.GetInteger());
 
   // Test that creating a `PythonInteger` object works correctly with the
   // int constructor.
   PythonInteger constructed_int(7);
-  auto value = As(constructed_int);
-  EXPECT_THAT_EXPECTED(value, llvm::HasValue(7));
+  EXPECT_EQ(7, constructed_int.GetInteger());
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonBoolean) {
@@ -338,8 +339,7 @@
   PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get());
   PythonString chk_str(PyRefType::Borrowed, chk_value2.get());
 
-  auto chkint = As(chk_value1);
-  ASSERT_THAT_EXPECTED(chkint, llvm::H

[Lldb-commits] [lldb] b424b0b - [lldb/Target] Avoid race between Communication::Disconnect calls.

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-04-22T16:56:42-07:00
New Revision: b424b0bf731d1fb338180553a86f912746f640ad

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

LOG: [lldb/Target] Avoid race between Communication::Disconnect calls.

Avoid a race between the Disconnect call in `Communication::ReadThread`
and the one in `Process::ShouldBroadcastEvent` by reordering the calls
to Disconnect and StopReadThread in `Process::ShouldBroadcastEvent`.

In D77295 Pavel suggested that that might explain the broken pipe I was
seeing. Indeed, changing the order resolved the issue.

Added: 


Modified: 
lldb/source/Target/Process.cpp

Removed: 




diff  --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 7797a4c60964..45dcfc489d81 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -3386,8 +3386,8 @@ bool Process::ShouldBroadcastEvent(Event *event_ptr) {
   case eStateExited:
   case eStateUnloaded:
 m_stdio_communication.SynchronizeWithReadThread();
-m_stdio_communication.Disconnect();
 m_stdio_communication.StopReadThread();
+m_stdio_communication.Disconnect();
 m_stdin_forward = false;
 
 LLVM_FALLTHROUGH;



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

[Lldb-commits] [PATCH] D77295: [lldb/Core] Fix a race in the Communication class

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D77295#1996337 , @labath wrote:

> In D77295#1995077 , @JDevlieghere 
> wrote:
>
> > Hi Pavel,
> >
> > After this landed I started seeing a bunch of reproducers fail with 
> > `SIGPIPE`. I ignored the signal and added a check for `EPIPE` to the 
> > `::write` call in `PipePosix.cpp`. Below is the thread state:
> >
> >   * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGPIPE
> > * frame #0: 0x7fff6876c55e libsystem_kernel.dylib`__ulock_wait + 10
> >   frame #1: 0x7fff6882f5c2 libsystem_pthread.dylib`_pthread_join + 
> > 347
> >   frame #2: 0x00010115ef62 
> > liblldb.11.0.0git.dylib`lldb_private::HostThreadPosix::Join(this=0x00011e7ca490,
> >  result=0x7ffeefbfe290) at HostThreadPosix.cpp:28:15
> >   frame #3: 0x00010110b64f 
> > liblldb.11.0.0git.dylib`lldb_private::HostThread::Join(this=0x00011e9f09a0,
> >  result=0x7ffeefbfe290) at HostThread.cpp:21:27
> >   frame #4: 0x0001012ea5ba 
> > liblldb.11.0.0git.dylib`lldb_private::Process::ControlPrivateStateThread(this=0x00011e9f0818,
> >  signal=1) at Process.cpp:3642:30
> >   frame #5: 0x0001012dbc81 
> > liblldb.11.0.0git.dylib`lldb_private::Process::StopPrivateStateThread(this=0x00011e9f0818)
> >  at Process.cpp:3593:5
> >   frame #6: 0x0001012dc514 
> > liblldb.11.0.0git.dylib`lldb_private::Process::Destroy(this=0x00011e9f0818,
> >  force_kill=true) at Process.cpp:3317:7
> >   frame #7: 0x000100981cc1 
> > liblldb.11.0.0git.dylib`lldb::SBProcess::Kill(this=0x00011e767de0) at 
> > SBProcess.cpp:662:35
> >  
> >   * thread #2
> > * frame #0: 0x7fff6876c4ce 
> > libsystem_kernel.dylib`__workq_kernreturn + 10
> >   frame #1: 0x7fff6882aaa1 
> > libsystem_pthread.dylib`_pthread_wqthread + 390
> >   frame #2: 0x7fff68829b77 libsystem_pthread.dylib`start_wqthread + 
> > 15
> >  
> >   * thread #3, name = ''
> > * frame #0: 0x7fff6876c55e libsystem_kernel.dylib`__ulock_wait + 10
> >   frame #1: 0x7fff6882f5c2 libsystem_pthread.dylib`_pthread_join + 
> > 347
> >   frame #2: 0x00010115ef62 
> > liblldb.11.0.0git.dylib`lldb_private::HostThreadPosix::Join(this=0x00011e7ca510,
> >  result=0x) at HostThreadPosix.cpp:28:15
> >   frame #3: 0x00010110b64f 
> > liblldb.11.0.0git.dylib`lldb_private::HostThread::Join(this=0x00011e9f0e28,
> >  result=0x) at HostThread.cpp:21:27
> >   frame #4: 0x000100f46f4e 
> > liblldb.11.0.0git.dylib`lldb_private::Communication::StopReadThread(this=0x00011e9f0de8,
> >  error_ptr=0x) at Communication.cpp:239:32
> >   frame #5: 0x0001012e9d95 
> > liblldb.11.0.0git.dylib`lldb_private::Process::ShouldBroadcastEvent(this=0x00011e9f0818,
> >  event_ptr=0x00011e76aa40) at Process.cpp:3390:27
> >   frame #6: 0x0001012e5e4f 
> > liblldb.11.0.0git.dylib`lldb_private::Process::HandlePrivateEvent(this=0x00011e9f0818,
> >  event_sp=std::__1::shared_ptr::element_type @ 
> > 0x00011e76aa40 strong=2 weak=1) at Process.cpp:3697:33
> >   frame #7: 0x0001012eb147 
> > liblldb.11.0.0git.dylib`lldb_private::Process::RunPrivateStateThread(this=0x00011e9f0818,
> >  is_secondary_thread=false) at Process.cpp:3891:7
> >   frame #8: 0x0001012ea2fa 
> > liblldb.11.0.0git.dylib`lldb_private::Process::PrivateStateThread(arg=0x00011e7c9a60)
> >  at Process.cpp:3789:25
> >   frame #9: 0x00010110b45b 
> > liblldb.11.0.0git.dylib`lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(arg=0x00011e7ca4e0)
> >  at HostNativeThreadBase.cpp:68:10
> >   frame #10: 0x00010726ab83 
> > liblldb.11.0.0git.dylib`lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(arg=0x00011e7ca4e0)
> >  at HostThreadMacOSX.mm:68:10
> >  
> >   * thread #4, name = '', stop reason = breakpoint 
> > 1.1
> > * frame #0: 0x00010116071a 
> > liblldb.11.0.0git.dylib`lldb_private::PipePosix::Write(this=0x00011e7ca2b0,
> >  buf=43698118819080247, size=1, bytes_written=0x776908b8) at 
> > PipePosix.cpp:312:7
> >   frame #1: 0x000101156d15 
> > liblldb.11.0.0git.dylib`lldb_private::ConnectionFileDescriptor::Disconnect(this=0x00011e7ca210,
> >  error_ptr=0x) at ConnectionFileDescriptorPosix.cpp:321:30
> >   frame #2: 0x000100f454e6 
> > liblldb.11.0.0git.dylib`lldb_private::Communication::Disconnect(this=0x00011e9f0de8,
> >  error_ptr=0x) at Communication.cpp:98:46
> >   frame #3: 0x000100f46d4e 
> > liblldb.11.0.0git.dylib`lldb_private::Communication::ReadThread(p=0x00011e9f0de8)
> >  at Communication.cpp:377:13
> >   frame #4: 0x00010110b45b 
> > liblldb.11.0.0git.dylib`lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(arg=0x00011e7ca4e0)
> >  at HostNativeThreadBas