Bug 341762

Comment 1 John Dallaway 2011-04-04 06:51:29 EDT

Created attachment 192447 [details]
MIDataReadMemory.patch

Fix for reported issue.

Comment 2 Marc Khouzam 2011-04-04 10:16:14 EDT

I don't know the details of this but this may have been done on purpose.

I'm copying Francois to see if he remembers.

Comment 3 Francois Chouinard 2011-04-04 11:10:36 EDT

I don't remember the details but, off the top of my head, this was related to a problem we had in MIDataReadMemoryInfo.parseResult() which is expected to store its result in a byte array (IMemory.getMemory() expects a MemoryByte[]).

As a simplification, I "temporarily" set the word_size to 1 and adjusted the number of bytes requested accordingly. I don't think I would have done that if there had been a simple MIDataReadMemoryInfo fix, but things might have changed since then.

I would recommend testing thoroughly to make sure this patch covers all angles.

Comment 4 John Dallaway 2011-04-04 11:48:27 EDT

I can confirm that the patched version of MIDataReadMemory works correctly in conjunction with the existing version of MIDataReadMemoryInfo#parseResult() for the case rows == 1, word_size == 4.

The patch has no overall effect for the case word_size == 1.

The case rows > 1, word_size > 1 has not been tested.

Comment 5 John Dallaway 2011-09-09 22:19:22 EDT

(In reply to comment #3)

> I would recommend testing thoroughly to make sure this patch covers all angles.

Can I suggest that MIDataReadMemory is fixed in HEAD now to give time for any possible issues to surface before Juno?

Comment 6 Marc Khouzam 2011-09-12 09:46:00 EDT

(In reply to comment #5)
> (In reply to comment #3)
> 
> > I would recommend testing thoroughly to make sure this patch covers all angles.
> 
> Can I suggest that MIDataReadMemory is fixed in HEAD now to give time for any
> possible issues to surface before Juno?

If we are to commit it, it should be ASAP.  However, since I don't understand all the possible impacts, let me ask, why you want this fix?

Comment 7 John Dallaway 2011-09-12 11:49:50 EDT

(In reply to comment #6)

> If we are to commit it, it should be ASAP.  However, since I don't understand
> all the possible impacts, let me ask, why you want this fix?

One of my own plugins attempts to read memory as 4 byte words via this method. I have a workaround in place and a fix is not urgent from my perspective, but it still seems sensible to fix for future releases.

The precise reason for the current erroneous implementation appears to have been lost over time. We do know that the current implementation triggers an error when word_size != 1 so it seems reasonable to assume that CDT is not calling this method with word_size != 1. My patch should not affect behaviour at all when word_size == 1.

Something might possibly break in implementing the fix, but we have quite some time before Juno to discover the breakage and determine the correct fix (or to revert if necessary).

Comment 8 Marc Khouzam 2011-10-26 16:01:26 EDT

Should we also change
   setOptions(new String[] { "-o", Long.toString(offset * word_size)});
to
   setOptions(new String[] { "-o", Long.toString(offset)});

Comment 9 Marc Khouzam 2011-10-26 16:18:57 EDT

Created attachment 206031 [details]
Patch committed to HEAD

(In reply to comment #7)

> The precise reason for the current erroneous implementation appears to have
> been lost over time. We do know that the current implementation triggers an
> error when word_size != 1 so it seems reasonable to assume that CDT is not
> calling this method with word_size != 1. My patch should not affect behaviour
> at all when word_size == 1.

This makes me feel safe enough to go ahead with the fix.
 
> Something might possibly break in implementing the fix, but we have quite some
> time before Juno to discover the breakage and determine the correct fix (or to
> revert if necessary).

Right, so we have over 6 months to detect any surprises.

Attached is the patch I committed to master.

John, can you confirm this is ok with you?

Comment 10 Marc Khouzam 2011-10-26 16:21:05 EDT

Fixed

Comment 11 CDT Genie 2011-10-26 16:23:06 EDT

*** cdt git genie on behalf of John Dallaway ***

    Bug 341762: MIDataReadMemory problem when word_size != 1

[*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=e03958b6ad00d5a09daf0d7f8933d8d2ef4bb153

Comment 12 John Dallaway 2011-10-27 03:21:37 EDT

(In reply to comment #9)

> Attached is the patch I committed to master.
> 
> John, can you confirm this is ok with you?

Yes. Thank you for sorting this out.

(In reply to comment #8)

> Should we also change
>    setOptions(new String[] { "-o", Long.toString(offset * word_size)});
> to
>    setOptions(new String[] { "-o", Long.toString(offset)});

Yes, good catch. "-o" always specifies a _byte_ offset (not a _word_ offset) to be added to the address. Your commit also ensures that the behaviours of the CDI and DSF versions of MIDataReadMemory() are the same.