NEW DATE! Bugzilla will undergo maintenance 2024-03-28 18h00 CET. Bugzilla will be placed in read-only mode at that time.

Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 322658 - Support for GDB -data-read-memory-bytes
Summary: Support for GDB -data-read-memory-bytes
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf-gdb (show other bugs)
Version: 8.0   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 8.0   Edit
Assignee: Marc Khouzam CLA
QA Contact: Marc Khouzam CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-13 10:15 EDT by Vladimir Prus CLA
Modified: 2015-07-14 05:06 EDT (History)
3 users (show)

See Also:


Attachments
The patch (17.40 KB, patch)
2010-08-13 10:15 EDT, Vladimir Prus CLA
no flags Details | Diff
Revision 2 (24.32 KB, patch)
2010-08-14 06:17 EDT, Vladimir Prus CLA
no flags Details | Diff
Updated patch with comments (26.48 KB, patch)
2010-08-16 16:00 EDT, Marc Khouzam CLA
marc.khouzam: iplog-
Details | Diff
Updated patch with proper @since tag (25.76 KB, patch)
2010-08-17 10:49 EDT, Marc Khouzam CLA
marc.khouzam: iplog-
Details | Diff
Revision 3 (14.88 KB, patch)
2010-08-18 06:44 EDT, Vladimir Prus CLA
no flags Details | Diff
Revision 4 (24.74 KB, patch)
2010-08-18 10:49 EDT, Vladimir Prus CLA
no flags Details | Diff
Revision 5 (25.12 KB, patch)
2010-08-18 15:18 EDT, Vladimir Prus CLA
no flags Details | Diff
Patch that applies cleanly (26.17 KB, patch)
2010-08-18 15:44 EDT, Marc Khouzam CLA
marc.khouzam: iplog-
Details | Diff
Revision 6 (27.09 KB, patch)
2010-08-20 13:09 EDT, Vladimir Prus CLA
marc.khouzam: iplog+
Details | Diff
Final solution (28.15 KB, patch)
2010-08-20 14:26 EDT, Marc Khouzam CLA
marc.khouzam: iplog-
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vladimir Prus CLA 2010-08-13 10:15:02 EDT
Build Identifier: 

I've just checked in a GDB patch that introduces new command for reading memory -- data-read-memory-bytes. It's biggest advantage is that it can detect when the requested memory range is partially readable and return the readable portion. This patch makes DSF use the new command when available.

Reproducible: Always
Comment 1 Vladimir Prus CLA 2010-08-13 10:15:37 EDT
Created attachment 176558 [details]
The patch
Comment 2 Marc Khouzam CLA 2010-08-13 10:25:28 EDT
Did you turn on API tooling?
http://wiki.eclipse.org/CDT/policy#Using_API_Tooling
Comment 3 Marc Khouzam CLA 2010-08-13 11:37:19 EDT
Thanks for the -list-features support, and the overall patch.

Comments below:




- missing @since tags

- please add copyright text at the top of new files

- please add javadoc to the new commands/output files, as in the existing files



MIDataReadMemoryBytes:

-Did you mean to have the context be IDMContext?  Usually, we have a more specific context to indicate to the user which context the command expects.  Probably IMemoryDMContext in this case.  (MIDataReadMemory should have been doing the same)



MIDataReadMemoryBytesInfo:

- you can use getMIOutput() instead of declaring a new fOutput.

- don't we want to parse the "begin" and "end" tokens?

- why do we need getMIMemoryBlock() to take a count?  -data-read-memory-bytes already takes a count, don't we have a fixed sized result from GDB?



GDBControl_7_0:

- please add the new ListFeaturesStep in shutdown(), in reverse order
- duplicate call to rm.done() in ListFeaturesStep#initialize()
-in listFeatures, don't override rm.done(), but instead rm.handleSuccess().



GDBControl.java:

- Please add a comment to say we don't support getFeatures() in earlier versions of GDB.

- Make fFeatures private



MIMemory:

- issuing -list-features as an initialization step of the control service, is good because it means other services can have access to the result when they initialize (as opposed to using FinalLaunchSequence to issue the command, which I would normally prefer).  We therefore should set a global flag in doInitialize() of MIMemory, instead of calling the controlService each time readMemoryBlock() is called. 

- can you create constants for the different GDB features of -list-features?  Or at least for "data-read-memory-bytes".  You'll probably need a new constants file e.g., IGDBDebugConstants

- how about having createInvalidBlock() return the block, instead of passing it the drm?  createInvalidBlock() does not seem to the right place to call rm.done(), it is limiting.



IGDBControl:

- javadoc for getFeatures() especially to say it cannot return null.
Comment 4 Vladimir Prus CLA 2010-08-14 05:29:07 EDT
I did not have API tooling enabled. I have enabled it now, using 7.0.0 as baseline. I see quite a number of issues, many unrelated to this patch. Do you get clean build?
Comment 5 Vladimir Prus CLA 2010-08-14 06:15:48 EDT
> - missing @since tags

What should I put in @since?

> - please add copyright text at the top of new files

Done.
 
> - please add javadoc to the new commands/output files, as in the existing files

Done.

> MIDataReadMemoryBytes:
> 
> -Did you mean to have the context be IDMContext?  Usually, we have a more
> specific context to indicate to the user which context the command expects. 
> Probably IMemoryDMContext in this case.  (MIDataReadMemory should have been
> doing the same)

I actually meant to do exactly the same as MIDataReadMemory. In fact, given
that IMemoryDMContext does not add anything to IDMContext, why is the
point?


> MIDataReadMemoryBytesInfo:
> 
> - you can use getMIOutput() instead of declaring a new fOutput.

Thanks, changed.

> - don't we want to parse the "begin" and "end" tokens?

Well, since we don't use them, we probably don't have to parse them either.

> - why do we need getMIMemoryBlock() to take a count?  -data-read-memory-bytes
> already takes a count, don't we have a fixed sized result from GDB?

No, we don't. GDB responds with a list of blocks it could read. Say, if you ask
for 100 bytes, GDB might give you a block of 70 bytes, with 'offset' of 0. Since
the rest of DSF/eclipse wants a block of 100 bytes, we pass '100' to this method
and it returns a block of 100 bytes, filling the first 70 bytes with response from
GDB.

> GDBControl_7_0:
> 
> - please add the new ListFeaturesStep in shutdown(), in reverse order
> - duplicate call to rm.done() in ListFeaturesStep#initialize()
> -in listFeatures, don't override rm.done(), but instead rm.handleSuccess().

Done.

> GDBControl.java:
> 
> - Please add a comment to say we don't support getFeatures() in earlier
> versions of GDB.
> 
> - Make fFeatures private

Done.

> MIMemory:
> 
> - issuing -list-features as an initialization step of the control service, is
> good because it means other services can have access to the result when they
> initialize (as opposed to using FinalLaunchSequence to issue the command, which
> I would normally prefer).  We therefore should set a global flag in
> doInitialize() of MIMemory, instead of calling the controlService each time
> readMemoryBlock() is called. 

Done.

> - can you create constants for the different GDB features of -list-features? 
> Or at least for "data-read-memory-bytes".  You'll probably need a new constants
> file e.g., IGDBDebugConstants

Do you think this is really needed? Those strings will not ever change on GDB
side, and while declaring a constant will prevent a typo on DSF side, such
typo can only go undetected if the code in question is not tested at all.

> - how about having createInvalidBlock() return the block, instead of passing it
> the drm?  createInvalidBlock() does not seem to the right place to call
> rm.done(), it is limiting.

Done.

> IGDBControl:
> 
> - javadoc for getFeatures() especially to say it cannot return null.

Done.
Comment 6 Vladimir Prus CLA 2010-08-14 06:17:20 EDT
Created attachment 176597 [details]
Revision 2

I believe all comments are addressed, here, except for "since" tags and adding constants for feature names.
Comment 7 Marc Khouzam CLA 2010-08-14 14:54:30 EDT
For the @since tags, if you click on the error icon in the editor margin, it will offer to add the @since tag for you; the tool will figure out what version to put automatically (4.0 for cdt.dsf.gdb and 2.2 for cdt.dsf.gdb.ui)
Comment 8 Marc Khouzam CLA 2010-08-14 17:57:12 EDT
(In reply to comment #5)

I haven't reviewed the patch yet, but I'll answer a couple of questions.
I'll also have to update to GDB HEAD to test the patch.

> > -Did you mean to have the context be IDMContext?  Usually, we have a more
> > specific context to indicate to the user which context the command expects. 
> > Probably IMemoryDMContext in this case.  (MIDataReadMemory should have been
> > doing the same)
> I actually meant to do exactly the same as MIDataReadMemory. In fact, given
> that IMemoryDMContext does not add anything to IDMContext, why is the
> point?

Putting the more specific context helps someone else that uses MIDataReadMemoryBytes to know which context it should use.  Also, it tells the compiler that if someone passes in, say, an IExecutionDMContext, that it should be an error.

> > - don't we want to parse the "begin" and "end" tokens?
> Well, since we don't use them, we probably don't have to parse them either.

But we may have someone else wanting to use this new class, so parsing all tokens allows for the class to be complete, and its functionality to be readily available; also, it potentially avoids an obvious future change that may break the API.  I'm not sure if the begin/end tokens are something that will ever be useful; you would know best, since you put them in GDB :-)  I leave it up to you if you want to parse them or not.

> > - why do we need getMIMemoryBlock() to take a count?  -data-read-memory-bytes
> > already takes a count, don't we have a fixed sized result from GDB?
> No, we don't. GDB responds with a list of blocks it could read. Say, if you ask
> for 100 bytes, GDB might give you a block of 70 bytes, with 'offset' of 0.
> Since
> the rest of DSF/eclipse wants a block of 100 bytes, we pass '100' to this
> method
> and it returns a block of 100 bytes, filling the first 70 bytes with response
> from GDB.

It is probably good for MIDataReadMemoryBytesInfo to return the same number of bytes that GDB returned.  The caller would then know how much GDB was able to read.  The caller would then create the full array of 100 bytes.

> > - can you create constants for the different GDB features of -list-features? 
> > Or at least for "data-read-memory-bytes".  You'll probably need a new constants
> > file e.g., IGDBDebugConstants
> Do you think this is really needed? Those strings will not ever change on GDB
> side, and while declaring a constant will prevent a typo on DSF side, such
> typo can only go undetected if the code in question is not tested at all.

Ok, but at least put the contant as a private final static field of the class itself.
Comment 9 Marc Khouzam CLA 2010-08-16 16:00:01 EDT
Created attachment 176720 [details]
Updated patch with comments

I hadn't actually applied the patch before, so I found a couple of new issues now that I applied it.  To continue testing the patch, I actually had to make the changes myself, so here is the updated patch based on my comments below, which I'm putting for your information.

- javadoc should start with /**

- patch does not apply cleanly to HEAD

- updates to @since tags

- GDBMemory_7_0#readMemoryBlock no longer compiles due to changes to MIMemory#readMemoryBlock.  Now, looking at what GDBMemory_7_0#readMemoryBlock, I see that it passes an IMIExecutionDMContext to the base class.  I think this was a bit of a hack that should have been done a little more cleanly.  Nevertheless, that means we can't force readMemoryBlock() to take an IMemoryDMC but need to leave it to IDMContext.  Not only that, but if you want you new command to be called, you can't even have MIDataReadMemoryBytes use IMemoryDMC like I asked.  So, sorry about the change back, but we'll need to accept that MIDataReadMemoryBytes will take only an IDMContext.

MIMemory:

- Instead of fetching both ICommandControlService and IGDBControl, which are both the same service, you can change the line
   ICommandControlService commandControl = getServicesTracker().getService(ICommandControlService.class);
for
   IGDBControl commandControl = getServicesTracker().getService(IGDBControl.class);
- please make a constant for "data-read-memory-bytes" and add //$NON-NLS-1$ at the end of the line to indicate the string does not need to be translated to another language.

MIDataReadMemoryBytesInfo:
-remove the //FIXME

GDBControl_7_0:
- please remove 'synchronized' to the overriding of handleSuccess() in listFeatures()


The last issue that needs your input is:

(In reply to comment #8)
> > > - why do we need getMIMemoryBlock() to take a count?  -data-read-memory-bytes
> > > already takes a count, don't we have a fixed sized result from GDB?
> > No, we don't. GDB responds with a list of blocks it could read. Say, if you ask
> > for 100 bytes, GDB might give you a block of 70 bytes, with 'offset' of 0.
> > Since
> > the rest of DSF/eclipse wants a block of 100 bytes, we pass '100' to this
> > method
> > and it returns a block of 100 bytes, filling the first 70 bytes with response
> > from GDB.
> 
> It is probably good for MIDataReadMemoryBytesInfo to return the same number of
> bytes that GDB returned.  The caller would then know how much GDB was able to
> read.  The caller would then create the full array of 100 bytes.

I still have to run a couple more tests.
Comment 10 Marc Khouzam CLA 2010-08-16 16:01:04 EDT
Adding Mikhail.
Once the patch is ready (not yet), if Mikhail is willing to commit it, it will avoid having to go through an IP review.
Comment 11 Nobody - feel free to take it CLA 2010-08-16 16:04:11 EDT
(In reply to comment #10)
> Adding Mikhail.
> Once the patch is ready (not yet), if Mikhail is willing to commit it, it will
> avoid having to go through an IP review.

Nice try, Marc :)
Comment 12 Vladimir Prus CLA 2010-08-17 04:11:35 EDT
Marc,
thanks for adjustments.

About the last issue -- the problem is that GDB may even return several disjoint memory ranges, so to accommodate that we'd need pretty extensive
changes all over. It seems that passing the expected size of buffer to
getMIMemoryBlock() is a reasonable boundary when this patch should stop
changing things.
Comment 13 Marc Khouzam CLA 2010-08-17 09:42:47 EDT
Volodya, were you able to test the patch with GDB HEAD?  I don't think DSF-GDB works with GDB HEAD...
Comment 14 Vladimir Prus CLA 2010-08-17 09:57:32 EDT
Oh, I was testing with our version of GDB, which should be fairly close to CVS HEAD. I'll get with CVS HEAD and report the result.
Comment 15 Marc Khouzam CLA 2010-08-17 10:01:46 EDT
(In reply to comment #14)thread-group-created
> Oh, I was testing with our version of GDB, which should be fairly close to CVS
> HEAD. I'll get with CVS HEAD and report the result.

Your version of GDB does not change "=thread-group-created" to "=thread-group-started"?

I've fixed DSF-GDB for urgent changes needed for GDB 7.2 as part of Bug 315796.  You may want to make sure you have those changes yourself.  Be aware that pserver checkouts may not reflect my commit right away.
Comment 16 Marc Khouzam CLA 2010-08-17 10:49:04 EDT
Created attachment 176790 [details]
Updated patch with proper @since tag

I had forgotten to change a @since tag
Comment 17 Marc Khouzam CLA 2010-08-17 11:22:07 EDT
Last few comments:

MIDataReadMemoryBytesInfo
- in getMIMemoryBlock() let's make sure that 
     offset + contents.length()/2 <= block.length()

- in getMIMemoryBlock() let's put try/catch (NumberFormatException) around both Integer.decode and Integer.parseInt (you an use two try/catch or a single global one), and if it happens, return block as it stands at the time.

- I think the result of getMIMemoryBlock() should be stored in a global variable, to avoid potentially parsing the result record multiple times.  Something like
  if (fMemoryBlock == null) {
     <parsing code>
     fMemoryBlock = block;
  }
  return fMemoryBlock;

(In reply to comment #12)
> About the last issue -- the problem is that GDB may even return several
> disjoint memory ranges, so to accommodate that we'd need pretty extensive
> changes all over. It seems that passing the expected size of buffer to
> getMIMemoryBlock() is a reasonable boundary when this patch should stop
> changing things.

I understand now.  However, I find it a brittle API to require the caller to pass the same number of bytes to MIDataReadMemoryBytes() and to MIDataReadMemoryBytesInfo#getMIMemoryBlock(); if the user does not, we could end up in a bad situation.  Instead, how about having MIDataReadMemoryBytesInfo's constructor take a numBytes parameter, which will be passed in by MIDataReadMemoryBytes() who knows that info already?
Comment 18 Nobody - feel free to take it CLA 2010-08-17 11:22:55 EDT
(In reply to comment #15)
> (In reply to comment #14)thread-group-created
> > Oh, I was testing with our version of GDB, which should be fairly close to CVS
> > HEAD. I'll get with CVS HEAD and report the result.
> 
> Your version of GDB does not change "=thread-group-created" to
> "=thread-group-started"?
> 
> I've fixed DSF-GDB for urgent changes needed for GDB 7.2 as part of Bug 315796.
>  You may want to make sure you have those changes yourself.  Be aware that
> pserver checkouts may not reflect my commit right away.

Our GDB uses "=thread-group-started", I am working on the required changes.
Comment 19 Marc Khouzam CLA 2010-08-17 11:26:01 EDT
(In reply to comment #18)

> Our GDB uses "=thread-group-started", I am working on the required changes.

So this patch was tested with your own GDB and your own DSF-GDB, which already supports 7.2?
Comment 20 Nobody - feel free to take it CLA 2010-08-17 11:34:28 EDT
(In reply to comment #19)
> (In reply to comment #18)
> 
> > Our GDB uses "=thread-group-started", I am working on the required changes.
> 
> So this patch was tested with your own GDB and your own DSF-GDB, which already
> supports 7.2?

I made (simple) changes to make "=thread-group-started" work for our GDB that already supports 7.2 and was about to submit a patch to CDT but you beat me :) It is not related to this particular patch.
Comment 21 Vladimir Prus CLA 2010-08-18 06:44:09 EDT
Created attachment 176881 [details]
Revision 3
Comment 22 Vladimir Prus CLA 2010-08-18 06:44:36 EDT
Marc, I have attached a patch with your last comments addressed.
Comment 23 Marc Khouzam CLA 2010-08-18 10:10:37 EDT
(In reply to comment #22)
> Marc, I have attached a patch with your last comments addressed.

The patch is missing a whole bunch of files :-)
Comment 24 Vladimir Prus CLA 2010-08-18 10:49:18 EDT
Created attachment 176898 [details]
Revision 4
Comment 25 Vladimir Prus CLA 2010-08-18 10:49:32 EDT
Ouch, this one should be better.
Comment 26 Marc Khouzam CLA 2010-08-18 12:06:28 EDT
The latest patch does not apply.  I don't think it was generated by Eclipse...
Also,

MIDataReadMemoryBytesInfo
- you have to set fSize in the constructor.  I'm surprised it worked at all.  In fact, to simplify things, why not create a parse(int size) method that would be called from the constructor and parse the memory block (like MIListFeaturesInfo does), then we don't need to store the size as a global.

The rest looks good.  I'll test the patch when you post it again and then we can see how we commit it.

Does CodeSourcery have a Member Committer Agreement?


According to the Eclipse Legal Process 

http://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf

 

if the code is "Written 100% by employees of the same employer as the Submitting Committer: (a) under the supervision of the PMC; and (b) where the employer has signed a Member Committer Agreement." it can be committed directly by Mikhail.
Comment 27 Nobody - feel free to take it CLA 2010-08-18 12:10:04 EDT
(In reply to comment #26)
> Does CodeSourcery have a Member Committer Agreement?

I haven't found it out yet, but if we do not have a Member Committer Agreement I'll commit the patch.
Comment 28 Marc Khouzam CLA 2010-08-18 13:59:05 EDT
(In reply to comment #27)
> (In reply to comment #26)
> > Does CodeSourcery have a Member Committer Agreement?
> 
> I haven't found it out yet, but if we do not have a Member Committer Agreement
> I'll commit the patch.

I assume you meant "if we _do have_ a Member Committer Agreement"
you are willing to commit the patch.  If you don't have that agreement, I guess
I'll have to go through IPReview.
Comment 29 Nobody - feel free to take it CLA 2010-08-18 14:09:39 EDT
(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > Does CodeSourcery have a Member Committer Agreement?
> > 
> > I haven't found it out yet, but if we do not have a Member Committer Agreement
> > I'll commit the patch.
> 
> I assume you meant "if we _do have_ a Member Committer Agreement"
> you are willing to commit the patch.  If you don't have that agreement, I guess
> I'll have to go through IPReview.

I didn't mean it, but now I think that you are right.
Comment 30 Vladimir Prus CLA 2010-08-18 15:18:21 EDT
Created attachment 176934 [details]
Revision 5

Another revision. Introduces the parse() method in MIDataReadMemoryBytesInfo. Also, the patch is generated with the right prefix.
Comment 31 Marc Khouzam CLA 2010-08-18 15:44:21 EDT
Created attachment 176936 [details]
Patch that applies cleanly

I'm not sure why but the patch still didn't apply perfectly.  Attached is one that does, exactly like the one called "Revision 5".
Comment 32 Marc Khouzam CLA 2010-08-18 15:49:10 EDT
(In reply to comment #31)
> Created an attachment (id=176936) [details]
> Patch that applies cleanly

Ok, this patch is the one to commit.

Mikhail, once you know, please let me know if you will commit or if I have to start the IPReview process.

Thanks everyone.
Comment 33 Nobody - feel free to take it CLA 2010-08-18 16:07:15 EDT
(In reply to comment #32)
> Mikhail, once you know, please let me know if you will commit or if I have to
> start the IPReview process.

Marc, as far as I understand from your previous comment it doesn't matter who commits the patch, you or me. What matters is that Vladimir doesn't have the official permission from his employer to submit this patch. Is that correct?

Now I know that CodeSourcery didn't sign the Member Committer Agreement. We signed the employer consent forms for Dmitry and myself. The same form can be signed for Vladimir or we can sign the Member Committer Agreement.
Comment 34 Marc Khouzam CLA 2010-08-19 09:22:44 EDT
(In reply to comment #33)
> (In reply to comment #32)
> > Mikhail, once you know, please let me know if you will commit or if I have to
> > start the IPReview process.
> 
> Marc, as far as I understand from your previous comment it doesn't matter who
> commits the patch, you or me. What matters is that Vladimir doesn't have the
> official permission from his employer to submit this patch. Is that correct?

I don't know this stuff very well, but from what I see in the Eclipse legal process poster, here is what I understood.

Since the patch is more than 250 lines, it cannot be committed directly, since Vladimir is not a committer.  Therefore we would have to get an IP Review to get approval (which may not be too bad, I just never did it before).  However, a way to get around that is to find a committer that works for the same company as Vladimir and have that person (you) do the commit.  But we need the employer to have signed the Member Committer Agreement it seems.

So, since CodeSourcery does not have such an agreement, I'll just file a Contribution Questionnaire to get the review process started.

Thanks for trying though :-)
Comment 35 Nobody - feel free to take it CLA 2010-08-19 09:48:31 EDT
(In reply to comment #34)
> I don't know this stuff very well, but from what I see in the Eclipse legal
> process poster, here is what I understood.
> 
> Since the patch is more than 250 lines, it cannot be committed directly, since
> Vladimir is not a committer.  Therefore we would have to get an IP Review to
> get approval (which may not be too bad, I just never did it before).  However,
> a way to get around that is to find a committer that works for the same company
> as Vladimir and have that person (you) do the commit.  But we need the employer
> to have signed the Member Committer Agreement it seems.
> 
> So, since CodeSourcery does not have such an agreement, I'll just file a
> Contribution Questionnaire to get the review process started.
> 
> Thanks for trying though :-)

I have done IP reviews before and it usually takes some time. The patch becomes outdated. In theory, the updated patch needs a new review. Anyway, if you want to try it, go ahead.
Comment 36 Marc Khouzam CLA 2010-08-19 10:07:17 EDT
(In reply to comment #35)

> I have done IP reviews before and it usually takes some time. The patch becomes
> outdated. In theory, the updated patch needs a new review. Anyway, if you want
> to try it, go ahead.

I'm gonna have to do it for the pretty-printing patch, so I don't really want to do it for this patch if not needed.  Do you have a easier way?
Comment 37 Nobody - feel free to take it CLA 2010-08-19 10:30:24 EDT
(In reply to comment #36)
> I'm gonna have to do it for the pretty-printing patch, so I don't really want
> to do it for this patch if not needed.  Do you have a easier way?

As far as I understand, CodeSourcery can sign the Employer Consent Form for Vladimir as we did it for Dmitry, or CodeSourcery can sign the Member Agreement. I don't know the details and how long it could take. And haven't got a response yet which of these options we are going to use.
So if you are not keen to start an IP review, I suggest to wait for the response.
Comment 38 Marc Khouzam CLA 2010-08-19 10:33:20 EDT
(In reply to comment #37)
> (In reply to comment #36)
> > I'm gonna have to do it for the pretty-printing patch, so I don't really want
> > to do it for this patch if not needed.  Do you have a easier way?
> 
> As far as I understand, CodeSourcery can sign the Employer Consent Form for
> Vladimir as we did it for Dmitry, or CodeSourcery can sign the Member
> Agreement. I don't know the details and how long it could take. And haven't got
> a response yet which of these options we are going to use.
> So if you are not keen to start an IP review, I suggest to wait for the
> response.

Ok, I'll wait until you have more news.  Thanks.
Comment 39 Nobody - feel free to take it CLA 2010-08-19 14:33:36 EDT
(In reply to comment #38)
> Ok, I'll wait until you have more news.  Thanks.

Marc, CodeSourcery have signed and faxed to Eclipse Foundation the Employer Consent Form (http://www.eclipse.org/legal/employer_consent.pdf) for Vladimir. It doesn't matter whether the individual is a committer or not, this form is just a permission given by an employer to an employee that he or she can submit patches to Eclipse. 
My understanding is that you can proceed with this patch, but you might want to clarify it with the Eclipse Foundation folks.
Comment 40 Marc Khouzam CLA 2010-08-19 22:22:22 EDT
(In reply to comment #39)
> (In reply to comment #38)
> > Ok, I'll wait until you have more news.  Thanks.
> 
> Marc, CodeSourcery have signed and faxed to Eclipse Foundation the Employer
> Consent Form (http://www.eclipse.org/legal/employer_consent.pdf) for Vladimir.
> It doesn't matter whether the individual is a committer or not, this form is
> just a permission given by an employer to an employee that he or she can submit
> patches to Eclipse. 
> My understanding is that you can proceed with this patch, but you might want to
> clarify it with the Eclipse Foundation folks.

I didn't know who to ask, so I opened a CQ in IPzilla and I asked there.  I'm covering all bases :-)

Volodya can you confirm:
- that you yourself wrote 100% of the new code you are submitting
Comment 41 Vladimir Prus CLA 2010-08-20 01:21:22 EDT
Yes, 100% of the code that is being submitted was written by myself.
Comment 42 Marc Khouzam CLA 2010-08-20 11:07:46 EDT
The CQ I opened is CQ 4429 (https://dev.eclipse.org/ipzilla/show_bug.cgi?id=4429)

It says:
> It's also important to ensure the Contributor Section of the header 
> contains the contributor's name.

Volodya, can you repost the patch adding your name to the header of _each_ file you modified?  Something like:

Vladimir Prus (CodeSourcery) - Support for -data-read-memory-bytes (bug 322658)
Comment 43 Vladimir Prus CLA 2010-08-20 11:35:29 EDT
Should the *new* files contain by name too, or just company name as they have now?
Comment 44 Marc Khouzam CLA 2010-08-20 11:39:22 EDT
(In reply to comment #43)
> Should the *new* files contain by name too, or just company name as they have
> now?

I don't know for sure, so let's put your name everywhere, to be safe.  I'm hoping to get more info about this for the next time.
Comment 45 Marc Khouzam CLA 2010-08-20 11:40:59 EDT
For future contributions, from CQ 4429:

> The employer [CodeSourcery] is not an Eclipse member which means they 
> couldn't sign a Member Committer Agreement.  Therefore, Figure 2 of 
> the Due Diligence Process does not apply. Figure 3 does apply; and 
> as the content is over 250 loc, it requires review.

So, for CodeSourcery contributions not written by Mikhail, we will need to go through the CQ process.
Comment 46 Vladimir Prus CLA 2010-08-20 13:09:12 EDT
Created attachment 177115 [details]
Revision 6

Revision with copyright updates in all affected files, as requested by Marc.
Comment 47 Marc Khouzam CLA 2010-08-20 13:16:08 EDT
(In reply to comment #46)
> Created an attachment (id=177115) [details]
> Revision 6
> 
> Revision with copyright updates in all affected files, as requested by Marc.

Not an eclipse patch.
Comment 48 Marc Khouzam CLA 2010-08-20 14:26:51 EDT
Created attachment 177129 [details]
Final solution

(In reply to comment #47)
> (In reply to comment #46)
> > Created an attachment (id=177115) [details] [details]
> > Revision 6
> > 
> > Revision with copyright updates in all affected files, as requested by Marc.
> 
> Not an eclipse patch.

It's ok, I was able to apply with only minor conflicts that I fixed manually.
This is the resulting final patch.
Comment 49 Nobody - feel free to take it CLA 2010-08-26 09:26:54 EDT
(In reply to comment #45)
> For future contributions, from CQ 4429:
> 
> > The employer [CodeSourcery] is not an Eclipse member which means they 
> > couldn't sign a Member Committer Agreement.  Therefore, Figure 2 of 
> > the Due Diligence Process does not apply. Figure 3 does apply; and 
> > as the content is over 250 loc, it requires review.
> 
> So, for CodeSourcery contributions not written by Mikhail, we will need to go
> through the CQ process.

I've have double-checked with Doug and he confirmed it. Please, start the process.
Comment 50 Marc Khouzam CLA 2010-08-26 13:06:17 EDT
The patch was just approved.
I will be committing it soon.
Comment 51 Marc Khouzam CLA 2010-08-27 10:02:12 EDT
Thanks for the contribution Volodya.

Committed to HEAD.
Comment 52 Vladimir Prus CLA 2010-08-27 11:03:53 EDT
Marc,

thanks for reviewing all iterations of this patch -- this is much appreciated!
Comment 53 Jonah Graham CLA 2015-07-14 05:06:36 EDT
(In reply to Marc Khouzam from comment #8)
> (In reply to comment #5)
> > > - don't we want to parse the "begin" and "end" tokens?
> > Well, since we don't use them, we probably don't have to parse them either.
> 
> But we may have someone else wanting to use this new class, so parsing all
> tokens allows for the class to be complete, and its functionality to be
> readily available; also, it potentially avoids an obvious future change that
> may break the API.  I'm not sure if the begin/end tokens are something that
> will ever be useful; you would know best, since you put them in GDB :-)  I
> leave it up to you if you want to parse them or not.

As a note to anyone in the future using these commands (as we were) who ends up in this bug because MIDataReadMemoryBytesInfo does not appear to parse the result as per the GDB docs https://sourceware.org/gdb/onlinedocs/gdb/GDB_002fMI-Data-Manipulation.html. It is important to note that AFAIK the code works perfectly well with stock GDB.

Our MI commands looked like:
32-data-read-memory-bytes -o 1 419752 1
32^done,memory=[{begin="0x667a8",offset="0x1",end="0x667a8",contents="fe"}]

which caused the MIDataReadMemoryBytesInfo code to assume that the "fe" contents were for address 0x667aa instead of 0x667a9 because the begin token is not parsed. This is because the code implicitly assumes that the "begin" token would have value 0x667a9 (as it does in GDB for the same sequence). GDB does something like this:

32-data-read-memory-bytes -o 1 419752 1
32^done,memory=[{begin="0x667a9",offset="0x0",end="0x667aa",contents="fe"}]