JDevlieghere added a comment.
I agree with Pavel, I prefer having a slightly more complex constructor if we
can make the call sites simpler for the simple common cases, especially since
we don't lose any generality by doing so. It's also not an uncommon pattern in
C++, e.g. `std::async` does so
tatyana-krasnukha updated this revision to Diff 135612.
tatyana-krasnukha added a comment.
Remove Optional from return type of FindInRange
https://reviews.llvm.org/D39967
Files:
include/lldb/Breakpoint/BreakpointSiteList.h
source/Breakpoint/BreakpointSiteList.cpp
source/Target/Process.cpp
clayborg added a comment.
In https://reviews.llvm.org/D43647#1016882, @aprantl wrote:
> I like Pavel's template syntax. It's not clear to me how to best implement
> the registering of the properties this way though. I think we want to avoid
> static intializers. Given how TargetProperties::Targ
clayborg added inline comments.
Comment at: include/lldb/Breakpoint/BreakpointSiteList.h:130
+ using ModifyingCallback = std::function;
+ bool ForEach(const ModifyingCallback &callback);
I was a bit vague with my explanations. Only ModifyingCallback needs to
alexandreyy accepted this revision.
alexandreyy added a comment.
Hi, @clayborg .
Could you please commit this patch?
https://reviews.llvm.org/D42917
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/lis
Author: labath
Date: Fri Feb 23 09:49:26 2018
New Revision: 325927
URL: http://llvm.org/viewvc/llvm-project?rev=325927&view=rev
Log:
Replace HashStringUsingDJB with llvm::djbHash
Summary:
The llvm function is equivalent to this one. Where possible I tried to
replace const char* with llvm::StringR
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325927: Replace HashStringUsingDJB with llvm::djbHash
(authored by labath, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43596?vs=135568&id=
Paul Robinson correctly pointed out this test was end-to-end so it
really didn't belong to clang.
After discussing with Adrian, I moved the test to the debuginfo-test
suite in rL325928.
We should be all set now.
Thanks,
--
Davide
On Thu, Feb 22, 2018 at 5:36 PM, Davide Italiano wrote:
> On Wed,
labath created this revision.
labath added reviewers: davide, zturner, jingham.
The command takes two input arguments: a module to use as a debug target
and a file containing a list of commands. The command will execute each
of the breakpoint commands in the file and dump the breakpoint state
afte
zturner added a comment.
Curious if we need lldb-test for this. Could we get by with just using lldb in
batch mode? Obviously I'm not opposed to adding whatever we need to lldb-test,
just want to make sure it's something that can't already be done with just lldb.
Comment at
amccarth created this revision.
amccarth added a reviewer: labath.
Herald added subscribers: aprantl, sanjoy.
Without this fix, the test ERRORs because the link of the inferior fails. This
patch adds the LLDB_TEST_API macro where needed and uses the new -2 magic value
for num_expected_locations
jingham added a comment.
This test has the advantage over running lldb in batch mode that it isolates
the tests you write using it from the output format of the break command.
The lldb test suite used to check the output of break all over the place, and
that meant when I went to change that out
labath added a comment.
In https://reviews.llvm.org/D43686#1017577, @zturner wrote:
> Curious if we need lldb-test for this. Could we get by with just using lldb
> in batch mode? Obviously I'm not opposed to adding whatever we need to
> lldb-test, just want to make sure it's something that ca
davide accepted this revision.
davide added a comment.
This revision is now accepted and ready to land.
I was going to suggest the same thing Zach suggested, but I think this fine as
is.
LGTM. The fact the test is more concise is definitely a win, but I don't think
this is the main reason for do
davide added a comment.
(and thanks for saving 1 minutes and 30 seconds of my life multiplied by the
many times I run the test suite per day).
https://reviews.llvm.org/D43686
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.ll
tatyana-krasnukha added inline comments.
Comment at: include/lldb/Breakpoint/BreakpointSiteList.h:130
+ using ModifyingCallback = std::function;
+ bool ForEach(const ModifyingCallback &callback);
clayborg wrote:
> I was a bit vague with my explanations. Only
To be fair, you could probably have made the dotest.py version of the test
close to as fast by not running a process. The old test was testing that we
got a location for the breakpoint AND hit it. The latter was probably
overkill. But the two tests aren't testing the same thing.
> On Feb 23,
vsk updated this revision to Diff 135671.
vsk added a comment.
Herald added a subscriber: mgorny.
Thanks for the feedback.
I've added support for forwarding arguments to the cleanup function though the
constructor. It uses std::bind instead of a lambda because I couldn't figure
out a simpler wa
labath added a comment.
This test deliberately compiles without `-g`, so the variable should not be in
the debug info. On non-windows systems we're expected to pick up the name from
the symbol table. Is there such a thing on windows as well? Maybe you need to
lookup the symbol as `_conflicting_
amccarth added a comment.
In https://reviews.llvm.org/D43688#1017638, @labath wrote:
> This test deliberately compiles without `-g`, so the variable should not be
> in the debug info.
Ah, that makes more sense than what I was thinking: that it was trying to make
sure the unused symbol wasn't
tatyana-krasnukha updated this revision to Diff 135674.
tatyana-krasnukha added a comment.
Change ForEach return type (bool -> void)
https://reviews.llvm.org/D39967
Files:
include/lldb/Breakpoint/BreakpointSiteList.h
source/Breakpoint/BreakpointSiteList.cpp
source/Target/Process.cpp
uni
vsk accepted this revision.
vsk added a comment.
I like that we can customize the command substitutions and the way we dump
internal state in this test mode. This offers some added flexibility over the
batch mode, IMO. LGTM, thanks!
https://reviews.llvm.org/D43686
__
clayborg added a comment.
So the question still remains: does this need to be done when debugging with
debugserver or lldb-server? I can't remember if memory write works around
breakpoint opcodes in debugserver and/or lldb-server. If it does, then this
code is only needed for targets that don't
clayborg added a comment.
If any of our lldb_private::Process subclasses handle this already, then we
need a new virtual function on lldb_private::Process like:
virtual bool lldb_private::Process::MemoryWorksAroundSoftwareBreakpoints(bool
read);
If "read" is true then we are asking about mem
labath added a comment.
In https://reviews.llvm.org/D43688#1017652, @amccarth wrote:
> In https://reviews.llvm.org/D43688#1017638, @labath wrote:
>
> > On non-windows systems we're expected to pick up the name from the symbol
> > table. Is there such a thing on windows as well?
>
>
> Given that
Author: jingham
Date: Fri Feb 23 13:10:42 2018
New Revision: 325958
URL: http://llvm.org/viewvc/llvm-project?rev=325958&view=rev
Log:
Fix breakpoint thread name conditionals after breakpoint options refactor.
PR36435
Modified:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread
tatyana-krasnukha added a comment.
May be I misunderstand your question, but I checked, that all targets of both
lldb-server and debugserver read opcode from memory and save it, when enable
software breakpoint.
https://reviews.llvm.org/D39967
___
ted added a comment.
Traditionally, remote gdbservers handle memory operations that overlap a
software breakpoint. Writes get the new value saved off, and reads get the
breakpoint opcode replaced by the saved value.
https://reviews.llvm.org/D39967
___
clayborg added a comment.
In https://reviews.llvm.org/D39967#1017732, @ted wrote:
> Traditionally, remote gdbservers handle memory operations that overlap a
> software breakpoint. Writes get the new value saved off, and reads get the
> breakpoint opcode replaced by the saved value.
Indeed. Br
tatyana-krasnukha added a comment.
Got it. Yes, you are right. At least debugserver works around breakpoints by
himself while writing memory.
I will handle this case.
https://reviews.llvm.org/D39967
___
lldb-commits mailing list
lldb-commits@lists.
labath added a comment.
I believe lldb-server does not work around breakpoint opcodes during writing
(it does for reading), but that can be fixed, of course.
https://reviews.llvm.org/D39967
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
ted added a comment.
In https://reviews.llvm.org/D39967#1017754, @labath wrote:
> I believe lldb-server does not work around breakpoint opcodes during writing
> (it does for reading), but that can be fixed, of course.
It should. If it doesn't, then a write over a breakpoint would clobber it.
On 23 February 2018 at 11:24, Jim Ingham wrote:
> To be fair, you could probably have made the dotest.py version of the test
> close to as fast by not running a process. The old test was testing that we
> got a location for the breakpoint AND hit it. The latter was probably
> overkill. But t
aprantl created this revision.
aprantl added reviewers: vsk, labath, jingham.
Herald added a subscriber: eraman.
When writing an inline test, there is no way to make sure that any of the
inline commands are actually executed, so this patch adds a sanity check that
at least one breakpoint was hit
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
This looks great, thanks. I've been wanting to do something about the CleanUp
class for a long time. I have just a couple of nits, but no need to go through
review again.
C
vsk added a comment.
Thanks! I'll clean up the issues you pointed out before committing.
https://reviews.llvm.org/D43662
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
Looks good. One extra possible check would be to make sure that at the exit of
the loop, the inferior is in an expected state (eStateExited with result 0?)
https://reviews.llvm.org/D43694
Author: vedantk
Date: Fri Feb 23 14:08:38 2018
New Revision: 325964
URL: http://llvm.org/viewvc/llvm-project?rev=325964&view=rev
Log:
[Utility] Simplify and generalize the CleanUp helper, NFC
Removing the template arguments and most of the mutating methods from
CleanUp makes it easier to understa
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325964: [Utility] Simplify and generalize the CleanUp
helper, NFC (authored by vedantk, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43662?
amccarth added a comment.
In https://reviews.llvm.org/D43688#1017691, @labath wrote:
> In https://reviews.llvm.org/D43688#1017652, @amccarth wrote:
>
> > In https://reviews.llvm.org/D43688#1017638, @labath wrote:
> >
> > > On non-windows systems we're expected to pick up the name from the symbol
amccarth added a comment.
Nit: As a single word, "cleanup" is a noun (or an adjective). As two words,
"clean up" is a verb. Given that, I'd expect to see classes and objects with
names like `Cleanup` or `cleanup` and functions to contain `CleanUp`. When I
see an identifier like `CleanUpTes
krytarowski created this revision.
krytarowski added reviewers: labath, joerg.
krytarowski added a project: LLDB.
Herald added a subscriber: llvm-commits.
Reset errno to 0 for error branches.
Without this, debugging real issues in LLDB is harder
as we don't know what and when caused the errno
to
labath added a subscriber: spyffe.
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
In https://reviews.llvm.org/D43688#1017831, @amccarth wrote:
> OK, as far as I can tell, Windows doesn't have anything equivalent to the
> .symtab: a DLL ha
labath added a comment.
This seems weird. I have never seen anyone resetting errno *after* a call.
Wouldn't it be better to log strerror(errno) somewhere, or make sure that the
Status object reflects the actual error?
Repository:
rL LLVM
https://reviews.llvm.org/D43698
__
tatyana-krasnukha updated this revision to Diff 135711.
tatyana-krasnukha added a comment.
Have no idea how to know about working around breakpoints for read and write
separately. Just rely on support of Z-packets for now.
This expectation could be fair, however...
In https://reviews.llvm.org/D3
clayborg added a comment.
Yeah, never seen that before. Are you sure CreateSocket is doing the right
thing and checking for the correct return value? That is the only way I can see
this causing issues.
Repository:
rL LLVM
https://reviews.llvm.org/D43698
__
krytarowski added a comment.
This just helps debugging connectivity issues in other unrelated code-parts
(I'm debugging why `select`(2) / `recv`(2) don't receive a packet of type
`qHostInfo`).
Floating errno is annoying.
`TCPSocket::Connect()` has no bugs at least I'm not aware of them.
Repo
krytarowski added a comment.
https://github.com/search?l=C&q=save_errno&ref=opensearch&type=Code
Here is an approach with `save_errno`, useful in libraries when we preserve
errno for public functions in the API of a library as-is, and not leaking it
from the internals to unrelated code.
Right
Author: vedantk
Date: Fri Feb 23 15:18:27 2018
New Revision: 325974
URL: http://llvm.org/viewvc/llvm-project?rev=325974&view=rev
Log:
Fix a compiler warning in ModuleCacheTest.cpp, NFC
Modified:
lldb/trunk/unittests/Target/ModuleCacheTest.cpp
Modified: lldb/trunk/unittests/Target/ModuleCache
labath added a comment.
Yes, but that's why we have the Error objects, so we don't have to do the errno
save-restore dance.
If this is just a debugging aid, then maybe you should keep it as such
(locally). There are plenty of places that will leave a stray errno around and
I don't think we wan
krytarowski abandoned this revision.
krytarowski added a comment.
This is considered to be a local patch.
Repository:
rL LLVM
https://reviews.llvm.org/D43698
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/
Sure, as long as we aren't turning the output of the "break set" or other
commands into test API - which your version of this lit test does not - then
this sort of test seems fine to me.
When we were starting out and building up the set of tests available I at least
tended to err on the side of
Author: vedantk
Date: Fri Feb 23 16:17:05 2018
New Revision: 326002
URL: http://llvm.org/viewvc/llvm-project?rev=326002&view=rev
Log:
Delete dead code in MachVMMemory.cpp, NFC
This addresses a compiler warning.
Modified:
lldb/trunk/tools/debugserver/source/MacOSX/MachVMMemory.cpp
Modified:
Author: vedantk
Date: Fri Feb 23 16:17:04 2018
New Revision: 326001
URL: http://llvm.org/viewvc/llvm-project?rev=326001&view=rev
Log:
[unittests] Disable lldb-server tests if an external debug server is in use
The lldb-server unit tests don't test the right thing when the debug
server in use is c
amccarth added inline comments.
Comment at:
lldb/trunk/packages/Python/lldbsuite/test/lang/c/unicode/TestUnicodeSymbols.py:13
+self.build()
+ spec = lldb.SBModuleSpec()
+ spec.SetFileSpec(lldb.SBFileSpec(self.getBuildArtifact("a.out")))
Oops!
amccarth created this revision.
amccarth added a reviewer: labath.
Herald added a subscriber: sanjoy.
https://reviews.llvm.org/D43705
Files:
lldb/packages/Python/lldbsuite/test/lang/c/unicode/TestUnicodeSymbols.py
Index: lldb/packages/Python/lldbsuite/test/lang/c/unicode/TestUnicodeSymbols.py
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
lgtm, of course. This happened because I created this file on my mac, which is
not my regular development environment.
https://reviews.llvm.org/D43705
_
57 matches
Mail list logo
