[Lldb-commits] [PATCH] D47495: Support relative paths with less than two components in DBGSourcePathRemapping
labath added a comment.
I was also thinking whether this behavior needs to be conditional. If nothing
depends on this, then I'm all for changing the condition. However, my question
is whether "." is the only path we should treat this way. I'm thinking it would
be more consistent to give the root directory the same treatment too (so,
`"/".RemoveLastComponent() == "/"`, `"//net".RemoveLastComponent()=="//net"`,
etc). I guess you're unlikely to encounter an absolute path with less than two
components in the path mappings, but it sounds like this is the behavior you
would want there anyway.
Also, a test for the new behavior of the FileSpec function would be in order,
as there are some interesting corner cases which I am not sure you get right
(e.g, what is the value of `"foo".RemoveLastComponent()`?)
Comment at: source/Utility/FileSpec.cpp:796
}
+ if (*this == FileSpec(".", false))
+return;
This won't change the result of this particular comparison, but it's best to
get in the habit of specifying the path syntax when constructing FileSpecs.
"native" is not always the right choice.
https://reviews.llvm.org/D47495
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r333517 - Fix return value of DWARFUnit::ExtractDIEsIfNeeded()
Author: jankratochvil
Date: Wed May 30 01:54:46 2018
New Revision: 333517
URL: http://llvm.org/viewvc/llvm-project?rev=333517&view=rev
Log:
Fix return value of DWARFUnit::ExtractDIEsIfNeeded()
This is a leftover regression from: https://reviews.llvm.org/D46810
Modified:
lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp?rev=333517&r1=333516&r2=333517&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp Wed May 30
01:54:46 2018
@@ -192,13 +192,12 @@ bool DWARFUnit::ExtractDIEsIfNeeded() {
ExtractDIEsEndCheck(offset);
- if (!m_dwo_symbol_file)
-return m_die_array.size();
+ if (m_dwo_symbol_file) {
+DWARFUnit *dwo_cu = m_dwo_symbol_file->GetCompileUnit();
+dwo_cu->ExtractDIEsIfNeeded();
+ }
- DWARFUnit *dwo_cu = m_dwo_symbol_file->GetCompileUnit();
- size_t dwo_die_count = dwo_cu->ExtractDIEsIfNeeded();
- return m_die_array.size() + dwo_die_count -
- 1; // We have 2 CU die, but we want to count it only as one
+ return true;
}
//--
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil marked an inline comment as done. jankratochvil added a comment. FYI I also checked in a regression (just looking at the source code) https://reviews.llvm.org/rL333517. Repository: rL LLVM https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap
labath added a comment.
The idea that came to me while looking at this is testing this gdb-client
style. This would allow you to mock the server responses to allocation and e.g.
test handling of allocation failures. However, the problem is these tests sit
on top of SBAPI and there seems to be no way to issue "raw" allocation
requests through that (although maybe there is a case to be made for
SBProcess.AllocateMemory as a generally useful API).
However, if this does the job you want, then I'm fine with that too.
Comment at: tools/lldb-test/lldb-test.cpp:503
+ uint8_t Alignment;
+ int Matches = sscanf(Line.data(), "malloc %lu %hhu", &Size, &Alignment);
+ if (Matches != 2)
is `Line` null-terminated here? Also a size_t arg should have a `%zu` modifier,
but I am not sure if all msvc versions support that. It might be best to make
the type uint64_t and then use SCNu64.
Comment at: tools/lldb-test/lldb-test.cpp:536-542
+ bool Overlaps = AllocatedIntervals.lookup(Addr, false);
+ if (Size && !Overlaps)
+Overlaps = AllocatedIntervals.lookup(Addr + Size - 1, false);
+ if (Overlaps) {
+outs() << "Malloc error: overlapping allocation detected\n";
+exit(1);
+ }
It looks like this won't detect the case when a larger interval is placed on
top of a smaller one (e.g. `0x1000-0x4000` and `0x2000-0x3000`).
https://reviews.llvm.org/D47508
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r333526 - @skipUnlessDarwin TestOrderedSet
Author: labath Date: Wed May 30 03:04:32 2018 New Revision: 333526 URL: http://llvm.org/viewvc/llvm-project?rev=333526&view=rev Log: @skipUnlessDarwin TestOrderedSet Modified: lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py Modified: lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py?rev=333526&r1=333525&r2=333526&view=diff == --- lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py (original) +++ lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py Wed May 30 03:04:32 2018 @@ -6,6 +6,7 @@ from lldbsuite.test import lldbutil class TestOrderedSet(TestBase): mydir = TestBase.compute_mydir(__file__) + @skipUnlessDarwin def test_ordered_set(self): self.build() src_file = "main.m" ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r333465 - [ObjC] Fix the formatter for NSOrderedSet.
I've added a @skipUnlessDarwin to the new test. Right now we don't
have the ability to build or run ObjC tests on other platforms.
On Tue, 29 May 2018 at 23:57, Davide Italiano via lldb-commits
wrote:
>
> I would like to apologize, I forgot to `git add `the Makefile and this
> broke the bots. It should be fixed now. I'll keep an eye to make sure
> everything stays green.
> Sorry for the disruption, folks!
>
> --
> Davide
>
> On Tue, May 29, 2018 at 3:08 PM, Davide Italiano via lldb-commits
> wrote:
> > Author: davide
> > Date: Tue May 29 15:08:07 2018
> > New Revision: 333465
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=333465&view=rev
> > Log:
> > [ObjC] Fix the formatter for NSOrderedSet.
> >
> > While I'm here, delete some dead code.
> >
> >
> >
> > Added:
> > lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/
> >
> > lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py
> > lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/main.m
> > Modified:
> > lldb/trunk/source/Plugins/Language/ObjC/NSSet.cpp
> >
> > Added:
> > lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py
> > URL:
> > http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py?rev=333465&view=auto
> > ==
> > ---
> > lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py
> > (added)
> > +++
> > lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/TestOrderedSet.py
> > Tue May 29 15:08:07 2018
> > @@ -0,0 +1,17 @@
> > +import lldb
> > +from lldbsuite.test.decorators import *
> > +from lldbsuite.test.lldbtest import *
> > +from lldbsuite.test import lldbutil
> > +
> > +class TestOrderedSet(TestBase):
> > + mydir = TestBase.compute_mydir(__file__)
> > +
> > + def test_ordered_set(self):
> > +self.build()
> > +src_file = "main.m"
> > +src_file_spec = lldb.SBFileSpec(src_file)
> > +(target, process, thread, main_breakpoint) =
> > lldbutil.run_to_source_breakpoint(self,
> > + "break here", src_file_spec, exe_name = "a.out")
> > +frame = thread.GetSelectedFrame()
> > +self.expect("expr -d run -- orderedSet", substrs=["3 elements"])
> > +self.expect("expr -d run -- *orderedSet", substrs=["(int)1", "(int)2",
> > "(int)3"])
> >
> > Added: lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/main.m
> > URL:
> > http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/main.m?rev=333465&view=auto
> > ==
> > --- lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/main.m
> > (added)
> > +++ lldb/trunk/packages/Python/lldbsuite/test/lang/objc/orderedset/main.m
> > Tue May 29 15:08:07 2018
> > @@ -0,0 +1,8 @@
> > +#import
> > +
> > +int main() {
> > + NSOrderedSet *orderedSet =
> > + [NSOrderedSet orderedSetWithArray:@[@1,@2,@3,@1]];
> > + NSLog(@"%@",orderedSet);
> > + return 0; // break here
> > +}
> >
> > Modified: lldb/trunk/source/Plugins/Language/ObjC/NSSet.cpp
> > URL:
> > http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Language/ObjC/NSSet.cpp?rev=333465&r1=333464&r2=333465&view=diff
> > ==
> > --- lldb/trunk/source/Plugins/Language/ObjC/NSSet.cpp (original)
> > +++ lldb/trunk/source/Plugins/Language/ObjC/NSSet.cpp Tue May 29 15:08:07
> > 2018
> > @@ -269,7 +269,8 @@ bool lldb_private::formatters::NSSetSumm
> >if (!class_name || !*class_name)
> > return false;
> >
> > - if (!strcmp(class_name, "__NSSetI")) {
> > + if (!strcmp(class_name, "__NSSetI") ||
> > + !strcmp(class_name, "__NSOrderedSetI")) {
> > Status error;
> > value = process_sp->ReadUnsignedIntegerFromMemory(valobj_addr +
> > ptr_size,
> >ptr_size, 0, error);
> > @@ -289,32 +290,7 @@ bool lldb_private::formatters::NSSetSumm
> > }
> > if (error.Fail())
> >return false;
> > - }
> > - /*else if (!strcmp(class_name,"__NSCFSet"))
> > - {
> > - Status error;
> > - value = process_sp->ReadUnsignedIntegerFromMemory(valobj_addr +
> > (is_64bit ?
> > - 20 : 12), 4, 0, error);
> > - if (error.Fail())
> > - return false;
> > - if (is_64bit)
> > - value &= ~0x1fffUL;
> > - }
> > - else if (!strcmp(class_name,"NSCountedSet"))
> > - {
> > - Status error;
> > - value = process_sp->ReadUnsignedIntegerFromMemory(valobj_addr +
> > ptr_size,
> > - ptr_size, 0, error);
> > - if (error.Fail())
> > - return false;
> > - value = process_sp->ReadUnsignedIntegerFromMemory(value + (is_64bit ?
> > 20 :
> > - 12), 4, 0, error);
> > - if (error.Fail())
> > - return false;
> > - if (is_64bit)
> > - value
[Lldb-commits] [PATCH] D47495: Support relative paths with less than two components in DBGSourcePathRemapping
JDevlieghere updated this revision to Diff 149083.
JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.
- Replace custom logic with LLVM's path logic.
- Add tests.
https://reviews.llvm.org/D47495
Files:
include/lldb/Utility/FileSpec.h
source/Utility/FileSpec.cpp
unittests/Utility/FileSpecTest.cpp
Index: unittests/Utility/FileSpecTest.cpp
===
--- unittests/Utility/FileSpecTest.cpp
+++ unittests/Utility/FileSpecTest.cpp
@@ -320,3 +320,44 @@
}
}
+TEST(FileSpecTest, RemoveLastPathComponent) {
+ FileSpec fs_posix("/foo/bar/baz", false, FileSpec::Style::posix);
+ EXPECT_STREQ("/foo/bar/baz", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/foo", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/", fs_posix.GetCString());
+ EXPECT_FALSE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/", fs_posix.GetCString());
+
+ FileSpec fs_posix_relative("./foo/bar/baz", false, FileSpec::Style::posix);
+ EXPECT_STREQ("foo/bar/baz", fs_posix_relative.GetCString());
+ EXPECT_TRUE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo/bar", fs_posix_relative.GetCString());
+ EXPECT_TRUE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+ EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+
+ FileSpec fs_posix_relative2("./", false, FileSpec::Style::posix);
+ EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+ EXPECT_FALSE(fs_posix_relative2.RemoveLastPathComponent());
+ EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+ EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+
+ FileSpec fs_windows("C:\\foo\\bar\\baz", false, FileSpec::Style::windows);
+ EXPECT_STREQ("C:\\foo\\bar\\baz", fs_windows.GetCString());
+ EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:\\foo\\bar", fs_windows.GetCString());
+ EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:\\foo", fs_windows.GetCString());
+ EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:\\", fs_windows.GetCString());
+ EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:", fs_windows.GetCString());
+ EXPECT_FALSE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:", fs_windows.GetCString());
+}
Index: source/Utility/FileSpec.cpp
===
--- source/Utility/FileSpec.cpp
+++ source/Utility/FileSpec.cpp
@@ -785,36 +785,15 @@
return AppendPathComponent(new_path.GetPath(false));
}
-void FileSpec::RemoveLastPathComponent() {
- // CLEANUP: Use StringRef for string handling.
-
- const bool resolve = false;
- if (m_filename.IsEmpty() && m_directory.IsEmpty()) {
-SetFile("", resolve);
-return;
- }
- if (m_directory.IsEmpty()) {
-SetFile("", resolve);
-return;
+bool FileSpec::RemoveLastPathComponent() {
+ llvm::SmallString<64> current_path;
+ GetPath(current_path, false);
+ if (llvm::sys::path::has_parent_path(current_path, m_style)) {
+SetFile(llvm::sys::path::parent_path(current_path, m_style), false,
+m_style);
+return true;
}
- if (m_filename.IsEmpty()) {
-const char *dir_cstr = m_directory.GetCString();
-const char *last_slash_ptr = ::strrchr(dir_cstr, '/');
-
-// check for obvious cases before doing the full thing
-if (!last_slash_ptr) {
- SetFile("", resolve);
- return;
-}
-if (last_slash_ptr == dir_cstr) {
- SetFile("/", resolve);
- return;
-}
-size_t last_slash_pos = last_slash_ptr - dir_cstr + 1;
-ConstString new_path(dir_cstr, last_slash_pos);
-SetFile(new_path.GetCString(), resolve);
- } else
-SetFile(m_directory.GetCString(), resolve);
+ return false;
}
//--
/// Returns true if the filespec represents an implementation source
Index: include/lldb/Utility/FileSpec.h
===
--- include/lldb/Utility/FileSpec.h
+++ include/lldb/Utility/FileSpec.h
@@ -532,7 +532,14 @@
void AppendPathComponent(llvm::StringRef component);
void AppendPathComponent(const FileSpec &new_path);
- void RemoveLastPathComponent();
+ //--
+ /// Removes the last path component by replacing the current path with its
+ /// parent. When the current path has no parent, this is a no-op.
+ ///
+ /// @return
+ /// A boolean value indicating whether the path was updated.
+ //--
+ bo
[Lldb-commits] [PATCH] D47495: [FileSpec] Re-implmenet removeLastPathComponent
labath added inline comments.
Comment at: unittests/Utility/FileSpecTest.cpp:342
+ EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+
Is this the behavior you want here? I was thinking we could fold this all the
way to "." (arguably "." is a parent of "foo", though I can see how that may be
thought to be too magical)
https://reviews.llvm.org/D47495
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47495: [FileSpec] Re-implmenet removeLastPathComponent
JDevlieghere added inline comments.
Comment at: unittests/Utility/FileSpecTest.cpp:342
+ EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+
labath wrote:
> Is this the behavior you want here? I was thinking we could fold this all the
> way to "." (arguably "." is a parent of "foo", though I can see how that may
> be thought to be too magical)
I like this approach it doesn't consider `.` to be a special case and therefore
things are consistent and straightforward. My worry is that if we add special
cases, we risk ending up with missing edge cases (like the previous
implementation).
https://reviews.llvm.org/D47495
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47495: [FileSpec] Re-implmenet removeLastPathComponent
labath accepted this revision.
labath added inline comments.
Comment at: unittests/Utility/FileSpecTest.cpp:342
+ EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+
JDevlieghere wrote:
> labath wrote:
> > Is this the behavior you want here? I was thinking we could fold this all
> > the way to "." (arguably "." is a parent of "foo", though I can see how
> > that may be thought to be too magical)
> I like this approach it doesn't consider `.` to be a special case and
> therefore things are consistent and straightforward. My worry is that if we
> add special cases, we risk ending up with missing edge cases (like the
> previous implementation).
Ok, if that works for your use case (as far as path remappings go, "." is a
more generic mapping than "foo"), then that's fine by me. I like how you were
able to concisely describe the new semantics of this function.
https://reviews.llvm.org/D47495
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r333540 - [FileSpec] Re-implmenet removeLastPathComponent
Author: jdevlieghere
Date: Wed May 30 06:03:16 2018
New Revision: 333540
URL: http://llvm.org/viewvc/llvm-project?rev=333540&view=rev
Log:
[FileSpec] Re-implmenet removeLastPathComponent
When reading DBGSourcePathRemapping from a dSYM, we remove the last two
path components to make the source lookup more general. However, when
dealing with a relative path that has less than 2 components, we ended
up with an invalid (empty) FileSpec.
This patch changes the behavior of removeLastPathComponent to remove the
last path component, if possible. It does this by checking whether a
parent path exists, and if so using that as the new path. We rely
entirely on LLVM's path implementation to do the heavy lifting.
We now also return a boolean which indicates whether the operator was
successful or not.
Differential revision: https://reviews.llvm.org/D47495
rdar://37791687
Modified:
lldb/trunk/include/lldb/Utility/FileSpec.h
lldb/trunk/source/Utility/FileSpec.cpp
lldb/trunk/unittests/Utility/FileSpecTest.cpp
Modified: lldb/trunk/include/lldb/Utility/FileSpec.h
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/FileSpec.h?rev=333540&r1=333539&r2=333540&view=diff
==
--- lldb/trunk/include/lldb/Utility/FileSpec.h (original)
+++ lldb/trunk/include/lldb/Utility/FileSpec.h Wed May 30 06:03:16 2018
@@ -532,7 +532,14 @@ public:
void AppendPathComponent(llvm::StringRef component);
void AppendPathComponent(const FileSpec &new_path);
- void RemoveLastPathComponent();
+ //--
+ /// Removes the last path component by replacing the current path with its
+ /// parent. When the current path has no parent, this is a no-op.
+ ///
+ /// @return
+ /// A boolean value indicating whether the path was updated.
+ //--
+ bool RemoveLastPathComponent();
ConstString GetLastPathComponent() const;
Modified: lldb/trunk/source/Utility/FileSpec.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/FileSpec.cpp?rev=333540&r1=333539&r2=333540&view=diff
==
--- lldb/trunk/source/Utility/FileSpec.cpp (original)
+++ lldb/trunk/source/Utility/FileSpec.cpp Wed May 30 06:03:16 2018
@@ -785,36 +785,15 @@ void FileSpec::AppendPathComponent(const
return AppendPathComponent(new_path.GetPath(false));
}
-void FileSpec::RemoveLastPathComponent() {
- // CLEANUP: Use StringRef for string handling.
-
- const bool resolve = false;
- if (m_filename.IsEmpty() && m_directory.IsEmpty()) {
-SetFile("", resolve);
-return;
- }
- if (m_directory.IsEmpty()) {
-SetFile("", resolve);
-return;
+bool FileSpec::RemoveLastPathComponent() {
+ llvm::SmallString<64> current_path;
+ GetPath(current_path, false);
+ if (llvm::sys::path::has_parent_path(current_path, m_style)) {
+SetFile(llvm::sys::path::parent_path(current_path, m_style), false,
+m_style);
+return true;
}
- if (m_filename.IsEmpty()) {
-const char *dir_cstr = m_directory.GetCString();
-const char *last_slash_ptr = ::strrchr(dir_cstr, '/');
-
-// check for obvious cases before doing the full thing
-if (!last_slash_ptr) {
- SetFile("", resolve);
- return;
-}
-if (last_slash_ptr == dir_cstr) {
- SetFile("/", resolve);
- return;
-}
-size_t last_slash_pos = last_slash_ptr - dir_cstr + 1;
-ConstString new_path(dir_cstr, last_slash_pos);
-SetFile(new_path.GetCString(), resolve);
- } else
-SetFile(m_directory.GetCString(), resolve);
+ return false;
}
//--
/// Returns true if the filespec represents an implementation source
Modified: lldb/trunk/unittests/Utility/FileSpecTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/FileSpecTest.cpp?rev=333540&r1=333539&r2=333540&view=diff
==
--- lldb/trunk/unittests/Utility/FileSpecTest.cpp (original)
+++ lldb/trunk/unittests/Utility/FileSpecTest.cpp Wed May 30 06:03:16 2018
@@ -320,3 +320,44 @@ TEST(FileSpecTest, IsRelative) {
}
}
+TEST(FileSpecTest, RemoveLastPathComponent) {
+ FileSpec fs_posix("/foo/bar/baz", false, FileSpec::Style::posix);
+ EXPECT_STREQ("/foo/bar/baz", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/foo", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/", fs_posix.GetCString());
+ EXPECT_FALSE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/", fs_posix.GetCString());
+
+ FileSpec fs_posix_relative("./foo/bar/baz", fals
[Lldb-commits] [PATCH] D47495: [FileSpec] Re-implmenet removeLastPathComponent
This revision was automatically updated to reflect the committed changes.
Closed by commit rL333540: [FileSpec] Re-implmenet removeLastPathComponent
(authored by JDevlieghere, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D47495?vs=149083&id=149102#toc
Repository:
rL LLVM
https://reviews.llvm.org/D47495
Files:
lldb/trunk/include/lldb/Utility/FileSpec.h
lldb/trunk/source/Utility/FileSpec.cpp
lldb/trunk/unittests/Utility/FileSpecTest.cpp
Index: lldb/trunk/unittests/Utility/FileSpecTest.cpp
===
--- lldb/trunk/unittests/Utility/FileSpecTest.cpp
+++ lldb/trunk/unittests/Utility/FileSpecTest.cpp
@@ -320,3 +320,44 @@
}
}
+TEST(FileSpecTest, RemoveLastPathComponent) {
+ FileSpec fs_posix("/foo/bar/baz", false, FileSpec::Style::posix);
+ EXPECT_STREQ("/foo/bar/baz", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/foo/bar", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/foo", fs_posix.GetCString());
+ EXPECT_TRUE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/", fs_posix.GetCString());
+ EXPECT_FALSE(fs_posix.RemoveLastPathComponent());
+ EXPECT_STREQ("/", fs_posix.GetCString());
+
+ FileSpec fs_posix_relative("./foo/bar/baz", false, FileSpec::Style::posix);
+ EXPECT_STREQ("foo/bar/baz", fs_posix_relative.GetCString());
+ EXPECT_TRUE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo/bar", fs_posix_relative.GetCString());
+ EXPECT_TRUE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+ EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ("foo", fs_posix_relative.GetCString());
+
+ FileSpec fs_posix_relative2("./", false, FileSpec::Style::posix);
+ EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+ EXPECT_FALSE(fs_posix_relative2.RemoveLastPathComponent());
+ EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+ EXPECT_FALSE(fs_posix_relative.RemoveLastPathComponent());
+ EXPECT_STREQ(".", fs_posix_relative2.GetCString());
+
+ FileSpec fs_windows("C:\\foo\\bar\\baz", false, FileSpec::Style::windows);
+ EXPECT_STREQ("C:\\foo\\bar\\baz", fs_windows.GetCString());
+ EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:\\foo\\bar", fs_windows.GetCString());
+ EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:\\foo", fs_windows.GetCString());
+ EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:\\", fs_windows.GetCString());
+ EXPECT_TRUE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:", fs_windows.GetCString());
+ EXPECT_FALSE(fs_windows.RemoveLastPathComponent());
+ EXPECT_STREQ("C:", fs_windows.GetCString());
+}
Index: lldb/trunk/source/Utility/FileSpec.cpp
===
--- lldb/trunk/source/Utility/FileSpec.cpp
+++ lldb/trunk/source/Utility/FileSpec.cpp
@@ -785,36 +785,15 @@
return AppendPathComponent(new_path.GetPath(false));
}
-void FileSpec::RemoveLastPathComponent() {
- // CLEANUP: Use StringRef for string handling.
-
- const bool resolve = false;
- if (m_filename.IsEmpty() && m_directory.IsEmpty()) {
-SetFile("", resolve);
-return;
- }
- if (m_directory.IsEmpty()) {
-SetFile("", resolve);
-return;
+bool FileSpec::RemoveLastPathComponent() {
+ llvm::SmallString<64> current_path;
+ GetPath(current_path, false);
+ if (llvm::sys::path::has_parent_path(current_path, m_style)) {
+SetFile(llvm::sys::path::parent_path(current_path, m_style), false,
+m_style);
+return true;
}
- if (m_filename.IsEmpty()) {
-const char *dir_cstr = m_directory.GetCString();
-const char *last_slash_ptr = ::strrchr(dir_cstr, '/');
-
-// check for obvious cases before doing the full thing
-if (!last_slash_ptr) {
- SetFile("", resolve);
- return;
-}
-if (last_slash_ptr == dir_cstr) {
- SetFile("/", resolve);
- return;
-}
-size_t last_slash_pos = last_slash_ptr - dir_cstr + 1;
-ConstString new_path(dir_cstr, last_slash_pos);
-SetFile(new_path.GetCString(), resolve);
- } else
-SetFile(m_directory.GetCString(), resolve);
+ return false;
}
//--
/// Returns true if the filespec represents an implementation source
Index: lldb/trunk/include/lldb/Utility/FileSpec.h
===
--- lldb/trunk/include/lldb/Utility/FileSpec.h
+++ lldb/trunk/include/lldb/Utility/FileSpec.h
@@ -532,7 +532,14 @@
void AppendPathComponent(llvm::StringRef component);
void AppendPathComponent(const FileSpec &new_path);
- void RemoveLastPathComponent();
+ //--
+ /// Removes the last path co
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
JDevlieghere created this revision.
JDevlieghere added reviewers: clayborg, labath.
Herald added a subscriber: llvm-commits.
Support both lambda's and function pointers as arguments to EnumerateDirectory.
Repository:
rL LLVM
https://reviews.llvm.org/D47535
Files:
include/lldb/Utility/FileSpec.h
source/Utility/FileSpec.cpp
Index: source/Utility/FileSpec.cpp
===
--- source/Utility/FileSpec.cpp
+++ source/Utility/FileSpec.cpp
@@ -640,6 +640,18 @@
bool find_other,
EnumerateDirectoryCallbackType callback,
void *callback_baton) {
+ auto callbackLambda = [=](llvm::sys::fs::file_type file_type,
+const FileSpec &spec) -> EnumerateDirectoryResult {
+return callback(callback_baton, file_type, spec);
+ };
+ return EnumerateDirectory(dir_path, find_directories, find_files, find_other,
+callbackLambda);
+}
+
+void FileSpec::EnumerateDirectory(llvm::StringRef dir_path,
+ bool find_directories, bool find_files,
+ bool find_other,
+ EnumerateDirectoryCallbackFnType callback) {
namespace fs = llvm::sys::fs;
std::error_code EC;
fs::recursive_directory_iterator Iter(dir_path, EC);
@@ -657,7 +669,7 @@
continue;
FileSpec Spec(Item.path(), false);
-auto Result = callback(callback_baton, Status->type(), Spec);
+auto Result = callback(Status->type(), Spec);
if (Result == eEnumerateDirectoryResultQuit)
return;
if (Result == eEnumerateDirectoryResultNext) {
Index: include/lldb/Utility/FileSpec.h
===
--- include/lldb/Utility/FileSpec.h
+++ include/lldb/Utility/FileSpec.h
@@ -562,7 +562,12 @@
typedef std::function
- DirectoryCallback;
+ EnumerateDirectoryCallbackFnType;
+
+ static void EnumerateDirectory(llvm::StringRef dir_path,
+ bool find_directories, bool find_files,
+ bool find_other,
+ EnumerateDirectoryCallbackFnType callback);
protected:
//--
Index: source/Utility/FileSpec.cpp
===
--- source/Utility/FileSpec.cpp
+++ source/Utility/FileSpec.cpp
@@ -640,6 +640,18 @@
bool find_other,
EnumerateDirectoryCallbackType callback,
void *callback_baton) {
+ auto callbackLambda = [=](llvm::sys::fs::file_type file_type,
+const FileSpec &spec) -> EnumerateDirectoryResult {
+return callback(callback_baton, file_type, spec);
+ };
+ return EnumerateDirectory(dir_path, find_directories, find_files, find_other,
+callbackLambda);
+}
+
+void FileSpec::EnumerateDirectory(llvm::StringRef dir_path,
+ bool find_directories, bool find_files,
+ bool find_other,
+ EnumerateDirectoryCallbackFnType callback) {
namespace fs = llvm::sys::fs;
std::error_code EC;
fs::recursive_directory_iterator Iter(dir_path, EC);
@@ -657,7 +669,7 @@
continue;
FileSpec Spec(Item.path(), false);
-auto Result = callback(callback_baton, Status->type(), Spec);
+auto Result = callback(Status->type(), Spec);
if (Result == eEnumerateDirectoryResultQuit)
return;
if (Result == eEnumerateDirectoryResultNext) {
Index: include/lldb/Utility/FileSpec.h
===
--- include/lldb/Utility/FileSpec.h
+++ include/lldb/Utility/FileSpec.h
@@ -562,7 +562,12 @@
typedef std::function
- DirectoryCallback;
+ EnumerateDirectoryCallbackFnType;
+
+ static void EnumerateDirectory(llvm::StringRef dir_path,
+ bool find_directories, bool find_files,
+ bool find_other,
+ EnumerateDirectoryCallbackFnType callback);
protected:
//--
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
labath added a comment. Could we just get rid of the baton version? Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
JDevlieghere added a comment. In https://reviews.llvm.org/D47535#1116274, @labath wrote: > Could we just get rid of the baton version? It's the only way the function is used currently. How about just phasing it out when we touch the relevant code? Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
labath added a comment. Actually, I wonder if we shouldn't just deprecate this function altogether. What was your motivation for this patch? It seems we already have `llvm::fs::(recursive_)directory_iterator` for this purpose. It would be great if we could use that for all new code. Have you looked at that? Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47539: [Platform] Accept arbitrary kext variants
JDevlieghere created this revision.
JDevlieghere added reviewers: jasonmolenda, labath.
When loading kexts in `PlatformDarwinKernel`, we use the BundleID as the
filename to to create shared modules. In `GetSharedModule` we call
`ExamineKextForMatchingUUID` for any BundleID it finds that is a match, to see
if the UUID is also a match. Until now we were using
`Host::ResolveExecutableInBundle` whcih calls a CoreFoundation API to obtain
the executable. However, it's possible that the executable has a variant suffix
(e.g. foo_development) and these files were ignored.
This patch replaces that call with logic that looks for all the binaries in the
bundle. Because of the way `ExamineKextForMatchingUUID` works, it's fine to try
to load executables that are not valid and we can just iterate over the list
until we found a match.
(I put it up for review so I can get feedback while I look into adding a test)
https://reviews.llvm.org/D47539
Files:
source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -127,6 +127,9 @@
const lldb_private::FileSpec &file_spec,
bool recurse);
+ std::vector
+ SearchForExecutablesRecursively(const lldb_private::ConstString &dir);
+
static void AddKextToMap(PlatformDarwinKernel *thisp,
const lldb_private::FileSpec &file_spec);
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -779,35 +779,59 @@
return error;
}
+std::vector
+PlatformDarwinKernel::SearchForExecutablesRecursively(const ConstString &dir) {
+ std::vector executables;
+
+ auto callback = [&](llvm::sys::fs::file_type ft,
+ const lldb_private::FileSpec &file_spec)
+ -> lldb_private::FileSpec::EnumerateDirectoryResult {
+llvm::SmallString<64> file_path;
+file_spec.GetPath(file_path, false);
+if (llvm::sys::fs::can_execute(file_path))
+ executables.push_back(file_spec);
+return FileSpec::eEnumerateDirectoryResultNext;
+ };
+
+ const bool find_directories = false;
+ const bool find_files = true;
+ const bool find_other = false;
+ FileSpec::EnumerateDirectory(dir.GetCString(), find_directories, find_files,
+ find_other, callback);
+
+ return executables;
+}
+
Status PlatformDarwinKernel::ExamineKextForMatchingUUID(
const FileSpec &kext_bundle_path, const lldb_private::UUID &uuid,
const ArchSpec &arch, ModuleSP &exe_module_sp) {
- Status error;
- FileSpec exe_file = kext_bundle_path;
- Host::ResolveExecutableInBundle(exe_file);
- if (exe_file.Exists()) {
-ModuleSpec exe_spec(exe_file);
-exe_spec.GetUUID() = uuid;
-if (!uuid.IsValid()) {
- exe_spec.GetArchitecture() = arch;
-}
+ for (const auto &exe_file :
+ SearchForExecutablesRecursively(kext_bundle_path.GetDirectory())) {
+if (exe_file.Exists()) {
+ ModuleSpec exe_spec(exe_file);
+ exe_spec.GetUUID() = uuid;
+ if (!uuid.IsValid()) {
+exe_spec.GetArchitecture() = arch;
+ }
-// First try to create a ModuleSP with the file / arch and see if the UUID
-// matches. If that fails (this exec file doesn't have the correct uuid),
-// don't call GetSharedModule (which may call in to the DebugSymbols
-// framework and therefore can be slow.)
-ModuleSP module_sp(new Module(exe_spec));
-if (module_sp && module_sp->GetObjectFile() &&
-module_sp->MatchesModuleSpec(exe_spec)) {
- error = ModuleList::GetSharedModule(exe_spec, exe_module_sp, NULL, NULL,
- NULL);
- if (exe_module_sp && exe_module_sp->GetObjectFile()) {
-return error;
+ // First try to create a ModuleSP with the file / arch and see if the
UUID
+ // matches. If that fails (this exec file doesn't have the correct uuid),
+ // don't call GetSharedModule (which may call in to the DebugSymbols
+ // framework and therefore can be slow.)
+ ModuleSP module_sp(new Module(exe_spec));
+ if (module_sp && module_sp->GetObjectFile() &&
+ module_sp->MatchesModuleSpec(exe_spec)) {
+Status error = ModuleList::GetSharedModule(exe_spec, exe_module_sp,
+ NULL, NULL, NULL);
+if (exe_module_sp && exe_module_sp->GetObjectFile()) {
+ return error;
+}
}
+ exe_module_sp.reset();
}
-exe_module_sp.reset();
}
- return error;
+
+ return {}
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
JDevlieghere added a comment. In https://reviews.llvm.org/D47535#1116364, @labath wrote: > Actually, I wonder if we shouldn't just deprecate this function altogether. > What was your motivation for this patch? It seems we already have > `llvm::fs::(recursive_)directory_iterator` for this purpose. It would be > great if we could use that for all new code. Have you looked at that? My motivation is https://reviews.llvm.org/D47539. I could use the iterators directly but since the FileSpec one is based on them anyway (and adds some functionality that is actually useful) I figured I might as well use them for consistency. I'm not opposed to using the iterators directly, but won't that'll result in more code? Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
labath added a reviewer: zturner.
labath added a comment.
In https://reviews.llvm.org/D47535#1116392, @JDevlieghere wrote:
> In https://reviews.llvm.org/D47535#1116364, @labath wrote:
>
> > Actually, I wonder if we shouldn't just deprecate this function altogether.
> > What was your motivation for this patch? It seems we already have
> > `llvm::fs::(recursive_)directory_iterator` for this purpose. It would be
> > great if we could use that for all new code. Have you looked at that?
>
>
> My motivation is https://reviews.llvm.org/D47539. I could use the iterators
> directly but since the FileSpec one is based on them anyway (and adds some
> functionality that is actually useful) I figured I might as well use them for
> consistency. I'm not opposed to using the iterators directly, but won't that
> result in more code?
Looking back at the last refactor of this function
(https://reviews.llvm.org/D30807) I think the intention even then was to
deprecate/remove it altogether.
Also, I don't think that this would increase the amount of code. Looking at
your patch, it seems that it could be equivalently implemented using llvm
iterators as:
std::error_code EC;
for(llvm::sys::fs::recursive_directory_iterator Iter(dir.GetStringRef(), EC),
End; Iter != End && !EC ; Iter.incement(EC)) {
auto Status = Iter->status();
if (!Status)
break;
if (llvm::sys::fs::is_regular_file(*Status) &&
llvm::sys::fs::can_execute(Status->path())
executables.push_back(FileSpec(Status->path()));
}
which is (a tiny bit) shorter. I also find code with no callbacks more readable.
Repository:
rL LLVM
https://reviews.llvm.org/D47535
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
+1 I’d like to get rid of all EnumerateXXX with callback functions and
replace with iterator based equivalents. Given that in this case the
iterator version already exists, I definitely think we should try to use it
instead
On Wed, May 30, 2018 at 9:30 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:
> labath added a reviewer: zturner.
> labath added a comment.
>
> In https://reviews.llvm.org/D47535#1116392, @JDevlieghere wrote:
>
> > In https://reviews.llvm.org/D47535#1116364, @labath wrote:
> >
> > > Actually, I wonder if we shouldn't just deprecate this function
> altogether. What was your motivation for this patch? It seems we already
> have `llvm::fs::(recursive_)directory_iterator` for this purpose. It would
> be great if we could use that for all new code. Have you looked at that?
> >
> >
> > My motivation is https://reviews.llvm.org/D47539. I could use the
> iterators directly but since the FileSpec one is based on them anyway (and
> adds some functionality that is actually useful) I figured I might as well
> use them for consistency. I'm not opposed to using the iterators directly,
> but won't that result in more code?
>
>
> Looking back at the last refactor of this function (
> https://reviews.llvm.org/D30807) I think the intention even then was to
> deprecate/remove it altogether.
>
> Also, I don't think that this would increase the amount of code. Looking
> at your patch, it seems that it could be equivalently implemented using
> llvm iterators as:
>
> std::error_code EC;
> for(llvm::sys::fs::recursive_directory_iterator Iter(dir.GetStringRef(),
> EC), End; Iter != End && !EC ; Iter.incement(EC)) {
> auto Status = Iter->status();
> if (!Status)
> break;
> if (llvm::sys::fs::is_regular_file(*Status) &&
> llvm::sys::fs::can_execute(Status->path())
> executables.push_back(FileSpec(Status->path()));
> }
>
> which is (a tiny bit) shorter. I also find code with no callbacks more
> readable.
>
>
> Repository:
> rL LLVM
>
> https://reviews.llvm.org/D47535
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
JDevlieghere added a comment.
In https://reviews.llvm.org/D47535#1116430, @labath wrote:
> In https://reviews.llvm.org/D47535#1116392, @JDevlieghere wrote:
>
> > In https://reviews.llvm.org/D47535#1116364, @labath wrote:
> >
> > > Actually, I wonder if we shouldn't just deprecate this function
> > > altogether. What was your motivation for this patch? It seems we already
> > > have `llvm::fs::(recursive_)directory_iterator` for this purpose. It
> > > would be great if we could use that for all new code. Have you looked at
> > > that?
> >
> >
> > My motivation is https://reviews.llvm.org/D47539. I could use the iterators
> > directly but since the FileSpec one is based on them anyway (and adds some
> > functionality that is actually useful) I figured I might as well use them
> > for consistency. I'm not opposed to using the iterators directly, but won't
> > that result in more code?
>
>
> Looking back at the last refactor of this function
> (https://reviews.llvm.org/D30807) I think the intention even then was to
> deprecate/remove it altogether.
>
> Also, I don't think that this would increase the amount of code. Looking at
> your patch, it seems that it could be equivalently implemented using llvm
> iterators as:
>
> std::error_code EC;
> for(llvm::sys::fs::recursive_directory_iterator Iter(dir.GetStringRef(),
> EC), End; Iter != End && !EC ; Iter.incement(EC)) {
> auto Status = Iter->status();
> if (!Status)
> break;
> if (llvm::sys::fs::is_regular_file(*Status) &&
> llvm::sys::fs::can_execute(Status->path())
> executables.push_back(FileSpec(Status->path()));
> }
>
>
> which is (a tiny bit) shorter. I also find code with no callbacks more
> readable.
Fair enough, I'll update the patch :-)
Repository:
rL LLVM
https://reviews.llvm.org/D47535
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
zturner added a subscriber: JDevlieghere. zturner added a comment. +1 I’d like to get rid of all EnumerateXXX with callback functions and replace with iterator based equivalents. Given that in this case the iterator version already exists, I definitely think we should try to use it instead Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47539: [Platform] Accept arbitrary kext variants
JDevlieghere updated this revision to Diff 149143.
JDevlieghere added a comment.
Don't use `EnumerateDirectory`
https://reviews.llvm.org/D47539
Files:
source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -127,6 +127,9 @@
const lldb_private::FileSpec &file_spec,
bool recurse);
+ static std::vector
+ SearchForExecutablesRecursively(const lldb_private::ConstString &dir);
+
static void AddKextToMap(PlatformDarwinKernel *thisp,
const lldb_private::FileSpec &file_spec);
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -779,35 +779,53 @@
return error;
}
+std::vector
+PlatformDarwinKernel::SearchForExecutablesRecursively(const ConstString &dir) {
+ std::vector executables;
+ std::error_code EC;
+ for (llvm::sys::fs::recursive_directory_iterator it(dir.GetStringRef(), EC),
+ end;
+ it != end && !EC; it.increment(EC)) {
+auto status = it->status();
+if (!status)
+ break;
+if (llvm::sys::fs::is_regular_file(*status) &&
+llvm::sys::fs::can_execute(it->path()))
+ executables.emplace_back(it->path(), false);
+ }
+ return executables;
+}
+
Status PlatformDarwinKernel::ExamineKextForMatchingUUID(
const FileSpec &kext_bundle_path, const lldb_private::UUID &uuid,
const ArchSpec &arch, ModuleSP &exe_module_sp) {
- Status error;
- FileSpec exe_file = kext_bundle_path;
- Host::ResolveExecutableInBundle(exe_file);
- if (exe_file.Exists()) {
-ModuleSpec exe_spec(exe_file);
-exe_spec.GetUUID() = uuid;
-if (!uuid.IsValid()) {
- exe_spec.GetArchitecture() = arch;
-}
+ for (const auto &exe_file :
+ SearchForExecutablesRecursively(kext_bundle_path.GetDirectory())) {
+if (exe_file.Exists()) {
+ ModuleSpec exe_spec(exe_file);
+ exe_spec.GetUUID() = uuid;
+ if (!uuid.IsValid()) {
+exe_spec.GetArchitecture() = arch;
+ }
-// First try to create a ModuleSP with the file / arch and see if the UUID
-// matches. If that fails (this exec file doesn't have the correct uuid),
-// don't call GetSharedModule (which may call in to the DebugSymbols
-// framework and therefore can be slow.)
-ModuleSP module_sp(new Module(exe_spec));
-if (module_sp && module_sp->GetObjectFile() &&
-module_sp->MatchesModuleSpec(exe_spec)) {
- error = ModuleList::GetSharedModule(exe_spec, exe_module_sp, NULL, NULL,
- NULL);
- if (exe_module_sp && exe_module_sp->GetObjectFile()) {
-return error;
+ // First try to create a ModuleSP with the file / arch and see if the
UUID
+ // matches. If that fails (this exec file doesn't have the correct uuid),
+ // don't call GetSharedModule (which may call in to the DebugSymbols
+ // framework and therefore can be slow.)
+ ModuleSP module_sp(new Module(exe_spec));
+ if (module_sp && module_sp->GetObjectFile() &&
+ module_sp->MatchesModuleSpec(exe_spec)) {
+Status error = ModuleList::GetSharedModule(exe_spec, exe_module_sp,
+ NULL, NULL, NULL);
+if (exe_module_sp && exe_module_sp->GetObjectFile()) {
+ return error;
+}
}
+ exe_module_sp.reset();
}
-exe_module_sp.reset();
}
- return error;
+
+ return {};
}
bool PlatformDarwinKernel::GetSupportedArchitectureAtIndex(uint32_t idx,
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -127,6 +127,9 @@
const lldb_private::FileSpec &file_spec,
bool recurse);
+ static std::vector
+ SearchForExecutablesRecursively(const lldb_private::ConstString &dir);
+
static void AddKextToMap(PlatformDarwinKernel *thisp,
const lldb_private::FileSpec &file_spec);
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -779,35 +779,53 @@
return error;
}
+std::vector
+PlatformDarwinKernel::SearchForExecutablesRecursiv
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
JDevlieghere abandoned this revision. JDevlieghere added a comment. Alright, let's abandon this and replace the existing uses with llvm iterators. Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap
vsk added inline comments.
Comment at: tools/lldb-test/lldb-test.cpp:503
+ uint8_t Alignment;
+ int Matches = sscanf(Line.data(), "malloc %lu %hhu", &Size, &Alignment);
+ if (Matches != 2)
labath wrote:
> is `Line` null-terminated here? Also a size_t arg should have a `%zu`
> modifier, but I am not sure if all msvc versions support that. It might be
> best to make the type uint64_t and then use SCNu64.
Yes, `Line` is null-terminated because `MemoryBuffer::getFileOrSTDIN` defaults
to adding a null terminator.
Comment at: tools/lldb-test/lldb-test.cpp:503
+ uint8_t Alignment;
+ int Matches = sscanf(Line.data(), "malloc %lu %hhu", &Size, &Alignment);
+ if (Matches != 2)
vsk wrote:
> labath wrote:
> > is `Line` null-terminated here? Also a size_t arg should have a `%zu`
> > modifier, but I am not sure if all msvc versions support that. It might be
> > best to make the type uint64_t and then use SCNu64.
> Yes, `Line` is null-terminated because `MemoryBuffer::getFileOrSTDIN`
> defaults to adding a null terminator.
LLVM currently requires MSVC >= 2015 Update 3 (see:
https://reviews.llvm.org/D47073), which supports %zu (see:
https://blogs.msdn.microsoft.com/vcblog/2014/06/03/visual-studio-14-ctp/#div-comment-77743).
I'll just use %zu.
Comment at: tools/lldb-test/lldb-test.cpp:536-542
+ bool Overlaps = AllocatedIntervals.lookup(Addr, false);
+ if (Size && !Overlaps)
+Overlaps = AllocatedIntervals.lookup(Addr + Size - 1, false);
+ if (Overlaps) {
+outs() << "Malloc error: overlapping allocation detected\n";
+exit(1);
+ }
labath wrote:
> It looks like this won't detect the case when a larger interval is placed on
> top of a smaller one (e.g. `0x1000-0x4000` and `0x2000-0x3000`).
Thanks for pointing this out. I wasn't sure how to do this efficiently. Taking
another look at things, it looks like IntervalMap surfaces iterators which can
be used to scan a range quickly. I'll try that out.
https://reviews.llvm.org/D47508
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap
vsk updated this revision to Diff 149159.
vsk edited the summary of this revision.
vsk added a comment.
- Use %zu, and improve detection of overlapping allocations.
https://reviews.llvm.org/D47508
Files:
lit/Expr/TestIRMemoryMap.test
source/Target/Process.cpp
tools/lldb-test/lldb-test.cpp
Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -15,37 +15,53 @@
#include "lldb/Core/Debugger.h"
#include "lldb/Core/Module.h"
#include "lldb/Core/Section.h"
+#include "lldb/Expression/IRMemoryMap.h"
#include "lldb/Initialization/SystemLifetimeManager.h"
#include "lldb/Interpreter/CommandInterpreter.h"
#include "lldb/Interpreter/CommandReturnObject.h"
#include "lldb/Symbol/ClangASTContext.h"
#include "lldb/Symbol/ClangASTImporter.h"
#include "lldb/Symbol/SymbolVendor.h"
#include "lldb/Symbol/TypeList.h"
#include "lldb/Symbol/VariableList.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
#include "lldb/Utility/CleanUp.h"
#include "lldb/Utility/DataExtractor.h"
#include "lldb/Utility/StreamString.h"
+#include "llvm/ADT/IntervalMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/MathExtras.h"
#include "llvm/Support/ManagedStatic.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/PrettyStackTrace.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/WithColor.h"
+#include
#include
using namespace lldb;
using namespace lldb_private;
using namespace llvm;
namespace opts {
+
static cl::SubCommand BreakpointSubcommand("breakpoints",
"Test breakpoint resolution");
cl::SubCommand ModuleSubcommand("module-sections",
"Display LLDB Module Information");
cl::SubCommand SymbolsSubcommand("symbols", "Dump symbols for an object file");
+cl::SubCommand IRMemoryMapSubcommand("ir-memory-map", "Test IRMemoryMap");
+cl::opt Log("log", cl::desc("Path to a log file"), cl::init(""),
+ cl::sub(IRMemoryMapSubcommand));
+
+/// Create a target using the file pointed to by \p Filename, or abort.
+TargetSP createTarget(Debugger &Dbg, const std::string &Filename);
+
+/// Read \p Filename into a null-terminated buffer, or abort.
+std::unique_ptr openFile(const std::string &Filename);
namespace breakpoint {
static cl::opt Target(cl::Positional, cl::desc(""),
@@ -135,8 +151,47 @@
static int dumpSymbols(Debugger &Dbg);
}
+
+namespace irmemorymap {
+static cl::opt Target(cl::Positional, cl::desc(""),
+ cl::Required,
+ cl::sub(IRMemoryMapSubcommand));
+static cl::opt CommandFile(cl::Positional,
+cl::desc(""),
+cl::init("-"),
+cl::sub(IRMemoryMapSubcommand));
+using AddrIntervalMap =
+ IntervalMap>;
+bool evalMalloc(IRMemoryMap &IRMemMap, StringRef Line,
+AddrIntervalMap &AllocatedIntervals);
+int evaluateMemoryMapCommands(Debugger &Dbg);
+} // namespace irmemorymap
+
} // namespace opts
+TargetSP opts::createTarget(Debugger &Dbg, const std::string &Filename) {
+ TargetSP Target;
+ Status ST =
+ Dbg.GetTargetList().CreateTarget(Dbg, Filename, /*triple*/ "",
+ /*get_dependent_modules*/ false,
+ /*platform_options*/ nullptr, Target);
+ if (ST.Fail()) {
+errs() << formatv("Failed to create target '{0}: {1}\n", Filename, ST);
+exit(1);
+ }
+ return Target;
+}
+
+std::unique_ptr opts::openFile(const std::string &Filename) {
+ auto MB = MemoryBuffer::getFileOrSTDIN(Filename);
+ if (!MB) {
+errs() << formatv("Could not open file '{0}: {1}\n", Filename,
+ MB.getError().message());
+exit(1);
+ }
+ return std::move(*MB);
+}
+
void opts::breakpoint::dumpState(const BreakpointList &List, LinePrinter &P) {
P.formatLine("{0} breakpoint{1}", List.GetSize(), plural(List.GetSize()));
if (List.GetSize() > 0)
@@ -177,7 +232,7 @@
switch (Cmd[0]) {
case '%':
if (Cmd.consume_front("%p") && (Cmd.empty() || !isalnum(Cmd[0]))) {
-OS << sys::path::parent_path(CommandFile);
+OS << sys::path::parent_path(breakpoint::CommandFile);
break;
}
// fall through
@@ -192,26 +247,11 @@
}
int opts::breakpoint::evaluateBreakpoints(Debugger &Dbg) {
- TargetSP Target;
- Status ST =
- Dbg.GetTargetList().CreateTarget(Dbg, breakpoint::Target, /*triple*/ "",
- /*get_dependent_modules*/ false,
- /*platform_options*/ nullptr, Target);
- if (ST.Fail()) {
-errs() << formatv("Failed to create target '{0}: {1}\n", breakpoint::Target,
- ST);
-
[Lldb-commits] [PATCH] D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap
vsk updated this revision to Diff 149173.
vsk edited the summary of this revision.
vsk added a comment.
- Really fix the allocation overlap test. The previous version of this patch
would not detect overlaps in which the end of the new allocation is contained
within an existing allocation.
> The idea that came to me while looking at this is testing this gdb-client
> style. This would allow you to mock the server responses to allocation and
> e.g. test handling of allocation failures. However, the problem is these
> tests sit on top of SBAPI and there seems to be no way to issue "raw"
> allocation requests through that (although maybe there is a case to be made
> for SBProcess.AllocateMemory as a generally useful API).
>
> However, if this does the job you want, then I'm fine with that too.
Testing at this level looks to be sufficient to uncover the bugs I'm concerned
about, so I'd prefer not to extend the SB API if possible.
https://reviews.llvm.org/D47508
Files:
lit/Expr/TestIRMemoryMap.test
source/Target/Process.cpp
tools/lldb-test/lldb-test.cpp
Index: tools/lldb-test/lldb-test.cpp
===
--- tools/lldb-test/lldb-test.cpp
+++ tools/lldb-test/lldb-test.cpp
@@ -15,37 +15,53 @@
#include "lldb/Core/Debugger.h"
#include "lldb/Core/Module.h"
#include "lldb/Core/Section.h"
+#include "lldb/Expression/IRMemoryMap.h"
#include "lldb/Initialization/SystemLifetimeManager.h"
#include "lldb/Interpreter/CommandInterpreter.h"
#include "lldb/Interpreter/CommandReturnObject.h"
#include "lldb/Symbol/ClangASTContext.h"
#include "lldb/Symbol/ClangASTImporter.h"
#include "lldb/Symbol/SymbolVendor.h"
#include "lldb/Symbol/TypeList.h"
#include "lldb/Symbol/VariableList.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
#include "lldb/Utility/CleanUp.h"
#include "lldb/Utility/DataExtractor.h"
#include "lldb/Utility/StreamString.h"
+#include "llvm/ADT/IntervalMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/MathExtras.h"
#include "llvm/Support/ManagedStatic.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/PrettyStackTrace.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/WithColor.h"
+#include
#include
using namespace lldb;
using namespace lldb_private;
using namespace llvm;
namespace opts {
+
static cl::SubCommand BreakpointSubcommand("breakpoints",
"Test breakpoint resolution");
cl::SubCommand ModuleSubcommand("module-sections",
"Display LLDB Module Information");
cl::SubCommand SymbolsSubcommand("symbols", "Dump symbols for an object file");
+cl::SubCommand IRMemoryMapSubcommand("ir-memory-map", "Test IRMemoryMap");
+cl::opt Log("log", cl::desc("Path to a log file"), cl::init(""),
+ cl::sub(IRMemoryMapSubcommand));
+
+/// Create a target using the file pointed to by \p Filename, or abort.
+TargetSP createTarget(Debugger &Dbg, const std::string &Filename);
+
+/// Read \p Filename into a null-terminated buffer, or abort.
+std::unique_ptr openFile(const std::string &Filename);
namespace breakpoint {
static cl::opt Target(cl::Positional, cl::desc(""),
@@ -135,8 +151,49 @@
static int dumpSymbols(Debugger &Dbg);
}
+
+namespace irmemorymap {
+static cl::opt Target(cl::Positional, cl::desc(""),
+ cl::Required,
+ cl::sub(IRMemoryMapSubcommand));
+static cl::opt CommandFile(cl::Positional,
+cl::desc(""),
+cl::init("-"),
+cl::sub(IRMemoryMapSubcommand));
+using AllocationT = std::pair;
+bool areAllocationsOverlapping(const AllocationT &L, const AllocationT &R);
+using AddrIntervalMap =
+ IntervalMap>;
+bool evalMalloc(IRMemoryMap &IRMemMap, StringRef Line,
+AddrIntervalMap &AllocatedIntervals);
+int evaluateMemoryMapCommands(Debugger &Dbg);
+} // namespace irmemorymap
+
} // namespace opts
+TargetSP opts::createTarget(Debugger &Dbg, const std::string &Filename) {
+ TargetSP Target;
+ Status ST =
+ Dbg.GetTargetList().CreateTarget(Dbg, Filename, /*triple*/ "",
+ /*get_dependent_modules*/ false,
+ /*platform_options*/ nullptr, Target);
+ if (ST.Fail()) {
+errs() << formatv("Failed to create target '{0}: {1}\n", Filename, ST);
+exit(1);
+ }
+ return Target;
+}
+
+std::unique_ptr opts::openFile(const std::string &Filename) {
+ auto MB = MemoryBuffer::getFileOrSTDIN(Filename);
+ if (!MB) {
+errs() << formatv("Could not open file '{0}: {1}\n", Filename,
+ MB.getError().message());
+exit(1);
+ }
+ return std::move(*MB);
+}
+
void opts::breakpoint::dumpState(const BreakpointList &List, LinePrinter &P)
[Lldb-commits] [PATCH] D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
Thank you for making the changes. This looks fine to me. The more testing, the
better.
Comment at: tools/lldb-test/lldb-test.cpp:532
+ // Print the result of the allocation before checking its validity.
+ outs() << format("Malloc: address = 0x%x\n", Addr);
+
`%x` is not right here. Maybe use `formatv`?
https://reviews.llvm.org/D47508
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc
vsk created this revision.
vsk added reviewers: labath, zturner, jingham, aprantl.
vsk edited the summary of this revision.
This prevents Malloc from allocating the same chunk of memory twice, as
a byproduct of an alignment adjustment which gave the client access to
unallocated memory.
Prior to this patch, the newly-added test failed with:
$ lldb-test ir-memory-map ... ir-memory-map-overlap1.test
Command: malloc(size=8, alignment=16)
Malloc: address = 0x1000cd000
Command: malloc(size=16, alignment=8)
Malloc: address = 0x1000cd010
Command: malloc(size=64, alignment=32)
Malloc: address = 0x1000cd020
Command: malloc(size=1, alignment=8)
Malloc: address = 0x1000cd060
Command: malloc(size=64, alignment=32)
Malloc: address = 0x1000cd080
Command: malloc(size=64, alignment=8)
Malloc: address = 0x1000cd0b0
Malloc error: overlapping allocation detected, previous allocation at
[0x1000cd080, 0x1000cd0c0)
I don't see anything controversial here (in fact Jim lgtm'd part of this patch
off-list), but as this is unfamiliar territory for me I think it'd help to have
a proper review. Depends on https://reviews.llvm.org/D47508.
https://reviews.llvm.org/D47551
Files:
lit/Expr/Inputs/ir-memory-map-basic.test
lit/Expr/Inputs/ir-memory-map-overlap1.test
lit/Expr/TestIRMemoryMap.test
source/Expression/IRMemoryMap.cpp
Index: source/Expression/IRMemoryMap.cpp
===
--- source/Expression/IRMemoryMap.cpp
+++ source/Expression/IRMemoryMap.cpp
@@ -304,12 +304,25 @@
size_t alignment_mask = alignment - 1;
size_t allocation_size;
- if (size == 0)
+ if (size == 0) {
+// FIXME: Malloc(0) should either return an invalid address or assert, in
+// order to cut down on unnecessary allocations.
allocation_size = alignment;
- else
-allocation_size = (size & alignment_mask)
- ? ((size + alignment) & (~alignment_mask))
- : size;
+ } else {
+// Round up the requested size to an aligned value, if needed.
+if (size & alignment_mask)
+ allocation_size = ((size + alignment) & (~alignment_mask));
+else
+ allocation_size = size;
+
+// The process page cache does not see the requested alignment. We can't
+// assume its result will be any more than 1-byte aligned. To work around
+// this, request `alignment` additional bytes.
+//
+// FIXME: Pass the requested alignment into the process page cache to
+// reduce internal fragmentation.
+allocation_size += alignment;
+ }
switch (policy) {
default:
Index: lit/Expr/TestIRMemoryMap.test
===
--- lit/Expr/TestIRMemoryMap.test
+++ lit/Expr/TestIRMemoryMap.test
@@ -1,28 +1,3 @@
# RUN: %cxx %p/Inputs/call-function.cpp -g -o %t
-# RUN: lldb-test ir-memory-map %t %s
-
-malloc 0 1
-malloc 1 1
-
-malloc 2 1
-malloc 2 2
-malloc 2 4
-
-malloc 3 1
-malloc 3 2
-malloc 3 4
-
-malloc 128 1
-malloc 128 2
-malloc 128 4
-malloc 128 128
-
-malloc 2048 1
-malloc 2048 2
-malloc 2048 4
-
-malloc 3968 1
-malloc 3968 2
-malloc 3968 4
-
-malloc 0 1
+# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic.test
+# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-overlap1.test
Index: lit/Expr/Inputs/ir-memory-map-overlap1.test
===
--- /dev/null
+++ lit/Expr/Inputs/ir-memory-map-overlap1.test
@@ -0,0 +1,10 @@
+malloc 8 16
+malloc 16 8
+malloc 64 32
+malloc 1 8
+malloc 64 32
+malloc 64 8
+malloc 1024 32
+malloc 1 16
+malloc 8 16
+malloc 1024 16
\ No newline at end of file
Index: lit/Expr/Inputs/ir-memory-map-basic.test
===
--- /dev/null
+++ lit/Expr/Inputs/ir-memory-map-basic.test
@@ -0,0 +1,25 @@
+malloc 0 1
+malloc 1 1
+
+malloc 2 1
+malloc 2 2
+malloc 2 4
+
+malloc 3 1
+malloc 3 2
+malloc 3 4
+
+malloc 128 1
+malloc 128 2
+malloc 128 4
+malloc 128 128
+
+malloc 2048 1
+malloc 2048 2
+malloc 2048 4
+
+malloc 3968 1
+malloc 3968 2
+malloc 3968 4
+
+malloc 0 1
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r333583 - [lldb-test] Add a testing harness for the JIT's IRMemoryMap
Author: vedantk
Date: Wed May 30 12:39:10 2018
New Revision: 333583
URL: http://llvm.org/viewvc/llvm-project?rev=333583&view=rev
Log:
[lldb-test] Add a testing harness for the JIT's IRMemoryMap
This teaches lldb-test how to launch a process, set up an IRMemoryMap,
and issue memory allocations in the target process through the map. This
makes it possible to test IRMemoryMap in a targeted way.
This has uncovered two bugs so far. The first bug is that Malloc
performs an adjustment on the pointer returned from AllocateMemory (for
alignment purposes) which ultimately allows overlapping memory regions
to be created. The second bug is that after most of the address space on
the host side is exhausted, Malloc may return the same address multiple
times. These bugs (and hopefully more!) can be uncovered and tested for
with targeted lldb-test commands.
At an even higher level, the motivation for addressing these bugs is
that they can lead to strange user-visible failures (e.g, variables
assume the wrong value during expression evaluation, or the debugger
crashes). See my third comment on this swift-lldb PR for an example:
https://github.com/apple/swift-lldb/pull/652
I hope lldb-test is the right place to add this testing harness. Setting
up a gtest-style unit test proved too cumbersome (you need to recreate
or mock way too much debugger state), as did writing end-to-end tests
(it's hard to write a test that actually hits a buggy path).
With lldb-test, it's easy to read/generate the test input and parse the
test output. I'll attach a simple "fuzz" tester which generates failing
test cases to the Phab review. Here's an example:
```
Command: malloc(size=1024, alignment=32)
Malloc: address = 0xca000
Command: malloc(size=64, alignment=16)
Malloc: address = 0xca400
Command: malloc(size=1024, alignment=16)
Malloc: address = 0xca440
Command: malloc(size=16, alignment=8)
Malloc: address = 0xca840
Command: malloc(size=2048, alignment=16)
Malloc: address = 0xcb000
Command: malloc(size=64, alignment=32)
Malloc: address = 0xca860
Command: malloc(size=1024, alignment=16)
Malloc: address = 0xca890
Malloc error: overlapping allocation detected, previous allocation at [0xca860,
0xca8a0)
```
{F6288839}
Differential Revision: https://reviews.llvm.org/D47508
Added:
lldb/trunk/lit/Expr/TestIRMemoryMap.test
Modified:
lldb/trunk/source/Target/Process.cpp
lldb/trunk/tools/lldb-test/lldb-test.cpp
Added: lldb/trunk/lit/Expr/TestIRMemoryMap.test
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Expr/TestIRMemoryMap.test?rev=333583&view=auto
==
--- lldb/trunk/lit/Expr/TestIRMemoryMap.test (added)
+++ lldb/trunk/lit/Expr/TestIRMemoryMap.test Wed May 30 12:39:10 2018
@@ -0,0 +1,28 @@
+# RUN: %cxx %p/Inputs/call-function.cpp -g -o %t
+# RUN: lldb-test ir-memory-map %t %s
+
+malloc 0 1
+malloc 1 1
+
+malloc 2 1
+malloc 2 2
+malloc 2 4
+
+malloc 3 1
+malloc 3 2
+malloc 3 4
+
+malloc 128 1
+malloc 128 2
+malloc 128 4
+malloc 128 128
+
+malloc 2048 1
+malloc 2048 2
+malloc 2048 4
+
+malloc 3968 1
+malloc 3968 2
+malloc 3968 4
+
+malloc 0 1
Modified: lldb/trunk/source/Target/Process.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/Process.cpp?rev=333583&r1=333582&r2=333583&view=diff
==
--- lldb/trunk/source/Target/Process.cpp (original)
+++ lldb/trunk/source/Target/Process.cpp Wed May 30 12:39:10 2018
@@ -2539,8 +2539,10 @@ Status Process::WriteObjectFile(std::vec
#define USE_ALLOCATE_MEMORY_CACHE 1
addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
- if (GetPrivateState() != eStateStopped)
+ if (GetPrivateState() != eStateStopped) {
+error.SetErrorToGenericError();
return LLDB_INVALID_ADDRESS;
+ }
#if defined(USE_ALLOCATE_MEMORY_CACHE)
return m_allocated_memory_cache.AllocateMemory(size, permissions, error);
Modified: lldb/trunk/tools/lldb-test/lldb-test.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-test/lldb-test.cpp?rev=333583&r1=333582&r2=333583&view=diff
==
--- lldb/trunk/tools/lldb-test/lldb-test.cpp (original)
+++ lldb/trunk/tools/lldb-test/lldb-test.cpp Wed May 30 12:39:10 2018
@@ -15,6 +15,7 @@
#include "lldb/Core/Debugger.h"
#include "lldb/Core/Module.h"
#include "lldb/Core/Section.h"
+#include "lldb/Expression/IRMemoryMap.h"
#include "lldb/Initialization/SystemLifetimeManager.h"
#include "lldb/Interpreter/CommandInterpreter.h"
#include "lldb/Interpreter/CommandReturnObject.h"
@@ -23,17 +24,22 @@
#include "lldb/Symbol/SymbolVendor.h"
#include "lldb/Symbol/TypeList.h"
#include "lldb/Symbol/VariableList.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
#include "lldb/Utility/CleanUp.h"
#include "lldb/Utility/DataExtractor.h"
#incl
[Lldb-commits] [PATCH] D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap
This revision was automatically updated to reflect the committed changes.
Closed by commit rL333583: [lldb-test] Add a testing harness for the JIT's
IRMemoryMap (authored by vedantk, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D47508?vs=149173&id=149183#toc
Repository:
rL LLVM
https://reviews.llvm.org/D47508
Files:
lldb/trunk/lit/Expr/TestIRMemoryMap.test
lldb/trunk/source/Target/Process.cpp
lldb/trunk/tools/lldb-test/lldb-test.cpp
Index: lldb/trunk/tools/lldb-test/lldb-test.cpp
===
--- lldb/trunk/tools/lldb-test/lldb-test.cpp
+++ lldb/trunk/tools/lldb-test/lldb-test.cpp
@@ -15,25 +15,31 @@
#include "lldb/Core/Debugger.h"
#include "lldb/Core/Module.h"
#include "lldb/Core/Section.h"
+#include "lldb/Expression/IRMemoryMap.h"
#include "lldb/Initialization/SystemLifetimeManager.h"
#include "lldb/Interpreter/CommandInterpreter.h"
#include "lldb/Interpreter/CommandReturnObject.h"
#include "lldb/Symbol/ClangASTContext.h"
#include "lldb/Symbol/ClangASTImporter.h"
#include "lldb/Symbol/SymbolVendor.h"
#include "lldb/Symbol/TypeList.h"
#include "lldb/Symbol/VariableList.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Target.h"
#include "lldb/Utility/CleanUp.h"
#include "lldb/Utility/DataExtractor.h"
#include "lldb/Utility/StreamString.h"
+#include "llvm/ADT/IntervalMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/MathExtras.h"
#include "llvm/Support/ManagedStatic.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/PrettyStackTrace.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/WithColor.h"
+#include
#include
using namespace lldb;
@@ -46,6 +52,15 @@
cl::SubCommand ModuleSubcommand("module-sections",
"Display LLDB Module Information");
cl::SubCommand SymbolsSubcommand("symbols", "Dump symbols for an object file");
+cl::SubCommand IRMemoryMapSubcommand("ir-memory-map", "Test IRMemoryMap");
+cl::opt Log("log", cl::desc("Path to a log file"), cl::init(""),
+ cl::sub(IRMemoryMapSubcommand));
+
+/// Create a target using the file pointed to by \p Filename, or abort.
+TargetSP createTarget(Debugger &Dbg, const std::string &Filename);
+
+/// Read \p Filename into a null-terminated buffer, or abort.
+std::unique_ptr openFile(const std::string &Filename);
namespace breakpoint {
static cl::opt Target(cl::Positional, cl::desc(""),
@@ -135,8 +150,49 @@
static int dumpSymbols(Debugger &Dbg);
}
+
+namespace irmemorymap {
+static cl::opt Target(cl::Positional, cl::desc(""),
+ cl::Required,
+ cl::sub(IRMemoryMapSubcommand));
+static cl::opt CommandFile(cl::Positional,
+cl::desc(""),
+cl::init("-"),
+cl::sub(IRMemoryMapSubcommand));
+using AllocationT = std::pair;
+bool areAllocationsOverlapping(const AllocationT &L, const AllocationT &R);
+using AddrIntervalMap =
+ IntervalMap>;
+bool evalMalloc(IRMemoryMap &IRMemMap, StringRef Line,
+AddrIntervalMap &AllocatedIntervals);
+int evaluateMemoryMapCommands(Debugger &Dbg);
+} // namespace irmemorymap
+
} // namespace opts
+TargetSP opts::createTarget(Debugger &Dbg, const std::string &Filename) {
+ TargetSP Target;
+ Status ST =
+ Dbg.GetTargetList().CreateTarget(Dbg, Filename, /*triple*/ "",
+ /*get_dependent_modules*/ false,
+ /*platform_options*/ nullptr, Target);
+ if (ST.Fail()) {
+errs() << formatv("Failed to create target '{0}: {1}\n", Filename, ST);
+exit(1);
+ }
+ return Target;
+}
+
+std::unique_ptr opts::openFile(const std::string &Filename) {
+ auto MB = MemoryBuffer::getFileOrSTDIN(Filename);
+ if (!MB) {
+errs() << formatv("Could not open file '{0}: {1}\n", Filename,
+ MB.getError().message());
+exit(1);
+ }
+ return std::move(*MB);
+}
+
void opts::breakpoint::dumpState(const BreakpointList &List, LinePrinter &P) {
P.formatLine("{0} breakpoint{1}", List.GetSize(), plural(List.GetSize()));
if (List.GetSize() > 0)
@@ -177,7 +233,7 @@
switch (Cmd[0]) {
case '%':
if (Cmd.consume_front("%p") && (Cmd.empty() || !isalnum(Cmd[0]))) {
-OS << sys::path::parent_path(CommandFile);
+OS << sys::path::parent_path(breakpoint::CommandFile);
break;
}
// fall through
@@ -192,26 +248,11 @@
}
int opts::breakpoint::evaluateBreakpoints(Debugger &Dbg) {
- TargetSP Target;
- Status ST =
- Dbg.GetTargetList().CreateTarget(Dbg, breakpoint::Target, /*triple*/ "",
- /*get_dependent_modules*/ false,
- /*platfor
[Lldb-commits] [lldb] r333585 - [lldb-test] ir-memory-map: Avoid accessing a bad iterator
Author: vedantk
Date: Wed May 30 12:46:47 2018
New Revision: 333585
URL: http://llvm.org/viewvc/llvm-project?rev=333585&view=rev
Log:
[lldb-test] ir-memory-map: Avoid accessing a bad iterator
Do not access Probe.start() when Probe is at the end of the interval
map.
Modified:
lldb/trunk/tools/lldb-test/lldb-test.cpp
Modified: lldb/trunk/tools/lldb-test/lldb-test.cpp
URL:
http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/lldb-test/lldb-test.cpp?rev=333585&r1=333584&r2=333585&view=diff
==
--- lldb/trunk/tools/lldb-test/lldb-test.cpp (original)
+++ lldb/trunk/tools/lldb-test/lldb-test.cpp Wed May 30 12:46:47 2018
@@ -551,17 +551,15 @@ bool opts::irmemorymap::evalMalloc(IRMem
auto Probe = AllocatedIntervals.begin();
Probe.advanceTo(Addr); //< First interval s.t stop >= Addr.
AllocationT NewAllocation = {Addr, EndOfRegion};
- if (Probe != AllocatedIntervals.end()) {
-while (Probe.start() < EndOfRegion) {
- AllocationT ProbeAllocation = {Probe.start(), Probe.stop()};
- if (areAllocationsOverlapping(ProbeAllocation, NewAllocation)) {
-outs() << "Malloc error: overlapping allocation detected"
- << formatv(", previous allocation at [{0:x}, {1:x})\n",
- Probe.start(), Probe.stop());
-exit(1);
- }
- ++Probe;
+ while (Probe != AllocatedIntervals.end() && Probe.start() < EndOfRegion) {
+AllocationT ProbeAllocation = {Probe.start(), Probe.stop()};
+if (areAllocationsOverlapping(ProbeAllocation, NewAllocation)) {
+ outs() << "Malloc error: overlapping allocation detected"
+ << formatv(", previous allocation at [{0:x}, {1:x})\n",
+Probe.start(), Probe.stop());
+ exit(1);
}
+++Probe;
}
// Insert the new allocation into the interval map.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc
vsk updated this revision to Diff 149198.
vsk added a reviewer: lhames.
vsk added a comment.
- Don't insert extra padding bytes when `alignment` = 1.
- + Lang
https://reviews.llvm.org/D47551
Files:
lit/Expr/Inputs/ir-memory-map-basic.test
lit/Expr/Inputs/ir-memory-map-overlap1.test
lit/Expr/TestIRMemoryMap.test
source/Expression/IRMemoryMap.cpp
Index: source/Expression/IRMemoryMap.cpp
===
--- source/Expression/IRMemoryMap.cpp
+++ source/Expression/IRMemoryMap.cpp
@@ -304,12 +304,26 @@
size_t alignment_mask = alignment - 1;
size_t allocation_size;
- if (size == 0)
+ if (size == 0) {
+// FIXME: Malloc(0) should either return an invalid address or assert, in
+// order to cut down on unnecessary allocations.
allocation_size = alignment;
- else
-allocation_size = (size & alignment_mask)
- ? ((size + alignment) & (~alignment_mask))
- : size;
+ } else {
+// Round up the requested size to an aligned value, if needed.
+if (size & alignment_mask)
+ allocation_size = ((size + alignment) & (~alignment_mask));
+else
+ allocation_size = size;
+
+// The process page cache does not see the requested alignment. We can't
+// assume its result will be any more than 1-byte aligned. To work around
+// this, request `alignment` additional bytes.
+//
+// FIXME: Pass the requested alignment into the process page cache to
+// reduce internal fragmentation.
+if (alignment > 1)
+ allocation_size += alignment;
+ }
switch (policy) {
default:
Index: lit/Expr/TestIRMemoryMap.test
===
--- lit/Expr/TestIRMemoryMap.test
+++ lit/Expr/TestIRMemoryMap.test
@@ -1,28 +1,3 @@
# RUN: %cxx %p/Inputs/call-function.cpp -g -o %t
-# RUN: lldb-test ir-memory-map %t %s
-
-malloc 0 1
-malloc 1 1
-
-malloc 2 1
-malloc 2 2
-malloc 2 4
-
-malloc 3 1
-malloc 3 2
-malloc 3 4
-
-malloc 128 1
-malloc 128 2
-malloc 128 4
-malloc 128 128
-
-malloc 2048 1
-malloc 2048 2
-malloc 2048 4
-
-malloc 3968 1
-malloc 3968 2
-malloc 3968 4
-
-malloc 0 1
+# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic.test
+# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-overlap1.test
Index: lit/Expr/Inputs/ir-memory-map-overlap1.test
===
--- /dev/null
+++ lit/Expr/Inputs/ir-memory-map-overlap1.test
@@ -0,0 +1,10 @@
+malloc 8 16
+malloc 16 8
+malloc 64 32
+malloc 1 8
+malloc 64 32
+malloc 64 8
+malloc 1024 32
+malloc 1 16
+malloc 8 16
+malloc 1024 16
\ No newline at end of file
Index: lit/Expr/Inputs/ir-memory-map-basic.test
===
--- /dev/null
+++ lit/Expr/Inputs/ir-memory-map-basic.test
@@ -0,0 +1,25 @@
+malloc 0 1
+malloc 1 1
+
+malloc 2 1
+malloc 2 2
+malloc 2 4
+
+malloc 3 1
+malloc 3 2
+malloc 3 4
+
+malloc 128 1
+malloc 128 2
+malloc 128 4
+malloc 128 128
+
+malloc 2048 1
+malloc 2048 2
+malloc 2048 4
+
+malloc 3968 1
+malloc 3968 2
+malloc 3968 4
+
+malloc 0 1
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc
lhames added a comment. LGTM. I haven't looked at process memory management. How hard would your FIXME be to implement? - Lang. https://reviews.llvm.org/D47551 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47539: [Platform] Accept arbitrary kext variants
jasonmolenda added a comment. LGTM. If we added more knowledge specifically about kext bundle layouts, we could restrict which files we test to see if they are valid binaries - but we'd need to parse the Info.plist at the top (to get the CFBundleExecutable name, and look for variations on that prefix) and we'd need to handle shallow/deep kext bundles. Given how few files are in kext bundles besides the kexts themselves (a couple of plists), this is code is much simpler than encoding all that extra specifics about kexts. https://reviews.llvm.org/D47539 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
