Re: [Mono-dev] Gendarme and Windows installer
Sebastien Pouliot
On Fri, 2008-03-14 at 14:36 +0000, Andy Hume wrote:
> I read that "For the first time Gendarme is available inside a Mono release." at http://anonsvn.mono-project.com/viewcvs/*checkout*/trunk/release/announce/mono-1.9.html?rev=98223&content-type=text%2Fplain
>
> I don't see it in the Windows preview releases I've installed. Is it meant to be there?
Gendarme is part of the 'mono-tools' package. I'm not sure if this is
packaged, or not, for Windows.
> It would be good if it were.
Yes - but I don't know what it really implies (for stuff other than
Gendarme inside mono-tools).
OTOH I've been thinking (and even worked a bit on it during hack week)
doing a specific installer for Windows (since the the next version of
Gendarme depends* on Mono.Cecil.Pdb.dll for reporting source files/lines
numbers associated with defects).
* well it's not a hard dependency but it's much more useful this way.
Let me know if you would be interested to test such a package.
Sebastien
> Andy
>
>
> _______________________________________________
> Mono-devel-list mailing list
> Mono-de...@lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
Sebastien Pouliot
installer ?
http://groups.google.com/group/gendarme/web/gendarme-0.1.0.0-win32-setup.msi
This installer includes a SVN-fresh version of Gendarme and a little
surprise :-)
Sebastien
Néstor Salceda
What a cool surprise !!
Well, my feedback is the following:
* The installation is quite simple. 2 clicks and go.
* Perhaps, in the installer we should put the Gendarme Logo.
* The installer checks the .net framework 3.5 dependency.
And for the surprise, I will put the feedback later because I don't want
reveal more details ;)
Great work Sebastien!
Néstor.
Sebastien Pouliot
On Tue, 2008-03-18 at 00:15 +0100, Néstor Salceda wrote:
> Hello,
>
> On dom, 2008-03-16 at 16:08 -0700, Sebastien Pouliot wrote:
> > So who want to beta-test and, hopefully, give feedback on a win32
> > installer ?
> >
> > http://groups.google.com/group/gendarme/web/gendarme-0.1.0.0-win32-setup.msi
First thing: the link is bad :-(
We're experiencing some strange problems with the Google Group. Not only
the file disappeared (and this is not the first time, I can still _see_,
but not access, stuff Dan added when I upload new stuff) but all changes
I did, yesterday, to the group documents are gone (but one of them is
still listed in my profile).
> >
> > This installer includes a SVN-fresh version of Gendarme and a little
> > surprise :-)
>
> What a cool surprise !!
>
> Well, my feedback is the following:
>
> * The installation is quite simple. 2 clicks and go.
> * Perhaps, in the installer we should put the Gendarme Logo.
There are option for "banners" but right now it's pretty basic (i.e.
almost "as-is" from the setup generated by VS.NET). Anyone feeling
artistic ? :-)
> * The installer checks the .net framework 3.5 dependency.
>
> And for the surprise, I will put the feedback later because I don't want
> reveal more details ;)
Bah, the code is in SVN right now. Sadly it does not work properly using
Mono right now, so it's still a win32-only "surprise" for the moment :-(
> Great work Sebastien!
Thanks :)
Keep the good news coming (you got a few in bank ;-)
Sebastien
> Néstor.
>
>
> >
Andy Hume
I haven't tried the installer yet but have run the WinForms runner! :-)
I'll try the installer when I get a moment.
I had checked-out mono-tool previously and built it using the VS
solutions (still needing to do the m4 stuff for the cygwin build), I had
used the console runner but it didn't work, now I've compiled and used
the WinForms runner and get good results. :-)
So various notes on the VS build etc. I'll put notes on the run in
another email.
I don't have VS2008 installed, but used MSBuild.exe from the
C:\WINDOWS\Microsoft.NET\Framework\v3.5\, which should be equivalent.
Most of the diffs that I needed to the various csproj files have been
fixed in the meantime. :-) I've still got a change to two places:
[[
Index: cecil/pdb/Mono.Cecil.Pdb/Mono.Cecil.Pdb.csproj
===================================================================
--- cecil/pdb/Mono.Cecil.Pdb/Mono.Cecil.Pdb.csproj (revision 98525)
+++ cecil/pdb/Mono.Cecil.Pdb/Mono.Cecil.Pdb.csproj (working copy)
@@ -12,7 +12,7 @@
<StartupObject>
</StartupObject>
<SignAssembly>true</SignAssembly>
-
<AssemblyOriginatorKeyFile>..\..\mcs\class\mono.snk</AssemblyOriginatorK
eyFile>
+
<AssemblyOriginatorKeyFile>..\..\..\mcs\class\mono.snk</AssemblyOriginat
orKeyFile>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' ==
'Debug|AnyCPU' ">
<DebugSymbols>true</DebugSymbols>
@@ -53,9 +53,9 @@
<Reference Include="System" />
</ItemGroup>
<ItemGroup>
- <ProjectReference
Include="..\..\mcs\class\Mono.Cecil\Mono.Cecil.csproj">
+ <ProjectReference Include="..\..\lib\Mono.Cecil.csproj">
<Project>{D8F63DFF-5230-43E4-9AB2-DA6E721A1FAE}</Project>
<Name>Mono.Cecil</Name>
</ProjectReference>
</ItemGroup>
Index:
mono-tools/gendarme/rules/Gendarme.Rules.Portability/Gendarme.Rules.Port
ability.csproj
===================================================================
---
mono-tools/gendarme/rules/Gendarme.Rules.Portability/Gendarme.Rules.Port
ability.csproj (revision 98525)
+++
mono-tools/gendarme/rules/Gendarme.Rules.Portability/Gendarme.Rules.Port
ability.csproj (working copy)
@@ -49,7 +49,7 @@
</PropertyGroup>
<ItemGroup>
<Reference Include="ICSharpCode.SharpZipLib, Version=0.85.4.369,
Culture=neutral, PublicKeyToken=1b03e6acf1164f73,
processorArchitecture=MSIL">
-
<HintPath>..\..\..\..\cecil\gendarme\rules\Gendarme.Rules.Portability\bi
n\Debug\ICSharpCode.SharpZipLib.dll</HintPath>
+
<HintPath>..\..\..\..\mcs\class\lib\net_2_0\ICSharpCode.SharpZipLib.dll<
/HintPath>
<SpecificVersion>False</SpecificVersion>
<Private>True</Private>
</Reference>
]]
For the second, there is no cecil\gendarme directory anywhere as I can
see. For the first, I have mono, mcs, cecil, and mono-tools all
checked-out in the same directory, is that correct?
I don't get the console app to run successfully -- assuming that's the
one to call. It just outputs "Gendarme - Development Snapshot" and
exits. However as noted above I did get the SWF runner to work. I'll
put some notes on that in another email.
Andy
Sebastien Pouliot
to /mcs/class/Mono.Cecil
This means that I (or anyone) cannot commit stuff inside /cecil/lib
but only under /mcs/class/Mono.Cecil (where the patch would be wrong)
but, OTOH, /cecil/pdb/* is not part of /mcs/class
I did this so that people could avoid a checkout of (the rather large)
mcs tree.
Now this is still required to get the mono.snk strongname key so maybe
I should make having /mcs mandatory ?!?
which, unlike /cecil/, is a released module).
I'll fix this tonight, thanks for spotting it
> For the first, I have mono, mcs, cecil, and mono-tools all
> checked-out in the same directory, is that correct?
(for gendarme itself) are required.
i.e. you should be able to build gendarme without /mono
> I don't get the console app to run successfully -- assuming that's the
> one to call. It just outputs "Gendarme - Development Snapshot" and
> exits.
without any command-line options.
In that case the header will be shown and the process will end.
> However as noted above I did get the SWF runner to work.
This proves that everything compiled correctly.
> I'll put some notes on that in another email.
Sebastien
Sebastien Pouliot
> > > So who want to beta-test and, hopefully, give feedback on a win32
> > > installer ?
>
> First thing: the link is bad :-(
>
> We're experiencing some strange problems with the Google Group. Not only
> the file disappeared (and this is not the first time, I can still _see_,
> but not access, stuff Dan added when I upload new stuff) but all changes
> I did, yesterday, to the group documents are gone (but one of them is
> still listed in my profile).
will stay) available from here:
http://gendarme.googlegroups.com/web/gendarme-0.1.0.0-win32-setup.zip
Have fun!
Sebastien
Sebastien Pouliot
I committed some change to make it easier to build from VS.NET 2008
(and hopefully MS build too). However due to the fact that
Mono.Cecil.Pdb is outside mono-tools you'll still need to keep your
local changes to make it compile(*). The good news is that this is not
something that change very often (which is why I had forgotten about
it ;-).
Let me know if you find some other things that could make it easier/
better (this is not my main environment).
Thanks
Sebastien
(*) I got some ideas around this (one short term, using Makefile, and
one long-term - but that requires a bit more stuff elsewhere first)
On Mar 19, 12:07 pm, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
RichB
wrote:
> > > > So who want to beta-test and, hopefully, give feedback on a win32
> > > > installer ?
>
> will stay) available from here:http://gendarme.googlegroups.com/web/gendarme-0.1.0.0-win32-setup.zip
>
when I suddenly came across this post - at last I don't have to build
from source every time I visit a new company! Here's a few comments in
no particular order:
1) It's a shame it's .Net 3.5 only - I suppose we all have to move on.
2) My report said: "Gendarme found 720 defects using 104 rules.". The
length of the report makes it really trick to read - and this is after
I've spent 3 days refactoring the system - the report would have been
several times longer then. I'd love to be able to collapse sections in
the HTML (and a Collapse All button). Plus, it would be nice if the
messages were color coded - it's tricky to visually see which method a
warning is referring to.
3) I got a false positive on AvoidUncalledPrivateCodeRule. It failed
on "private static TResult RepeatableTry<TResult>(Func<TResult>
function) where TResult : class" - which is definately called by 20
other methods. You can sortof guess what the method does.
4) I absolutely love the AvoidCodeDuplicatedInSameClassRule. It's
something I've wanted in a static analyzer for many many years. Next
step - automated assembly compression based on pattern matching of
IL...
5) DoubleCheckLockingRule - I thought that
"DoubleCheckedLockingIsBroken" only applied to Java and that DCL
actually worked OK in .Net due to the difference in memory barriers.
Yes - this article says it works correctly in .Net:
http://msdn2.microsoft.com/en-us/library/ms998558.aspx
6) FloatComparisonRule - Brilliant. No other static analyzer I've run
has spotted this.
7) NonConstantStaticFieldsShouldNotBeVisibleRule spells "synchronized"
incorrectly.
Overall, I love it.
Richard
Néstor Salceda
> On Mar 20, 1:27 am, Sebastien Pouliot <sebastien.poul...@gmail.com>
> wrote:
Hey Richard, thanks for the report.
> I almost posted a message on here bemoaning the lack of any binaries -
> when I suddenly came across this post - at last I don't have to build
> from source every time I visit a new company! Here's a few comments in
> no particular order:
>
> 1) It's a shame it's .Net 3.5 only - I suppose we all have to move on.
Yes, this is because we are using Linq and extension methods.
> 2) My report said: "Gendarme found 720 defects using 104 rules.". The
> length of the report makes it really trick to read - and this is after
> I've spent 3 days refactoring the system - the report would have been
> several times longer then. I'd love to be able to collapse sections in
> the HTML (and a Collapse All button). Plus, it would be nice if the
> messages were color coded - it's tricky to visually see which method a
> warning is referring to.
The collapse section is a nice feature and the color messages too. I
will take note and write them in the wiki :)
> 3) I got a false positive on AvoidUncalledPrivateCodeRule. It failed
> on "private static TResult RepeatableTry<TResult>(Func<TResult>
> function) where TResult : class" - which is definately called by 20
> other methods. You can sortof guess what the method does.
Oops, we will fix it as fast as we can.
> 4) I absolutely love the AvoidCodeDuplicatedInSameClassRule. It's
> something I've wanted in a static analyzer for many many years. Next
> step - automated assembly compression based on pattern matching of
> IL...
Hehe, nice. I hope improve this rule soon, because there is some code
which the rule can't detect.
> 5) DoubleCheckLockingRule - I thought that
> "DoubleCheckedLockingIsBroken" only applied to Java and that DCL
> actually worked OK in .Net due to the difference in memory barriers.
> Yes - this article says it works correctly in .Net:
> http://msdn2.microsoft.com/en-us/library/ms998558.aspx
> 6) FloatComparisonRule - Brilliant. No other static analyzer I've run
> has spotted this.
Yes, this is an error that people shouldn't take care when they are
writting code.
> 7) NonConstantStaticFieldsShouldNotBeVisibleRule spells "synchronized"
> incorrectly.
This is fixed now in the SVN.
> Overall, I love it.
Thanks for the good comments, and also thanks for your report.
Néstor.
Sebastien Pouliot
On Fri, 2008-04-11 at 09:06 -0700, RichB wrote:
> On Mar 20, 1:27 am, Sebastien Pouliot <sebastien.poul...@gmail.com>
> wrote:
> > > > On dom, 2008-03-16 at 16:08 -0700, Sebastien Pouliot wrote:
> > > > > So who want to beta-test and, hopefully, give feedback on a win32
> > > > > installer ?
> >
> > The issue seems to be fixed and the installer is now (and hopefully
> > will stay) available from here:http://gendarme.googlegroups.com/web/gendarme-0.1.0.0-win32-setup.zip
> >
>
> I almost posted a message on here bemoaning the lack of any binaries -
> when I suddenly came across this post - at last I don't have to build
> from source every time I visit a new company!
oops, this reminds me that I need to update the wiki about this (and a
few other things too).
> Here's a few comments in
> no particular order:
>
> 1) It's a shame it's .Net 3.5 only - I suppose we all have to move on.
It was not an easy decision. I took a long time before accepting 2.0
stuff inside Gendarme for two reasons:
1- end users runtime requirements;
2- it was not officially supported by Mono
However it was hurting Gendarme itself, slowing it's progress and making
it (a bit) less interesting for developers. In the end it seemed, to me,
that end users[1] would be better served with a more complete Gendarme
(than an potential runtime upgrade). Also since we now sync our Gendarme
release(*) with Mono it became possible (for Mono's users) to ensure
compatibility[2].
(*) the windows installer is an exception to this rule since the
mono-tools package is not (yet) build for Windows.
> 2) My report said: "Gendarme found 720 defects using 104 rules.". The
> length of the report makes it really trick to read - and this is after
> I've spent 3 days refactoring the system - the report would have been
> several times longer then. I'd love to be able to collapse sections in
> the HTML (and a Collapse All button).
You're the second person to request it this week. I guess I need to
spend a bit of them fighting HTML/CSS/XSLT soon...
> Plus, it would be nice if the
> messages were color coded - it's tricky to visually see which method a
> warning is referring to.
Interesting. Care to elaborate on that ?
> 3) I got a false positive on AvoidUncalledPrivateCodeRule. It failed
> on "private static TResult RepeatableTry<TResult>(Func<TResult>
> function) where TResult : class" - which is definately called by 20
> other methods. You can sortof guess what the method does.
Last week I fixed a problem wrt to generics for
AvoidUninstantiatedInternalClassesRule, which is a very similar rule
(note: the fix is only in SVN and not available in the current
installer). It's possible that the same (missing) logic is needed
there...
> 4) I absolutely love the AvoidCodeDuplicatedInSameClassRule. It's
> something I've wanted in a static analyzer for many many years. Next
> step - automated assembly compression based on pattern matching of
> IL...
Thank Nestor and, indirectly, Google (Summer of Code).
> 5) DoubleCheckLockingRule - I thought that
> "DoubleCheckedLockingIsBroken" only applied to Java and that DCL
> actually worked OK in .Net due to the difference in memory barriers.
> Yes - this article says it works correctly in .Net:
> http://msdn2.microsoft.com/en-us/library/ms998558.aspx
I need to read it (fully) and talk with the JIT guys (to be sure this is
also true for Mono). A quick look shows that the double-check use a
volatile variable - which is not something checked by the rule.
> 6) FloatComparisonRule - Brilliant. No other static analyzer I've run
> has spotted this.
Great. This rule was also done in last year Google Summer of Code (by
Lukasz Knop). As you see Google programs (GSoC and GHOP) have been very
helpful to Gendarme.
> 7) NonConstantStaticFieldsShouldNotBeVisibleRule spells "synchronized"
> incorrectly.
>
>
> Overall, I love it.
Thanks :) your comments are very useful at this stage (keep them coming
good or bad!).
Sebastien
>
> Richard
>
>
>
> >
RichB
wrote:
> On Fri, 2008-04-11 at 09:06 -0700, RichB wrote:
> > Plus, it would be nice if the
> > messages were color coded - it's tricky to visually see which method a
> > warning is referring to.
>
> Interesting. Care to elaborate on that ?
failed. While scrolling through a list of failures its difficult to
see the method name. Either it should be positioned at the same
horizontal location for every report, or it should be highlighted in
some way (bold, color etc). Does anyone really need the fully
qualified assembly name (ie with PublicKeyToken) for every single
report?
I've also just spotted another strange message in the report:
Source: C:\xxx\yyy\zzz\Conversion.cs(16707566,0)
Details: Instance of an empty string has been found
Now there definately isn't that many lines in Conversion.cs. The code
which reported this is:
public static Operand OperandFromString(string value)
{
switch (value)
{
case "":
return (Operand.Blank);
The real line number is 175. So I guess it's something to do with the
empty string being the first in the case statement. However from what
I remember of IL, the IL switch instruction is only used for integer
lookups - not for strings. So I'm not sure why Gendarme/Cecil would
get this wrong.
Thanks,
Richard
Exception
constant ("") is used instead of preferred String.Empty value. This
has nothing to do with the switch statement.
>
> Thanks,
> Richard
Sebastien Pouliot
On Sat, 2008-04-12 at 14:20 -0700, RichB wrote:
> On Apr 11, 6:36 pm, Sebastien Pouliot <sebastien.poul...@gmail.com>
> wrote:
> >
> > On Fri, 2008-04-11 at 09:06 -0700, RichB wrote:
> > > Plus, it would be nice if the
> > > messages were color coded - it's tricky to visually see which method a
> > > warning is referring to.
> >
> > Interesting. Care to elaborate on that ?
>
> One of the most important things in the report is the method that
> failed. While scrolling through a list of failures its difficult to
> see the method name. Either it should be positioned at the same
> horizontal location for every report, or it should be highlighted in
> some way (bold, color etc).
That make sense. I'll look into it. If you (or anyone else) has a
proposal I'm interested in hearing it :)
> Does anyone really need the fully
> qualified assembly name (ie with PublicKeyToken) for every single
> report?
Probably not unless
(a) there are multiple assemblies; and
(b) no debugging symbol files are available
Since reports tends to be long (or very long), I think it would be a
good idea to omit things that are not strictly required.
> I've also just spotted another strange message in the report:
>
> Source: C:\xxx\yyy\zzz\Conversion.cs(16707566,0)
> Details: Instance of an empty string has been found
I can't recall where exactly (source wise*) but I've seen this before.
It happens only with CSC/PDB (at least I never got that with
[g]mcs/mdb).
Maybe JB recalls this ??? I think I discussed this with him on IRC when
it happened...
>
> Now there definately isn't that many lines in Conversion.cs. The code
> which reported this is:
> public static Operand OperandFromString(string value)
> {
> switch (value)
> {
> case "":
> return (Operand.Blank);
>
> The real line number is 175. So I guess it's something to do with the
> empty string being the first in the case statement. However from what
> I remember of IL, the IL switch instruction is only used for integer
> lookups - not for strings. So I'm not sure why Gendarme/Cecil would
> get this wrong.
My previous debugging shows that the MS library, used for PDB support,
is returning this value :(
Now maybe we're doing something wrong (prior to getting the value), as
this is not one of the best API documentation I have seen. Or maybe the
value 0xFEEFEE has a special (and undocumented) meaning. I'll try to
replicate this using code similar to your switch/case/""
Otherwise we could "omit" the line number (or show something else, like
"unknown") for those rare cases.
Thanks!
Sebastien
Exception
with the line number.
Salut!
On 13 апр, 02:26, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
RichB
wrote:
> On Sat, 2008-04-12 at 14:20 -0700, RichB wrote:
>
> > Source: C:\xxx\yyy\zzz\Conversion.cs(16707566,0)
> > Details: Instance of an empty string has been found
> >
> > which reported this is:
> > public static Operand OperandFromString(string value)
> > {
> > switch (value)
> > {
> > case "":
> > return (Operand.Blank);
>
> is returning this value :(
>
compiler dissallows the use of String.Empty in a switch statement:
Conversion.cs(175,22): error CS0150: A constant value is expected
I haven't tested other C# compilers.
Richard
Sebastien Pouliot
On Sun, 2008-04-13 at 06:51 -0700, RichB wrote:
> On Apr 12, 11:26 pm, Sebastien Pouliot <sebastien.poul...@gmail.com>
> wrote:
> >
> > On Sat, 2008-04-12 at 14:20 -0700, RichB wrote:
> >
> > > Source: C:\xxx\yyy\zzz\Conversion.cs(16707566,0)
btw googling 0xFEEFEE and PDB returned interesting stuff :)
http://blogs.msdn.com/jmstall/archive/2005/06/19/FeeFee_SequencePoints.aspx
is this code inside (or near) some #line hidden ?
> > > Details: Instance of an empty string has been found
> > >
> > > Now there definately isn't that many lines in Conversion.cs. The code
> > > which reported this is:
> > > public static Operand OperandFromString(string value)
> > > {
> > > switch (value)
> > > {
> > > case "":
> > > return (Operand.Blank);
> >
> > My previous debugging shows that the MS library, used for PDB support,
> > is returning this value :(
> >
>
> Even if you fix this bug, the report is incorrect - the MS C# v2
> compiler dissallows the use of String.Empty in a switch statement:
>
> Conversion.cs(175,22): error CS0150: A constant value is expected
Oh, I missed that when I saw the line number.
It's not easy to fix since the switch code, for a small number of cases,
is optimized into an 'if' (at least by csc) so the rule can't be sure if
the original source code was using an "if" (which should be reported) or
a switch (which should not be reported).
For larger switch the generated code varies by compiler, e.g. CSC will
using a generic Dictionary (older 7.x versions did not for certain).
In the mean time I think we should add this to the rule documentation
(anyway it will still need to be there for the short-switch case).
> I haven't tested other C# compilers.
I'll check [g]mcs and older MS compilers.
Thanks
Sebastien
>
> Richard
> >
RichB
wrote:
>
> btw googling 0xFEEFEE and PDB returned interesting stuff :)http://blogs.msdn.com/jmstall/archive/2005/06/19/FeeFee_SequencePoint...
> is this code inside (or near) some #line hidden ?
create a minimal testcase:
----
using System;
class X {
static void Main() {
Console.WriteLine(OperandFromString(""));
}
internal enum Operand
{
Blank = 10
public static Operand OperandFromString(string value)
{
switch (value)
{
case "":
default:
return Operand.Blank;
}
}
}
----
Compiled with csc /debug test.cs
This produces the following Gendarme report:
Target: X/Operand X::OperandFromString(System.String)
Assembly: test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
Severity: Medium Confidence: High
Source: c:\Program Files\Microsoft Visual Studio 8\VC
\test.cs(16707566,0)
thread.
Richard
Sebastien Pouliot
> using System;
>
> class X {
> static void Main() {
> Console.WriteLine(OperandFromString(""));
> }
>
> internal enum Operand
> {
> Blank = 10
> }
>
> public static Operand OperandFromString(string value)
> {
> switch (value)
> {
> case "":
> return Operand.Blank;
> default:
> return Operand.Blank;
> }
> }
>
> }
(or I would have asked first ;-)
> Compiled with csc /debug test.cs
> This produces the following Gendarme report:
>
> Target: X/Operand X::OperandFromString(System.String)
> Assembly: test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null
>
> Severity: Medium Confidence: High
> Source: c:\Program Files\Microsoft Visual Studio 8\VC
> \test.cs(16707566,0)
> Details: Instance of an empty string has been found
After seeing this I guessed, half-wrongly, that this was caused by the
optimization from a switch/case into if/else. However it is not
exactly the case (pun intended ;-) as I added enough case to make it
generate a real (IL) switch.
So what happens, from Gendarme (well the symbols) point of view, is
that no sequence point exist for the "case"s - which is also why it's
impossible to set a breakpoint (vs) on them.
The current (and simple) logic is that when an instruction is found
without a sequence point (quite common), it looks back to previous
instruction. After a few loops we get normally get a valid sequence
point. In the case of with your code sample we end up to a 0xFEEFEE -
I'm not entirely sure why but it looks like the compiler insert this
symbol to help the debugger (i.e. to ignore some code related to how
the switch is built). At least this is what I would do, now that I
know the trick, to hide "complex" code generation by the compiler.
There are many options to "fix" this but none of them very complete.
(a) my original idea was to replace 0xFEEFEE with "(hidden)". That
allows Gendarme to report errors even when #line hidden is used, which
is a good thing IMO;
(b) we could ignore 0xFEEFEE *but* that "works" only when debugging
symbols are available (it would still be reported if symbols are
missing). Same would apply to "legal" hidden stuff (not good IMO);
(c) rework the logic to find the line numbers. That sounded the best
until I looked at the IL - a switch statement is *ugly* in IL. It
seems pretty complex to do correctly, without messing with other
"legal" 0xFEEFEE cases - and it does not fix the fact that the "right"
line number is not available (so we would still end with an
approximation).
So right now I'm thinking about handling 0xFEEFEE with a variation of
(a) and show something like: "(unavailable, IL offset 0xFFFF)". It's
worth noting that [g]mcs, or the MDB format does not use this trick,
so the "switch" line (the previous line with a sequence point) would
be reported.
But there are good news :-)
(1) very few rules could apply to a "case" statement, so it's not
likely to bite us elsewhere;
(2) this whole digging around the problem has given me (and maybe
others ;-) a idea on how to potentially avoid the report... (at least
when debugging symbols are present) and no report means no
0xFEEFEE :-)
Gonna try this tonight... well after cleaning up a Cecil patch... and
maybe a few other things too.
Thanks again for the sample!
Sebastien
RichB
On Apr 15, 12:11 am, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
>
set of beautiful IL switch statements - which are simply an integer
lookup table. I can understand that the resulting IL switch is nothing
like the original C# switch, but the resulting IL is elegant :-)
> (1) very few rules could apply to a "case" statement, so it's not
> likely to bite us elsewhere;
the case statement and another on a DoubleCheckLockingRule. The method
where the DoubleCheckLockingRule reports looks like:
If() {
try {
lock() {
if() {
} else {
}
}
}
}
ie it doesn't look like a switch at all.
Any idea what 0xFEEFED is (16707565)? I get this line number reported
18 times in the following rules:
AvoidCodeDuplicatedInSameClassRule : 9
AvoidLongMethodsRule: 7
DetectNonAlphanumericInTypeNamesRule: 2
Richard
Sebastien Pouliot
On Tue, 2008-04-15 at 02:14 -0700, RichB wrote:
>
> On Apr 15, 12:11 am, Sebastien Pouliot <sebastien.poul...@gmail.com>
> wrote:
> >
> > until I looked at the IL - a switch statement is *ugly* in IL. It
bad choice of word, I blame my limited English ;-)
> I have to disagree(!). A C# switch on integers gets compiled down to a
> set of beautiful IL switch statements - which are simply an integer
> lookup table. I can understand that the resulting IL switch is nothing
> like the original C# switch, but the resulting IL is elegant :-)
Seriously my point is that a lot of (different) code can be generated
for the same "switch" (if/else optimization, different compilers,
different runtime versions) which makes it a bit harder from an analysis
point of view (at least if you need to treat it specially).
> > (1) very few rules could apply to a "case" statement, so it's not
> > likely to bite us elsewhere;
>
> I get 0xFEEFEE (16707566) reported twice. Once on the String.Empty in
> the case statement and another on a DoubleCheckLockingRule. The method
> where the DoubleCheckLockingRule reports looks like:
>
> If() {
> try {
> lock() {
> if() {
> } else {
> }
> }
> }
> }
>
> ie it doesn't look like a switch at all.
I will try this tonight (and it may change my previous ideas on the
subject). A wild guess (this morning) is that you can't set a breakpoint
on a "try"(*) so the instruction has 0xFEEFEE.
(*) but you can set one on the following '{' so it gets unnoticed (even
more if you put the bracket on the same line).
> Any idea what 0xFEEFED is (16707565)? I get this line number reported
> 18 times in the following rules:
>
> AvoidCodeDuplicatedInSameClassRule : 9
> AvoidLongMethodsRule: 7
> DetectNonAlphanumericInTypeNamesRule: 2
It's a 0xFEEFEE like the other ones. When a rule report a defect without
specifying an instruction, e.g. on a method, then the current logic
(soon to be replaced ;-) tries to provide an "approximate" line number
for the method. It does so, as you can guess, by looking at the first IL
instruction and subtracting 1.
There's two way to fix this:
(a) never subtract 1, showing the same text as the previous case;
(b) subtract 1 only when it's not 0xFEEFEE, showing the same text as the
previous case;
(c) looking for other instructions (later in the code) and reporting
that line (c1) or "around X" (c2)
In all case this won't always be the "right" line for the method
definition. Hopefully we'll have someday a runners integrated with IDE
which will be able to adjust their "goto source" function to hit the
right line.
Thanks again,
Sebastien
p.s. a second wild guess... all those methods reported with 0xFEEFED
starts with a try. right ?
RichB
On Apr 15, 1:15 pm, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
>
> > 18 times in the following rules:
>
> > AvoidCodeDuplicatedInSameClassRule : 9
> > AvoidLongMethodsRule: 7
> > DetectNonAlphanumericInTypeNamesRule: 2
>
> starts with a try. right ?
contain a try.
Most of them are in the same 2,600-line file/class. And several of
them start with a List<T> variable declaration. Other than that, I
can't see any patterns.
... a quick ildasm later...
OK - I've got it. They all contain an anonymous method doing something
like List<T>.FindAll(delegate() {....})
Richard
Sebastien Pouliot
Well it makes sense[1]. I've been thinking about something similar
(LINQ[2]) when I was looking at the switch IL. This is another case
where the compiler does generate quite a bit of code (in this case after
the call) and where the debugger "must" ignore them (not sure why they
don't simply omit emitting sequence points).
The biggest difference is our usage of the symbols. The debugger wants
the source to map to IL while Gendarme (or code analysis in general)
wants the IL to map to source.
Now the big question, what would be the most useful to you ? an
approximate line number, possibly inaccurate when possible, or some text
to show the information is not available (e.g. with in IL offset) ???
[1] this reminds me that we're lacking tests for many new C#3 features
(well even generics are not tested for every rules). Even there 98% of
the testing is done on Mono/Linux (which won't exhibit this).
[2] since methods using LINQ are very likely to be reported by
AvoidLongMethodRule, not because the source is large but because the
generated IL is.
Thanks a lot for the analysis!
Sebastien
RichB
On Apr 16, 1:49 am, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
>
> approximate line number, possibly inaccurate when possible, or some text
> to show the information is not available (e.g. with in IL offset) ???
tend to write quite small methods. However, when I'm refactoring
someone else's code (like I am now), it's extremely useful to find
where in a 700-line method a warning is reported. So therefore, I'd
prefer an approximate location to no location at all.
Clearly, it would be helpful to know the line number was a guess.
Richard
Sebastien Pouliot
> 3) I got a false positive on AvoidUncalledPrivateCodeRule. It failed
> on "private static TResult RepeatableTry<TResult>(Func<TResult>
> function) where TResult : class" - which is definately called by 20
> other methods. You can sortof guess what the method does.
generic cases), but only when the assemblies are compiled with CSC
(right now a mono bug makes the check fail every time). Are you able
to test this from SVN ? I'm planning a windows installer "refresh" but
that will be after our "rule day".
Sebastien
RichB
On Apr 20, 8:24 pm, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
> to test this from SVN ? I'm planning a windows installer "refresh" but
> that will be after our "rule day".
>
1) I don't have VS 2008. So I downloaded 3.5 SDK.
2) Gendarme tarball doesn't come with Cecil. So I downloaded Cecil
0.6.
3) Getting Cecil 0.6 doesn't compile with Gendarme. So I downloaded
Cecil trunk tarball.
4) Cecil on Trunk doesn't look anything like the directory structure
of Cecil 0.6. In fact, it doesn't look like Cecil at all. It's got
ildasm and the verifier in there.
Sorry, I gave it a go.
Richard
Sebastien Pouliot
Wow, I didn't anticipate so much problems/work when I asked this :-(
I was planning to do a win32 installer refresh *after* rule day but now
I think I'll make one *before* so anyone on Win32 will be able to get
the latest stuff (like Cecil) from the package. I'll email the group
once this is available (there are a few things I want to complete/commit
before then).
Note: the problems with Cecil are mostly because /cecil/lib on SVN only
links to /mcs/class/Mono.Cecil and (it would seems) does not include
them automagically in the tarballs :-|
> Sorry, I gave it a go.
Don't be. I'm the one sorry I asked (and wasted your time). Hopefully
the (soon to be updated) win32 installer will redeem me a bit.
Thanks
Sebastien
RichB
>
> > 3) I got a false positive on AvoidUncalledPrivateCodeRule. It failed
> > on "private static TResult RepeatableTry<TResult>(Func<TResult>
> > function) where TResult : class" - which is definately called by 20
> > other methods. You can sortof guess what the method does.
>
> I just made a commit that should fix the issue
AvoidUncalledPrivateCodeRule. I'll start a new thread as this one is
getting a bit tired.
Richard