[Lldb-commits] [PATCH] D32732: "target variable" not showing all global variable if we print any global variable before execution starts
vbalu added a comment. In https://reviews.llvm.org/D32732#753348, @jingham wrote: > That looks like the right way to do it. What was your thinking behind > returning null rather than the partial list of variables already parsed if > can_create is false? That doesn't seem like what somebody who is bothering > to pass in can_create false intends. The issue is use of m_variables when it is partially Parsed. when a function looking of a global variable count it just call the GetVariableList with "can_create=false" which will return the partially parsed m_variabe leads to wrong count. "can_create" avoid the recursion while parsing the variables and to get the parsed global variable list to the callee. So i thought returning NULL should be wise instead of sending partially parsed list. Repository: rL LLVM https://reviews.llvm.org/D32732 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
labath added inline comments. Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk:1 +LEVEL := ../../../make + Thanks for the effort. It almost works for me :), except for the part where you clear out the CFLAGS. We cannot do that, as CFLAGS sometimes contains important flags that we cannot compile without. The "canonical" way to compile without debug info is to use the CFLAGS_NO_DEBUG flag and write the compile rule yourself. This makefile worked for me, and I hope it will work for you as well as it was pieced together from existing tests: ``` LEVEL := ../../../make DYLIB_NAME := Two DYLIB_C_SOURCES := Two/Two.c Two/TwoConstant.c DYLIB_ONLY := YES include $(LEVEL)/Makefile.rules CFLAGS += -fPIC Two/TwoConstant.o: Two/TwoConstant.c $(CC) $(CFLAGS_NO_DEBUG) -c $< -o $@ ``` Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py:16 + +mydir = TestBase.compute_mydir(__file__) + Since the test does not depend on debug info, you may want to consider adding `NO_DEBUG_INFO_TESTCASE = True` here to avoid running it multiple times. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r303160 - Skip TestWatchedVarHitWhenInScope on android arm because it triggers a kernel bug
Author: labath
Date: Tue May 16 06:58:18 2017
New Revision: 303160
URL: http://llvm.org/viewvc/llvm-project?rev=303160&view=rev
Log:
Skip TestWatchedVarHitWhenInScope on android arm because it triggers a kernel
bug
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/variable_out_of_scope/TestWatchedVarHitWhenInScope.py
lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/variable_out_of_scope/TestWatchedVarHitWhenInScope.py
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/variable_out_of_scope/TestWatchedVarHitWhenInScope.py?rev=303160&r1=303159&r2=303160&view=diff
==
---
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/variable_out_of_scope/TestWatchedVarHitWhenInScope.py
(original)
+++
lldb/trunk/packages/Python/lldbsuite/test/functionalities/watchpoint/variable_out_of_scope/TestWatchedVarHitWhenInScope.py
Tue May 16 06:58:18 2017
@@ -11,6 +11,7 @@ import time
import lldb
from lldbsuite.test.lldbtest import *
import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.decorators import *
class WatchedVariableHitWhenInScopeTestCase(TestBase):
@@ -33,6 +34,8 @@ class WatchedVariableHitWhenInScopeTestC
self.exe_name = self.testMethodName
self.d = {'C_SOURCES': self.source, 'EXE': self.exe_name}
+# Test hangs due to a kernel bug, see fdfeff0f in the linux kernel for
details
+@skipIfTargetAndroid(api_levels=list(range(25+1)), archs=["aarch64",
"arm"])
@unittest2.expectedFailure("rdar://problem/18685649")
def test_watched_var_should_only_hit_when_in_scope(self):
"""Test that a variable watchpoint should only hit when in scope."""
Modified: lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp?rev=303160&r1=303159&r2=303160&view=diff
==
--- lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp (original)
+++ lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp Tue May 16
06:58:18 2017
@@ -46,7 +46,7 @@ using namespace std::chrono;
namespace {
-const seconds kReadTimeout(8);
+const seconds kReadTimeout(12);
const char *kOKAY = "OKAY";
const char *kFAIL = "FAIL";
const char *kDATA = "DATA";
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33241: Add Status -- llvm::Error glue
labath created this revision.
I tried convert a function to llvm::Error/Expected but I quickly ran into the
issue of interfacing with code that still expects the Status objects.
This is my proposal for conversion functions between the two. I use
constructors and conversion operators on the Status class for the job,
but I make them explicit to make sure they're not converted
accidentally, as they have different semantics. I made sure conversion
preserves the errno error codes (by converting them to std::error_code,
which llvm::Error understands) as we have bits of code which switch
based on the error value. As for the other errors, only the error
message is preserved during the conversion.
https://reviews.llvm.org/D33241
Files:
include/lldb/Utility/Status.h
source/Utility/Status.cpp
unittests/Utility/StatusTest.cpp
Index: unittests/Utility/StatusTest.cpp
===
--- unittests/Utility/StatusTest.cpp
+++ unittests/Utility/StatusTest.cpp
@@ -11,9 +11,40 @@
#include "gtest/gtest.h"
using namespace lldb_private;
+using namespace lldb;
TEST(StatusTest, Formatv) {
EXPECT_EQ("", llvm::formatv("{0}", Status()).str());
EXPECT_EQ("Hello Status", llvm::formatv("{0}", Status("Hello Status")).str());
EXPECT_EQ("Hello", llvm::formatv("{0:5}", Status("Hello Error")).str());
}
+
+TEST(StatusTest, ErrorConstructor) {
+ EXPECT_TRUE(Status(llvm::Error::success()).Success());
+
+ Status eagain(
+ llvm::errorCodeToError(std::error_code(EAGAIN, std::generic_category(;
+ EXPECT_TRUE(eagain.Fail());
+ EXPECT_EQ(eErrorTypePOSIX, eagain.GetType());
+ EXPECT_EQ(Status::ValueType(EAGAIN), eagain.GetError());
+
+ Status foo(llvm::make_error(
+ "foo", llvm::inconvertibleErrorCode()));
+ EXPECT_TRUE(foo.Fail());
+ EXPECT_EQ(eErrorTypeGeneric, foo.GetType());
+ EXPECT_STREQ("foo", foo.AsCString());
+}
+
+TEST(StatusTest, ErrorConversion) {
+ EXPECT_FALSE(bool(llvm::Error(Status(;
+
+ llvm::Error eagain(Status(EAGAIN, ErrorType::eErrorTypePOSIX));
+ EXPECT_TRUE(bool(eagain));
+ std::error_code ec = llvm::errorToErrorCode(std::move(eagain));
+ EXPECT_EQ(EAGAIN, ec.value());
+ EXPECT_EQ(std::generic_category(), ec.category());
+
+ llvm::Error foo(Status("foo"));
+ EXPECT_TRUE(bool(foo));
+ EXPECT_EQ("foo", llvm::toString(std::move(foo)));
+}
Index: source/Utility/Status.cpp
===
--- source/Utility/Status.cpp
+++ source/Utility/Status.cpp
@@ -56,6 +56,37 @@
va_end(args);
}
+Status::Status(llvm::Error error)
+: m_code(0), m_type(ErrorType::eErrorTypeGeneric) {
+ if (!error)
+return;
+
+ // if the error happens to be a errno error, preserve the error code
+ error = llvm::handleErrors(
+ std::move(error), [&](std::unique_ptr e) -> llvm::Error {
+std::error_code ec = e->convertToErrorCode();
+if (ec.category() == std::generic_category()) {
+ m_code = ec.value();
+ m_type = ErrorType::eErrorTypePOSIX;
+ return llvm::Error::success();
+}
+return llvm::Error(std::move(e));
+ });
+
+ // Otherwise, just preserve the message
+ if (error)
+SetErrorString(llvm::toString(std::move(error)));
+}
+
+Status::operator llvm::Error() {
+ if (Success())
+return llvm::Error::success();
+ if (m_type == ErrorType::eErrorTypePOSIX)
+return llvm::errorCodeToError(std::error_code(m_code, std::generic_category()));
+ return llvm::make_error(AsCString(),
+ llvm::inconvertibleErrorCode());
+}
+
//--
// Assignment operator
//--
Index: include/lldb/Utility/Status.h
===
--- include/lldb/Utility/Status.h
+++ include/lldb/Utility/Status.h
@@ -1,29 +1,26 @@
-//===-- Status.h -*- C++
-//-*-===//
+//===-- Status.h *- C++ -*-===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===--===//
-#ifndef __DCError_h__
-#define __DCError_h__
-#if defined(__cplusplus)
+#ifndef LLDB_UTILITY_STATUS_H
+#define LLDB_UTILITY_STATUS_H
#include "lldb/lldb-defines.h"
#include "lldb/lldb-enumerations.h" // for ErrorType, ErrorType...
#include "llvm/ADT/StringRef.h" // for StringRef
+#include "llvm/Support/Error.h"
#include "llvm/Support/FormatVariadic.h"
-
#include
+#include // for uint32_t
#include
#include // for error_code
#include // for forward
-#include // for uint32_t
-
namespace llvm {
class raw_ostream;
}
@@ -106,6 +103,10 @@
~Statu
[Lldb-commits] [PATCH] D32899: [RuntimeDyld] Fix debug section relocation (pr20457)
lhames accepted this revision. lhames added a comment. This revision is now accepted and ready to land. LGTM. :) And yes - I think it's reasonable to add that MCJIT unit test case too. Thanks for working on this! https://reviews.llvm.org/D32899 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
scott.smith created this revision.
scott.smith added a project: LLDB.
Herald added a subscriber: mgorny.
Remove the thread pool and for_each-like iteration functions.
Keep RunTasks, which has no analog in llvm::parallel, but implement it using
llvm::parallel.
Repository:
rL LLVM
https://reviews.llvm.org/D33246
Files:
include/lldb/Utility/TaskPool.h
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Utility/CMakeLists.txt
source/Utility/TaskPool.cpp
unittests/Utility/TaskPoolTest.cpp
Index: unittests/Utility/TaskPoolTest.cpp
===
--- unittests/Utility/TaskPoolTest.cpp
+++ unittests/Utility/TaskPoolTest.cpp
@@ -2,20 +2,6 @@
#include "lldb/Utility/TaskPool.h"
-TEST(TaskPoolTest, AddTask) {
- auto fn = [](int x) { return x * x + 1; };
-
- auto f1 = TaskPool::AddTask(fn, 1);
- auto f2 = TaskPool::AddTask(fn, 2);
- auto f3 = TaskPool::AddTask(fn, 3);
- auto f4 = TaskPool::AddTask(fn, 4);
-
- ASSERT_EQ(10, f3.get());
- ASSERT_EQ(2, f1.get());
- ASSERT_EQ(17, f4.get());
- ASSERT_EQ(5, f2.get());
-}
-
TEST(TaskPoolTest, RunTasks) {
std::vector r(4);
@@ -29,15 +15,3 @@
ASSERT_EQ(10, r[2]);
ASSERT_EQ(17, r[3]);
}
-
-TEST(TaskPoolTest, TaskMap) {
- int data[4];
- auto fn = [&data](int x) { data[x] = x * x; };
-
- TaskMapOverInt(0, 4, fn);
-
- ASSERT_EQ(data[0], 0);
- ASSERT_EQ(data[1], 1);
- ASSERT_EQ(data[2], 4);
- ASSERT_EQ(data[3], 9);
-}
Index: source/Utility/TaskPool.cpp
===
--- source/Utility/TaskPool.cpp
+++ /dev/null
@@ -1,98 +0,0 @@
-//===- TaskPool.cpp -*- C++ -*-===//
-//
-// The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===--===//
-
-#include "lldb/Utility/TaskPool.h"
-
-#include // for uint32_t
-#include// for queue
-#include // for thread
-
-namespace {
-class TaskPoolImpl {
-public:
- static TaskPoolImpl &GetInstance();
-
- void AddTask(std::function &&task_fn);
-
-private:
- TaskPoolImpl();
-
- static void Worker(TaskPoolImpl *pool);
-
- std::queue> m_tasks;
- std::mutex m_tasks_mutex;
- uint32_t m_thread_count;
-};
-
-} // end of anonymous namespace
-
-TaskPoolImpl &TaskPoolImpl::GetInstance() {
- static TaskPoolImpl g_task_pool_impl;
- return g_task_pool_impl;
-}
-
-void TaskPool::AddTaskImpl(std::function &&task_fn) {
- TaskPoolImpl::GetInstance().AddTask(std::move(task_fn));
-}
-
-TaskPoolImpl::TaskPoolImpl() : m_thread_count(0) {}
-
-void TaskPoolImpl::AddTask(std::function &&task_fn) {
- static const uint32_t max_threads = std::thread::hardware_concurrency();
-
- std::unique_lock lock(m_tasks_mutex);
- m_tasks.emplace(std::move(task_fn));
- if (m_thread_count < max_threads) {
-m_thread_count++;
-// Note that this detach call needs to happen with the m_tasks_mutex held.
-// This prevents the thread
-// from exiting prematurely and triggering a linux libc bug
-// (https://sourceware.org/bugzilla/show_bug.cgi?id=19951).
-std::thread(Worker, this).detach();
- }
-}
-
-void TaskPoolImpl::Worker(TaskPoolImpl *pool) {
- while (true) {
-std::unique_lock lock(pool->m_tasks_mutex);
-if (pool->m_tasks.empty()) {
- pool->m_thread_count--;
- break;
-}
-
-std::function f = pool->m_tasks.front();
-pool->m_tasks.pop();
-lock.unlock();
-
-f();
- }
-}
-
-void TaskMapOverInt(size_t begin, size_t end,
-std::function const &func) {
- std::atomic idx{begin};
- size_t num_workers =
- std::min(end, std::thread::hardware_concurrency());
-
- auto wrapper = [&idx, end, &func]() {
-while (true) {
- size_t i = idx.fetch_add(1);
- if (i >= end)
-break;
- func(i);
-}
- };
-
- std::vector> futures;
- futures.reserve(num_workers);
- for (size_t i = 0; i < num_workers; i++)
-futures.push_back(TaskPool::AddTask(wrapper));
- for (size_t i = 0; i < num_workers; i++)
-futures[i].wait();
-}
Index: source/Utility/CMakeLists.txt
===
--- source/Utility/CMakeLists.txt
+++ source/Utility/CMakeLists.txt
@@ -26,7 +26,6 @@
StringExtractorGDBRemote.cpp
StringLexer.cpp
StringList.cpp
- TaskPool.cpp
TildeExpressionResolver.cpp
UserID.cpp
UriParser.cpp
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -11,6 +11,7 @@
// Other libraries and framework includes
#include "llvm/Support/Casting.h"
+#include "llvm/Support/Parallel.h"
#include "llvm/Support/Threading.h"
#include "lldb/Core/ArchSpec
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
spyffe updated this revision to Diff 99186.
spyffe added a comment.
Updated to reflect Pavel Labath's suggestions:
- Used `CFLAGS_NO_DEBUG` where appropriate
- Marked the testcase as having no debug info variants
https://reviews.llvm.org/D33083
Files:
include/lldb/Symbol/SymbolContext.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
source/Symbol/SymbolContext.cpp
Index: source/Symbol/SymbolContext.cpp
===
--- source/Symbol/SymbolContext.cpp
+++ source/Symbol/SymbolContext.cpp
@@ -799,6 +799,163 @@
return true;
}
+const Symbol *
+SymbolContext::FindBestGlobalDataSymbol(const ConstString &name, Status &error) {
+ error.Clear();
+
+ if (!target_sp) {
+return nullptr;
+ }
+
+ Target &target = *target_sp;
+ Module *module = module_sp.get();
+
+ auto ProcessMatches = [this, &name, &target, module]
+ (SymbolContextList &sc_list, Status &error) -> const Symbol* {
+llvm::SmallVector external_symbols;
+llvm::SmallVector internal_symbols;
+const uint32_t matches = sc_list.GetSize();
+for (uint32_t i = 0; i < matches; ++i) {
+ SymbolContext sym_ctx;
+ sc_list.GetContextAtIndex(i, sym_ctx);
+ if (sym_ctx.symbol) {
+const Symbol *symbol = sym_ctx.symbol;
+const Address sym_address = symbol->GetAddress();
+
+if (sym_address.IsValid()) {
+ switch (symbol->GetType()) {
+case eSymbolTypeData:
+case eSymbolTypeRuntime:
+case eSymbolTypeAbsolute:
+case eSymbolTypeObjCClass:
+case eSymbolTypeObjCMetaClass:
+case eSymbolTypeObjCIVar:
+ if (symbol->GetDemangledNameIsSynthesized()) {
+// If the demangled name was synthesized, then don't use it
+// for expressions. Only let the symbol match if the mangled
+// named matches for these symbols.
+if (symbol->GetMangled().GetMangledName() != name)
+ break;
+ }
+ if (symbol->IsExternal()) {
+external_symbols.push_back(symbol);
+ } else {
+internal_symbols.push_back(symbol);
+ }
+ break;
+case eSymbolTypeReExported: {
+ ConstString reexport_name = symbol->GetReExportedSymbolName();
+ if (reexport_name) {
+ModuleSP reexport_module_sp;
+ModuleSpec reexport_module_spec;
+reexport_module_spec.GetPlatformFileSpec() =
+symbol->GetReExportedSymbolSharedLibrary();
+if (reexport_module_spec.GetPlatformFileSpec()) {
+ reexport_module_sp =
+ target.GetImages().FindFirstModule(reexport_module_spec);
+ if (!reexport_module_sp) {
+reexport_module_spec.GetPlatformFileSpec()
+.GetDirectory()
+.Clear();
+reexport_module_sp =
+target.GetImages().FindFirstModule(reexport_module_spec);
+ }
+}
+// Don't allow us to try and resolve a re-exported symbol if it is
+// the same as the current symbol
+if (name == symbol->GetReExportedSymbolName() &&
+module == reexport_module_sp.get())
+ return nullptr;
+
+return FindBestGlobalDataSymbol(
+symbol->GetReExportedSymbolName(), error);
+ }
+} break;
+
+case eSymbolTypeCode: // We already lookup functions elsewhere
+case eSymbolTypeVariable:
+case eSymbolTypeLocal:
+case eSymbolTypeParam:
+case eSymbolTypeTrampoline:
+case eSymbolTypeInvalid:
+case eSymbolTypeException:
+case eSymbolTypeSourceFile:
+case eSymbolTypeHeaderFile:
+case eSymbolTypeObjectFile:
+case eSymbolTypeCommonBlock:
+
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Just a few makefile changes where CFLAGS is being modified and this will be good to go. Much better location for this. Sean, please keep an eye out for this kind of thing in the future, I believe there is a lot of type searching code and function searching code that could have the same thing done in the future. Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile:6 + +main.o : CFLAGS += -g -O0 + CFLAGS_EXTRAS is the way to add CFLAGS: ``` CFLAGS_EXTRAS += "-g -O0" ``` Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk:9 + +CFLAGS += -fPIC + avoid modifying CFLAGS, modify CFLAGS_EXTRAS and then use it if needed Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk:9 + +CFLAGS += -fPIC + avoid modifying CFLAGS, modify CFLAGS_EXTRAS https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
spyffe updated this revision to Diff 99211.
spyffe added a comment.
Used `CFLAGS_EXTRAS` instead of `CFLAGS` at Greg Clayton's suggestion.
https://reviews.llvm.org/D33083
Files:
include/lldb/Symbol/SymbolContext.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
source/Symbol/SymbolContext.cpp
Index: source/Symbol/SymbolContext.cpp
===
--- source/Symbol/SymbolContext.cpp
+++ source/Symbol/SymbolContext.cpp
@@ -799,6 +799,163 @@
return true;
}
+const Symbol *
+SymbolContext::FindBestGlobalDataSymbol(const ConstString &name, Status &error) {
+ error.Clear();
+
+ if (!target_sp) {
+return nullptr;
+ }
+
+ Target &target = *target_sp;
+ Module *module = module_sp.get();
+
+ auto ProcessMatches = [this, &name, &target, module]
+ (SymbolContextList &sc_list, Status &error) -> const Symbol* {
+llvm::SmallVector external_symbols;
+llvm::SmallVector internal_symbols;
+const uint32_t matches = sc_list.GetSize();
+for (uint32_t i = 0; i < matches; ++i) {
+ SymbolContext sym_ctx;
+ sc_list.GetContextAtIndex(i, sym_ctx);
+ if (sym_ctx.symbol) {
+const Symbol *symbol = sym_ctx.symbol;
+const Address sym_address = symbol->GetAddress();
+
+if (sym_address.IsValid()) {
+ switch (symbol->GetType()) {
+case eSymbolTypeData:
+case eSymbolTypeRuntime:
+case eSymbolTypeAbsolute:
+case eSymbolTypeObjCClass:
+case eSymbolTypeObjCMetaClass:
+case eSymbolTypeObjCIVar:
+ if (symbol->GetDemangledNameIsSynthesized()) {
+// If the demangled name was synthesized, then don't use it
+// for expressions. Only let the symbol match if the mangled
+// named matches for these symbols.
+if (symbol->GetMangled().GetMangledName() != name)
+ break;
+ }
+ if (symbol->IsExternal()) {
+external_symbols.push_back(symbol);
+ } else {
+internal_symbols.push_back(symbol);
+ }
+ break;
+case eSymbolTypeReExported: {
+ ConstString reexport_name = symbol->GetReExportedSymbolName();
+ if (reexport_name) {
+ModuleSP reexport_module_sp;
+ModuleSpec reexport_module_spec;
+reexport_module_spec.GetPlatformFileSpec() =
+symbol->GetReExportedSymbolSharedLibrary();
+if (reexport_module_spec.GetPlatformFileSpec()) {
+ reexport_module_sp =
+ target.GetImages().FindFirstModule(reexport_module_spec);
+ if (!reexport_module_sp) {
+reexport_module_spec.GetPlatformFileSpec()
+.GetDirectory()
+.Clear();
+reexport_module_sp =
+target.GetImages().FindFirstModule(reexport_module_spec);
+ }
+}
+// Don't allow us to try and resolve a re-exported symbol if it is
+// the same as the current symbol
+if (name == symbol->GetReExportedSymbolName() &&
+module == reexport_module_sp.get())
+ return nullptr;
+
+return FindBestGlobalDataSymbol(
+symbol->GetReExportedSymbolName(), error);
+ }
+} break;
+
+case eSymbolTypeCode: // We already lookup functions elsewhere
+case eSymbolTypeVariable:
+case eSymbolTypeLocal:
+case eSymbolTypeParam:
+case eSymbolTypeTrampoline:
+case eSymbolTypeInvalid:
+case eSymbolTypeException:
+case eSymbolTypeSourceFile:
+case eSymbolTypeHeaderFile:
+case eSymbolTypeObjectFile:
+case eSymbolTypeCommonBlock:
+case eSymbolTypeBlock:
+case eSymbolTypeVariableType:
+
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Very nice. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33241: Add Status -- llvm::Error glue
zturner added a reviewer: lhames.
zturner added inline comments.
Comment at: include/lldb/Utility/Status.h:108
+ explicit Status(llvm::Error error);
+ explicit operator llvm::Error();
+
I think we should remove the conversion operator. Instead, why don't we make a
class called `LLDBError` and then just have people say:
```
Status S = getSomeStatus();
return make_error(S);
```
Comment at: source/Utility/Status.cpp:81-88
+Status::operator llvm::Error() {
+ if (Success())
+return llvm::Error::success();
+ if (m_type == ErrorType::eErrorTypePOSIX)
+return llvm::errorCodeToError(std::error_code(m_code,
std::generic_category()));
+ return llvm::make_error(AsCString(),
+ llvm::inconvertibleErrorCode());
Delete in favor of an `LLDBError` class as mentioned before.
https://reviews.llvm.org/D33241
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33241: Add Status -- llvm::Error glue
zturner added inline comments.
Comment at: source/Utility/Status.cpp:81-88
+Status::operator llvm::Error() {
+ if (Success())
+return llvm::Error::success();
+ if (m_type == ErrorType::eErrorTypePOSIX)
+return llvm::errorCodeToError(std::error_code(m_code,
std::generic_category()));
+ return llvm::make_error(AsCString(),
+ llvm::inconvertibleErrorCode());
zturner wrote:
> Delete in favor of an `LLDBError` class as mentioned before.
Actually this doesn't really work, because you don't want to call
`make_error(S)` if `S` is a success condition.
So perhaps instead, I would at least call it something more explicit.
`llvm::Error toError() const` perhaps.
https://reviews.llvm.org/D33241
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r303223 - [Expression parser] Look up module symbols before hunting globally
Author: spyffe Date: Tue May 16 18:46:13 2017 New Revision: 303223 URL: http://llvm.org/viewvc/llvm-project?rev=303223&view=rev Log: [Expression parser] Look up module symbols before hunting globally When it resolves symbol-only variables, the expression parser currently looks only in the global module list. It should prefer the current module. I've fixed that behavior by making it search the current module first, and only search globally if it finds nothing. I've also added a test case. After review, I moved the core of the lookup algorithm into SymbolContext for use by other code that needs it. Thanks to Greg Clayton and Pavel Labath for their help. Differential Revision: https://reviews.llvm.org/D33083 Added: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c Modified: lldb/trunk/include/lldb/Symbol/SymbolContext.h lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h lldb/trunk/source/Symbol/SymbolContext.cpp Modified: lldb/trunk/include/lldb/Symbol/SymbolContext.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Symbol/SymbolContext.h?rev=303223&r1=303222&r2=303223&view=diff == --- lldb/trunk/include/lldb/Symbol/SymbolContext.h (original) +++ lldb/trunk/include/lldb/Symbol/SymbolContext.h Tue May 16 18:46:13 2017 @@ -235,6 +235,29 @@ public: bool GetAddressRangeFromHereToEndLine(uint32_t end_line, AddressRange &range, Status &error); + + //-- + /// Find the best global data symbol visible from this context. + /// + /// Symbol priority is: + /// - extern symbol in the current module if there is one + /// - non-extern symbol in the current module if there is one + /// - extern symbol in the target + /// - non-extern symbol in the target + /// It is an error if the highest-priority result is ambiguous. + /// + /// @param[in] name + /// The name of the symbol to search for. + /// + /// @param[out] error + /// An error that will be populated with a message if there was an + /// ambiguous result. The error will not be populated if no result + /// was found. + /// + /// @return + /// The symbol that was found, or \b nullptr if none was found. + //-- + const Symbol *FindBestGlobalDataSymbol(const ConstString &name, Status &error); void GetDescription(Stream *s, lldb::DescriptionLevel level, Target *target) const; Added: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile?rev=303223&view=auto == --- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile (added) +++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile Tue May 16 18:46:13 2017 @@ -0,0 +1,18 @@ +LEVEL := ../../../make + +LD_EXTRAS := -L. -l$(LIB_PREFIX)One -l$(LIB_PREFIX)Two +C_SOURCES := main.c + +main.o : CFLAGS_EXTRAS += -g -O0 + +include $(LEVEL)/Makefile.rules + +.PHONY: +a.out: lib_One lib_Two + +lib_%: + $(MAKE) -f $*.mk + +clean:: + $(MAKE) -f One.mk clean + $(MAKE) -f Two.mk clean Added: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk?rev=303223&view=auto == --- lldb/trunk/packages/P
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
This revision was automatically updated to reflect the committed changes.
Closed by commit rL303223: [Expression parser] Look up module symbols before
hunting globally (authored by spyffe).
Changed prior to commit:
https://reviews.llvm.org/D33083?vs=99211&id=99227#toc
Repository:
rL LLVM
https://reviews.llvm.org/D33083
Files:
lldb/trunk/include/lldb/Symbol/SymbolContext.h
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
lldb/trunk/source/Symbol/SymbolContext.cpp
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
@@ -0,0 +1,4 @@
+#ifndef TWO_H
+#define TWO_H
+void two();
+#endif
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
@@ -0,0 +1,6 @@
+#include "Two.h"
+#include
+
+void two() {
+ printf("Two\n"); // break here
+}
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
@@ -0,0 +1 @@
+int __attribute__ ((visibility("hidden"))) conflicting_symbol = 2;
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
@@ -0,0 +1,12 @@
+LEVEL := ../../../make
+
+DYLIB_NAME := One
+DYLIB_C_SOURCES := One/One.c One/OneConstant.c
+DYLIB_ONLY := YES
+
+include $(LEVEL)/Makefile.rules
+
+CFLAGS_EXTRAS += -fPIC
+
+One/OneConstant.o: One/OneConstant.c
+ $(CC) $(CFLAGS_NO_DEBUG) -c $< -o $@
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
@@ -0,0 +1,11 @@
+#include "One/One.h"
+#include "Two/Two.h"
+
+#include
+
+int main() {
+ one();
+ two();
+ printf("main\n"); // break here
+ return(0);
+}
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
@@ -0,0 +1,4 @@
+#ifndef ONE_H
+#define ONE_H
+void one();
+#endif
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
@@ -0,0 +1,6 @@
+#include "One.h"
+#include
+
+void one() {
+ printf("One\n"); // break here
+}
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
@@ -0,0 +1 @@
+int __attribute__ ((visibility("hidden"))) conflicting_symbol = 1;
Index: ll
[Lldb-commits] [PATCH] D33241: Add Status -- llvm::Error glue
jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. This seems fine to me. Zachary didn't give reasons why he didn't like the conversion form so I can't really assess that point. The use in the ErrorConversion test case seems pretty natural to me, but it's possible I'm missing whatever was bugging him about this. So I'll okay this for my part, but if you guys want to continue discussing, have at it! https://reviews.llvm.org/D33241 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33241: Add Status -- llvm::Error glue
zturner added a comment. Mostly just that implicit conversion operators are a very subtle source of bugs, and you can't even find where they're being used because it's impossible to grep for it. If you're signing up to get an object that has strict usage semantics like `llvm::Error`, you had better be explicit about it. https://reviews.llvm.org/D33241 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
zturner added inline comments.
Comment at: include/lldb/Utility/TaskPool.h:18-20
+ std::function cbs[sizeof...(T)]{tasks...};
+ llvm::parallel::for_each_n(llvm::parallel::par, static_cast(0),
+ sizeof...(T), [&cbs](size_t idx) { cbs[idx](); });
I'm not sure this is the most efficient implementation. `std::function` has
pretty poor performance, and there might be no need to even convert everything
to `std::function` to begin with. You could make this a bit better by using
`llvm::function_ref` instead.
That said, I wonder if it's worth adding a function like this to
`llvm::TaskGroup`? And you could just enqueue all the tasks, rather than
`for_each_n`. Not sure if there would be a different in practice, what do you
think?
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1995-1996
//--
-TaskMapOverInt(0, num_compile_units, extract_fn);
+llvm::parallel::for_each_n(llvm::parallel::par, 0U, num_compile_units,
+ extract_fn);
What did you decide about the recursive parallelism? I don't know if that
works yet using LLVM's default executor.
Repository:
rL LLVM
https://reviews.llvm.org/D33246
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
scott.smith added inline comments.
Comment at: include/lldb/Utility/TaskPool.h:18-20
+ std::function cbs[sizeof...(T)]{tasks...};
+ llvm::parallel::for_each_n(llvm::parallel::par, static_cast(0),
+ sizeof...(T), [&cbs](size_t idx) { cbs[idx](); });
zturner wrote:
> I'm not sure this is the most efficient implementation. `std::function` has
> pretty poor performance, and there might be no need to even convert
> everything to `std::function` to begin with. You could make this a bit
> better by using `llvm::function_ref` instead.
>
> That said, I wonder if it's worth adding a function like this to
> `llvm::TaskGroup`? And you could just enqueue all the tasks, rather than
> `for_each_n`. Not sure if there would be a different in practice, what do
> you think?
I'm not too worried about std::function vs llvm::function_ref; it isn't called
often, and we still need allocations for the tasks that get enqueued. That
said, there's no reason *to* use std::function, so I'll cahnge it.
I like using for_each_n mostly to regularize the interface. For example,
for_each_n/for_each can then optimize the type of TaskGroup it creates to
ensure that it gets the right # of threads right away, rather than spawning up
enough for full hardware concurrency. Or, if there are a lot of tasks
(unlikely, but possible), then for_each can change to a model of enqueueing one
task per thread, and having that thread loop using std::atomic to increment the
iterator, which reduces allocations in TaskGroup and reduces lock contention
(assuming TaskGroup doesn't use a lock free queue).
i.e. the more things funnel through a single interface, the more we benefit
from optimizing that one implementation.
Also it means we can have for_each_n manage TaskGroups itself (maybe keeping
one around for repeated use, then creating more as needed to support recursion,
etc (more on that later)).
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1995-1996
//--
-TaskMapOverInt(0, num_compile_units, extract_fn);
+llvm::parallel::for_each_n(llvm::parallel::par, 0U, num_compile_units,
+ extract_fn);
zturner wrote:
> What did you decide about the recursive parallelism? I don't know if that
> works yet using LLVM's default executor.
1. This code doesn't care.
2. It looks like it works, since (I think) for_each creates a separate
TaskGroup for each call.
3. However I got a deadlock when using this for parallelizing the dynamic
library loading itself, which used to work. That could either be due to other
code changes, some oversight on my part, or it could be that for_each_n doesn't
actually support recursion - which means that I misunderstood for_each_n. So I
have more work to do...
Repository:
rL LLVM
https://reviews.llvm.org/D33246
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
scott.smith added inline comments.
Comment at: include/lldb/Utility/TaskPool.h:18-20
+ std::function cbs[sizeof...(T)]{tasks...};
+ llvm::parallel::for_each_n(llvm::parallel::par, static_cast(0),
+ sizeof...(T), [&cbs](size_t idx) { cbs[idx](); });
scott.smith wrote:
> zturner wrote:
> > I'm not sure this is the most efficient implementation. `std::function`
> > has pretty poor performance, and there might be no need to even convert
> > everything to `std::function` to begin with. You could make this a bit
> > better by using `llvm::function_ref` instead.
> >
> > That said, I wonder if it's worth adding a function like this to
> > `llvm::TaskGroup`? And you could just enqueue all the tasks, rather than
> > `for_each_n`. Not sure if there would be a different in practice, what do
> > you think?
> I'm not too worried about std::function vs llvm::function_ref; it isn't
> called often, and we still need allocations for the tasks that get enqueued.
> That said, there's no reason *to* use std::function, so I'll cahnge it.
>
> I like using for_each_n mostly to regularize the interface. For example,
> for_each_n/for_each can then optimize the type of TaskGroup it creates to
> ensure that it gets the right # of threads right away, rather than spawning
> up enough for full hardware concurrency. Or, if there are a lot of tasks
> (unlikely, but possible), then for_each can change to a model of enqueueing
> one task per thread, and having that thread loop using std::atomic to
> increment the iterator, which reduces allocations in TaskGroup and reduces
> lock contention (assuming TaskGroup doesn't use a lock free queue).
>
> i.e. the more things funnel through a single interface, the more we benefit
> from optimizing that one implementation.
>
> Also it means we can have for_each_n manage TaskGroups itself (maybe keeping
> one around for repeated use, then creating more as needed to support
> recursion, etc (more on that later)).
>
oh, and I wouldn't be surprised if there's a better home in llvm. I'm fine
with moving it. I doubt it should go in llvm::parallel, since that seems like
it's trying to be similar to std::parallel (though note for_each_n is
incompatible, since it should take Iter,Size instead of Type,Type, and it tries
to dereference Iter, which means you can't pass in a number like all the
callsites do. I tried fixing that but failed due to the deref assumption, and
the LLD dependency).
It's small enough that it does seem like it should fit under something else
rather than be standalone; I'm open to suggestions.
Repository:
rL LLVM
https://reviews.llvm.org/D33246
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
Even though it creates a different TaskGroup for each one, TaskGroup itself
just says getDefaultExecutor, and that returns a global instance. On
Windows this will work because it all delegates to an OS thing that's sort
of like libdispatch and that does support recursion, but the thread pool
executor just has a fixed number of threads and there's no way to increase
or decrease them. Can you put this test inside of ParallelTest.cpp and see
what you get?
TEST(Parallel, parallel_for_recursive) {
uint32_t range[10001];
std::fill(range, range + 10001, 1);
for_each_n(parallel::par, 0, 100, [&range](size_t I) {
for_each_n(parallel::par, 100 * I, 100 * (I+1), [&range](size_t J) {
++range[J];
});
});
uint32_t expected[1];
std::fill(expected, expected + 1, 2);
ASSERT_TRUE(std::equal(range, range + 1, expected));
// Check that we don't write past the end of the requested range.
ASSERT_EQ(range[1], 1u);
}
This passes me for me on Windows, but I suspect it will deadlock for you.
On Tue, May 16, 2017 at 7:45 PM Scott Smith via Phabricator <
revi...@reviews.llvm.org> wrote:
> scott.smith added inline comments.
>
>
>
> Comment at: include/lldb/Utility/TaskPool.h:18-20
> + std::function cbs[sizeof...(T)]{tasks...};
> + llvm::parallel::for_each_n(llvm::parallel::par, static_cast(0),
> + sizeof...(T), [&cbs](size_t idx) {
> cbs[idx](); });
>
> zturner wrote:
> > I'm not sure this is the most efficient implementation. `std::function`
> has pretty poor performance, and there might be no need to even convert
> everything to `std::function` to begin with. You could make this a bit
> better by using `llvm::function_ref` instead.
> >
> > That said, I wonder if it's worth adding a function like this to
> `llvm::TaskGroup`? And you could just enqueue all the tasks, rather than
> `for_each_n`. Not sure if there would be a different in practice, what do
> you think?
> I'm not too worried about std::function vs llvm::function_ref; it isn't
> called often, and we still need allocations for the tasks that get
> enqueued. That said, there's no reason *to* use std::function, so I'll
> cahnge it.
>
> I like using for_each_n mostly to regularize the interface. For example,
> for_each_n/for_each can then optimize the type of TaskGroup it creates to
> ensure that it gets the right # of threads right away, rather than spawning
> up enough for full hardware concurrency. Or, if there are a lot of tasks
> (unlikely, but possible), then for_each can change to a model of enqueueing
> one task per thread, and having that thread loop using std::atomic to
> increment the iterator, which reduces allocations in TaskGroup and reduces
> lock contention (assuming TaskGroup doesn't use a lock free queue).
>
> i.e. the more things funnel through a single interface, the more we
> benefit from optimizing that one implementation.
>
> Also it means we can have for_each_n manage TaskGroups itself (maybe
> keeping one around for repeated use, then creating more as needed to
> support recursion, etc (more on that later)).
>
>
>
>
> Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1995-1996
>
> //--
> -TaskMapOverInt(0, num_compile_units, extract_fn);
> +llvm::parallel::for_each_n(llvm::parallel::par, 0U, num_compile_units,
> + extract_fn);
>
>
> zturner wrote:
> > What did you decide about the recursive parallelism? I don't know if
> that works yet using LLVM's default executor.
> 1. This code doesn't care.
> 2. It looks like it works, since (I think) for_each creates a separate
> TaskGroup for each call.
> 3. However I got a deadlock when using this for parallelizing the dynamic
> library loading itself, which used to work. That could either be due to
> other code changes, some oversight on my part, or it could be that
> for_each_n doesn't actually support recursion - which means that I
> misunderstood for_each_n. So I have more work to do...
>
>
> Repository:
> rL LLVM
>
> https://reviews.llvm.org/D33246
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33246: Remove most of lldb's TaskPool in favor of llvm's parallel functions
scott.smith added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1995-1996 //-- -TaskMapOverInt(0, num_compile_units, extract_fn); +llvm::parallel::for_each_n(llvm::parallel::par, 0U, num_compile_units, + extract_fn); scott.smith wrote: > zturner wrote: > > What did you decide about the recursive parallelism? I don't know if that > > works yet using LLVM's default executor. > 1. This code doesn't care. > 2. It looks like it works, since (I think) for_each creates a separate > TaskGroup for each call. > 3. However I got a deadlock when using this for parallelizing the dynamic > library loading itself, which used to work. That could either be due to > other code changes, some oversight on my part, or it could be that for_each_n > doesn't actually support recursion - which means that I misunderstood > for_each_n. So I have more work to do... On further inspection, llvm::parallel does not support recursion, since TaskGroup uses a single static Executor, and provides no way to override that (and besides, there's no way to pass parameters from for_each_n to the TaskGroup). That's fixable though, by making the default executor a thread local variable, so that worker threads can enqueue work to a different executor. Repository: rL LLVM https://reviews.llvm.org/D33246 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
