Bug 360085

Comment 1 Martin Oberhuber 2011-10-06 08:16:34 EDT

A partial fix (for issue 2) is adding the following into ValgrindLaunchDelegate:

  public boolean finalLaunchCheck(ILaunchConfiguration configuration,
            String mode, IProgressMonitor monitor) throws CoreException {
	//Delete our own problem markers
	IWorkspaceRoot root = ResourcesPlugin.getWorkspace().getRoot();
	root.deleteMarkers(ValgrindLaunchPlugin.MARKER_TYPE, true,
                           IResource.DEPTH_INFINITE); //$NON-NLS-1$
	return super.finalLaunchCheck(configuration, mode, monitor);
  }


This at least avoids the situation where multiple identical problem markers stack up at the same location... it seems like a safe and logical fix to me that all valgrind related markers are cleared when new markers are about to be generated.

For issue 3 (workflow: valgrind, then debug), I'm most leaning towards an explicit "clear error markers" action in the valgrind view. That's safe since we likely can't always be smart enough to clear markers when we should. As an alternative, how do people feel about using "warning" severity for valgrind? 

Another problem that might be there (can't easily verify now) is this: Assume that valgrind reports errors in 5 source files of the same project. I'm fixing the issues in one source file and have autobuild turned on --> will the issues in the other 4 files be gone? I'd believe that I'd prefer keeping these...

Comment 2 Martin Oberhuber 2011-10-06 15:48:38 EDT

Created attachment 204715 [details]
patch v1

Attached patch (git format, against master) fixes all issues by simply decoupling the valgrind markers from the C_MODEL_PROBLEM_MARKER type.

This makes the valgrind tooling behave as I expect it to behave:

1. Marker severity is driven by valgrind severity of issue reported.
   If valgrind thinks it's an error it should be reported as error.

2. Rebuilding doesn't clear valgrind errors ... the only way clearing errors
   is running valgrind again. This allows me to work on errors one by one, and
   have the autobuilder enabled for multiple builds in-between.

3. Even when valgrind errors exist, the "errors exist are you sure you want
   to launch" dialog doesn't appear because that dialog is only tied to the
   CDT build markers (which it makes sense for).

What do others think ? Do users intuitively expect markers to go away by building? But IMO that's a slippery road (see the autobuild problem).

I think that the one outstanding issue / inconvenience with my patch is a workflow where valgrind reports some known positives and I don't want to be distracted / disturbed by the error markers that I have investigated. But that's IMO not a blocking issue (I can valgrind an empty / flawless program to clear the markers as a workaround), so it could probably be tracked by a separate request.

IMO a solutions to that additional problem include
- Explicit "clear error markers" action in the valgrind view as a simple
  but effective poor man's approach
- Preference to select valgrind marker severity (similar to API tooling)
- Ideal case: provide "problem filter support" similar to API Tooling thus
  allowing to mark known false positives and team-share the filters

Comment 3 Andrew Overholt 2011-10-07 09:32:24 EDT

Targeting 0.9.0.

Comment 4 Andrew Overholt 2011-10-07 09:33:19 EDT

*** Bug 347547 has been marked as a duplicate of this bug. ***

Comment 5 Jeff Johnston 2011-10-07 11:59:14 EDT

(In reply to comment #2)
> Created attachment 204715 [details]
> patch v1
> 
> Attached patch (git format, against master) fixes all issues by simply
> decoupling the valgrind markers from the C_MODEL_PROBLEM_MARKER type.
> 

Makes sense.

> This makes the valgrind tooling behave as I expect it to behave:
> 
> 1. Marker severity is driven by valgrind severity of issue reported.
>    If valgrind thinks it's an error it should be reported as error.
> 
> 2. Rebuilding doesn't clear valgrind errors ... the only way clearing errors
>    is running valgrind again. This allows me to work on errors one by one, and
>    have the autobuilder enabled for multiple builds in-between.
> 
> 3. Even when valgrind errors exist, the "errors exist are you sure you want
>    to launch" dialog doesn't appear because that dialog is only tied to the
>    CDT build markers (which it makes sense for).
> 
> What do others think ? Do users intuitively expect markers to go away by
> building? But IMO that's a slippery road (see the autobuild problem).
>

I think this should be a preference as some users will want this and others may not.  If I do a large number of changes to my file and then rebuild, the markers may be meaningless and carrying them around until I run Valgrind again or forcing me to manually clear them seems annoying.  I think it would be reasonable to make it the default since that matches current/old behaviour.

> I think that the one outstanding issue / inconvenience with my patch is a
> workflow where valgrind reports some known positives and I don't want to be
> distracted / disturbed by the error markers that I have investigated. But
> that's IMO not a blocking issue (I can valgrind an empty / flawless program to
> clear the markers as a workaround), so it could probably be tracked by a
> separate request.
> 
> IMO a solutions to that additional problem include
> - Explicit "clear error markers" action in the valgrind view as a simple
>   but effective poor man's approach

I like this idea.  Perhaps in addition, an option to delete an individual marker (right-click) from the view and even adding a quick fix for each error which would allow one to clear the marker from the editor (the quick fix simply being clear this error marker).  This would allow one to check off errors as they are investigated/handled.  Does that seem useful to you?

> - Preference to select valgrind marker severity (similar to API tooling)
> - Ideal case: provide "problem filter support" similar to API Tooling thus
>   allowing to mark known false positives and team-share the filters

Comment 6 Jeff Johnston 2011-10-11 18:15:30 EDT

Martin, just a heads-up.  If this is going to make the 0.9.0 Linux Tools release, any patches will have to be completed before this Friday so we can submit them in the iplog.  I had some comments in the previous reply.  I would see the default behaviour of clearing on build being an issue to address.  If you don't want to add a preference now, then at least leave the current behaviour to occur and add the ability to clear the markers via the Valgrind view so that a build is not required to clear them.  The quick fix idea and ability to clear individual errors does not need to addressed right now.

I'll also need a statement from you regarding the patches to say that you are 100% the author and that you have permission from your employers to submit the code.  If each patch is no longer than 250 lines, we can iplog+ them without a CQ.

If there isn't enough time, no worries.  It can always officially go into the next Linux Tools release and made accessible before then via our nightly builds.

Comment 7 Martin Oberhuber 2011-10-12 07:37:32 EDT

(In reply to comment #6)
I'm afraid I don't have time at the moment to continue working on this, but note that the current patch is only 20 lines or so (2.56KB) so IP review should not be an issue.

I'd also want to separate concerns. This is a hi-priority defect due to not clearing error markers in certain situations today. There's other aspects which could be tracked as separate enhancement requests, such as the "clean errors from valgrind view" or the quickfix idea.

I've thought again about this. It would be easy to contribute a ResourceChangedListener which listens to build events for clearing valgrind markers for the sake of backward compatibility. But conceptually this just seems wrong to me. Runtime errors are of a different nature than build errors, and have a different lifecycle... that's the whole reason this defect exists.

I'd rather recommend the following approach:
- Change the visual appearance of valgrind error markers:
  - Have them not appear in the problems view, but in the valgrind view and
    the editor only ... problems view is unnecessary duplication IMO, the
    valgrind view is the main view driving this and gives better context
  - Appearance different to build errors (eg similar to CDT codan markers,
    hinting more to a "warning" kind of issue ... the "error" nature is
    already visible from the Valgrind view anyways) 
  - This should make them much less intrusive so people won't expect them to 
    be cleared on build
- Optional: Contribute a "clear errors" action into the valgrind view.

> I'll also need a statement from you regarding the patches to say that you are
> 100% the author and that you have permission from your employers to submit the
> code.

Legal Message: I, Martin Oberhuber, declare that I developed attached code
from scratch, without referencing any 3rd party materials except material
licensed under the EPL. {I am authorized by my employer to make this
contribution under the EPL.}

> If there isn't enough time, no worries.  It can always officially go into the
> next Linux Tools release and made accessible before then via our nightly
> builds.

Well I really want this in 0.9. At the minimum you could integrate the "clean on Launch" part of the patch as per comment 1 with zero risk. Please also think again about your opinion regarding the "clean on build" listener - is that really a must-have for you ?

Comment 8 Jeff Johnston 2011-10-12 18:17:47 EDT

(In reply to comment #7)
> (In reply to comment #6)
> I'm afraid I don't have time at the moment to continue working on this, but
> note that the current patch is only 20 lines or so (2.56KB) so IP review should
> not be an issue.
> 
> I'd also want to separate concerns. This is a hi-priority defect due to not
> clearing error markers in certain situations today. There's other aspects which
> could be tracked as separate enhancement requests, such as the "clean errors
> from valgrind view" or the quickfix idea.
> 
> I've thought again about this. It would be easy to contribute a
> ResourceChangedListener which listens to build events for clearing valgrind
> markers for the sake of backward compatibility. But conceptually this just
> seems wrong to me. Runtime errors are of a different nature than build errors,
> and have a different lifecycle... that's the whole reason this defect exists.
> 
> I'd rather recommend the following approach:
> - Change the visual appearance of valgrind error markers:
>   - Have them not appear in the problems view, but in the valgrind view and
>     the editor only ... problems view is unnecessary duplication IMO, the
>     valgrind view is the main view driving this and gives better context
>   - Appearance different to build errors (eg similar to CDT codan markers,
>     hinting more to a "warning" kind of issue ... the "error" nature is
>     already visible from the Valgrind view anyways) 
>   - This should make them much less intrusive so people won't expect them to 
>     be cleared on build
> - Optional: Contribute a "clear errors" action into the valgrind view.
> 

I have checked in your changes to master, plus some additional modifications.

I have added new annotation markers for the Valgrind markers.  These icons are modified from the Valgrind view icon.  We can redo them after 0.9.0 if someone has a better design.  The new markers won't be misinterpreted as compiler errors in the editor so a user will have a better understanding about which errors go away on a build and which don't.  I can't change the icon for the Problems view.  For now, the errors will still show up in Problems view.

I have also added a "Remove all Valgrind markers" button in the Valgrind view.  This removes the markers (from editors and problems view) and also clears the contents of the view.

> > I'll also need a statement from you regarding the patches to say that you are
> > 100% the author and that you have permission from your employers to submit the
> > code.
> 
> Legal Message: I, Martin Oberhuber, declare that I developed attached code
> from scratch, without referencing any 3rd party materials except material
> licensed under the EPL. {I am authorized by my employer to make this
> contribution under the EPL.}
>

Thanks.  I'll iplog+ the patch so it can make 0.9.0.
 
> > If there isn't enough time, no worries.  It can always officially go into the
> > next Linux Tools release and made accessible before then via our nightly
> > builds.
> 
> Well I really want this in 0.9. At the minimum you could integrate the "clean
> on Launch" part of the patch as per comment 1 with zero risk. Please also think
> again about your opinion regarding the "clean on build" listener - is that
> really a must-have for you ?

It is much less important to me now with a different annotation marker in the editor.  I might still consider making it an optional preference, but I can certainly live with the new behaviour being default.

Comment 9 Jeff Johnston 2011-10-12 18:18:21 EDT

Comment on attachment 204715 [details]
patch v1

Less than 250 lines.

Comment 10 Martin Oberhuber 2011-10-17 07:58:22 EDT

(In reply to comment #8)
> I have checked in your changes to master, plus some additional modifications.

Jeff, I can't find your checkins in master, maybe you forgot to push?

http://git.eclipse.org/c/linuxtools/org.eclipse.linuxtools.git/log/

Reopen to clarify, since we can't claim this is fixed unless it's in master.

Comment 11 Jeff Johnston 2011-10-17 16:00:35 EDT

It appears you are correct that I hadn't pushed.  I pushed some fixes today and the Valgrind fix is part of that.

Comment 12 Martin Oberhuber 2011-10-20 20:08:47 EDT

This looks good in master as of today !

Thanks for taking the extra steps. I really like the new marker design, and the "clear all markers" button in the view.

At first, I was confused because the markers in the editor are different than the markers in the view. I adapted to this pretty quickly, but it might still be worth a thought whether the marker icons in the valgrind view shouldn't resemble the marker icons in the editor, to indicate that they are the same. 

I'm not sure whether the icon of markers in the problems view can be changed; and, in the project explorer there is also a standard error decorator and not a valgrind specific one. 

Again, I think this fix has been a pretty good step in the right direction.
Thanks!

Comment 13 Andrew Overholt 2011-10-26 14:02:03 EDT

Jeff, please file bugs to follow-up on Martin's comments.

Comment 14 Jeff Johnston 2011-10-26 15:00:05 EDT

(In reply to comment #12)
> This looks good in master as of today !
> 
> Thanks for taking the extra steps. I really like the new marker design, and the
> "clear all markers" button in the view.
> 
> At first, I was confused because the markers in the editor are different than
> the markers in the view. I adapted to this pretty quickly, but it might still
> be worth a thought whether the marker icons in the valgrind view shouldn't
> resemble the marker icons in the editor, to indicate that they are the same. 
> 
> I'm not sure whether the icon of markers in the problems view can be changed;
> and, in the project explorer there is also a standard error decorator and not a
> valgrind specific one. 
> 
> Again, I think this fix has been a pretty good step in the right direction.
> Thanks!

Thanks for the comments Martin.  The problems view cannot be changed.  There is a long-standing bug for being able to customize the icons for it.  I have opened https://bugs.eclipse.org/bugs/show_bug.cgi?id=362125 for making the Valgrind view and editor consistent.