clayborg added a comment.
indeed very nice
https://reviews.llvm.org/D29496
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
clayborg added a comment.
I realize the functionality would add a "error: " prefix if it wasn't there,
but it seems like we could add this as a formatting option of the
lldb_private::Error class? Then we can just switch the logging code to put the
error in as a log item and add the extra flag
clayborg added inline comments.
Comment at: source/Target/SectionLoadList.cpp:70
bool warn_multiple) {
- Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER |
-
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D29510
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
So I agree with Pavel. Let us know what you think
https://reviews.llvm.org/D29514
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
If we can add a comment around "bndcfgu" where we use an SBData since it is a
vector register and SBValue::GetValueAsUnsigned(...) won't work because it is a
vector that would be nice for
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good as long as if we type two log enable commands like:
(lldb) log enable -f /tmp/a.txt lldb process
(lldb) log enable -f /tmp/a.txt lldb api
share the same log stream (we
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
I would prefer to see NativeBreakpoint struct expanded to have more member
variables instead of adding a new hardware breakpoint list. Then you just ask
any breakpoint to
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Thanks for doing the right thing in LLVM first, looks great!
Repository:
rL LLVM
https://reviews.llvm.org/D29288
___
lldb-commits mailing
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D28808
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D28944
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good as long as the test suite is happy.
https://reviews.llvm.org/D29036
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
Noticed another efficiency thing, see inlined comment.
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:37
+ ptr_str.insert(0, "&");
+ lldb::SBValue ptr_addr = frame.GetValueForVariablePath(ptr_str.c_str());
+ if (!ptr_addr.IsValid()) {
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
There are a few places where you are reading memory and then wanting to decode
data from it. Right now, you first read memory into a local buffer and then we
create an SBData
clayborg added a comment.
I fixed SBData with:
Sendingpackages/Python/lldbsuite/test/python_api/sbdata/TestSBData.py
Sendingsource/API/SBData.cpp
Transmitting file data ..done
Committing transaction...
Committed revision 293102.
https://reviews.llvm.org/D29078
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Be very careful when using this, you can't change member variables that used to
be std::once to be statics. We also don't need the llvm namespace to be
included with "using
clayborg added a comment.
Let me know why your GetValueAsUnsigned isn't working on your register by
stepping through it.
https://reviews.llvm.org/D29078
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added inline comments.
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:151
+ //
+ bd_entry--;
+
There is indeed a bug in SBData::SetData():
```
void SBData::SetData(lldb::SBError , const void *buf, size_t size,
clayborg added inline comments.
Comment at: tools/intel-mpx/IntelMPXTablePlugin.cpp:151
+ //
+ bd_entry--;
+
clayborg wrote:
> There is indeed a bug in SBData::SetData():
>
> ```
> void SBData::SetData(lldb::SBError , const void *buf, size_t size,
>
clayborg added a comment.
LGTM, but Pavel should give the ok as well.
https://reviews.llvm.org/D29669
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
clayborg added a comment.
I would still vote to check Buffer for NULL. GetByteSize() and GetBytes() are
usually accessed one time so there won't be a performance issue. If anyone
wanted to actually use a DataBufferLLVM as a member variable, they would need
to use a std::unique_ptr to one with
clayborg accepted this revision.
clayborg added a comment.
That is fine, just wanted to check.
Repository:
rL LLVM
https://reviews.llvm.org/D30287
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D30255
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
I don't know enough to say for sure. Go ahead and try it with this patch as the
files are pretty simple and losing the history wouldn't be too bad on these
files. If it fails to have history, then you should use an SVN workflow for any
future moves.
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks fine.
https://reviews.llvm.org/D29909
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
Hopefully we are maintaining the SVN history for these files? Hard to tell from
the patch.
https://reviews.llvm.org/D29909
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
Anyone still care about this? It would be nice to move it along or abandon it.
https://reviews.llvm.org/D19603
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good!
https://reviews.llvm.org/D29895
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
If it isn't making too much extra code I am fine with it being separate if you
believe this is the right way forward. Give it some thought and if you still
believe they should be separate, I won't object. It might be nice to send the
HardwareBreakpoint structure down
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Please add Xcode changes I sent you offline and this will be good to go.
https://reviews.llvm.org/D29964
___
lldb-commits mailing
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
Comment at:
source/Plugins/InstrumentationRuntime/ThreadSanitizer/ThreadSanitizerRuntime.cpp:89
+
+void *dlsym(void* handle, const char* symbol);
+
clayborg added a comment.
I am fine as long is Jim Ingham is fine. Jim, if you are good with this, just
put it into Accept Revision.
https://reviews.llvm.org/D30007
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Very close, just add a test for a bad value since someone can have code like:
StateType state;
llvm::formatv("{0}", state);
Just to make sure we don't crash during logging.
clayborg added inline comments.
Comment at: unittests/Core/StateTest.cpp:19
+ EXPECT_EQ("exited", llvm::formatv("{0}", eStateExited).str());
+ EXPECT_EQ("stopped", llvm::formatv("{0}", eStateStopped).str());
+}
Add a test like:
```
EXPECT_EQ("(null)",
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
We need to agree on options names and move the object file loading code into
ObjectFile.cpp. See inlined comments.
Comment at:
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D27459
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
We need to check stub_max_size to make sure we don't get an integer underflow.
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3999
+
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
A few cleanups on the logging. See inlined comments.
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4004
+// hope that data being
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
See easy inline fix and this will be good to go
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4004
+// hope that data being
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Just set an error when "set_pc" is true and you get no entry point back from
thee object file and this is good to go.
Comment at:
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Very nice!
https://reviews.llvm.org/D28804
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
This logic is wrong. If there is no object name, true should be returned. The
old logic is correct.
https://reviews.llvm.org/D30454
clayborg added a comment.
Object name is used for a module spec where the file is "/tmp/foo.a" and the
object name is "bar.o". So this is for named objects within a container file
like a BSD archive. If there is no object name, it doesn't need to be matched.
If there is an object name, then
clayborg resigned from this revision.
clayborg added a comment.
I will let Pavel do this review as it is in his area of expertise.
https://reviews.llvm.org/D30457
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Feel free to remote ConnectionSharedMemory as well in another change.
https://reviews.llvm.org/D27134
___
lldb-commits mailing list
clayborg added a comment.
Seems weird that we are a C++ codebase and we fall back to C macros. We
currently need the macro only for file + line and to also not have to worry
about doing "if (log) log->". I am not a big fan of macros, but I am open to it
if everyone else wants it.
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
I would like to see the FileSpec having its formatv stuff done for this since
it will start a whole slew of people wanting to use it and we should have an
example of a class
clayborg added a comment.
It is nice to be able to use StringRef and std::string as is. I was wondering
if you have:
StringRef null;
StringRef empty("");
StringRef hello("hello")
What does this show:
formatv("{0} {1} {2}", null, empty, hello);
I would hope for:
(null) "" "hello"
clayborg added a comment.
We should be able to handle this being thread specific. Each stop reply packet
and qThreadStopInfo (asks for a complete stop reply packet for each thread), or
the the "jThreadsInfo" packet (see "$trunk/docs/lldb-gdb-remote.txt" for
detail) has the ability to return
clayborg added a comment.
I am fine with us changing logging, just want to make sure it works well.
Ideally it would include:
- almost zero cost when logging disabled. No arguments should be passed to any
functions and then determine that logging is disabled, it should check if
logging is
clayborg added a comment.
Does llvm::formatv allow you to specify the base of integers, or just where
they go? Can you give it width, number base, leading characters and other
things like a lot of what printf does?
https://reviews.llvm.org/D27459
clayborg added a comment.
Part of the reason I like the current "if (log) log->Printf()" is that it
doesn't cost you much if logging isn't enabled. So if you have a log line like:
if (log)
log->Printf("%s %s %s", myStringRef1.str().c_str(),
myStringRef2.str().c_str(),
clayborg added a comment.
The other way to do this that might be easier is to require this register is
always read via a "p" packet and watch for the size that comes back and adjust
the register size for that register accordingly.
https://reviews.llvm.org/D27088
clayborg added a comment.
There seems to be a bunch of places that we are passing a string literal to
functions that take a StringRef. Can we pass L("--") instead of "--" to
functions that take a string ref and have it be more efficient so a StringRef
isn't implicitly constructed for us each
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D27223
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Ok, as long as the StringRef constructors are quick and efficient and not
running strlen() each time I am good.
https://reviews.llvm.org/D27780
clayborg added a comment.
Looking good. A few little changes and possibly supporting long option names
that can be minimally specified. See inlined comments.
Comment at: source/Host/common/FileSpec.cpp:1394
+ assert(
+ (Style.empty() || Style.equals_lower("F") ||
clayborg added inline comments.
Comment at: source/Host/common/FileSpec.cpp:1394
+ assert(
+ (Style.empty() || Style.equals_lower("F") || Style.equals_lower("D")) &&
+ "Invalid FileSpec style!");
zturner wrote:
> clayborg wrote:
> > If we are going to
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
So we need to formalize this in the formatv documentation before we begin wide
adoption otherwise people can do what ever they want. It would be good to write
this up so we get consistent
clayborg added a comment.
Sounds good. Take it as my vote this is ok to start with if we get consensus
https://reviews.llvm.org/D27632
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
If we can parse the register info that was retrieved via the GDB remote packets
and emulate the DWARF expression that would allow us to do things correctly in
the test case. Anything that is agnostic will work and we probably have all the
info we need. We will need to
clayborg added a comment.
Lets start with this. I would by default just emit the error string and then
let users opt in via additional format modifiers to show the "error: " prefix
and add the category.
https://reviews.llvm.org/D28519
___
clayborg added a comment.
You can't add anything extra to the AsCString() since it returns a "const char
*". You can't return a StringRef because it isn't backed by anything. You could
return a std::string.
My vote would be to leave AsCString() alone and have it just return a pointer
to the
clayborg requested changes to this revision.
clayborg added a reviewer: clayborg.
clayborg added a comment.
This revision now requires changes to proceed.
Looks fine except I would rather not have file + line on by default.
Comment at:
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks fine.
https://reviews.llvm.org/D28519
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
See inlined comment.
Comment at: include/lldb/Core/Error.h:308-309
+ llvm::StringRef Options) {
+llvm::format_provider::format(error.AsCString(), OS,
+ Options);
+ }
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Most of the DWARF stuff is about to go away anyway in favor of using the LLVM
DWARF parser as I am currently modifying it to support all we need in LLDB so
we can get rid of the entire
clayborg added a comment.
I am not sure I like the implications of all this MIPS specific knowledge in
this test. I would like it if we can abstract this into the GDB remote
protocol. Since we get the register descriptions from the lldb-server, it would
be nice if the register description for
clayborg resigned from this revision.
clayborg removed a reviewer: clayborg.
clayborg added a comment.
I let Jim OK this patch.
https://reviews.llvm.org/D27124
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a reviewer: spyffe.
clayborg added a comment.
I am going to include Sean Callanan in on this one to see what he thinks. We
might just want to fix the AST importer so that it can handle types in a
BlockDecl.
https://reviews.llvm.org/D27394
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Switch over to using llvm::StringRef in the functions where mentioned.
Comment at: source/Symbol/Type.cpp:623
bool Type::GetTypeScopeAndBasename(const char
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
This change removes support for using a FILE* instead of a file descriptor.
This needs to be fixed. The old Read function used to do this:
ssize_t bytes_read = -1;
if
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
I would skip the SBDwarf.h and just make a simple DWARF opcode parser all in
python. We only need it to handle the opcodes that are currently used by any
supported targets.
clayborg added a comment.
In https://reviews.llvm.org/D28305#635877, @labath wrote:
> The way this deals with it is by using `GetDescriptor()` which extracts the
> raw fd from FILE *, and then always deals with those. I guess this has the
> potential of bypassing any libc cache that FILE * may
clayborg added a comment.
Any progress on this? Read the previous comments and see if what I said makes
sense.
https://reviews.llvm.org/D19603
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Please swift to using the new formatv stuff as this is the main reason Zach
made those changes.
Comment at:
clayborg added a comment.
Let me know what you think about the MergeFrom comment. I am generally ok with
this, but just wanted to check in case the merge made any sense in this patch
somewhere.
Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:220
+ //
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Simple logic change so we don't check the range validity more than once.
Comment at:
clayborg added a comment.
It almost seems like we need some sort of an architecture plug-in here. Maybe
something like Architecture plugins. The Architecture::FindPlugin() would take
an ArchSpec and return a lldb_private::Architecture class instance that can be
cached in the target or process.
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good, sorry for the delay.
https://reviews.llvm.org/D30454
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D31485
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Looks fine as long as all of the old modes work. :
- "*:1234" that will accept a connection from any host on port 1234
- "foo.bar.com:1234" only accept connections from "foo.bar.com" on
clayborg added a comment.
when users edit some config file they can have "localhost" map to some other
port... Can't remember the unix file for this.
https://reviews.llvm.org/D31824
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Very close. A few misuses of ConstString and this will be good to go.
Comment at: include/lldb/Interpreter/Property.h:43
+ ConstString GetName() const {
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
This will break the Swift REPL as it relies on debug info and we won't be told
about the ".debug_XXX" or "__debug_XXX" sections if we disable this. Jim or
Sean should verify
clayborg created this revision.
This patch adds support for type units and parsing the .debug_types section
which allows LLDB to debug binaries that were built with "-gdwarf-4
-fdebug-types-section". This patch takes into account how DWARF 5 will
represent type units: all compile units will
clayborg resigned from this revision.
clayborg added a reviewer: jingham.
clayborg added a comment.
Jim, can you take a look at this and see if this could be fixed in a nicer way?
I would prefer to not see anything related to delay slots in a test. Can we
abstract this better? Maybe ask the
clayborg accepted this revision.
clayborg added a comment.
I'm good if Pavel is good.
https://reviews.llvm.org/D31823
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
clayborg added a comment.
Yeah, I was having trouble getting a real read on what was failing due to a non
Ubuntu setup at work for me. Is there any way to run a patch on one of the
buildbots? If not, can you email me the list of what is failing?
https://reviews.llvm.org/D32167
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
With my limited std::atomic experience, this seems fine.
https://reviews.llvm.org/D30702
___
lldb-commits mailing list
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
clayborg added a comment.
So a ModuleSpec allows you to specify a module by path, UUID and many other
things. This is falling down for a magic file that doesn't actually exist
right? "vsdo" is just a made up name for the table of loaded shared libraries?
Is that correct? I need to understand
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Pretty close. My only objection is we have many "lldb_private::Process::Will"
and "lldb_private::Process::Did" prefixed functions and none of them are
required to call the
clayborg added a comment.
My main objection is that if we have 10 "lldb_private::Process::Will*"
functions and only some require you to call the superclass, then it is
confusing. It is also hard to enforce. We probably have other process
subclasses that override these functions and they all
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Much better. Found some extra stuff. In general we should be using
SBStructuredData when ever we want to get/set stuff from structured data like
JSON, XML or Apple plist, etc.
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Very close. Can we try to get UpdateAutomaticSignalFiltering out of
lldb_private::Process as my inline comments suggest? It would be cleaner and I
am not sure we actually need
clayborg added a comment.
Jim Ingham said (in email):
> I disagree. The different processes are at this point more about transport
> than about the platform details. That indicates to me that it's more likely
> that different process implementations will have different ways of
>
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Ok, sounds like everyone thought through the solution. Lets start with this and
we can iterate if needed.
https://reviews.llvm.org/D30520
clayborg added a comment.
How does VSDO get loaded when debugging a normal process? In the core file case
you probably need to create the VSDO module from memory by locating it somehow
and add it to the target before the dynamic loader goes looking for it.
Wouldn't that fix the issue?
clayborg created this revision.
We use GDBRemoteCommunication::ScopedTimeout in many places to change the
packet timeout that is used for individual packets. If someone modifies the
default timeout manually or the GDB remote server requests a longer timeout in
a 'q' packet, then don't ever
1 - 100 of 2669 matches
Mail list logo
