[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.
labath added a comment.
Seems very reasonable. A couple of details I noticed are in the inline comments.
As for the name, I'd suggest just dropping the `2` and letting overload
resolution handle that. If we don't need the interpreter argument, then maybe
we should aim for removing that overload in the future.
Comment at: lldb/include/lldb/Utility/TildeExpressionResolver.h:18
+namespace lldb_private {
+struct TildeExpressionResolver {
+public:
Is there a reason why one these is a struct and the other a class?
btw, if you move the destructor definition into the .cpp file, it will also
serve as a key method.
Comment at: lldb/source/Commands/CommandCompletions.cpp:112
+ if (!Resolver)
+Resolver = &SR;
+
amccarth wrote:
> This leaves the caller with no way to say the don't want a tilde resolver. I
> didn't expect that from reading the API. Perhaps a comment on the API could
> clarify that you're going to get this default behavior if you specify nullptr.
I agree with Adrian.
What do you think about an API like:
`DiskFilesOrDirectories(..., const TildeExpressionResolver &Resolver =
StandardTildeExpressionResolver())` ?
Comment at: lldb/source/Commands/CommandCompletions.cpp:116
+
+ llvm::SmallString CompletionBuffer;
+ llvm::SmallString Storage;
in `PosixApi.h` you define PATH_MAX as 32768, which sees a bit too much for a
stack object. As this doesn't actually impact correctness, should we use a more
down-to-earth value here?
Comment at: lldb/source/Commands/CommandCompletions.cpp:167
- if (remainder[0] == '\0' || strstr(name, remainder) == name) {
-if (strlen(name) + parameters->baselen >= PATH_MAX)
- return FileSpec::eEnumerateDirectoryResultNext;
+// We want to keep the form the user typed, so we special case this to
+// search
This comment could use some reformatting.
Comment at: lldb/source/Utility/TildeExpressionResolver.cpp:44
+#if defined(LLVM_ON_WIN32)
+ return false;
+#else
return 0;
Comment at: lldb/source/Utility/TildeExpressionResolver.cpp:49
+ struct passwd *user_entry;
+ Expr = Expr.drop_front();
+
Is this function allowed to be called with an empty `Expr`? The assert above
seems to indicate that is the case, but then this will be undefined.
https://reviews.llvm.org/D30789
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D29581: Initial implementation of SB APIs for Tracing support.
ravitheja added inline comments.
Comment at: scripts/interface/SBTrace.i:12
+
+class LLDB_API SBTrace {
+public:
clayborg wrote:
> Do we want something in here that explains what kind of trace buffer data you
> will be getting? I am thinking that even though we know we ask for
> "eTraceTypeProcessorTrace", this might not be enough for a plug-in to
> interpret these bytes. Do we need something like:
>
> ```
> class SBTrace {
> const char *GetTraceDataName();
> };
> ```
>
> And for intel it might return "intel processor trace version 2.0"? Or Maybe
> it would. be better as:
>
> ```
> class SBTrace {
> lldb::SBStructuredData GetTraceDataInfo();
> };
> ```
>
> This could return data that could be accessed as JSON. The version could be
> encoded in here. Maybe the exact CPU type, or CPU ID could also be encoded. I
> am guessing that the intel processor trace has already changed format from
> CPU to CPU and might also change in the future? This would help the plug-in
> that will interpret the trace byte to extract them correctly?
Hello, Couldn't this be done in the GetTraceConfig (currently I am doing this)
, as the TraceOptions already has a StructuredData for custom parameters and
configurations, any trace technology could just convey any special or trace
specific configurations in the custom parameters of TraceOptions ?
https://reviews.llvm.org/D29581
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30807: Use LLVM's directory enumeration code
labath added a comment. Comment at: lldb/source/Host/common/FileSpec.cpp:786 + continue; +if (!find_directories && fs::is_directory(Status)) + continue; This looks like it changes behavior. Previously, if `find_directories` was false this function would *not* recurse into them, whereas now it will. I guess you did not intend to do that (?) https://reviews.llvm.org/D30807 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30817: BreakpointResolverFileLine: Correct treatment of move-to-nearest-code for multiple modules
labath created this revision.
move-to-nearest-code needs special treatment to avoid creating
superfluous breakpoints in case multiple compilation units. It already
had code to handle the case when the compilation units are in the same
module, but it still did not properly handle the case when the
compilation units are in different modules.
This fixes the issue by manually iterating over the modules (instead of
just CUs) to make sure we aggregate the matches properly.
https://reviews.llvm.org/D30817
Files:
packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/Makefile
packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/TestMoveNearest.py
packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/foo.cpp
packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/foo.h
packages/Python/lldbsuite/test/functionalities/breakpoint/move_nearest/main.cpp
source/Breakpoint/BreakpointResolverFileLine.cpp
Index: source/Breakpoint/BreakpointResolverFileLine.cpp
===
--- source/Breakpoint/BreakpointResolverFileLine.cpp
+++ source/Breakpoint/BreakpointResolverFileLine.cpp
@@ -17,6 +17,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Symbol/CompileUnit.h"
#include "lldb/Symbol/Function.h"
+#include "lldb/Target/Target.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/StreamString.h"
@@ -112,40 +113,49 @@
BreakpointResolverFileLine::SearchCallback(SearchFilter &filter,
SymbolContext &context,
Address *addr, bool containing) {
+
SymbolContextList sc_list;
assert(m_breakpoint != NULL);
// There is a tricky bit here. You can have two compilation units that
- // #include the same file, and
- // in one of them the function at m_line_number is used (and so code and a
- // line entry for it is generated) but in the
- // other it isn't. If we considered the CU's independently, then in the
- // second inclusion, we'd move the breakpoint
- // to the next function that actually generated code in the header file. That
- // would end up being confusing.
- // So instead, we do the CU iterations by hand here, then scan through the
- // complete list of matches, and figure out
- // the closest line number match, and only set breakpoints on that match.
+ // #include the same file, and in one of them the function at m_line_number is
+ // used (and so code and a line entry for it is generated) but in the other it
+ // isn't. If we considered the CU's independently, then in the second
+ // inclusion, we'd move the breakpoint to the next function that actually
+ // generated code in the header file. That would end up being confusing. So
+ // instead, we do the CU iterations by hand here, then scan through the
+ // complete list of matches, and figure out the closest line number match, and
+ // only set breakpoints on that match.
// Note also that if file_spec only had a file name and not a directory, there
- // may be many different file spec's in
- // the resultant list. The closest line match for one will not be right for
- // some totally different file.
- // So we go through the match list and pull out the sets that have the same
- // file spec in their line_entry
- // and treat each set separately.
-
- const size_t num_comp_units = context.module_sp->GetNumCompileUnits();
- for (size_t i = 0; i < num_comp_units; i++) {
-CompUnitSP cu_sp(context.module_sp->GetCompileUnitAtIndex(i));
-if (cu_sp) {
- if (filter.CompUnitPasses(*cu_sp))
-cu_sp->ResolveSymbolContext(m_file_spec, m_line_number, m_inlines,
-m_exact_match, eSymbolContextEverything,
-sc_list);
+ // may be many different file spec's in the resultant list. The closest line
+ // match for one will not be right for some totally different file. So we go
+ // through the match list and pull out the sets that have the same file spec
+ // in their line_entry and treat each set separately.
+ const ModuleList &target_images = context.target_sp->GetImages();
+ std::lock_guard guard(target_images.GetMutex());
+
+ size_t n_modules = target_images.GetSize();
+ for (size_t i = 0; i < n_modules; i++) {
+// If this is the last level supplied, then call the callback directly,
+// otherwise descend.
+ModuleSP module_sp(target_images.GetModuleAtIndexUnlocked(i));
+if (!filter.ModulePasses(module_sp))
+ continue;
+
+const size_t num_comp_units = module_sp->GetNumCompileUnits();
+for (size_t i = 0; i < num_comp_units; i++) {
+ CompUnitSP cu_sp(module_sp->GetCompileUnitAtIndex(i));
+ if (! cu_sp || !filter.CompUnitPasses(*cu_sp))
+continue;
+
+ cu_sp->ResolveSymbolContext(m_file_spec, m_line_number, m_inlines,
+ m_exact_match, e
Re: [Lldb-commits] [PATCH] D30807: Use LLVM's directory enumeration code
You're right, I didn't notice that. But since you mention it, surely that had to have been a bug in the original implementation right? That flag isn't intended to be a synonym for "non recursive iteration ", because that's what the Next enumeration value is for. The algorithm would intentionally leave that decision up to the callback. Find directories seems to want to mean "should you call my callback with directories?" Imagine someone wants all files recursively but nothing else, that was impossible before, they would have to opt in to seeing each directory just so they could return Enter On Fri, Mar 10, 2017 at 2:27 AM Pavel Labath via Phabricator < revi...@reviews.llvm.org> wrote: > labath added a comment. > > > > > > > Comment at: lldb/source/Host/common/FileSpec.cpp:786 > + continue; > +if (!find_directories && fs::is_directory(Status)) > + continue; > > This looks like it changes behavior. Previously, if `find_directories` was > false this function would *not* recurse into them, whereas now it will. I > guess you did not intend to do that (?) > > > https://reviews.llvm.org/D30807 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D30807: Use LLVM's directory enumeration code
I can see both of them making sense, but I would actually prefer the new behavior you inadvertently introduced. I've looked at the callers and it seems no user sets find_directories to false, so I guess it's fine to change the behavior, if we want to. On 10 March 2017 at 14:08, Zachary Turner wrote: > You're right, I didn't notice that. But since you mention it, surely that > had to have been a bug in the original implementation right? That flag isn't > intended to be a synonym for "non recursive iteration ", because that's what > the Next enumeration value is for. The algorithm would intentionally leave > that decision up to the callback. > > Find directories seems to want to mean "should you call my callback with > directories?" > > Imagine someone wants all files recursively but nothing else, that was > impossible before, they would have to opt in to seeing each directory just > so they could return Enter > > On Fri, Mar 10, 2017 at 2:27 AM Pavel Labath via Phabricator > wrote: >> >> labath added a comment. >> >> >> >> >> >> >> Comment at: lldb/source/Host/common/FileSpec.cpp:786 >> + continue; >> +if (!find_directories && fs::is_directory(Status)) >> + continue; >> >> This looks like it changes behavior. Previously, if `find_directories` was >> false this function would *not* recurse into them, whereas now it will. I >> guess you did not intend to do that (?) >> >> >> https://reviews.llvm.org/D30807 >> >> >> > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.
zturner updated this revision to Diff 91361.
zturner added a comment.
Addressed suggestions from labath@
https://reviews.llvm.org/D30789
Files:
lldb/include/lldb/Interpreter/CommandCompletions.h
lldb/include/lldb/Utility/TildeExpressionResolver.h
lldb/source/Commands/CommandCompletions.cpp
lldb/source/Utility/CMakeLists.txt
lldb/source/Utility/TildeExpressionResolver.cpp
lldb/unittests/Interpreter/CMakeLists.txt
lldb/unittests/Interpreter/TestCompletion.cpp
Index: lldb/unittests/Interpreter/TestCompletion.cpp
===
--- /dev/null
+++ lldb/unittests/Interpreter/TestCompletion.cpp
@@ -0,0 +1,346 @@
+//===-- TestCompletion.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Core/StringList.h"
+#include "lldb/Interpreter/CommandCompletions.h"
+#include "lldb/Utility/TildeExpressionResolver.h"
+
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace fs = llvm::sys::fs;
+namespace path = llvm::sys::path;
+using namespace llvm;
+using namespace lldb_private;
+
+#define ASSERT_NO_ERROR(x) \
+ if (std::error_code ASSERT_NO_ERROR_ec = x) {\
+SmallString<128> MessageStorage; \
+raw_svector_ostream Message(MessageStorage); \
+Message << #x ": did not return errc::success.\n" \
+<< "error number: " << ASSERT_NO_ERROR_ec.value() << "\n" \
+<< "error message: " << ASSERT_NO_ERROR_ec.message() << "\n"; \
+GTEST_FATAL_FAILURE_(MessageStorage.c_str()); \
+ } else { \
+ }
+
+namespace {
+
+class MockTildeExpressionResolver : public TildeExpressionResolver {
+ StringRef CurrentUser;
+ StringMap UserDirectories;
+
+public:
+ explicit MockTildeExpressionResolver(StringRef CurrentUser, StringRef HomeDir)
+ : CurrentUser(CurrentUser) {
+UserDirectories.insert(std::make_pair(CurrentUser, HomeDir));
+ }
+
+ void AddKnownUser(StringRef User, StringRef HomeDir) {
+assert(UserDirectories.find(User) == UserDirectories.end());
+UserDirectories.insert(std::make_pair(User, HomeDir));
+ }
+
+ void Clear() {
+CurrentUser = StringRef();
+UserDirectories.clear();
+ }
+
+ void SetCurrentUser(StringRef User) {
+assert(UserDirectories.find(User) != UserDirectories.end());
+CurrentUser = User;
+ }
+
+ bool ResolveExact(StringRef Expr, SmallVectorImpl &Output) override {
+Output.clear();
+
+assert(!llvm::any_of(Expr, llvm::sys::path::is_separator));
+assert(Expr.empty() || Expr[0] == '~');
+Expr = Expr.drop_front();
+if (Expr.empty()) {
+ auto Dir = UserDirectories[CurrentUser];
+ Output.append(Dir.begin(), Dir.end());
+ return true;
+}
+
+for (const auto &User : UserDirectories) {
+ if (User.getKey() != Expr)
+continue;
+ Output.append(User.getValue().begin(), User.getValue().end());
+ return true;
+}
+return false;
+ }
+
+ bool ResolvePartial(StringRef Expr, StringSet<> &Output) override {
+Output.clear();
+
+assert(!llvm::any_of(Expr, llvm::sys::path::is_separator));
+assert(Expr.empty() || Expr[0] == '~');
+Expr = Expr.drop_front();
+
+SmallString<16> QualifiedName = "~";
+for (const auto &User : UserDirectories) {
+ if (!User.getKey().startswith(Expr))
+continue;
+ QualifiedName.resize(1);
+ QualifiedName.append(User.getKey().begin(), User.getKey().end());
+ Output.insert(QualifiedName);
+}
+
+return !Output.empty();
+ }
+};
+
+class CompletionTest : public testing::Test {
+protected:
+ /// Unique temporary directory in which all created filesystem entities must
+ /// be placed. It is removed at the end of the test suite.
+ static SmallString<128> BaseDir;
+
+ static SmallString<128> DirFoo;
+ static SmallString<128> DirFooA;
+ static SmallString<128> DirFooB;
+ static SmallString<128> DirFooC;
+ static SmallString<128> DirBar;
+ static SmallString<128> DirBaz;
+ static SmallString<128> DirTestFolder;
+
+ static SmallString<128> FileAA;
+ static SmallString<128> FileAB;
+ static SmallString<128> FileAC;
+ static SmallString<128> FileFoo;
+ static SmallString<128> FileBar;
+ static SmallString<128> FileBaz;
+
+ static void SetUpTestCase() {
+ASSERT_NO_ERROR(fs::createUniqueDirectory("FsCompletion", BaseDir));
+const char
[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.
zturner added inline comments. Comment at: lldb/source/Commands/CommandCompletions.cpp:112 + if (!Resolver) +Resolver = &SR; + labath wrote: > amccarth wrote: > > This leaves the caller with no way to say the don't want a tilde resolver. > > I didn't expect that from reading the API. Perhaps a comment on the API > > could clarify that you're going to get this default behavior if you specify > > nullptr. > I agree with Adrian. > What do you think about an API like: > `DiskFilesOrDirectories(..., const TildeExpressionResolver &Resolver = > StandardTildeExpressionResolver())` ? I don't actually think there *should* be a way to specify no resolver. That sounds like YAGNI. The only real use cases are "Do it for real" or "do it with a mock implementation". I've updated this to take a reference, which should indicate that it is required, and now the version that delegates to this function allocates a real instance in its own stack frame and passes it in. Hopefully this makes things more clear. https://reviews.llvm.org/D30789 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30817: BreakpointResolverFileLine: Correct treatment of move-to-nearest-code for multiple modules
jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Can you say more about the problem you are trying to solve. As far as breakpoints are concerned, if you find a match in each of several modules it seems to me you would always want to set a locations in each because modules can come and go independently. This also introduces an N^2 dependency on number of modules since the searcher is already looping over the modules. https://reviews.llvm.org/D30817 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30817: BreakpointResolverFileLine: Correct treatment of move-to-nearest-code for multiple modules
labath added a comment. In https://reviews.llvm.org/D30817#697960, @jingham wrote: > Can you say more about the problem you are trying to solve. Yes. Consider `foo.h` in the test case. Since the functions `foo1` and `foo2` are inline, they will only show up in the module if they are used. This means that the line table for module 1 will only contain entry for line 1 and module 2 will only contain line 2. If we treat each module separately, and we have move-to-nearest=true, we will set breakpoints on both lines, which are in two different functions and certainly not what the user had intended. This is basically an extension of the existing code, which treated the same case, but only when the two compilation units were in the same module. > As far as breakpoints are concerned, if you find a match in each of several > modules it seems to me you would always want to set a locations in each > because modules can come and go independently. Hm.. I had not considered that. I guess it's true that this would mean that adding or removing a module can affect how a breakpoint is resolved elsewhere, which does not seem ideal. I would still argue that this increases correctness, because then we will be misplacing the breakpoint only in some cases, where as now we do it always. I am open to suggestions though... > This also introduces an N^2 dependency on number of modules since the > searcher is already looping over the modules. I was under the impression that setting depth to `target` disables the iteration in the searcher. https://reviews.llvm.org/D30817 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D30789 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30817: BreakpointResolverFileLine: Correct treatment of move-to-nearest-code for multiple modules
jingham added a comment.
I missed that you had set this to target. I'd rather avoid doing that unless
there's good reason since it means we duplicate all the module iteration logic.
To my mind, within reason it's always better to be conservative and set too
many locations rather than too few. There's nothing the user can do to
construct a missing breakpoint - especially after they've missed it..., but we
have "move-to-nearest-code false" as the way for users to tell us to be more
radical in rejecting matches, and it is also simple to disable a location that
was errantly set.
And linking breakpoint location logic cross-module seems like a bad idea to me.
If I decide that Module A's version of the breakpoint wins over module B's in
the move, when A goes away there's nothing that will tell me to go revise that
decision for a module that hasn't changed (B). So this seems the wrong way to
solve this problem.
BTW, the formally correct way to solve this problem when
"move-to-nearest-code" is true is to reject moves that cross function
boundaries. IRL, you can't tell when a breakpoint leaves a function that
didn't get emitted since you have no way of knowing on what line it would have
ended without doing syntax analysis on the sources, and we can't assume sources
are available. But you should be able to tell when a moving a breakpoint by
line crosses INTO a new function because you have the decl_file, decl_line for
the function you would be moving it into. Last time I looked, the debug info
from clang wasn't quite right, however. If you have:
int
foo()
{
}
Then clang put the decl file & line on the "foo" line. So by those lights
moving from a breakpoint set on the "int" line would be disallowed as moving
across a function boundary. This is actually something I've seen people do
quite often - particularly when using a GUI. But I think it would actually be
good enough to establish a window, so that moving from "decl line -
fudge-factor" was still allowed, where fudge factor is 2 or 3 lines. That
would make most reasonable cases work as expected. It won't help for people
who write:
int short_func() {}
int other_short_func () {}
with no spaces. But for that is the sort of case the "move-to-nearest-code
false" is for.
This is actually something that's been on my plate to do for a while, but it is
kind of low on the list at present. If you really want to solve this problem,
the above suggestion would I think be a much better approach.
https://reviews.llvm.org/D30817
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r297496 - Simplify & correct the patch I wrote in r297441, after thinking
Author: jmolenda
Date: Fri Mar 10 13:31:54 2017
New Revision: 297496
URL: http://llvm.org/viewvc/llvm-project?rev=297496&view=rev
Log:
Simplify & correct the patch I wrote in r297441, after thinking
about this more I realized I could make the change isolated to
whether we decide an empty accelerator table is valid or not.
Modified:
lldb/trunk/include/lldb/Core/MappedHash.h
lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
Modified: lldb/trunk/include/lldb/Core/MappedHash.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/MappedHash.h?rev=297496&r1=297495&r2=297496&view=diff
==
--- lldb/trunk/include/lldb/Core/MappedHash.h (original)
+++ lldb/trunk/include/lldb/Core/MappedHash.h Fri Mar 10 13:31:54 2017
@@ -356,10 +356,6 @@ public:
m_header.bucket_count > 0;
}
-bool HasContent() const {
-return IsValid() && m_header.hashes_count > 0;
-}
-
uint32_t GetHashIndex(uint32_t bucket_idx) const {
if (m_hash_indexes && bucket_idx < m_header.bucket_count)
return m_hash_indexes[bucket_idx];
Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp?rev=297496&r1=297495&r2=297496&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp Fri Mar 10
13:31:54 2017
@@ -448,19 +448,19 @@ void SymbolFileDWARF::InitializeObject()
if (m_data_apple_names.m_data.GetByteSize() > 0) {
m_apple_names_ap.reset(new DWARFMappedHash::MemoryTable(
m_data_apple_names.m_data, get_debug_str_data(), ".apple_names"));
-if (!m_apple_names_ap->IsValid())
- m_apple_names_ap.reset();
-if (m_apple_names_ap->HasContent())
+if (m_apple_names_ap->IsValid())
m_using_apple_tables = true;
+else
+ m_apple_names_ap.reset();
}
get_apple_types_data();
if (m_data_apple_types.m_data.GetByteSize() > 0) {
m_apple_types_ap.reset(new DWARFMappedHash::MemoryTable(
m_data_apple_types.m_data, get_debug_str_data(), ".apple_types"));
-if (!m_apple_types_ap->IsValid())
- m_apple_types_ap.reset();
-if (m_apple_types_ap->HasContent())
+if (m_apple_types_ap->IsValid())
m_using_apple_tables = true;
+else
+ m_apple_types_ap.reset();
}
get_apple_namespaces_data();
@@ -468,20 +468,20 @@ void SymbolFileDWARF::InitializeObject()
m_apple_namespaces_ap.reset(new DWARFMappedHash::MemoryTable(
m_data_apple_namespaces.m_data, get_debug_str_data(),
".apple_namespaces"));
-if (!m_apple_namespaces_ap->IsValid())
- m_apple_namespaces_ap.reset();
-if (m_apple_namespaces_ap->HasContent())
+if (m_apple_namespaces_ap->IsValid())
m_using_apple_tables = true;
+else
+ m_apple_namespaces_ap.reset();
}
get_apple_objc_data();
if (m_data_apple_objc.m_data.GetByteSize() > 0) {
m_apple_objc_ap.reset(new DWARFMappedHash::MemoryTable(
m_data_apple_objc.m_data, get_debug_str_data(), ".apple_objc"));
-if (!m_apple_objc_ap->IsValid())
- m_apple_objc_ap.reset();
-if (m_apple_objc_ap->HasContent())
+if (m_apple_objc_ap->IsValid())
m_using_apple_tables = true;
+else
+ m_apple_objc_ap.reset();
}
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
jroelofs created this revision.
Herald added a subscriber: srhines.
Alternatively, I could teach llvm:set_thread_name() how to take a thread
parameter, and use that here instead.
https://reviews.llvm.org/D30844
Files:
source/Host/linux/HostThreadLinux.cpp
Index: source/Host/linux/HostThreadLinux.cpp
===
--- source/Host/linux/HostThreadLinux.cpp
+++ source/Host/linux/HostThreadLinux.cpp
@@ -23,7 +23,8 @@
: HostThreadPosix(thread) {}
void HostThreadLinux::SetName(lldb::thread_t thread, llvm::StringRef name) {
-#if (defined(__GLIBC__) && defined(_GNU_SOURCE)) || defined(__ANDROID__)
+#if (((__GLIBC__ > 2) || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 12)) && \
+ defined(_GNU_SOURCE)) || defined(__ANDROID__)
::pthread_setname_np(thread, name.data());
#else
(void)thread;
Index: source/Host/linux/HostThreadLinux.cpp
===
--- source/Host/linux/HostThreadLinux.cpp
+++ source/Host/linux/HostThreadLinux.cpp
@@ -23,7 +23,8 @@
: HostThreadPosix(thread) {}
void HostThreadLinux::SetName(lldb::thread_t thread, llvm::StringRef name) {
-#if (defined(__GLIBC__) && defined(_GNU_SOURCE)) || defined(__ANDROID__)
+#if (((__GLIBC__ > 2) || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 12)) && \
+ defined(_GNU_SOURCE)) || defined(__ANDROID__)
::pthread_setname_np(thread, name.data());
#else
(void)thread;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
zturner added a comment. Hmm.. I thought this code was gone? Are you at tip of trunk? If so then I must be crazy. I added `llvm::set_thread_name()` which already handles all this correctly, and I thought I converted all of LLDB's clients over to using the LLVM function. https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
jroelofs added a comment. Yeah, I'm on tip of trunk as of a few minutes ago. `llvm::set_thread_name()` exists, but it doesn't take a thread parameter like this one does (no idea if that's (still?) used or not in lldb). https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
zturner added a comment. It looks like I just forgot to remove those functions. All the callers have been updated to use the llvm method. Can you just delete this method (and the one in `HostThreadFreeBSD`? https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
jroelofs added a comment. sure https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
jroelofs added a comment. looks like lldb.xcodeproj refers to the files which I'll be deleting... do I need to worry about that? https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
zturner added a comment. I don't think you need to delete the entire files. Just the `SetName` function. https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
zturner added a comment. Oh wait, there's nothing else in those files Usually someone on the Apple team picks up the slack when changes are required to the Xcode project. If you use Xcode, you're welcome to remove them yourself. (Goes without saying, but make sure the build still succeeds after removing them) https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
jroelofs added a comment. erm, nevermind, I misread. https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
jroelofs updated this revision to Diff 91419.
jroelofs added a comment.
Herald added a subscriber: emaste.
Built successfully on linux (lots of tests fail, not sure if that's expected or
not?). I don't have a FreeBSD machine, but it looks "obviously" the same.
https://reviews.llvm.org/D30844
Files:
include/lldb/Host/freebsd/HostThreadFreeBSD.h
include/lldb/Host/linux/HostThreadLinux.h
source/Host/freebsd/HostThreadFreeBSD.cpp
source/Host/linux/HostThreadLinux.cpp
source/Plugins/Process/Linux/NativeThreadLinux.cpp
Index: source/Plugins/Process/Linux/NativeThreadLinux.cpp
===
--- source/Plugins/Process/Linux/NativeThreadLinux.cpp
+++ source/Plugins/Process/Linux/NativeThreadLinux.cpp
@@ -97,7 +97,7 @@
// const NativeProcessLinux *const process =
// reinterpret_cast (process_sp->get ());
llvm::SmallString<32> thread_name;
- HostNativeThread::GetName(GetID(), thread_name);
+ llvm::get_thread_name(thread_name);
return thread_name.c_str();
}
Index: source/Host/linux/HostThreadLinux.cpp
===
--- source/Host/linux/HostThreadLinux.cpp
+++ source/Host/linux/HostThreadLinux.cpp
@@ -8,13 +8,7 @@
//===--===//
#include "lldb/Host/linux/HostThreadLinux.h"
-#include "Plugins/Process/Linux/ProcFileReader.h"
-#include "lldb/Utility/DataBuffer.h"
-#include "llvm/ADT/SmallVector.h"
-
-#include
-
using namespace lldb_private;
HostThreadLinux::HostThreadLinux() : HostThreadPosix() {}
@@ -21,25 +15,3 @@
HostThreadLinux::HostThreadLinux(lldb::thread_t thread)
: HostThreadPosix(thread) {}
-
-void HostThreadLinux::SetName(lldb::thread_t thread, llvm::StringRef name) {
-#if (defined(__GLIBC__) && defined(_GNU_SOURCE)) || defined(__ANDROID__)
- ::pthread_setname_np(thread, name.data());
-#else
- (void)thread;
- (void)name;
-#endif
-}
-
-void HostThreadLinux::GetName(lldb::thread_t thread,
- llvm::SmallVectorImpl &name) {
- // Read /proc/$TID/comm file.
- lldb::DataBufferSP buf_sp =
- process_linux::ProcFileReader::ReadIntoDataBuffer(thread, "comm");
- const char *comm_str = (const char *)buf_sp->GetBytes();
- const char *cr_str = ::strchr(comm_str, '\n');
- size_t length = cr_str ? (cr_str - comm_str) : strlen(comm_str);
-
- name.clear();
- name.append(comm_str, comm_str + length);
-}
Index: source/Host/freebsd/HostThreadFreeBSD.cpp
===
--- source/Host/freebsd/HostThreadFreeBSD.cpp
+++ source/Host/freebsd/HostThreadFreeBSD.cpp
@@ -11,19 +11,6 @@
#include "lldb/Host/freebsd/HostThreadFreeBSD.h"
#include "lldb/Host/Host.h"
-// C includes
-#include
-#include
-#if defined(__FreeBSD__)
-#include
-#endif
-#include
-#include
-#include
-
-// C++ includes
-#include
-
using namespace lldb_private;
HostThreadFreeBSD::HostThreadFreeBSD() {}
@@ -30,41 +17,3 @@
HostThreadFreeBSD::HostThreadFreeBSD(lldb::thread_t thread)
: HostThreadPosix(thread) {}
-
-void HostThreadFreeBSD::GetName(lldb::tid_t tid,
-llvm::SmallVectorImpl &name) {
- name.clear();
- int pid = Host::GetCurrentProcessID();
-
- struct kinfo_proc *kp = nullptr, *nkp;
- size_t len = 0;
- int error;
- int ctl[4] = {CTL_KERN, KERN_PROC, KERN_PROC_PID | KERN_PROC_INC_THREAD,
-(int)pid};
-
- while (1) {
-error = sysctl(ctl, 4, kp, &len, nullptr, 0);
-if (kp == nullptr || (error != 0 && errno == ENOMEM)) {
- // Add extra space in case threads are added before next call.
- len += sizeof(*kp) + len / 10;
- nkp = (struct kinfo_proc *)realloc(kp, len);
- if (nkp == nullptr) {
-free(kp);
-return;
- }
- kp = nkp;
- continue;
-}
-if (error != 0)
- len = 0;
-break;
- }
-
- for (size_t i = 0; i < len / sizeof(*kp); i++) {
-if (kp[i].ki_tid == (lwpid_t)tid) {
- name.append(kp[i].ki_tdname, kp[i].ki_tdname + strlen(kp[i].ki_tdname));
- break;
-}
- }
- free(kp);
-}
Index: include/lldb/Host/linux/HostThreadLinux.h
===
--- include/lldb/Host/linux/HostThreadLinux.h
+++ include/lldb/Host/linux/HostThreadLinux.h
@@ -21,9 +21,6 @@
public:
HostThreadLinux();
HostThreadLinux(lldb::thread_t thread);
-
- static void SetName(lldb::thread_t thread, llvm::StringRef name);
- static void GetName(lldb::thread_t thread, llvm::SmallVectorImpl &name);
};
}
Index: include/lldb/Host/freebsd/HostThreadFreeBSD.h
===
--- include/lldb/Host/freebsd/HostThreadFreeBSD.h
+++ include/lldb/Host/freebsd/HostThreadFreeBSD.h
@@ -21,8 +21,6 @@
public:
HostThreadFreeBSD();
HostThreadFreeBSD(lldb::thread_t thread);
-
- static void GetName(lldb::tid_t tid, llvm::SmallVector
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
zturner added a comment. labath@ will need to comment on this. I wasn't aware that we were getting the thread name anywhere else. Im not sure if this code is even important, but hopefully it is not. The test suite is full of flakiness, but as long as it's the same before and after your change, it doesn't seem like anything is broken. https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
jroelofs added a subscriber: labath. jroelofs added a comment. @labath thoughts? https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
jroelofs added a comment. In https://reviews.llvm.org/D30844#698294, @zturner wrote: > The test suite is full of flakiness, but as long as it's the same before and > after your change, it doesn't seem like anything is broken. Results are pretty much the same, with 1 or 2 twinkling tests (couldn't say which ones it is though: I'm not at all familiar with lldb's test harness). Before: === Test Result Summary === Test Methods: 1897 Reruns: 14 Success: 877 Expected Failure:109 Failure: 306 Error:21 Exceptional Exit: 0 Unexpected Success: 15 Skip:568 Timeout: 1 Expected Timeout: 0 After: === Test Result Summary === Test Methods: 1897 Reruns: 14 Success: 876 Expected Failure:108 Failure: 307 Error:21 Exceptional Exit: 0 Unexpected Success: 16 Skip:568 Timeout: 1 Expected Timeout: 0 https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30006: [lldb] Remove the "MemoryHistory" plugin type and merge it into InstrumentationRuntime [NFC]
clayborg added a comment. Looks ok to me. Jim, are you ok with this? If Jim is OK with this, then I am. https://reviews.llvm.org/D30006 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12
labath is in Europe, so he will probably not see this until tonight. On Fri, Mar 10, 2017 at 3:31 PM Jonathan Roelofs via Phabricator < revi...@reviews.llvm.org> wrote: > jroelofs added a comment. > > In https://reviews.llvm.org/D30844#698294, @zturner wrote: > > > The test suite is full of flakiness, but as long as it's the same before > and after your change, it doesn't seem like anything is broken. > > > Results are pretty much the same, with 1 or 2 twinkling tests (couldn't > say which ones it is though: I'm not at all familiar with lldb's test > harness). > > Before: > > === > Test Result Summary > === > Test Methods: 1897 > Reruns: 14 > Success: 877 > Expected Failure:109 > Failure: 306 > Error:21 > Exceptional Exit: 0 > Unexpected Success: 15 > Skip:568 > Timeout: 1 > Expected Timeout: 0 > > After: > > === > Test Result Summary > === > Test Methods: 1897 > Reruns: 14 > Success: 876 > Expected Failure:108 > Failure: 307 > Error:21 > Exceptional Exit: 0 > Unexpected Success: 16 > Skip:568 > Timeout: 1 > Expected Timeout: 0 > > > https://reviews.llvm.org/D30844 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r297538 - fix xunit attribute parsing
Author: penryu
Date: Fri Mar 10 18:58:41 2017
New Revision: 297538
URL: http://llvm.org/viewvc/llvm-project?rev=297538&view=rev
Log:
fix xunit attribute parsing
Modified:
lldb/trunk/packages/Python/lldbsuite/test_event/formatter/xunit.py
Modified: lldb/trunk/packages/Python/lldbsuite/test_event/formatter/xunit.py
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test_event/formatter/xunit.py?rev=297538&r1=297537&r2=297538&view=diff
==
--- lldb/trunk/packages/Python/lldbsuite/test_event/formatter/xunit.py
(original)
+++ lldb/trunk/packages/Python/lldbsuite/test_event/formatter/xunit.py Fri Mar
10 18:58:41 2017
@@ -336,7 +336,7 @@ class XunitFormatter(ResultsFormatter):
test_event,
inner_content=(
''.format(
-"timeout",
+XunitFormatter._quote_attribute("timeout"),
XunitFormatter._quote_attribute(message))
))
with self.lock:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D30006: [lldb] Remove the "MemoryHistory" plugin type and merge it into InstrumentationRuntime [NFC]
jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. The InstrumentationRuntime implements a fairly specific model of something that might provide history threads. There has to be a module in the program that implements this, and a breakpoint that presumably triggers the report. What if you implemented this instead with dtrace behind the program's back? It wouldn't look like an InstrumentationRuntime plugin as currently sketched out. This doesn't necessarily mean that the MemoryHistory has to stand separate from the InstrumentationRuntime. Maybe there should be a more general base for InstrumentationRuntime that has the notion of providing memory histories but is less specific about how it goes about doing so? https://reviews.llvm.org/D30006 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
