Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5633007:
This change contains changes that were made on a separate copy of this code,... (Closed)
|
Created:
10 years ago by mlinck Modified:
9 years, 7 months ago Reviewers:
mmortensen, noelallen_use_chromium, Cliff L. Biffle, garianov, Ian Ni-Lewis (Google), David Springer CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionThis change contains changes that were made on a separate copy of this code,
in the nativeclient_vsx project. The idea is to create a canonical version
of this code so development can progress (finally).
Also, there are a number of bug fixes:
- Breakpoints can be set.
- Continue no longer crashes the rsp.
- Continue no longer wipes out breakpoints.
With these changes gdb debugging a program succesfully via the rsp has been
tested.
Note that there is now some dead code that can be removed, and a lot of cleanup
to the rsp code still remains to be done. I'm not doing this in this CL
because I want to get some form of automated testing on this working
functionality before any further code modifications. -mike
Related issue: http://code.google.com/p/nativeclient/issues/detail?id=1245
Patch Set 1 #
Total comments: 46
Patch Set 2 : '' #
Total comments: 22
Patch Set 3 : '' #
Total comments: 18
Patch Set 4 : '' #
Messages
Total messages: 9 (0 generated)
I've uploaded the first RSP CL. The point is to get the last changes ilewis and noelallen made over in the VSX version of this code, into the canonical version. Also, there are a few added comments for readability and a fixes for a couple of bugs. See the description for the CL for more details. Note that this code is still dormant by default. Alex and I will be working that once we feel that it is more stable and tested.
I just had a few questions and nits...LGTM although I'm sure someone closer to the code (i.e. Noel or Ian) should give an LGTM too. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.cc File src/trusted/gdb_rsp/host_mock.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:10: #include <stdio.h> Minor nit: we could use C++ style headers here, such as #include <cassert> #include <cstring> #include <cstdlib> #include <cstdio> http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc File src/trusted/gdb_rsp/packet.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc#n... src/trusted/gdb_rsp/packet.cc:269: printf("GetWord8 got chars: %c %c\n", seq1, seq2); Having the printf is probably a big help here, since it's hard to debug the debugger. Two questions: 1) Do we need to guard them with a boolean variable or ifdefs? 2) Should other functions (GetWord16, GetWord32) also consistently printf for debug help? I'm not sure the answer to the questions, but it's worth thinking about while you are in the code. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc#n... src/trusted/gdb_rsp/target.cc:44: run_req_(-1), Since this is an unsigned int, do we cause warnings by assigning a -1 here? Same comment for line 46 below http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h File src/trusted/gdb_rsp/target.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h#ne... src/trusted/gdb_rsp/target.h:187: uint32_t run_req_; // Requested thread for run operations. If run_req and run_reg can bo -1, why are the declared as unsigned int? Are we utilizing the fact the -1 has the 2's complement value of 0xFFFFFFFF when we assign -1 to ii? In target.cc we do in fact assign -1 to these values.
I'm not done with the review, but I wanted to give you early feedback so you can fix get started. Make sure you: 1) Do the right thing WRT include guards. You can ask Cliff for an opinion on whether or not they should be changed, however not changing something that's already checked in seems safer. :) 2) Search for 'printf' there appears to be several places in the code where we will spew debugging information that was probably used for development. If it's a valid debug message, then it should probably be a Platform::Log 3) Make sure this is building correctly with all errors and warnings turned on. There are several things seem that they could not have compiled with our standard flags. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/abi.h File src/trusted/gdb_rsp/abi.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/abi.h#newco... src/trusted/gdb_rsp/abi.h:20: #ifndef SRC_TRUSTED_GDB_RSP_ABI_H_ While this probably what we SHOULD do, it's inconsistent with what we ACTUALLY did. If you grep for #ifndef you will see that "NATIVE_CLIENT_<MODULE>_<NAME>" or "NATIVE_CLIENT_SRC_<PATH...>_<NAME>" is the norm for src/trusted/*/*.h http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.cc File src/trusted/gdb_rsp/host.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.cc#new... src/trusted/gdb_rsp/host.cc:242: bool Host::WaitForBreak() { Should we kill HS_STOPPING? http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.h File src/trusted/gdb_rsp/host.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.h#newc... src/trusted/gdb_rsp/host.h:14: #ifndef SRC_TRUSTED_GDB_RSP_HOST_H_ NATIVE_CLIENT_xxxx http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.h#newc... src/trusted/gdb_rsp/host.h:106: // Generic Request nit: The other comments are sentence, this is a noun. What about a generic request, send, receive? For example: // Issue a generic request or // Fetch a response from a generic request Nice to know this is both input and output. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.h#newc... src/trusted/gdb_rsp/host.h:137: protected: Add a comment that this was switched from private to protected, to allow mocking, so that someone does not mistakenly believe it is okay to access these when inheriting. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.cc File src/trusted/gdb_rsp/host_mock.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:34: HostMock::HostMock() : Host(0) { } http://www.corp.google.com/eng/doc/cppguide.xml#0_and_NULL use NULL http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:83: char brk[2] = { 0x03, 0x00 }; Where is this used? This should have generated a an error (due to warn as error) during compile. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:90: SignalMock* sig = new SignalMock; While this is not REALLY trusted code, since it will not get executed by sel_ldr, there should be a NULL check to make sure we are not out of memory. Are the mock files part of the gdb_rsp.lib, or are they stand alone test files? http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:119: /* Check if block is availible */ Misleading: Fail if block does not exist. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:125: out += max; Does this actually work? Addr does not appear to be modified, only out, so how do we get a new new/offs? Are we copying the same original block over all target blocks? http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:134: while (size) { Same as above. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:172: Either the source or the header should describe the difference between AddSignal, AddSignalAbs, and AddSignalDelta // Add a signal to the mock, on the specified thread which moves the IP by zero. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:218: sig->ip += ip; Should this be ip += sig->ip http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:229: snprintf(tmp, "S%02d", sig->sig, 63); should use sizeof(tmp) instead of 63 in case the size of tmp ever changes. Should also document how we came up with the size for temp to show it's big enough. 'S' + NUL + 2 digits = max of 4 characters. Speaking of which I don't see how this could have compiled.shouldn't this be: int snprintf(char *str, size_t size, const char *format, ...); This shouldn't have compiled since the second param is a const char * instead of a size_t. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.h File src/trusted/gdb_rsp/host_mock.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.h... src/trusted/gdb_rsp/host_mock.h:7: // This module provides an object for handling RSP responsibilities of Is this comment a DUP? It doesn't make clear that this is a mock. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.h... src/trusted/gdb_rsp/host_mock.h:15: #define SRC_TRUSTED_GDB_RSP_HOST_MOCK_H_ 1 NATIVE_CLIENT_xxx http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.h... src/trusted/gdb_rsp/host_mock.h:35: /* Mocked functions */ Should probably be '//' we should be consistent on commenting style through out. If /* turns out to be more common, then we should switch the previous header file. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.h... src/trusted/gdb_rsp/host_mock.h:90: struct SignalMock { What this is fairly obvious we should probably add a comment o say what the purpose of this structure is. A module comment would probably have made it clear what we generate these objects then 'play' them back to simulate signals. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.h... src/trusted/gdb_rsp/host_mock.h:101: Too many empty lines. Shouldn't do more than two empty lines. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc File src/trusted/gdb_rsp/packet.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc#n... src/trusted/gdb_rsp/packet.cc:268: This looks like it should be removed. Accidental debugging spew from development? http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc#n... src/trusted/gdb_rsp/packet.cc:304: printf("GetWord64 running \n"); same. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/session.h File src/trusted/gdb_rsp/session.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/session.h#n... src/trusted/gdb_rsp/session.h:27: #ifndef SRC_TRUSTED_GDB_RSP_SESSION_H_ NATIVE_CLIENT_... http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc#n... src/trusted/gdb_rsp/target.cc:36: : abi_(abi), This should generate a warning. Initializers need to be constructed in the same order they are declared in the header. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc#n... src/trusted/gdb_rsp/target.cc:48: printf("Target constructed "); looks like bogus prints. Fix this one and the ones bellow. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc#n... src/trusted/gdb_rsp/target.cc:77: printf("Populating Target Architecture with: #%s#.\n", abi_->GetName()); print (I'll assume you will grep for print instead of marking each one I find.) http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h File src/trusted/gdb_rsp/target.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h#ne... src/trusted/gdb_rsp/target.h:138: port::IThread *GetThread(uint32_t id) const; Good catch.
Hi, I took care of all the printfs and other concerns. I also broke out the host_mock into a separate CL since it needs to be built and tested. I hadn't used it yet, but was just bringing it over from the duplicate version of the code. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/abi.h File src/trusted/gdb_rsp/abi.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/abi.h#newco... src/trusted/gdb_rsp/abi.h:20: #ifndef SRC_TRUSTED_GDB_RSP_ABI_H_ On 2010/12/07 23:04:44, noelallen wrote: > While this probably what we SHOULD do, it's inconsistent with what we ACTUALLY > did. If you grep for #ifndef you will see that "NATIVE_CLIENT_<MODULE>_<NAME>" > or "NATIVE_CLIENT_SRC_<PATH...>_<NAME>" is the norm for src/trusted/*/*.h > Fair enough. I was just delinting. reverted. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.cc File src/trusted/gdb_rsp/host.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.cc#new... src/trusted/gdb_rsp/host.cc:242: bool Host::WaitForBreak() { On 2010/12/07 23:04:44, noelallen wrote: > Should we kill HS_STOPPING? A break request results in the "stopping" state. To wait for a break is valid if status == hs_stopping. I'm not sure if hs_running is still a necessary check here but I'd want to make sure I have written test coverage for this before modifying it further. (note that I didn't make this change originally) http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.h File src/trusted/gdb_rsp/host.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.h#newc... src/trusted/gdb_rsp/host.h:106: // Generic Request On 2010/12/07 23:04:44, noelallen wrote: > nit: The other comments are sentence, this is a noun. What about a generic > request, send, receive? For example: > // Issue a generic request or > // Fetch a response from a generic request > Nice to know this is both input and output. Done. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host.h#newc... src/trusted/gdb_rsp/host.h:137: protected: On 2010/12/07 23:04:44, noelallen wrote: > Add a comment that this was switched from private to protected, to allow > mocking, so that someone does not mistakenly believe it is okay to access these > when inheriting. Done. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.cc File src/trusted/gdb_rsp/host_mock.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:10: #include <stdio.h> On 2010/12/07 22:53:58, mmortensen wrote: > Minor nit: we could use C++ style headers here, such as > #include <cassert> > #include <cstring> > #include <cstdlib> > #include <cstdio> I'm leaning towards leaving these since they're working. I don't think nativeclient team ever adopted the SDK team's more nitpicky convention on this. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/host_mock.c... src/trusted/gdb_rsp/host_mock.cc:90: SignalMock* sig = new SignalMock; On 2010/12/07 23:04:44, noelallen wrote: > While this is not REALLY trusted code, since it will not get executed by > sel_ldr, there should be a NULL check to make sure we are not out of memory. > Are the mock files part of the gdb_rsp.lib, or are they stand alone test files? There's a separate target for these called gdb_rsp_test. There is now also a separate CL which will contain the work involved in getting this tested and building. I wasn't actually using this code for anything because I wanted the gdb_rsp library working but it was part of what I found in the newer code that I think should be added to nativeclient in due time. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc File src/trusted/gdb_rsp/packet.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc#n... src/trusted/gdb_rsp/packet.cc:268: On 2010/12/07 23:04:44, noelallen wrote: > This looks like it should be removed. Accidental debugging spew from > development? Done. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc#n... src/trusted/gdb_rsp/packet.cc:269: printf("GetWord8 got chars: %c %c\n", seq1, seq2); On 2010/12/07 22:53:58, mmortensen wrote: > Having the printf is probably a big help here, since it's hard to debug the > debugger. Two questions: > 1) Do we need to guard them with a boolean variable or ifdefs? > 2) Should other functions (GetWord16, GetWord32) also consistently printf for > debug help? > > I'm not sure the answer to the questions, but it's worth thinking about while > you are in the code. removed this http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/packet.cc#n... src/trusted/gdb_rsp/packet.cc:304: printf("GetWord64 running \n"); On 2010/12/07 23:04:44, noelallen wrote: > same. Done. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/session.h File src/trusted/gdb_rsp/session.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/session.h#n... src/trusted/gdb_rsp/session.h:27: #ifndef SRC_TRUSTED_GDB_RSP_SESSION_H_ On 2010/12/07 23:04:44, noelallen wrote: > NATIVE_CLIENT_... Done. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc#n... src/trusted/gdb_rsp/target.cc:36: : abi_(abi), On 2010/12/07 23:04:44, noelallen wrote: > This should generate a warning. Initializers need to be constructed in the > same order they are declared in the header. In this particular case, it is the same order as in the header. Turned out conState was dead code so I removed it. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc#n... src/trusted/gdb_rsp/target.cc:44: run_req_(-1), On 2010/12/07 22:53:58, mmortensen wrote: > Since this is an unsigned int, do we cause warnings by assigning a -1 here? > Same comment for line 46 below I'll resolve this once I get some feedback on how to proceed. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.cc#n... src/trusted/gdb_rsp/target.cc:48: printf("Target constructed "); On 2010/12/07 23:04:44, noelallen wrote: > looks like bogus prints. Fix this one and the ones bellow. Done. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h File src/trusted/gdb_rsp/target.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h#ne... src/trusted/gdb_rsp/target.h:187: uint32_t run_req_; // Requested thread for run operations. On 2010/12/07 22:53:58, mmortensen wrote: > If run_req and run_reg can bo -1, why are the declared as unsigned int? > > Are we utilizing the fact the -1 has the 2's complement value of 0xFFFFFFFF when > we assign -1 to ii? In target.cc we do in fact assign -1 to these values. Excellent question. I'm not sure what the best approach here is. I'm thinking I want to go with int64_t for each pair so that one can be set to -1 and one to 0(like now) but we don't get overflow problems. Not sure if that's valid for 32 bit platforms though. Any thoughts (Cliff or Noel)?
http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h File src/trusted/gdb_rsp/target.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h#ne... src/trusted/gdb_rsp/target.h:187: uint32_t run_req_; // Requested thread for run operations. I set this to 32b so you would get that behavior. Linux returns (0 > pid < 65535). So if you want you can make this an int32 and be fine. For Windows, 0 and -1 are also special. In the case of windows, the thread ID is 32b. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/session... File src/trusted/gdb_rsp/session_mock.h (left): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/session... src/trusted/gdb_rsp/session_mock.h:8: Did you mean to remove this from the CL? This still has the guard change BTW. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (left): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:40: send_done_(false), Why did you remove this? You have an uninitialized member. Did you mean to remove this from the structure as well? http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (right): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:209: // Lock the signal state information to revent a race prevent http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:211: This comment and surrounding code is a bit confusing and probably could use a better comment. I believe what is happening here is as follows: We "wait" for a signal to start ensuring that we are the only debugable thread processing a signal. We start with the signal "primed" so that the first signal can take place without waiting. We then lock the state to ensure the stub thread is not currently in it's run look and thus using state data. Finally we update the state information, unlock, allowing the stub thread to make progress. Finally we wait for 'done' which gives us permission to return from the exception state. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:286: // but the debugger is talking to use us http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:508: // This packet is requesting a read operation for more than one byte. Why more than one byte? It should be legal for 'llll' to be 1. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:534: // starting at a specific address. one or more http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:550: pktIn->GetBlock(block, len); Where is the delete[]? Is this a leak? http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:617: return true; Where is SetStep(false)? Make sure we turn off single step somewhere. Probably should always turn in off in the signal handler. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/test.h File src/trusted/gdb_rsp/test.h (right): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/test.h#... src/trusted/gdb_rsp/test.h:8: Guard. Belongs in gsb_rsp_test CL?
I fixed all requested items except those that seem to be more appropriately referred to future tickets. I only did the latter in cases where said tickets already existed. http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h File src/trusted/gdb_rsp/target.h (right): http://codereview.chromium.org/5633007/diff/1/src/trusted/gdb_rsp/target.h#ne... src/trusted/gdb_rsp/target.h:187: uint32_t run_req_; // Requested thread for run operations. On 2010/12/13 19:28:10, noelallen wrote: > I set this to 32b so you would get that behavior. Linux returns (0 > pid < > 65535). So if you want you can make this an int32 and be fine. For Windows, 0 > and -1 are also special. In the case of windows, the thread ID is 32b. Changed to int32_t http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/session... File src/trusted/gdb_rsp/session_mock.h (left): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/session... src/trusted/gdb_rsp/session_mock.h:8: On 2010/12/13 19:28:10, noelallen wrote: > Did you mean to remove this from the CL? > This still has the guard change BTW. removed. The weird guard change was the only thing different about this one anyway so it's no longer in the CL. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (left): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:40: send_done_(false), On 2010/12/13 19:28:10, noelallen wrote: > Why did you remove this? You have an uninitialized member. Did you mean to > remove this from the structure as well? I didn't remove this line. It had been removed in the version of code I'm trying to consolidate, but the change was left unfinished. I removed it from the struct since it's not used. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (right): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:209: // Lock the signal state information to revent a race On 2010/12/13 19:28:10, noelallen wrote: > prevent Done. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:211: On 2010/12/13 19:28:10, noelallen wrote: > This comment and surrounding code is a bit confusing and probably could use a > better comment. I believe what is happening here is as follows: > > We "wait" for a signal to start ensuring that we are the only debugable thread > processing a signal. We start with the signal "primed" so that the first signal > can take place without waiting. We then lock the state to ensure the stub > thread is not currently in it's run look and thus using state data. Finally we > update the state information, unlock, allowing the stub thread to make progress. > > Finally we wait for 'done' which gives us permission to return from the > exception state. Thanks for bringing this to my attention. There are two outstanding bugs against this code that are not in the scope of this CL. We'll make sure we comment this code properly once we have something that actually works. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:286: // but the debugger is talking to use On 2010/12/13 19:28:10, noelallen wrote: > us Done. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:508: // This packet is requesting a read operation for more than one byte. On 2010/12/13 19:28:10, noelallen wrote: > Why more than one byte? It should be legal for 'llll' to be 1. Done. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:534: // starting at a specific address. On 2010/12/13 19:28:10, noelallen wrote: > one or more Done. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:550: pktIn->GetBlock(block, len); On 2010/12/13 19:28:10, noelallen wrote: > Where is the delete[]? Is this a leak? Done. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:617: return true; On 2010/12/13 19:28:10, noelallen wrote: > Where is SetStep(false)? Make sure we turn off single step somewhere. Probably > should always turn in off in the signal handler. Well, there was a massive bug associated SetStep(false) because it caused the application to no longer stop at breakpoints. Stepping needs to be revisited and implemented in a way that doesn't break breakpoint behavior. There are outstanding issues that will need to resolve step behavior. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/test.h File src/trusted/gdb_rsp/test.h (right): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/test.h#... src/trusted/gdb_rsp/test.h:8: On 2010/12/13 19:28:10, noelallen wrote: > Guard. Belongs in gsb_rsp_test CL? Done.
Okay, looking pretty good. We need to figure out what the correct thing WRT step is, then we can pass this on to Cliff. FYI Cliff, only target.cc and target.h are used by native client, host.h and host.cc are debugger side only. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (right): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:617: return true; That seems incorrect. This function should be turning on or off the STEP flag on the CPU. If on, this thread will report a sig 5 every instruction. Turning HW step on and off does not affect breakpoints. On 2010/12/14 17:23:24, mlinck wrote: > On 2010/12/13 19:28:10, noelallen wrote: > > Where is SetStep(false)? Make sure we turn off single step somewhere. > Probably > > should always turn in off in the signal handler. > > Well, there was a massive bug associated SetStep(false) because it caused the > application to no longer stop at breakpoints. Stepping needs to be revisited > and implemented in a way that doesn't break breakpoint behavior. There are > outstanding issues that will need to resolve step behavior.
Comments on target.{cc,h}. I'm rather confused about the use of some of these
member variables -- see below.
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.cc
File src/trusted/gdb_rsp/target.cc (right):
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.cc:44: run_req_(-1),
You appear to be using two magic numbers, 0 and -1, here for thread IDs. What's
the difference?
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.cc:110: for (size_t cnt = parts.size(); cnt ; cnt--)
{
Excess space before semicolon.
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.cc:121: if ((cnt == 4) && (parts[3] == "read") &&
(parts.size() == 5)) {
Doing this inside the loop is confusing. It seems like you could do this check
at the top-level. But it's possible that I'm confused.
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.cc:225: char tmp[16];
Looks like this will be, at largest, 11 bytes.
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.cc:226: snprintf(tmp, sizeof(tmp), "QC%x",
sig_thread_);
Consider using NACL_ARRAY_SIZE macro here -- it will defend you against the
future possibility of tmp becoming a char* instead of a char[], which will cause
sizeof to behave rather differently.
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.cc:226: snprintf(tmp, sizeof(tmp), "QC%x",
sig_thread_);
Incidentally, isn't there a nice C++ way of doing this?
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.cc:255: if (cur_signal_) {
I see cur_signal_ being set to 0, -1, and actual values. Below, code says it is
"simulating a signal" by setting it to 0, and then resets it to -1. Can you
explain more about the lifecycle of this variable?
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.h
File src/trusted/gdb_rsp/target.h (right):
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.h:69: // MatchQuery does a longest prefix match
against the locally cased
cased? cached?
http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target....
src/trusted/gdb_rsp/target.h:175: int32_t run_req_; // Requested thread for
run operations.
These names are not great. They should probably contain the word 'thread.'
I've made some more changes based on cliff's review. This has also brought to light at least one new problem. A bug has been written. Since this code is so far from working, I've also decided we should really spend some time figuring out how to remove it from the TCB as soon as this review is finished so that I have "beachhead" version of the code. http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.cc File src/trusted/gdb_rsp/target.cc (right): http://codereview.chromium.org/5633007/diff/15001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:617: return true; On 2010/12/14 20:18:41, noelallen wrote: > That seems incorrect. This function should be turning on or off the STEP flag > on the CPU. If on, this thread will report a sig 5 every instruction. Turning > HW step on and off does not affect breakpoints. > > > On 2010/12/14 17:23:24, mlinck wrote: > > On 2010/12/13 19:28:10, noelallen wrote: > > > Where is SetStep(false)? Make sure we turn off single step somewhere. > > Probably > > > should always turn in off in the signal handler. > > > > Well, there was a massive bug associated SetStep(false) because it caused the > > application to no longer stop at breakpoints. Stepping needs to be revisited > > and implemented in a way that doesn't break breakpoint behavior. There are > > outstanding issues that will need to resolve step behavior. > That is false. I haven't looked closely at how step was implemented because it is beyond the scope of this CL (seeing how there are two outstanding tickets to address it separetely) but I know that the "resume" function used to result in a call to setStep(false) and I verified by testing that that line was responsible for the debug_stub ignoring breakpoints after resume and never breaking again. I had to remove the line in order to fix breakpoint behavior. What I want here as a baseline is code that I know does breakpoints, and tickets to fix all the other stuff that never worked properly in an organized fashion. Here's the version of the function that was clearing breakpoints. Resume() line 268 was the problem. http://code.google.com/p/nativeclient-vsx/source/browse/third_party/nacl_sub/... http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:44: run_req_(-1), On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > You appear to be using two magic numbers, 0 and -1, here for thread IDs. What's > the difference? I have addressed this by creating a new constant, used here and in "UpdateThreadId" below. Said function is the result of quickly factoring two almost identical functions because I got tired of making the same changes twice. http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:110: for (size_t cnt = parts.size(); cnt ; cnt--) { On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > Excess space before semicolon. Done. http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:121: if ((cnt == 4) && (parts[3] == "read") && (parts.size() == 5)) { On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > Doing this inside the loop is confusing. It seems like you could do this check > at the top-level. But it's possible that I'm confused. I've clarified this code a little bit but the only effect was to make it more obvious that it never worked, and that the body of the next if-statement makes no sense. I wrote an issue that requires this function to be spec'd, isolated, and tested. http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:225: snprintf(tmp, sizeof(tmp), "QC%x", sig_thread_); On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > Looks like this will be, at largest, 11 bytes. Done. http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:226: SetProperty("C", tmp); On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > Consider using NACL_ARRAY_SIZE macro here -- it will defend you against the > future possibility of tmp becoming a char* instead of a char[], which will cause > sizeof to behave rather differently. Since we have a known max-length and don't want to use magic numbers I've declared a constant and used that here instead. http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:226: SetProperty("C", tmp); On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > Incidentally, isn't there a nice C++ way of doing this? Implicit conversion between string and char * is being deprecated so if you start with C-types you're probably best of sticking with them. And I'm not sure there's a "nice" way to do anything to char[] or char* since std::copy still requires you to first allocate memory. I could use a stringstream to do the formatting but it certainly wouldn't be less code, it certainly wouldn't be faster and, since we know the bounds of this buffer, not really safer. http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.cc:255: sig_done_->Signal(); On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > I see cur_signal_ being set to 0, -1, and actual values. Below, code says it is > "simulating a signal" by setting it to 0, and then resets it to -1. Can you > explain more about the lifecycle of this variable This has never worked. I have an issue to investigate it and fix it properly, along with providing tests. I think we were blocking on a change in actual signal or platform code that kept the actual break from the debugger from ever getting here. http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.h File src/trusted/gdb_rsp/target.h (right): http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.h:69: // MatchQuery does a longest prefix match against the locally cased On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > cased? cached? Done. http://codereview.chromium.org/5633007/diff/25001/src/trusted/gdb_rsp/target.... src/trusted/gdb_rsp/target.h:175: int32_t run_req_; // Requested thread for run operations. On 2010/12/14 21:46:08, Cliff L. Biffle wrote: > These names are not great. They should probably contain the word 'thread.' Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
