[Mono-devel-list] [Patch] RReader not to rely on bounds checking

[Lluis Sanchez]

On dl, 2005-01-31 at 16:14 +0100, Willibald Krenn wrote:
> Lluis Sanchez schrieb:
> > Bounds checking will always work. It is a requirement of the CLR.
> 
> Well, the reason I 'found' this code was that bc did not work. You know 
> what error I got?
> "System.Runtime.Remoting.RemotingException: Configuration file 
> '/usr/etc/mono/1.0/machine.config' could not be loaded: Expected element"
> 
> Despite the fact that the parser did not say what element was expected 
> on what line and what elements were on parser stack that time, it took 
> quite a while of _not_ intended debugging to finally see RReader was the 
> culprit (and not the config file or parser). It took some hours more to 
> finally realize that indeed bc wasn't working correctly...

Well, with your patch you would have never realized that bc wasn't
working. Then somebody else would have spent that time debugging
whatever error this bug could induce.

> 
> > I don't think abcremoval can optimize this case.
> 
> I'm just checking. Unfortunately I found yet aother bug 
> (MonoMethod->signature == null where it's not expected to be null) that 
> prevents me from using monodis...
> 
> >> So we'd still have one 'cmp' 
> >>but no exception would be raised in case pos >= len.
> > 
> > 
> > this should never happen anyway if the configuration file is valid.
> 
> The exception will always be thrown, because in my rev. of 'mcs' code, 
> the Parser reads in until RReader.Read returns -1..

I don't think that one exception out of 16.000 calls is a problem.

> 
> > I just don't want to lose time reviewing patches that don't fix real and
> > reproducible bugs. But since now I've already lost that time (and even
> > more time writing this mail), you can commit the patch if you want.
> 
> You're talking about losing time? How much time did I lose just because 
> my test program (I need for my cont. optimization work) did not run and 
> mono did not return any meaningful error message? If the catch would not 
> have swallowed a SigSegv, it would have been a piece of cake, but so..

I'm sorry you lost that time, but this doesn't mean that the original
code was incorrect. What if the ++ operator stops working? do we need to
add a check for this as well in the Read() code? We don't need to. We
can write code assuming that ++ will work, as well as we can assume that
array bounds check will work. If any of those stop working somebody will
have a hard time debugging it, but it doesn't mean that code written
with those assumptions is wrong.

> 
> Lluis, I have no problem if you clearly say 'no, this won't go in, 
> because I don't like it'.

I didn't say I don't like the patch. I said that you can commit it. I
agree in that it's more elegant. 

> 
> I'll commit it, if it turns out that abcrem handles this case
> Willi
> 
> P.S.: You really don't want to know how much time I need for writing 
> emails like this.
> 
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list