Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 350837

Summary:

AbstractMIControl.startCommandProcessing() uses concrete GDBBackend class to create stderr thread

Product:

[Tools] CDT

Reporter:

Andy Jin <ajin>

Component:

cdt-debug-dsf-gdb

Assignee:

Marc Khouzam <marc.khouzam>

Status:

RESOLVED FIXED

QA Contact:

Marc Khouzam <marc.khouzam>

Severity:

normal

Priority:

CC:

cdtdoug, nobody, pawel.1.piech

Version:

8.0

Flags:

nobody: review+

Target Milestone:

8.1.0

Hardware:

OS:

Windows 7

Whiteboard:

Attachments:

Description	Flags
Check for null in cdt_8_0	marc.khouzam: iplog-
API change for master	marc.khouzam: iplog-
Fix for master using IMIBackend2	marc.khouzam: iplog-

Description Andy Jin

2011-06-30 10:13:22 EDT

Build Identifier: I20110613-1736

I am migrating to CDT8. In DSF debug I have customized IGDBBackend for domain specific implementations.

New in CDT8, the AbstractMIControl.startCommandProcessing() method creates a stderr stream thread to fix bug 327617. However when creating the error thread it uses the concrete GDBBackend class so that my customized IGDBBackend class is not being used.

***Reply from Marc Khouzam***

The fix for bug 327617 came after API freeze, so I couldn't add the hook I needed to IMIBackend.
That interface has getMIInputStream() and getMIOutputStream() but does not have getMIErrorStream()

The way I found to work around this limitation was to get the gdbProcess from GDBBackend directly, but that method is not part of an interface either.

I assume that your version of IGDBBackend does not extend GDBBackend and that you are getting an NPE?

I think overriding startCommandProcessing() is your best approach.
The other thing you could do is have your IGDBBackend service extend GDBackend which is probably more change than overriding.

I suggest writing a bug to properly use IMIBackend for this.

Thanks
Marc


Reproducible: Always

Comment 1 Marc Khouzam

2011-08-03 10:19:51 EDT

Created attachment 200816 [details]
Check for null in cdt_8_0

Since I cannot change the API in the maintenance branch, the best I can do is make sure we don't get an NPE.  For people that don't have a service named GDBBackend, it will simply not create the ErrorThread to read output on stderror.  In such a case the fix for Bug 327617 is broken, but it is better than an NPE.

I'll commit this to cdt_8_0 and do a proper fix for master.

Comment 2 Marc Khouzam

2011-08-03 10:37:32 EDT

Created attachment 200818 [details]
API change for master

This patch adds a 
    public InputStream getMIErrorStream();
to IMIBackend.java

This will require a major version change, or I will need to create a new interface e.g., IMIBackend2.

I'll leave this patch without committing until we decide what we want to do with the version for the next release.

I'll add a target to remind myself to resolve this eventually.

Comment 3 CDT Genie

2011-08-03 11:23:15 EDT

*** cdt git genie on behalf of Marc Khouzam ***

    Bug 350837: AbstractMIControl.startCommandProcessing() uses concrete GDBBackend class to create stderr thread

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

Comment 4 Marc Khouzam

2012-03-05 07:59:44 EST

Comment on attachment 200818 [details]
API change for master

Since the next version of CDT will be 8.1, I cannot modify the existing IMIBackend interface.

Because our current hierarchy for the backend service looks like this:

IMIBackend
  IGDBBackend
    GDBBackend 

Multiple solutions are possible.
1- Create IGDBBackend2 with getMIErrorStream().  I don't like that because getMIInputStream() and getMIOutputStream() are both in IMIBackend and not IGDBBackend.

2- Create IMIBackend2.  This is fine, except that we cannot modify IGDBBackend to extend it or it would break the API.  So we would need to create IGDBBakend2 also, to extend IMIBackend2.  This seems excessive to me.

3- Create IMIBackend2 and treat it as a separate interface than IGDBBackend.  We just make GDBBackend implement it explicitly. 

I will post a patch using #3 for review.

Comment 5 Marc Khouzam

2012-03-05 08:59:44 EST

Created attachment 212065 [details]
Fix for master using IMIBackend2

Here is the fix I propose.

The solution is straightforward, but the interface hierarchy makes the solution less elegant.

Mikhail, do you agree with my approach?

Comment 6 Nobody - feel free to take it

2012-03-05 10:25:42 EST

The fix looks fine. I don't think we need IGDBBackend2 for this purpose.

Comment 7 Marc Khouzam

2012-03-05 10:55:28 EST

(In reply to comment #6)
> The fix looks fine. I don't think we need IGDBBackend2 for this purpose.

Thanks for the quick answer.
I committed the fix to master.

Comment 8 CDT Genie

2012-03-05 11:23:04 EST

*** cdt git genie on behalf of Marc Khouzam ***

    Bug 350837: Need an interface to access the GDB error stream.

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