[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you. I think this looks much better now.

It occurred to me that the (de)serializer classes are fully standalone. Since 
they already have a comprehensive test suite, do you think it would make sense 
to split them off into a separate patch, which can be committed separately?




Comment at: include/lldb/API/SBReproducer.h:19
+public:
+  bool Replay() const;
+};

Should this be static?



Comment at: include/lldb/Utility/ReproducerInstrumentation.h:290-291
+protected:
+  /// Initialize the registry by registering function.
+  virtual void Init() = 0;
+

Is this function necessary? It seems like the subclass could easily achieve 
this by just calling `Register` from its constructor. (I also don't see it 
being called anywhere).



Comment at: include/lldb/Utility/ReproducerInstrumentation.h:294
+  /// Register the given replayer for a function (and the ID mapping).
+  void DoRegister(uintptr_t RunID, Replayer *replayer);
+

How about having this function take a unique_ptr, and then store that 
unique_ptr in (e.g.) the `m_ids` map. It'd be nice not to leak _every_ object 
we create in this patch.



Comment at: include/lldb/Utility/ReproducerInstrumentation.h:298
+  /// Mapping of function addresses to replayers and their ID.
+  std::map> m_sbreplayers;
+

s/m_sbreplayers/m_replayers`



Comment at: include/lldb/Utility/ReproducerInstrumentation.h:393
+  void Serialize(void *v) {
+// Do nothing.
+  }

Since you don't support `void *`, maybe this should be `llvm_unreachable`, or 
`=delete`?



Comment at: include/lldb/Utility/ReproducerInstrumentation.h:432
+if (Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_API))
+  LLDB_LOG(log, "#%u '%s'", id, m_pretty_func);
+

I guess I forgot to mention that LLDB_LOG uses the `llvm::formatv` syntax for 
substitutions (which is why it's able to handle `llvm::StringRef`, and 
non-null-terminated strings. So this would just be `LLDB_LOG(log, "#{0} '{1}'", 
id, m_pretty_func)`.



Comment at: source/API/SBReproducer.cpp:42-59
+static void SetInputFileHandleRedirect(SBDebugger *t, FILE *, bool) {
+  repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+  if (!loader)
+return;
+
+  FileSpec fs = loader->GetFile();
+  if (!fs)

Unused functions.



Comment at: source/API/SBReproducerPrivate.h:121
+public:
+  SBDeserializer(llvm::StringRef buffer = {}) : m_buffer(buffer), m_offset(0) 
{}
+

JDevlieghere wrote:
> labath wrote:
> > Instead of the m_offset variable, what do you think about just 
> > `drop_front`ing the appropriate amount of bytes when you are done with the? 
> > It doesn't look like you'll ever need to go back to an earlier point in the 
> > stream...
> We don't go back, but the buffer is the backing memory for deserialized 
> c-strings.
Well, the buffer is a StringRef, so it doesn't really "back" anything :), but 
if you think this is more understandable then I can live with it.



Comment at: source/API/SBReproducerPrivate.h:182
+typedef typename std::remove_reference::type UnderlyingT;
+// If this is a reference to a fundamental type we just read its value.
+return *m_index_to_object.template GetObjectForIndex(

JDevlieghere wrote:
> labath wrote:
> > Is this correct? I would expect the fundamental references to not go 
> > through this overload at all...
> Pretty sure, but please elaborate on why you think that. 
I am referring to the comment in the method (the implementation seems fine). It 
says "If this is a reference to a fundamental type, ...", but fundamental types 
should go through the `FundamentalReferenceTag` overload, right?



Comment at: source/API/SBReproducerPrivate.h:453
+public:
+  SBSerializer(llvm::raw_ostream &stream = llvm::outs()) : m_stream(stream) {}
+

JDevlieghere wrote:
> labath wrote:
> > Is this default value used for anything? It don't see it being used, and it 
> > doesn't seem particularly useful anyway, as you'd just get a lot of binary 
> > junk on stdout.
> I'm 100% sure it's needed because I got rid of it only to find out we 
> couldn't do without it. I'm not entirely sure anymore, but I think we didn't 
> know if the function was going to have a (useful) return value or not.
I don't see how the default stream argument could have anything to do with 
whether a function has a return value. Are you sure you replied to the right 
comment?



Comment at: source/API/SBReproducerPrivate.h:525-533
+  void RecordOmittedResult() {
+assert(!m_expect_result_recored && "Did you forget SB_RECORD_RESULT?");
+if (m_result_recorded)
+  return;
+if (!ShouldCapture())
+  return;
+

Instead of this functio

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D56230#1365898 , @Hui wrote:

> Hello labath, I am thinking maybe it is just because the windows command 
> 'dir' can't interpret "\\" in the root directory path. For example,
>
> This is working
>  "C:\\WINDOWS\\system32\\cmd.exe" /C " dir **c:\**Users\\abc"
>
>   This is not working
>
> "C:\\WINDOWS\\system32\\cmd.exe" /C " dir **c:\\**Users\\abc"
>
> Other commands, like **ls** seems not have this problem.
>
> So could we just go ahead with  llvm::sys::flattenWindowsCommandLine but set 
> a red light on 'dir' command only, at least handle it specifically?


Sorry about the delay.

I've done some experiments of my own, and my conclusion is that the "special" 
part here is not the "dir" command but cmd.exe itself. It handles quotes 
differently than typical windows applications. It behavior is documented like 
this:

  1.  If all of the following conditions are met, then quote characters
  on the command line are preserved:
  
  - no /S switch
  - exactly two quote characters
  - no special characters between the two quote characters,
where special is one of: &<>()@^|
  - there are one or more whitespace characters between the
the two quote characters
  - the string between the two quote characters is the name
of an executable file.
  
  2.  Otherwise, old behavior is to see if the first character is
  a quote character and if so, strip the leading character and
  remove the last quote character on the command line, preserving
  any text after the last quote character.

So given these rules, and the lldb command `platform shell dir c:\`, the 
correct string to pass to CreateProcess would be `cmd.exe /c "dir c:\"`. 
Conceptually, that is easy to achieve, but it practice that's a bit hard 
because the road from "platform shell" to CreateProcess is long and winding:

1. first, the string `dir c:\` gets converted to a Args array in 
Host::RunShellCommand (the `const char *` version). This step uses the "lldb" 
quoting rules and results in something like `dir`, `c:\`.
2. then the Args array gets transmogrified in 
ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell. This uses some 
approximation of the posix shell quoting rules, but I believe that is not fully 
correct for posix shells either. The result is `cmd.exe`, `/c`, `dir c:\` 
(IIRC).
3. finally, we call `flattenWindowsCommandLine`. This uses the standard windows 
quoting rules, which gives us the string `cmd.exe /c "dir c:\\"

So, I think the proper solution here would be to find a way to skip steps 1. 
and 2. They're completely unnecessary even in the posix case -- the user types 
a string, the shell expects a string, we should be able to just pass that 
string from one place to the other and not play around with parsing and 
reconstituting it. Then, it will be easy to construct the proper command line 
for windows, or the right argv array in the posix case. This will probably 
involve having ProcessLaunchInfo be backed by either an argv array or a plain 
string.

However, this is getting pretty far from the original intention of your patch. 
For the time being I would be happy with checking in the present version of 
your patch, with the knowledge that this breaks some "platform shell" use 
cases. I don't think that's too bad, because "platform shell" is not the most 
important feature in lldb, and it's not fully correct now anyway. In return for 
that, we would get all tests in TestQuoting.py passing.


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

https://reviews.llvm.org/D56230



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

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

[Pavel Labath via Phabricator via lldb-commits]

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

Going to the Host straight from the DynamicLoader is the worst possible option, 
since then this code will not be correct for remote processes (it will just 
randomly pick up some data from the local process with the same pid, if you 
happen to have one). I'd like to hear what you think about the latest comments 
by Greg & myself.

In D56237#1373788 , @clayborg wrote:

> In D56237#1373234 , @labath wrote:
>
> > I think there are some architectural issues here in how ProcessWindows 
> > works. Normally (i.e., on all non-windows process classes), it is the job 
> > of the dynamic loader plugin to create modules and set their load 
> > addresses. However, ProcessWindows does things differently, and creates the 
> > modules by itself (ProcessWindows::OnLoadDLL).
> >  I think the behavior of ProcessWindows is fine (it flows naturally from 
> > how modules are handled on windows), but if it's going to handle module 
> > loading by itself, then I think it should do the job completely, and not 
> > only half-way. So, if we want ProcessWindows to do the module loading, then 
> > it should return a null value for `GetDynamicLoader()` (or a reasonably 
> > stubbed out implementation) instead of relying on a generic dynamic loader 
> > plugin to finish the job.
> >
> > Or, if we want to have the DynamicLoader plugin, then ProcessWindows should 
> > stop meddling with the module list in the OnLoadDLL function. One way to 
> > achieve that would be:
> >
> > - ProcessWindows::OnLoadDLL calls DynamicLoaderWindowsDYLD::OnLoadDll
> > - dynamic loader loads the module and sets load address
> > - dynamic loader does not have to call `ProcessWindows::GetFileLoadAddress` 
> > as it already got the load address from in the OnLoadDLL function This 
> > would make the flow very similar to how other platforms handle it (the only 
> > difference would be that the "OnLoadDLL" equivalent is called from a 
> > breakpoint callback function, instead of from a special debug event 
> > callback, but that's just due to a difference in how modules are reported).
>
>
> I would like the see the dynamic loader do all of the work. We can easily add 
> any new virtual functions to the DynamicLoader class to fit how windows does 
> things.


That should be fairly easy to achieve. The process class is the one that 
creates the dynamic loader (in `GetDynamicLoader()`), so it can easily call any 
special method on it that it needs from OnLoadDLL (i.e., we don't even need to 
extend the public DynamicLoader API).

However, note that this would not be the first process class that handles 
module loading on its own. ProcessMinidump already does that too. It could be 
changed to use a special dynamic loader too, but it's not clear to me whether 
that is worth the trouble, as the loading process for minidumps is very simple.


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

https://reviews.llvm.org/D56237



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

[Lldb-commits] [PATCH] D56602: Move FileAction, ProcessInfo and ProcessLaunchInfo from Target to Host

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Yep, I have only one or two more of these to do and then the only source of 
circular dependencies will be `Host/$OS/Symbols.cpp`. That one is going to be 
tricky, but I have some ideas how to solve that. I'll send an email about that 
once I'm ready.


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

https://reviews.llvm.org/D56602



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

[Lldb-commits] [lldb] r353047 - Move FileAction, ProcessInfo and ProcessLaunchInfo from Target to Host

[Pavel Labath via lldb-commits]

Author: labath
Date: Mon Feb  4 06:28:08 2019
New Revision: 353047

URL: http://llvm.org/viewvc/llvm-project?rev=353047&view=rev
Log:
Move FileAction, ProcessInfo and ProcessLaunchInfo from Target to Host

Summary:
These classes describe the details of the process we are about to
launch, and so they are naturally used by the launching code in the Host
module. Previously they were present in Target because that is the most
important (but by far not the only) user of the launching code.

Since the launching code has other customers, must of which do not care
about Targets, it makes sense to move these classes to the Host layer,
next to the launching code.

This move reduces the number of times that Target is included from host
to 8 (it used to be 14).

Reviewers: zturner, clayborg, jingham, davide, teemperor

Subscribers: emaste, mgorny, lldb-commits

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

Added:
lldb/trunk/include/lldb/Host/FileAction.h
  - copied, changed from r352902, 
lldb/trunk/include/lldb/Target/FileAction.h
lldb/trunk/include/lldb/Host/ProcessInfo.h
  - copied, changed from r352902, 
lldb/trunk/include/lldb/Target/ProcessInfo.h
lldb/trunk/include/lldb/Host/ProcessLaunchInfo.h
  - copied, changed from r352902, 
lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
lldb/trunk/source/Host/common/FileAction.cpp
  - copied, changed from r352902, lldb/trunk/source/Target/FileAction.cpp
lldb/trunk/source/Host/common/ProcessInfo.cpp
  - copied, changed from r352902, lldb/trunk/source/Target/ProcessInfo.cpp
lldb/trunk/source/Host/common/ProcessLaunchInfo.cpp
  - copied, changed from r352902, 
lldb/trunk/source/Target/ProcessLaunchInfo.cpp
lldb/trunk/unittests/Host/FileActionTest.cpp
lldb/trunk/unittests/Host/ProcessInfoTest.cpp
lldb/trunk/unittests/Host/ProcessLaunchInfoTest.cpp
Removed:
lldb/trunk/include/lldb/Target/FileAction.h
lldb/trunk/include/lldb/Target/ProcessInfo.h
lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
lldb/trunk/source/Target/FileAction.cpp
lldb/trunk/source/Target/ProcessInfo.cpp
lldb/trunk/source/Target/ProcessLaunchInfo.cpp
Modified:
lldb/trunk/include/lldb/Target/Process.h
lldb/trunk/include/lldb/Target/Target.h
lldb/trunk/include/lldb/module.modulemap
lldb/trunk/source/API/SBLaunchInfo.cpp
lldb/trunk/source/Host/CMakeLists.txt
lldb/trunk/source/Host/common/Host.cpp
lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
lldb/trunk/source/Host/macosx/objcxx/Host.mm
lldb/trunk/source/Host/posix/ProcessLauncherPosixFork.cpp
lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp

lldb/trunk/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
lldb/trunk/source/Target/CMakeLists.txt
lldb/trunk/unittests/Host/CMakeLists.txt
lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
lldb/trunk/unittests/tools/lldb-server/tests/TestClient.h

Copied: lldb/trunk/include/lldb/Host/FileAction.h (from r352902, 
lldb/trunk/include/lldb/Target/FileAction.h)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/FileAction.h?p2=lldb/trunk/include/lldb/Host/FileAction.h&p1=lldb/trunk/include/lldb/Target/FileAction.h&r1=352902&r2=353047&rev=353047&view=diff
==
--- lldb/trunk/include/lldb/Target/FileAction.h (original)
+++ lldb/trunk/include/lldb/Host/FileAction.h Mon Feb  4 06:28:08 2019
@@ -6,8 +6,8 @@
 //
 
//===--===//
 
-#ifndef liblldb_Target_FileAction_h
-#define liblldb_Target_FileAction_h
+#ifndef LLDB_HOST_FILEACTION_H
+#define LLDB_HOST_FILEACTION_H
 
 #include "lldb/Utility/FileSpec.h"
 #include 

Copied: lldb/trunk/include/lldb/Host/ProcessInfo.h (from r352902, 
lldb/trunk/include/lldb/Target/ProcessInfo.h)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/ProcessInfo.h?p2=lldb/trunk/include/lldb/Host/ProcessInfo.h&p1=lldb/trunk/include/lldb/Target/ProcessInfo.h&r1=352902&r2=353047&rev=353047&view=diff
==
(empty)

Copied: lldb/trunk/include/lldb/Host/ProcessLaunchInfo.h (from r352902, 
lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h)
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Host/ProcessLaunchInfo.h?p2=lldb/trunk/include/lldb/Host/ProcessLaunchInfo.h&p1=lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h&r1=352902&r2=353047&

[Lldb-commits] [PATCH] D56602: Move FileAction, ProcessInfo and ProcessLaunchInfo from Target to Host

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353047: Move FileAction, ProcessInfo and ProcessLaunchInfo 
from Target to Host (authored by labath, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56602?vs=181278&id=185036#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56602

Files:
  lldb/trunk/include/lldb/Host/FileAction.h
  lldb/trunk/include/lldb/Host/ProcessInfo.h
  lldb/trunk/include/lldb/Host/ProcessLaunchInfo.h
  lldb/trunk/include/lldb/Target/FileAction.h
  lldb/trunk/include/lldb/Target/Process.h
  lldb/trunk/include/lldb/Target/ProcessInfo.h
  lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
  lldb/trunk/include/lldb/Target/Target.h
  lldb/trunk/include/lldb/module.modulemap
  lldb/trunk/source/API/SBLaunchInfo.cpp
  lldb/trunk/source/Host/CMakeLists.txt
  lldb/trunk/source/Host/common/FileAction.cpp
  lldb/trunk/source/Host/common/Host.cpp
  lldb/trunk/source/Host/common/MonitoringProcessLauncher.cpp
  lldb/trunk/source/Host/common/ProcessInfo.cpp
  lldb/trunk/source/Host/common/ProcessLaunchInfo.cpp
  lldb/trunk/source/Host/macosx/objcxx/Host.mm
  lldb/trunk/source/Host/posix/ProcessLauncherPosixFork.cpp
  lldb/trunk/source/Host/windows/ProcessLauncherWindows.cpp
  
lldb/trunk/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
  lldb/trunk/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
  lldb/trunk/source/Target/CMakeLists.txt
  lldb/trunk/source/Target/FileAction.cpp
  lldb/trunk/source/Target/ProcessInfo.cpp
  lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  lldb/trunk/unittests/Host/CMakeLists.txt
  lldb/trunk/unittests/Host/FileActionTest.cpp
  lldb/trunk/unittests/Host/ProcessInfoTest.cpp
  lldb/trunk/unittests/Host/ProcessLaunchInfoTest.cpp
  lldb/trunk/unittests/tools/lldb-server/tests/TestClient.cpp
  lldb/trunk/unittests/tools/lldb-server/tests/TestClient.h

Index: lldb/trunk/unittests/Host/FileActionTest.cpp
===
--- lldb/trunk/unittests/Host/FileActionTest.cpp
+++ lldb/trunk/unittests/Host/FileActionTest.cpp
@@ -0,0 +1,20 @@
+//===-- FileActionTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Host/FileAction.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+TEST(FileActionTest, Open) {
+  FileAction Action;
+  Action.Open(47, FileSpec("/tmp"), /*read*/ true, /*write*/ false);
+  EXPECT_EQ(Action.GetAction(), FileAction::eFileActionOpen);
+  EXPECT_EQ(Action.GetFileSpec(), FileSpec("/tmp"));
+}
Index: lldb/trunk/unittests/Host/CMakeLists.txt
===
--- lldb/trunk/unittests/Host/CMakeLists.txt
+++ lldb/trunk/unittests/Host/CMakeLists.txt
@@ -1,9 +1,12 @@
 set (FILES
+  FileActionTest.cpp
   FileSystemTest.cpp
   HostInfoTest.cpp
   HostTest.cpp
   MainLoopTest.cpp
   NativeProcessProtocolTest.cpp
+  ProcessInfoTest.cpp
+  ProcessLaunchInfoTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   SymbolsTest.cpp
Index: lldb/trunk/unittests/Host/ProcessInfoTest.cpp
===
--- lldb/trunk/unittests/Host/ProcessInfoTest.cpp
+++ lldb/trunk/unittests/Host/ProcessInfoTest.cpp
@@ -0,0 +1,20 @@
+//===-- ProcessInfoTest.cpp -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Host/ProcessInfo.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+TEST(ProcessInfoTest, Constructor) {
+  ProcessInfo Info("foo", ArchSpec("x86_64-pc-linux"), 47);
+  EXPECT_STREQ("foo", Info.GetName());
+  EXPECT_EQ(ArchSpec("x86_64-pc-linux"), Info.GetArchitecture());
+  EXPECT_EQ(47, Info.GetProcessID());
+}
Index: lldb/trunk/unittests/Host/ProcessLaunchInfoTest.cpp
===
--- lldb/trunk/unittests/Host/ProcessLaunchInfoTest.cpp
+++ lldb/trunk/unittests/Host/ProcessLaunchInfoTest.cpp
@@ -0,0 +1,28 @@
+//===-- ProcessLaun

[Lldb-commits] [PATCH] D56737: [SymbolFile] Split ParseVariablesForContext into two functions.

[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 to me.


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

https://reviews.llvm.org/D56737



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

[Lldb-commits] [lldb] r353049 - Fixes for the ProcessLaunchInfo move

[Pavel Labath via lldb-commits]

Author: labath
Date: Mon Feb  4 07:03:06 2019
New Revision: 353049

URL: http://llvm.org/viewvc/llvm-project?rev=353049&view=rev
Log:
Fixes for the ProcessLaunchInfo move

Modified:

lldb/trunk/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h
lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp

Modified: 
lldb/trunk/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h?rev=353049&r1=353048&r2=353049&view=diff
==
--- 
lldb/trunk/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h
 (original)
+++ 
lldb/trunk/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h
 Mon Feb  4 07:03:06 2019
@@ -19,7 +19,7 @@
 #else
 typedef void *id;
 #endif
-#include "lldb/Target/ProcessLaunchInfo.h"
+#include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"

Modified: lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp?rev=353049&r1=353048&r2=353049&view=diff
==
--- lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp 
(original)
+++ lldb/trunk/source/Plugins/Process/Windows/Common/DebuggerThread.cpp Mon Feb 
 4 07:03:06 2019
@@ -11,12 +11,12 @@
 #include "IDebugDelegate.h"
 
 #include "lldb/Core/ModuleSpec.h"
+#include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Host/windows/HostProcessWindows.h"
 #include "lldb/Host/windows/HostThreadWindows.h"
 #include "lldb/Host/windows/ProcessLauncherWindows.h"
 #include "lldb/Target/Process.h"
-#include "lldb/Target/ProcessLaunchInfo.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Predicate.h"


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

[Lldb-commits] [PATCH] D56904: [NativePDB] Process virtual bases in the correct order

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov added a comment.

Ping! What do you think about it?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56904



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

[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov added a comment.

Ping! Can you take a look, please?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55318



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

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

[Hui Huang via Phabricator via lldb-commits]

Hui added inline comments.



Comment at: 
source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp:82
+  // Second, try through the underlying platform.
+  load_addr = Host::GetProcessBaseAddress(m_process->GetID());
+  if (load_addr != LLDB_INVALID_ADDRESS)

Shall not come to this point for a remote process. Need to bail out ahead.


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

https://reviews.llvm.org/D56237



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

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp:82
+  // Second, try through the underlying platform.
+  load_addr = Host::GetProcessBaseAddress(m_process->GetID());
+  if (load_addr != LLDB_INVALID_ADDRESS)

Hui wrote:
> Shall not come to this point for a remote process. Need to bail out ahead.
Yes, but what's the reason for the fallback in the first place? ProcessWindows 
is already by definition on the host, so you could do the same work in 
ProcessWindows::GetFileLoadAddress (but I'm not saying you should, since that 
information should already be known through ProcessWindows, as it received it 
through the windows debug api in OnLoadDLL ).


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

https://reviews.llvm.org/D56237



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

[Lldb-commits] [PATCH] D57689: Adds property to force enabling of GDB JIT loader for MacOS

[Yury Delendik via Phabricator via lldb-commits]

yurydelendik created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Based on 
https://gist.github.com/thlorenz/30bf0a3f67b1d97b2945#patching-and-rebuilding

The functionality was disabled at 
https://github.com/llvm/llvm-project/commit/521c2278abb16f0148cef1bd061cadb01ef43192


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D57689

Files:
  lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp


Index: lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
===
--- lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -59,9 +59,12 @@
 
 static constexpr PropertyDefinition g_properties[] = {
 {"enable-jit-breakpoint", OptionValue::eTypeBoolean, true, true, nullptr,
- {}, "Enable breakpoint on __jit_debug_register_code."}};
+ {}, "Enable breakpoint on __jit_debug_register_code."},
+{"enable-loader-for-darwin", OptionValue::eTypeBoolean, true, false, 
nullptr,
+ {}, "Enable process loader monitor for macOS - disabled there for "
+ "performance reasons."}};
 
-enum { ePropertyEnableJITBreakpoint };
+enum { ePropertyEnableJITBreakpoint, ePropertyEnableLoaderForDarwin };
 
 class PluginProperties : public Properties {
 public:
@@ -79,6 +82,12 @@
 nullptr, ePropertyEnableJITBreakpoint,
 g_properties[ePropertyEnableJITBreakpoint].default_uint_value != 0);
   }
+
+  bool GetEnableLoaderForDwarwin() const {
+return m_collection_sp->GetPropertyAtIndexAsBoolean(
+nullptr, ePropertyEnableLoaderForDarwin,
+g_properties[ePropertyEnableLoaderForDarwin].default_uint_value != 0);
+  }
 };
 
 typedef std::shared_ptr JITLoaderGDBPropertiesSP;
@@ -401,7 +410,8 @@
 JITLoaderSP JITLoaderGDB::CreateInstance(Process *process, bool force) {
   JITLoaderSP jit_loader_sp;
   ArchSpec arch(process->GetTarget().GetArchitecture());
-  if (arch.GetTriple().getVendor() != llvm::Triple::Apple)
+  if (arch.GetTriple().getVendor() != llvm::Triple::Apple ||
+  GetGlobalPluginProperties()->GetEnableLoaderForDwarwin())
 jit_loader_sp.reset(new JITLoaderGDB(process));
   return jit_loader_sp;
 }


Index: lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
===
--- lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -59,9 +59,12 @@
 
 static constexpr PropertyDefinition g_properties[] = {
 {"enable-jit-breakpoint", OptionValue::eTypeBoolean, true, true, nullptr,
- {}, "Enable breakpoint on __jit_debug_register_code."}};
+ {}, "Enable breakpoint on __jit_debug_register_code."},
+{"enable-loader-for-darwin", OptionValue::eTypeBoolean, true, false, nullptr,
+ {}, "Enable process loader monitor for macOS - disabled there for "
+ "performance reasons."}};
 
-enum { ePropertyEnableJITBreakpoint };
+enum { ePropertyEnableJITBreakpoint, ePropertyEnableLoaderForDarwin };
 
 class PluginProperties : public Properties {
 public:
@@ -79,6 +82,12 @@
 nullptr, ePropertyEnableJITBreakpoint,
 g_properties[ePropertyEnableJITBreakpoint].default_uint_value != 0);
   }
+
+  bool GetEnableLoaderForDwarwin() const {
+return m_collection_sp->GetPropertyAtIndexAsBoolean(
+nullptr, ePropertyEnableLoaderForDarwin,
+g_properties[ePropertyEnableLoaderForDarwin].default_uint_value != 0);
+  }
 };
 
 typedef std::shared_ptr JITLoaderGDBPropertiesSP;
@@ -401,7 +410,8 @@
 JITLoaderSP JITLoaderGDB::CreateInstance(Process *process, bool force) {
   JITLoaderSP jit_loader_sp;
   ArchSpec arch(process->GetTarget().GetArchitecture());
-  if (arch.GetTriple().getVendor() != llvm::Triple::Apple)
+  if (arch.GetTriple().getVendor() != llvm::Triple::Apple ||
+  GetGlobalPluginProperties()->GetEnableLoaderForDwarwin())
 jit_loader_sp.reset(new JITLoaderGDB(process));
   return jit_loader_sp;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D57689: Adds property to force enabling of GDB JIT loader for MacOS

[Yury Delendik via Phabricator via lldb-commits]

yurydelendik updated this revision to Diff 185053.
yurydelendik added a comment.

Fix GetEnableLoaderForDarwin name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57689

Files:
  lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp


Index: lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
===
--- lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -59,9 +59,12 @@
 
 static constexpr PropertyDefinition g_properties[] = {
 {"enable-jit-breakpoint", OptionValue::eTypeBoolean, true, true, nullptr,
- {}, "Enable breakpoint on __jit_debug_register_code."}};
+ {}, "Enable breakpoint on __jit_debug_register_code."},
+{"enable-loader-for-darwin", OptionValue::eTypeBoolean, true, false, 
nullptr,
+ {}, "Enable process loader for macOS - disabled there for performance "
+ "reasons."}};
 
-enum { ePropertyEnableJITBreakpoint };
+enum { ePropertyEnableJITBreakpoint, ePropertyEnableLoaderForDarwin };
 
 class PluginProperties : public Properties {
 public:
@@ -79,6 +82,12 @@
 nullptr, ePropertyEnableJITBreakpoint,
 g_properties[ePropertyEnableJITBreakpoint].default_uint_value != 0);
   }
+
+  bool GetEnableLoaderForDarwin() const {
+return m_collection_sp->GetPropertyAtIndexAsBoolean(
+nullptr, ePropertyEnableLoaderForDarwin,
+g_properties[ePropertyEnableLoaderForDarwin].default_uint_value != 0);
+  }
 };
 
 typedef std::shared_ptr JITLoaderGDBPropertiesSP;
@@ -401,7 +410,8 @@
 JITLoaderSP JITLoaderGDB::CreateInstance(Process *process, bool force) {
   JITLoaderSP jit_loader_sp;
   ArchSpec arch(process->GetTarget().GetArchitecture());
-  if (arch.GetTriple().getVendor() != llvm::Triple::Apple)
+  if (arch.GetTriple().getVendor() != llvm::Triple::Apple ||
+  GetGlobalPluginProperties()->GetEnableLoaderForDarwin())
 jit_loader_sp.reset(new JITLoaderGDB(process));
   return jit_loader_sp;
 }


Index: lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
===
--- lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -59,9 +59,12 @@
 
 static constexpr PropertyDefinition g_properties[] = {
 {"enable-jit-breakpoint", OptionValue::eTypeBoolean, true, true, nullptr,
- {}, "Enable breakpoint on __jit_debug_register_code."}};
+ {}, "Enable breakpoint on __jit_debug_register_code."},
+{"enable-loader-for-darwin", OptionValue::eTypeBoolean, true, false, nullptr,
+ {}, "Enable process loader for macOS - disabled there for performance "
+ "reasons."}};
 
-enum { ePropertyEnableJITBreakpoint };
+enum { ePropertyEnableJITBreakpoint, ePropertyEnableLoaderForDarwin };
 
 class PluginProperties : public Properties {
 public:
@@ -79,6 +82,12 @@
 nullptr, ePropertyEnableJITBreakpoint,
 g_properties[ePropertyEnableJITBreakpoint].default_uint_value != 0);
   }
+
+  bool GetEnableLoaderForDarwin() const {
+return m_collection_sp->GetPropertyAtIndexAsBoolean(
+nullptr, ePropertyEnableLoaderForDarwin,
+g_properties[ePropertyEnableLoaderForDarwin].default_uint_value != 0);
+  }
 };
 
 typedef std::shared_ptr JITLoaderGDBPropertiesSP;
@@ -401,7 +410,8 @@
 JITLoaderSP JITLoaderGDB::CreateInstance(Process *process, bool force) {
   JITLoaderSP jit_loader_sp;
   ArchSpec arch(process->GetTarget().GetArchitecture());
-  if (arch.GetTriple().getVendor() != llvm::Triple::Apple)
+  if (arch.GetTriple().getVendor() != llvm::Triple::Apple ||
+  GetGlobalPluginProperties()->GetEnableLoaderForDarwin())
 jit_loader_sp.reset(new JITLoaderGDB(process));
   return jit_loader_sp;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

[Hui Huang via Phabricator via lldb-commits]

Hui added a comment.

In D56237#1383070 , @labath wrote:

> Going to the Host straight from the DynamicLoader is the worst possible 
> option, since then this code will not be correct for remote processes (it 
> will just randomly pick up some data from the local process with the same 
> pid, if you happen to have one). I'd like to hear what you think about the 
> latest comments by Greg & myself.
>
> In D56237#1373788 , @clayborg wrote:
>
> > In D56237#1373234 , @labath wrote:
> >
> > > I think there are some architectural issues here in how ProcessWindows 
> > > works. Normally (i.e., on all non-windows process classes), it is the job 
> > > of the dynamic loader plugin to create modules and set their load 
> > > addresses. However, ProcessWindows does things differently, and creates 
> > > the modules by itself (ProcessWindows::OnLoadDLL).
> > >  I think the behavior of ProcessWindows is fine (it flows naturally from 
> > > how modules are handled on windows), but if it's going to handle module 
> > > loading by itself, then I think it should do the job completely, and not 
> > > only half-way. So, if we want ProcessWindows to do the module loading, 
> > > then it should return a null value for `GetDynamicLoader()` (or a 
> > > reasonably stubbed out implementation) instead of relying on a generic 
> > > dynamic loader plugin to finish the job.
> > >
> > > Or, if we want to have the DynamicLoader plugin, then ProcessWindows 
> > > should stop meddling with the module list in the OnLoadDLL function. One 
> > > way to achieve that would be:
> > >
> > > - ProcessWindows::OnLoadDLL calls DynamicLoaderWindowsDYLD::OnLoadDll
> > > - dynamic loader loads the module and sets load address
> > > - dynamic loader does not have to call 
> > > `ProcessWindows::GetFileLoadAddress` as it already got the load address 
> > > from in the OnLoadDLL function This would make the flow very similar to 
> > > how other platforms handle it (the only difference would be that the 
> > > "OnLoadDLL" equivalent is called from a breakpoint callback function, 
> > > instead of from a special debug event callback, but that's just due to a 
> > > difference in how modules are reported).
> >
> >
> > I would like the see the dynamic loader do all of the work. We can easily 
> > add any new virtual functions to the DynamicLoader class to fit how windows 
> > does things.
>
>
> That should be fairly easy to achieve. The process class is the one that 
> creates the dynamic loader (in `GetDynamicLoader()`), so it can easily call 
> any special method on it that it needs from OnLoadDLL (i.e., we don't even 
> need to extend the public DynamicLoader API).
>
> However, note that this would not be the first process class that handles 
> module loading on its own. ProcessMinidump already does that too. It could be 
> changed to use a special dynamic loader too, but it's not clear to me whether 
> that is worth the trouble, as the loading process for minidumps is very 
> simple.


I am thinking getting load address and setting load address by dyld could be 
done in separate reviews?  For the latter, it might lead to some architecture 
changes in processwindows plugin.

For this review, getting load address of a host process can just leverage the 
OS API if no processwindows plugin's help is wanted.

Servers other than lldb-server.exe can easily implement the qFileLoadAddress 
packet (using their own loader) as a reply to m_process->GetFileLoadAddress().


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

https://reviews.llvm.org/D56237



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

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

[Hui Huang via Phabricator via lldb-commits]

Hui added inline comments.



Comment at: 
source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp:82
+  // Second, try through the underlying platform.
+  load_addr = Host::GetProcessBaseAddress(m_process->GetID());
+  if (load_addr != LLDB_INVALID_ADDRESS)

labath wrote:
> Hui wrote:
> > Shall not come to this point for a remote process. Need to bail out ahead.
> Yes, but what's the reason for the fallback in the first place? 
> ProcessWindows is already by definition on the host, so you could do the same 
> work in ProcessWindows::GetFileLoadAddress (but I'm not saying you should, 
> since that information should already be known through ProcessWindows, as it 
> received it through the windows debug api in OnLoadDLL ).
Server other lldb-server.exe could respond with a bogus address to 
qFileLoadAddress packet and then lldb (on host) will come to this point and 
result in the case you mentioned.

>  since then this code will not be correct for remote processes (it will just 
> randomly pick up some data from the local process with the same pid, if you 
> happen to have one)
> 





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

https://reviews.llvm.org/D56237



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

[Lldb-commits] [PATCH] D56237: Implement GetLoadAddress for the Windows process plugin

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D56237#1383232 , @Hui wrote:

> In D56237#1383070 , @labath wrote:
>
> > Going to the Host straight from the DynamicLoader is the worst possible 
> > option, since then this code will not be correct for remote processes (it 
> > will just randomly pick up some data from the local process with the same 
> > pid, if you happen to have one). I'd like to hear what you think about the 
> > latest comments by Greg & myself.
> >
> > In D56237#1373788 , @clayborg 
> > wrote:
> >
> > > In D56237#1373234 , @labath 
> > > wrote:
> > >
> > > > I think there are some architectural issues here in how ProcessWindows 
> > > > works. Normally (i.e., on all non-windows process classes), it is the 
> > > > job of the dynamic loader plugin to create modules and set their load 
> > > > addresses. However, ProcessWindows does things differently, and creates 
> > > > the modules by itself (ProcessWindows::OnLoadDLL).
> > > >  I think the behavior of ProcessWindows is fine (it flows naturally 
> > > > from how modules are handled on windows), but if it's going to handle 
> > > > module loading by itself, then I think it should do the job completely, 
> > > > and not only half-way. So, if we want ProcessWindows to do the module 
> > > > loading, then it should return a null value for `GetDynamicLoader()` 
> > > > (or a reasonably stubbed out implementation) instead of relying on a 
> > > > generic dynamic loader plugin to finish the job.
> > > >
> > > > Or, if we want to have the DynamicLoader plugin, then ProcessWindows 
> > > > should stop meddling with the module list in the OnLoadDLL function. 
> > > > One way to achieve that would be:
> > > >
> > > > - ProcessWindows::OnLoadDLL calls DynamicLoaderWindowsDYLD::OnLoadDll
> > > > - dynamic loader loads the module and sets load address
> > > > - dynamic loader does not have to call 
> > > > `ProcessWindows::GetFileLoadAddress` as it already got the load address 
> > > > from in the OnLoadDLL function This would make the flow very similar to 
> > > > how other platforms handle it (the only difference would be that the 
> > > > "OnLoadDLL" equivalent is called from a breakpoint callback function, 
> > > > instead of from a special debug event callback, but that's just due to 
> > > > a difference in how modules are reported).
> > >
> > >
> > > I would like the see the dynamic loader do all of the work. We can easily 
> > > add any new virtual functions to the DynamicLoader class to fit how 
> > > windows does things.
> >
> >
> > That should be fairly easy to achieve. The process class is the one that 
> > creates the dynamic loader (in `GetDynamicLoader()`), so it can easily call 
> > any special method on it that it needs from OnLoadDLL (i.e., we don't even 
> > need to extend the public DynamicLoader API).
> >
> > However, note that this would not be the first process class that handles 
> > module loading on its own. ProcessMinidump already does that too. It could 
> > be changed to use a special dynamic loader too, but it's not clear to me 
> > whether that is worth the trouble, as the loading process for minidumps is 
> > very simple.
>
>
> I am thinking getting load address and setting load address by dyld could be 
> done in separate reviews?  For the latter, it might lead to some architecture 
> changes in processwindows plugin.
>
> For this review, getting load address of a host process can just leverage the 
> OS API if no processwindows plugin's help is wanted.


Yes, two reviews would be great. However, the way I see this, the second patch 
would basically remove the need for the special Host function you're trying to 
introduce here. So I'd suggest to do the second part first, and then this 
should be  very simple addon on top of that.

> Servers other than lldb-server.exe can easily implement the qFileLoadAddress 
> packet (using their own loader) as a reply to m_process->GetFileLoadAddress().

I am aware of that. However, my point is that you shouldn't need *any* fallback 
here. If ProcessWindows::GetFileLoadAddress does the right thing, then all the 
dyld plugin would not need to call any host functions *at all* (which is just a 
fundamentally wrong thing to do).


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

https://reviews.llvm.org/D56237



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

[Lldb-commits] [PATCH] D51578: Contiguous .debug_info+.debug_types for D32167

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:693-707
+DataBufferHeap *databufferheap = new DataBufferHeap();
+DataBufferSP databuffer = DataBufferSP(databufferheap);
+databufferheap->AppendData(
+debug_info_data.GetDataStart(),
+debug_info_data.GetByteSize());
+m_debug_info_concatenated_types_offset =
+databufferheap->GetByteSize();

So if we have 3GB of .debug_info and 1GB of .debug_types, we are expecting to 
allocate a heap based buffer and copy all the data into this? This will fail.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D51578



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Jim Ingham via Phabricator via lldb-commits]

jingham marked an inline comment as done.
jingham added inline comments.



Comment at: source/Target/PathMappingList.cpp:223
+  
+  if (prefix_ref == ".") {
+prefix_is_relative = true;

clayborg wrote:
> We are we finding a "." in any path now? I thought we normalized those all 
> out? Can a PathMappingList contain non normalized paths? if so, can't we just 
> normalize them as they go into the list?
That's the incoming path from the path maps.  You can't normalize just a "." 
from the path mappings or it would become "" and we don't really know what that 
means.  These are currently left as ".".  I think that's right.


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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D56904: [NativePDB] Process virtual bases in the correct order

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

In D56904#1383148 , @aleksandr.urakov 
wrote:

> Ping! What do you think about it?


Generally the change looks good, but I'm not sure why the compiler can't 
properly compile the source file.  I know it's something to do with the 
environment, but it's hard to say what.

One idea would be to add `-###` to the end of your command line when running 
the command manually, and then update the `build.py` script so that it also 
runs `-###` from the command line and prints the output.  Then maybe compare 
the two command lines and see if there's a difference.

Another idea would be to hack up the code in the `build.py` script that 
modifies the child process's environment.  Start with the child environment 
hardcoded to your terminal's environment, and then delete variables until the 
problem reproduces.  That should tell you what environment variable is causing 
the problem.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56904



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

[Lldb-commits] [PATCH] D57689: Adds property to force enabling of GDB JIT loader for MacOS

[Jim Ingham via Phabricator via lldb-commits]

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

It would be better to have the setting be an enum of "on/off/default", and then 
have the somebody - the current DynamicLoader plugin seems the best somebody - 
provide the default value if the setting hasn't been explicitly set.  That way 
on any platform one could turn the loading on and off, which seems useful, and 
we wouldn't have to have a Darwin specific setting that will cease being 
applicable when the Darwin default switches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57689



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

[Lldb-commits] [lldb] r353087 - Update stale comment in lang/c/struct_types/main.c

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Mon Feb  4 12:33:35 2019
New Revision: 353087

URL: http://llvm.org/viewvc/llvm-project?rev=353087&view=rev
Log:
Update stale comment in lang/c/struct_types/main.c

rdar://47322760

Modified:
lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c

Modified: lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c?rev=353087&r1=353086&r2=353087&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c 
(original)
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c Mon 
Feb  4 12:33:35 2019
@@ -26,7 +26,7 @@ int main (int argc, char const *argv[])
 }; //% self.expect("frame variable pt.padding[0]", 
DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["pt.padding[0] = "])
//% self.expect("frame variable pt.padding[1]", 
DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["pt.padding[1] = "])
//% self.expect("expression -- (pt.padding[0])", 
DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["(char)", " = "])
-   //% self.expect("image lookup -t point_tag", 
DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ['padding[]']) # Once 
rdar://problem/12566646 is fixed, this should display correctly
+   //% self.expect("image lookup -t point_tag", 
DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ['padding[]'])
 
 struct {} empty;
 //% self.expect("frame variable empty", substrs = ["empty = {}"])
@@ -42,5 +42,5 @@ int main (int argc, char const *argv[])
 
 int sum = sum_things(tts); //% self.expect("expression -- &pt == (struct 
point_tag*)0", substrs = ['false'])
//% self.expect("expression -- 
sum_things(tts)", substrs = ['9'])
-return 0; 
+return 0;
 }


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

Re: [Lldb-commits] [lldb] r353087 - Update stale comment in lang/c/struct_types/main.c

[Adrian Prantl via lldb-commits]

> On Feb 4, 2019, at 12:33 PM, Jonas Devlieghere via lldb-commits 
>  wrote:
> 
> Author: jdevlieghere
> Date: Mon Feb  4 12:33:35 2019
> New Revision: 353087
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=353087&view=rev
> Log:
> Update stale comment in lang/c/struct_types/main.c
> 
> rdar://47322760
> 
> Modified:
>lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c
> 
> Modified: lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c?rev=353087&r1=353086&r2=353087&view=diff
> ==
> --- lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c 
> (original)
> +++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/struct_types/main.c Mon 
> Feb  4 12:33:35 2019
> @@ -26,7 +26,7 @@ int main (int argc, char const *argv[])
> }; //% self.expect("frame variable pt.padding[0]", 
> DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["pt.padding[0] = "])
>//% self.expect("frame variable pt.padding[1]", 
> DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["pt.padding[1] = "])
>//% self.expect("expression -- (pt.padding[0])", 
> DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ["(char)", " = "])
> -   //% self.expect("image lookup -t point_tag", 
> DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ['padding[]']) # Once 
> rdar://problem/12566646 is fixed, this should display correctly
Was this meant to be checking for something stricter, or was the comment just 
bogus?

thanks!
adrian
> +   //% self.expect("image lookup -t point_tag", 
> DATA_TYPES_DISPLAYED_CORRECTLY, substrs = ['padding[]'])
> 
> struct {} empty;
> //% self.expect("frame variable empty", substrs = ["empty = {}"])
> @@ -42,5 +42,5 @@ int main (int argc, char const *argv[])
> 
> int sum = sum_things(tts); //% self.expect("expression -- &pt == (struct 
> point_tag*)0", substrs = ['false'])
>//% self.expect("expression -- 
> sum_things(tts)", substrs = ['9'])
> -return 0; 
> +return 0;
> }
> 
> 
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

[Aaron Smith via Phabricator via lldb-commits]

asmith added a comment.

Thanks for the feedback and interesting discussion! If no more comments, let's 
go with this version for now.


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

https://reviews.llvm.org/D56230



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

[Lldb-commits] [PATCH] D57714: [Reproducers] Instrumentation Framework: Serialization

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: labath, davide.
JDevlieghere added a project: LLDB.
Herald added a subscriber: mgorny.

This is the is serialization/deserialization part of the reproducer 
instrumentation framework.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D57714

Files:
  include/lldb/Utility/ReproducerInstrumentation.h
  source/Utility/CMakeLists.txt
  source/Utility/ReproducerInstrumentation.cpp
  unittests/Utility/CMakeLists.txt
  unittests/Utility/ReproducerInstrumentationTest.cpp

Index: unittests/Utility/ReproducerInstrumentationTest.cpp
===
--- /dev/null
+++ unittests/Utility/ReproducerInstrumentationTest.cpp
@@ -0,0 +1,208 @@
+//===-- ReproducerTest.cpp *- 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
+//
+//===--===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/ReproducerInstrumentation.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+
+namespace {
+struct Foo {
+  int m = 1;
+};
+struct Bar {
+  double m = 2;
+};
+
+bool operator==(const Foo &LHS, const Foo &RHS) { return LHS.m == RHS.m; }
+bool operator==(const Bar &LHS, const Bar &RHS) { return LHS.m == RHS.m; }
+
+struct Pod {
+  bool a = true;
+  bool b = false;
+  char c = 'a';
+  float d = 1.1;
+  int e = 2;
+  long long f = 3;
+  long g = 4;
+  short h = 5;
+  unsigned char i = 'b';
+  unsigned int j = 6;
+  unsigned long long k = 7;
+  unsigned long l = 8;
+  unsigned short m = 9;
+};
+} // namespace
+
+static const Pod p;
+
+TEST(IndexToObjectTest, ObjectForIndex) {
+  IndexToObject index_to_object;
+  Foo foo;
+  Bar bar;
+
+  EXPECT_EQ(nullptr, index_to_object.GetObjectForIndex(1));
+  EXPECT_EQ(nullptr, index_to_object.GetObjectForIndex(2));
+
+  index_to_object.AddObjectForIndex(1, foo);
+  index_to_object.AddObjectForIndex(2, &bar);
+
+  EXPECT_EQ(&foo, index_to_object.GetObjectForIndex(1));
+  EXPECT_EQ(&bar, index_to_object.GetObjectForIndex(2));
+}
+
+TEST(DeserializerTest, HasData) {
+  {
+Deserializer deserializer("");
+EXPECT_FALSE(deserializer.HasData(1));
+  }
+
+  {
+Deserializer deserializer("a");
+EXPECT_TRUE(deserializer.HasData(1));
+EXPECT_FALSE(deserializer.HasData(2));
+  }
+}
+
+TEST(SerializationRountripTest, SerializeDeserializePod) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(p.a, p.b, p.c, p.d, p.e, p.f, p.g, p.h, p.i, p.j, p.k,
+  p.l, p.m);
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  EXPECT_EQ(p.a, deserializer.Deserialize());
+  EXPECT_EQ(p.b, deserializer.Deserialize());
+  EXPECT_EQ(p.c, deserializer.Deserialize());
+  EXPECT_EQ(p.d, deserializer.Deserialize());
+  EXPECT_EQ(p.e, deserializer.Deserialize());
+  EXPECT_EQ(p.f, deserializer.Deserialize());
+  EXPECT_EQ(p.g, deserializer.Deserialize());
+  EXPECT_EQ(p.h, deserializer.Deserialize());
+  EXPECT_EQ(p.i, deserializer.Deserialize());
+  EXPECT_EQ(p.j, deserializer.Deserialize());
+  EXPECT_EQ(p.k, deserializer.Deserialize());
+  EXPECT_EQ(p.l, deserializer.Deserialize());
+  EXPECT_EQ(p.m, deserializer.Deserialize());
+}
+
+TEST(SerializationRountripTest, SerializeDeserializePodPointers) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(&p.a, &p.b, &p.c, &p.d, &p.e, &p.f, &p.g, &p.h, &p.i,
+  &p.j, &p.k, &p.l, &p.m);
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  EXPECT_EQ(p.a, *deserializer.Deserialize());
+  EXPECT_EQ(p.b, *deserializer.Deserialize());
+  EXPECT_EQ(p.c, *deserializer.Deserialize());
+  EXPECT_EQ(p.d, *deserializer.Deserialize());
+  EXPECT_EQ(p.e, *deserializer.Deserialize());
+  EXPECT_EQ(p.f, *deserializer.Deserialize());
+  EXPECT_EQ(p.g, *deserializer.Deserialize());
+  EXPECT_EQ(p.h, *deserializer.Deserialize());
+  EXPECT_EQ(p.i, *deserializer.Deserialize());
+  EXPECT_EQ(p.j, *deserializer.Deserialize());
+  EXPECT_EQ(p.k, *deserializer.Deserialize());
+  EXPECT_EQ(p.l, *deserializer.Deserialize());
+  EXPECT_EQ(p.m, *deserializer.Deserialize());
+}
+
+TEST(SerializationRountripTest, SerializeDeserializePodReferences) {
+  std::string str;
+  llvm::raw_string_ostream os(str);
+
+  Serializer serializer(os);
+  serializer.SerializeAll(p.a, p.b, p.c, p.d, p.e, p.f, p.g, p.h, p.i, p.j, p.k,
+  p.l, p.m);
+
+  llvm::StringRef buffer(os.str());
+  Deserializer deserializer(buffer);
+
+  EXPECT_EQ(p.a, deserializer.Deserialize());
+  EXPECT_EQ(p.b, deserializer.Deserialize());
+  EXPEC

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 11 inline comments as done.
JDevlieghere added a comment.

In D56322#1382971 , @labath wrote:

> Thank you. I think this looks much better now.
>
> It occurred to me that the (de)serializer classes are fully standalone. Since 
> they already have a comprehensive test suite, do you think it would make 
> sense to split them off into a separate patch, which can be committed 
> separately?


Sure, makes sense to me, please see D57714 


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

https://reviews.llvm.org/D56322



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

[Lldb-commits] [PATCH] D57689: Adds property to force enabling of GDB JIT loader for MacOS

[Yury Delendik via Phabricator via lldb-commits]

yurydelendik updated this revision to Diff 185164.
yurydelendik added a comment.

- Change to on/off/default property.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57689

Files:
  lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp


Index: lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
===
--- lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -57,11 +57,29 @@
 
 namespace {
 
+enum EnableJITLoaderGDB {
+  eEnableJITLoaderGDBDefault,
+  eEnableJITLoaderGDBOn,
+  eEnableJITLoaderGDBOff,
+};
+
+static constexpr OptionEnumValueElement g_enable_jit_loader_gdb_enumerators[] 
= {
+{eEnableJITLoaderGDBDefault, "default", "Enable JIT compilation interface "
+ "for all platforms except macOS"},
+{eEnableJITLoaderGDBOn, "on", "Enable JIT compilation interface"},
+{eEnableJITLoaderGDBOff, "off", "Disable JIT compilation interface"}
+ };
+
 static constexpr PropertyDefinition g_properties[] = {
+{"enable", OptionValue::eTypeEnum, true,
+ eEnableJITLoaderGDBDefault, nullptr,
+ OptionEnumValues(g_enable_jit_loader_gdb_enumerators),
+ "Enable GDB's JIT compilation interface (default: enabled on "
+ "all platforms except macOS)"},
 {"enable-jit-breakpoint", OptionValue::eTypeBoolean, true, true, nullptr,
  {}, "Enable breakpoint on __jit_debug_register_code."}};
 
-enum { ePropertyEnableJITBreakpoint };
+enum { ePropertyEnable, ePropertyEnableJITBreakpoint };
 
 class PluginProperties : public Properties {
 public:
@@ -79,6 +97,12 @@
 nullptr, ePropertyEnableJITBreakpoint,
 g_properties[ePropertyEnableJITBreakpoint].default_uint_value != 0);
   }
+
+  EnableJITLoaderGDB GetEnable() const {
+return 
(EnableJITLoaderGDB)m_collection_sp->GetPropertyAtIndexAsEnumeration(
+nullptr, ePropertyEnable,
+g_properties[ePropertyEnable].default_uint_value);
+  }
 };
 
 typedef std::shared_ptr JITLoaderGDBPropertiesSP;
@@ -400,8 +424,20 @@
 
 JITLoaderSP JITLoaderGDB::CreateInstance(Process *process, bool force) {
   JITLoaderSP jit_loader_sp;
-  ArchSpec arch(process->GetTarget().GetArchitecture());
-  if (arch.GetTriple().getVendor() != llvm::Triple::Apple)
+  bool enable;
+  switch (GetGlobalPluginProperties()->GetEnable()) {
+case EnableJITLoaderGDB::eEnableJITLoaderGDBOn:
+  enable = true;
+  break;
+case EnableJITLoaderGDB::eEnableJITLoaderGDBOff:
+  enable = false;
+  break;
+case EnableJITLoaderGDB::eEnableJITLoaderGDBDefault:
+  ArchSpec arch(process->GetTarget().GetArchitecture());
+  enable = arch.GetTriple().getVendor() != llvm::Triple::Apple;
+  break;
+  }
+  if (enable)
 jit_loader_sp.reset(new JITLoaderGDB(process));
   return jit_loader_sp;
 }


Index: lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
===
--- lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
+++ lldb/source/Plugins/JITLoader/GDB/JITLoaderGDB.cpp
@@ -57,11 +57,29 @@
 
 namespace {
 
+enum EnableJITLoaderGDB {
+  eEnableJITLoaderGDBDefault,
+  eEnableJITLoaderGDBOn,
+  eEnableJITLoaderGDBOff,
+};
+
+static constexpr OptionEnumValueElement g_enable_jit_loader_gdb_enumerators[] = {
+{eEnableJITLoaderGDBDefault, "default", "Enable JIT compilation interface "
+ "for all platforms except macOS"},
+{eEnableJITLoaderGDBOn, "on", "Enable JIT compilation interface"},
+{eEnableJITLoaderGDBOff, "off", "Disable JIT compilation interface"}
+ };
+
 static constexpr PropertyDefinition g_properties[] = {
+{"enable", OptionValue::eTypeEnum, true,
+ eEnableJITLoaderGDBDefault, nullptr,
+ OptionEnumValues(g_enable_jit_loader_gdb_enumerators),
+ "Enable GDB's JIT compilation interface (default: enabled on "
+ "all platforms except macOS)"},
 {"enable-jit-breakpoint", OptionValue::eTypeBoolean, true, true, nullptr,
  {}, "Enable breakpoint on __jit_debug_register_code."}};
 
-enum { ePropertyEnableJITBreakpoint };
+enum { ePropertyEnable, ePropertyEnableJITBreakpoint };
 
 class PluginProperties : public Properties {
 public:
@@ -79,6 +97,12 @@
 nullptr, ePropertyEnableJITBreakpoint,
 g_properties[ePropertyEnableJITBreakpoint].default_uint_value != 0);
   }
+
+  EnableJITLoaderGDB GetEnable() const {
+return (EnableJITLoaderGDB)m_collection_sp->GetPropertyAtIndexAsEnumeration(
+nullptr, ePropertyEnable,
+g_properties[ePropertyEnable].default_uint_value);
+  }
 };
 
 typedef std::shared_ptr JITLoaderGDBPropertiesSP;
@@ -400,8 +424,20 @@
 
 JITLoaderSP JITLoaderGDB::CreateInstance(Process *process, bool force) {
   JITLoaderSP jit_loader_sp;
-  ArchSpec arch(process->GetTarget().GetArchitecture());
-  if (arch.GetTriple().getVendor() != llvm::Triple::Apple)
+  bool enable;
+  switch (GetGlobalPluginPro

[Lldb-commits] [PATCH] D57689: Adds property to force enabling of GDB JIT loader for MacOS

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

That looks good.  Could you add a test for this setting to the 
./functionalities/jitloader_gdb/TestJITLoaderGDB.py test?  I wouldn't test that 
the default has any particular behavior because that might change over time.  
But test that if you turn it on, you do get load notifications, and if you turn 
it off you don't.  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57689



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

[Lldb-commits] [PATCH] D55318: [Expressions] Add support of expressions evaluation in some object's context

[Jim Ingham via Phabricator via lldb-commits]

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

Thanks for adding the ObjC tests!  I had two trivial requests for the test 
comments to be a little clearer, but even without that this is fine.




Comment at: 
packages/Python/lldbsuite/test/expression_command/context-object-objc/TestContextObjectObjc.py:42-52
+# Test retrieveing of a property
+value = obj_val.EvaluateExpression("self.property")
+self.assertTrue(value.IsValid())
+self.assertTrue(value.GetError().Success())
+self.assertEqual(value.GetValueAsSigned(), )
+
+# Test functions evaluation

IIUC, these last two are testing that evaluating in the context of an object 
doesn't effect computations that don't refer to that object.  Is that right?  
After all "self.property" doesn't need any help from the objcClass variable to 
look up the result.  Can you make the comment a little clearer.



Comment at: 
packages/Python/lldbsuite/test/expression_command/context-object-objc/TestContextObjectObjc.py:54
+
+# Test computation result
+obj_val = frame.EvaluateExpression("[ObjcClass createNew]")

This one is testing that when the reference object is an lldb result object, we 
can still use it correctly.  "Test computation result" is a little terse.  Can 
you make this a little clearer.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55318



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

[Lldb-commits] [lldb] r353130 - [Python2 to Python 3] Fix print -> print().

[Davide Italiano via lldb-commits]

Author: davide
Date: Mon Feb  4 16:59:57 2019
New Revision: 353130

URL: http://llvm.org/viewvc/llvm-project?rev=353130&view=rev
Log:
[Python2 to Python 3] Fix print -> print().

Modified:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py?rev=353130&r1=353129&r2=353130&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py
 Mon Feb  4 16:59:57 2019
@@ -170,28 +170,28 @@ class TestScriptedResolver(TestBase):
 # Make a breakpoint that has no __get_depth__, check that that is 
converted to eSearchDepthModule:
 bkpt = target.BreakpointCreateFromScript("resolver.Resolver", 
extra_args, module_list, file_list)
 self.assertTrue(bkpt.GetNumLocations() > 0, "Resolver got no 
locations.")
-self.expect("script print resolver.Resolver.got_files", substrs=["2"], 
msg="Was only passed modules")
+self.expect("script print(resolver.Resolver.got_files)", 
substrs=["2"], msg="Was only passed modules")
 
 # Make a breakpoint that asks for modules, check that we didn't get 
any files:
 bkpt = 
target.BreakpointCreateFromScript("resolver.ResolverModuleDepth", extra_args, 
module_list, file_list)
 self.assertTrue(bkpt.GetNumLocations() > 0, "ResolverModuleDepth got 
no locations.")
-self.expect("script print resolver.Resolver.got_files", substrs=["2"], 
msg="Was only passed modules")
+self.expect("script print(resolver.Resolver.got_files)", 
substrs=["2"], msg="Was only passed modules")
 
 # Make a breakpoint that asks for compile units, check that we didn't 
get any files:
 bkpt = target.BreakpointCreateFromScript("resolver.ResolverCUDepth", 
extra_args, module_list, file_list)
 self.assertTrue(bkpt.GetNumLocations() > 0, "ResolverCUDepth got no 
locations.")
-self.expect("script print resolver.Resolver.got_files", substrs=["1"], 
msg="Was passed compile units")
+self.expect("script print(resolver.Resolver.got_files)", 
substrs=["1"], msg="Was passed compile units")
 
 # Make a breakpoint that returns a bad value - we should convert that 
to "modules" so check that:
 bkpt = target.BreakpointCreateFromScript("resolver.ResolverBadDepth", 
extra_args, module_list, file_list)
 self.assertTrue(bkpt.GetNumLocations() > 0, "ResolverBadDepth got no 
locations.")
-self.expect("script print resolver.Resolver.got_files", substrs=["2"], 
msg="Was only passed modules")
+self.expect("script print(resolver.Resolver.got_files)", 
substrs=["2"], msg="Was only passed modules")
 
 # Make a breakpoint that searches at function depth:
 bkpt = target.BreakpointCreateFromScript("resolver.ResolverFuncDepth", 
extra_args, module_list, file_list)
 self.assertTrue(bkpt.GetNumLocations() > 0, "ResolverFuncDepth got no 
locations.")
-self.expect("script print resolver.Resolver.got_files", substrs=["3"], 
msg="Was only passed modules")
-self.expect("script print resolver.Resolver.func_list", 
substrs=["break_on_me", "main", "test_func"], msg="Saw all the functions")
+self.expect("script print(resolver.Resolver.got_files)", 
substrs=["3"], msg="Was only passed modules")
+self.expect("script print(resolver.Resolver.func_list)", 
substrs=["break_on_me", "main", "test_func"], msg="Saw all the functions")
 
 def do_test_cli(self):
 target = self.make_target_and_import()


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

[Lldb-commits] [PATCH] D56221: [lldb] Make frame recognizers vend synthesized eValueTypeVariableArgument values

[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:
  rLLDB LLDB

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

https://reviews.llvm.org/D56221



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

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 185207.

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

https://reviews.llvm.org/D56322

Files:
  include/lldb/API/SBReproducer.h
  include/lldb/Utility/ReproducerInstrumentation.h
  source/API/CMakeLists.txt
  source/API/SBReproducer.cpp
  source/API/SBReproducerPrivate.h
  source/Utility/ReproducerInstrumentation.cpp
  tools/driver/Driver.cpp
  unittests/Utility/ReproducerInstrumentationTest.cpp

Index: unittests/Utility/ReproducerInstrumentationTest.cpp
===
--- unittests/Utility/ReproducerInstrumentationTest.cpp
+++ unittests/Utility/ReproducerInstrumentationTest.cpp
@@ -9,12 +9,24 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include 
+#include 
+
 #include "lldb/Utility/ReproducerInstrumentation.h"
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
 
-namespace {
+#define SB_REGISTER_CONSTRUCTOR(Class, Signature)  \
+  Register(&construct::doit)
+#define SB_REGISTER_METHOD(Result, Class, Method, Signature)   \
+  Register(&invoke::method<&Class::Method>::doit)
+#define SB_REGISTER_METHOD_CONST(Result, Class, Method, Signature) \
+  Register(&invoke::method_const<&Class::Method>::doit)
+#define SB_REGISTER_STATIC_METHOD(Result, Class, Method, Signature)\
+  Register(static_cast(&Class::Method))
+
 struct Foo {
   int m = 1;
 };
@@ -40,7 +52,166 @@
   unsigned long l = 8;
   unsigned short m = 9;
 };
-} // namespace
+class TestingRegistry : public Registry {
+public:
+  TestingRegistry();
+};
+
+template  bool Equals(T LHS, T RHS) {
+  return std::fabs(RHS - LHS) < std::numeric_limits::epsilon();
+}
+
+static Serializer g_serializer;
+static TestingRegistry g_registry;
+
+#define SB_RECORD_CONSTRUCTOR(Class, Signature, ...)   \
+  lldb_private::repro::Recorder sb_recorder(g_serializer, g_registry,  \
+LLVM_PRETTY_FUNCTION); \
+  sb_recorder.Record(&lldb_private::repro::construct::doit,   \
+ __VA_ARGS__); \
+  sb_recorder.RecordResult(this);
+
+#define SB_RECORD_CONSTRUCTOR_NO_ARGS(Class)   \
+  lldb_private::repro::Recorder sb_recorder(g_serializer, g_registry,  \
+LLVM_PRETTY_FUNCTION); \
+  sb_recorder.Record(&lldb_private::repro::construct::doit);  \
+  sb_recorder.RecordResult(this);
+
+#define SB_RECORD_METHOD(Result, Class, Method, Signature, ...)\
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record(&lldb_private::repro::invoke::method<&Class::Method>::doit,  \
+  this, __VA_ARGS__);
+
+#define SB_RECORD_METHOD_CONST(Result, Class, Method, Signature, ...)  \
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record( \
+  &lldb_private::repro::invoke::method_const<&Class::Method>::doit,  \
+  this, __VA_ARGS__);
+
+#define SB_RECORD_METHOD_NO_ARGS(Result, Class, Method)\
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record(&lldb_private::repro::invoke::method<&Class::Method>::doit,  \
+  this);
+
+#define SB_RECORD_METHOD_CONST_NO_ARGS(Result, Class, Method)  \
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record( \
+  &lldb_private::repro::invoke::method_const<&Class::Method>::doit,  \
+  this);
+
+#define SB_RECORD_STATIC_METHOD(Result, Class, Method, Signature, ...) \
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record(static_cast(&Class::Method),\
+  __VA_ARGS__);
+
+#define SB_RECORD_STATIC_METHOD_NO_ARGS(Result, Class, Method) \
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record(static_cast(&Class::Method));
+
+#define SB_RECORD_RESULT(Result)   \
+  sb_recorder ? sb_recorder->RecordResult(Result) : Result;
+
+class InstrumentedFoo {
+public:
+  InstrumentedFoo();
+  void A(int a);
+  void B(int &b) const;
+  int C(float *c);
+  int D(const char *d) const;
+  static v

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 185208.
JDevlieghere added a comment.

Add context


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

https://reviews.llvm.org/D56322

Files:
  include/lldb/API/SBReproducer.h
  include/lldb/Utility/ReproducerInstrumentation.h
  source/API/CMakeLists.txt
  source/API/SBReproducer.cpp
  source/API/SBReproducerPrivate.h
  source/Utility/ReproducerInstrumentation.cpp
  tools/driver/Driver.cpp
  unittests/Utility/ReproducerInstrumentationTest.cpp

Index: unittests/Utility/ReproducerInstrumentationTest.cpp
===
--- unittests/Utility/ReproducerInstrumentationTest.cpp
+++ unittests/Utility/ReproducerInstrumentationTest.cpp
@@ -9,12 +9,24 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+#include 
+#include 
+
 #include "lldb/Utility/ReproducerInstrumentation.h"
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
 
-namespace {
+#define SB_REGISTER_CONSTRUCTOR(Class, Signature)  \
+  Register(&construct::doit)
+#define SB_REGISTER_METHOD(Result, Class, Method, Signature)   \
+  Register(&invoke::method<&Class::Method>::doit)
+#define SB_REGISTER_METHOD_CONST(Result, Class, Method, Signature) \
+  Register(&invoke::method_const<&Class::Method>::doit)
+#define SB_REGISTER_STATIC_METHOD(Result, Class, Method, Signature)\
+  Register(static_cast(&Class::Method))
+
 struct Foo {
   int m = 1;
 };
@@ -40,7 +52,166 @@
   unsigned long l = 8;
   unsigned short m = 9;
 };
-} // namespace
+class TestingRegistry : public Registry {
+public:
+  TestingRegistry();
+};
+
+template  bool Equals(T LHS, T RHS) {
+  return std::fabs(RHS - LHS) < std::numeric_limits::epsilon();
+}
+
+static Serializer g_serializer;
+static TestingRegistry g_registry;
+
+#define SB_RECORD_CONSTRUCTOR(Class, Signature, ...)   \
+  lldb_private::repro::Recorder sb_recorder(g_serializer, g_registry,  \
+LLVM_PRETTY_FUNCTION); \
+  sb_recorder.Record(&lldb_private::repro::construct::doit,   \
+ __VA_ARGS__); \
+  sb_recorder.RecordResult(this);
+
+#define SB_RECORD_CONSTRUCTOR_NO_ARGS(Class)   \
+  lldb_private::repro::Recorder sb_recorder(g_serializer, g_registry,  \
+LLVM_PRETTY_FUNCTION); \
+  sb_recorder.Record(&lldb_private::repro::construct::doit);  \
+  sb_recorder.RecordResult(this);
+
+#define SB_RECORD_METHOD(Result, Class, Method, Signature, ...)\
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record(&lldb_private::repro::invoke::method<&Class::Method>::doit,  \
+  this, __VA_ARGS__);
+
+#define SB_RECORD_METHOD_CONST(Result, Class, Method, Signature, ...)  \
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record( \
+  &lldb_private::repro::invoke::method_const<&Class::Method>::doit,  \
+  this, __VA_ARGS__);
+
+#define SB_RECORD_METHOD_NO_ARGS(Result, Class, Method)\
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record(&lldb_private::repro::invoke::method<&Class::Method>::doit,  \
+  this);
+
+#define SB_RECORD_METHOD_CONST_NO_ARGS(Result, Class, Method)  \
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record( \
+  &lldb_private::repro::invoke::method_const<&Class::Method>::doit,  \
+  this);
+
+#define SB_RECORD_STATIC_METHOD(Result, Class, Method, Signature, ...) \
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record(static_cast(&Class::Method),\
+  __VA_ARGS__);
+
+#define SB_RECORD_STATIC_METHOD_NO_ARGS(Result, Class, Method) \
+  llvm::Optional sb_recorder;   \
+  sb_recorder.emplace(g_serializer, g_registry, LLVM_PRETTY_FUNCTION); \
+  sb_recorder->Record(static_cast(&Class::Method));
+
+#define SB_RECORD_RESULT(Result)   \
+  sb_recorder ? sb_recorder->RecordResult(Result) : Result;
+
+class InstrumentedFoo {
+public:
+  InstrumentedFoo();
+  void A(int a);
+  void B(int &b) const;
+  int C(float *c)

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

(I plan to add another test that deals with returning objects)


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

https://reviews.llvm.org/D56322



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