date:20190108

[Lldb-commits] [PATCH] D56233: [lldb-server] Add initial support for building lldb-server on Windows

2019-01-08 Thread Pavel Labath via Phabricator via lldb-commits

labath added inline comments.



Comment at: include/lldb/Host/windows/PosixApi.h:78-79
 
+#define WNOHANG 1
+#define WUNTRACED 2
+

zturner wrote:
> krytarowski wrote:
> > I think that these symbols should not be leaked here in the first place.
> In general we should avoid mocking posix APIs for Windows.  If something is 
> currently implemented in terms of a Posix API that doesn't exist on Windows, 
> we should provide an implementation of that function with an entirely new 
> name and platform-agnostic set of parameters whose interface need not 
> resemble the posix interface in any way..  
> 
> For example, I see this is used in `lldb-platform.cpp` when we call 
> `waitpid()`.  Instead, we could add something like `void 
> WaitForAllChildProcessesToExit();` and then just implement that function on 
> posix and Windows completely separately.  This is more flexible and portable 
> than trying to fit Windows semantics into a posix api, which doesn't always 
> work.
For the record, the intention of that code in lldb-platform is 
`reapAllChildProcessesThatHaveAlreadyExited()`



Comment at: include/lldb/Host/windows/PosixApi.h:108-111
+inline pid_t waitpid(pid_t pid, int *status, int options) {
+  // To be implemented.
+  return pid_t(-1);
+}

zturner wrote:
> labath wrote:
> > What are your plans for this?
> > 
> > I assume you needed this for the fork-server in lldb-platform.cpp. I think 
> > the best way to address that would be to change the code from spawning a 
> > separate process for each connection to having a new thread for each 
> > connection. That should be much more portable and will avoid the need to 
> > mock posix apis on windows. I can help you with that if you run into any 
> > problems there.
> Are you ok with the behavioral change?  What if one connection crashes?
> 
> Assuming we wanted to implement this behavior on Windows, the way to do it 
> would be to create all child processes in a single "job" (aka process group), 
> as that is the best way to get notifications for when all childs have 
> terminated.
Yes, I would be fine with that (in fact I wanted to do that at some point 
anyway). The reason the fork server was added was to enable parallel running of 
remote tests (before that we only ever handled one concurrent connection). Also 
the platform mode of llgs doesn't do anything fancy. It's mostly just about 
copying files around and launching gdb-remote processes (so the real debugging 
still happens in a separate process), so the crashing surface is pretty limited.

BTW, the only reason we call waitpid there is that on unix, you *have* to do 
that or you will die of zombie infestation. So, if on windows you can create 
fire-and-forget child processes, then this whole code is unnecessary there.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56233



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

[Lldb-commits] [PATCH] D56232: [lldb-server] Add remote platform capabilities for Windows

2019-01-08 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

In D56232#1348514 , @zturner wrote:

> Is that even necessary?  If a platform is not remote aware, `IsHost()` will 
> always just return `true` by definition.  So we could put all of this in the 
> existing `Platform` base class.

I remember looking at this a while a go and concluding against it, but i'm not 
sure if it was impossible of just I didn't like the result.

The issue here is that PlatformWindows and PlatformPosix already have a 
m_remote_platform member (which normally is an instance of 
PlatformRemoteGDBServer). To move the common class into the base one, we'd need 
to move this member too. That would mean that any platform has a "remote" 
member, even those that already are "remote". That sounds a bit weird.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56232

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

[Lldb-commits] [PATCH] D56400: [CMake] Some cleanup around test preparations

2019-01-08 Thread Pavel Labath via Phabricator via lldb-commits

labath added a reviewer: stella.stamenova.
labath added a comment.

This looks fine to me. I recommend including Stella in changes relating to 
this, as I recall some subtleties here wrt. multi-config (e.g., MSVC) 
generators.




Comment at: CMakeLists.txt:43
 if(LLDB_INCLUDE_TESTS)
+  # FIXME: LLDB_TEST_CXX_COMPILER is unused.
+  # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build 
directory and not to LLVM's!

JDevlieghere wrote:
> Why is this unused? Are we only using `LLDB_TEST_C_COMPILER` and if so, why?
I think that's because this only gets passed to dotest, that it has it's own 
logic for deducing the C++ compiler name.


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

https://reviews.llvm.org/D56400



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

[Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

2019-01-08 Thread Pavel Labath via Phabricator via lldb-commits

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

The lldb-test change itself looks fine. I don't feel like I know the full 
impact of the proposed follow-up changes (but they sounds reasonable).




Comment at: lldb/lit/SymbolFile/PDB/enums-layout.test:3
 RUN: %build --compiler=msvc --arch=32 --nodefaultlib 
--output=%T/SimpleTypesTest.cpp.enums.exe %S/Inputs/SimpleTypesTest.cpp
-RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck %s
+RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck 
--check-prefix=ENUM %s
+RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck 
--check-prefix=ENUM_CONST %s

I guess this is because the order in the two plugins ends up being different?


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

https://reviews.llvm.org/D56418



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

Re: [Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

2019-01-08 Thread Zachary Turner via lldb-commits

The same plugin is being used in both cases, the patch only touches the
nonnative PDB reader. The followup changes to the plugin itself are
necessary because the results were not identical (another reason i want to
de-support ParseDeclsForContext at global acope - having 2 implementations
of the same thing means they will differ slightly).

In this case it happened that the ParseAllDebugSymbolsPath reconstructed
both the lldb type hierarchy and the clang ast hierarchy, but the
ParseAllDebugSymbols path missed a few things in the clang ast hierarchy )

The changes to the test that you see are because the two paths — even
though they’re still in the same plugin — result in things being
constructed in a different order
On Tue, Jan 8, 2019 at 2:38 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:

> labath accepted this revision.
> labath added a comment.
> This revision is now accepted and ready to land.
>
> The lldb-test change itself looks fine. I don't feel like I know the full
> impact of the proposed follow-up changes (but they sounds reasonable).
>
>
>
> 
> Comment at: lldb/lit/SymbolFile/PDB/enums-layout.test:3
>  RUN: %build --compiler=msvc --arch=32 --nodefaultlib
> --output=%T/SimpleTypesTest.cpp.enums.exe %S/Inputs/SimpleTypesTest.cpp
> -RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck %s
> +RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck
> --check-prefix=ENUM %s
> +RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck
> --check-prefix=ENUM_CONST %s
> 
> I guess this is because the order in the two plugins ends up being
> different?
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D56418/new/
>
> https://reviews.llvm.org/D56418
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D56400: [CMake] Some cleanup around test preparations

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz added inline comments.



Comment at: CMakeLists.txt:43
 if(LLDB_INCLUDE_TESTS)
+  # FIXME: LLDB_TEST_CXX_COMPILER is unused.
+  # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build 
directory and not to LLVM's!

labath wrote:
> JDevlieghere wrote:
> > Why is this unused? Are we only using `LLDB_TEST_C_COMPILER` and if so, why?
> I think that's because this only gets passed to dotest, that it has it's own 
> logic for deducing the C++ compiler name.
Yes, we only ever pass on `LLDB_TEST_C_COMPILER` and it should be sufficient.
https://github.com/llvm-mirror/lldb/blob/2b03c2512ebf1999e330e85d60b7f93703483485/test/CMakeLists.txt#L53

https://lldb.llvm.org/build.html reads: //Note that MSVC is not supported here, 
it must be a path to a clang executable.//
As clang == clang++ ⇒ q.e.d :)

I think what we really want here is a `LLDB_TEST_COMPILER` setting, but this 
means: update docs and build bots, potentially phase out the setting? I am not 
sure about their popularity.



Comment at: CMakeLists.txt:44
+  # FIXME: LLDB_TEST_CXX_COMPILER is unused.
+  # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build 
directory and not to LLVM's!
+  # FIXME: LLDB_TEST_USE_CUSTOM_C/CXX_COMPILER options don't work as intended!

JDevlieghere wrote:
> sgraenitz wrote:
> > This was here already when the file was created:
> > https://github.com/llvm-mirror/lldb/blob/98c187f2613fd8e2f5cdf5f8f35503a102a54f11/cmake/modules/LLDBStandalone.cmake#L67
> > 
> > Apparently the below default paths for dsymutil, FileCheck and clang never 
> > worked in standalone builds..
> Instead of this FIXME, should we explicitly unset this variable to prevent 
> mistakes in the future? Or is this fixable?
Good point. Not sure what the consequences would be. I will try and see what 
happens.


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

https://reviews.llvm.org/D56400



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

Re: [Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

2019-01-08 Thread Pavel Labath via lldb-commits


On 08/01/2019 11:45, Zachary Turner wrote:
The same plugin is being used in both cases, the patch only touches the 
nonnative PDB reader. The followup changes to the plugin itself are 
necessary because the results were not identical (another reason i want 
to de-support ParseDeclsForContext at global acope - having 2 
implementations of the same thing means they will differ slightly).


In this case it happened that the ParseAllDebugSymbolsPath reconstructed 
both the lldb type hierarchy and the clang ast hierarchy, but the 
ParseAllDebugSymbols path missed a few things in the clang ast hierarchy )


The changes to the test that you see are because the two paths — even 
though they’re still in the same plugin — result in things being 
constructed in a different order


Ok, that makes sense. Thanks.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r350617 - ProcessLaunchInfo: Remove Target reference

2019-01-08 Thread Pavel Labath via lldb-commits

Author: labath
Date: Tue Jan  8 03:55:19 2019
New Revision: 350617

URL: http://llvm.org/viewvc/llvm-project?rev=350617&view=rev
Log:
ProcessLaunchInfo: Remove Target reference

Summary:
The target was being used in FinalizeFileActions to provide default
values for stdin/out/err. Also, most of the logic of this function was
very specific to how the lldb's Target class wants to launch processes,
so I, move it to Target::FinalizeFileActions, inverting the dependency.
The only piece of logic that was useful elsewhere (lldb-server) was the
part which sets up a pty and relevant file actions. I've kept this part
as ProcessLaunchInfo::SetUpPtyRedirection.

This makes ProcessLaunchInfo independent of any high-level lldb constructs.

Reviewers: zturner, jingham, teemperor

Subscribers: lldb-commits

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

Modified:
lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
lldb/trunk/include/lldb/Target/Target.h

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

Modified: lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h?rev=350617&r1=350616&r2=350617&view=diff
==
--- lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h (original)
+++ lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h Tue Jan  8 03:55:19 2019
@@ -52,7 +52,10 @@ public:
 
   bool AppendSuppressFileAction(int fd, bool read, bool write);
 
-  void FinalizeFileActions(Target *target, bool default_to_use_pty);
+  // Redirect stdin/stdout/stderr to a pty, if no action for the respective 
file
+  // descriptor is specified. (So if stdin and stdout already have file 
actions,
+  // but stderr doesn't, then only stderr will be redirected to a pty.)
+  llvm::Error SetUpPtyRedirection();
 
   size_t GetNumFileActions() const { return m_file_actions.size(); }
 

Modified: lldb/trunk/include/lldb/Target/Target.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Target.h?rev=350617&r1=350616&r2=350617&view=diff
==
--- lldb/trunk/include/lldb/Target/Target.h (original)
+++ lldb/trunk/include/lldb/Target/Target.h Tue Jan  8 03:55:19 2019
@@ -1354,6 +1354,8 @@ private:
 
   void AddBreakpoint(lldb::BreakpointSP breakpoint_sp, bool internal);
 
+  void FinalizeFileActions(ProcessLaunchInfo &info);
+
   DISALLOW_COPY_AND_ASSIGN(Target);
 };
 

Modified: 
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp?rev=350617&r1=350616&r2=350617&view=diff
==
--- 
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
 (original)
+++ 
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
 Tue Jan  8 03:55:19 2019
@@ -218,8 +218,10 @@ Status GDBRemoteCommunicationServerLLGS:
   m_process_launch_info.SetLaunchInSeparateProcessGroup(true);
   m_process_launch_info.GetFlags().Set(eLaunchFlagDebug);
 
-  const bool default_to_use_pty = true;
-  m_process_launch_info.FinalizeFileActions(nullptr, default_to_use_pty);
+  if (should_forward_stdio) {
+if (llvm::Error Err = m_process_launch_info.SetUpPtyRedirection())
+  return Status(std::move(Err));
+  }
 
   {
 std::lock_guard guard(m_debugged_process_mutex);

Modified: lldb/trunk/source/Target/ProcessLaunchInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/ProcessLaunchInfo.cpp?rev=350617&r1=350616&r2=350617&view=diff
==
--- lldb/trunk/source/Target/ProcessLaunchInfo.cpp (original)
+++ lldb/trunk/source/Target/ProcessLaunchInfo.cpp Tue Jan  8 03:55:19 2019
@@ -14,7 +14,6 @@
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/FileAction.h"
 #include "lldb/Target/ProcessLaunchInfo.h"
-#include "lldb/Target/Target.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
@@ -206,123 +205,38 @@ void ProcessLaunchInfo::SetDetachOnError
 m_flags.Clear(lldb::eLaunchFlagDetachOnError);
 }
 
-void ProcessLaunchInfo::FinalizeFileActions(Target *target,
-bool default_to_use_pty) {
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
-
-  // If nothing for stdin or stdout or stderr was specified, then check the
-  // process for any default settings that were set with "settings set"
-  if (GetFileActionForFD(STDIN_FILENO) == nullptr ||
-  GetFileActionForFD(STDOUT_FILENO) == nullptr ||
-  GetFileActionForFD(STDERR_FILENO)

[Lldb-commits] [PATCH] D56196: ProcessLaunchInfo: Remove Target reference

2019-01-08 Thread Pavel Labath via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL350617: ProcessLaunchInfo: Remove Target reference (authored 
by labath, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56196

Files:
  lldb/trunk/include/lldb/Target/ProcessLaunchInfo.h
  lldb/trunk/include/lldb/Target/Target.h
  
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/trunk/source/Target/ProcessLaunchInfo.cpp
  lldb/trunk/source/Target/Target.cpp

Index: lldb/trunk/source/Target/Target.cpp
===
--- lldb/trunk/source/Target/Target.cpp
+++ lldb/trunk/source/Target/Target.cpp
@@ -2834,18 +2834,7 @@
 
   PlatformSP platform_sp(GetPlatform());
 
-  // Finalize the file actions, and if none were given, default to opening up a
-  // pseudo terminal
-  const bool default_to_use_pty = platform_sp ? platform_sp->IsHost() : false;
-  if (log)
-log->Printf("Target::%s have platform=%s, platform_sp->IsHost()=%s, "
-"default_to_use_pty=%s",
-__FUNCTION__, platform_sp ? "true" : "false",
-platform_sp ? (platform_sp->IsHost() ? "true" : "false")
-: "n/a",
-default_to_use_pty ? "true" : "false");
-
-  launch_info.FinalizeFileActions(this, default_to_use_pty);
+  FinalizeFileActions(launch_info);
 
   if (state == eStateConnected) {
 if (launch_info.GetFlags().Test(eLaunchFlagLaunchInTTY)) {
@@ -3061,6 +3050,86 @@
   return error;
 }
 
+void Target::FinalizeFileActions(ProcessLaunchInfo &info) {
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+
+  // Finalize the file actions, and if none were given, default to opening up a
+  // pseudo terminal
+  PlatformSP platform_sp = GetPlatform();
+  const bool default_to_use_pty =
+  m_platform_sp ? m_platform_sp->IsHost() : false;
+  LLDB_LOG(
+  log,
+  "have platform={0}, platform_sp->IsHost()={1}, default_to_use_pty={2}",
+  bool(platform_sp),
+  platform_sp ? (platform_sp->IsHost() ? "true" : "false") : "n/a",
+  default_to_use_pty);
+
+  // If nothing for stdin or stdout or stderr was specified, then check the
+  // process for any default settings that were set with "settings set"
+  if (info.GetFileActionForFD(STDIN_FILENO) == nullptr ||
+  info.GetFileActionForFD(STDOUT_FILENO) == nullptr ||
+  info.GetFileActionForFD(STDERR_FILENO) == nullptr) {
+LLDB_LOG(log, "at least one of stdin/stdout/stderr was not set, evaluating "
+  "default handling");
+
+if (info.GetFlags().Test(eLaunchFlagLaunchInTTY)) {
+  // Do nothing, if we are launching in a remote terminal no file actions
+  // should be done at all.
+  return;
+}
+
+if (info.GetFlags().Test(eLaunchFlagDisableSTDIO)) {
+  LLDB_LOG(log, "eLaunchFlagDisableSTDIO set, adding suppression action "
+"for stdin, stdout and stderr");
+  info.AppendSuppressFileAction(STDIN_FILENO, true, false);
+  info.AppendSuppressFileAction(STDOUT_FILENO, false, true);
+  info.AppendSuppressFileAction(STDERR_FILENO, false, true);
+} else {
+  // Check for any values that might have gotten set with any of: (lldb)
+  // settings set target.input-path (lldb) settings set target.output-path
+  // (lldb) settings set target.error-path
+  FileSpec in_file_spec;
+  FileSpec out_file_spec;
+  FileSpec err_file_spec;
+  // Only override with the target settings if we don't already have an
+  // action for in, out or error
+  if (info.GetFileActionForFD(STDIN_FILENO) == nullptr)
+in_file_spec = GetStandardInputPath();
+  if (info.GetFileActionForFD(STDOUT_FILENO) == nullptr)
+out_file_spec = GetStandardOutputPath();
+  if (info.GetFileActionForFD(STDERR_FILENO) == nullptr)
+err_file_spec = GetStandardErrorPath();
+
+  LLDB_LOG(log, "target stdin='{0}', target stdout='{1}', stderr='{1}'",
+   in_file_spec, out_file_spec, err_file_spec);
+
+  if (in_file_spec) {
+info.AppendOpenFileAction(STDIN_FILENO, in_file_spec, true, false);
+LLDB_LOG(log, "appended stdin open file action for {0}", in_file_spec);
+  }
+
+  if (out_file_spec) {
+info.AppendOpenFileAction(STDOUT_FILENO, out_file_spec, false, true);
+LLDB_LOG(log, "appended stdout open file action for {0}",
+ out_file_spec);
+  }
+
+  if (err_file_spec) {
+info.AppendOpenFileAction(STDERR_FILENO, err_file_spec, false, true);
+LLDB_LOG(log, "appended stderr open file action for {0}",
+ err_file_spec);
+  }
+
+  if (default_to_use_pty &&
+  (!in_file_spec || !out_file_spec || !err_file_spec)) {
+llvm::Error Err = info.SetUpPtyRedirection

[Lldb-commits] [PATCH] D55998: ELF: create "container" sections from PT_LOAD segments

2019-01-08 Thread Pavel Labath via Phabricator via lldb-commits

labath added a comment.

Greg, what do you think about the current version of this patch.

Joerg, I propose to check the patch in in the current form, as this solves 
existing issues in the lldb's ELF representation and improves the overall 
consistency of lldb's representation of object files. I'm not opposed to 
changing things again if we find a use case that this breaks, but I can do that 
only once I know what that use case is (and that might involve making changes 
to the generic representation of modules in lldb).


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

https://reviews.llvm.org/D55998



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

[Lldb-commits] [PATCH] D56400: [CMake] Some cleanup around test preparations

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz updated this revision to Diff 180643.
sgraenitz added a comment.

Act on FIXME's in subsequent commits and remove comments here


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

https://reviews.llvm.org/D56400

Files:
  CMakeLists.txt
  lit/CMakeLists.txt


Index: lit/CMakeLists.txt
===
--- lit/CMakeLists.txt
+++ lit/CMakeLists.txt
@@ -12,11 +12,11 @@
 endif()
 
 if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER 
${LLDB_TEST_C_COMPILER})
+  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER 
"${LLDB_TEST_C_COMPILER}")
 endif ()
 
 if (NOT LLDB_TEST_USE_CUSTOM_CXX_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_CXX_COMPILER 
${LLDB_TEST_CXX_COMPILER})
+  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_CXX_COMPILER 
"${LLDB_TEST_CXX_COMPILER}")
 endif ()
 
 get_property(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -35,43 +35,29 @@
 add_subdirectory(source)
 add_subdirectory(tools)
 
-option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests."
-  ${LLVM_INCLUDE_TESTS})
+option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests." 
${LLVM_INCLUDE_TESTS})
 option(LLDB_TEST_USE_CUSTOM_C_COMPILER "Use the C compiler provided via 
LLDB_TEST_C_COMPILER for building test inferiors (instead of the just-built 
compiler). Defaults to OFF." OFF)
 option(LLDB_TEST_USE_CUSTOM_CXX_COMPILER "Use the C++ compiler provided via 
LLDB_TEST_CXX_COMPILER for building test inferiors (instead of the just-built 
compiler). Defaults to OFF." OFF)
-if(LLDB_INCLUDE_TESTS)
 
-  # Set the path to the default lldb test executable. Make the path relative to
-  # LLVM_RUNTIME_OUTPUT_INTDIR: this will be correct even when LLVM and LLDB
-  # have separate binary directories.
+if(LLDB_INCLUDE_TESTS)
+  # Set the path to the default lldb test executable.
   set(LLDB_DEFAULT_TEST_EXECUTABLE 
"${LLVM_RUNTIME_OUTPUT_INTDIR}/lldb${CMAKE_EXECUTABLE_SUFFIX}")
 
-  # Set the paths to default llvm tools. Make these paths relative to the LLVM
-  # binary directory.
+  # Set the paths to default llvm tools.
   set(LLDB_DEFAULT_TEST_DSYMUTIL 
"${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/dsymutil${CMAKE_EXECUTABLE_SUFFIX}")
   set(LLDB_DEFAULT_TEST_FILECHECK 
"${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/FileCheck${CMAKE_EXECUTABLE_SUFFIX}")
-
-  if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER AND TARGET clang)
-set(LLDB_DEFAULT_TEST_C_COMPILER 
"${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/clang${CMAKE_EXECUTABLE_SUFFIX}")
-  else()
-set(LLDB_DEFAULT_TEST_C_COMPILER "")
-  endif()
-
-  if (NOT LLDB_TEST_USE_CUSTOM_CXX_COMPILER AND TARGET clang)
-set(LLDB_DEFAULT_TEST_CXX_COMPILER 
"${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/clang++${CMAKE_EXECUTABLE_SUFFIX}")
-  else()
-set(LLDB_DEFAULT_TEST_CXX_COMPILER "")
+  if(TARGET clang)
+set(LLDB_DEFAULT_TEST_COMPILER 
"${LLVM_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin/clang${CMAKE_EXECUTABLE_SUFFIX}")
   endif()
 
   set(LLDB_TEST_EXECUTABLE "${LLDB_DEFAULT_TEST_EXECUTABLE}" CACHE PATH "lldb 
executable used for testing")
-  set(LLDB_TEST_C_COMPILER "${LLDB_DEFAULT_TEST_C_COMPILER}" CACHE PATH "C 
Compiler to use for building LLDB test inferiors")
-  set(LLDB_TEST_CXX_COMPILER "${LLDB_DEFAULT_TEST_CXX_COMPILER}" CACHE PATH 
"C++ Compiler to use for building LLDB test inferiors")
+  set(LLDB_TEST_C_COMPILER "${LLDB_DEFAULT_TEST_COMPILER}" CACHE PATH "C 
Compiler to use for building LLDB test inferiors")
+  set(LLDB_TEST_CXX_COMPILER "${LLDB_DEFAULT_TEST_COMPILER}" CACHE PATH "C++ 
Compiler to use for building LLDB test inferiors")
   set(LLDB_TEST_DSYMUTIL "${LLDB_DEFAULT_TEST_DSYMUTIL}" CACHE PATH "dsymutil 
used for generating dSYM bundles")
   set(LLDB_TEST_FILECHECK "${LLDB_DEFAULT_TEST_FILECHECK}" CACHE PATH 
"FileCheck used for testing purposes")
 
-  if (("${LLDB_TEST_C_COMPILER}" STREQUAL "") OR
-  ("${LLDB_TEST_CXX_COMPILER}" STREQUAL ""))
-message(FATAL_ERROR "LLDB test compilers not specified.  Tests will not 
run")
+  if((NOT LLDB_TEST_C_COMPILER) OR (NOT LLDB_TEST_CXX_COMPILER))
+message(WARNING "LLDB test compilers not specified. Tests will not run.")
   endif()
 
   set(LLDB_TEST_DEPS lldb)


Index: lit/CMakeLists.txt
===
--- lit/CMakeLists.txt
+++ lit/CMakeLists.txt
@@ -12,11 +12,11 @@
 endif()
 
 if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER ${LLDB_TEST_C_COMPILER})
+  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER "${LLDB_TEST_C_COMPILER}")
 endif ()
 
 if (NOT LLDB_TEST_USE_CUSTOM_CXX_COMPILER)
-  string(REPLACE ${CMAKE_CF

[Lldb-commits] [PATCH] D56440: [CMake] Phase out LLDB_TEST_C/CXX_COMPILER in favor of single LLDB_TEST_COMPILER

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz created this revision.
sgraenitz added reviewers: labath, zturner, ted, JDevlieghere, stella.stamenova.
Herald added a subscriber: mgorny.

Following up from D56400 :

- add `LLDB_TEST_COMPILER` setting, while still supporting 
`LLDB_TEST_C/CXX_COMPILER`
- remove `LLDB_TEST_USE_CUSTOM_C/CXX_COMPILER`: overriding `LLDB_TEST_COMPILER` 
alone is sufficient
- use `LLDB_TEST_COMPILER_IS_DEFAULT` to determine whether or not to replace 
configuration names
- update documentation to reflect changes

As we import targets like `clang` and `dsymutil` anyway, we might use generator 
expressions to fetch paths of the required tools.
The problem is not that generator expressions don't work in the multi-config 
case. We just need a generation-time step for each lit.site.cfg, something like 
this:
https://github.com/llvm-mirror/lldb/blob/5febeff671748655213b74bbd71e7d2efc1a9efe/test/CMakeLists.txt#L169


https://reviews.llvm.org/D56440

Files:
  CMakeLists.txt
  lit/CMakeLists.txt
  test/CMakeLists.txt
  www/build.html
  www/test.html

Index: www/test.html
===
--- www/test.html
+++ www/test.html
@@ -39,9 +39,7 @@
   The easiest way to run the LLDB test suite is to use the check-lldb build
   target. By default, the check-lldb target builds the test programs with
   the same compiler that was used to build LLDB. To build the tests with a different
-  compiler, you can set the LLDB_TEST_C_COMPILER or the LLDB_TEST_CXX_COMPILER CMake variables.
-  These variables are ignored unless the respective LLDB_TEST_USE_CUSTOM_C_COMPILER and
-  LLDB_TEST_USE_CUSTOM_CXX_COMPILER are set to ON.
+  compiler, you can set the LLDB_TEST_COMPILER CMake variable.
   It is possible to customize the architecture of the test binaries and compiler used by appending -A
   and -C options respectively to the CMake variable LLDB_TEST_USER_ARGS. For
   example, to test LLDB against 32-bit binaries
Index: www/build.html
===
--- www/build.html
+++ www/build.html
@@ -116,15 +116,14 @@
 the PYTHONHOME environment variable if it is specified).
   
   
-LLDB_TEST_C_COMPILER or LLDB_TEST_CXX_COMPILER: The test suite needs to be able to find a copy of clang.exe
+LLDB_TEST_COMPILER: The test suite needs to be able to find a copy of clang.exe
 that it can use to compile inferior programs.  Note that MSVC is not supported here, it must be a path to a
 clang executable.  Note that using a release clang.exe is strongly recommended here, as it will make the test suite run much faster.
-This can be a path to any recent clang.exe, including one you built yourself.  These variables are ignored unless the respective
-LLDB_TEST_USE_CUSTOM_C_COMPILER and LLDB_TEST_USE_CUSTOM_CXX_COMPILER are set to ON.
+This can be a path to any recent clang.exe, including one you built yourself.
   
 
 Sample command line:
-cmake -G Ninja -DLLDB_TEST_DEBUG_TEST_CRASHES=1 -DPYTHON_HOME=C:\Python35 -DLLDB_TEST_USE_CUSTOM_C_COMPILER=ON -DLLDB_TEST_C_COMPILER=d:\src\llvmbuild\ninja_release\bin\clang.exe ..\..\llvm
+cmake -G Ninja -DLLDB_TEST_DEBUG_TEST_CRASHES=1 -DPYTHON_HOME=C:\Python35 -DLLDB_TEST_COMPILER=d:\src\llvmbuild\ninja_release\bin\clang.exe ..\..\llvm
 Working with both Ninja and MSVC
 
   Compiling with ninja is both faster and simpler than compiling with MSVC, but chances are you still want
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -50,7 +50,7 @@
   --executable ${LLDB_TEST_EXECUTABLE}
   --dsymutil ${LLDB_TEST_DSYMUTIL}
   --filecheck ${LLDB_TEST_FILECHECK}
-  -C ${LLDB_TEST_C_COMPILER}
+  -C ${LLDB_TEST_COMPILER}
   )
 
 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
Index: lit/CMakeLists.txt
===
--- lit/CMakeLists.txt
+++ lit/CMakeLists.txt
@@ -11,14 +11,6 @@
   set(LLDB_IS_64_BITS 1)
 endif()
 
-if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER "${LLDB_TEST_C_COMPILER}")
-endif ()
-
-if (NOT LLDB_TEST_USE_CUSTOM_CXX_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_CXX_COMPILER "${LLDB_TEST_CXX_COMPILER}")
-endif ()
-
 get_property(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)
 
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_LIBS_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
Index: CMakeLists.txt
===
--- CMakeL

[Lldb-commits] [PATCH] D56440: [CMake] Phase out LLDB_TEST_C/CXX_COMPILER in favor of single LLDB_TEST_COMPILER

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz updated this revision to Diff 180662.
sgraenitz added a comment.

In test/CMakeLists.txt pass on LLDB_TEST_COMPILER_USED instead of 
LLDB_TEST_COMPILER


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

https://reviews.llvm.org/D56440

Files:
  CMakeLists.txt
  lit/CMakeLists.txt
  test/CMakeLists.txt
  www/build.html
  www/test.html

Index: www/test.html
===
--- www/test.html
+++ www/test.html
@@ -39,9 +39,7 @@
   The easiest way to run the LLDB test suite is to use the check-lldb build
   target. By default, the check-lldb target builds the test programs with
   the same compiler that was used to build LLDB. To build the tests with a different
-  compiler, you can set the LLDB_TEST_C_COMPILER or the LLDB_TEST_CXX_COMPILER CMake variables.
-  These variables are ignored unless the respective LLDB_TEST_USE_CUSTOM_C_COMPILER and
-  LLDB_TEST_USE_CUSTOM_CXX_COMPILER are set to ON.
+  compiler, you can set the LLDB_TEST_COMPILER CMake variable.
   It is possible to customize the architecture of the test binaries and compiler used by appending -A
   and -C options respectively to the CMake variable LLDB_TEST_USER_ARGS. For
   example, to test LLDB against 32-bit binaries
Index: www/build.html
===
--- www/build.html
+++ www/build.html
@@ -116,15 +116,14 @@
 the PYTHONHOME environment variable if it is specified).
   
   
-LLDB_TEST_C_COMPILER or LLDB_TEST_CXX_COMPILER: The test suite needs to be able to find a copy of clang.exe
+LLDB_TEST_COMPILER: The test suite needs to be able to find a copy of clang.exe
 that it can use to compile inferior programs.  Note that MSVC is not supported here, it must be a path to a
 clang executable.  Note that using a release clang.exe is strongly recommended here, as it will make the test suite run much faster.
-This can be a path to any recent clang.exe, including one you built yourself.  These variables are ignored unless the respective
-LLDB_TEST_USE_CUSTOM_C_COMPILER and LLDB_TEST_USE_CUSTOM_CXX_COMPILER are set to ON.
+This can be a path to any recent clang.exe, including one you built yourself.
   
 
 Sample command line:
-cmake -G Ninja -DLLDB_TEST_DEBUG_TEST_CRASHES=1 -DPYTHON_HOME=C:\Python35 -DLLDB_TEST_USE_CUSTOM_C_COMPILER=ON -DLLDB_TEST_C_COMPILER=d:\src\llvmbuild\ninja_release\bin\clang.exe ..\..\llvm
+cmake -G Ninja -DLLDB_TEST_DEBUG_TEST_CRASHES=1 -DPYTHON_HOME=C:\Python35 -DLLDB_TEST_COMPILER=d:\src\llvmbuild\ninja_release\bin\clang.exe ..\..\llvm
 Working with both Ninja and MSVC
 
   Compiling with ninja is both faster and simpler than compiling with MSVC, but chances are you still want
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -50,7 +50,7 @@
   --executable ${LLDB_TEST_EXECUTABLE}
   --dsymutil ${LLDB_TEST_DSYMUTIL}
   --filecheck ${LLDB_TEST_FILECHECK}
-  -C ${LLDB_TEST_C_COMPILER}
+  -C ${LLDB_TEST_COMPILER_USED}
   )
 
 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
Index: lit/CMakeLists.txt
===
--- lit/CMakeLists.txt
+++ lit/CMakeLists.txt
@@ -11,14 +11,6 @@
   set(LLDB_IS_64_BITS 1)
 endif()
 
-if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER "${LLDB_TEST_C_COMPILER}")
-endif ()
-
-if (NOT LLDB_TEST_USE_CUSTOM_CXX_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_CXX_COMPILER "${LLDB_TEST_CXX_COMPILER}")
-endif ()
-
 get_property(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)
 
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_LIBS_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -36,9 +36,6 @@
 add_subdirectory(tools)
 
 option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests." ${LLVM_INCLUDE_TESTS})
-option(LLDB_TEST_USE_CUSTOM_C_COMPILER "Use the C compiler provided via LLDB_TEST_C_COMPILER for building test inferiors (instead of the just-built compiler). Defaults to OFF." OFF)
-option(LLDB_TEST_USE_CUSTOM_CXX_COMPILER "Use the C++ compiler provided via LLDB_TEST_CXX_COMPILER for building test inferiors (instead of the just-built compiler). Defaults to OFF." OFF)
-
 if(LLDB_INCLUDE_TESTS)
   # Set the path to the default lldb test executable.
   set(LLDB_DEFAULT_TEST_EXECUTABLE "${LLVM_RUNTIME_OUTPUT_INTDIR}/lldb${CMAKE_EXECU

[Lldb-commits] [PATCH] D56440: [CMake] Phase out LLDB_TEST_C/CXX_COMPILER in favor of single LLDB_TEST_COMPILER

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz updated this revision to Diff 180670.
sgraenitz added a comment.

Remove remove unused `LLDB_HAVE_LLD` and `ENABLE_SHARED` in lit/CMakeLists.txt


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

https://reviews.llvm.org/D56440

Files:
  CMakeLists.txt
  lit/CMakeLists.txt
  test/CMakeLists.txt
  www/build.html
  www/test.html

Index: www/test.html
===
--- www/test.html
+++ www/test.html
@@ -39,9 +39,7 @@
   The easiest way to run the LLDB test suite is to use the check-lldb build
   target. By default, the check-lldb target builds the test programs with
   the same compiler that was used to build LLDB. To build the tests with a different
-  compiler, you can set the LLDB_TEST_C_COMPILER or the LLDB_TEST_CXX_COMPILER CMake variables.
-  These variables are ignored unless the respective LLDB_TEST_USE_CUSTOM_C_COMPILER and
-  LLDB_TEST_USE_CUSTOM_CXX_COMPILER are set to ON.
+  compiler, you can set the LLDB_TEST_COMPILER CMake variable.
   It is possible to customize the architecture of the test binaries and compiler used by appending -A
   and -C options respectively to the CMake variable LLDB_TEST_USER_ARGS. For
   example, to test LLDB against 32-bit binaries
Index: www/build.html
===
--- www/build.html
+++ www/build.html
@@ -116,15 +116,14 @@
 the PYTHONHOME environment variable if it is specified).
   
   
-LLDB_TEST_C_COMPILER or LLDB_TEST_CXX_COMPILER: The test suite needs to be able to find a copy of clang.exe
+LLDB_TEST_COMPILER: The test suite needs to be able to find a copy of clang.exe
 that it can use to compile inferior programs.  Note that MSVC is not supported here, it must be a path to a
 clang executable.  Note that using a release clang.exe is strongly recommended here, as it will make the test suite run much faster.
-This can be a path to any recent clang.exe, including one you built yourself.  These variables are ignored unless the respective
-LLDB_TEST_USE_CUSTOM_C_COMPILER and LLDB_TEST_USE_CUSTOM_CXX_COMPILER are set to ON.
+This can be a path to any recent clang.exe, including one you built yourself.
   
 
 Sample command line:
-cmake -G Ninja -DLLDB_TEST_DEBUG_TEST_CRASHES=1 -DPYTHON_HOME=C:\Python35 -DLLDB_TEST_USE_CUSTOM_C_COMPILER=ON -DLLDB_TEST_C_COMPILER=d:\src\llvmbuild\ninja_release\bin\clang.exe ..\..\llvm
+cmake -G Ninja -DLLDB_TEST_DEBUG_TEST_CRASHES=1 -DPYTHON_HOME=C:\Python35 -DLLDB_TEST_COMPILER=d:\src\llvmbuild\ninja_release\bin\clang.exe ..\..\llvm
 Working with both Ninja and MSVC
 
   Compiling with ninja is both faster and simpler than compiling with MSVC, but chances are you still want
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -50,7 +50,7 @@
   --executable ${LLDB_TEST_EXECUTABLE}
   --dsymutil ${LLDB_TEST_DSYMUTIL}
   --filecheck ${LLDB_TEST_FILECHECK}
-  -C ${LLDB_TEST_C_COMPILER}
+  -C ${LLDB_TEST_COMPILER_USED}
   )
 
 if ( CMAKE_SYSTEM_NAME MATCHES "Windows" )
Index: lit/CMakeLists.txt
===
--- lit/CMakeLists.txt
+++ lit/CMakeLists.txt
@@ -11,14 +11,6 @@
   set(LLDB_IS_64_BITS 1)
 endif()
 
-if (NOT LLDB_TEST_USE_CUSTOM_C_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_C_COMPILER "${LLDB_TEST_C_COMPILER}")
-endif ()
-
-if (NOT LLDB_TEST_USE_CUSTOM_CXX_COMPILER)
-  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TEST_CXX_COMPILER "${LLDB_TEST_CXX_COMPILER}")
-endif ()
-
 get_property(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)
 
 string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_LIBS_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
@@ -38,17 +30,8 @@
 
 if(TARGET lld)
   list(APPEND LLDB_TEST_DEPS lld)
-  set(LLDB_HAVE_LLD 1)
-else()
-  set(LLDB_HAVE_LLD 0)
 endif()
 
-if(BUILD_SHARED_LIBS)
-  set(ENABLE_SHARED 1)
-else()
-  set(ENABLE_SHARED 0)
-endif(BUILD_SHARED_LIBS)
-
 # the value is not canonicalized within LLVM
 llvm_canonicalize_cmake_booleans(
   LLDB_DISABLE_PYTHON
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -36,9 +36,6 @@
 add_subdirectory(tools)
 
 option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests." ${LLVM_INCLUDE_TESTS})
-option(LLDB_TEST_USE_CUSTOM_C_COMPILER "Use the C compiler provided via LLDB_TEST_C_COMPILER for building test inferiors (instead of the just-built compiler). Defaults to OFF." OFF)
-option(LLD

[Lldb-commits] [PATCH] D56443: [CMake] In standalone builds, LLVM_BINARY_DIR should point to LLVM's binary directory

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz created this revision.
sgraenitz added reviewers: zturner, labath, clayborg, JDevlieghere, 
stella.stamenova.
Herald added a subscriber: mgorny.
Herald added a reviewer: serge-sans-paille.

In standalone builds `LLVM_BINARY_DIR` was equal to `LLDB_BINARY_DIR` so far. 
This is counterintuitive and invalidated the values of 
`LLDB_DEFAULT_TEST_DSYMUTIL/FILECHECK/COMPILER` etc.


https://reviews.llvm.org/D56443

Files:
  cmake/modules/LLDBStandalone.cmake
  lit/Suite/lit.site.cfg.in
  lit/Unit/lit.site.cfg.py.in


Index: lit/Unit/lit.site.cfg.py.in
===
--- lit/Unit/lit.site.cfg.py.in
+++ lit/Unit/lit.site.cfg.py.in
@@ -1,6 +1,6 @@
 @LIT_SITE_CFG_IN_HEADER@
 
-config.test_exec_root = "@LLVM_BINARY_DIR@" 
+config.test_exec_root = "@LLDB_BINARY_DIR@"
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
Index: lit/Suite/lit.site.cfg.in
===
--- lit/Suite/lit.site.cfg.in
+++ lit/Suite/lit.site.cfg.in
@@ -1,6 +1,6 @@
 @LIT_SITE_CFG_IN_HEADER@
 
-config.test_exec_root = "@LLVM_BINARY_DIR@"
+config.test_exec_root = "@LLDB_BINARY_DIR@"
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
Index: cmake/modules/LLDBStandalone.cmake
===
--- cmake/modules/LLDBStandalone.cmake
+++ cmake/modules/LLDBStandalone.cmake
@@ -108,10 +108,8 @@
 
   set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
 
-  set(LLVM_BINARY_DIR ${CMAKE_BINARY_DIR})
-
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  include_directories("${LLVM_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
+  include_directories("${CMAKE_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
   # Next three include directories are needed when llvm-config is located in 
build directory.
   # LLVM and Clang are assumed to be built together
   if (EXISTS "${LLVM_OBJ_ROOT}/include")


Index: lit/Unit/lit.site.cfg.py.in
===
--- lit/Unit/lit.site.cfg.py.in
+++ lit/Unit/lit.site.cfg.py.in
@@ -1,6 +1,6 @@
 @LIT_SITE_CFG_IN_HEADER@
 
-config.test_exec_root = "@LLVM_BINARY_DIR@" 
+config.test_exec_root = "@LLDB_BINARY_DIR@"
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
Index: lit/Suite/lit.site.cfg.in
===
--- lit/Suite/lit.site.cfg.in
+++ lit/Suite/lit.site.cfg.in
@@ -1,6 +1,6 @@
 @LIT_SITE_CFG_IN_HEADER@
 
-config.test_exec_root = "@LLVM_BINARY_DIR@"
+config.test_exec_root = "@LLDB_BINARY_DIR@"
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
Index: cmake/modules/LLDBStandalone.cmake
===
--- cmake/modules/LLDBStandalone.cmake
+++ cmake/modules/LLDBStandalone.cmake
@@ -108,10 +108,8 @@
 
   set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
 
-  set(LLVM_BINARY_DIR ${CMAKE_BINARY_DIR})
-
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  include_directories("${LLVM_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
+  include_directories("${CMAKE_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
   # Next three include directories are needed when llvm-config is located in build directory.
   # LLVM and Clang are assumed to be built together
   if (EXISTS "${LLVM_OBJ_ROOT}/include")
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Adrian Prantl via Phabricator via lldb-commits

aprantl added inline comments.



Comment at: source/Breakpoint/BreakpointList.cpp:18
+static void NotifyChange(BreakpointSP bp, BreakpointEventType event) {
+  if (bp->GetTarget().EventTypeHasListeners(
+  Target::eBroadcastBitBreakpointChanged))

perhaps:
- factor out bp->GetTarget()?
- pass a BerakpointSP & or Breakpoint & to avoid shared pointer traffic?




Comment at: source/Breakpoint/BreakpointList.cpp:189
+  for (const auto &bp_sp : m_breakpoints) {
 if (curr_i == i)
+  return bp_sp;

is the whole point of this loop to do something like m_breakpoints[i]?
why not `std::next(..., i)` ?



Comment at: source/Breakpoint/BreakpointList.cpp:200
   size_t curr_i = 0;
-  for (pos = m_breakpoints.begin(), curr_i = 0; pos != end; ++pos, ++curr_i) {
+  for (const auto &bp_sp : m_breakpoints) {
 if (curr_i == i)

separate commit?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56425



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

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added a comment.

Second vote from me to switch to a std::vector since we have a function get 
breakpoint at index. Other than that, looks good.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56425



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

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added inline comments.



Comment at: source/Breakpoint/BreakpointList.cpp:22
+Target::eBroadcastBitBreakpointChanged,
+new Breakpoint::BreakpointEventData(event, bp));
+}

If we don't pass a "BreakpointSP& sp", then use std::move?



Comment at: source/Breakpoint/BreakpointList.cpp:123-125
   return std::find_if(m_breakpoints.begin(),
   m_breakpoints.end(),// Search full range
   BreakpointIDMatches(break_id)); // Predicate

If m_breakpoints is a std::vector, we can keep it sorted and use 
std::lower_bound for faster lookups



Comment at: source/Breakpoint/BreakpointList.cpp:130
 BreakpointList::GetBreakpointIDConstIterator(break_id_t break_id) const {
   return std::find_if(m_breakpoints.begin(),
   m_breakpoints.end(),// Search full range

ditto



Comment at: source/Breakpoint/BreakpointList.cpp:188
   size_t curr_i = 0;
-  for (pos = m_breakpoints.begin(), curr_i = 0; pos != end; ++pos, ++curr_i) {
+  for (const auto &bp_sp : m_breakpoints) {
 if (curr_i == i)

If we use std::vector this will be O(1)



Comment at: source/Breakpoint/BreakpointList.cpp:196
 
 const BreakpointSP BreakpointList::GetBreakpointAtIndex(size_t i) const {
   std::lock_guard guard(m_mutex);

vsk wrote:
> Just call the non-const overload and const_cast?
If we use std::vector this will be O(1)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56425



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

[Lldb-commits] [PATCH] D55998: ELF: create "container" sections from PT_LOAD segments

2019-01-08 Thread Greg Clayton via Phabricator via lldb-commits

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

LGTM


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

https://reviews.llvm.org/D55998



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

[Lldb-commits] [PATCH] D56400: [CMake] Some cleanup around test preparations

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz marked 7 inline comments as done.
sgraenitz added inline comments.



Comment at: CMakeLists.txt:43
 if(LLDB_INCLUDE_TESTS)
+  # FIXME: LLDB_TEST_CXX_COMPILER is unused.
+  # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build 
directory and not to LLVM's!

sgraenitz wrote:
> labath wrote:
> > JDevlieghere wrote:
> > > Why is this unused? Are we only using `LLDB_TEST_C_COMPILER` and if so, 
> > > why?
> > I think that's because this only gets passed to dotest, that it has it's 
> > own logic for deducing the C++ compiler name.
> Yes, we only ever pass on `LLDB_TEST_C_COMPILER` and it should be sufficient.
> https://github.com/llvm-mirror/lldb/blob/2b03c2512ebf1999e330e85d60b7f93703483485/test/CMakeLists.txt#L53
> 
> https://lldb.llvm.org/build.html reads: //Note that MSVC is not supported 
> here, it must be a path to a clang executable.//
> As clang == clang++ ⇒ q.e.d :)
> 
> I think what we really want here is a `LLDB_TEST_COMPILER` setting, but this 
> means: update docs and build bots, potentially phase out the setting? I am 
> not sure about their popularity.
There was a previous initiative for cleanup in D39215. My follow-up patch 
D56440 aims to finish this.



Comment at: CMakeLists.txt:44
+  # FIXME: LLDB_TEST_CXX_COMPILER is unused.
+  # FIXME: In standalone builds LLVM_BINARY_DIR points to LLDB's build 
directory and not to LLVM's!
+  # FIXME: LLDB_TEST_USE_CUSTOM_C/CXX_COMPILER options don't work as intended!

sgraenitz wrote:
> JDevlieghere wrote:
> > sgraenitz wrote:
> > > This was here already when the file was created:
> > > https://github.com/llvm-mirror/lldb/blob/98c187f2613fd8e2f5cdf5f8f35503a102a54f11/cmake/modules/LLDBStandalone.cmake#L67
> > > 
> > > Apparently the below default paths for dsymutil, FileCheck and clang 
> > > never worked in standalone builds..
> > Instead of this FIXME, should we explicitly unset this variable to prevent 
> > mistakes in the future? Or is this fixable?
> Good point. Not sure what the consequences would be. I will try and see what 
> happens.
See followup patch D56443.


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

https://reviews.llvm.org/D56400



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

[Lldb-commits] [PATCH] D56399: [CMake] Fix standalone builds: workaround the cxx target not getting imported yet (unlike clang target)

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz updated this revision to Diff 180679.
sgraenitz added a comment.

Update documentation: libc++ should be checked out when building for macOS


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

https://reviews.llvm.org/D56399

Files:
  CMakeLists.txt
  www/build.html


Index: www/build.html
===
--- www/build.html
+++ www/build.html
@@ -160,7 +160,7 @@
   In Xcode 4.x: lldb/lldb.xcworkspace, select the 
lldb-tool scheme, and build.
 
 Building LLDB with CMake
- First download the LLVM, Clang, and LLDB sources. Refer to this page for precise instructions on this step.
+ First download the LLVM, Clang, libc++ and LLDB sources. Refer 
to this page for precise instructions on this 
step.
  Refer to the code signing instructions in 
lldb/docs/code-signing.txt for info on codesigning debugserver during 
the build.
  Using CMake is documented on the http://llvm.org/docs/CMake.html";>Building LLVM with CMake page.
 Ninja is the recommended generator to use when building LLDB with 
CMake.
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -101,10 +101,22 @@
 list(APPEND LLDB_TEST_DEPS liblldb)
   endif()
 
+  # Add dependencies if we test with the in-tree clang.
+  # This works with standalone builds as they import the clang target.
   if(TARGET clang)
 list(APPEND LLDB_TEST_DEPS clang)
 if(APPLE)
-  list(APPEND LLDB_TEST_DEPS cxx)
+  # If we build clang, we should build libcxx.
+  # FIXME: Standalone builds should import the cxx target as well.
+  if(LLDB_BUILT_STANDALONE)
+# For now check that the include directory exists.
+set(cxx_dir "${LLVM_DIR}/../../../include/c++")
+if(NOT EXISTS ${cxx_dir})
+  message(WARNING "LLDB test suite requires libc++ in 
llvm/projects/libcxx or an existing build symlinked to ${cxx_dir}")
+endif()
+  else()
+list(APPEND LLDB_TEST_DEPS cxx)
+  endif()
 endif()
   endif()
 


Index: www/build.html
===
--- www/build.html
+++ www/build.html
@@ -160,7 +160,7 @@
   In Xcode 4.x: lldb/lldb.xcworkspace, select the lldb-tool scheme, and build.
 
 Building LLDB with CMake
- First download the LLVM, Clang, and LLDB sources. Refer to this page for precise instructions on this step.
+ First download the LLVM, Clang, libc++ and LLDB sources. Refer to this page for precise instructions on this step.
  Refer to the code signing instructions in lldb/docs/code-signing.txt for info on codesigning debugserver during the build.
  Using CMake is documented on the http://llvm.org/docs/CMake.html";>Building LLVM with CMake page.
 Ninja is the recommended generator to use when building LLDB with CMake.
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -101,10 +101,22 @@
 list(APPEND LLDB_TEST_DEPS liblldb)
   endif()
 
+  # Add dependencies if we test with the in-tree clang.
+  # This works with standalone builds as they import the clang target.
   if(TARGET clang)
 list(APPEND LLDB_TEST_DEPS clang)
 if(APPLE)
-  list(APPEND LLDB_TEST_DEPS cxx)
+  # If we build clang, we should build libcxx.
+  # FIXME: Standalone builds should import the cxx target as well.
+  if(LLDB_BUILT_STANDALONE)
+# For now check that the include directory exists.
+set(cxx_dir "${LLVM_DIR}/../../../include/c++")
+if(NOT EXISTS ${cxx_dir})
+  message(WARNING "LLDB test suite requires libc++ in llvm/projects/libcxx or an existing build symlinked to ${cxx_dir}")
+endif()
+  else()
+list(APPEND LLDB_TEST_DEPS cxx)
+  endif()
 endif()
   endif()
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D56443: [CMake] In standalone builds, LLVM_BINARY_DIR should point to LLVM's binary directory

2019-01-08 Thread Stefan Gränitz via Phabricator via lldb-commits

sgraenitz updated this revision to Diff 180683.
sgraenitz added a comment.

Use LLVM_BINARY_DIR when checking for existance of libc++ include directory


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

https://reviews.llvm.org/D56443

Files:
  CMakeLists.txt
  cmake/modules/LLDBStandalone.cmake
  lit/Suite/lit.site.cfg.in
  lit/Unit/lit.site.cfg.py.in


Index: lit/Unit/lit.site.cfg.py.in
===
--- lit/Unit/lit.site.cfg.py.in
+++ lit/Unit/lit.site.cfg.py.in
@@ -1,6 +1,6 @@
 @LIT_SITE_CFG_IN_HEADER@
 
-config.test_exec_root = "@LLVM_BINARY_DIR@" 
+config.test_exec_root = "@LLDB_BINARY_DIR@"
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
Index: lit/Suite/lit.site.cfg.in
===
--- lit/Suite/lit.site.cfg.in
+++ lit/Suite/lit.site.cfg.in
@@ -1,6 +1,6 @@
 @LIT_SITE_CFG_IN_HEADER@
 
-config.test_exec_root = "@LLVM_BINARY_DIR@"
+config.test_exec_root = "@LLDB_BINARY_DIR@"
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
Index: cmake/modules/LLDBStandalone.cmake
===
--- cmake/modules/LLDBStandalone.cmake
+++ cmake/modules/LLDBStandalone.cmake
@@ -108,10 +108,8 @@
 
   set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
 
-  set(LLVM_BINARY_DIR ${CMAKE_BINARY_DIR})
-
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  include_directories("${LLVM_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
+  include_directories("${CMAKE_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
   # Next three include directories are needed when llvm-config is located in 
build directory.
   # LLVM and Clang are assumed to be built together
   if (EXISTS "${LLVM_OBJ_ROOT}/include")
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -114,7 +114,7 @@
   # FIXME: Standalone builds should import the cxx target as well.
   if(LLDB_BUILT_STANDALONE)
 # For now check that the include directory exists.
-set(cxx_dir "${LLVM_DIR}/../../../include/c++")
+set(cxx_dir "${LLVM_BINRAY_DIR}/include/c++")
 if(NOT EXISTS ${cxx_dir})
   message(WARNING "LLDB test suite requires libc++ in 
llvm/projects/libcxx or an existing build symlinked to ${cxx_dir}")
 endif()


Index: lit/Unit/lit.site.cfg.py.in
===
--- lit/Unit/lit.site.cfg.py.in
+++ lit/Unit/lit.site.cfg.py.in
@@ -1,6 +1,6 @@
 @LIT_SITE_CFG_IN_HEADER@
 
-config.test_exec_root = "@LLVM_BINARY_DIR@" 
+config.test_exec_root = "@LLDB_BINARY_DIR@"
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
Index: lit/Suite/lit.site.cfg.in
===
--- lit/Suite/lit.site.cfg.in
+++ lit/Suite/lit.site.cfg.in
@@ -1,6 +1,6 @@
 @LIT_SITE_CFG_IN_HEADER@
 
-config.test_exec_root = "@LLVM_BINARY_DIR@"
+config.test_exec_root = "@LLDB_BINARY_DIR@"
 config.llvm_src_root = "@LLVM_SOURCE_DIR@"
 config.llvm_obj_root = "@LLVM_BINARY_DIR@"
 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"
Index: cmake/modules/LLDBStandalone.cmake
===
--- cmake/modules/LLDBStandalone.cmake
+++ cmake/modules/LLDBStandalone.cmake
@@ -108,10 +108,8 @@
 
   set(PACKAGE_VERSION "${LLVM_PACKAGE_VERSION}")
 
-  set(LLVM_BINARY_DIR ${CMAKE_BINARY_DIR})
-
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
-  include_directories("${LLVM_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
+  include_directories("${CMAKE_BINARY_DIR}/include" "${LLVM_MAIN_INCLUDE_DIR}")
   # Next three include directories are needed when llvm-config is located in build directory.
   # LLVM and Clang are assumed to be built together
   if (EXISTS "${LLVM_OBJ_ROOT}/include")
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -114,7 +114,7 @@
   # FIXME: Standalone builds should import the cxx target as well.
   if(LLDB_BUILT_STANDALONE)
 # For now check that the include directory exists.
-set(cxx_dir "${LLVM_DIR}/../../../include/c++")
+set(cxx_dir "${LLVM_BINRAY_DIR}/include/c++")
 if(NOT EXISTS ${cxx_dir})
   message(WARNING "LLDB test suite requires libc++ in llvm/projects/libcxx or an existing build symlinked to ${cxx_dir}")
 endif()
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D56443: [CMake] In standalone builds, LLVM_BINARY_DIR should point to LLVM's binary directory

2019-01-08 Thread Pavel Labath via Phabricator via lldb-commits

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

Makes sense to me.




Comment at: CMakeLists.txt:117
 # For now check that the include directory exists.
-set(cxx_dir "${LLVM_DIR}/../../../include/c++")
+set(cxx_dir "${LLVM_BINRAY_DIR}/include/c++")
 if(NOT EXISTS ${cxx_dir})

typo (but BINRAY sounds cool, we should make that mean something :P).


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

https://reviews.llvm.org/D56443



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

[Lldb-commits] [PATCH] D56440: [CMake] Phase out LLDB_TEST_C/CXX_COMPILER in favor of single LLDB_TEST_COMPILER

2019-01-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added a comment.

It looks like `LLDB_TEST_COMPILER_IS_DEFAULT` is set but never read. Why do we 
need it exactly?


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

https://reviews.llvm.org/D56440



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

[Lldb-commits] [lldb] r350651 - Convert to LLDB coding style (NFC)

2019-01-08 Thread Adrian Prantl via lldb-commits

Author: adrian
Date: Tue Jan  8 12:48:40 2019
New Revision: 350651

URL: http://llvm.org/viewvc/llvm-project?rev=350651&view=rev
Log:
Convert to LLDB coding style (NFC)

Modified:
lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp?rev=350651&r1=350650&r2=350651&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
(original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp Tue Jan  
8 12:48:40 2019
@@ -125,7 +125,7 @@ ClangASTImporter &DWARFASTParserClang::G
 }
 
 /// Detect a forward declaration that is nested in a DW_TAG_module.
-static bool isClangModuleFwdDecl(const DWARFDIE &Die) {
+static bool IsClangModuleFwdDecl(const DWARFDIE &Die) {
   if (!Die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0))
 return false;
   auto Parent = Die.GetParent();
@@ -151,17 +151,17 @@ TypeSP DWARFASTParserClang::ParseTypeFro
 
   if (!dwo_module_sp->GetSymbolVendor()->FindTypes(decl_context, true,
dwo_types)) {
-if (!isClangModuleFwdDecl(die))
+if (!IsClangModuleFwdDecl(die))
   return TypeSP();
 
 // Since this this type is defined in one of the Clang modules imported by
 // this symbol file, search all of them.
-auto *SymFile = die.GetCU()->GetSymbolFileDWARF();
-for (const auto &NameModule : SymFile->getExternalTypeModules()) {
-  if (!NameModule.second)
+auto *sym_file = die.GetCU()->GetSymbolFileDWARF();
+for (const auto &name_module : sym_file->getExternalTypeModules()) {
+  if (!name_module.second)
 continue;
-  SymbolVendor *SymVendor = NameModule.second->GetSymbolVendor();
-  if (SymVendor->FindTypes(decl_context, true, dwo_types))
+  SymbolVendor *sym_vendor = name_module.second->GetSymbolVendor();
+  if (sym_vendor->FindTypes(decl_context, true, dwo_types))
 break;
 }
   }


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

[Lldb-commits] [lldb] r350652 - [PdbAstBuilder] Remove unused functions

2019-01-08 Thread Jonas Devlieghere via lldb-commits

Author: jdevlieghere
Date: Tue Jan  8 12:58:54 2019
New Revision: 350652

URL: http://llvm.org/viewvc/llvm-project?rev=350652&view=rev
Log:
[PdbAstBuilder] Remove unused functions

PdbAstBuilder.cpp:273:20: warning: unused function 'GetParentUniqueName' 
[-Wunused-function]
PdbAstBuilder.cpp:267:13: warning: unused function 'IsUniqueNameEnumTag' 
[-Wunused-function]

Modified:
lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp

Modified: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp?rev=350652&r1=350651&r2=350652&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp Tue Jan  8 
12:58:54 2019
@@ -264,23 +264,6 @@ PdbAstBuilder::CreateDeclInfoForType(con
   return {context, uname};
 }
 
-static bool IsUniqueNameEnumTag(llvm::StringRef unique_name) {
-  if (unique_name.size() < 4)
-return false;
-  return unique_name[3] == 'W';
-}
-
-static std::string GetParentUniqueName(llvm::StringRef unique_name) {
-  if (unique_name.size() < 4)
-return unique_name;
-  size_t start = IsUniqueNameEnumTag(unique_name) ? 5 : 4;
-  size_t end = unique_name.find('@');
-  if (end == llvm::StringRef::npos)
-return unique_name;
-  std::string result = unique_name.str();
-  return result.erase(start, end - start + 1);
-}
-
 void PdbAstBuilder::BuildParentMap() {
   LazyRandomTypeCollection &types = m_index.tpi().typeCollection();
 


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

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

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

- Use std::vector
- Code review feedback
- Remove code duplication when returning a const by value.


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

https://reviews.llvm.org/D56425

Files:
  include/lldb/Breakpoint/BreakpointList.h
  source/Breakpoint/BreakpointList.cpp

Index: source/Breakpoint/BreakpointList.cpp
===
--- source/Breakpoint/BreakpointList.cpp
+++ source/Breakpoint/BreakpointList.cpp
@@ -14,6 +14,13 @@
 using namespace lldb;
 using namespace lldb_private;
 
+static void NotifyChange(const BreakpointSP &bp, BreakpointEventType event) {
+  Target &target = bp->GetTarget();
+  if (target.EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged))
+target.BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
+  new Breakpoint::BreakpointEventData(event, bp));
+}
+
 BreakpointList::BreakpointList(bool is_internal)
 : m_mutex(), m_breakpoints(), m_next_break_id(0),
   m_is_internal(is_internal) {}
@@ -22,37 +29,34 @@
 
 break_id_t BreakpointList::Add(BreakpointSP &bp_sp, bool notify) {
   std::lock_guard guard(m_mutex);
+
   // Internal breakpoint IDs are negative, normal ones are positive
   bp_sp->SetID(m_is_internal ? --m_next_break_id : ++m_next_break_id);
 
   m_breakpoints.push_back(bp_sp);
-  if (notify) {
-if (bp_sp->GetTarget().EventTypeHasListeners(
-Target::eBroadcastBitBreakpointChanged))
-  bp_sp->GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
-new Breakpoint::BreakpointEventData(
-eBreakpointEventTypeAdded, bp_sp));
-  }
+
+  if (notify)
+NotifyChange(bp_sp, eBreakpointEventTypeAdded);
+
   return bp_sp->GetID();
 }
 
 bool BreakpointList::Remove(break_id_t break_id, bool notify) {
   std::lock_guard guard(m_mutex);
-  bp_collection::iterator pos = GetBreakpointIDIterator(break_id); // Predicate
-  if (pos != m_breakpoints.end()) {
-BreakpointSP bp_sp(*pos);
-m_breakpoints.erase(pos);
-if (notify) {
-  if (bp_sp->GetTarget().EventTypeHasListeners(
-  Target::eBroadcastBitBreakpointChanged))
-bp_sp->GetTarget().BroadcastEvent(
-Target::eBroadcastBitBreakpointChanged,
-new Breakpoint::BreakpointEventData(eBreakpointEventTypeRemoved,
-bp_sp));
-}
-return true;
-  }
-  return false;
+
+  auto it = std::find_if(
+  m_breakpoints.begin(), m_breakpoints.end(),
+  [&](const BreakpointSP &bp) { return bp->GetID() == break_id; });
+
+  if (it == m_breakpoints.end())
+return false;
+
+  if (notify)
+NotifyChange(*it, eBreakpointEventTypeRemoved);
+
+  m_breakpoints.erase(it);
+
+  return true;
 }
 
 void BreakpointList::RemoveInvalidLocations(const ArchSpec &arch) {
@@ -79,93 +83,50 @@
   ClearAllBreakpointSites();
 
   if (notify) {
-bp_collection::iterator pos, end = m_breakpoints.end();
-for (pos = m_breakpoints.begin(); pos != end; ++pos) {
-  if ((*pos)->GetTarget().EventTypeHasListeners(
-  Target::eBroadcastBitBreakpointChanged)) {
-(*pos)->GetTarget().BroadcastEvent(
-Target::eBroadcastBitBreakpointChanged,
-new Breakpoint::BreakpointEventData(eBreakpointEventTypeRemoved,
-*pos));
-  }
-}
+for (const auto &bp_sp : m_breakpoints)
+  NotifyChange(bp_sp, eBreakpointEventTypeRemoved);
   }
-  m_breakpoints.erase(m_breakpoints.begin(), m_breakpoints.end());
+
+  m_breakpoints.clear();
 }
 
 void BreakpointList::RemoveAllowed(bool notify) {
   std::lock_guard guard(m_mutex);
 
-  bp_collection::iterator pos, end = m_breakpoints.end();
-  if (notify) {
-for (pos = m_breakpoints.begin(); pos != end; ++pos) {
-  if(!(*pos)->AllowDelete())
-continue;
-  if ((*pos)->GetTarget().EventTypeHasListeners(
-  Target::eBroadcastBitBreakpointChanged)) {
-(*pos)->GetTarget().BroadcastEvent(
-Target::eBroadcastBitBreakpointChanged,
-new Breakpoint::BreakpointEventData(eBreakpointEventTypeRemoved,
-*pos));
-  }
-}
-  }
-  pos = m_breakpoints.begin();
-  while ( pos != end) {
-auto bp = *pos;
-if (bp->AllowDelete()) {
-  bp->ClearAllBreakpointSites();
-  pos = m_breakpoints.erase(pos);
-} else
-  pos++;
+  for (const auto &bp_sp : m_breakpoints) {
+if (bp_sp->AllowDelete())
+  bp_sp->ClearAllBreakpointSites();
+if (notify)
+  NotifyChange(bp_sp, eBreakpointEventTypeRemoved);
   }
-}
-
-class BreakpointIDMatches {
-public:
-  BreakpointIDMatches(break_id_t break_id) : m_break_id(break_id) {}
 
-  bool operator()(const BreakpointSP &bp) const {
-return m_break_id == bp->GetID(

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere marked 10 inline comments as done.
JDevlieghere added inline comments.



Comment at: source/Breakpoint/BreakpointList.cpp:200
   size_t curr_i = 0;
-  for (pos = m_breakpoints.begin(), curr_i = 0; pos != end; ++pos, ++curr_i) {
+  for (const auto &bp_sp : m_breakpoints) {
 if (curr_i == i)

aprantl wrote:
> separate commit?
I'm happy to split it up but since I'm touching so much already might as well 
merge it into a single change. 


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

https://reviews.llvm.org/D56425



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

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Adrian Prantl via Phabricator via lldb-commits

aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.



Comment at: source/Breakpoint/BreakpointList.cpp:49
+  m_breakpoints.begin(), m_breakpoints.end(),
+  [&](const BreakpointSP &bp) { return bp->GetID() == break_id; });
+

very nice!



Comment at: source/Breakpoint/BreakpointList.cpp:106
+ [&](const BreakpointSP &bp) { return bp->AllowDelete(); 
}),
+  m_breakpoints.end());
+}

also nice!


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

https://reviews.llvm.org/D56425



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

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Greg Clayton via Phabricator via lldb-commits

clayborg accepted this revision.
clayborg added a comment.

If we keep the list sorted we might be able to improve finding breakpoints by 
ID, but that can be done if we need to. BreakpointList::Add would need to 
insert it sorted, then we can get better than O(n) performance on 
FindBreakpointByID and Remove (anything that was using find_if when it is 
searching for a breakpoint by ID).


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

https://reviews.llvm.org/D56425



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

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.

In D56425#1350234 , @clayborg wrote:

> If we keep the list sorted we might be able to improve finding breakpoints by 
> ID, but that can be done if we need to. BreakpointList::Add would need to 
> insert it sorted, then we can get better than O(n) performance on 
> FindBreakpointByID and Remove (anything that was using find_if when it is 
> searching for a breakpoint by ID).


I'll do this as a follow-up.


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

https://reviews.llvm.org/D56425



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

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB350659: [BreakpointList] Simplify/modernize 
BreakpointList (NFC) (authored by JDevlieghere, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D56425?vs=180728&id=180735#toc

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56425

Files:
  include/lldb/Breakpoint/BreakpointList.h
  source/Breakpoint/BreakpointList.cpp

Index: source/Breakpoint/BreakpointList.cpp
===
--- source/Breakpoint/BreakpointList.cpp
+++ source/Breakpoint/BreakpointList.cpp
@@ -14,6 +14,13 @@
 using namespace lldb;
 using namespace lldb_private;
 
+static void NotifyChange(const BreakpointSP &bp, BreakpointEventType event) {
+  Target &target = bp->GetTarget();
+  if (target.EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged))
+target.BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
+  new Breakpoint::BreakpointEventData(event, bp));
+}
+
 BreakpointList::BreakpointList(bool is_internal)
 : m_mutex(), m_breakpoints(), m_next_break_id(0),
   m_is_internal(is_internal) {}
@@ -22,37 +29,34 @@
 
 break_id_t BreakpointList::Add(BreakpointSP &bp_sp, bool notify) {
   std::lock_guard guard(m_mutex);
+
   // Internal breakpoint IDs are negative, normal ones are positive
   bp_sp->SetID(m_is_internal ? --m_next_break_id : ++m_next_break_id);
 
   m_breakpoints.push_back(bp_sp);
-  if (notify) {
-if (bp_sp->GetTarget().EventTypeHasListeners(
-Target::eBroadcastBitBreakpointChanged))
-  bp_sp->GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
-new Breakpoint::BreakpointEventData(
-eBreakpointEventTypeAdded, bp_sp));
-  }
+
+  if (notify)
+NotifyChange(bp_sp, eBreakpointEventTypeAdded);
+
   return bp_sp->GetID();
 }
 
 bool BreakpointList::Remove(break_id_t break_id, bool notify) {
   std::lock_guard guard(m_mutex);
-  bp_collection::iterator pos = GetBreakpointIDIterator(break_id); // Predicate
-  if (pos != m_breakpoints.end()) {
-BreakpointSP bp_sp(*pos);
-m_breakpoints.erase(pos);
-if (notify) {
-  if (bp_sp->GetTarget().EventTypeHasListeners(
-  Target::eBroadcastBitBreakpointChanged))
-bp_sp->GetTarget().BroadcastEvent(
-Target::eBroadcastBitBreakpointChanged,
-new Breakpoint::BreakpointEventData(eBreakpointEventTypeRemoved,
-bp_sp));
-}
-return true;
-  }
-  return false;
+
+  auto it = std::find_if(
+  m_breakpoints.begin(), m_breakpoints.end(),
+  [&](const BreakpointSP &bp) { return bp->GetID() == break_id; });
+
+  if (it == m_breakpoints.end())
+return false;
+
+  if (notify)
+NotifyChange(*it, eBreakpointEventTypeRemoved);
+
+  m_breakpoints.erase(it);
+
+  return true;
 }
 
 void BreakpointList::RemoveInvalidLocations(const ArchSpec &arch) {
@@ -79,93 +83,50 @@
   ClearAllBreakpointSites();
 
   if (notify) {
-bp_collection::iterator pos, end = m_breakpoints.end();
-for (pos = m_breakpoints.begin(); pos != end; ++pos) {
-  if ((*pos)->GetTarget().EventTypeHasListeners(
-  Target::eBroadcastBitBreakpointChanged)) {
-(*pos)->GetTarget().BroadcastEvent(
-Target::eBroadcastBitBreakpointChanged,
-new Breakpoint::BreakpointEventData(eBreakpointEventTypeRemoved,
-*pos));
-  }
-}
+for (const auto &bp_sp : m_breakpoints)
+  NotifyChange(bp_sp, eBreakpointEventTypeRemoved);
   }
-  m_breakpoints.erase(m_breakpoints.begin(), m_breakpoints.end());
+
+  m_breakpoints.clear();
 }
 
 void BreakpointList::RemoveAllowed(bool notify) {
   std::lock_guard guard(m_mutex);
 
-  bp_collection::iterator pos, end = m_breakpoints.end();
-  if (notify) {
-for (pos = m_breakpoints.begin(); pos != end; ++pos) {
-  if(!(*pos)->AllowDelete())
-continue;
-  if ((*pos)->GetTarget().EventTypeHasListeners(
-  Target::eBroadcastBitBreakpointChanged)) {
-(*pos)->GetTarget().BroadcastEvent(
-Target::eBroadcastBitBreakpointChanged,
-new Breakpoint::BreakpointEventData(eBreakpointEventTypeRemoved,
-*pos));
-  }
-}
-  }
-  pos = m_breakpoints.begin();
-  while ( pos != end) {
-auto bp = *pos;
-if (bp->AllowDelete()) {
-  bp->ClearAllBreakpointSites();
-  pos = m_breakpoints.erase(pos);
-} else
-  pos++;
+  for (const auto &bp_sp : m_breakpoints) {
+if (bp_sp->AllowDelete())
+  bp_sp->ClearAllBreakpointSites();
+if (notify)
+  NotifyChange(bp_sp, eBreakpointEventTypeRemoved);
   }
-}
-
-class BreakpointIDMatches {
-public:
-  BreakpointIDMatches(brea

[Lldb-commits] [lldb] r350659 - [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Jonas Devlieghere via lldb-commits

Author: jdevlieghere
Date: Tue Jan  8 14:07:42 2019
New Revision: 350659

URL: http://llvm.org/viewvc/llvm-project?rev=350659&view=rev
Log:
[BreakpointList] Simplify/modernize BreakpointList (NFC)

I was looking at the code in BreakpointList.cpp and found it deserved a
quick cleanup.

  -  Use std::vector instead of a std::list.
  -  Extract duplicate code for notifying.
  -  Remove code duplication when returning a const value.
  -  Use range-based for loop.
  -  Use early return in loops.

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

Modified:
lldb/trunk/include/lldb/Breakpoint/BreakpointList.h
lldb/trunk/source/Breakpoint/BreakpointList.cpp

Modified: lldb/trunk/include/lldb/Breakpoint/BreakpointList.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Breakpoint/BreakpointList.h?rev=350659&r1=350658&r2=350659&view=diff
==
--- lldb/trunk/include/lldb/Breakpoint/BreakpointList.h (original)
+++ lldb/trunk/include/lldb/Breakpoint/BreakpointList.h Tue Jan  8 14:07:42 2019
@@ -50,18 +50,6 @@ public:
   void Dump(Stream *s) const;
 
   //--
-  /// Returns a shared pointer to the breakpoint with id \a breakID.
-  ///
-  /// @param[in] breakID
-  ///   The breakpoint ID to seek for.
-  ///
-  /// @result
-  ///   A shared pointer to the breakpoint.  May contain a NULL pointer if the
-  ///   breakpoint doesn't exist.
-  //--
-  lldb::BreakpointSP FindBreakpointByID(lldb::break_id_t breakID);
-
-  //--
   /// Returns a shared pointer to the breakpoint with id \a breakID.  Const
   /// version.
   ///
@@ -72,7 +60,7 @@ public:
   ///   A shared pointer to the breakpoint.  May contain a NULL pointer if the
   ///   breakpoint doesn't exist.
   //--
-  const lldb::BreakpointSP FindBreakpointByID(lldb::break_id_t breakID) const;
+  lldb::BreakpointSP FindBreakpointByID(lldb::break_id_t breakID) const;
 
   //--
   /// Returns a shared pointer to the breakpoint with index \a i.
@@ -84,20 +72,7 @@ public:
   ///   A shared pointer to the breakpoint.  May contain a NULL pointer if the
   ///   breakpoint doesn't exist.
   //--
-  lldb::BreakpointSP GetBreakpointAtIndex(size_t i);
-
-  //--
-  /// Returns a shared pointer to the breakpoint with index \a i, const
-  /// version
-  ///
-  /// @param[in] i
-  ///   The breakpoint index to seek for.
-  ///
-  /// @result
-  ///   A shared pointer to the breakpoint.  May contain a NULL pointer if the
-  ///   breakpoint doesn't exist.
-  //--
-  const lldb::BreakpointSP GetBreakpointAtIndex(size_t i) const;
+  lldb::BreakpointSP GetBreakpointAtIndex(size_t i) const;
 
   //--
   /// Find all the breakpoints with a given name
@@ -197,7 +172,7 @@ public:
   void GetListMutex(std::unique_lock &lock);
 
 protected:
-  typedef std::list bp_collection;
+  typedef std::vector bp_collection;
 
   bp_collection::iterator GetBreakpointIDIterator(lldb::break_id_t breakID);
 
@@ -207,7 +182,7 @@ protected:
   std::recursive_mutex &GetMutex() const { return m_mutex; }
 
   mutable std::recursive_mutex m_mutex;
-  bp_collection m_breakpoints; // The breakpoint list, currently a list.
+  bp_collection m_breakpoints;
   lldb::break_id_t m_next_break_id;
   bool m_is_internal;
 

Modified: lldb/trunk/source/Breakpoint/BreakpointList.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/BreakpointList.cpp?rev=350659&r1=350658&r2=350659&view=diff
==
--- lldb/trunk/source/Breakpoint/BreakpointList.cpp (original)
+++ lldb/trunk/source/Breakpoint/BreakpointList.cpp Tue Jan  8 14:07:42 2019
@@ -14,6 +14,13 @@
 using namespace lldb;
 using namespace lldb_private;
 
+static void NotifyChange(const BreakpointSP &bp, BreakpointEventType event) {
+  Target &target = bp->GetTarget();
+  if (target.EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged))
+target.BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
+  new Breakpoint::BreakpointEventData(event, bp));
+}
+
 BreakpointList::BreakpointList(bool is_internal)
 : m_mutex(), m_breakpoints(), m_next_break_id(0),
   m_is_internal(is_internal) {}
@@ -22,37 +29,34 @@ BreakpointList::~BreakpointList() {}
 
 break_id_t BreakpointList::Add(BreakpointSP &bp_sp, bool notify) {
   std::lock_guard guard(m_mutex);
+
   // Internal breakpoint IDs a

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Jonas Devlieghere via Phabricator via lldb-commits

JDevlieghere added a comment.

In D56425#1350236 , @JDevlieghere 
wrote:

> In D56425#1350234 , @clayborg wrote:
>
> > If we keep the list sorted we might be able to improve finding breakpoints 
> > by ID, but that can be done if we need to. BreakpointList::Add would need 
> > to insert it sorted, then we can get better than O(n) performance on 
> > FindBreakpointByID and Remove (anything that was using find_if when it is 
> > searching for a breakpoint by ID).
>
>
> I'll do this as a follow-up.

On second thought, maybe it's not such a good idea after all. Internal 
breakpoints are negative so if we keep the vector sorted we'd always insert 
them at the front of the vector. We could change strategies based on whether 
the list is internal or not, but that seems overkill, especially given the code 
is not that hot as Vedant pointed out.

Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56425

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

[Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

2019-01-08 Thread Adrian McCarthy via Phabricator via lldb-commits

amccarth added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:318
+
+  TypeSystem *type_system = 
GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus);
+  if (!type_system)

Is it appropriate to hard code that language type here?  I realize we don't 
have good PDB support of many other languages, but what about C?  Or various 
versions of C++?


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

https://reviews.llvm.org/D56418



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

[Lldb-commits] [PATCH] D56458: Fix unused private field warning.

2019-01-08 Thread Raphael Isemann via Phabricator via lldb-commits

teemperor created this revision.
Herald added a subscriber: lldb-commits.

The member is private and unused if HAVE_LIBCOMPRESSION is undefined, which 
triggers Clang's -Wunused-private-field warning.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D56458

Files:
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp


Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -69,8 +69,9 @@
   m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
   m_send_acks(true), m_compression_type(CompressionType::None),
   m_listen_url(), m_decompression_scratch_type(CompressionType::None),
-  m_decompression_scratch(nullptr)
-{ 
+  m_decompression_scratch(nullptr) {
+  // Unused unless HAVE_LIBCOMPRESSION is defined.
+  (void)m_decompression_scratch_type;
 }
 
 //--


Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -69,8 +69,9 @@
   m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
   m_send_acks(true), m_compression_type(CompressionType::None),
   m_listen_url(), m_decompression_scratch_type(CompressionType::None),
-  m_decompression_scratch(nullptr)
-{ 
+  m_decompression_scratch(nullptr) {
+  // Unused unless HAVE_LIBCOMPRESSION is defined.
+  (void)m_decompression_scratch_type;
 }
 
 //--
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

2019-01-08 Thread Zachary Turner via Phabricator via lldb-commits

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



Comment at: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:318
+
+  TypeSystem *type_system = 
GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus);
+  if (!type_system)

amccarth wrote:
> Is it appropriate to hard code that language type here?  I realize we don't 
> have good PDB support of many other languages, but what about C?  Or various 
> versions of C++?
Well, the question is really whether any of those other languages would ever 
use a different type system.  IMHO, the Language enumeration specifies too many 
unnecessary values (YAGNI), and it really should just be "some flavor of C" and 
"Swift", since no other type systems are even supported (and even Swift is 
downstream, so probably shouldn't even be in the upstream repo).  The entire 
plugin (and indeed, pretty much all of LLDB) is already coupled very tightly 
with clang, and what flavor of C++ we're using won't matter for which 
TypeSystem we need (which is going to be `ClangASTContext` every single time, 
no exceptions).


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

https://reviews.llvm.org/D56418



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

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

2019-01-08 Thread Hui Huang via Phabricator via lldb-commits

Hui added inline comments.



Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1138
+  // Double quote the string.
+  ::snprintf(arg_cstr, sizeof(arg_cstr), "--log-channels=\"%s\"",
+ env_debugserver_log_channels.c_str());

labath wrote:
> zturner wrote:
> > Hui wrote:
> > > This change is essential others seem not that necessary but nice to have 
> > > in order to support multiple systems.
> > I would prefer to avoid using C-style formatting if possible.  LLVM has 
> > some nice formatting routines that are safer and easier to understand.
> > 
> > While we're here, perhaps we could change `char arg_cstr[PATH_MAX];` up at 
> > the top of this function to `std::string arg_str;`
> > 
> > Regardless, we can re-write this in a number of ways, such as:
> > ```
> > arg_str = llvm::formatv("--log-channels = \"{0}\", 
> > env_debugserver_log_channels).str();
> > arg_str = (Twine("--log-channels = \"") + env_debugserver_log_channels + 
> > "\"").str();
> > arg_str = llvm::format("--log-channels = \"%s\"", 
> > env_debugserver_log_channels);
> > ```
> > 
> > to get rid of this snprintf call.
> Actually, this shouldn't be done here at all. The assumption here is that the 
> contents of the `Args` vector will get passed verbatim to the argc+argv of 
> the called program. On unix systems we will do just that, so if you add the 
> quotes here they will be passed the lldb-server, and then the parse there 
> will fail because we will get extra quotes we didn't expect.
> 
> Since Windows needs special logic in order to ensure this, that logic should 
> be inside windows-specific code, probably ProcessLauncherWindows. That will 
> make all other instances of launching processes with spaces work, not just 
> this particular case. It seems ProcessLauncherWindows uses 
> Args::GetQuotedCommandString to do this job, but this function seems 
> extremely under-equipped for this job. I'd suggest implementing a 
> windows-specific version of this in ProcessLauncherWindows, which will do 
> whatever fixups are necessary to make this happen.
It is not that applicable for the windows process launcher to determine which 
entry in the args needs to be quoted unless given very specific flag or option. 
I think this issue shall be documented and ask user to be responsible for 
setting the "right" variables. For example, 

on Windows PS,
$env:LLDB_SERVER_LOG_CHANNELS="""lldb all:windows all"""

instead of
$env:LLDB_SERVER_LOG_CHANNELS="lldb all:windows all"

on MSVC, 
LLDB_DEBUGSERVER_LOG_FILE="d:\\a b c\\log.txt" 

instead of
LLDB_DEBUGSERVER_LOG_FILE=d:\\a b c\\log.txt 
 




Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56230



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

[Lldb-commits] [lldb] r350675 - Fix unused private field warning.

2019-01-08 Thread Raphael Isemann via lldb-commits

Author: teemperor
Date: Tue Jan  8 14:55:02 2019
New Revision: 350675

URL: http://llvm.org/viewvc/llvm-project?rev=350675&view=rev
Log:
Fix unused private field warning.

Summary: The member is private and unused if HAVE_LIBCOMPRESSION is undefined, 
which triggers Clang's -Wunused-private-field warning.

Subscribers: lldb-commits

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

Modified:
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

Modified: 
lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp?rev=350675&r1=350674&r2=350675&view=diff
==
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp 
(original)
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp Tue 
Jan  8 14:55:02 2019
@@ -69,8 +69,9 @@ GDBRemoteCommunication::GDBRemoteCommuni
   m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
   m_send_acks(true), m_compression_type(CompressionType::None),
   m_listen_url(), m_decompression_scratch_type(CompressionType::None),
-  m_decompression_scratch(nullptr)
-{ 
+  m_decompression_scratch(nullptr) {
+  // Unused unless HAVE_LIBCOMPRESSION is defined.
+  (void)m_decompression_scratch_type;
 }
 
 //--


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

[Lldb-commits] [PATCH] D56458: Fix unused private field warning.

2019-01-08 Thread Raphael Isemann via Phabricator via lldb-commits

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL350675: Fix unused private field warning. (authored by 
teemperor, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56458?vs=180748&id=180750#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56458

Files:
  lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp


Index: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -69,8 +69,9 @@
   m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
   m_send_acks(true), m_compression_type(CompressionType::None),
   m_listen_url(), m_decompression_scratch_type(CompressionType::None),
-  m_decompression_scratch(nullptr)
-{ 
+  m_decompression_scratch(nullptr) {
+  // Unused unless HAVE_LIBCOMPRESSION is defined.
+  (void)m_decompression_scratch_type;
 }
 
 //--


Index: lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===
--- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -69,8 +69,9 @@
   m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
   m_send_acks(true), m_compression_type(CompressionType::None),
   m_listen_url(), m_decompression_scratch_type(CompressionType::None),
-  m_decompression_scratch(nullptr)
-{ 
+  m_decompression_scratch(nullptr) {
+  // Unused unless HAVE_LIBCOMPRESSION is defined.
+  (void)m_decompression_scratch_type;
 }
 
 //--
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC)

2019-01-08 Thread Greg Clayton via Phabricator via lldb-commits

clayborg added a comment.

In D56425#1350253 , @JDevlieghere 
wrote:

> In D56425#1350236 , @JDevlieghere 
> wrote:
>
> > In D56425#1350234 , @clayborg 
> > wrote:
> >
> > > If we keep the list sorted we might be able to improve finding 
> > > breakpoints by ID, but that can be done if we need to. 
> > > BreakpointList::Add would need to insert it sorted, then we can get 
> > > better than O(n) performance on FindBreakpointByID and Remove (anything 
> > > that was using find_if when it is searching for a breakpoint by ID).
> >
> >
> > I'll do this as a follow-up.
>
>
> On second thought, maybe it's not such a good idea after all. Internal 
> breakpoints are negative so if we keep the vector sorted we'd always insert 
> them at the front of the vector. We could change strategies based on whether 
> the list is internal or not, but that seems overkill, especially given the 
> code is not that hot as Vedant pointed out.


No worries. This is why I mentioned "if we need to". Fine to wait until we need 
to do this for some reason that proves to be an efficiency issue


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D56425



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

[Lldb-commits] [lldb] r350679 - Change std::sort to llvm::sort to detect non-determinism.

2019-01-08 Thread Jonas Devlieghere via lldb-commits

Author: jdevlieghere
Date: Tue Jan  8 15:25:06 2019
New Revision: 350679

URL: http://llvm.org/viewvc/llvm-project?rev=350679&view=rev
Log:
Change std::sort to llvm::sort to detect non-determinism.

LLVM added wrappers to std::sort (r327219) that randomly shuffle the
container before sorting. The goal is to uncover non-determinism due to
undefined sorting order of objects having the same key.

This can be enabled with -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON.

Modified:
lldb/trunk/include/lldb/Core/UniqueCStringMap.h
lldb/trunk/source/Breakpoint/BreakpointResolver.cpp
lldb/trunk/source/Commands/CommandObjectType.cpp
lldb/trunk/source/Interpreter/OptionValueArray.cpp
lldb/trunk/source/Interpreter/OptionValueFileSpecLIst.cpp
lldb/trunk/source/Interpreter/OptionValuePathMappings.cpp

lldb/trunk/source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp

lldb/trunk/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
lldb/trunk/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/source/Symbol/ArmUnwindInfo.cpp
lldb/trunk/source/Symbol/CompileUnit.cpp
lldb/trunk/source/Symbol/Function.cpp
lldb/trunk/source/Symbol/Symtab.cpp
lldb/trunk/source/Target/Target.cpp
lldb/trunk/source/Utility/Timer.cpp

Modified: lldb/trunk/include/lldb/Core/UniqueCStringMap.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/UniqueCStringMap.h?rev=350679&r1=350678&r2=350679&view=diff
==
--- lldb/trunk/include/lldb/Core/UniqueCStringMap.h (original)
+++ lldb/trunk/include/lldb/Core/UniqueCStringMap.h Tue Jan  8 15:25:06 2019
@@ -217,7 +217,7 @@ public:
   // }
   // my_map.Sort();
   //--
-  void Sort() { std::sort(m_map.begin(), m_map.end()); }
+  void Sort() { llvm::sort(m_map.begin(), m_map.end()); }
 
   //--
   // Since we are using a vector to contain our items it will always double its

Modified: lldb/trunk/source/Breakpoint/BreakpointResolver.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Breakpoint/BreakpointResolver.cpp?rev=350679&r1=350678&r2=350679&view=diff
==
--- lldb/trunk/source/Breakpoint/BreakpointResolver.cpp (original)
+++ lldb/trunk/source/Breakpoint/BreakpointResolver.cpp Tue Jan  8 15:25:06 2019
@@ -238,10 +238,10 @@ void BreakpointResolver::SetSCMatchesByL
   worklist_begin, worklist_end,
   [&](const SymbolContext &sc) { return SourceLoc(sc) < requested; });
   // Sort the remaining entries by (line, column).
-  std::sort(worklist_begin, worklist_end,
-[](const SymbolContext &a, const SymbolContext &b) {
-  return SourceLoc(a) < SourceLoc(b);
-});
+  llvm::sort(worklist_begin, worklist_end,
+ [](const SymbolContext &a, const SymbolContext &b) {
+   return SourceLoc(a) < SourceLoc(b);
+ });
 
   // Filter out all locations with a source location after the closest 
match.
   if (worklist_begin != worklist_end)
@@ -261,11 +261,11 @@ void BreakpointResolver::SetSCMatchesByL
 }
 
 // Sort by file address.
-std::sort(worklist_begin, worklist_end,
-  [](const SymbolContext &a, const SymbolContext &b) {
-return a.line_entry.range.GetBaseAddress().GetFileAddress() <
-   b.line_entry.range.GetBaseAddress().GetFileAddress();
-  });
+llvm::sort(worklist_begin, worklist_end,
+   [](const SymbolContext &a, const SymbolContext &b) {
+ return a.line_entry.range.GetBaseAddress().GetFileAddress() <
+b.line_entry.range.GetBaseAddress().GetFileAddress();
+   });
 
 // Go through and see if there are line table entries that are
 // contiguous, and if so keep only the first of the contiguous range.

Modified: lldb/trunk/source/Commands/CommandObjectType.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectType.cpp?rev=350679&r1=350678&r2=350679&view=diff
==
--- lldb/trunk/source/Commands/CommandObjectType.cpp (original)
+++ lldb/trunk/source/Commands/CommandObjectType.cpp Tue Jan  8 15:25:06 2019
@@ -2907,7 +2907,7 @@ public:
 if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
   guessed_language = GuessLanguage(frame);
   if (guessed_language != eLanguageTypeUnknown) {
-std::sort(
+llvm::sort(
 languages.begin(), languages.end(),
 [guessed_language](Language *l

[Lldb-commits] [PATCH] D56461: [NativePDB] Add support for parsing typedefs and make lldb-test work with the native reader.

2019-01-08 Thread Zachary Turner via Phabricator via lldb-commits

zturner created this revision.
zturner added reviewers: aleksandr.urakov, amccarth, lemo, labath.
Herald added a subscriber: aprantl.

Typedefs are represented as `S_UDT` records in the globals stream.  This 
creates a strange situation where "types" are actually represented as 
"symbols", so they need special handling.

In order to test this, we don't just use lldb and print out some variables 
causing the AST to get created, because variables whose type is a typedef will 
have debug info referencing the original type, not the typedef.  So we use 
`lldb-test` instead which will parse all debug info in the entire file.  This 
exposed some problems with `lldb-test` and the native reader, mainly that 
certain types of obscure symbols which we can find when iterating every single 
record would trigger crashes.  These have been fixed as well so that 
`lldb-test` can be used to test this functionality.

After this patch, I think most of the remaining DIA PDB tests can be re-written 
as native PDB tests, so hopefully this means we cane eliminate the DIA reader 
quite soon.


https://reviews.llvm.org/D56461

Files:
  lldb/include/lldb/Symbol/CompileUnit.h
  lldb/lit/SymbolFile/NativePDB/typedefs.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h
  lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -203,12 +203,14 @@
   lldb::VariableSP GetOrCreateLocalVariable(PdbCompilandSymId scope_id,
 PdbCompilandSymId var_id,
 bool is_param);
+  lldb::TypeSP GetOrCreateTypedef(PdbGlobalSymId id);
 
   lldb::FunctionSP CreateFunction(PdbCompilandSymId func_id,
   CompileUnit &comp_unit);
   Block &CreateBlock(PdbCompilandSymId block_id);
   lldb::VariableSP CreateLocalVariable(PdbCompilandSymId scope_id,
PdbCompilandSymId var_id, bool is_param);
+  lldb::TypeSP CreateTypedef(PdbGlobalSymId id);
   lldb::CompUnitSP CreateCompileUnit(const CompilandIndexItem &cci);
   lldb::TypeSP CreateType(PdbTypeSymId type_id, CompilerType ct);
   lldb::TypeSP CreateAndCacheType(PdbTypeSymId type_id);
@@ -222,6 +224,7 @@
   llvm::BumpPtrAllocator m_allocator;
 
   lldb::addr_t m_obj_load_address = 0;
+  bool m_done_full_type_scan = false;
 
   std::unique_ptr m_index;
 
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Type.h"
 
+#include "Plugins/Language/CPlusPlus/MSVCUndecoratedNameParser.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamBuffer.h"
@@ -394,7 +395,11 @@
 
   ProcSym proc(static_cast(sym_record.kind()));
   cantFail(SymbolDeserializer::deserializeAs(sym_record, proc));
+  if (proc.FunctionType == TypeIndex::None())
+return nullptr;
   TypeSP func_type = GetOrCreateType(proc.FunctionType);
+  if (!func_type)
+return nullptr;
 
   PdbTypeSymId sig_id(proc.FunctionType, false);
   Mangled mangled(proc.Name);
@@ -515,9 +520,19 @@
 }
 
 static std::string GetUnqualifiedTypeName(const TagRecord &record) {
+  if (!record.hasUniqueName()) {
+MSVCUndecoratedNameParser parser(record.Name);
+llvm::ArrayRef specs = parser.GetSpecifiers();
+
+return specs.back().GetBaseName();
+  }
+
   llvm::ms_demangle::Demangler demangler;
   StringView sv(record.UniqueName.begin(), record.UniqueName.size());
   llvm::ms_demangle::TagTypeNode *ttn = demangler.parseTagUniqueName(sv);
+  if (demangler.Error)
+return record.Name;
+
   llvm::ms_demangle::IdentifierNode *idn =
   ttn->QualifiedName->getUnqualifiedIdentifier();
   return idn->toString();
@@ -696,7 +711,10 @@
   if (iter != m_types.end())
 return iter->second;
 
-  return CreateAndCacheType(type_id);
+  TypeSP type = CreateAndCacheType(type_id);
+  if (type)
+m_obj_file->GetModule()->GetTypeList()->Insert(type);
+  return type;
 }
 
 VariableSP SymbolFileNativePDB::CreateGlobalVariable(PdbGlobalSymId var_id) {
@@ -821,7 +839,6 @@
   if (emplace_result.second)
 emplace_result.first->second = CreateFunction(func_id, comp_unit);
 
-  lldbassert(emplace_result.first->second);
   return emplace_result.first->second;
 }
 
@@ -887,7 +9

[Lldb-commits] [PATCH] D56462: Change SymbolFile::ParseTypes to ParseTypesForCompileUnit

2019-01-08 Thread Zachary Turner via Phabricator via lldb-commits

zturner created this revision.
zturner added reviewers: clayborg, labath, davide, jingham.
Herald added a subscriber: JDevlieghere.

The function `SymbolFile::ParseTypes` previously accepted a `SymbolContext`.  
This makes it extremely difficult to implement faithfully, because you have to 
account for all possible combinations of members being set in the 
`SymbolContext`.  On the other hand, no clients of this function actually care 
about implementing this function to this strict of a standard.  AFAICT, there 
is actually only 1 client in the entire codebase, and it is the function 
`ParseAllDebugSymbols`, which is itself only called for testing purposes when 
dumping information.  At this call-site, the only field it sets is the 
`CompileUnit`, meaning that an implementer of a `SymbolFile` need not worry 
about any examining or handling any other fields which might be set.

By restricting this API to accept exactly a `CompileUnit&` and nothing more, we 
can simplify the life of new `SymbolFile` plugin implementers by making it 
clear exactly what the necessary and sufficient set of functionality they need 
to implement is, while at the same time removing some dead code that tried to 
handle other types of `SymbolContext` fields that were never going to be set 
anyway.


https://reviews.llvm.org/D56462

Files:
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/include/lldb/Symbol/SymbolVendor.h
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
  lldb/source/Symbol/SymbolVendor.cpp

Index: lldb/source/Symbol/SymbolVendor.cpp
===
--- lldb/source/Symbol/SymbolVendor.cpp
+++ lldb/source/Symbol/SymbolVendor.cpp
@@ -200,12 +200,12 @@
   return 0;
 }
 
-size_t SymbolVendor::ParseTypes(const SymbolContext &sc) {
+size_t SymbolVendor::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
 if (m_sym_file_ap.get())
-  return m_sym_file_ap->ParseTypes(sc);
+  return m_sym_file_ap->ParseTypesForCompileUnit(comp_unit);
   }
   return 0;
 }
Index: lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
===
--- lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
+++ lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
@@ -70,7 +70,8 @@
 
   size_t ParseFunctionBlocks(const lldb_private::SymbolContext &sc) override;
 
-  size_t ParseTypes(const lldb_private::SymbolContext &sc) override;
+  size_t
+  ParseTypesForCompileUnit(lldb_private::CompileUnit &comp_unit) override;
 
   size_t
   ParseVariablesForContext(const lldb_private::SymbolContext &sc) override;
Index: lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
===
--- lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
+++ lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
@@ -229,7 +229,9 @@
   return 0;
 }
 
-size_t SymbolFileSymtab::ParseTypes(const SymbolContext &sc) { return 0; }
+size_t SymbolFileSymtab::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
+  return 0;
+}
 
 size_t SymbolFileSymtab::ParseVariablesForContext(const SymbolContext &sc) {
   return 0;
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
@@ -81,7 +81,8 @@
 
   size_t ParseFunctionBlocks(const lldb_private::SymbolContext &sc) override;
 
-  size_t ParseTypes(const lldb_private::SymbolContext &sc) override;
+  size_t
+  ParseTypesForCompileUnit(lldb_private::CompileUnit &comp_unit) override;
 
   size_t
   ParseVariablesForContext(const lldb_private::SymbolContext &sc) override;
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -464,13 +464,10 @@
   return num_added;
 }
 
-size_t SymbolFilePDB::ParseTypes(const lldb_private::SymbolContext &sc) {
-  lldbassert(sc.module_sp.get());
-  if (!sc.comp_unit)
-return 0;
+size_t SymbolFilePDB::ParseTypesForCompileUnit(CompileUnit &comp_unit) {
 
   size_t num_adde

[Lldb-commits] [lldb] r350682 - [CMakeLists] Sort tools/CMakeLists.txt

2019-01-08 Thread Jonas Devlieghere via lldb-commits

Author: jdevlieghere
Date: Tue Jan  8 16:31:30 2019
New Revision: 350682

URL: http://llvm.org/viewvc/llvm-project?rev=350682&view=rev
Log:
[CMakeLists] Sort tools/CMakeLists.txt

Modified:
lldb/trunk/tools/CMakeLists.txt

Modified: lldb/trunk/tools/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/CMakeLists.txt?rev=350682&r1=350681&r2=350682&view=diff
==
--- lldb/trunk/tools/CMakeLists.txt (original)
+++ lldb/trunk/tools/CMakeLists.txt Tue Jan  8 16:31:30 2019
@@ -1,13 +1,15 @@
-if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
-  add_subdirectory(darwin-debug)
-  add_subdirectory(debugserver)
-endif()
 add_subdirectory(argdumper)
 add_subdirectory(driver)
+add_subdirectory(intel-features)
 add_subdirectory(lldb-mi)
+add_subdirectory(lldb-test)
 add_subdirectory(lldb-vscode)
+
+if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
+  add_subdirectory(darwin-debug)
+  add_subdirectory(debugserver)
+endif()
+
 if (LLDB_CAN_USE_LLDB_SERVER AND NOT SKIP_LLDB_SERVER_BUILD)
   add_subdirectory(lldb-server)
 endif()
-add_subdirectory(intel-features)
-add_subdirectory(lldb-test)


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

45 matches

Mail list logo