Community
Participate
Working Groups
In looking at bug 341542, I found various other scenarios where the Executables view fails to update. I'm going to address all of these in bulk. I've attached a manual test suite with the scenarios I could think of (some work, some don't). The solution or any tweaks in the future to this logic should be validated with the suite. Also, as more scenarios are found, the test suite shold be extended. Unfortunately, there are no automated tests for the Executables view in the CDT repository (I think Nokia may have some non-contributed ones). Unfortunately, I don't have the time to code up such an automated suite using pure junit; if SWTBot was part of the standard environment, I would. The test suite points out what scenarios are currently failing
Created attachment 192722 [details] Fix should pass this suite 100%
Created attachment 192723 [details] Setup for test suite The test suite requires having the mingw toolchain and running a small script to compile some files into various programs. Instructions are in the test suite. The test suite and this setup is specific to a Windows environment, though the steps could easily be followed in other platforms with slight adjustments
Created attachment 193203 [details] Fix should pass this suite 100% Additional test cases
The attached fix makes extensive changes, as there are a lot of deficiencies and bugs in the current implementation. Below is a summary of those issues. I've attached a fairly extensive manual test. This fix passes 100% of the cases. Many of the cases fail with the current code. * There's caching done at various levels and those caches are not being invalidated properly. The fact that adding a file to a project and rebuilding it doesn't cause the Executables view's right pane to update is the most obvious failure. * ExecutablesManager should react to CDT model events, not platform resource events. The latter results in race condition failures, since the Executables view relies on getting state from the CDT model. Reacting to a platform resource change then asynchronously running code that queries the CDT model is problematic since the CDT model itself is reacting to platform resource changes. We need to let the CDT model update first and then react to its delta notifications. * Viewers should not themselves listen for model changes and force a refresh. They technically should know little to nothing about the model. A viewer’s content/label providers are responsible for keeping tabs on the model and asking the viewer to refresh as needed. * There's a lot of state manipulation on arbitrary threads, involving multiple locks. This not only requires careful crafting of the code to avoid deadlocks, but the resulting logic is prone to inconsistencies. These states can and should be updated on the UI thread, as the code involved is lightweight. Heavy operations (looking for executables, parsing them, mapping source files) should be done in asynchronous jobs, and this fix keeps it that way. By doing the lightweight state manipulation strictly on the UI thread, no locks are needed and the code can ensure all related states are updated in consistent and predictable ways. * Canceling an executable parse, though not yet supported by the dwarf reader, is not well supported by the viewer itself. The viewer should expect that the user may cancel the parse and the UI should react in an appropriate way. For starters, the canceling of a parse should leave the right pane with a cue that the parse was canceled and not show partial results. Secondly, the user should be able to restart the parse via the refresh button. The attached fix adds both of these behaviors. Other issues and enhancements: * use of launch config reference as key in CSourceFinder map is problematic and unnecessary. Use launch config name instead, as that is unique * added new interface to allow executable listener to determine specifically when an executable has been added vs removed. Currently, they can only tell that the list has changed. * WorkbenchJobs should be used instead of Display.asyncExec * Improved tracing.
Created attachment 193208 [details] Fix
*** Bug 341838 has been marked as a duplicate of this bug. ***
*** Bug 341542 has been marked as a duplicate of this bug. ***
Nice work and great conceptual overview and cleanup. I'm going to produce a build of Carbide with these changes and try it out, but I think this looks good to go.
Thanks for the review. Committed to HEAD.
*** cdt cvs genie on behalf of jcortell *** Bug 342141 - Executables view content goes stale in various scenarios [*] Messages.properties 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/Messages.properties?root=Tools_Project&r1=1.1&r2=1.2 [*] ExecutablesViewer.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/ExecutablesViewer.java?root=Tools_Project&r1=1.8&r2=1.9 [*] Messages.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/Messages.java?root=Tools_Project&r1=1.7&r2=1.8 [*] SourceFilesViewer.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/SourceFilesViewer.java?root=Tools_Project&r1=1.7&r2=1.8 [*] ExecutablesContentProvider.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/ExecutablesContentProvider.java?root=Tools_Project&r1=1.7&r2=1.8 [*] ExecutablesView.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/ExecutablesView.java?root=Tools_Project&r1=1.9&r2=1.10 [*] SourceFilesContentProvider.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/views/executables/SourceFilesContentProvider.java?root=Tools_Project&r1=1.7&r2=1.8 [*] CSourceFinder.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/internal/core/srcfinder/CSourceFinder.java?root=Tools_Project&r1=1.5&r2=1.6 [+] IExecutablesChangeListener2.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/executables/IExecutablesChangeListener2.java?root=Tools_Project&revision=1.1&view=markup [*] IExecutablesChangeListener.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/executables/IExecutablesChangeListener.java?root=Tools_Project&r1=1.3&r2=1.4 [*] Executable.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/executables/Executable.java?root=Tools_Project&r1=1.14&r2=1.15 [*] ExecutablesManager.java 1.17 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/executables/ExecutablesManager.java?root=Tools_Project&r1=1.16&r2=1.17 [*] StandardSourceFilesProvider.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/internal/core/executables/StandardSourceFilesProvider.java?root=Tools_Project&r1=1.4&r2=1.5 [*] StandardExecutableImporter.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/internal/core/executables/StandardExecutableImporter.java?root=Tools_Project&r1=1.4&r2=1.5
John, I have updated the source and have an API breakage that would force to upgrade "org.eclipse.cdt.debug.core" to version 8.0.0. This is due to a public considered class ExecutablesManager no longer implementing IResourceChangeListener.
Strange. The API Tooling did not flag that. I'll reintroduce the method/interface and just no-op it (I won't be reintroducing the calls to register and deregister the listener).
(In reply to comment #12) > Strange. The API Tooling did not flag that. I'll reintroduce the > method/interface and just no-op it (I won't be reintroducing the calls to > register and deregister the listener). Well, I double checked and the tooling is working in my workspace. If I remove the resourceChanged() method, I get: The major version should be incremented in version 7.1.0.qualifier, since API breakage occurred since version 7.0.0.201006141710 Yet, it does not complain about the loss of the interface. Clearly it should. Hmmm...
(In reply to comment #13) > Yet, it does not complain about the loss of the interface. Clearly it should. > Hmmm... You should check in workspace setting that "API Errors/Warnings -> API Compatibility -> Class -> The superinterface hierarchy has been reduce" is set to error.
*** cdt cvs genie on behalf of jcortell *** Bug 342141 - Executables view content goes stale in various scenarios [corrected inadvertent API breakage] [*] ExecutablesManager.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/executables/ExecutablesManager.java?root=Tools_Project&r1=1.17&r2=1.18
*** cdt cvs genie on behalf of jcortell *** Bug 342141 - Executables view content goes stale in various scenarios [Discovered inefficiency in new code. There's no need to research a project for executables when we've been able to determine that one or more specific Executable objects were removed. Just update the model and notify listeners.] [*] ExecutablesManager.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/executables/ExecutablesManager.java?root=Tools_Project&r1=1.18&r2=1.19
(In reply to comment #16) BTW, I exercised the relevant parts of the manual test suite (attached to this bugzilla) and had no failures.
The fix uses IResource.getModificationStamp(), and is dependent on the enhancement in 160728 (committed a while back)
