Community
Participate
Working Groups
Created attachment 191065 [details] trace.log with 4 SESSIONs; 2 prior to EDCTrace patches, and 2 after EDCTrace patches applied the 1st attachment demonstrates that the current implementation of EDCTrace is creating trace.log entries that look like the following: … | org.eclipse.cdt.debug.edc.internal.EDCTrace$EDCTraceWrapper | traceEntry | 137 | … … | org.eclipse.cdt.debug.edc.internal.EDCTrace$EDCTraceWrapper | traceExit | 140 | … … | org.eclipse.cdt.debug.edc.internal.EDCTrace$EDCTraceWrapper | trace | 122 | … i.e. every single item throughout the trace is referred to as EDCTrace$EDCTraceWrapper traceXXX … versus what it should be displaying for such call sites, as follows: … | org.eclipse.cdt.debug.edc.internal.services.dsf.RunControl$ExecutionDMC | contextResumed | 847 | Entering… … | org.eclipse.cdt.debug.edc.internal.services.dsf.RunControl$ExecutionDMC | contextResumed | 856 | Exiting… … | org.eclipse.cdt.debug.edc.internal.services.dsf.RunControl$ExecutionDMC$3$1$1 | run | 614 | Resume command succeeded. | the follow-up patches will contain the patch i have created to fix this problem. (and the 1st attachment actually demonstrates SESSIONS from both before and after the application of the patch to see the difference.) debugging demonstrates that FrameworkDebugTraceEntry() creates an exception, then gets the exception stack info, then loops through that exception's stack info looking for any items whose name is neither FrameworkDebugTraceEntry or EclipseDebugTrace . as soon as it finds one, it uses that as the name of the thing it is tracing. naturally, when EDCTrace wraps the call, the first thing that's not one of those network items is, of course, EDCTrace$EDCTraceWrapper . the patch modifies EDCTrace to attempt to preserve the performance/logical structure desired in the recent changes to create EDCTraceWrapper, but instead of returning a pointer to the EDCTraceWrapper, it instead returns a pointer to the original DebugTrace object, and this allows the stack walk in the internal trace code to find a stack-frame in a place outside the wrapper. to preserve the FixArgument/FixArguments functionality, EDCTrace has been restructured a bit, and requires the caller to call FixArg/FixArgs separetely as might be required for certain objects. please review the attachments to confirm the desired behavior, and then commit as appropriate, as i am not a committer on this project.
Created attachment 191066 [details] patch containing EDCTrace.java changes only, to demonstrate restructuring of pointers
Created attachment 191067 [details] full patch to be applied to pick up all necessary FixArg/FixArgs additions
Ken, David Dubrow is the best person to review these changes. David's rev 1.4 of EDCTrace reintroduced the problem I fixed in rev 1.3. Kirk's proposed changes tries to fix that regression and additionally tweaks additional logic David's 1.4 rev introduced.
(In reply to comment #3) > Ken, David Dubrow is the best person to review these changes. David's rev 1.4 > of EDCTrace reintroduced the problem I fixed in rev 1.3. Kirk's proposed > changes tries to fix that regression and additionally tweaks additional logic > David's 1.4 rev introduced. John's right. My fix to the curly-bracket/IllegalArgumentException bug did reintroduce the problem John fixed in 1.3. I must have been too myopic and probably didn't look at the content of the traces but just that it corrected the problem and allowed the debugger to work with tracing turned on. I'll look at the patch.
(In reply to comment #2) > Created attachment 191067 [details] > full patch to be applied to pick up all necessary FixArg/FixArgs additions In my original fix, I was hoping we wouldn't have to fix the argument in all calls to traceEntry() individually. But if that's the only way without messing up the content of the trace, then I'm glad you did that, Kirk. I didn't individually check each of the 10,000 calls you updated, but I'll take your word for it. ;)
(In reply to comment #5) > (In reply to comment #2) > > Created attachment 191067 [details] [details] > > full patch to be applied to pick up all necessary FixArg/FixArgs additions > > In my original fix, I was hoping we wouldn't have to fix the argument in all > calls to traceEntry() individually. But if that's the only way without messing > up the content of the trace, then I'm glad you did that, Kirk. I didn't > individually check each of the 10,000 calls you updated, but I'll take your > word for it. ;) David, did you introduce EDCTraceWrapper solely to avoid changing all those calls, or did it have some additional benefit? If the former, then we probably should remove that abstraction.
(In reply to comment #6) > David, did you introduce EDCTraceWrapper solely to avoid changing all those > calls, or did it have some additional benefit? If the former, then we probably > should remove that abstraction. It's been a while, but I'm fairly certain that was the only reason to introduce that wrapper class. Given that Kirk is modifying every call to traceEntry() individually now, I agree with you that EDCTrace can pretty much be reverted to what you had before and the fixArguments() functionality can be made into utility static calls. I was hoping that the problem could be addressed without each trace call having to have a special call to fix up the argument since this increases the likelihood of error in any future trace calls, but I guess now there will be ample examples showing it needs to be done. :P (too bad Java doesn't have macros)
(In reply to comment #7) > (In reply to comment #6) > > David, did you introduce EDCTraceWrapper solely to avoid changing all those > > calls, or did it have some additional benefit? If the former, then we probably > > should remove that abstraction. > > It's been a while, but I'm fairly certain that was the only reason to introduce > that wrapper class. > i left the EDCTraceWrapper & NullTraceWrapper in place because EDCDebugger.getDefault() can return null, which appears to represent an edge case where we don't want to be calling traceEntry()/traceExit()/trace() any longer. if you look closely, you'll see what i changed in the getTrace() function of EDCTraceWrapper is to return the original DebugTrace object, and not to call the saved object itself, so that each call-site would be making a direct call instead of an indirect call (and this is what thus requires the at-site calls of FixArg() / FixArgs() ). for NullTraceWrapper, the point appears to be to avoid the call altogether, since the plugin could be gone by the time the call is made. i hope that makes what's in the "EDCTrace.java only" patch above more clear, and why i think we don't want to completely eliminate EDCTraceWrapper.
We definitely still have a need for a NullTraceWrapper as you describe, but there seems to be no real need for EDCTraceWrapper. It seems John's code did everything that was needed except the fixing of the arguments' curly brackets and that can be done with a simple static call as you're already doing, right? (In reply to comment #8) > i left the EDCTraceWrapper & NullTraceWrapper in place because > EDCDebugger.getDefault() can return null, which appears to represent an edge > case where we don't want to be calling traceEntry()/traceExit()/trace() any > longer. > > if you look closely, you'll see what i changed in the getTrace() function of > EDCTraceWrapper is to return the original DebugTrace object, and not to call > the saved object itself, so that each call-site would be making a direct call > instead of an indirect call (and this is what thus requires the at-site calls > of FixArg() / FixArgs() ). > > for NullTraceWrapper, the point appears to be to avoid the call altogether, > since the plugin could be gone by the time the call is made. > > i hope that makes what's in the "EDCTrace.java only" patch above more clear, > and why i think we don't want to completely eliminate EDCTraceWrapper.
(In reply to comment #9) > We definitely still have a need for a NullTraceWrapper as you describe, but > there seems to be no real need for EDCTraceWrapper. It seems John's code did > everything that was needed except the fixing of the arguments' curly brackets > and that can be done with a simple static call as you're already doing, right? > ok, re-working the patches to eliminate EDCTraceWrapper …
Created attachment 191446 [details] replacement EDCTrace.java patch (eliminating EDCTraceWrapper) david/john, is this more what you had in mind in terms of eliminating the EDCTraceWrapper ?
Created attachment 191447 [details] full patch (eliminating trace-wrapper) with all fixArg()/fixArgs() call sites the full patch, to be applied to dev.eclipse.org cvs HEAD
(In reply to comment #11) > Created attachment 191446 [details] > replacement EDCTrace.java patch (eliminating EDCTraceWrapper) > > david/john, is this more what you had in mind in terms of eliminating the > EDCTraceWrapper ? Looks good to me. I'm assuming David will commit the patch after he reviews and accepts.
(In reply to comment #13) > (In reply to comment #11) > > Created attachment 191446 [details] [details] > > replacement EDCTrace.java patch (eliminating EDCTraceWrapper) > > > > david/john, is this more what you had in mind in terms of eliminating the > > EDCTraceWrapper ? > > Looks good to me. I'm assuming David will commit the patch after he reviews and > accepts. Yeah, me too. I'll commit it in the next few days.
Created attachment 191460 [details] EDCTrace.java patch only (eliminate EDCTraceWrapper, init sTrace = null) fix initialization of sTrace since used in static fixArg()/fixArgs()
Created attachment 191462 [details] full patch (elimniate EDCTraceWrapper, init sTrace = null) fix to init sTrace = null since it's used in static functions fixArg() / fixArgs()
(In reply to comment #16) > Created attachment 191462 [details] > full patch (elimniate EDCTraceWrapper, init sTrace = null) > > fix to init sTrace = null since it's used in static functions fixArg() / > fixArgs() FYI, all Java class fields (static and instance) are automatically initialized to their default "zero" value. Local variables, however, are not. As a general rule, explicit initializations of fields to zero should be avoided. Hopefully, that statement won't spark a religious debate. If you want to see one reason why, create a class with 10 int fields, and explicitly initialize them to zero. Then step into the instantiation (a 'new' statement) of that class in the debugger and you'll see one reason why these unnecessary initializations are a pain.
(In reply to comment #17) > FYI, all Java class fields (static and instance) are automatically initialized > to their default "zero" value. Local variables, however, are not. As a general > rule, explicit initializations of fields to zero should be avoided. Hopefully, > that statement won't spark a religious debate. If you want to see one reason > why, create a class with 10 int fields, and explicitly initialize them to zero. > Then step into the instantiation (a 'new' statement) of that class in the > debugger and you'll see one reason why these unnecessary initializations are a > pain. Agreed, even if only because it unnecessarily busies the source. :)
Created attachment 191472 [details] EDCTrace.java patch only (eliminate EDCTraceWrapper) no debate, just too much time working on c/c++ code recently. putting the original patch back, without the sTrace=null initialization.
Created attachment 191473 [details] full patch (eliminate trace wrapper) no debate, just too much time working on c/c++ code recently. putting the original patch back, without the sTrace=null initialization.
*** cdt cvs genie on behalf of ddubrow *** Bug 339809 - Kirk Beitz' fix to show actual call location in EDC traces + remove some unneeded @SuppressWarnings("unused") that were causing compile errors. [*] MemoryCache.java 1.16 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/services/dsf/MemoryCache.java?root=Tools_Project&r1=1.15&r2=1.16 [*] Breakpoints.java 1.28 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/services/dsf/Breakpoints.java?root=Tools_Project&r1=1.27&r2=1.28 [*] Expressions.java 1.29 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/services/dsf/Expressions.java?root=Tools_Project&r1=1.28&r2=1.29 [*] Memory.java 1.17 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/services/dsf/Memory.java?root=Tools_Project&r1=1.16&r2=1.17 [*] RunControl.java 1.52 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/services/dsf/RunControl.java?root=Tools_Project&r1=1.51&r2=1.52 [*] OperandValue.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/eval/ast/engine/instructions/OperandValue.java?root=Tools_Project&r1=1.7&r2=1.8 [*] DwarfInfoReader.java 1.31 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/symbols/dwarf/DwarfInfoReader.java?root=Tools_Project&r1=1.30&r2=1.31 [*] TypeEngine.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/symbols/TypeEngine.java?root=Tools_Project&r1=1.9&r2=1.10 [*] .api_filters 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/.settings/.api_filters?root=Tools_Project&r1=1.6&r2=1.7 [*] EDCTrace.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/EDCTrace.java?root=Tools_Project&r1=1.5&r2=1.6 [*] ASTInstructionCompiler.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/eval/ast/engine/ASTInstructionCompiler.java?root=Tools_Project&r1=1.11&r2=1.12 [*] Stack.java 1.53 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/services/Stack.java?root=Tools_Project&r1=1.52&r2=1.53
