labath added a comment.
Thanks for the heads up. I can test the android part on Wednesday, but right
now I don't see a reason why that shouldn't work there.
Overall, I like the idea of using the process class for caching some platform
resources. If we end up needing to do that more often, we ca
labath added a comment.
The part I am not sure about here is that you are creating a Module which has
no associated object file, but it still has some sections. That's not how any
of our current modules/object files work, and I worry that this may cause
problems down the line (and plainly put,
clayborg added inline comments.
Comment at: include/lldb/Target/Process.h:811
//--
+ /// Get the cached UtilityFunction that assists in loading binary
+ /// images into the process.
Fix comment
labath added a comment.
I don't think doing this is necessary when we have only one customer, but if we
are going to be designing an general purpose storage facility then I don't
think we should be using strings (and particularly not ConstStrings) as the
lookup keys.
I would propose going for
clayborg added a comment.
In https://reviews.llvm.org/D45703#1069921, @labath wrote:
> I don't think doing this is necessary when we have only one customer, but if
> we are going to be designing an general purpose storage facility then I don't
> think we should be using strings (and particularl
jingham added a comment.
I'm not sure what facility we are designing here? If it is only utility
functions, I think that is a very narrow use case, and I'm not sure it needs a
special purpose facility. I would prefer to wait till I see a second instance
for that. After all, this is not SB AP
zturner added inline comments.
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:957-958
+
char path[PATH_MAX];
remote_file.GetPath(path, sizeof(path));
+
Can we use the version that returns a `std::string`? I consider this version
unsafe as i
lemo updated this revision to Diff 142799.
lemo edited the summary of this revision.
lemo added a comment.
Thanks for the feedback.
PS. The sample output in the description if from LLDB running on Linux, reading
minidumps captured on Windows.
https://reviews.llvm.org/D45700
Files:
packages
jingham updated this revision to Diff 142800.
jingham added a comment.
Address review comments.
I changed over to passing a up to get stored in the process.
I separated the maker code into a separate function.
I changed GetPath versions (the buffer one was in the original, but I agree the
std::s
lemo marked 4 inline comments as done.
lemo added inline comments.
Comment at:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53
+for module in self.target.modules:
+self.assertTrue(module.IsValid())
+
laba
clayborg added a comment.
I am fine if we don't want to do the general solution now. LGTM if all is well
in the test suite.
Repository:
rL LLVM
https://reviews.llvm.org/D45703
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://list
labath added a comment.
I like the idea of leaving some comment to have a record that we discussed
making a more generic storage facility, but I don't think we need to do
anything else right now. I doubt we will see many new clients for this very
soon.
Comment at: source/Plu
labath added inline comments.
Comment at:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53
+for module in self.target.modules:
+self.assertTrue(module.IsValid())
+
lemo wrote:
> labath wrote:
> > Could we
Author: labath
Date: Tue Apr 17 11:53:35 2018
New Revision: 330200
URL: http://llvm.org/viewvc/llvm-project?rev=330200&view=rev
Log:
Move Args.cpp from Interpreter to Utility
Summary:
The Args class is used in plenty of places besides the command
interpreter (e.g., anything requiring an argc+argv
lemo marked 2 inline comments as done.
lemo added inline comments.
Comment at:
packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:53
+for module in self.target.modules:
+self.assertTrue(module.IsValid())
+
laba
jingham added a comment.
I added a comment, I'll upload the diff with that in a sec.
Comment at: source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:1166
+ // The dlopen succeeded!
+ if (token != 0x0)
+return process->AddImageToken(token);
labath wrote:
> Use
jingham updated this revision to Diff 142814.
jingham added a comment.
Added a comment describing our discussions for a general "stash this in the
process" facility.
Repository:
rL LLVM
https://reviews.llvm.org/D45703
Files:
include/lldb/Target/Process.h
source/Plugins/Platform/POSIX/Pl
Author: jingham
Date: Tue Apr 17 13:35:00 2018
New Revision: 330211
URL: http://llvm.org/viewvc/llvm-project?rev=330211&view=rev
Log:
Fix the xcode project for the Args -> Utility move.
Modified:
lldb/trunk/lldb.xcodeproj/project.pbxproj
Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj
UR
jingham added a comment.
The only outstanding question is whether we should enforce "Targets can't
change their platforms" in which case we could remove the check in
GetLoadImageUtilityFunction. The check does no harm either way, and this
doesn't seem the right forum to decide the larger polic
Author: jingham
Date: Tue Apr 17 13:44:47 2018
New Revision: 330214
URL: http://llvm.org/viewvc/llvm-project?rev=330214&view=rev
Log:
Change PlatformPosix::DoLoadImage to use a UtilityFunction.
That way we won't have to compile a new expression every time we want
dlopen a library.
Differentia
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL330214: Change PlatformPosix::DoLoadImage to use a
UtilityFunction. (authored by jingham, committed by ).
Changed prior t
alur updated this revision to Diff 142862.
https://reviews.llvm.org/D45628
Files:
packages/Python/lldbsuite/test/linux/compressed-debug-info/Makefile
packages/Python/lldbsuite/test/linux/compressed-debug-info/TestCompressedDebugInfo.py
packages/Python/lldbsuite/test/linux/compressed-debug-
amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.
LGTM, but consider highlighting the side effect to `target` when the factory
makes a Placeholder module.
In the future, we need to refactor the minidump tests to get rid of the
redundancy
alur updated this revision to Diff 142869.
https://reviews.llvm.org/D45628
Files:
packages/Python/lldbsuite/test/linux/compressed-debug-info/Makefile
packages/Python/lldbsuite/test/linux/compressed-debug-info/TestCompressedDebugInfo.py
packages/Python/lldbsuite/test/linux/compressed-debug-
alur marked an inline comment as done.
alur added inline comments.
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1773-1781
+static SectionType getSectionType(llvm::StringRef section_name) {
+ llvm::StringRef mapped_name;
+ if (section_name.startswith(".zdebug")) {
alur updated this revision to Diff 142875.
alur marked an inline comment as done.
alur added a comment.
Changed the diff to be against the NFCs.
https://reviews.llvm.org/D45628
Files:
packages/Python/lldbsuite/test/linux/compressed-debug-info/Makefile
packages/Python/lldbsuite/test/linux/c
26 matches
Mail list logo
