Bug 337851

Comment 1 Patrick Chuong 2013-07-18 15:54:17 EDT

Hi Toni, I have pushed a patch to Gerrit, can you please talk a look at it to see whether the changes make sense or not? I have tested with GDB/DSF and TI debugger. To enable target access when it is running, I need to extend DisassemblyBackendDsf and override the canDisassemble and getLastKnownAddress methods in my backend. 

https://git.eclipse.org/r/#/c/14669/

Thanks!

Comment 2 Anton Leherbauer 2013-07-19 03:52:02 EDT

Hi Patrick, 

I don't quite understand the line

   if (canDisassemble() || (dmContext instanceof IFrameDMContext)) {

in DisassemblyBackendDsf.setDebugContext()

When the debug context is about to change, how could we determine whether it is possible to disassemble? I would think during a debug context change canDisassemble() must not be called.

Another point is that you do an identity comparison of BigIntegers in retrieveFrameAddressInSessionThread():

   address != BigInteger.valueOf(-1)

You should use equals(). And I would also suggest to use a constant for BigInteger.valueOf(-1).

Comment 3 Patrick Chuong 2013-07-22 13:42:35 EDT

Hi Toni, 

Thanks for taking the time to go through my proposed patch. Regarding the first comment, will it be more clearer if canDisassemble accepts an IAdaptable object? And have the corresponding backend handles the canDisassemble method differently? With such change, the line of code that you are referring to can be simplified to "if (canDisassemble(context))".

i.e 

AbstractDisassemblyBackend::canDisassemble(IAdaptable context) {
    return isSuspended();
}

DisassemblyBackendDsf::canDisassemble(IAdaptable context) {
    return super.canDisassemble(context) ||          
           ((context.getAdapter(IDMVMContext.class).getDMContext) instanceof IFrameDMContext));
}

When setDebugContext is called, the backend should be able to determine whether it is possible to disassemble or not based on the current debug context. Is your concern about not having the context passed to the canDisassemble method?

Comment 4 Anton Leherbauer 2013-07-24 10:32:21 EDT

(In reply to comment #3)
> Hi Toni, 
> 
> Thanks for taking the time to go through my proposed patch. Regarding the
> first comment, will it be more clearer if canDisassemble accepts an
> IAdaptable object? And have the corresponding backend handles the
> canDisassemble method differently? With such change, the line of code that
> you are referring to can be simplified to "if (canDisassemble(context))".

I would suggest to keep the no-argument canDisassemble() method in the common backend interface and add a protected method canDisassembleContext(IDMContext) to DisassemblyBackendDsf.
That way there is a clean separation between the two use cases.

Comment 5 Patrick Chuong 2013-07-24 17:18:45 EDT

Toni, good suggestion. I have pushed the new patch to gerrit. Can you let me know if there is anything else that you would like me to change? Otherwise, I would like to push it to HEAD. Thanks!

Comment 6 Anton Leherbauer 2013-07-25 02:33:46 EDT

(In reply to comment #5)
> Toni, good suggestion. I have pushed the new patch to gerrit. Can you let me
> know if there is anything else that you would like me to change? Otherwise,
> I would like to push it to HEAD. Thanks!

Looks good! Just two minor cosmetic things:

- The Javadoc of canDisassemble() should mention that this is a test for the current context.
- getLastKnownAddress() should return the constant UNKNOWN_ADDRESS.

Thanks.

Comment 7 Patrick Chuong 2013-07-25 12:27:02 EDT

Fixed on master

Comment 8 Patrick Chuong 2013-07-26 09:59:14 EDT

Toni, are you OK for porting this patch back to the 8.2 branch?

I am currently having network connectivity issues, which preventing me from pushing changes to up stream. I'll make the cosmetic changes as you have suggested as soon as my network issue is resolved.

Comment 9 CDT Genie 2013-07-29 17:22:07 EDT

*** cdt git genie on behalf of Patrick Chuong ***

    Bug 337851 - cosmetics

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

Comment 10 Anton Leherbauer 2013-08-19 04:53:14 EDT

(In reply to comment #8)
> Toni, are you OK for porting this patch back to the 8.2 branch?

No, because it breaks binary compatibility.